net: raise V1_PREFIX_LEN from 12 to 16 #28577

pull sipa wants to merge 1 commits into bitcoin:master from sipa:202310_bip324_prefix16 changing 2 files +6 −6
  1. sipa commented at 6:48 pm on October 3, 2023: member

    A “version” message in the V1 protocol starts with a fixed 16 bytes:

    • The 4-byte network magic
    • The 12-byte command string: “version” plus 5 0x00 bytes

    The current code detects incoming V1 connections by just looking at the first 12 bytes (matching an earlier version of BIP324), but 16 bytes is more precise. This isn’t an observable difference right now, as a 12 byte prefix ought to be negligible already, but it may become observable with future extensions to the protocol, so make the code match the specification.

  2. DrahtBot commented at 6:48 pm on October 3, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK theStack, mzumsande, achow101
    Stale ACK vasild

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label P2P on Oct 3, 2023
  4. maflcko added this to the milestone 26.0 on Oct 3, 2023
  5. fanquake requested review from Sjors on Oct 4, 2023
  6. fanquake requested review from theStack on Oct 4, 2023
  7. fanquake requested review from vasild on Oct 4, 2023
  8. fanquake requested review from mzumsande on Oct 4, 2023
  9. in src/net.cpp:1101 in 78fc36df11 outdated
    1098@@ -1099,9 +1099,9 @@ void V2Transport::ProcessReceivedMaybeV1Bytes() noexcept
    1099     Assume(m_recv_state == RecvState::KEY_MAYBE_V1);
    1100     // We still have to determine if this is a v1 or v2 connection. The bytes being received could
    1101     // be the beginning of either a v1 packet (network magic + "version\x00"), or of a v2 public
    


    vasild commented at 1:40 pm on October 4, 2023:

    Now it reads network magic + "version\x00" ... this 16-byte string, but that’s 12-byte string.

    0    // be the beginning of either a v1 packet (network magic + "version\x00\x00\x00\x00\x00"), or of a v2 public
    

    sipa commented at 3:04 pm on October 4, 2023:
    Fixed.
  10. vasild approved
  11. vasild commented at 1:43 pm on October 4, 2023: contributor
    ACK 78fc36df114b858fb227dc01b09145f10287d33e
  12. net: raise V1_PREFIX_LEN from 12 to 16
    A "version" message in the V1 protocol starts with a fixed 16 bytes:
    * The 4-byte network magic
    * The 12-byte zero-padded command "version" plus 5 0x00 bytes
    
    The current code detects incoming V1 connections by just looking at the
    first 12 bytes (matching an earlier version of BIP324), but 16 bytes is
    more precise. This isn't an observable difference right now, as a 12 byte
    prefix ought to be negligible already, but it may become observable with
    future extensions to the protocol, so make the code match the
    specification.
    ba2e5bfc67
  13. theStack approved
  14. theStack commented at 3:05 pm on October 4, 2023: contributor

    ACK 78fc36df114b858fb227dc01b09145f10287d33e

    FWIW, I’ve extended the functional test to add coverage for incoming v1 connection and wrong network magic detections: #28588 (it could be included here, though I think the change in this PR is simple enough to to merge it without the test)

  15. sipa force-pushed on Oct 4, 2023
  16. fanquake requested review from vasild on Oct 4, 2023
  17. fanquake requested review from theStack on Oct 4, 2023
  18. theStack approved
  19. theStack commented at 5:14 pm on October 4, 2023: contributor
    re-ACK ba2e5bfc67dcffca26af9e231652ada1767cbeb2
  20. mzumsande commented at 8:35 pm on October 4, 2023: contributor
    Code review ACK ba2e5bfc67dcffca26af9e231652ada1767cbeb2
  21. achow101 commented at 8:50 pm on October 4, 2023: member
    ACK ba2e5bfc67dcffca26af9e231652ada1767cbeb2
  22. achow101 merged this on Oct 4, 2023
  23. achow101 closed this on Oct 4, 2023

  24. fanquake referenced this in commit 78fd3c2672 on Oct 5, 2023
  25. Frank-GER referenced this in commit f74e322c25 on Oct 13, 2023
  26. kwvg referenced this in commit 0c93a1e7d9 on Sep 17, 2024
  27. kwvg referenced this in commit a64dfc803e on Sep 17, 2024
  28. kwvg referenced this in commit 6101132f42 on Sep 17, 2024
  29. kwvg referenced this in commit 4a082173dc on Oct 2, 2024
  30. kwvg referenced this in commit de40366144 on Oct 9, 2024
  31. kwvg referenced this in commit 280136433c on Oct 14, 2024
  32. kwvg referenced this in commit 8abe5ad3d0 on Oct 15, 2024
  33. kwvg referenced this in commit 6074974849 on Oct 15, 2024
  34. PastaPastaPasta referenced this in commit 5903fb7898 on Oct 16, 2024
  35. bitcoin locked this on Dec 3, 2024

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: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me