[tests] Add P2P interface to TestNode #11182

jnewbery · 2017-08-28T18:20:04Z

Final two steps of #10082 : Adding the "mininode" P2P interface to TestNode

This PR adds the mininode P2P interface to TestNode. It simplifies the process for opening a P2P connection to the node-under-test from this:

node0 = NodeConnCB()
connections = []
connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], node0))
node0.add_connection(connections[0])

to this:

self.nodes[0].add_p2p_connection(p2p_conn_type=NodeConnCB)

The first commit adds the infrastructure to test_node.py. The second updates the individual test cases to use it. Can be separated if this is too much review for one PR.

jimpo

This is so much cleaner. I love the work you have done on this testing infra. Writing my first P2P test was super easy.

jimpo · 2017-08-28T19:14:56Z

test/functional/example_test.py


        # wait_until() will loop until a predicate condition is met. Use it to test properties of the
        # NodeConnCB objects.
-        wait_until(lambda: sorted(blocks) == sorted(list(node2.block_receive_map.keys())), timeout=5, lock=mininode_lock)
+        wait_until(lambda: sorted(blocks) == sorted(list(self.nodes[2].block_receive_map.keys())), timeout=5, lock=mininode_lock)


self.nodes[2].p2p.block_receive_map, right?

yes. Thank you!

jimpo · 2017-08-28T19:19:41Z

test/functional/test_framework/test_node.py

+    def send_message(self, message):
+        """Send a p2p message to the node."""
+        if not self.p2p_ever_connected:
+            self.add_p2p_connections()


Where is add_p2p_connections defined? Is it virtual?

oops. This is vestigial. I previously had an add_p2p_connections(), but it turned out to not be very useful. I'll fix this up. Thanks for pointing this out.

jimpo · 2017-08-28T19:23:04Z

test/functional/test_framework/test_node.py

@@ -141,6 +160,33 @@ def node_encrypt_wallet(self, passphrase):
        self.rpc = None
        self.rpc_connected = False

+    def add_p2p_connection(self, p2p_conn_type, dstaddr='127.0.0.1', dstport=None, services=NODE_NETWORK, send_version=True):


It seems very common to refer to self.nodes[i].p2p. Maybe this method should return the new connection so the client code can assign to a local variable.

Good idea. I'll add that

jnewbery · 2017-08-29T15:08:38Z

@jimpo - thanks for the review. I'm glad you like the new framework. My aim was to make test-writing easier and quicker for contributors, so I appreciate the feedback.

I think I've addressed your review comments.

jnewbery · 2017-09-01T17:01:58Z

rebased

jnewbery · 2017-09-14T14:53:11Z

rebased

jnewbery · 2017-09-20T13:49:46Z

rebased

mess110 · 2017-09-21T21:12:15Z

p2p vs p2ps

I am not 100% sure about the p2p and p2ps magic. What would somebody expect in a case where two p2p connections are made, we disconnect the first peer and call send_message?

When we disconnect a peer should the p2p variable be updated? Maybe p2p should be a method which returns the first connection in the p2ps array.

Imo the behavior is not really predictable and a cleaner way would be to always call p2ps[0] or whatever index is needed, despite less syntax sugar.

import *

Since you modified the files in this patch, do you think it would be a good idea to replace import * with specific imports? (exceptions could be p2p-segwit and p2p-compactblocks since the imports would be quite big).

ACK

I think this patch is really helpful, way cleaner peer connection initialization.

If the above mentioned points are not relevant, ACK 5f92947

jnewbery · 2017-09-22T14:45:26Z

Thanks for the review @mess110 . Responses:

p2p vs p2ps: You're right that this is syntactic sugar, but I think being able to use node.p2p in place of node.p2ps[0] is useful. Most tests that use the p2p interface will only have a single p2p connection to the node, so writing node.p2ps[0] many times is redundant and distracts from the intent of the test. However, your feedback has convinced me to remove some of the syntactic sugar and simplify the implementation:

