From faa404e119c54526077b056f6f380782e64986c4 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 30 Jan 2026 11:18:17 +0100 Subject: [PATCH 1/4] test: Add is_connected_to helper Needed in the next commit. Co-Authored-By: David Gumberg --- test/functional/test_framework/test_node.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 541c758b5ec..0cd6dd1e840 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -914,6 +914,11 @@ class TestNode(): self.wait_until(lambda: self.num_test_p2p_connections() == 0) + def is_connected_to(self, other): + assert isinstance(other, TestNode) + other_subver = other.getnetworkinfo()["subversion"] + return any(peer["subver"] == other_subver for peer in self.getpeerinfo()) + def bumpmocktime(self, seconds): """Fast forward using setmocktime to self.mocktime + seconds. Requires setmocktime to have been called at some point in the past.""" From fa21edddb2722f7188130e56b06985ee2ec3ec1c Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 27 Mar 2026 16:09:16 +0100 Subject: [PATCH 2/4] test: Stricter checks in rpc_setban.py Make the checks stricter and easier to follow: * Fix a typo. * After the first ban from node 1 wait until node 0 "sees" the ban. * Move the restart_node out of the debug log context, to avoid bloat. * Removed the timeout from the outer/lower exit stack to check "dropped (banned)\n" on node 1, because the inner/top exit stack waits longer. * The inner/top exit stack checks for the both disconnections peer=2 and possibly peer=3 (for v2->v1 retry). * And finally, add a redundant assert to confirm once more that node 0 is has "seen" the ban. --- test/functional/rpc_setban.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/test/functional/rpc_setban.py b/test/functional/rpc_setban.py index 9b684ae4574..7d1873c796a 100755 --- a/test/functional/rpc_setban.py +++ b/test/functional/rpc_setban.py @@ -26,19 +26,28 @@ class SetBanTests(BitcoinTestFramework): peerinfo = self.nodes[1].getpeerinfo()[0] assert "noban" not in peerinfo["permissions"] - # Node 0 get banned by Node 1 + # Node 0 gets banned by Node 1 self.nodes[1].setban("127.0.0.1", "add") + self.wait_until(lambda: not self.nodes[0].is_connected_to(self.nodes[1])) # Node 0 should not be able to reconnect + self.restart_node(1, []) context = ExitStack() - context.enter_context(self.nodes[1].assert_debug_log(expected_msgs=['dropped (banned)\n'], timeout=50)) + context.enter_context(self.nodes[1].assert_debug_log(expected_msgs=["dropped (banned)\n"])) # When disconnected right after connecting, a v2 node will attempt to reconnect with v1. - # Wait for that to happen so that it cannot mess with later tests. - if self.options.v2transport: - context.enter_context(self.nodes[0].assert_debug_log(expected_msgs=['trying v1 connection'], timeout=50)) + # Wait for all disconnects on node0, so that it cannot mess with later tests. + context.enter_context(self.nodes[0].assert_debug_log( + expected_msgs=[ + "retrying with v1 transport protocol for peer=2", + "Cleared nodestate for peer=2", + "Cleared nodestate for peer=3", + ] if self.options.v2transport else [ + "Cleared nodestate for peer=2", # Just one v1 disconnect to wait for + ], + timeout=8)) with context: - self.restart_node(1, []) self.nodes[0].addnode("127.0.0.1:" + str(p2p_port(1)), "onetry") + assert not self.nodes[0].is_connected_to(self.nodes[1]) # However, node 0 should be able to reconnect if it has noban permission self.restart_node(1, ['-whitelist=127.0.0.1']) From fab27726478d686dcd66d26d3b7b34aa323b8066 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 27 Jan 2026 22:50:43 +0100 Subject: [PATCH 3/4] test: Fix all races after a socket is closed gracefully This waits for any disconnect (e.g. from a restart of one of the nodes) to fully happen before the next connect. Can be reviewed with the git option: --color-moved=dimmed-zebra --- .../test_framework/test_framework.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index b809e68f4df..229ff893389 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -559,6 +559,17 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): """ from_connection = self.nodes[a] to_connection = self.nodes[b] + + # Use subversion as peer id. Test nodes have their node number appended to the user agent string + from_connection_subver = from_connection.getnetworkinfo()['subversion'] + to_connection_subver = to_connection.getnetworkinfo()['subversion'] + + def find_conn(node, peer_subversion, inbound): + return next(filter(lambda peer: peer['subver'] == peer_subversion and peer['inbound'] == inbound, node.getpeerinfo()), None) + + self.wait_until(lambda: not find_conn(from_connection, to_connection_subver, inbound=False)) + self.wait_until(lambda: not find_conn(to_connection, from_connection_subver, inbound=True)) + ip_port = "127.0.0.1:" + str(p2p_port(b)) if peer_advertises_v2 is None: @@ -574,13 +585,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): if not wait_for_connect: return - # Use subversion as peer id. Test nodes have their node number appended to the user agent string - from_connection_subver = from_connection.getnetworkinfo()['subversion'] - to_connection_subver = to_connection.getnetworkinfo()['subversion'] - - def find_conn(node, peer_subversion, inbound): - return next(filter(lambda peer: peer['subver'] == peer_subversion and peer['inbound'] == inbound, node.getpeerinfo()), None) - self.wait_until(lambda: find_conn(from_connection, to_connection_subver, inbound=False) is not None) self.wait_until(lambda: find_conn(to_connection, from_connection_subver, inbound=True) is not None) From fae807ed25561bab7148c5a0d7bd847314c79d88 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 27 Mar 2026 16:21:42 +0100 Subject: [PATCH 4/4] test: Remove unused, confusing and brittle connect_nodes.wait_for_connect The option is unused since the last removals in: * 4c40a923f003420193aa574745f70788bcf35265, and * 81bf3ebff7e7108bbfbf6fe4e122f4e52f278701 It was brittle and lead to intermittent test issues. Generally, it is also confusing, because if a test wanted to connect nodes without checking their connection, it can use `addnode`, like the rpc_setban.py test. So fix all issues by removing it. --- test/functional/test_framework/test_framework.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 229ff893389..256a384f89f 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -549,14 +549,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): def wait_for_node_exit(self, i, timeout): self.nodes[i].process.wait(timeout) - def connect_nodes(self, a, b, *, peer_advertises_v2=None, wait_for_connect: bool = True): - """ - Kwargs: - wait_for_connect: if True, block until the nodes are verified as connected. You might - want to disable this when using -stopatheight with one of the connected nodes, - since there will be a race between the actual connection and performing - the assertions before one node shuts down. - """ + def connect_nodes(self, a, b, *, peer_advertises_v2=None): from_connection = self.nodes[a] to_connection = self.nodes[b] @@ -582,9 +575,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): # compatibility with older clients from_connection.addnode(ip_port, "onetry") - if not wait_for_connect: - return - self.wait_until(lambda: find_conn(from_connection, to_connection_subver, inbound=False) is not None) self.wait_until(lambda: find_conn(to_connection, from_connection_subver, inbound=True) is not None)