[tests] Split NodeConn from NodeConnCB

jnewbery commented at 9:47 pm on November 17, 2017: member

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 (review) . The idea, as always, is to present a simple interface to the test writer.

fanquake added the label Tests on Nov 17, 2017

in test/functional/p2p-segwit.py:56 in ff3b19c608 outdated

51+    """Send a block to the node and check that it's accepted
52+
53+    - Submit the block over the p2p interface
54+    - use the getbestblockhash rpc to check for acceptance."""
55+    if with_witness:
56+        node.p2p.send_message(msg_witness_block(block))

ryanofsky commented at 7:25 pm on November 21, 2017:

In commit “[tests] Remove rpc property from TestNode in p2p-segwit.py”

Did you mean to s/node.p2p/p2p/ here and below?

in test/functional/p2p-segwit.py:60 in ff3b19c608 outdated

55+    if with_witness:
56+        node.p2p.send_message(msg_witness_block(block))
57+    else:
58+        node.p2p.send_message(msg_block(block))
59+    p2p.sync_with_ping()
60+    assert_equal(node.rpc.getbestblockhash() == block.hash, accepted)

ryanofsky commented at 7:30 pm on November 21, 2017:

In commit “[tests] Remove rpc property from TestNode in p2p-segwit.py”

Do you want to s/node.rpc/node/ here for consistency with test_transaction_acceptance? Another alternative which would also be nice would be to pass these functions just rpc objects instead of nodes.

in test/functional/p2p-segwit.py:1865 in ff3b19c608 outdated

1878+        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[2], True, True)
1879         p2sh_txs[3].wit.vtxinwit[0].scriptWitness.stack = [pad, pad, pad, scripts[3]]
1880-        self.std_node.test_transaction_acceptance(p2sh_txs[3], True, False, b'bad-witness-nonstandard')
1881-        self.test_node.test_transaction_acceptance(p2sh_txs[3], True, True)
1882+        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[3], True, False, b'bad-witness-nonstandard')
1883+        test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[3], True, True)

ryanofsky commented at 7:36 pm on November 21, 2017:

[tests] Remove rpc property from TestNode in p2p-segwit.py

Not a comment on this PR but this whole test file is very repetitive. With a few lambdas / local helper functions it could probably be made a lot shorter and more readable.

in test/functional/test_framework/test_node.py:161 in fb8592236b outdated

157@@ -158,16 +158,20 @@ def node_encrypt_wallet(self, passphrase):
158         self.encryptwallet(passphrase)
159         self.wait_until_stopped()
160 
161-    def add_p2p_connection(self, p2p_conn, **kwargs):
162+    def add_p2p_connection(self, p2p_conn_type, **kwargs):

ryanofsky commented at 7:48 pm on November 21, 2017:

In commit “[tests] instantiate NodeConnCB in TestNode”

Can similary control flow and revert this whole commit, see main comment.

in test/functional/example_test.py:38 in 40876ac740 outdated

34@@ -34,7 +35,7 @@
35 # message is received from the node-under-test. Subclass NodeConnCB and
36 # override the on_*() methods if you need custom behaviour.
37 class BaseNode(NodeConnCB):
38-    def __init__(self):
39+    def __init__(self, dstaddr, dstport, net="regtest", services=NODE_NETWORK, send_version=True):

ryanofsky commented at 9:17 pm on November 21, 2017:

In commit “[tests] Make NodeConnCB a subclass of NodeConn”

Can get rid of all these default parameters repeated so many times. See main comment.

ryanofsky commented at 9:49 pm on November 21, 2017: member