I've removed the ever_connected latch and automatic connecting on first send_message. Test cases should explicitly create a p2p connection if they wish to use it.
I've turned self.p2p into a property which returns the p2ps[0]
I've removed the redundant connected variable, which should always be equivalent to p2ps is []

wildcard imports: yes, I'd love to clean this up, but I meet resistance whenever I try to do that! I've left it out of this PR so this only focuses on adding the p2p interface.

mess110 · 2017-09-24T19:07:50Z

ACK 0cb3474

TheBlueMatt · 2017-10-18T17:36:05Z

utACK 0cb3474

ryanofsky

utACK 0cb3474. Changes to tests seem good. Growth of TestNode class seems less good, because now it's bigger, more coupled to other parts of framework and importing functionality that many tests won't use. Probably you could make the same test simplifications by improving NodeConn directly (defaulting to '127.0.0.1', combining NodeConn constructor and add_connection method) instead of wrapping it up in a new interface. But this seems like an improvement overall regardless.

ryanofsky · 2017-10-19T15:13:14Z

test/functional/test_framework/test_node.py

+
+    def send_message(self, message):
+        """Send a p2p message to the node."""
+        assert self.p2ps != [], "No p2p connection"


Probably should drop != to be pythonic and consistent with previous assert.

ryanofsky · 2017-10-19T15:20:05Z

test/functional/test_framework/test_node.py

        """Dispatches any unrecognised messages to the RPC connection."""
        assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection"
-        return self.rpc.__getattr__(*args, **kwargs)
+        return self.rpc.__getattr__(name)



This change seems to be unrelated, so maybe revert or move to another commit.

Also, it would be better return getattr(self.rpc, name) here to avoid making assumptions about python rpc class (like that it even has a __getattr__ method), and so any exceptions thrown will be properly reported.

ryanofsky · 2017-10-19T15:24:52Z

test/functional/test_framework/test_node.py

@@ -119,6 +131,7 @@ def stop_node(self):
            self.stop()
        except http.client.CannotSendRequest:
            self.log.exception("Unable to stop node.")
+        self.p2ps = []


Probably better to do del self.p2ps[:] to make sure connections are freed if there are other references to the list. Also would make it clearer that p2ps is an existing member.

ryanofsky · 2017-10-19T15:30:39Z

test/functional/test_framework/test_node.py

@@ -151,6 +164,39 @@ def node_encrypt_wallet(self, passphrase):
        self.encryptwallet(passphrase)
        self.wait_until_stopped()

+    def add_p2p_connection(self, p2p_conn_type, dstaddr='127.0.0.1', dstport=None, services=NODE_NETWORK, send_version=True):


Seem to be missing net argument. Maybe just take **kwargs here to simplify this function, avoid the need to update it if NodeConn gets new options, and avoid repeating default values for services and send_version options that might get out of sync.

ryanofsky · 2017-10-19T15:33:00Z

test/functional/test_framework/test_node.py

+
+    def disconnect_p2p(self, index=0):
+        """Close the p2p connection to the node."""
+        if self.p2ps[index].connection is not None:


Maybe add comment saying why it isn't an error for connection to be none. Saying e.g. when this condition is expected.

ryanofsky · 2017-10-19T15:34:06Z

test/functional/test_framework/test_node.py

+        """Close the p2p connection to the node."""
+        if self.p2ps[index].connection is not None:
+            self.p2ps[index].connection.disconnect_node()
+        self.p2ps.pop(index)


Probably should write del self.p2ps[index] since not using the return value.

ryanofsky · 2017-10-19T15:36:06Z

test/functional/test_framework/test_node.py

+from .authproxy import JSONRPCException
+from .mininode import (
+    NodeConn,
+    NodeConnCB,


Don't see NodeConnCB being used here. NODE_NETWORK import could also go away if using **kwargs as suggested below.

ryanofsky · 2017-10-19T15:37:49Z

test/functional/test_framework/test_node.py

+        assert self.p2ps, "No p2p connection"
+        return self.p2ps[0]
+
+    def send_message(self, message):


Maybe call it send_p2p_message to be clearer and consistent with add_p2p_connection. This class has a lot of functionality already so it would be nice if all the new p2p attributes mentioned p2p.

Another alternative would be to drop this since node.send_p2p_message() hardly seems better than node.p2p.send_message()

ryanofsky · 2017-10-19T15:45:22Z

test/functional/assumevalid.py

-        connections.append(NodeConn('127.0.0.1', p2p_port(2), self.nodes[2], node2))
-        node2.add_connection(connections[2])
-        node2.wait_for_verack()
+        p2p2 =self.nodes[2].add_p2p_connection(p2p_conn_type=BaseNode)


Weird spacing

jnewbery · 2017-10-20T14:00:58Z

Thanks for the review @ryanofsky . I think I've addressed all your comments.

Previous branch: https://github.com/jnewbery/bitcoin/tree/pr11182.2
Rebased on master: https://github.com/jnewbery/bitcoin/tree/pr11182.3
Comments addressed: https://github.com/jnewbery/bitcoin/tree/pr11182.4

Growth of TestNode class seems less good, because now it's bigger, more coupled to other parts of framework and importing functionality that many tests won't use

Can you expand a bit on what you think the concrete problems are? My hope for TestNode is that it can be a unified interface to a node under test with all batteries included. If an individual test doesn't need to use the p2p interface, that's fine - just don't add one. I'd rather that TestNode be more coupled to the mininode implementation than have all the individual test scripts coupled to that implementation, since it makes refactoring mininode and changing the implementation more straightforward (see #11518 for example).

jnewbery · 2017-10-23T13:32:40Z

rebased

promag · 2017-10-30T15:54:28Z

test/functional/test_framework/test_node.py

+    def send_p2p_message(self, message):
+        """Send a p2p message to the node."""
+        assert self.p2ps, "No p2p connection"
+        self.p2ps[0].send_message(message)


self.p2p.send_message and remove assert above?

And why not remove this function? It doesn't save a lot.

maflcko · 2017-11-01T14:00:42Z

Sorry for letting this sit in rebase hell. You might want to wait until #11389 is merged and then ping me after rebase.

ryanofsky

Can you expand a bit on what you think the concrete problems are?

The concrete problems I'm concerned about are:

Code duplicated between mininode & testnode getting out of sync
Confusing and inconsistent test code, with some parts of tests calling mininode functions directly and others calling testnode wrappers around mininode functions. Even if you can avoid this for now it seems likely to happen in the future unless a clear separation is made.
Complicated control flow. A control flow where test code just calls mininode code is more straightforward than a control flow where test code calls testnode, which calls back into test code via a callback, which calls mininode constructors via inheritance, and then returns to testnode, which stores mininode objects in an array, and provides methods for test code to call back into testnode to get access to the array, and then call other mininode functions.

The first problem above was addressed by your change to add_p2p_connection in the last push. The second and third problems will be substantially mitigated if you implement suggestions in my code review comments below.

If you take all my suggestions, the TestNode will only have 3 mininode related members: add_p2p_connection, p2ps, and p2p will which be pretty self contained and straightforward. I would still probably prefer 0 members to 3, and just having a nice standalone helper method to wire up testnode&peernode objects together where necessary. But if you want TestNode to be a big batteries included class that does a collection of marginally related things, then I think you can mostly address my concerns and still have that.

ryanofsky · 2017-11-03T17:35:50Z

test/functional/example_test.py

@@ -180,7 +175,7 @@ def run_test(self):
            block.solve()
            block_message = msg_block(block)
            # Send message is used to send a P2P message to the node over our NodeConn connection
-            node0.send_message(block_message)
+            self.nodes[0].send_message(block_message)


In commit "[tests] Add p2p connection to TestNode"

send_message function is now send_p2p_message and these lines are changing in the next commit. I would move all these example_test.py changes out of here and into the next "use TestNode p2p connection in tests" commit because they would fit better there and avoid being a distraction from the more substantive changes here.

I've left the changes to example_test.py in the first commit, since I think it's good to demonstrate the usage of the new functionality in the same commit as the functionality being added.

However, I have changed these calls to be node.p2p.send_message() as suggested below.

ryanofsky · 2017-11-03T17:47:20Z

test/functional/test_framework/test_node.py

-    Any unrecognised messages will be dispatched to the RPC connection."""
+    To make things easier for the test writer, this class will try to dispatch
+    messages over the correct interface:
+    - send_message is dispatched to the first P2P connection.


In commit "[tests] Add p2p connection to TestNode"

This is now send_p2p_message. But I agree with João that this should be dropped. node.send_p2p_message() is not any more convenient than node.p2p.send_message(), and now tests can wind up with a confusing mix of the two calls. It's also weird that we would arbitrarily provide a wrapper for this one single NodeConnCB method, but not any of the other methods.

ok, I've removed the send_p2p_message() method

ryanofsky · 2017-11-03T17:48:15Z

test/functional/test_framework/test_node.py

@@ -151,6 +160,45 @@ def node_encrypt_wallet(self, passphrase):
        self.encryptwallet(passphrase)
        self.wait_until_stopped()

+    # def add_p2p_connection(self, p2p_conn_type, dstaddr='127.0.0.1', dstport=None, services=NODE_NETWORK, send_version=True):


In commit "[tests] Add p2p connection to TestNode"

Dead code, should delete

ryanofsky · 2017-11-03T18:20:02Z

test/functional/test_framework/test_node.py

+            kwargs['dstport'] = p2p_port(self.index)
+        if 'dstaddr' not in kwargs:
+            kwargs['dstaddr'] = '127.0.0.1'
+        p2p_conn = p2p_conn_type()


In commit "[tests] Add p2p connection to TestNode"

Should drop p2p_conn_type and just take p2p_conn as an argument.

Here there is two much coupling in the other direction (Tests<->TestNode rather than TestNode<->MiniNode), and the control flow is unnecessarily convoluted. Test calls add_p2p_connection, add_p2p_connection calls back into test using p2p_conn_type callback to construct the test's custom NodeConnCB object, wires it up, and then passes it back to test.

Would be cleaner and simpler for tests to construct their own NodeConnCB objects and just pass them here to be wired up.

(I see in #11518 you are using this flow to indirectly pass dstport to the P2PConnection constructor up through the p2p_conn_type callback and inherited contructors. But I think a direct approach would be better there, too: instead of connecting in the P2PConnection constructor, you should just add a P2PConnection connect method and have this code call it.)

Tests should be largely unaffected by this change. Instead of saying add_p2p_connection(CustomHandler) they would say add_p2p_connection(CustomHandler()) which would be better because it will allow them to more easily pass options to their own handlers, and would be more straightforward because it will be obvious just looking at the test code how and when the handler classes that it defines are getting instantiated.

yes, I agree this is better. Changed

ryanofsky · 2017-11-03T18:32:01Z

test/functional/test_framework/test_node.py

@@ -66,7 +66,7 @@ def __init__(self, i, dirname, extra_args, rpchost, timewait, binary, stderr, mo
    def __getattr__(self, *args, **kwargs):


In commit "[tests] fix TestNode.__getattr__() method"

Need to change args/kwargs to name in this commit (it is done in a later commit but broken here).

ryanofsky · 2017-11-03T19:05:55Z

test/functional/test_framework/test_node.py

+        """Close the p2p connection to the node."""
+        # Connection could have already been closed by other end. Calling disconnect_p2p()
+        # on an already disconnected p2p connection is not an error.
+        if self.p2ps[index].connection is not None:


In commit "[tests] Add p2p connection to TestNode"

This is an internal implementation detail of NodeConnCB and you should add a NodeConnCB.disconnect() method to take care of it. This disconnect_p2p method can then be removed and tests call node.p2p.disconnect() instead of node.disconnect_p2p().

If for some tests need to clear the p2ps array (I don't see any that do) they should just do it directly. This would be nicer than the way the two current disconnect_p2p callers are effectively doing del p2ps[0] in a loop to clear the array, for no apparent reason.

I like this as it is. TestNode is responsible for appending new NodeConnCBs to the self.p2ps array. I think it should also be responsible for removing them.

p2p connections can now be added to TestNode instances. This commit also updates the example test to use the new p2p interface in TestNode to demonstrate usage. A future commit will update the existing tests to use p2p through the TestNode.

jnewbery · 2017-11-08T14:41:27Z

rebased on master now that #11389 is merged

maflcko · 2017-11-08T18:01:10Z

utACK 32ae82f

32ae82f [tests] use TestNode p2p connection in tests (John Newbery) 5e5725c [tests] Add p2p connection to TestNode (John Newbery) b86c1cd [tests] fix TestNode.__getattr__() method (John Newbery) Pull request description: Final two steps of #10082 : Adding the "mininode" P2P interface to `TestNode` This PR adds the mininode P2P interface to `TestNode`. It simplifies the process for opening a P2P connection to the node-under-test from this: ```python node0 = NodeConnCB() connections = [] connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], node0)) node0.add_connection(connections[0]) ``` to this: ```python self.nodes[0].add_p2p_connection(p2p_conn_type=NodeConnCB) ``` The first commit adds the infrastructure to `test_node.py`. The second updates the individual test cases to use it. Can be separated if this is too much review for one PR. Tree-SHA512: 44f1a6320f44eefc70489ae8350c6a16ad1a6035e4b9b7bafbdf19f5905ed0e2db85beaaf4758eec3059dd89a375a47a45352a029f39f57a86ab38a9ae66650e

@ryanofsky

e9dfa9b [tests] Move version message sending from NodeConn to NodeConnCB (John Newbery) dad596f [tests] Make NodeConnCB a subclass of NodeConn (John Newbery) e30d404 [tests] Move only: move NodeConnCB below NodeConn (John Newbery) 4d50598 [tests] Tidy up mininode (John Newbery) f2ae6f3 [tests] Remove mininode periodic (half-hour) ping messages (John Newbery) ec59523 [tests] Remove rpc property from TestNode in p2p-segwit.py. (John Newbery) Pull request description: This is the final step in #11518, except for possibly renaming - for motivation, please see that PR. If this is merged, then migrating the test framework from asyncore to asyncio should be easier (I say should because I haven't dug too deeply into what would be required). Requesting review from @ryanofsky , since he always has good feedback on these refactor PRs, and I'd appreciate his take on this refactor. Note particularly that I've reverted the change suggested here: #11182 (comment) . The idea, as always, is to present a simple interface to the test writer. Tree-SHA512: 94dd467a13ec799b101108cf47d4dccb6f6240b601e375e3d785313333bbb389c26072a50759aca663bbf3d6c8b867b99e36ae8800ab8ea115e0496c151926ce

32ae82f [tests] use TestNode p2p connection in tests (John Newbery) 5e5725c [tests] Add p2p connection to TestNode (John Newbery) b86c1cd [tests] fix TestNode.__getattr__() method (John Newbery) Pull request description: Final two steps of bitcoin#10082 : Adding the "mininode" P2P interface to `TestNode` This PR adds the mininode P2P interface to `TestNode`. It simplifies the process for opening a P2P connection to the node-under-test from this: ```python node0 = NodeConnCB() connections = [] connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], node0)) node0.add_connection(connections[0]) ``` to this: ```python self.nodes[0].add_p2p_connection(p2p_conn_type=NodeConnCB) ``` The first commit adds the infrastructure to `test_node.py`. The second updates the individual test cases to use it. Can be separated if this is too much review for one PR. Tree-SHA512: 44f1a6320f44eefc70489ae8350c6a16ad1a6035e4b9b7bafbdf19f5905ed0e2db85beaaf4758eec3059dd89a375a47a45352a029f39f57a86ab38a9ae66650e

jnewbery mentioned this pull request Aug 28, 2017

[tests] Demostration/concept: Add a TestNode class to test_Framework.py #10082

Closed

6 tasks

jimpo reviewed Aug 28, 2017

View reviewed changes

fanquake added the Tests label Aug 28, 2017

jnewbery force-pushed the test_node_p2p branch from ba30027 to f2cde05 Compare August 29, 2017 15:06

jnewbery force-pushed the test_node_p2p branch from f2cde05 to 278a5ef Compare September 1, 2017 17:01

jnewbery force-pushed the test_node_p2p branch from 278a5ef to 0a5e73f Compare September 14, 2017 14:53

jnewbery force-pushed the test_node_p2p branch 2 times, most recently from 18d5849 to 5f92947 Compare September 20, 2017 13:49

jnewbery force-pushed the test_node_p2p branch from 5f92947 to 0cb3474 Compare September 22, 2017 14:38

maflcko added this to the 0.16.0 milestone Oct 3, 2017

jnewbery mentioned this pull request Oct 17, 2017

[tests] [demonstration] Simplify/clarify the NodeConn/NodeConnCB mininode classes #11518

Closed

6 tasks

ryanofsky reviewed Oct 19, 2017

View reviewed changes

jnewbery force-pushed the test_node_p2p branch from 0cb3474 to c220d49 Compare October 20, 2017 13:53

jnewbery force-pushed the test_node_p2p branch from c220d49 to eb5e86d Compare October 23, 2017 13:32

jnewbery force-pushed the test_node_p2p branch from eb5e86d to cb882d0 Compare October 23, 2017 16:46

promag reviewed Oct 30, 2017

View reviewed changes

ryanofsky reviewed Nov 3, 2017

View reviewed changes

jnewbery force-pushed the test_node_p2p branch from cb882d0 to a7f3169 Compare November 6, 2017 21:24

[tests] fix TestNode.__getattr__() method

b86c1cd

jnewbery added 2 commits November 8, 2017 09:04

[tests] Add p2p connection to TestNode

5e5725c

p2p connections can now be added to TestNode instances. This commit also updates the example test to use the new p2p interface in TestNode to demonstrate usage. A future commit will update the existing tests to use p2p through the TestNode.

[tests] use TestNode p2p connection in tests

32ae82f

jnewbery force-pushed the test_node_p2p branch from a7f3169 to 32ae82f Compare November 8, 2017 14:32

maflcko merged commit 32ae82f into bitcoin:master Nov 8, 2017

jnewbery deleted the test_node_p2p branch November 8, 2017 20:20

jnewbery mentioned this pull request Nov 17, 2017

[tests] Split NodeConn from NodeConnCB #11712

Merged

maflcko mentioned this pull request Nov 22, 2017

qa: Add multiwallet prefix test #11743

Merged

robot-dreams mentioned this pull request Sep 1, 2020

test/refactor: reference p2p objects explicitly and remove confusing Test_Node.p2p property #19804

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -66,7 +66,7 @@ def __init__(self, i, dirname, extra_args, rpchost, timewait, binary, stderr, mo
		def __getattr__(self, args, *kwargs):

[tests] Add P2P interface to TestNode #11182

[tests] Add P2P interface to TestNode #11182

Uh oh!

Conversation

jnewbery commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Aug 29, 2017

Uh oh!

jnewbery commented Sep 1, 2017

Uh oh!

jnewbery commented Sep 14, 2017

Uh oh!

jnewbery commented Sep 20, 2017

Uh oh!

mess110 commented Sep 21, 2017

p2p vs p2ps

import *

ACK

Uh oh!

jnewbery commented Sep 22, 2017

Uh oh!

mess110 commented Sep 24, 2017

Uh oh!

TheBlueMatt commented Oct 18, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 20, 2017

Uh oh!

jnewbery commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 1, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jnewbery commented Aug 28, 2017 •

edited

Loading