Merge bitcoin/bitcoin#28805: test: Make existing functional tests compatible with --v2transport

35fb9930adb3501b29d3ad20d2e74c0114f2bcbe test: enable v2 transport for p2p_timeouts.py (Martin Zumsande)
2c1669c37a9759e15ff5f4e340aeaa8778a81b9a test: enable v2 transport for rpc_net.py (Sebastian Falbesoner)
cc961c26956859850202f56191981b0306a65fcf test: enable v2 transport for p2p_node_network_limited.py (Sebastian Falbesoner)
3598a1b5c932634dc7ccb991cc83df5e1a1dcaa9 test: enable --v2transport in combination with --usecli (Martin Zumsande)
68a90017519874793e34e3b439a63e5aa3a6f6a7 test: persist -v2transport over restarts and respect -v2transport=0 (Martin Zumsande)

Pull request description:

  This makes the functional test suite compatible with BIP324, so that
  `python3 test_runner.py --v2transport`
  should succeed (currently, 12 tests fail for me on master).
  Includes two commits by TheStack I found in an old discussion https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1326714164

  Note that even though all tests should pass, the python `p2p.py` module will do v2 connections only after the merge of #24748, so that for now only connections between two full nodes will actually run v2.
  Some of the fixed tests were added with `--v2transport` to the test runner. Though after #24748 we might also want to consider running the entire suite with `--v2transport` in some CI.

ACKs for top commit:
  sipa:
    utACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe. Thanks for taking care of this.
  achow101:
    ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe
  theStack:
    ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe
  stratospher:
    ACK 35fb993.

Tree-SHA512: 80dc0bf211fa525ff1d092043aea9f222f14c02e5832a548fb8b83b9ede1fcee03c5e8ade0d05c331bdaa492af9c1cf3d0f0b15b846673c6eacea82dd4cefbc3

6 files changed, 53 insertions, 21 deletions

diff --git a/test/functional/p2p_node_network_limited.py b/test/functional/p2p_node_network_limited.py
index a56afbcf7b..89c35e943b 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/p2p_timeouts.py b/test/functional/p2p_timeouts.py
index a308577c02..b4fa5099d8 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())
 
-        self.mock_forward(1)
+        # 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/rpc_net.py b/test/functional/rpc_net.py
index 773ab3b50e..b193ffd462 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")
@@ -345,7 +349,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_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 0369b4be05..0ee332b75b 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -507,8 +507,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),
@@ -527,6 +525,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):
@@ -601,7 +600,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 281469f9ed..850aa20db2 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)
@@ -782,15 +790,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)
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 35c662b0d9..6016a482f8 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -156,6 +156,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',
@@ -243,6 +244,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',
@@ -368,6 +370,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',