net: Drop v2 garbage authentication packet #28525

pull real-or-random wants to merge 2 commits into bitcoin:master from real-or-random:202309-garbauth changing 3 files +69 −103
  1. real-or-random commented at 1:46 pm on September 24, 2023: contributor

    Note that this is a breaking change, see also https://github.com/bitcoin/bips/pull/1498

    The benefit is a simpler implementation:

    • The protocol state machine does not need separate states for garbage authentication and version phases.
    • The special case of “ignoring the ignore bit” is removed.
    • The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing.
  2. DrahtBot commented at 1:46 pm on September 24, 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 naumenkogs, sipa, Sjors, theStack, ajtowns, achow101

    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 Sep 24, 2023
  4. in src/net.cpp:1263 in 8753b6af9b outdated
    1264                 // Version message received; transition to application phase. The contents is
    1265                 // ignored, but can be used for future extensions.
    1266                 SetReceiveState(RecvState::APP);
    1267             }
    1268+            // We have decrypted one valid packet (which may or may not have been a decoy) with the
    1269+            // received garbage as AAD. We no longer need the received garbage and further packets
    


    naumenkogs commented at 8:38 am on September 26, 2023:

    further packets are expected to use the empty string as AAD.

    this part is irrelevant here?


    real-or-random commented at 11:38 am on September 26, 2023:
    Hah, it’s not. We need to clear m_recv_garbage to make sure potential further packets in VERSION state don’t except AAD. I agree that it’s confusing. Added a commit to simply further. (Happy to squash if that’s simpler.)
  5. in src/net.h:497 in 8753b6af9b outdated
    504-         * and the decrypted contents is interpreted as version negotiation (currently, that means
    505-         * ignoring it, but it can be used for negotiating future extensions). If it fails, the
    506-         * connection aborts. */
    507+         * A packet is received, and decrypted/verified. The first received packet in this state
    508+         * (whether it's a decoy or not) is expected to authenticate the garbage received during
    509+         * the GARB_GARBTERM state as AAD. If decryption/verification (of the first non-decoy
    


    naumenkogs commented at 8:39 am on September 26, 2023:
    non-decoy decoy? Not sure if this is intentional, although i can see some sense in this… :)

    real-or-random commented at 10:58 am on September 26, 2023:
    Oh, I wanted to write “first non-decoy packet” here. Fixed.
  6. in src/net.h:499 in 8753b6af9b outdated
    506-         * connection aborts. */
    507+         * A packet is received, and decrypted/verified. The first received packet in this state
    508+         * (whether it's a decoy or not) is expected to authenticate the garbage received during
    509+         * the GARB_GARBTERM state as AAD. If decryption/verification (of the first non-decoy
    510+         * decoy) succeeds, the state becomes APP, and the decrypted contents is interpreted as
    511+         * version negotiation (currently, that means ignoring it, but it can be used for
    


    naumenkogs commented at 8:44 am on September 26, 2023:

    This description is confusing. Currently, we will switch to APP no matter whether the authenticating package was decoy or version.

    In future, the state becomes APP, and the decrypted contents is interpreted as version negotiationmight not be the case, if the authenticating package was decoy.

    At that point we might as well change this comment, but why having something certainly confusing in the first place?


    real-or-random commented at 11:02 am on September 26, 2023:

    In future, the state becomes APP, and the decrypted contents is interpreted as version negotiation might not be the case, if the authenticating package was decoy.

    the state becomes APP, and the decrypted contents is interpreted as version negotiation is always the case. The first non-decoy packet received in VERSION state is the version packet, and that means that we interpret the contents as version info and switch to APP.

    At that point we might as well change this comment, but why having something certainly confusing in the first place?

    I’m not sure what you’re suggesting. Can you elaborate? (Maybe it’s based on a misunderstanding that has been resolved now?)


    naumenkogs commented at 7:19 am on September 27, 2023:
    I think my issue was resolved with the typo fix right above.
  7. naumenkogs commented at 8:48 am on September 26, 2023: member
    Concept ACK
  8. ajtowns commented at 9:04 am on September 26, 2023: contributor
    Approach ACK assuming BIP change is accepted
  9. real-or-random force-pushed on Sep 26, 2023
  10. in src/net.cpp:1256 in f02fb7119e outdated
    1266-            if (!ignore) {
    1267+        // At this point we have a valid packet decrypted into m_recv_decode_buffer. If it's not a
    1268+        // decoy, which we simply ignore, use the current state to decide what to do with it.
    1269+        if (!ignore) {
    1270+            switch (m_recv_state) {
    1271+            case RecvState::VERSION:
    


    sipa commented at 12:19 pm on September 26, 2023:

    The if (m_recv_state != RecvState::APP_READY) ClearShrink(m_recv_decode_buffer); below could be moved into this state transition.

    EDIT: no, we also want to do that for decoy packets.


    real-or-random commented at 12:24 pm on September 26, 2023:
    You mean “into this switch”? Since I moved the switch block into the if (!ignore) block, this means we would not clean the buffer in case of a decoy packet. That seems fine to me, but is this what you’re suggesting?
  11. in src/net.h:500 in f02fb7119e outdated
    507+         * A packet is received, and decrypted/verified. The first received packet in this state
    508+         * (whether it's a decoy or not) is expected to authenticate the garbage received during
    509+         * the GARB_GARBTERM state as AAD. If decryption/verification (of the first non-decoy
    510+         * packet) succeeds, the state becomes APP, and the decrypted contents is interpreted as
    511+         * version negotiation (currently, that means ignoring it, but it can be used for
    512+         * negotiating future extensions). If it fails, the connection aborts. */
    


    sipa commented at 12:29 pm on September 26, 2023:

    Nit: the it in this sentence is easily interpreted as “decryption/verification of the first non-decoy packet”, which is slightly confusing, as failure to decrypt will causing aborting even for decoy packets.

    Suggestion:

    A packet is received, and decrypted/verified. If that fails, the connection aborts. The first received packet in this state (whether it’s a decoy or not) is expected to authenticate the garbage received during the GARB_GARBTERM state as AAD. The first non-decoy packet in this state is interpreted as version negotiation (currently, that means ignoring it, but it can be used for negotiating future extensions), and afterwards the state becomes APP.


    naumenkogs commented at 7:22 am on September 27, 2023:

    currently, that means ignoring it, but it can be used for negotiating future extensions

    Perhaps say ignoring the content, to make sure aad is not meant to be ignored?


    real-or-random commented at 10:20 am on September 27, 2023:
    Thanks, done.
  12. in src/test/net_tests.cpp:1196 in f93f8b457f outdated
    1194@@ -1193,25 +1195,26 @@ class V2TransportTester
    1195 
    1196     /** Schedule garbage terminator and authentication packet to be sent to the transport (only
    


    sipa commented at 7:21 pm on September 26, 2023:
    Comment is outdated (“authentication packet”) here.

    real-or-random commented at 10:20 am on September 27, 2023:
    Fixed, along with some more outdated comments found by grepping for “auth” in all touched files.
  13. sipa commented at 7:24 pm on September 26, 2023: member
    Code review ACK, with minor nits. I’m also on board with the corresponding BIP change (see the discussion there).
  14. real-or-random force-pushed on Sep 27, 2023
  15. net: Drop v2 garbage authentication packet
    See also https://github.com/bitcoin/bips/pull/1498
    
    The benefit is a simpler implementation:
     - The protocol state machine does not need separate states for garbage
       authentication and version phases.
     - The special case of "ignoring the ignore bit" is removed.
     - The freedom to choose the contents of the garbage authentication
       packet is removed. This simplifies testing.
    b0f5175c04
  16. net: Simplify v2 recv logic by decoupling AAD from state machine e3720bca39
  17. real-or-random force-pushed on Sep 27, 2023
  18. real-or-random commented at 11:40 am on September 27, 2023: contributor
    The CI failure seems unrelated: test_framework.authproxy.JSONRPCException: Unable to create transaction. Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate) (-4). edit: Okay, this is #28491.
  19. naumenkogs commented at 1:22 pm on September 27, 2023: member
    ACK e3720bca398820038b3e97f467adb2c45ef9ef5f
  20. sipa commented at 1:56 pm on September 27, 2023: member
    ACK e3720bca398820038b3e97f467adb2c45ef9ef5f. Re-ran the v2 transport fuzzer overnight.
  21. in src/net.h:582 in e3720bca39
    580     /** Receive buffer; meaning is determined by m_recv_state. */
    581     std::vector<uint8_t> m_recv_buffer GUARDED_BY(m_recv_mutex);
    582-    /** During GARBAUTH, the garbage received during GARB_GARBTERM. */
    583-    std::vector<uint8_t> m_recv_garbage GUARDED_BY(m_recv_mutex);
    584+    /** AAD expected in next received packet (currently used only for garbage). */
    585+    std::vector<uint8_t> m_recv_aad GUARDED_BY(m_recv_mutex);
    


    Sjors commented at 4:15 pm on September 27, 2023:

    Conceptually it’s a bit weird to receive an AAD in one message but then verify it in the next message. Perhaps it’s cleaner to to keep m_recv_garbage around and explicitly move it into m_recv_aad as soon as we start receiving bytes for the decoy/version message?

    That way we can Assume that m_recv_aad is empty when start receiving bytes and clear it when we’re done processing a message. (this commit already does the latter)


    sipa commented at 4:58 pm on September 27, 2023:

    Perhaps it’s cleaner to to keep m_recv_garbage around and explicitly move it into m_recv_aad as soon as we start receiving bytes for the decoy/version message?

    We receive data into m_recv_buffer, and by the time we process it, we’re effectively already done with it. So what you’re suggesting would just amount to moving/shrinking from m_recv_buffer to m_recv_garbage and immediately after (on the next line) moving that to m_recv_aad.

    I think the better way of looking at it is not that the AAD is data received at all; it’s a side effect of processing data in a particular phase that results in a requirement that the next message commit to it. For now, the only place where this happens is that during the processing of received bytes in the GARB_TERM phase the garbage needs to be committed to (but not all the data received during that phase, as it doesn’t include the terminator).

    That way we can Assume that m_recv_aad is empty when start receiving bytes and clear it when we’re done processing a message. (this commit already does the latter).

    I don’t think that works? At the start of the version phase, the previous garbage will be in m_recv_aad.


    real-or-random commented at 5:44 pm on September 27, 2023:

    Conceptually it’s a bit weird to receive an AAD in one message but then verify it in the next message. Perhaps it’s cleaner to to keep m_recv_garbage around and explicitly move it into m_recv_aad as soon as we start receiving bytes for the decoy/version message?

    If I understand correctly, this is roughly equivalent to dropping the second commit again, and I don’t think this was cleaner.


    Sjors commented at 8:24 am on September 28, 2023:

    At the start of the version phase, the previous garbage will be in m_recv_aad.

    This is the part that feels a bit strange to me. But may it’s not too bad:

    We could instead Assume that m_recv_aad is empty when we enter the APP and when we enter the APP_READY state. It can be cleared in the VERSION state and after each normal message in the APP state, which indeed this PR does.


    Sjors commented at 9:24 am on September 28, 2023:
    I suppose an Assume in APP may or may not be future proof, because it depends on how AADs will be used: do we know what to expect for the next message (as is the case here for garbage), do we infer it from the current message, is it some time dependent thing or even a constant?

    real-or-random commented at 10:15 am on September 28, 2023:

    I suppose an Assume in APP may or may not be future proof, because it depends on how AADs will be used: do we know what to expect for the next message (as is the case here for garbage), do we infer it from the current message, is it some time dependent thing or even a constant?

    Well, if we ever have a v3 that uses AAD for more than the garbage, we could drop the Assume, so I guess that’s not a big deal.

    On the other hand, I don’t see that much value in adding an Assume. The sender and receiver code are sufficiently independent. So unless we have the exact same bug in the sender logic, a bug in the receiver logic that leads to m_recv_aad being non-empty would lead to an immediate disconnect. That is, such a bug will certainly be noticed during testing, even without the Assume. (The Assume would just make it easier to find the cause.)

    My current thinking is that I don’t want to touch the PR now that it has three ACKs. But I’ll add an Assume if there’s another good reason to touch it (or if you have strong opinions on this, of course.)


    real-or-random commented at 10:24 am on September 28, 2023:

    Not totally relevant to the PR but some remarks:

    do we know what to expect for the next message (as is the case here for garbage) do we infer it from the current message,

    I think one of these two is true in 99.9% of the cases I can imagine.

    As for inferring it from current message: Inferring it from the contents of the AEAD is excluded by design (you can’t decrypt a ciphertext that doesn’t verify). It would just be possible for data in the “outer” header (currently that’s just the length field). But even if we want some additional data in the outer header, this doesn’t really contradict the current logic in the code (e.g., we could concatenate the current-message thing to whatever m_recv_aad is, or then start Assuming it’s empty).

    is it some time dependent thing

    That will be too fragile, i.e., we would abort the connection if clocks are off.

    even a constant?

    Then there will be no reason to include it.


    Sjors commented at 10:33 am on September 28, 2023:
    Thanks for the clarifications. Agree that adding an Assume can wait.
  22. Sjors commented at 4:19 pm on September 27, 2023: member
    Concept ACK
  23. theStack commented at 9:31 pm on September 27, 2023: contributor
    Concept ACK
  24. sipa commented at 9:57 pm on September 27, 2023: member
    Observation during discussion with @achow101: the old behavior is actually equivalent to the new behavior, but with the added rule that the very first packet is implicitly a decoy (whether the bit is set or not).
  25. real-or-random commented at 7:23 am on September 28, 2023: contributor

    old behavior […] with the added rule that the very first packet is implicitly a decoy (whether the bit is set or not).

    Interesting observation, and this confirms that the old behavior is somewhat unnatural.

  26. Sjors approved
  27. Sjors commented at 9:30 am on September 28, 2023: member

    utACK e3720bca398820038b3e97f467adb2c45ef9ef5f

    Timing wise, I plan to run this on mainnet once it’s added to the integration PR.

  28. theStack approved
  29. theStack commented at 2:55 pm on September 28, 2023: contributor

    Code-review ACK e3720bca398820038b3e97f467adb2c45ef9ef5f

    I didn’t follow the full discussion in the BIP change PR yet, but to my understanding, this approach simplifies the state machine and doesn’t have any drawbacks (other than adaptions needed in #24748 and a temporary incompatibility between BIP324 nodes running old/new version, obviously).

  30. achow101 commented at 7:19 pm on September 28, 2023: member
    RFM but waiting for BIP change to be merged first.
  31. ajtowns commented at 6:36 am on September 29, 2023: contributor
    ACK e3720bca398820038b3e97f467adb2c45ef9ef5f - simpler and more flexible, nice
  32. Sjors commented at 7:42 am on September 29, 2023: member
    BIP has been merged
  33. Sjors commented at 10:53 am on September 29, 2023: member

    FYI this is what happens if you don’t upgrade your node and try to connect to an upgrade one:

     02023-09-29T10:45:39.069931Z [net] Added connection to ... peer=369
     12023-09-29T10:45:39.069954Z [net] sending version (103 bytes) peer=369
     22023-09-29T10:45:39.069969Z [net] send version message: version 70016, blocks=809861, them=[...]:8333, txrelay=1, peer=369
     32023-09-29T10:45:39.111424Z [net] start sending v2 handshake to peer=369
     42023-09-29T10:45:39.295724Z [net] received: wtxidrelay (0 bytes) peer=369
     52023-09-29T10:45:39.295749Z [net] non-version message before version handshake. Message "wtxidrelay" from peer=369
     62023-09-29T10:45:39.295860Z [net] received: sendaddrv2 (0 bytes) peer=369
     72023-09-29T10:45:39.295883Z [net] non-version message before version handshake. Message "sendaddrv2" from peer=369
     82023-09-29T10:45:39.295990Z [net] received: verack (0 bytes) peer=369
     92023-09-29T10:45:39.296011Z [net] non-version message before version handshake. Message "verack" from peer=369
    102023-09-29T10:46:40.011229Z [net] version handshake timeout, disconnecting peer=369
    112023-09-29T10:46:40.011279Z [net] disconnecting peer=369
    122023-09-29T10:46:40.011323Z [net] Cleared nodestate for peer=369
    

    (bangs head on table for forgetting make install)

    The other way around, if you upgrade but connect to a node that didn’t upgrade, you get something like:

    02023-09-29T11:02:27.083928Z [net] Added connection to ... peer=55
    12023-09-29T11:02:27.084037Z [net] sending version (103 bytes) peer=55
    22023-09-29T11:02:27.084096Z [net] send version message: version 70016, blocks=809862, them=...:8333, txrelay=1, peer=55
    32023-09-29T11:02:27.117891Z [net] start sending v2 handshake to peer=55
    42023-09-29T11:02:27.222922Z [net] V2 transport error: invalid message type (0 bytes contents), peer=55
    52023-09-29T11:02:27.522980Z [net] socket closed for peer=55
    62023-09-29T11:02:27.523016Z [net] disconnecting peer=55
    72023-09-29T11:02:27.523059Z [net] Cleared nodestate for peer=55
    
  34. achow101 commented at 1:49 pm on September 29, 2023: member
    ACK e3720bca398820038b3e97f467adb2c45ef9ef5f
  35. achow101 merged this on Sep 29, 2023
  36. achow101 closed this on Sep 29, 2023

  37. Frank-GER referenced this in commit 478612d643 on Oct 5, 2023
  38. kwvg referenced this in commit c14b211653 on Sep 17, 2024
  39. kwvg referenced this in commit 5b36766408 on Sep 17, 2024
  40. kwvg referenced this in commit 397f34aa7b on Sep 18, 2024
  41. kwvg referenced this in commit 08472d4c31 on Sep 20, 2024
  42. kwvg referenced this in commit 63b13aa519 on Sep 20, 2024
  43. PastaPastaPasta referenced this in commit 3b8f24431c on Sep 24, 2024
  44. bitcoin locked this on Sep 28, 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