test: Move MIN_VERSION_SUPPORTED to p2p.py #20524

pull jnewbery wants to merge 6 commits into bitcoin:master from jnewbery:2020-11-subversion-string changing 5 files +61 −31
  1. 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.

  2. jnewbery commented at 8:35 PM on November 28, 2020: member

    cc @marcofalke @amitiuttarwar @jonatack

  3. DrahtBot added the label Tests on Nov 28, 2020
  4. DrahtBot commented at 1:37 AM on November 29, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  5. DrahtBot cross-referenced this on Nov 29, 2020 from issue test: Fix Version Message Deserialization Discrepancy by troygiorshev
  6. DrahtBot cross-referenced this on Nov 29, 2020 from issue test: Extend p2p_ibd_tx_relay.py to verify no-transaction are requested during IBD by ariard
  7. DrahtBot cross-referenced this on Nov 29, 2020 from issue Add functional test test_txid_inv_delay by ariard
  8. DrahtBot cross-referenced this on Nov 29, 2020 from issue p2p: Treat handshake misbehavior like unknown message by MarcoFalke
  9. DrahtBot cross-referenced this on Nov 29, 2020 from issue [tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar
  10. troygiorshev commented at 2:41 PM on December 2, 2020: contributor

    ACK c2f5a3b386fa6e81fe453232915fef60f8cc607f

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

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

  12. 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').

  13. jnewbery renamed this:
    [test] Move MIN_VERSION_SUPPORTED to p2p.py
    test: Move MIN_VERSION_SUPPORTED to p2p.py
    on Dec 3, 2020
  14. 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.

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

  16. DrahtBot added the label Needs rebase on Dec 12, 2020
  17. jnewbery commented at 2:17 PM on December 12, 2020: member

    Rebased

  18. jnewbery force-pushed on Dec 12, 2020
  19. jnewbery commented at 2:18 PM on December 12, 2020: member

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

  20. jnewbery marked this as a draft on Dec 12, 2020
  21. jnewbery force-pushed on Jan 20, 2021
  22. jnewbery marked this as ready for review on Jan 20, 2021
  23. jnewbery commented at 4:04 PM on January 20, 2021: member

    #19315 is merged. Marking this ready for review.

  24. DrahtBot removed the label Needs rebase on Jan 20, 2021
  25. DrahtBot cross-referenced this on Jan 23, 2021 from issue test: store subversion (user agent) as string in msg_version by theStack
  26. DrahtBot cross-referenced this on Feb 4, 2021 from issue p2p: Add DISABLETX message for negotiating block-relay-only connections by sdaftuar
  27. DrahtBot added the label Needs rebase on Feb 17, 2021
  28. [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
  29. [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
  30. [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
  31. [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
  32. [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
  33. [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
  34. jnewbery force-pushed on Feb 17, 2021
  35. jnewbery commented at 9:32 AM on February 17, 2021: member

    rebased

  36. DrahtBot removed the label Needs rebase on Feb 17, 2021
  37. 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.

  38. laanwj merged this on Feb 18, 2021
  39. laanwj closed this on Feb 18, 2021
  40. jnewbery deleted the branch on Feb 18, 2021
  41. sidhujag referenced this in commit ca4c86fe47 on Feb 18, 2021
  42. 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.

  43. MarcoFalke commented at 1:15 PM on February 19, 2021: member

    pm ACK 9f21ed4037 💈

    <details><summary>Show signature and timestamp</summary>

    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 -

    </details>

  44. bitcoin locked this on Feb 27, 2021
  45. bitcoin deleted a comment on Feb 27, 2021
  46. bitcoin deleted a comment on Feb 27, 2021
  47. bitcoin deleted a comment on Feb 27, 2021
  48. bitcoin deleted a comment on Feb 27, 2021
  49. bitcoin deleted a comment on Feb 27, 2021
  50. bitcoin deleted a comment on Feb 27, 2021
Contributors
jnewberyDrahtBottroygiorshevlaanwjMarcoFalkebrunoerg
Labels
Tests
Linked (view graph)
#20434 contrib: Parse ELF directly for symbol and security checks#20522 [test] Fix sync issue in disconnect_p2ps
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC