[tests] Add P2P interface to TestNode

jnewbery commented at 6:20 pm on August 28, 2017: member

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:

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

to this:

0self.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.

in test/functional/example_test.py:205 in ba30027a70 outdated

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

jimpo commented at 7:14 pm on August 28, 2017:

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

jnewbery commented at 10:03 pm on August 28, 2017:

yes. Thank you!

in test/functional/test_framework/test_node.py:178 in ba30027a70 outdated

173+        self.p2p_ever_connected = True
174+
175+    def send_message(self, message):
176+        """Send a p2p message to the node."""
177+        if not self.p2p_ever_connected:
178+            self.add_p2p_connections()

jimpo commented at 7:19 pm on August 28, 2017:

Where is add_p2p_connections defined? Is it virtual?

jnewbery commented at 10:04 pm on August 28, 2017:

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.

in test/functional/test_framework/test_node.py:167 in c3730eff46 outdated

159@@ -141,6 +160,33 @@ def node_encrypt_wallet(self, passphrase):
160         self.rpc = None
161         self.rpc_connected = False
162 
163+    def add_p2p_connection(self, p2p_conn_type, dstaddr='127.0.0.1', dstport=None, services=NODE_NETWORK, send_version=True):

jimpo commented at 7:23 pm on August 28, 2017:

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.

jnewbery commented at 10:03 pm on August 28, 2017:

Good idea. I’ll add that

jimpo commented at 7:24 pm on August 28, 2017: contributor

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

fanquake added the label Tests on Aug 28, 2017

jnewbery force-pushed on Aug 29, 2017

jnewbery commented at 3:08 pm on August 29, 2017: member

@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 force-pushed on Sep 1, 2017

jnewbery commented at 5:01 pm on September 1, 2017: member

rebased

jnewbery force-pushed on Sep 14, 2017

jnewbery commented at 2:53 pm on September 14, 2017: member

rebased

jnewbery force-pushed on Sep 14, 2017

jnewbery force-pushed on Sep 20, 2017

jnewbery commented at 1:49 pm on September 20, 2017: member

rebased

mess110 commented at 9:12 pm on September 21, 2017: contributor

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 https://github.com/bitcoin/bitcoin/pull/11182/commits/5f9294759df348eca9a6361f7e98360fc07cd3f4

jnewbery force-pushed on Sep 22, 2017

jnewbery commented at 2:45 pm on September 22, 2017: member

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 commented at 7:07 pm on September 24, 2017: contributor

ACK https://github.com/bitcoin/bitcoin/pull/11182/commits/0cb3474b664fc04a23640614e2e8b0deae6c2d50

MarcoFalke added this to the milestone 0.16.0 on Oct 3, 2017

TheBlueMatt commented at 5:36 pm on October 18, 2017: member

utACK 0cb3474b664fc04a23640614e2e8b0deae6c2d50

in test/functional/test_framework/test_node.py:191 in 2d5ca2e645 outdated

186+        assert self.p2ps, "No p2p connection"
187+        return self.p2ps[0]
188+
189+    def send_message(self, message):
190+        """Send a p2p message to the node."""
191+        assert self.p2ps != [], "No p2p connection"

ryanofsky commented at 3:13 pm on October 19, 2017:

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

in test/functional/test_framework/test_node.py:76 in 2d5ca2e645 outdated

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

ryanofsky commented at 3:20 pm on October 19, 2017:

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.

in test/functional/test_framework/test_node.py:134 in 2d5ca2e645 outdated

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

ryanofsky commented at 3:24 pm on October 19, 2017:

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.

in test/functional/test_framework/test_node.py:167 in 2d5ca2e645 outdated

163@@ -151,6 +164,39 @@ def node_encrypt_wallet(self, passphrase):
164         self.encryptwallet(passphrase)
165         self.wait_until_stopped()
166 
167+    def add_p2p_connection(self, p2p_conn_type, dstaddr='127.0.0.1', dstport=None, services=NODE_NETWORK, send_version=True):

ryanofsky commented at 3:30 pm on October 19, 2017:

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.

in test/functional/test_framework/test_node.py:189 in 2d5ca2e645 outdated

191+        assert self.p2ps != [], "No p2p connection"
192+        self.p2ps[0].send_message(message)
193+
194+    def disconnect_p2p(self, index=0):
195+        """Close the p2p connection to the node."""
196+        if self.p2ps[index].connection is not None:

ryanofsky commented at 3:33 pm on October 19, 2017:

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

in test/functional/test_framework/test_node.py:198 in 2d5ca2e645 outdated

193+
194+    def disconnect_p2p(self, index=0):
195+        """Close the p2p connection to the node."""
196+        if self.p2ps[index].connection is not None:
197+            self.p2ps[index].connection.disconnect_node()
198+        self.p2ps.pop(index)

ryanofsky commented at 3:34 pm on October 19, 2017:

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

in test/functional/test_framework/test_node.py:19 in 2d5ca2e645 outdated

12@@ -13,13 +13,19 @@
13 import subprocess
14 import time
15 
16+from .authproxy import JSONRPCException
17+from .mininode import (
18+    NodeConn,
19+    NodeConnCB,

ryanofsky commented at 3:36 pm on October 19, 2017:

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

in test/functional/test_framework/test_node.py:189 in 2d5ca2e645 outdated

184+        Convenience property - most tests only use a single p2p connection to each
185+        node, so this saves having to write node.p2ps[0] many times."""
186+        assert self.p2ps, "No p2p connection"
187+        return self.p2ps[0]
188+
189+    def send_message(self, message):

ryanofsky commented at 3:37 pm on October 19, 2017:

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()

in test/functional/assumevalid.py:168 in 0cb3474b66 outdated

171         self.start_node(2, extra_args=["-assumevalid=" + hex(block102.sha256)])
172-        node2 = BaseNode()  # connects to node2
173-        connections.append(NodeConn('127.0.0.1', p2p_port(2), self.nodes[2], node2))
174-        node2.add_connection(connections[2])
175-        node2.wait_for_verack()
176+        p2p2 =self.nodes[2].add_p2p_connection(p2p_conn_type=BaseNode)

ryanofsky commented at 3:45 pm on October 19, 2017:

Weird spacing

ryanofsky commented at 4:01 pm on October 19, 2017: member

utACK 0cb3474b664fc04a23640614e2e8b0deae6c2d50. 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.

jnewbery force-pushed on Oct 20, 2017

jnewbery commented at 2:00 pm on October 20, 2017: member

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 force-pushed on Oct 23, 2017

jnewbery commented at 1:32 pm on October 23, 2017: member

rebased

jnewbery force-pushed on Oct 23, 2017

in test/functional/test_framework/test_node.py:192 in cb882d0d4c outdated

187+        return self.p2ps[0]
188+
189+    def send_p2p_message(self, message):
190+        """Send a p2p message to the node."""
191+        assert self.p2ps, "No p2p connection"
192+        self.p2ps[0].send_message(message)

promag commented at 3:54 pm on October 30, 2017:

self.p2p.send_message and remove assert above?

promag commented at 3:58 pm on October 30, 2017:

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

MarcoFalke commented at 2:00 pm on November 1, 2017: member

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

in test/functional/example_test.py:178 in 9054fc17e6 outdated

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

ryanofsky commented at 5:35 pm on November 3, 2017:

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.

jnewbery commented at 6:33 pm on November 6, 2017:

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.

in test/functional/test_framework/test_node.py:41 in 9054fc17e6 outdated

38 
39-    To make things easier for the test writer, a bit of magic is happening under the covers.
40-    Any unrecognised messages will be dispatched to the RPC connection."""
41+    To make things easier for the test writer, this class will try to dispatch
42+    messages over the correct interface:
43+    - send_message is dispatched to the first P2P connection.

ryanofsky commented at 5:47 pm on November 3, 2017:

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.

jnewbery commented at 6:28 pm on November 6, 2017:

ok, I’ve removed the send_p2p_message() method

in test/functional/test_framework/test_node.py:163 in 9054fc17e6 outdated

159@@ -151,6 +160,45 @@ def node_encrypt_wallet(self, passphrase):
160         self.encryptwallet(passphrase)
161         self.wait_until_stopped()
162 
163+    # def add_p2p_connection(self, p2p_conn_type, dstaddr='127.0.0.1', dstport=None, services=NODE_NETWORK, send_version=True):

ryanofsky commented at 5:48 pm on November 3, 2017:

In commit “[tests] Add p2p connection to TestNode”

Dead code, should delete

jnewbery commented at 6:33 pm on November 6, 2017:

done

in test/functional/test_framework/test_node.py:173 in 9054fc17e6 outdated

168+        returns the connection to the caller."""
169+        if 'dstport' not in kwargs:
170+            kwargs['dstport'] = p2p_port(self.index)
171+        if 'dstaddr' not in kwargs:
172+            kwargs['dstaddr'] = '127.0.0.1'
173+        p2p_conn = p2p_conn_type()

ryanofsky commented at 6:20 pm on November 3, 2017:

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.

jnewbery commented at 7:42 pm on November 6, 2017:

yes, I agree this is better. Changed

in test/functional/test_framework/test_node.py:66 in efbe3de977 outdated

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

ryanofsky commented at 6:32 pm on November 3, 2017:

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).

jnewbery commented at 6:28 pm on November 6, 2017:

done

in test/functional/test_framework/test_node.py:189 in cb882d0d4c outdated

193+
194+    def disconnect_p2p(self, index=0):
195+        """Close the p2p connection to the node."""
196+        # Connection could have already been closed by other end. Calling disconnect_p2p()
197+        # on an already disconnected p2p connection is not an error.
198+        if self.p2ps[index].connection is not None:

ryanofsky commented at 7:05 pm on November 3, 2017:

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.

jnewbery commented at 8:54 pm on November 6, 2017:

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.

ryanofsky commented at 7:29 pm on November 3, 2017: member

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.

jnewbery force-pushed on Nov 6, 2017

[tests] fix TestNode.__getattr__() method b86c1cd208

[tests] Add p2p connection to TestNode

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.

5e5725cc2b

[tests] use TestNode p2p connection in tests 32ae82f5c3

jnewbery force-pushed on Nov 8, 2017

jnewbery commented at 2:41 pm on November 8, 2017: member

rebased on master now that #11389 is merged

MarcoFalke commented at 6:01 pm on November 8, 2017: member

utACK 32ae82f5c3d51a66ad4deb84819809b890664245

MarcoFalke merged this on Nov 8, 2017

MarcoFalke closed this on Nov 8, 2017

MarcoFalke referenced this in commit f7388e93d3 on Nov 8, 2017

jnewbery deleted the branch on Nov 8, 2017

MarcoFalke referenced this in commit 9f2c2dba21 on Nov 29, 2017

codablock referenced this in commit 65487ade38 on Sep 30, 2019

codablock referenced this in commit 0f17abaa29 on Sep 30, 2019

codablock referenced this in commit ec1d2b6722 on Oct 2, 2019

codablock referenced this in commit 7b9dd03e27 on Oct 2, 2019

UdjinM6 referenced this in commit c9a8bac497 on Oct 3, 2019

codablock referenced this in commit 7c163b1ffd on Oct 3, 2019

barrystyle referenced this in commit fca03b56ee on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

[tests] Add P2P interface to TestNode #11182

p2p vs p2ps

import *

ACK