net: raise V1_PREFIX_LEN from 12 to 16 #28577

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

Contributors
sipa DrahtBot vasild theStack mzumsande achow101

Review Requested
Sjors vasild

Labels
P2P

Milestone
26.0