Merge bitcoin/bitcoin#28433: Follow-up to BIP324 connection support

6470438 doc: fix typos and mistakes in BIP324 code comments (Pieter Wuille)
9bde93d net: do not use send buffer to store/cache garbage (Pieter Wuille)
b6934fd net: merge V2Transport constructors, move key gen (Pieter Wuille)

Pull request description:

  This addresses a few remaining comments on #28196:

  * Deduplicate the `V2Transport` constructors (bitcoin/bitcoin#28196 (comment))
  * Do not use the send buffer to store garbage (bitcoin/bitcoin#28196 (comment))
  * Fix typo (bitcoin/bitcoin#28196 (comment))

  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).

ACKs for top commit:
  naumenkogs:
    ACK 6470438
  theStack:
    Code-review ACK 6470438

Tree-SHA512: 4bf6d2fe73c8054502d0b60e9de1722f8b3dd269c2dd6bf67197c3fb6eabcf047b6360cdab3c1fd5504215c2ac4ac2890a022780efc30ff583776242c8112451

Author: fanquake

src/bip324.cpp

@@ -22,13 +22,6 @@
 #include <iterator>
 #include <string>
 
-BIP324Cipher::BIP324Cipher() noexcept
-{
-    m_key.MakeNewKey(true);
-    uint256 entropy = GetRandHash();
-    m_our_pubkey = m_key.EllSwiftCreate(MakeByteSpan(entropy));
-}
-
 BIP324Cipher::BIP324Cipher(const CKey& key, Span<const std::byte> ent32) noexcept :
     m_key(key)
 {

src/bip324.h

@@ -41,8 +41,8 @@ class BIP324Cipher
     std::array<std::byte, GARBAGE_TERMINATOR_LEN> m_recv_garbage_terminator;
 
 public:
-    /** Initialize a BIP324 cipher with securely generated random keys. */
-    BIP324Cipher() noexcept;
+    /** No default constructor; keys must be provided to create a BIP324Cipher. */
+    BIP324Cipher() = delete;
 
     /** Initialize a BIP324 cipher with specified key and encoding entropy (testing only). */
     BIP324Cipher(const CKey& key, Span<const std::byte> ent32) noexcept;

src/net.cpp

@@ -979,36 +979,56 @@ class V2MessageMap
 
 const V2MessageMap V2_MESSAGE_MAP;
 
-} // namespace
+CKey GenerateRandomKey() noexcept
+{
+    CKey key;
+    key.MakeNewKey(/*fCompressed=*/true);
+    return key;
+}
 
-V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept :
-    m_cipher{}, m_initiating{initiating}, m_nodeid{nodeid},
-    m_v1_fallback{nodeid, type_in, version_in}, m_recv_type{type_in}, m_recv_version{version_in},
-    m_recv_state{initiating ? RecvState::KEY : RecvState::KEY_MAYBE_V1},
-    m_send_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1}
+std::vector<uint8_t> GenerateRandomGarbage() noexcept
 {
-    // Construct garbage (including its length) using a FastRandomContext.
+    std::vector<uint8_t> ret;
     FastRandomContext rng;
-    size_t garbage_len = rng.randrange(MAX_GARBAGE_LEN + 1);
-    // Initialize the send buffer with ellswift pubkey + garbage.
-    m_send_buffer.resize(EllSwiftPubKey::size() + garbage_len);
+    ret.resize(rng.randrange(V2Transport::MAX_GARBAGE_LEN + 1));
+    rng.fillrand(MakeWritableByteSpan(ret));
+    return ret;
+}
+
+} // namespace
+
+void V2Transport::StartSendingHandshake() noexcept
+{
+    AssertLockHeld(m_send_mutex);
+    Assume(m_send_state == SendState::AWAITING_KEY);
+    Assume(m_send_buffer.empty());
+    // Initialize the send buffer with ellswift pubkey + provided garbage.
+    m_send_buffer.resize(EllSwiftPubKey::size() + m_send_garbage.size());
     std::copy(std::begin(m_cipher.GetOurPubKey()), std::end(m_cipher.GetOurPubKey()), MakeWritableByteSpan(m_send_buffer).begin());
-    rng.fillrand(MakeWritableByteSpan(m_send_buffer).subspan(EllSwiftPubKey::size()));
+    std::copy(m_send_garbage.begin(), m_send_garbage.end(), m_send_buffer.begin() + EllSwiftPubKey::size());
+    // We cannot wipe m_send_garbage as it will still be used to construct the garbage
+    // authentication packet.
 }
 
-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 :
+V2Transport::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 :
     m_cipher{key, ent32}, m_initiating{initiating}, m_nodeid{nodeid},
     m_v1_fallback{nodeid, type_in, version_in}, m_recv_type{type_in}, m_recv_version{version_in},
     m_recv_state{initiating ? RecvState::KEY : RecvState::KEY_MAYBE_V1},
