Skip to content

Conversation

@real-or-random
Copy link
Contributor

by merging it with the version packet. Or more accurately, by merging it with the first packet sent after garbage termination, which may be a decoy packet or the version packet.

The new protocol simplifies implementations:

  • A protocol state machine won't 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.

The reason for having a separate garbage authentication packet was to materialize the separation of the key exchange phase and version negotiation phase even in the bytestream on the wire. However, this is not necessary, and arguably, these phases are still properly separated: Since the AEAD will ensure that AAD (=garbage) is checked before looking at the contents (=version), the peers won't interpret version data before having authenticated the garbage.

@real-or-random real-or-random marked this pull request as draft September 24, 2023 13:30
@real-or-random
Copy link
Contributor Author

cc @sipa @dhruv

@sipa
Copy link
Member

sipa commented Sep 24, 2023

ACK, though I want to wait for some more opinions - the BIP is fine as is, and I'm not sure whether this minor improvement is worth the breaking change at this point.

For a bit of history, this was a conscious decision earlier, to separate the garbage phase from the version negotiation phase. But arguably, the result isn't all that consistent either. The idea was to use the "packet" interface for everything after garbage terminator, but the abstraction is not perfect: packets have a decoy bit, but the decoy bit of the garbage authentication is ignored. In retrospect, seeing both specification and (one) implementation, it appears simpler to merge the auth packet and version packet.

As for conceptual way of looking at it, I believe this makes sense:

  • We see the packet abstraction as consisting of two layers:
    • The lower-level length encoding + aead
    • The higher-level notion of header byte and decoy packets.
  • The change here is moving the garbage auth functionality to the lower-level layer: whenever additional authentication is needed, the first packet (at this layer decoy or not does not matter, so the first whether that's a decoy or not) after it does that authentication. And the higher-level header/decoy functionality remains for version and application level packets, where this feature makes sense.

@naumenkogs
Copy link
Contributor

ACK
I reviewed changes to the BIP and changes to the codebase (bitcoin#28525). I think at this point, it's worth breaking the current version, it makes everything cleaner and better (although I see the intuition behind the initial logic).

Either peer may send such decoy packets at any point after this.

Should be clearer saying that a garbage authentication package could be a decoy package (answering "after which point exactly?").

So the first two phases together, jointly called the handshake, comprise just 1.5 roundtrips:

Kinda confusing now that garbage authentication is not there.

@real-or-random
Copy link
Contributor Author

Either peer may send such decoy packets at any point after this.

Should be clearer saying that a garbage authentication package could be a decoy package (answering "after which point exactly?").

Rephrased and restructured this to improve clarity.

So the first two phases together, jointly called the handshake, comprise just 1.5 roundtrips:

Kinda confusing now that garbage authentication is not there.

Happy to improve here, but I can't follow. It's still 3 phases altogether, and the first two are 1.5 roundtrips. What do you find confusing?

@real-or-random
Copy link
Contributor Author

ACK
I reviewed changes to the BIP and changes to the codebase (bitcoin#28525)

Want to leave a review or (Concept) ACK also in bitcoin/bitcoin#28525 then? :)

@naumenkogs
Copy link
Contributor

Happy to improve here, but I can't follow. It's still 3 phases altogether, and the first two are 1.5 roundtrips. What do you find confusing?

It was not immediately clear why garbage authentication is not in the list following comprise just 1.5 roundtrips. Now I agree that it doesn't add much value in that description.

@ajtowns
Copy link
Contributor

ajtowns commented Sep 26, 2023

Concept ACK -- fewer states seems like a win, and it seems like the simpler this is, the more easily it could be used for other applications

The 1.5 round trip handshake could arguably be more obscure if described/implemented as:

  • the initiator sends public key + garbage-part-1
  • the responder sends public key + garbage + garbage terminator + version packet + decoy packet
  • the initiator sends garbage-part-2 + garbage terminator + version packet + decoy packet

(otherwise the "garbage-terminator + version packet" may be identifiable as a fixed size response?)

Does it make sense to have a decoy packet before the version packet rather than after it? The version packet tells you how to interpret the header, so it seems slightly odd to be parsing headers before you've actually figured out what the header format is.

Perhaps it would it be simpler but still workable to just have the first packet after the garbage terminator always be the version packet, and have its entire contents (with no header) be interpreted as the version negotiation? In that case, an initial "empty" version message could literally be empty, with no header and no content. In that case after the first version message, you'd pass every message's content to a version-specific decoder that splits out the header, and routes the body to the appropriate application handler.

(In order for the responder to be able to append decoy packets before receiving the initiator's version packet, you'd still need to guarantee that packets with the high-bit of the first byte are ignored no matter what version ends up being negotiated)

It might be slightly clearer to make the pseudocode more stateful as far as the aad info goes, ie drop it as a parameter to v2_enc_packet and v2_receive_packet and instead initialize it based on peer.sent_garbage and peer.recv_garbage, and clear them after first use?

@real-or-random
Copy link
Contributor Author

The 1.5 round trip handshake could arguably be more obscure if described/implemented as:

Just for my understanding: You're saying that the handshake could look more obscure to wiretappers, right? And not that the description of the handshake in the BIP would be more obscure to readers? ;)

(otherwise the "garbage-terminator + version packet" may be identifiable as a fixed size response?)

I don't think so. The "garbage-terminator + version packet" is always preceded by some form "garbage". So if you want to get rid of the fixed-size response, you can also just send garbage. And yes, if you need more bytes, you can always send more decoys after the version packet, but I don't think this needs to be in the handshake description. (You can always send decoys.)

Does it make sense to have a decoy packet before the version packet rather than after it? The version packet tells you how to interpret the header, so it seems slightly odd to be parsing headers before you've actually figured out what the header format is.

I agree that it seems odd to parse headers before you've agreed on a version. And @sipa had similar remarks when we talked about this somewhere else. The thing is that we bootstrap communications, so we build up the layers step by step. And yes, you can argue that at the point when we've just received the garbage terminator, we have not agreed on a header format yet.

And I agree that the possibility to send decoy packets before the version packet doesn't really add functionality. Instead of ignoring the ignore bit (what we do in the current BIP), we could also get rid of this by dropping the header byte entirely (what you suggest). Another way is to simply disallow packages where the ignore bit is set. Though, I don't believe that this will make things simpler. It's adding back a special rule again.

So I think if we agree that it makes sense to drop the explicit garbage authentication packet and get rid of one state in the state machine, then I see the following possibilities after the garbage terminator.

  1. Switch to "normal" packet handling, but the first packet has AAD set: This is what this PR proposes.
  2. Like 1, but additionally ignore the ignore bit: This is what the current BIP says, and it has a special case which could be avoided.
  3. Like 1, but additionally reject the first packet if it's a decoy packet and disconnect: On one hand, that's just a different special case. On the other hand, then it's then always the case that the AAD is in the version packet.
  4. Switch to a simpler headerless packet format just for the version negotiation: This is what you propose. It seems conceptually cleaner, but this requires that we expose a way to parse headerless packets to the high-level net code. In that sense. I don't see how a rule "parse a raw version packet and delegate further packets to version-specific decoder" is much simpler than a rule "parse a v2 version packet, and delegate further packets to version-specific decoder". The former will always need one parser more, namely one for raw packets.

Among these approaches, I like 1 and 3 most.

I think approach 1 (in this PR) is conceptually nice because it decouples the AAD stuff from the protocol states. You simply keep a variable around that tells what you expect as AAD in the next received packet [1]. Approach 3 does not need this variable and just needs an if (ignore) return false in the VERSION state in the C code. That's also pretty simple. Happy to switch to this approach, I guess it's really on par with approach 1.

It might be slightly clearer to make the pseudocode more stateful as far as the aad info goes, ie drop it as a parameter to v2_enc_packet and v2_receive_packet and instead initialize it based on peer.sent_garbage and peer.recv_garbage, and clear them after first use?

Will probably give this a shot later.

[1] Note that when you wrote your comment here, the way I was implementing this (https://github.com/real-or-random/bitcoin/blob/8753b6af9bf8767be50d71d1bf7e3711738fcfd2/src/net.cpp#L1239-L1278) technically did not achieve this decoupling yet. I've improved this now in the implementation PR this by renaming the m_recv_buffer to m_recv_aad and handling AAD independently of whether the receiver is in VERSION state or not. This also means that the handling of the ignore bit does not depend on the state.

@sipa
Copy link
Member

sipa commented Sep 26, 2023

The purpose of having decoy functionality available during the VERSION phase is mostly for future extensions that may extend the number of messages that can be sent in that phase. I agree it's rather pointless right now, but with the prospect of extensions, I think it's cleaner to have it available for the entirety of the phase.

Just as a note, if we'd go for an approach with 2 layers, where some parts use headerless packets and other parts use headerful packets, the length descriptor semantics need to change too (because they're currently excluding the header byte length).

@ajtowns I don't believe there is a benefit to splitting up the garbage into a part sent before and after the other side's key; whatever you sent as garbage part 2 can instead be sent as decoy data (even if only possible after the version packet).

@ajtowns
Copy link
Contributor

ajtowns commented Sep 26, 2023

Just for my understanding: You're saying that the handshake could look more obscure to wiretappers, right? And not that the description of the handshake in the BIP would be more obscure to readers? ;)

Yeah, that's what I mean, and your further reply makes sense. Still kinda think mentioning decoy packets as well as garbage in the 1.5 round trip summary would be clearer; but the rest of the BIP already makes it pretty clear anyway, so 🤷‍♂️

The purpose of having decoy functionality available during the VERSION phase is mostly for future extensions that may extend the number of messages that can be sent in that phase.

Hmm, I guess I look at the layers as:

  • "over the wire":
    • negotiate a key
    • send packets
    • handle rekeying
    • negotiate "application support" for future extensibility
    • do all that while looking like a pseudorandom stream
  • "application layer":
    • is dispatched to different applications (decoy -> /dev/null, bitcoin p2p -> net_processing)
    • application layer just sees packet contents (and the session key)
    • someday, when the decoy "application" actually sends data, it might become an actual complicated application in its own right, eg in order to mimic traffic patterns of some other protocol
    • someday, if we get really worried about obscuring things, messages being sent might go through a third layer that delays them a bit to try to make it harder to do timing analysis or figure out packet boundaries

ie, I guess I think "negotiate for future extensibility" is essentially a "meta application" that's built into the bip324 (or future extension bip) implementation, that while it gets data from packets just like a regular application, also controls how data from packets goes to applications, so it's really a low-level thing, not a high-level thing. Maybe "built-in as part of the implementation" vs "user of the API" is more the distinction that I'm thinking of.

Just as a note, if we'd go for an approach with 2 layers, where some parts use headerless packets and other parts use headerful packets, the length descriptor semantics need to change too (because they're currently excluding the header byte length).

Yeah, I guess that makes sense: better to be able to have a full 0xFF_FFFF bytes of application data in a packet than be off by two instead of just one. I also guess that's easy enough to extend in some future version if it's ever desirable:

  • 16MiB isn't enough: if bit 6 of the header is set, concatenate the contents of the next packet, and repeat, until you can send gigameg blocks back to the application as a single unit
  • 8 bits of header isn't enough: if bit 5 of the header is set, the first however many bytes of the contents is a varint header

LGTM 👍

@sipa
Copy link
Member

sipa commented Sep 26, 2023

I'm really not convinced that separating headerless and headerful packets is worth it.

At a conceptual level you can say:

  • (1) In the BIP right now, garbage auth packets are headerless, while version and application packets are headerful. The rationale arguably is that the initial phase has its own traffic shaping mechanism (garbage) so it has no need for decoy packets anyway. However, the instantiation of the garbage auth packets still used header bytes, leading to the inconsistency (ignoring decoy bits) that led to this PR.
  • (2) In the proposed BIP change in this PR, garbage auth packets are gone, and the remaining packets (version and application) are headerful both conceptually and in the way they are instantiated. While it's true that the version phase right now needs no traffic shaping (because it's so short the shaping can be done by the phase that follows), it's not true conceptually that version negotiation needs no shaping entirely (something that could become apparent with future extensions).
  • (3) In what we're discussing now, version packets become conceptually headerless, but application packets stay headerful. That can be done with or without getting rid of the header byte entirely, but I'm not sure any either is all that appealing:
    • (3a) Version packets retain a header byte, but it is ignored (resulting in a similar inconsistency to how garbage auth packets has ignored header bytes right now).
    • (3b) Version packets have no header byte at all (meaning implementations gain some complexity due to distinguishing the two types of packets). This means changing the meaning of length descriptors (either make them depend on the phase, or deal with application level packets that are too short to contain a header). Version extensions may need a traffic shaping mechanism, but this could be done by enabling headers mid-phase, or having their own ad-hoc shaping mechanism (or extensions could of course overhaul the decoy mechanism entirely).

So while it does make sense to see the version negotiation as the thing that negotiates there being a decoy mechanism, and so logically that phase itself shouldn't already have them, I don't think this practically gains us anything:

  • Right now (ignoring future extensions) the specification and implementation seem simpler with (2) simply because there is only one type of packet, and its semantics are always the same.
  • For future extensions, it may or may not be desirable to have the same decoy mechanism present during the version negotiation, but if it isn't, it can still be changed.

@real-or-random real-or-random force-pushed the 202309-0324-garbauth branch 2 times, most recently from 8738bc8 to 2a32d65 Compare September 27, 2023 09:11
@real-or-random
Copy link
Contributor Author

I've forced-push to address the comments. I'm undrafting the PR because there seems to be broad agreement that we want to do this.

@ajtowns

Still kinda think mentioning decoy packets as well as garbage in the 1.5 round trip summary would be clearer; but the rest of the BIP already makes it pretty clear anyway, so 🤷‍♂️

Ok, that makes sense. We mentioned it everywhere in the BIP except in this summary. Added it to the summary for consistency.

It might be slightly clearer to make the pseudocode more stateful as far as the aad info goes, ie drop it as a parameter to v2_enc_packet and v2_receive_packet and instead initialize it based on peer.sent_garbage and peer.recv_garbage, and clear them after first use?

I tried this, but I'm not convinced that this will make it clearer: Keeping the state local to complete_handshake also has its merits and keeping the AAD logic there keeps it close to where this logic is explained in prose.

@real-or-random real-or-random marked this pull request as ready for review September 27, 2023 09:32
real-or-random added a commit to real-or-random/bitcoin that referenced this pull request Sep 27, 2023
See also bitcoin/bips#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.
@sipa
Copy link
Member

sipa commented Sep 27, 2023

LGTM

@naumenkogs
Copy link
Contributor

ACK

#* At this point, both parties have the same keys, and all further communication proceeds in the form of encrypted packets. Packets have an '''ignore bit''', which makes them '''decoy packets''' if set. Decoy packets are to be ignored by the receiver apart from verifying they decrypt correctly. Either peer may send such decoy packets at any point after this. These form the primary shapability mechanism in the protocol. How and when to use them is out of scope for this document.
#* At this point, both parties have the same keys, and all further communication proceeds in the form of '''encrypted packets'''.
#** Encrypted packets have an '''ignore bit''', which makes them '''decoy packets''' if set. Decoy packets are to be ignored by the receiver apart from verifying they decrypt correctly. Either peer may send such decoy packets at any point from here on. These form the primary shapability mechanism in the protocol. How and when to use them is out of scope for this document.
#** For each of the two directions, the first encrypted packet that will be sent in that direction (regardless of it being a decoy packet or not) will make use of the associated authenticated data (AAD) feature of the AEAD to authenticate the garbage that has been sent in that direction.<ref name="why_garbage_auth">'''Why does the protocol authenticate the garbage?''' Without garbage authentication, the garbage would be modifiable by a third party without consequences. We want to force any active attacker to have to maintain a full protocol state. In addition, such malleability without the consequence of connection termination could enable protocol fingerprinting.</ref>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what kind of fingerprinting will be possible. Maybe it's just me. Can you explain in more detail?

Copy link
Contributor Author

@real-or-random real-or-random Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be very uncommon for a fully encrypted protocol not to abort in case of an attacker modifying data on the wire. So as an attacker, you could flip some bits in the garbage, notice that the connection is not dropped, and then conclude that v2 is running. (edit: Note that I'm touching this sentence.)

This comment was marked as spam.

@naumenkogs
Copy link
Contributor

One thing you may want to improve is making it clear that garbage-terminator is not a part of garbage when doing aad. This is something i had to double-check everywhere manually on my own :) A BIP could be explicit about this.

@Sjors
Copy link
Member

Sjors commented Sep 27, 2023

Concept ACK, but haven't studied the BIP changes, only the code change.

by merging it with the version packet. Or more accurately, by merging
it with the first packet sent after garbage termination, which may be
a decoy packet or the version packet.

The new protocol simplifies implementations:
 - A protocol state machine won't 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.

The reason for having a separate garbage authentication packet was
to materialize the separation of the key exchange phase and version
negotiation phase even in the bytestream on the wire. However, this
is not necessary, and arguably, these phases are still properly
separated: Since the AEAD will ensure that AAD (=garbage) is checked
before looking at the contents (=version), the peers won't interpret
version data before having authenticated the garbage.
@real-or-random
Copy link
Contributor Author

One thing you may want to improve is making it clear that garbage-terminator is not a part of garbage when doing aad. This is something i had to double-check everywhere manually on my own :) A BIP could be explicit about this.

Added "not including the garbage terminator" at an appropriate location in the text.

Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 75dc363 modulo some example code suggestions

send(v2_enc_packet(peer, decoy_content_len * b'\x00', aad=aad))
aad = b''
# Send version packet.
send(v2_enc_packet(peer, TRANSPORT_VERSION, aad=aad))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

aad = b''

Maybe also rename to send_aad.

v2_receive_packet(peer)
# Receive, decode, and ignore version packet.
# This includes skipping decoys and authenticating the received garbage.
v2_receive_packet(peer, aad=received_garbage)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, the v2_receive_packet helper now includes assumptions that only make sense in the handshake.

Maybe instead it can return a type and content? And also not do aad = b''.

So then this code becomes something like:

# Receive, decode, and ignore version packet.
# This includes skipping decoys and authenticating the received garbage.
aad = received_garbage
while true:
  (type, content) = v2_receive_packet(peer, aad=aad)
  aad = b''
  break if type == VERSION
  throw if type != IGNORE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, the v2_receive_packet helper now includes assumptions that only make sense in the handshake.

Does it? Can you spell them out explicitly?

If you're referring to "Only the first packet is expected to have non-empty AAD.", then I think this will also be the case if we ever use AAD for anything else than garbage. Even in that case, you'd always want to skip decoy packets and it's reasonable to include the AAD in the first packet. This assumes that the AAD will always be determined by the received (non-decoy) packets or the outer header (see my response in the Core PR: bitcoin/bitcoin#28525 (comment)).

