Merge bitcoin#28782: test: Add missing sync on send_version in peer_connect

fanquake · fanquake · commit 1162d046ecf2 · 2023-11-08T09:55:53.000Z
fa02598 test: Add missing sync on send_version in peer_connect (MarcoFalke) Pull request description: Without the sync, the logic will be racy. For example, `p2p_sendtxrcncl.py` is failing locally (and on CI occasionally), because non-version messages will be sent before the version message: ```py self.log.info('SENDTXRCNCL with version=0 triggers a disconnect') sendtxrcncl_low_version = create_sendtxrcncl_msg() sendtxrcncl_low_version.version = 0 peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]): peer.send_message(sendtxrcncl_low_version) peer.wait_for_disconnect() ``` ``` test 2023-11-02T08:15:19.620000Z TestFramework (INFO): SENDTXRCNCL with version=0 triggers a disconnect test 2023-11-02T08:15:19.621000Z TestFramework.p2p (DEBUG): Connecting to Bitcoin Node: 127.0.0.1:11312 test 2023-11-02T08:15:19.624000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:11312 test 2023-11-02T08:15:19.798000Z TestFramework.p2p (DEBUG): Send message to 127.0.0.1:11312: msg_sendtxrcncl(version=0, salt=2) test 2023-11-02T08:15:19.799000Z TestFramework.p2p (DEBUG): Send message to 127.0.0.1:11312: msg_version(nVersion=70016 nServices=9 nTime=Thu Nov 2 08:15:19 2023 addrTo=CAddress(nServices=1 net=IPv4 addr=127.0.0.1 port=11312) addrFrom=CAddress(nServices=1 net=IPv4 addr=0.0.0.0 port=0) nNonce=0x369AC031CDA96022 strSubVer=/python-p2p-tester:0.0.3/ nStartingHeight=-1 relay=1) node0 2023-11-02T08:15:19.804409Z [net] [net.cpp:3676] [CNode] [net] Added connection peer=0 node0 2023-11-02T08:15:19.805256Z [net] [net.cpp:1825] [CreateNodeFromAcceptedSocket] [net] connection from 127.0.0.1:55964 accepted node0 2023-11-02T08:15:19.809861Z [msghand] [net_processing.cpp:3356] [ProcessMessage] [net] received: sendtxrcncl (12 bytes) peer=0 node0 2023-11-02T08:15:19.810297Z [msghand] [net_processing.cpp:3582] [ProcessMessage] [net] non-version message before version handshake. Message "sendtxrcncl" from peer=0 node0 2023-11-02T08:15:19.810928Z [msghand] [net_processing.cpp:3356] [ProcessMessage] [net] received: version (111 bytes) peer=0 ... test 2023-11-02T09:35:20.166000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: '''' def test_function(): if check_connected: assert self.is_connected return test_function_in() ''' test 2023-11-02T09:35:20.187000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main self.run_test() File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/p2p_sendtxrcncl.py", line 188, in run_test peer.wait_for_disconnect() File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/p2p.py", line 478, in wait_for_disconnect self.wait_until(test_function, timeout=timeout, check_connected=False) File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/p2p.py", line 470, in wait_until wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor) File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/util.py", line 275, in wait_until_helper_internal raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout)) AssertionError: Predicate '''' def test_function(): if check_connected: assert self.is_connected return test_function_in() ''' not true after 4800.0 seconds ACKs for top commit: mzumsande: ACK fa02598 Tree-SHA512: 78871f603d387e2df8c0acbdfa95441fa186f80e94593021bb219bbf1bc9dc7efc4e266bd254b5cc41114c38227ff3b7f6172335d9bb828427f0a2acffde752d
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
@@ -364,8 +364,8 @@ def peer_connect_send_version(self, services):
         vt.addrFrom.port = 0
         self.on_connection_send_msg = vt  # Will be sent in connection_made callback
 
-    def peer_connect(self, *args, services=P2P_SERVICES, send_version=True, **kwargs):
-        create_conn = super().peer_connect(*args, **kwargs)
+    def peer_connect(self, *, services=P2P_SERVICES, send_version, **kwargs):
+        create_conn = super().peer_connect(**kwargs)
 
         if send_version:
             self.peer_connect_send_version(services)
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
@@ -634,7 +634,7 @@ def assert_start_raises_init_error(self, extra_args=None, expected_msg=None, mat
                     assert_msg += "with expected error " + expected_msg
                 self._raise_assertion_error(assert_msg)
 
-    def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
+    def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=True, **kwargs):
         """Add an inbound p2p connection to the node.
 
         This method adds the p2p connection to the self.p2ps list and also
@@ -645,9 +645,11 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
             kwargs['dstaddr'] = '127.0.0.1'
 
         p2p_conn.p2p_connected_to_node = True
-        p2p_conn.peer_connect(**kwargs, net=self.chain, timeout_factor=self.timeout_factor)()
+        p2p_conn.peer_connect(**kwargs, send_version=send_version, net=self.chain, timeout_factor=self.timeout_factor)()
         self.p2ps.append(p2p_conn)
         p2p_conn.wait_until(lambda: p2p_conn.is_connected, check_connected=False)
+        if send_version:
+            p2p_conn.wait_until(lambda: not p2p_conn.on_connection_send_msg)
         if wait_for_verack:
             # Wait for the node to send us the version and verack
             p2p_conn.wait_for_verack()
