From 32ec395c7acc6efc098a45545eb7b7be85f39d1b Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Wed, 17 Feb 2021 09:36:27 +0100
Subject: [PATCH] Merge #20993: test: store subversion (user agent) as string
 in msg_version

de85af5cce727981383ac0fe81f635451b331f23 test: store subversion (user agent) as string in msg_version (Sebastian Falbesoner)

Pull request description:

  It seems more natural to treat the "subversion" field (=user agent string, see [BIP 14](https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#Proposal)) of a node as pure string rather than a bytestring within the test framework. This is also suggested with the naming prefix in `msg_version.strSubVer`: one probably wouldn't expect a field starting with "str" to be a bytestring that needs further decoding to be useful. This PR moves the encoding/decoding parts to the serialization/deserialization routines so that the user doesn't have to bother with that anymore.

  Note that currently, in the master branch the `msg_version.strSubVer` is never read (only in `msg_version.__repr__`); However, one issue that is solved by this PR came up while testing #19509 (not merged yet): A decoding script for binary message capture files takes use of the functional test framework convert it into JSON format. Bytestrings will be convered to hexstrings, while pure strings will (surprise surprise) end up without modification in the file.

  So without this patch, we get:

  ```
  $ jq . out.json | grep -m5 strSubVer
        "strSubVer": "2f5361746f7368693a32312e39392e302f"
        "strSubVer": "2f5361746f7368693a302e32302e312f"
        "strSubVer": "2f5361746f7368693a32312e39392e302f"
        "strSubVer": "2f5361746f7368693a302e32302e312f"
        "strSubVer": "2f5361746f7368693a32312e39392e302f"
  ```

  After this patch:

  ```
  $ jq . out2.json | grep -m5 strSubVer
        "strSubVer": "/Satoshi:21.99.0/"
        "strSubVer": "/Satoshi:0.20.1/"
        "strSubVer": "/Satoshi:21.99.0/"
        "strSubVer": "/Satoshi:0.20.1/"
        "strSubVer": "/Satoshi:21.99.0/"
  ```

ACKs for top commit:
  jnewbery:
    utACK de85af5cce727981383ac0fe81f635451b331f23

Tree-SHA512: ff23642705c858e8387a625537dfec82e6b8a15da6d99b8d12152560e52d243ba17431b602b26f60996d897e00e3f37dcf8dc8a303ffb1d544df29a5937080f9
---
 test/functional/p2p_quorum_data.py          | 2 +-
 test/functional/test_framework/messages.py  | 8 ++++----
 test/functional/test_framework/p2p.py       | 8 ++++----
 test/functional/test_framework/test_node.py | 4 ++--
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/test/functional/p2p_quorum_data.py b/test/functional/p2p_quorum_data.py
index d14ce4874d0e31..7257007f8a8f6a 100755
--- a/test/functional/p2p_quorum_data.py
+++ b/test/functional/p2p_quorum_data.py
@@ -78,7 +78,7 @@ def get_id():
             for p2p in node.p2ps:
                 if uacomment is not None and p2p.uacomment != uacomment:
                     continue
-                if p["subver"] == p2p.strSubVer.decode():
+                if p["subver"] == p2p.strSubVer:
                     return p["id"]
         return None
     wait_until_helper(lambda: get_id() is not None, timeout=10)
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 17164eb55ab6de..d4a8eca3c2dd5b 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -33,7 +33,7 @@
 
 MIN_VERSION_SUPPORTED = 60001
 MY_VERSION = 70231  # NO_LEGACY_ISLOCK_PROTO_VERSION
-MY_SUBVERSION = b"/python-p2p-tester:0.0.3%s/"
+MY_SUBVERSION = "/python-p2p-tester:0.0.3%s/"
 MY_RELAY = 1 # from version 70001 onwards, fRelay should be appended to version messages (BIP37)
 
 MAX_LOCATOR_SZ = 101
@@ -1521,7 +1521,7 @@ def __init__(self):
         self.addrTo = CAddress()
         self.addrFrom = CAddress()
         self.nNonce = random.getrandbits(64)
-        self.strSubVer = MY_SUBVERSION % b""
+        self.strSubVer = MY_SUBVERSION % ""
         self.nStartingHeight = -1
         self.nRelay = MY_RELAY
 
@@ -1535,7 +1535,7 @@ def deserialize(self, f):
         self.addrFrom = CAddress()
         self.addrFrom.deserialize(f, with_time=False)
         self.nNonce = struct.unpack("<Q", f.read(8))[0]
-        self.strSubVer = deser_string(f)
+        self.strSubVer = deser_string(f).decode('utf-8')
 
         self.nStartingHeight = struct.unpack("<i", f.read(4))[0]
 
@@ -1554,7 +1554,7 @@ def serialize(self):
         r += self.addrTo.serialize(with_time=False)
         r += self.addrFrom.serialize(with_time=False)
         r += struct.pack("<Q", self.nNonce)
-        r += ser_string(self.strSubVer)
+        r += ser_string(self.strSubVer.encode('utf-8'))
         r += struct.pack("<i", self.nStartingHeight)
         r += struct.pack("<b", self.nRelay)
         return r
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 4e94149a487065..8a93e7b618c4e0 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -186,13 +186,13 @@ def peer_connect_helper(self, dstaddr, dstport, net, timeout_factor, uacomment):
         if net == "devnet":
             devnet_name = "devnet1"  # see initialize_datadir()
             if self.uacomment is None:
-                self.strSubVer = MY_SUBVERSION % ("(devnet.devnet-%s)" % devnet_name).encode()
+                self.strSubVer = MY_SUBVERSION % ("(devnet.devnet-%s)" % devnet_name)
             else:
-                self.strSubVer = MY_SUBVERSION % ("(devnet.devnet-%s,%s)" % (devnet_name, self.uacomment)).encode()
+                self.strSubVer = MY_SUBVERSION % ("(devnet.devnet-%s,%s)" % (devnet_name, self.uacomment))
         elif self.uacomment is not None:
-            self.strSubVer = MY_SUBVERSION % ("(%s)" % self.uacomment).encode()
+            self.strSubVer = MY_SUBVERSION % ("(%s)" % self.uacomment)
         else:
-            self.strSubVer = MY_SUBVERSION % b""
+            self.strSubVer = MY_SUBVERSION % ""
 
     def peer_connect(self, dstaddr, dstport, *, net, timeout_factor, uacomment=None):
         self.peer_connect_helper(dstaddr, dstport, net, timeout_factor, uacomment)
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index f8a813b55a352a..101d4dbac4074f 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -594,7 +594,7 @@ def addconnection_callback(address, port):
 
     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.decode("utf-8")])
+        return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION])
 
     def disconnect_p2ps(self):
         """Close all p2p connections to the node."""
@@ -605,7 +605,7 @@ def disconnect_p2ps(self):
         def check_peers():
             for p in self.getpeerinfo():
                 for p2p in self.p2ps:
-                    if p['subver'] == p2p.strSubVer.decode():
+                    if p['subver'] == p2p.strSubVer:
                         return False
             return True
         wait_until_helper(check_peers, timeout=5)