From 68a90017519874793e34e3b439a63e5aa3a6f6a7 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 3 Nov 2023 16:54:19 -0400 Subject: [PATCH 1/5] test: persist -v2transport over restarts and respect -v2transport=0 Before, a global -v2transport provided to the test would be dropped when restarting the node within a test and specifying any extra_args. Fix this by adding "v2transport=1" to args (not extra_args) based on the global parameter, and deciding for each (re)start of the node based on this default and test-specific extra_args (which take precedence over args) whether v2 should be used. --- test/functional/test_framework/test_framework.py | 5 ++--- test/functional/test_framework/test_node.py | 10 +++++++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 70b39434783..4660d2fcdea 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -508,8 +508,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): assert_equal(len(binary_cli), num_nodes) for i in range(num_nodes): args = list(extra_args[i]) - if self.options.v2transport and ("-v2transport=0" not in args): - args.append("-v2transport=1") test_node_i = TestNode( i, get_datadir_path(self.options.tmpdir, i), @@ -528,6 +526,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): start_perf=self.options.perf, use_valgrind=self.options.valgrind, descriptors=self.options.descriptors, + v2transport=self.options.v2transport, ) self.nodes.append(test_node_i) if not test_node_i.version_is_at_least(170000): @@ -602,7 +601,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): ip_port = "127.0.0.1:" + str(p2p_port(b)) if peer_advertises_v2 is None: - peer_advertises_v2 = self.options.v2transport + peer_advertises_v2 = from_connection.use_v2transport if peer_advertises_v2: from_connection.addnode(node=ip_port, command="onetry", v2transport=True) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index b6af71d85c9..435140cbebe 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -67,7 +67,7 @@ class TestNode(): To make things easier for the test writer, any unrecognised messages will be dispatched to the RPC connection.""" - def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False): + def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False, v2transport=False): """ Kwargs: start_perf (bool): If True, begin profiling the node with `perf` as soon as @@ -126,6 +126,12 @@ class TestNode(): if self.version_is_at_least(239000): self.args.append("-loglevel=trace") + # Default behavior from global -v2transport flag is added to args to persist it over restarts. + # May be overwritten in individual tests, using extra_args. + self.default_to_v2 = v2transport + if self.default_to_v2: + self.args.append("-v2transport=1") + self.cli = TestNodeCLI(bitcoin_cli, self.datadir_path) self.use_cli = use_cli self.start_perf = start_perf @@ -198,6 +204,8 @@ class TestNode(): if extra_args is None: extra_args = self.extra_args + self.use_v2transport = "-v2transport=1" in extra_args or (self.default_to_v2 and "-v2transport=0" not in extra_args) + # Add a new stdout and stderr file each time bitcoind is started if stderr is None: stderr = tempfile.NamedTemporaryFile(dir=self.stderr_dir, delete=False) From 3598a1b5c932634dc7ccb991cc83df5e1a1dcaa9 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 6 Nov 2023 16:03:47 -0500 Subject: [PATCH 2/5] test: enable --v2transport in combination with --usecli By renaming the "command" send_cli arg. The old name was unsuitable because the "addnode" RPC has its own "command" arg, leading to ambiguity when included in kwargs. Can be tested with "python3 wallet_multiwallet.py --usecli --v2transport" which fails on master because of this (python throws a TypeError). --- test/functional/test_framework/test_node.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 435140cbebe..c23119c6cb2 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -787,15 +787,15 @@ class TestNodeCLI(): results.append(dict(error=e)) return results - def send_cli(self, command=None, *args, **kwargs): + def send_cli(self, clicommand=None, *args, **kwargs): """Run bitcoin-cli command. Deserializes returned string as python object.""" pos_args = [arg_to_cli(arg) for arg in args] named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items()] p_args = [self.binary, f"-datadir={self.datadir}"] + self.options if named_args: p_args += ["-named"] - if command is not None: - p_args += [command] + if clicommand is not None: + p_args += [clicommand] p_args += pos_args + named_args self.log.debug("Running bitcoin-cli {}".format(p_args[2:])) process = subprocess.Popen(p_args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) From cc961c26956859850202f56191981b0306a65fcf Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Fri, 15 Sep 2023 02:18:38 +0200 Subject: [PATCH 3/5] test: enable v2 transport for p2p_node_network_limited.py --- test/functional/p2p_node_network_limited.py | 12 +++++++++++- test/functional/test_runner.py | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/test/functional/p2p_node_network_limited.py b/test/functional/p2p_node_network_limited.py index a56afbcf7b3..89c35e943b5 100755 --- a/test/functional/p2p_node_network_limited.py +++ b/test/functional/p2p_node_network_limited.py @@ -8,7 +8,15 @@ Tests that a node configured with -prune=550 signals NODE_NETWORK_LIMITED correc and that it responds to getdata requests for blocks correctly: - send a block within 288 + 2 of the tip - disconnect peers who request blocks older than that.""" -from test_framework.messages import CInv, MSG_BLOCK, msg_getdata, msg_verack, NODE_NETWORK_LIMITED, NODE_WITNESS +from test_framework.messages import ( + CInv, + MSG_BLOCK, + NODE_NETWORK_LIMITED, + NODE_P2P_V2, + NODE_WITNESS, + msg_getdata, + msg_verack, +) from test_framework.p2p import P2PInterface from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -50,6 +58,8 @@ class NodeNetworkLimitedTest(BitcoinTestFramework): node = self.nodes[0].add_p2p_connection(P2PIgnoreInv()) expected_services = NODE_WITNESS | NODE_NETWORK_LIMITED + if self.options.v2transport: + expected_services |= NODE_P2P_V2 self.log.info("Check that node has signalled expected services.") assert_equal(node.nServices, expected_services) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 341a31909b6..cbc03a3e3a6 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -365,6 +365,7 @@ BASE_SCRIPTS = [ 'wallet_orphanedreward.py', 'wallet_timelock.py', 'p2p_node_network_limited.py', + 'p2p_node_network_limited.py --v2transport', 'p2p_permissions.py', 'feature_blocksdir.py', 'wallet_startup.py', From 2c1669c37a9759e15ff5f4e340aeaa8778a81b9a Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Fri, 15 Sep 2023 03:20:07 +0200 Subject: [PATCH 4/5] test: enable v2 transport for rpc_net.py - "transport_protocol_type" of inbound peer before version handshake is "detecting" on p2p v2 nodes (as opposed to "v1" for p2p v1) - size of a ping/pong message is 29 bytes (as opposed to 32 for p2p v1) - for the sendmsgtopeer RPC sub-test, enforce p2p v1 connection to have a peer id of zero --- test/functional/rpc_net.py | 23 +++++++++++++++-------- test/functional/test_runner.py | 1 + 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 50a022fc7e1..a74a8cac2fa 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -150,7 +150,7 @@ class NetTest(BitcoinTestFramework): "synced_blocks": -1, "synced_headers": -1, "timeoffset": 0, - "transport_protocol_type": "v1", + "transport_protocol_type": "v1" if not self.options.v2transport else "detecting", "version": 0, }, ) @@ -160,19 +160,23 @@ class NetTest(BitcoinTestFramework): def test_getnettotals(self): self.log.info("Test getnettotals") # Test getnettotals and getpeerinfo by doing a ping. The bytes - # sent/received should increase by at least the size of one ping (32 - # bytes) and one pong (32 bytes). + # sent/received should increase by at least the size of one ping + # and one pong. Both have a payload size of 8 bytes, but the total + # size depends on the used p2p version: + # - p2p v1: 24 bytes (header) + 8 bytes (payload) = 32 bytes + # - p2p v2: 21 bytes (header/tag with short-id) + 8 bytes (payload) = 29 bytes + ping_size = 32 if not self.options.v2transport else 29 net_totals_before = self.nodes[0].getnettotals() peer_info_before = self.nodes[0].getpeerinfo() self.nodes[0].ping() - self.wait_until(lambda: (self.nodes[0].getnettotals()['totalbytessent'] >= net_totals_before['totalbytessent'] + 32 * 2), timeout=1) - self.wait_until(lambda: (self.nodes[0].getnettotals()['totalbytesrecv'] >= net_totals_before['totalbytesrecv'] + 32 * 2), timeout=1) + self.wait_until(lambda: (self.nodes[0].getnettotals()['totalbytessent'] >= net_totals_before['totalbytessent'] + ping_size * 2), timeout=1) + self.wait_until(lambda: (self.nodes[0].getnettotals()['totalbytesrecv'] >= net_totals_before['totalbytesrecv'] + ping_size * 2), timeout=1) for peer_before in peer_info_before: peer_after = lambda: next(p for p in self.nodes[0].getpeerinfo() if p['id'] == peer_before['id']) - self.wait_until(lambda: peer_after()['bytesrecv_per_msg'].get('pong', 0) >= peer_before['bytesrecv_per_msg'].get('pong', 0) + 32, timeout=1) - self.wait_until(lambda: peer_after()['bytessent_per_msg'].get('ping', 0) >= peer_before['bytessent_per_msg'].get('ping', 0) + 32, timeout=1) + self.wait_until(lambda: peer_after()['bytesrecv_per_msg'].get('pong', 0) >= peer_before['bytesrecv_per_msg'].get('pong', 0) + ping_size, timeout=1) + self.wait_until(lambda: peer_after()['bytessent_per_msg'].get('ping', 0) >= peer_before['bytessent_per_msg'].get('ping', 0) + ping_size, timeout=1) def test_getnetworkinfo(self): self.log.info("Test getnetworkinfo") @@ -342,7 +346,10 @@ class NetTest(BitcoinTestFramework): node = self.nodes[0] self.restart_node(0) - self.connect_nodes(0, 1) + # we want to use a p2p v1 connection here in order to ensure + # a peer id of zero (a downgrade from v2 to v1 would lead + # to an increase of the peer id) + self.connect_nodes(0, 1, peer_advertises_v2=False) self.log.info("Test sendmsgtopeer") self.log.debug("Send a valid message") diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index cbc03a3e3a6..320e0abf100 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -240,6 +240,7 @@ BASE_SCRIPTS = [ 'p2p_getdata.py', 'p2p_addrfetch.py', 'rpc_net.py', + 'rpc_net.py --v2transport', 'wallet_keypool.py --legacy-wallet', 'wallet_keypool.py --descriptors', 'wallet_descriptor.py --descriptors', From 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 6 Nov 2023 15:07:45 -0500 Subject: [PATCH 5/5] test: enable v2 transport for p2p_timeouts.py by skipping the part where we send a non-version message before the version - this message would be interpreted as part of the v2 handshake. --- test/functional/p2p_timeouts.py | 15 ++++++++++----- test/functional/test_runner.py | 1 + 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/test/functional/p2p_timeouts.py b/test/functional/p2p_timeouts.py index a308577c020..b4fa5099d87 100755 --- a/test/functional/p2p_timeouts.py +++ b/test/functional/p2p_timeouts.py @@ -68,11 +68,14 @@ class TimeoutsTest(BitcoinTestFramework): with self.nodes[0].assert_debug_log(['Unsupported message "ping" prior to verack from peer=0']): no_verack_node.send_message(msg_ping()) - with self.nodes[0].assert_debug_log(['non-version message before version handshake. Message "ping" from peer=1']): - no_version_node.send_message(msg_ping()) + + # With v2, non-version messages before the handshake would be interpreted as part of the key exchange. + # Therefore, don't execute this part of the test if v2transport is chosen. + if not self.options.v2transport: + with self.nodes[0].assert_debug_log(['non-version message before version handshake. Message "ping" from peer=1']): + no_version_node.send_message(msg_ping()) self.mock_forward(1) - assert "version" in no_verack_node.last_message assert no_verack_node.is_connected @@ -80,11 +83,12 @@ class TimeoutsTest(BitcoinTestFramework): assert no_send_node.is_connected no_verack_node.send_message(msg_ping()) - no_version_node.send_message(msg_ping()) + if not self.options.v2transport: + no_version_node.send_message(msg_ping()) expected_timeout_logs = [ "version handshake timeout peer=0", - "socket no message in first 3 seconds, 1 0 peer=1", + f"socket no message in first 3 seconds, {'0' if self.options.v2transport else '1'} 0 peer=1", "socket no message in first 3 seconds, 0 0 peer=2", ] @@ -100,5 +104,6 @@ class TimeoutsTest(BitcoinTestFramework): extra_args=['-peertimeout=0'], ) + if __name__ == '__main__': TimeoutsTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 320e0abf100..a9354425955 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -153,6 +153,7 @@ BASE_SCRIPTS = [ 'p2p_invalid_messages.py', 'rpc_createmultisig.py', 'p2p_timeouts.py', + 'p2p_timeouts.py --v2transport', 'wallet_dump.py --legacy-wallet', 'rpc_signer.py', 'wallet_signer.py --descriptors',