Merge 67a72466e1 into merged_master (Bitcoin PR #18247)

test/functional/p2p_dos_header_tree.py

@@ -47,8 +47,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework):
 
         self.log.info("Feed all non-fork headers, including and up to the first checkpoint")
         self.nodes[0].add_p2p_connection(P2PInterface())
-        self.nodes[0].p2p.send_message(msg_headers(self.headers))
-        self.nodes[0].p2p.sync_with_ping()
+        self.nodes[0].p2p.send_and_ping(msg_headers(self.headers))
         assert {
             'height': 546,
             'hash': '000000002a936ca763904c3c35fce2f3556c559c0214345d31b1bcebf76acb70',
@@ -65,8 +64,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework):
         # On node 0 it succeeds because checkpoints are disabled
         self.restart_node(0, extra_args=['-nocheckpoints'])
         self.nodes[0].add_p2p_connection(P2PInterface())
-        self.nodes[0].p2p.send_message(msg_headers(self.headers_fork))
-        self.nodes[0].p2p.sync_with_ping()
+        self.nodes[0].p2p.send_and_ping(msg_headers(self.headers_fork))
         assert {
             "height": 2,
             "hash": "00000000b0494bd6c3d5ff79c497cfce40831871cbf39b1bc28bd1dac817dc39",
@@ -76,8 +74,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework):
 
         # On node 1 it succeeds because no checkpoint has been reached yet by a chain tip
         self.nodes[1].add_p2p_connection(P2PInterface())
-        self.nodes[1].p2p.send_message(msg_headers(self.headers_fork))
-        self.nodes[1].p2p.sync_with_ping()
+        self.nodes[1].p2p.send_and_ping(msg_headers(self.headers_fork))
         assert {
             "height": 2,
             "hash": "00000000b0494bd6c3d5ff79c497cfce40831871cbf39b1bc28bd1dac817dc39",

test/functional/p2p_invalid_messages.py

@@ -140,7 +140,6 @@ class InvalidMessagesTest(BitcoinTestFramework):
 
         # Node is still up.
         conn = node.add_p2p_connection(P2PDataStore())
-        conn.sync_with_ping()
 
     def test_magic_bytes(self):
         conn = self.nodes[0].add_p2p_connection(P2PDataStore())

test/functional/p2p_leak.py

@@ -19,6 +19,7 @@ from test_framework.util import wait_until
 
 banscore = 10
 
+
 class CLazyNode(P2PInterface):
     def __init__(self):
         super().__init__()
@@ -88,6 +89,7 @@ class CNodeNoVerackIdle(CLazyNode):
         self.send_message(msg_ping())
         self.send_message(msg_getaddr())
 
+
 class P2PLeakTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
@@ -96,7 +98,11 @@ class P2PLeakTest(BitcoinTestFramework):
     def run_test(self):
         no_version_bannode = self.nodes[0].add_p2p_connection(CNodeNoVersionBan(), send_version=False, wait_for_verack=False)
         no_version_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVersionIdle(), send_version=False, wait_for_verack=False)
-        no_verack_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVerackIdle())
+        no_verack_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVerackIdle(), wait_for_verack=False)
+
+        # Wait until we got the verack in response to the version. Though, don't wait for the other node to receive the
+        # verack, since we never sent one
+        no_verack_idlenode.wait_for_verack()
 
         wait_until(lambda: no_version_bannode.ever_connected, timeout=10, lock=mininode_lock)
         wait_until(lambda: no_version_idlenode.ever_connected, timeout=10, lock=mininode_lock)

test/functional/p2p_timeouts.py

@@ -27,11 +27,13 @@ from test_framework.messages import msg_ping
 from test_framework.mininode import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 
+
 class TestP2PConn(P2PInterface):
     def on_version(self, message):
         # Don't send a verack in response
         pass
 
+
 class TimeoutsTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
@@ -41,10 +43,14 @@ class TimeoutsTest(BitcoinTestFramework):
 
     def run_test(self):
         # Setup the p2p connections
-        no_verack_node = self.nodes[0].add_p2p_connection(TestP2PConn())
+        no_verack_node = self.nodes[0].add_p2p_connection(TestP2PConn(), wait_for_verack=False)
         no_version_node = self.nodes[0].add_p2p_connection(TestP2PConn(), send_version=False, wait_for_verack=False)
         no_send_node = self.nodes[0].add_p2p_connection(TestP2PConn(), send_version=False, wait_for_verack=False)
 
+        # Wait until we got the verack in response to the version. Though, don't wait for the other node to receive the
+        # verack, since we never sent one
+        no_verack_node.wait_for_verack()
+
         sleep(1)
 
         assert no_verack_node.is_connected
@@ -81,5 +87,6 @@ class TimeoutsTest(BitcoinTestFramework):
             assert not no_version_node.is_connected
             assert not no_send_node.is_connected
 
+
 if __name__ == '__main__':
     TimeoutsTest().main()

test/functional/test_framework/test_node.py

@@ -484,7 +484,19 @@ class TestNode():
         p2p_conn.peer_connect(**kwargs, net=self.chain)()
         self.p2ps.append(p2p_conn)
         if wait_for_verack:
+            # Wait for the node to send us the version and verack
             p2p_conn.wait_for_verack()
+            # At this point we have sent our version message and received the version and verack, however the full node
+            # has not yet received the verack from us (in reply to their version). So, the connection is not yet fully
+            # established (fSuccessfullyConnected).
+            #
+            # This shouldn't lead to any issues when sending messages, since the verack will be in-flight before the
+            # message we send. However, it might lead to races where we are expecting to receive a message. E.g. a
+            # transaction that will be added to the mempool as soon as we return here.
+            #
+            # So syncing here is redundant when we only want to send a message, but the cost is low (a few milliseconds)
+            # in comparision to the upside of making tests less fragile and unexpected intermittent errors less likely.
+            p2p_conn.sync_with_ping()
 
         return p2p_conn
 
@@ -503,6 +515,7 @@ class TestNode():
             p.peer_disconnect()
         del self.p2ps[:]
 
+
 class TestNodeCLIAttr:
     def __init__(self, cli, command):
         self.cli = cli
@@ -514,6 +527,7 @@ class TestNodeCLIAttr:
     def get_request(self, *args, **kwargs):
         return lambda: self(*args, **kwargs)
 
+
 def arg_to_cli(arg):
     if isinstance(arg, bool):
         return str(arg).lower()
@@ -522,6 +536,7 @@ def arg_to_cli(arg):
     else:
         return str(arg)
 
+
 class TestNodeCLI():
     """Interface to bitcoin-cli for an individual node"""