Note that this is a breaking change, see also https://github.com/bitcoin/bips/pull/1498
The benefit is a simpler implementation:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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
further packets are expected to use the empty string as AAD.
this part is irrelevant here?
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.)
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
non-decoy decoy? Not sure if this is intentional, although i can see some sense in this… :)
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
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?
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.
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?)
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:
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.
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?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. */
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.
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?
1194@@ -1193,25 +1195,26 @@ class V2TransportTester
1195
1196 /** Schedule garbage terminator and authentication packet to be sent to the transport (only
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.
test_framework.authproxy.JSONRPCException: Unable to create transaction. Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate) (-4). edit: Okay, this is #28491.
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);
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)
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
Assumethatm_recv_aadis 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.
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_garbagearound and explicitly move it intom_recv_aadas 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.
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.
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?
I suppose an
AssumeinAPPmay 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.)
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.
Assume can wait.
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.
utACK e3720bca398820038b3e97f467adb2c45ef9ef5f
Timing wise, I plan to run this on mainnet once it’s added to the integration PR.
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).
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