From fa9064704524a0fd1fa9ea73eea45b07316ac3d1 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 2 Mar 2020 14:14:30 -0500 Subject: [PATCH 1/2] test: Wait for both veracks in add_p2p_connection --- test/functional/p2p_leak.py | 8 +++++++- test/functional/p2p_timeouts.py | 9 ++++++++- test/functional/test_framework/test_node.py | 15 +++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py index 06049db54..2751ae6a5 100755 --- a/test/functional/p2p_leak.py +++ b/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) diff --git a/test/functional/p2p_timeouts.py b/test/functional/p2p_timeouts.py index 02ceec3dc..c1235f8a6 100755 --- a/test/functional/p2p_timeouts.py +++ b/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() diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index c7559ac7c..8260c917f 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -468,7 +468,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 @@ -487,6 +499,7 @@ class TestNode(): p.peer_disconnect() del self.p2ps[:] + class TestNodeCLIAttr: def __init__(self, cli, command): self.cli = cli @@ -498,6 +511,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() @@ -506,6 +520,7 @@ def arg_to_cli(arg): else: return str(arg) + class TestNodeCLI(): """Interface to bitcoin-cli for an individual node""" From faf1d047313e71658fb31f6b94fdd5d37705ab85 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 31 Mar 2020 15:36:46 -0400 Subject: [PATCH 2/2] test: Remove redundant sync_with_ping after add_p2p_connection Also replace the two-line (send_message + sync_with_ping) with the one-line send_and_ping --- test/functional/p2p_dos_header_tree.py | 9 +++------ test/functional/p2p_invalid_messages.py | 1 - 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/test/functional/p2p_dos_header_tree.py b/test/functional/p2p_dos_header_tree.py index 6676b84e5..28dd809ca 100755 --- a/test/functional/p2p_dos_header_tree.py +++ b/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", diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 9876d749f..759f111e6 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/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())