test: Move MIN_VERSION_SUPPORTED to p2p.py #20524
Conversation
|
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. |
|
ACK c2f5a3b I'll rebase my 20411, and therefore 19509 onto this. |
I don't think regarding it as a unicode string is strictly correct here. It is a byte slice from the wire and ideally should be regarded as such (for a similar reason I do not use strings in #20434. ELF "strings" are arbitrary byte sequences). |
|
P2P_SUBVERSION is really an ascii-string (for which the byte representation and unicode representation are identical). Theoretically, could be clarified by using |
jnewbery
changed the title
|
I think it's fine to store this as a string locally and then serialize/deserialize to bytes on the wire. |
|
Side-note: Not sure what the current status of mypy is, but it might be able to detect the difference between |
|
Rebased |
jnewbery
force-pushed
the
2020-11-subversion-string
branch
from
c2f5a3b
to
26a76d2
Compare
|
Marking this as draft. It conflicts with #19315, which is higher priority. |
jnewbery
force-pushed
the
2020-11-subversion-string
branch
from
26a76d2
to
78860ff
Compare
|
#19315 is merged. Marking this ready for review. |
The messages.py module should contain code and helpers for [de]serializing p2p messages. Specific usage of those messages should be in p2p.py. Therefore move MIN_VERSION_SUPPORTED to p2p.py. Also rename to MIN_P2P_VERSION_SUPPORTED to distinguish it from other versioning used in Bitcoin/Bitcoin Core.
The messages.py module should contain code and helpers for [de]serializing p2p messages. Specific usage of those messages should be in p2p.py. Therefore move MY_VERSION to p2p.py. Also rename to P2P_VERSION to distinguish it from other versioning used in Bitcoin/Bitcoin Core. Also always set the nVersion field in CBlockLocator to 0 and ignore the field in deserialized messages. The field is not currently used for anything in Bitcoin Core.
The messages.py module should contain code and helpers for [de]serializing p2p messages. Specific usage of those messages should be in p2p.py. Therefore move MY_SUBVERSION to p2p.py. Also rename to P2P_SUBVERSION.
messages.py is for message and primitive data structures. Specifics about the test framework's p2p implementation should be in p2p.py. Also rename to P2P_VERSION_RELAY. Also rename msg_version.nRelay to relay. In Bitcoin Core, this is referred to as fRelay, since it's a bool, so this field has always been misnamed.
The messages.py module should contain code and helpers for [de]serializing p2p messages. Specific usage of those messages should be in p2p.py. Therefore specify the nServices value in the calling code, not in the messages.py module.
Add a check that new connections from the test framework to the node have the correct user agent string. This makes bugs easier to detect if the user agent string ever changes.
jnewbery
force-pushed
the
2020-11-subversion-string
branch
from
78860ff
to
9f21ed4
Compare
|
rebased |
|
Code review ACK 9f21ed4
It's pretty good at that. |
| 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. |
There was a problem hiding this comment.
I think It should be:
f = struct.unpack("<i", f.read(4))[0]
instead of
struct.unpack("<i", f.read(4))[0]
because you're not using the return value..
There was a problem hiding this comment.
f is the stream that we're deserializing from. We intentionally discard the version field from this message.
There was a problem hiding this comment.
pm ACK 9f21ed4 💈
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
pm ACK 9f21ed4037 💈
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh2ywv/RnsCXhZRXPNcUo2XYQ/whSycVTKtNLKN0Yb90EXcE3pwOKRetzovq2HN
HbanioTGj4nCUB9IApkoux7uTOYK6a2GVf3zhj78B0JIDvk1DiljPSzKiCN/upN2
7Cy7tzw+A7PIIHqzMpTCsd3772WEkY+hkrM7Ue2W42uaHuU0uBYpkRCKadArxRuP
bkSjc51EanRaAsv5lD0upaR7kBLjycM3HyZpvZeTlc+j90H0ZDnMeJswKyqnaT+f
kiIEYU45OD5I2I20LQw6VJAjTKqy8bfopYuRon9mP+MCsviqCIlh7VpSmvwcUo+f
NPFbitgn78K+F7tiblnuiuY388DaIkLtw635LvxM7HWq1x3dSjRwo3zKaAGO/450
JpQF/I7z6+BA2vMzr+1/ZlCEX3HQJFl5yPIvK2fpdbhI7u4iTsSA5FG43ybAYbSL
Cy24ZJd3AXkxKX78otf3FUhFhSA7fzAwQWs4fzTsAYQKsIk33+ZQNzCNepMpyUfb
Tt0jScdb
=Ee/R
-----END PGP SIGNATURE-----
Timestamp of file with hash a32d81c8ced92d74226de54ba290f24e73c7935f027b285032460f24a5ee09e6 -
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.