test: store subversion (user agent) as string in msg_version #20993

pull theStack wants to merge 1 commits into bitcoin:master from theStack:2021-01-test-store_user_agent_as_string changing 2 files +4 −4

theStack commented at 4:03 PM on January 23, 2021: member
It seems more natural to treat the "subversion" field (=user agent string, see BIP 14) 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/"
```
test: store subversion (user agent) as string in msg_version de85af5cce
DrahtBot added the label Tests on Jan 23, 2021
laanwj commented at 4:49 PM on January 23, 2021: member

Mind that subversion strings on the P2P network are not guaranteed to be valid UTF-8 encoded. That said, for the test framework this assumption is fine.
in test/functional/test_framework/messages.py:1072 in de85af5cce
```
1068 | @@ -1069,7 +1069,7 @@ def serialize(self):
1069 |          r += self.addrTo.serialize(with_time=False)
1070 |          r += self.addrFrom.serialize(with_time=False)
1071 |          r += struct.pack("<Q", self.nNonce)
1072 | -        r += ser_string(self.strSubVer)
1073 | +        r += ser_string(self.strSubVer.encode('utf-8'))
```
MarcoFalke commented at 5:33 PM on January 23, 2021:

Bitcoin Core will only produce and read ascii

r += ser_string(self.strSubVer.encode('ascii'))
laanwj commented at 11:38 AM on February 18, 2021:

Only in the test framework. Outside it, the user could provide a uacomment in bitcoin.conf with some emoji in it. Or, even non-UTF-8. There, IIRC, are no checks on the content. (sorry for resolving this, apparently github thought that should be the function of my space button)

MarcoFalke commented at 12:34 PM on February 18, 2021:

Are you sure? SAFE_CHARS_UA_COMMENT only contains the ranges a-z0-9 and the chars .,;-_?@. See commit 1c1b1b315f2f89584abe9a7558945dea2fbee708

laanwj commented at 12:42 PM on February 18, 2021:

I thought that was only for printing/logging/RPC etc, but seems you're right.
DrahtBot commented at 9:28 PM on January 23, 2021: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #20726 (p2p: Add DISABLETX message for negotiating block-relay-only connections by sdaftuar)
- #20524 (test: Move MIN_VERSION_SUPPORTED to p2p.py by jnewbery)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
jnewbery commented at 4:29 PM on February 14, 2021: member

utACK de85af5cce727981383ac0fe81f635451b331f23

This change is part of #20524, but it seems fine to make the change separate from that PR. This fixes potential silent failures like the one seen in #20522.
MarcoFalke approved
MarcoFalke merged this on Feb 17, 2021
MarcoFalke closed this on Feb 17, 2021
sidhujag referenced this in commit 611fac43b1 on Feb 17, 2021
Fabcien referenced this in commit 8a56fba12f on Jan 31, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#19509 Per-Peer Message Capture

test: store subversion (user agent) as string in msg_version #20993

Conflicts