test: Move MIN_VERSION_SUPPORTED to p2p.py

jnewbery commented at 8:35 pm on November 28, 2020: member

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.

jnewbery commented at 8:35 pm on November 28, 2020: member

cc @marcofalke @amitiuttarwar @jonatack

DrahtBot added the label Tests on Nov 28, 2020

DrahtBot commented at 1:37 am on November 29, 2020: 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)

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.

troygiorshev commented at 2:41 pm on December 2, 2020: contributor

ACK c2f5a3b386fa6e81fe453232915fef60f8cc607f

I’ll rebase my 20411, and therefore 19509 onto this.

laanwj commented at 1:06 pm on December 3, 2020: member

It also changes the SUBVERSION constant to be a string instead of a bytes object.

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). Where this can cause problems is in deserialization, if there’s >0x80 bytes and they’re not used in valid UTF-8 then .decode() will raise an exception. That said this change doesn’t touch deserialization and the test framework doesn’t have to handle weird nodes sending junk, so I think it’s okay.

MarcoFalke commented at 1:37 pm on December 3, 2020: member

P2P_SUBVERSION is really an ascii-string (for which the byte representation and unicode representation are identical). Theoretically, could be clarified by using .en/decode('ascii').

jnewbery renamed this:
~~[test] Move MIN_VERSION_SUPPORTED to p2p.py~~
test: Move MIN_VERSION_SUPPORTED to p2p.py
on Dec 3, 2020

jnewbery commented at 10:20 am on December 10, 2020: member

I think it’s fine to store this as a string locally and then serialize/deserialize to bytes on the wire.

MarcoFalke commented at 10:32 am on December 10, 2020: member

Side-note: Not sure what the current status of mypy is, but it might be able to detect the difference between bytes and str, if annotated.

DrahtBot added the label Needs rebase on Dec 12, 2020

jnewbery commented at 2:17 pm on December 12, 2020: member

Rebased

jnewbery force-pushed on Dec 12, 2020

jnewbery commented at 2:18 pm on December 12, 2020: member

Marking this as draft. It conflicts with #19315, which is higher priority.

jnewbery marked this as a draft on Dec 12, 2020

jnewbery force-pushed on Jan 20, 2021

jnewbery marked this as ready for review on Jan 20, 2021

jnewbery commented at 4:04 pm on January 20, 2021: member

#19315 is merged. Marking this ready for review.

DrahtBot removed the label Needs rebase on Jan 20, 2021

DrahtBot added the label Needs rebase on Feb 17, 2021

[test] Move MIN_VERSION_SUPPORTED to p2p.py

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.

652311165c

[test] Move MY_VERSION to p2p.py

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.

7e158a6910

[test] Move MY_SUBVERSION to p2p.py

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.

9b4054cb7a

[test] Move MY_RELAY to p2p.py

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.

010542614d

[test] Add P2P_SERVICES to p2p.py

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.

9ce4c3c4c1

[test] Check user agent string from test framework connections

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.

9f21ed4037

jnewbery force-pushed on Feb 17, 2021

jnewbery commented at 9:32 am on February 17, 2021: member

rebased

DrahtBot removed the label Needs rebase on Feb 17, 2021

laanwj commented at 12:59 pm on February 18, 2021: member

Code review ACK 9f21ed4037758f407b536c0dd129f8da83173c79

Side-note: Not sure what the current status of mypy is, but it might be able to detect the difference between bytes and str, if annotated.

It’s pretty good at that.

laanwj merged this on Feb 18, 2021

laanwj closed this on Feb 18, 2021

jnewbery deleted the branch on Feb 18, 2021

sidhujag referenced this in commit ca4c86fe47 on Feb 18, 2021

in test/functional/test_framework/messages.py:327 in 9f21ed4037

320@@ -326,22 +321,20 @@ class CBlockLocator:
321     __slots__ = ("nVersion", "vHave")
322 
323     def __init__(self):
324-        self.nVersion = MY_VERSION
325         self.vHave = []
326 
327     def deserialize(self, f):
328-        self.nVersion = struct.unpack("<i", f.read(4))[0]
329+        struct.unpack("<i", f.read(4))[0]  # Ignore version field.

brunoerg commented at 12:51 pm on February 19, 2021:

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..

jnewbery commented at 1:19 pm on February 19, 2021:

f is the stream that we’re deserializing from. We intentionally discard the version field from this message.

MarcoFalke commented at 1:15 pm on February 19, 2021: member

pm ACK 9f21ed4037 💈

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3pm ACK 9f21ed4037 💈
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh2ywv/RnsCXhZRXPNcUo2XYQ/whSycVTKtNLKN0Yb90EXcE3pwOKRetzovq2HN
 8HbanioTGj4nCUB9IApkoux7uTOYK6a2GVf3zhj78B0JIDvk1DiljPSzKiCN/upN2
 97Cy7tzw+A7PIIHqzMpTCsd3772WEkY+hkrM7Ue2W42uaHuU0uBYpkRCKadArxRuP
10bkSjc51EanRaAsv5lD0upaR7kBLjycM3HyZpvZeTlc+j90H0ZDnMeJswKyqnaT+f
11kiIEYU45OD5I2I20LQw6VJAjTKqy8bfopYuRon9mP+MCsviqCIlh7VpSmvwcUo+f
12NPFbitgn78K+F7tiblnuiuY388DaIkLtw635LvxM7HWq1x3dSjRwo3zKaAGO/450
13JpQF/I7z6+BA2vMzr+1/ZlCEX3HQJFl5yPIvK2fpdbhI7u4iTsSA5FG43ybAYbSL
14Cy24ZJd3AXkxKX78otf3FUhFhSA7fzAwQWs4fzTsAYQKsIk33+ZQNzCNepMpyUfb
15Tt0jScdb
16=Ee/R
17-----END PGP SIGNATURE-----

Timestamp of file with hash a32d81c8ced92d74226de54ba290f24e73c7935f027b285032460f24a5ee09e6 -

test: Move MIN_VERSION_SUPPORTED to p2p.py #20524

Conflicts