Merge #20524: test: Move MIN_VERSION_SUPPORTED to p2p.py

9f21ed4037758f407b536c0dd129f8da83173c79 [test] Check user agent string from test framework connections (John Newbery)
9ce4c3c4c1682032500c97af2d6383897b87c413 [test] Add P2P_SERVICES to p2p.py (John Newbery)
010542614dbebba5f5ad6a58c0554930e9e214fc [test] Move MY_RELAY to p2p.py (John Newbery)
9b4054cb7af22123c7fcc4989e143606a630b2af [test] Move MY_SUBVERSION to p2p.py (John Newbery)
7e158a69104831611462cb555da931331b237c78 [test] Move MY_VERSION to p2p.py (John Newbery)
652311165c4ef298dab71d7162f9054abf439f77 [test] Move MIN_VERSION_SUPPORTED to p2p.py (John Newbery)

Pull request description:

  The messages.py module should contain code and helpers for
  [de]serializing p2p messages. Specific usage of those messages should
  be in p2p.py. This PR moves test framework specific constants to p2p.py.

  It also changes the SUBVERSION constant to be a string instead of a bytes object. That means that it needs to be explicitly converted to a bytes object to serialize into a version message. Failing to do so would cause an easy-to-spot bug. This should avoid silent failures like the one solved in #20522.

ACKs for top commit:
  laanwj:
    Code review ACK 9f21ed4037758f407b536c0dd129f8da83173c79

Tree-SHA512: 41d46575ac0ec36ad074d6c6a5b9cef50b05eeb8ddd8ed0a8f0d0c4617cc7b8baa6580af5b83a668230ce1ac27bf0e56914d0361a48b1b05fd75e2e60350eeaf

5 files changed, 61 insertions, 31 deletions

diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py
index 458e5235b6..8f64419138 100755
--- a/test/functional/p2p_filter.py
+++ b/test/functional/p2p_filter.py
@@ -19,7 +19,13 @@ from test_framework.messages import (
     msg_mempool,
     msg_version,
 )
-from test_framework.p2p import P2PInterface, p2p_lock
+from test_framework.p2p import (
+    P2PInterface,
+    P2P_SERVICES,
+    P2P_SUBVERSION,
+    P2P_VERSION,
+    p2p_lock,
+)
 from test_framework.script import MAX_SCRIPT_ELEMENT_SIZE
 from test_framework.test_framework import BitcoinTestFramework
 
@@ -216,9 +222,12 @@ class FilterTest(BitcoinTestFramework):
         self.log.info('Test BIP 37 for a node with fRelay = False')
         # Add peer but do not send version yet
         filter_peer_without_nrelay = self.nodes[0].add_p2p_connection(P2PBloomFilter(), send_version=False, wait_for_verack=False)
-        # Send version with fRelay=False
+        # Send version with relay=False
         version_without_fRelay = msg_version()
-        version_without_fRelay.nRelay = 0
+        version_without_fRelay.nVersion = P2P_VERSION
+        version_without_fRelay.strSubVer = P2P_SUBVERSION
+        version_without_fRelay.nServices = P2P_SERVICES
+        version_without_fRelay.relay = 0
         filter_peer_without_nrelay.send_message(version_without_fRelay)
         filter_peer_without_nrelay.wait_for_verack()
         assert not self.nodes[0].getpeerinfo()[0]['relaytxes']
diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py
index ca8bf908a9..12b8b7baff 100755
--- a/test/functional/p2p_leak.py
+++ b/test/functional/p2p_leak.py
@@ -17,7 +17,12 @@ from test_framework.messages import (
     msg_ping,
     msg_version,
 )
-from test_framework.p2p import P2PInterface
+from test_framework.p2p import (
+    P2PInterface,
+    P2P_SUBVERSION,
+    P2P_SERVICES,
+    P2P_VERSION_RELAY,
+)
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -125,12 +130,15 @@ class P2PLeakTest(BitcoinTestFramework):
         assert_equal(ver.addrFrom.port, 0)
         assert_equal(ver.addrFrom.ip, '0.0.0.0')
         assert_equal(ver.nStartingHeight, 201)
-        assert_equal(ver.nRelay, 1)
+        assert_equal(ver.relay, 1)
 
         self.log.info('Check that old peers are disconnected')
         p2p_old_peer = self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False)
         old_version_msg = msg_version()
         old_version_msg.nVersion = 31799