Copy link
Member

@Sjors Sjors Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how the AAD is used, which you elaborated on here: bitcoin/bitcoin#28525 (comment)

If the AAD is set at the level of traffic shaping then it might be different for decoy and real messages. If it's only used at the application level then I agree it's probably fine to set it in the first (decoy) message.

If we believe the latter is much more likely, then I suppose a simple comment in the helper function would suffice:

# This includes skipping decoys and authenticating the received garbage.
# We assume that for future AAD uses, beyond the handshake, its contents
# is determined before a package is received and is checked in the first
# message, even if that's a decoy.

Copy link
Member

@sipa sipa Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truth is, we have no idea how the AAD would be used in future extensions, or even whether it'll ever get used for anything but garbage authentication.

In light of that, it feels unnecessary to impose a particular "grand design" for future extensions or their interaction with AAD and explain it in the specification. We simply pick a particular design because it's concise and can be rationalized, but I don't think we need to specify future behavior - that feels like an analogue of premature optimization (premature specification?).

"Set the expected AAD when processing garbage + terminator, verify the expected AAD whenever any packet is received, and reset the expected AAD to nothing after verifying a packet" is one concise and simple description, and I can imagine that it remains usable for hypothetical extensions that appropriate AAD for other purposes. And if not, those extensions can just change the specification.

@sipa
Copy link
Member

sipa commented Sep 28, 2023

I consider this ready for merging. @luke-jr @kallewoof

@ajtowns
Copy link
Contributor

ajtowns commented Sep 29, 2023

ACK 75dc363 fwiw

@kallewoof kallewoof merged commit e918b50 into bitcoin:master Sep 29, 2023
real-or-random added a commit to real-or-random/bips that referenced this pull request Sep 29, 2023
achow101 added a commit to bitcoin-core/gui that referenced this pull request Sep 29, 2023
e3720bc net: Simplify v2 recv logic by decoupling AAD from state machine (Tim Ruffing)
b0f5175 net: Drop v2 garbage authentication packet (Tim Ruffing)

Pull request description:

  Note that this is a breaking change, see also bitcoin/bips#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.

ACKs for top commit:
  naumenkogs:
    ACK e3720bc
  sipa:
    ACK e3720bc. Re-ran the v2 transport fuzzer overnight.
  ajtowns:
    ACK e3720bc - simpler and more flexible, nice
  achow101:
    ACK e3720bc
  Sjors:
    utACK e3720bc
  theStack:
    Code-review ACK e3720bc

Tree-SHA512: 16600ed868c8a346828de075c4072e37cf86440751d08ab099fe8b58ff71d8b371a90397d6a4247096796db68794275e7e0403f218859567d04838e0411dadd6
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 5, 2023
e3720bc net: Simplify v2 recv logic by decoupling AAD from state machine (Tim Ruffing)
b0f5175 net: Drop v2 garbage authentication packet (Tim Ruffing)

Pull request description:

  Note that this is a breaking change, see also bitcoin/bips#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.

ACKs for top commit:
  naumenkogs:
    ACK e3720bc
  sipa:
    ACK e3720bc. Re-ran the v2 transport fuzzer overnight.
  ajtowns:
    ACK e3720bc - simpler and more flexible, nice
  achow101:
    ACK e3720bc
  Sjors:
    utACK e3720bc
  theStack:
    Code-review ACK e3720bc

Tree-SHA512: 16600ed868c8a346828de075c4072e37cf86440751d08ab099fe8b58ff71d8b371a90397d6a4247096796db68794275e7e0403f218859567d04838e0411dadd6
</pre>
Upon receiving the encoded responder public key, the initiator derives the shared ECDH secret and instantiates the encrypted transport. It then sends the derived 16-byte <code>initiator_garbage_terminator</code> followed by an authenticated, encrypted packet with empty contents<ref name="send_empty_garbauth">'''Does the content of the garbage authentication packet need to be empty?''' The receiver ignores the content of the garbage authentication packet, so its content can be anything, and it can in principle be used as a shaping mechanism too. There is however no need for that, as immediately afterward the initiator can start using decoy packets as (a much more flexible) shaping mechanism instead.</ref> to authenticate the garbage, and its own version packet. It then receives the responder's garbage and garbage authentication packet (delimited by the garbage terminator), and checks if the garbage is authenticated correctly. The responder performs very similar steps but includes the earlier received prefix bytes in the public key. As mentioned before, the encrypted packets for the '''version negotiation phase''' can be piggybacked with the garbage authentication packet to minimize roundtrips.
Upon receiving the encoded responder public key, the initiator derives the shared ECDH secret and instantiates the encrypted transport. It then sends the derived 16-byte <code>initiator_garbage_terminator</code>, optionally followed by an arbitrary number of decoy packets. Afterwards, it receives the responder's garbage (delimited by the garbage terminator). The responder performs very similar steps but includes the earlier received prefix bytes in the public key. Both the initiator and the responder set the AAD of the first encrypted packet they send after the garbage terminator (i.e., either an optional decoy packet or the version packet) to the garbage they have just sent, not including the garbage terminator.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since there's no limit on the number of decoy packets that a peer can send (after sending the garbage terminator, before sending the version packet), wouldn't we want to disconnect from the handshake at some point - so that the connection can be dropped and we use that slot for a more useful peer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @sipa

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stratospher We certainly do want to disconnect useless peers, but I don't think the transport layer is the right place to address that. After all, peers can be equally useless by just behaving weirdly at the application level (e.g. never do anything except respond to pings). In Bitcoin Core, connection rotation and inbound eviction already deal with these cases (but can be improved, of course), and apply just fine here, because if a node only sends decoys, the application layer will just see a connection that never sends a version/verack packet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! makes sense.

janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Apr 1, 2024
See also bitcoin/bips#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants