Follow-up to BIP324 connection support

sipa commented at 7:25 pm on September 8, 2023: member

This addresses a few remaining comments on #28196:

Deduplicate the V2Transport constructors (https://github.com/bitcoin/bitcoin/pull/28196#discussion_r1318573111)
Do not use the send buffer to store garbage (https://github.com/bitcoin/bitcoin/pull/28196#discussion_r1319134141)
Fix typo (https://github.com/bitcoin/bitcoin/pull/28196#discussion_r1315179378)

In addition, also fix an incorrect description in V2Transport::SendState (it claimed garbage was sent in the READY state, but it’s in the AWAITING_KEY state).

DrahtBot commented at 7:26 pm on September 8, 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, naumenkogs
Stale ACK	Sjors

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28331 (BIP324 integration by sipa)
#24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa force-pushed on Sep 8, 2023

DrahtBot added the label CI failed on Sep 8, 2023

sipa force-pushed on Sep 8, 2023

Sjors commented at 8:23 pm on September 8, 2023: member

I’m not sure what you mean with this commit message:

This allows us to write the random-key V2Transport constructor in function of the explicit-key one.

sipa commented at 8:26 pm on September 8, 2023: member

I’m not sure what you mean with this commit message:

This allows us to write the random-key V2Transport constructor in function of the explicit-key one.

It lets us write V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept using delegation to V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span<const std::byte> ent32, std::vector<uint8_t> garbage) noexcept.

sipa force-pushed on Sep 8, 2023

in src/net.cpp:1005 in 73ad8bbcd2 outdated

 996@@ -997,17 +997,29 @@ std::vector<uint8_t> GenerateRandomGarbage() noexcept
 997 
 998 } // namespace
 999 
1000-V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span<const std::byte> ent32, Span<const uint8_t> garbage) noexcept :
1001+void V2Transport::StartSendingHandshake() noexcept
1002+{
1003+    AssertLockHeld(m_send_mutex);
1004+    Assume(m_send_state == SendState::AWAITING_KEY);
1005+    // Initialize the send buffer with ellswift pubkey + provided garbage.

Sjors commented at 2:05 pm on September 9, 2023:

73ad8bbcd23c6bdf3bde0481ae39633ba5b38c1e: maybe add an Assume that the send buffer was still empty at this point

sipa commented at 2:59 pm on September 9, 2023:

Done.

in src/net.cpp:1008 in 73ad8bbcd2 outdated

1003+    AssertLockHeld(m_send_mutex);
1004+    Assume(m_send_state == SendState::AWAITING_KEY);
1005+    // Initialize the send buffer with ellswift pubkey + provided garbage.
1006+    m_send_buffer.resize(EllSwiftPubKey::size() + m_send_garbage.size());
1007+    std::copy(std::begin(m_cipher.GetOurPubKey()), std::end(m_cipher.GetOurPubKey()), MakeWritableByteSpan(m_send_buffer).begin());
1008+    std::copy(m_send_garbage.begin(), m_send_garbage.end(), m_send_buffer.begin() + EllSwiftPubKey::size());

Sjors commented at 2:07 pm on September 9, 2023:

73ad8bbcd23c6bdf3bde0481ae39633ba5b38c1e: could add a note here that we can’t clear m_send_garbage yet because we haven’t constructed the garbage authentication. (could just move the comment you deleted below)

sipa commented at 2:58 pm on September 9, 2023:

Done.

in src/net.cpp:1537 in 73ad8bbcd2 outdated

1550@@ -1537,9 +1551,6 @@ Transport::BytesToSend V2Transport::GetBytesToSend(bool have_next_message) const
1551     LOCK(m_send_mutex);
1552     if (m_send_state == SendState::V1) return m_v1_fallback.GetBytesToSend(have_next_message);
1553 
1554-    // We do not send anything in MAYBE_V1 state (as we don't know if the peer is v1 or v2),
1555-    // despite there being data in the send buffer in that state.
1556-    if (m_send_state == SendState::MAYBE_V1) return {{}, false, m_send_type};

Sjors commented at 2:12 pm on September 9, 2023:

73ad8bbcd23c6bdf3bde0481ae39633ba5b38c1e: maybe sanity check:

0if (m_send_state == SendState::MAYBE_V1) Assume(m_send_buffer.size() == 0);

sipa commented at 2:58 pm on September 9, 2023:

Done.

Sjors approved

Sjors commented at 2:21 pm on September 9, 2023: member

utACK 2d46e5c01b04a18a08c6088f6ee5d490d4f704e6

(I also cherry-picked this on the integration PR and running that now)

sipa force-pushed on Sep 9, 2023

Sjors commented at 3:41 pm on September 9, 2023: member

re-utACK b45a16b95271048281fc762f2f79d850e8efe954

sipa force-pushed on Sep 10, 2023

DrahtBot removed the label CI failed on Sep 10, 2023

net: merge V2Transport constructors, move key gen

This removes the ability for BIP324Cipher to generate its own key, moving that
responsibility to the caller (mostly, V2Transport). This allows us to write
the random-key V2Transport constructor by delegating to the explicit-key one.

b6934fd03f

net: do not use send buffer to store/cache garbage

Before this commit the V2Transport::m_send_buffer is used to store the
garbage:
* During MAYBE_V1 state, it's there despite not being sent.
* During AWAITING_KEY state, while it is being sent.
* At the end of the AWAITING_KEY state it cannot be wiped as it's still
  needed to compute the garbage authentication packet.

Change this by introducing a separate m_send_garbage field, taking over
the first and last role listed above. This means the garbage is only in
the send buffer when it's actually being sent, removing a few special
cases related to this.

9bde93df2c

doc: fix typos and mistakes in BIP324 code comments 64704386b2

sipa force-pushed on Sep 10, 2023

theStack approved

theStack commented at 10:07 pm on September 10, 2023: contributor

Code-review ACK 64704386b28ce3a1ab607a946ec729286da8faa6

In terms of readability I think it’s nice that there are now dedicated functions/methods for generating garbage and putting pubkey+garbage into the send buffer. The GenerateRandomKey helper might be useful in other places too (unit tests, wallet etc.), but that’s out of scope for this PR; it can easily be moved to some shared moudule in e.g. src/util (or even directly key.h/key.cpp) later if needed.

DrahtBot requested review from Sjors on Sep 10, 2023

naumenkogs commented at 7:25 am on September 11, 2023: member

ACK 64704386b28ce3a1ab607a946ec729286da8faa6

fanquake merged this on Sep 11, 2023

fanquake closed this on Sep 11, 2023

Frank-GER referenced this in commit 4bbfc7f0b9 on Sep 19, 2023

achow101 referenced this in commit 265250687b on Jan 2, 2024

bitcoin locked this on Sep 10, 2024

Follow-up to BIP324 connection support #28433

Reviews

Conflicts