+        old_version_msg.strSubVer = P2P_SUBVERSION
+        old_version_msg.nServices = P2P_SERVICES
+        old_version_msg.relay = P2P_VERSION_RELAY
         with self.nodes[0].assert_debug_log(['peer=3 using obsolete version 31799; disconnecting']):
             p2p_old_peer.send_message(old_version_msg)
             p2p_old_peer.wait_for_disconnect()
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 561d1813c1..a18a9ec109 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -31,11 +31,6 @@ import time
 from test_framework.siphash import siphash256
 from test_framework.util import hex_str_to_bytes, assert_equal
 
-MIN_VERSION_SUPPORTED = 60001
-MY_VERSION = 70016  # past wtxid relay
-MY_SUBVERSION = "/python-p2p-tester:0.0.3/"
-MY_RELAY = 1 # from version 70001 onwards, fRelay should be appended to version messages (BIP37)
-
 MAX_LOCATOR_SZ = 101
 MAX_BLOCK_BASE_SIZE = 1000000
 MAX_BLOOM_FILTER_SIZE = 36000
@@ -326,22 +321,20 @@ class CBlockLocator:
     __slots__ = ("nVersion", "vHave")
 
     def __init__(self):
-        self.nVersion = MY_VERSION
         self.vHave = []
 
     def deserialize(self, f):
-        self.nVersion = struct.unpack("<i", f.read(4))[0]
+        struct.unpack("<i", f.read(4))[0]  # Ignore version field.
         self.vHave = deser_uint256_vector(f)
 
     def serialize(self):
         r = b""
-        r += struct.pack("<i", self.nVersion)
+        r += struct.pack("<i", 0)  # Bitcoin Core ignores version field. Set it to 0.
         r += ser_uint256_vector(self.vHave)
         return r
 
     def __repr__(self):
-        return "CBlockLocator(nVersion=%i vHave=%s)" \
-            % (self.nVersion, repr(self.vHave))
+        return "CBlockLocator(vHave=%s)" % (repr(self.vHave))
 
 
 class COutPoint:
@@ -1023,20 +1016,20 @@ class CMerkleBlock:
 
 # Objects that correspond to messages on the wire
 class msg_version:
-    __slots__ = ("addrFrom", "addrTo", "nNonce", "nRelay", "nServices",
+    __slots__ = ("addrFrom", "addrTo", "nNonce", "relay", "nServices",
                  "nStartingHeight", "nTime", "nVersion", "strSubVer")
     msgtype = b"version"
 
     def __init__(self):
-        self.nVersion = MY_VERSION
-        self.nServices = NODE_NETWORK | NODE_WITNESS
+        self.nVersion = 0
+        self.nServices = 0
         self.nTime = int(time.time())
         self.addrTo = CAddress()
         self.addrFrom = CAddress()
         self.nNonce = random.getrandbits(64)
-        self.strSubVer = MY_SUBVERSION
+        self.strSubVer = ''
         self.nStartingHeight = -1
-        self.nRelay = MY_RELAY
+        self.relay = 0
 
     def deserialize(self, f):
         self.nVersion = struct.unpack("<i", f.read(4))[0]
@@ -1055,11 +1048,11 @@ class msg_version:
         if self.nVersion >= 70001:
             # Relay field is optional for version 70001 onwards
             try:
-                self.nRelay = struct.unpack("<b", f.read(1))[0]
+                self.relay = struct.unpack("<b", f.read(1))[0]
             except:
-                self.nRelay = 0
+                self.relay = 0
         else:
-            self.nRelay = 0
+            self.relay = 0
 
     def serialize(self):
         r = b""
@@ -1071,14 +1064,14 @@ class msg_version:
         r += struct.pack("<Q", self.nNonce)
         r += ser_string(self.strSubVer.encode('utf-8'))
         r += struct.pack("<i", self.nStartingHeight)
-        r += struct.pack("<b", self.nRelay)
+        r += struct.pack("<b", self.relay)
         return r
 
     def __repr__(self):
-        return 'msg_version(nVersion=%i nServices=%i nTime=%s addrTo=%s addrFrom=%s nNonce=0x%016X strSubVer=%s nStartingHeight=%i nRelay=%i)' \
+        return 'msg_version(nVersion=%i nServices=%i nTime=%s addrTo=%s addrFrom=%s nNonce=0x%016X strSubVer=%s nStartingHeight=%i relay=%i)' \
             % (self.nVersion, self.nServices, time.ctime(self.nTime),
                repr(self.addrTo), repr(self.addrFrom), self.nNonce,
-               self.strSubVer, self.nStartingHeight, self.nRelay)
+               self.strSubVer, self.nStartingHeight, self.relay)
 
 
 class msg_verack:
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index fa4a567aac..05099f3339 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -31,7 +31,6 @@ import threading
 from test_framework.messages import (
     CBlockHeader,
     MAX_HEADERS_RESULTS,
-    MIN_VERSION_SUPPORTED,
     msg_addr,
     msg_addrv2,
     msg_block,
@@ -79,6 +78,18 @@ from test_framework.util import (
 
 logger = logging.getLogger("TestFramework.p2p")
 
+# The minimum P2P version that this test framework supports
+MIN_P2P_VERSION_SUPPORTED = 60001
+# The P2P version that this test framework implements and sends in its `version` message
+# Version 70016 supports wtxid relay
+P2P_VERSION = 70016
+# The services that this test framework offers in its `version` message
+P2P_SERVICES = NODE_NETWORK | NODE_WITNESS
+# The P2P user agent string that this test framework sends in its `version` message
+P2P_SUBVERSION = "/python-p2p-tester:0.0.3/"
+# Value for relay that this test framework sends in its `version` message
+P2P_VERSION_RELAY = 1
+
 MESSAGEMAP = {
     b"addr": msg_addr,
     b"addrv2": msg_addrv2,
@@ -327,6 +338,9 @@ class P2PInterface(P2PConnection):
     def peer_connect_send_version(self, services):
         # Send a version msg
         vt = msg_version()
+        vt.nVersion = P2P_VERSION
+        vt.strSubVer = P2P_SUBVERSION
+        vt.relay = P2P_VERSION_RELAY
         vt.nServices = services
         vt.addrTo.ip = self.dstaddr
         vt.addrTo.port = self.dstport
@@ -334,7 +348,7 @@ class P2PInterface(P2PConnection):
         vt.addrFrom.port = 0
         self.on_connection_send_msg = vt  # Will be sent in connection_made callback
 
-    def peer_connect(self, *args, services=NODE_NETWORK | NODE_WITNESS, send_version=True, **kwargs):
+    def peer_connect(self, *args, services=P2P_SERVICES, send_version=True, **kwargs):
         create_conn = super().peer_connect(*args, **kwargs)
 
         if send_version:
@@ -417,7 +431,7 @@ class P2PInterface(P2PConnection):
         pass
 
     def on_version(self, message):
-        assert message.nVersion >= MIN_VERSION_SUPPORTED, "Version {} received. Test framework only supports versions greater than {}".format(message.nVersion, MIN_VERSION_SUPPORTED)
+        assert message.nVersion >= MIN_P2P_VERSION_SUPPORTED, "Version {} received. Test framework only supports versions greater than {}".format(message.nVersion, MIN_P2P_VERSION_SUPPORTED)
         if message.nVersion >= 70016 and self.wtxidrelay:
             self.send_message(msg_wtxidrelay())
         if self.support_addrv2:
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index 9f2b570913..5bc1409ba2 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -23,9 +23,10 @@ import sys
 
 from .authproxy import JSONRPCException
 from .descriptors import descsum_create
-from .messages import MY_SUBVERSION
+from .p2p import P2P_SUBVERSION
 from .util import (
     MAX_NODES,
+    assert_equal,
     append_config,
     delete_cookie_file,
     get_auth_cookie,
@@ -545,6 +546,11 @@ class TestNode():
             # in comparison to the upside of making tests less fragile and unexpected intermittent errors less likely.
             p2p_conn.sync_with_ping()
 
+            # Consistency check that the Bitcoin Core has received our user agent string. This checks the
+            # node's newest peer. It could be racy if another Bitcoin Core node has connected since we opened
+            # our connection, but we don't expect that to happen.
+            assert_equal(self.getpeerinfo()[-1]['subver'], P2P_SUBVERSION)
+
         return p2p_conn
 
     def add_outbound_p2p_connection(self, p2p_conn, *, p2p_idx, connection_type="outbound-full-relay", **kwargs):
@@ -572,7 +578,7 @@ class TestNode():
 
     def num_test_p2p_connections(self):
         """Return number of test framework p2p connections to the node."""
-        return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION])
+        return len([peer for peer in self.getpeerinfo() if peer['subver'] == P2P_SUBVERSION])
 
     def disconnect_p2ps(self):
         """Close all p2p connections to the node."""