This basically looks good, but I really think it’s important to stop creating peer objects via callback in add_p2p_connection. The change I think you should make is here a683588fea93681f8dae67b1532532ee7b19884a (previously described in #11182 (review)).

Advantages of this change:

Simpler control flow. Tests create their own peer objects. You can see when and where they get constructed and what arguments they are constructed with. No need to pass around callbacks. There’s no advantage I can see to passing around callbacks unless saving two () characters is an advantage.
Less coupling and repetition. TestNode no longer needs need to know anything about peer objects other than that they have connect and disconnect methods. The connect arguments (dstaddr, dstport, net=“regtest”, services=NODE_NETWORK, send_version=True) are defined and used one place instead of repeated 12 times in testnode, mininode, and comptool code, and in individual test case code.
Lets you drop second commit, reduces overall diff from 502 lines to 457 lines.

[tests] Remove rpc property from TestNode in p2p-segwit.py.

Change the helper methods to functions which take a node and a p2p
connection as arguments.

ec59523c59

jnewbery force-pushed on Nov 22, 2017

[tests] Remove mininode periodic (half-hour) ping messages f2ae6f32a6

jnewbery force-pushed on Nov 23, 2017

jnewbery commented at 3:37 pm on November 23, 2017: member

Thanks Russ. I much prefer your suggested way of passing the constructed NodeConnCB object to TestNode. I’ve incorporated your change and split the PR into smaller commits to aid reviewing.

in test/functional/test_framework/test_node.py:188 in 4e8531ff2d outdated

183@@ -185,10 +184,8 @@ def p2p(self):
184     def disconnect_p2ps(self):
185         """Close all p2p connections to the node."""
186         for p in self.p2ps:
187-            # Connection could have already been closed by other end.
188-            if p.connection is not None:
189-                p.connection.disconnect_node()
190-        self.p2ps = []
191+            p.peer_disconnect()
192+        del self.p2ps[:]

MarcoFalke commented at 3:50 pm on November 27, 2017:

stylte-nit: Any reason for this change? I think the previous is easier to read python code.

jnewbery commented at 5:45 pm on November 27, 2017:

There’s some discussion of the differences here: https://stackoverflow.com/questions/850795/different-ways-of-clearing-lists

If there was a different reference to this array, then self.p2ps = [] wouldn’t clear that array, it would just change self.p2ps to point to a new, empty array.

Practically, there’s not much difference, but I think I prefer del self.p2ps[:]. I actually got this from @ryanofsky’s commit - perhaps he had a specific reason to make this change?

MarcoFalke commented at 8:08 pm on November 27, 2017:

Ok, fine to keep, since it is ever-so-slightly safer and already used by this syntax in stop_node

ryanofsky commented at 8:19 pm on November 27, 2017:

Main reason for suggesting this is that having multiple instances of the same list can make things confusing when you are debugging.

MarcoFalke commented at 3:58 pm on November 27, 2017: member

Concept ACK a328ab35888baaa481b5b59e41472f91e8802f8e. Going to review this week or so.

MarcoFalke assigned MarcoFalke on Nov 27, 2017

in test/functional/test_framework/mininode.py:132 in e5e64d5843 outdated

263@@ -262,12 +264,18 @@ def readable(self):
264         return True
265 
266     def handle_read(self):
267+        """asyncore callback when data is read from the socket."""

ryanofsky commented at 10:35 pm on November 27, 2017:

In commit “[tests] Tidy up mininode”

Think you missed the readble() function above. Would be good to note that is an asyncore callback like writeable().

jnewbery commented at 5:14 pm on November 28, 2017:

We can actually just remove the readable() override entirely. From https://docs.python.org/3.6/library/asyncore.html#asyncore.dispatcher.readable : “The default method simply returns True, indicating that by default, all channels will be interested in read events.” so this override isn’t actually doing anything.

I’ll update the commit.

in test/functional/test_framework/mininode.py:273 in e5e64d5843 outdated

270             self.recvbuf += t
271-            self.got_data()
272+            self.read_message()
273 
274-    def got_data(self):
275+    def read_message(self):

ryanofsky commented at 10:42 pm on November 27, 2017:

In commit “[tests] Tidy up mininode”

Seems like this method should begin with an underscore (like _log_message below), since this is part of the read handler, and not something that should be called externally. I also think a name like _got_data _on_data _handle_data _process_messages might be more descriptive since read_message suggests a method that blocks waiting for a message as opposed to a method handling incoming data.

jnewbery commented at 5:29 pm on November 28, 2017:

Agree. Changed to _on_data.

in test/functional/test_framework/mininode.py:274 in e5e64d5843 outdated

271-            self.got_data()
272+            self.read_message()
273 
274-    def got_data(self):
275+    def read_message(self):
276+        """Try to read a P2P message from the recv buffer.

ryanofsky commented at 10:44 pm on November 27, 2017:

In commit “[tests] Tidy up mininode”

Would change “a message” to “messages” everywhere since this can parse more than just one message.

jnewbery commented at 5:30 pm on November 28, 2017:

Yes. Fixed.

in test/functional/test_framework/mininode.py:261 in 4e8531ff2d outdated

277+
278+    TODO: rename this class P2PInterface"""
279+    def peer_connect(self, *args, **kwargs):
280+        super().peer_connect(*args, **kwargs)
281 
282         # Track number of messages of each type received and the most recent

ryanofsky commented at 11:06 pm on November 27, 2017:

In commit “[tests] Make NodeConnCB a subclass of NodeConn”

Maybe keep assigning nServices/messages_count/last_message/ping_counter members that don’t depend on connection information in __init__ rather than peer_connect. This would mean you don’t need to introduce a NodeConnCB.peer_connect method in this commit, and in the next commit it can be a shorter, more focused override. Just a suggestion, though, difference is minor.

jnewbery commented at 5:47 pm on November 28, 2017:

Agree. Makes more sense. Thanks.

ryanofsky commented at 11:08 pm on November 27, 2017: member

utACK a328ab35888baaa481b5b59e41472f91e8802f8e

[tests] Tidy up mininode

Add docstrings and renames some methods.
Also removes the redundant NodeConn.readable() method override.

4d50598569

[tests] Move only: move NodeConnCB below NodeConn

This is required since NodeConnCB will inherit from NodeConn
after the next commit.

e30d404385

[tests] Make NodeConnCB a subclass of NodeConn

This makes NodeConnCB a subclass of NodeConn, and
removes the need for the client code to know
anything about the implementation details of NodeConnCB.

NodeConn can now be swapped out for any other implementation
of a low-level connection without changing client code.

dad596fc37

[tests] Move version message sending from NodeConn to NodeConnCB

This commit moves the logic that sends a version message
on connection from NodeConn to NodeConnCB. NodeConn should
not be aware of the semantics or meaning of the P2P payloads.

e9dfa9bccc

jnewbery force-pushed on Nov 28, 2017

jnewbery commented at 5:48 pm on November 28, 2017: member

Addressed @ryanofsky’s comments

ryanofsky commented at 6:00 pm on November 28, 2017: member

utACK e9dfa9bccc5cbb6096c60498651b451297f0a931. Only changes since last review were the various things commented above.

in test/functional/test_framework/test_node.py:160 in dad596fc37 outdated

156@@ -158,7 +157,7 @@ def node_encrypt_wallet(self, passphrase):
157         self.encryptwallet(passphrase)
158         self.wait_until_stopped()
159 
160-    def add_p2p_connection(self, p2p_conn, **kwargs):
161+    def add_p2p_connection(self, p2p_conn, *args, **kwargs):

MarcoFalke commented at 7:14 pm on November 29, 2017:

nit: I’d rather do the opposite and disallow passing through unnamed args here. Forcing code to be explicit should be safer and ease future review, imo.

0def add_p2p_connection(self, p2p_conn, *, **kwargs)

ryanofsky commented at 8:37 pm on November 29, 2017:

If the goal is to disallow unnamed arguments, this place to do is where the arguments are actually declared (in the peer_connect method). If you make that change here instead of there, you will be introducing an inconsistency between add_p2p_connection and peer_connect. It is better for testnode to just forward arguments on to peernode, instead making unnecessary assumptions about how it works internally and creating unnecessary differences. Forwarding *args and **kwargs is also idiomatic in python while forwarding just **kwargs is more unusual.

jnewbery commented at 9:03 pm on November 29, 2017:

I think @ryanofsky is right - the right place for this is in NodeConnCB.peer_connect(). Can we leave that for a future PR?

MarcoFalke commented at 9:25 pm on November 29, 2017:

@jnewbery Sure, future PR sounds good.

MarcoFalke commented at 7:17 pm on November 29, 2017: member

utACK e9dfa9bccc5cbb6096c60498651b451297f0a931

MarcoFalke merged this on Nov 29, 2017

MarcoFalke closed this on Nov 29, 2017

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

jnewbery deleted the branch on Nov 29, 2017

UdjinM6 referenced this in commit 52bf616cc4 on Apr 5, 2020

ckti referenced this in commit 2b5d05d944 on Mar 28, 2021

gades referenced this in commit 6a35ce1423 on Jun 30, 2021

MarcoFalke locked this on Dec 16, 2021

[tests] Split NodeConn from NodeConnCB #11712