+    m_send_garbage{std::move(garbage)},
     m_send_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1}
 {
-    assert(garbage.size() <= MAX_GARBAGE_LEN);
-    // Initialize the send buffer with ellswift pubkey + provided garbage.
-    m_send_buffer.resize(EllSwiftPubKey::size() + garbage.size());
-    std::copy(std::begin(m_cipher.GetOurPubKey()), std::end(m_cipher.GetOurPubKey()), MakeWritableByteSpan(m_send_buffer).begin());
-    std::copy(garbage.begin(), garbage.end(), m_send_buffer.begin() + EllSwiftPubKey::size());
+    Assume(m_send_garbage.size() <= MAX_GARBAGE_LEN);
+    // Start sending immediately if we're the initiator of the connection.
+    if (initiating) {
+        LOCK(m_send_mutex);
+        StartSendingHandshake();
+    }
 }
 
+V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept :
+    V2Transport{nodeid, initiating, type_in, version_in, GenerateRandomKey(),
+                MakeByteSpan(GetRandHash()), GenerateRandomGarbage()} { }
+
 void V2Transport::SetReceiveState(RecvState recv_state) noexcept
 {
     AssertLockHeld(m_recv_mutex);
@@ -1087,9 +1107,10 @@ void V2Transport::ProcessReceivedMaybeV1Bytes() noexcept
     if (!std::equal(m_recv_buffer.begin(), m_recv_buffer.end(), v1_prefix.begin())) {
         // Mismatch with v1 prefix, so we can assume a v2 connection.
         SetReceiveState(RecvState::KEY); // Convert to KEY state, leaving received bytes around.
-        // Transition the sender to AWAITING_KEY state (if not already).
+        // Transition the sender to AWAITING_KEY state and start sending.
         LOCK(m_send_mutex);
         SetSendState(SendState::AWAITING_KEY);
+        StartSendingHandshake();
     } else if (m_recv_buffer.size() == v1_prefix.size()) {
         // Full match with the v1 prefix, so fall back to v1 behavior.
         LOCK(m_send_mutex);
@@ -1149,7 +1170,6 @@ bool V2Transport::ProcessReceivedKeyBytes() noexcept
         SetSendState(SendState::READY);
 
         // Append the garbage terminator to the send buffer.
-        size_t garbage_len = m_send_buffer.size() - EllSwiftPubKey::size();
         m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::GARBAGE_TERMINATOR_LEN);
         std::copy(m_cipher.GetSendGarbageTerminator().begin(),
                   m_cipher.GetSendGarbageTerminator().end(),
@@ -1160,9 +1180,12 @@ bool V2Transport::ProcessReceivedKeyBytes() noexcept
         m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::EXPANSION);
         m_cipher.Encrypt(
             /*contents=*/{},
-            /*aad=*/MakeByteSpan(m_send_buffer).subspan(EllSwiftPubKey::size(), garbage_len),
+            /*aad=*/MakeByteSpan(m_send_garbage),
             /*ignore=*/false,
             /*output=*/MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::EXPANSION));
+        // We no longer need the garbage.
+        m_send_garbage.clear();
+        m_send_garbage.shrink_to_fit();
 
         // Construct version packet in the send buffer.
         m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::EXPANSION + VERSION_CONTENTS.size());
@@ -1532,9 +1555,7 @@ Transport::BytesToSend V2Transport::GetBytesToSend(bool have_next_message) const
     LOCK(m_send_mutex);
     if (m_send_state == SendState::V1) return m_v1_fallback.GetBytesToSend(have_next_message);
 
-    // We do not send anything in MAYBE_V1 state (as we don't know if the peer is v1 or v2),
-    // despite there being data in the send buffer in that state.
-    if (m_send_state == SendState::MAYBE_V1) return {{}, false, m_send_type};
+    if (m_send_state == SendState::MAYBE_V1) Assume(m_send_buffer.empty());
     Assume(m_send_pos <= m_send_buffer.size());
     return {
         Span{m_send_buffer}.subspan(m_send_pos),
@@ -1553,10 +1574,8 @@ void V2Transport::MarkBytesSent(size_t bytes_sent) noexcept
 
     m_send_pos += bytes_sent;
     Assume(m_send_pos <= m_send_buffer.size());
-    // Only wipe the buffer when everything is sent in the READY state. In the AWAITING_KEY state
-    // we still need the garbage that's in the send buffer to construct the garbage authentication
-    // packet.
-    if (m_send_state == SendState::READY && m_send_pos == m_send_buffer.size()) {
+    // Wipe the buffer when everything is sent.
+    if (m_send_pos == m_send_buffer.size()) {
         m_send_pos = 0;
         m_send_buffer = {};
     }

src/net.h

@@ -540,25 +540,25 @@ class V2Transport final : public Transport
     enum class SendState : uint8_t {
         /** (Responder only) Not sending until v1 or v2 is detected.
          *
-         * This is the initial state for responders. The send buffer contains the public key to
-         * send, but nothing is sent in this state yet. When the receiver determines whether this
+         * This is the initial state for responders. The send buffer is empty.
+         * When the receiver determines whether this
          * is a V1 or V2 connection, the sender state becomes AWAITING_KEY (for v2) or V1 (for v1).
          */
         MAYBE_V1,
 
         /** Waiting for the other side's public key.
          *
-         * This is the initial state for initiators. The public key is sent out. When the receiver
-         * receives the other side's public key and transitions to GARB_GARBTERM, the sender state
-         * becomes READY. */
+         * This is the initial state for initiators. The public key and garbage is sent out. When
+         * the receiver receives the other side's public key and transitions to GARB_GARBTERM, the
+         * sender state becomes READY. */
         AWAITING_KEY,
 
         /** Normal sending state.
          *
          * In this state, the ciphers are initialized, so packets can be sent. When this state is
-         * entered, the garbage, garbage terminator, garbage authentication packet, and version
-         * packet are appended to the send buffer (in addition to the key which may still be
-         * there). In this state a message can be provided if the send buffer is empty. */
+         * entered, the garbage terminator, garbage authentication packet, and version
+         * packet are appended to the send buffer (in addition to the key and garbage which may
+         * still be there). In this state a message can be provided if the send buffer is empty. */
         READY,
 
         /** This transport is using v1 fallback.
@@ -601,6 +601,8 @@ class V2Transport final : public Transport
     std::vector<uint8_t> m_send_buffer GUARDED_BY(m_send_mutex);
     /** How many bytes from the send buffer have been sent so far. */
     uint32_t m_send_pos GUARDED_BY(m_send_mutex) {0};
+    /** The garbage sent, or to be sent (MAYBE_V1 and AWAITING_KEY state only). */
+    std::vector<uint8_t> m_send_garbage GUARDED_BY(m_send_mutex);
     /** Type of the message being sent. */
     std::string m_send_type GUARDED_BY(m_send_mutex);
     /** Current sender state. */
@@ -614,6 +616,8 @@ class V2Transport final : public Transport
     static std::optional<std::string> GetMessageType(Span<const uint8_t>& contents) noexcept;
     /** Determine how many received bytes can be processed in one go (not allowed in V1 state). */
     size_t GetMaxBytesToProcess() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_recv_mutex);
+    /** Put our public key + garbage in the send buffer. */
+    void StartSendingHandshake() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_send_mutex);
     /** Process bytes in m_recv_buffer, while in KEY_MAYBE_V1 state. */
     void ProcessReceivedMaybeV1Bytes() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_recv_mutex, !m_send_mutex);
     /** Process bytes in m_recv_buffer, while in KEY state. */
@@ -636,7 +640,7 @@ class V2Transport final : public Transport
     V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept;
 
     /** Construct a V2 transport with specified keys and garbage (test use only). */
-    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;
+    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;
 
     // Receive side functions.
     bool ReceivedMessageComplete() const noexcept override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex);

src/test/fuzz/p2p_transport_serialization.cpp

@@ -366,7 +366,7 @@ std::unique_ptr<Transport> MakeV2Transport(NodeId nodeid, bool initiator, RNG& r
              .Write(garb.data(), garb.size())
              .Finalize(UCharCast(ent.data()));
 
-    return std::make_unique<V2Transport>(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, garb);
+    return std::make_unique<V2Transport>(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, std::move(garb));
 }
 
 } // namespace

src/test/net_tests.cpp

@@ -1008,12 +1008,20 @@ BOOST_AUTO_TEST_CASE(advertise_local_address)
 
 namespace {
 
+CKey GenerateRandomTestKey() noexcept
+{
+    CKey key;
+    uint256 key_data = InsecureRand256();
+    key.Set(key_data.begin(), key_data.end(), true);
+    return key;
+}
+
 /** A class for scenario-based tests of V2Transport
  *
  * Each V2TransportTester encapsulates a V2Transport (the one being tested), and can be told to
  * interact with it. To do so, it also encapsulates a BIP324Cipher to act as the other side. A
  * second V2Transport is not used, as doing so would not permit scenarios that involve sending
- * invalid data, or ones scenarios using BIP324 features that are not implemented on the sending
+ * invalid data, or ones using BIP324 features that are not implemented on the sending
  * side (like decoy packets).
  */
 class V2TransportTester
@@ -1031,6 +1039,7 @@ class V2TransportTester
     /** Construct a tester object. test_initiator: whether the tested transport is initiator. */
     V2TransportTester(bool test_initiator) :
         m_transport(0, test_initiator, SER_NETWORK, INIT_PROTO_VERSION),
+        m_cipher{GenerateRandomTestKey(), MakeByteSpan(InsecureRand256())},
         m_test_initiator(test_initiator) {}
 
     /** Data type returned by Interact: