[test] Change P2P_SUBVERSION to be a string

jnewbery · jnewbery · commit c2f5a3b386fa · 2020-11-28T20:15:16.000Z
This forces the user to convert to bytes to encode it in a version
message, which would result in an easy-to-detect type error if omitted.

Also add a check that new connections from the test framework to the
node have the correct user agent string. Again, this makes bugs easier
to detect if the user agent string ever changes.
diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py
@@ -227,7 +227,7 @@ def run_test(self):
         version_without_fRelay = msg_version()
         version_without_fRelay.nVersion = P2P_VERSION
         version_without_fRelay.nServices = P2P_SERVICES
-        version_without_fRelay.strSubVer = P2P_SUBVERSION
+        version_without_fRelay.strSubVer = P2P_SUBVERSION.encode('utf-8')
         version_without_fRelay.relay = 0
         filter_peer_without_nrelay.send_message(version_without_fRelay)
         filter_peer_without_nrelay.wait_for_verack()
diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py
@@ -154,7 +154,7 @@ def run_test(self):
         old_version_msg = msg_version()
         old_version_msg.nVersion = 31799
         old_version_msg.nServices = P2P_SERVICES
-        old_version_msg.strSubVer = P2P_SUBVERSION
+        old_version_msg.strSubVer = P2P_SUBVERSION.encode('utf-8')
         old_version_msg.relay = P2P_VERSION_RELAY
         with self.nodes[0].assert_debug_log(['peer=4 using obsolete version 31799; disconnecting']):
             p2p_old_peer.send_message(old_version_msg)
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
@@ -82,7 +82,7 @@
 # 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 = b"/python-p2p-tester:0.0.3/"
+P2P_SUBVERSION = "/python-p2p-tester:0.0.3/"
 # Value for relay that this test framework sends in its `version` message
 P2P_VERSION_RELAY = 1
 
@@ -328,7 +328,7 @@ def peer_connect(self, *args, services=P2P_SERVICES, send_version=True, **kwargs
             # Send a version msg
             vt = msg_version()
             vt.nVersion = P2P_VERSION
-            vt.strSubVer = P2P_SUBVERSION
+            vt.strSubVer = P2P_SUBVERSION.encode('utf-8')
             vt.relay = P2P_VERSION_RELAY
             vt.nServices = services
             vt.addrTo.ip = self.dstaddr
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
@@ -26,6 +26,7 @@
 from .p2p import P2P_SUBVERSION
 from .util import (
     MAX_NODES,
+    assert_equal,
     append_config,
     delete_cookie_file,
     get_auth_cookie,
@@ -540,11 +541,16 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
             # 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 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'] == P2P_SUBVERSION.decode("utf-8")])
+        return len([peer for peer in self.getpeerinfo() if peer['subver'] == P2P_SUBVERSION])
 
     def disconnect_p2ps(self):
         """Close all p2p connections to the node."""
