test: store subversion (user agent) as string in msg_version #20993
Conversation
|
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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| 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')) |
There was a problem hiding this comment.
Bitcoin Core will only produce and read ascii
| r += ser_string(self.strSubVer.encode('utf-8')) | |
| r += ser_string(self.strSubVer.encode('ascii')) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Are you sure? SAFE_CHARS_UA_COMMENT only contains the ranges a-z0-9 and the chars .,;-_?@. See commit 1c1b1b3
There was a problem hiding this comment.
I thought that was only for printing/logging/RPC etc, but seems you're right.
Summary: > 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. This is a backport of [[bitcoin/bitcoin#20993 | core#20993]] Test Plan: `ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10943
5 participants
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.strSubVeris never read (only inmsg_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:
After this patch: