Merge bitcoin/bitcoin#28196: BIP324 connection support

db9888f net: detect wrong-network V1 talking to V2Transport (Pieter Wuille)
91e1ef8 test: add unit tests for V2Transport (Pieter Wuille)
297c888 net: make V2Transport preallocate receive buffer space (Pieter Wuille)
3ffa5fb net: make V2Transport send uniformly random number garbage bytes (Pieter Wuille)
0be752d net: add short message encoding/decoding support to V2Transport (Pieter Wuille)
8da8642 net: make V2Transport auto-detect incoming V1 and fall back to it (Pieter Wuille)
13a7f01 net: add V2Transport class with subset of BIP324 functionality (Pieter Wuille)
dc2d7eb crypto: Spanify EllSwiftPubKey constructor (Pieter Wuille)
5f4b2c6 net: remove unused Transport::SetReceiveVersion (Pieter Wuille)
c3fad1f net: add have_next_message argument to Transport::GetBytesToSend() (Pieter Wuille)

Pull request description:

  This is part of #27634.

  This implements the BIP324 v2 transport (which implements all of what the BIP calls transport layer *and* application layer), though in a non-exposed way. It is tested through an extensive fuzz test, which verifies that v2 transports can talk to v2 transports, and v1 transports can talk to v2 transports, and a unit test that exercises a number of unusual scenarios. The transport is functionally complete, including:
  * Autodetection of incoming V1 connections.
  * Garbage, both sending and receiving.
  * Short message type IDs, both sending and receiving.
  * Ignore packets (receiving only, but tested in a unit test).
  * Session IDs are visible in `getpeerinfo` output (for manual comparison).

  Things that are not included, left for future PRs, are:
  * Actually using the v2 transport for connections.
  * Support for the `NODE_P2P_V2` service flag.
  * Retrying downgrade to V1 when attempted outbound V2 connections immediately fail.
  * P2P functional and unit tests

ACKs for top commit:
  naumenkogs:
    ACK db9888f
  theStack:
    re-ACK db9888f
  mzumsande:
    Code Review ACK db9888f

Tree-SHA512: 8906ac1e733a99e1f31c9111055611f706d80bbfc2edf6a07fa6e47b21bb65baacd1ff17993cbbf588063b2f5ad30b3af674a50c7bc8e8ebf4671483a21bbfeb

src/pubkey.cpp

@@ -336,6 +336,12 @@ bool CPubKey::Derive(CPubKey& pubkeyChild, ChainCode &ccChild, unsigned int nChi
     return true;
 }
 
+EllSwiftPubKey::EllSwiftPubKey(Span<const std::byte> ellswift) noexcept
+{
+    assert(ellswift.size() == SIZE);
+    std::copy(ellswift.begin(), ellswift.end(), m_pubkey.begin());
+}
+
 CPubKey EllSwiftPubKey::Decode() const
 {
     secp256k1_pubkey pubkey;

src/pubkey.h

@@ -303,8 +303,7 @@ struct EllSwiftPubKey
     EllSwiftPubKey() noexcept = default;
 
     /** Construct a new ellswift public key from a given serialization. */
-    EllSwiftPubKey(const std::array<std::byte, SIZE>& ellswift) :
-        m_pubkey(ellswift) {}
+    EllSwiftPubKey(Span<const std::byte> ellswift) noexcept;
 
     /** Decode to normal compressed CPubKey (for debugging purposes). */
     CPubKey Decode() const;

src/test/bip324_tests.cpp

@@ -38,14 +38,8 @@ void TestBIP324PacketVector(
 {
     // Convert input from hex to char/byte vectors/arrays.
     const auto in_priv_ours = ParseHex(in_priv_ours_hex);
-    const auto in_ellswift_ours_vec = ParseHex<std::byte>(in_ellswift_ours_hex);
-    assert(in_ellswift_ours_vec.size() == 64);
-    std::array<std::byte, 64> in_ellswift_ours;
-    std::copy(in_ellswift_ours_vec.begin(), in_ellswift_ours_vec.end(), in_ellswift_ours.begin());
-    const auto in_ellswift_theirs_vec = ParseHex<std::byte>(in_ellswift_theirs_hex);
-    assert(in_ellswift_theirs_vec.size() == 64);
-    std::array<std::byte, 64> in_ellswift_theirs;
-    std::copy(in_ellswift_theirs_vec.begin(), in_ellswift_theirs_vec.end(), in_ellswift_theirs.begin());
+    const auto in_ellswift_ours = ParseHex<std::byte>(in_ellswift_ours_hex);
+    const auto in_ellswift_theirs = ParseHex<std::byte>(in_ellswift_theirs_hex);
     const auto in_contents = ParseHex<std::byte>(in_contents_hex);
     const auto in_aad = ParseHex<std::byte>(in_aad_hex);
     const auto mid_send_garbage = ParseHex<std::byte>(mid_send_garbage_hex);

src/test/denialofservice_tests.cpp

@@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 
     {
         LOCK(dummyNode1.cs_vSend);
-        const auto& [to_send, _more, _msg_type] = dummyNode1.m_transport->GetBytesToSend();
+        const auto& [to_send, _more, _msg_type] = dummyNode1.m_transport->GetBytesToSend(false);
         BOOST_CHECK(!to_send.empty());
     }
     connman.FlushSendBuffer(dummyNode1);
@@ -97,7 +97,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
     {
         LOCK(dummyNode1.cs_vSend);
-        const auto& [to_send, _more, _msg_type] = dummyNode1.m_transport->GetBytesToSend();
+        const auto& [to_send, _more, _msg_type] = dummyNode1.m_transport->GetBytesToSend(false);
         BOOST_CHECK(!to_send.empty());
     }
     // Wait 3 more minutes

src/test/fuzz/p2p_transport_serialization.cpp

@@ -25,6 +25,7 @@ std::vector<std::string> g_all_messages;
 
 void initialize_p2p_transport_serialization()
 {
+    ECC_Start();
     SelectParams(ChainType::REGTEST);
     g_all_messages = getAllNetMessageTypes();
     std::sort(g_all_messages.begin(), g_all_messages.end());
@@ -92,7 +93,7 @@ FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serial
             assert(queued);
             std::optional<bool> known_more;
             while (true) {
-                const auto& [to_send, more, _msg_type] = send_transport.GetBytesToSend();
+                const auto& [to_send, more, _msg_type] = send_transport.GetBytesToSend(false);
                 if (known_more) assert(!to_send.empty() == *known_more);
                 if (to_send.empty()) break;
                 send_transport.MarkBytesSent(to_send.size());
@@ -124,11 +125,13 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa
     // Vectors with bytes last returned by GetBytesToSend() on transport[i].
     std::array<std::vector<uint8_t>, 2> to_send;
 
-    // Last returned 'more' values (if still relevant) by transport[i]->GetBytesToSend().
-    std::array<std::optional<bool>, 2> last_more;
+    // Last returned 'more' values (if still relevant) by transport[i]->GetBytesToSend(), for
+    // both have_next_message false and true.
+    std::array<std::optional<bool>, 2> last_more, last_more_next;
 
-    // Whether more bytes to be sent are expected on transport[i].
-    std::array<std::optional<bool>, 2> expect_more;
+    // Whether more bytes to be sent are expected on transport[i], before and after
+    // SetMessageToSend().
+    std::array<std::optional<bool>, 2> expect_more, expect_more_next;
 
     // Function to consume a message type.
     auto msg_type_fn = [&]() {
@@ -177,18 +180,27 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa
 
     // Wrapper around transport[i]->GetBytesToSend() that performs sanity checks.
     auto bytes_to_send_fn = [&](int side) -> Transport::BytesToSend {
-        const auto& [bytes, more, msg_type] = transports[side]->GetBytesToSend();
+        // Invoke GetBytesToSend twice (for have_next_message = {false, true}). This function does
+        // not modify state (it's const), and only the "more" return value should differ between
+        // the calls.
+        const auto& [bytes, more_nonext, msg_type] = transports[side]->GetBytesToSend(false);
+        const auto& [bytes_next, more_next, msg_type_next] = transports[side]->GetBytesToSend(true);
         // Compare with expected more.
         if (expect_more[side].has_value()) assert(!bytes.empty() == *expect_more[side]);
+        // Verify consistency between the two results.
+        assert(bytes == bytes_next);
+        assert(msg_type == msg_type_next);
+        if (more_nonext) assert(more_next);
         // Compare with previously reported output.
         assert(to_send[side].size() <= bytes.size());
         assert(to_send[side] == Span{bytes}.first(to_send[side].size()));
         to_send[side].resize(bytes.size());
         std::copy(bytes.begin(), bytes.end(), to_send[side].begin());
-        // Remember 'more' result.
-        last_more[side] = {more};
+        // Remember 'more' results.
+        last_more[side] = {more_nonext};
+        last_more_next[side] = {more_next};
         // Return.
-        return {bytes, more, msg_type};
+        return {bytes, more_nonext, msg_type};
     };
 
     // Function to make side send a new message.
@@ -199,7 +211,8 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa
         CSerializedNetMsg msg = next_msg[side].Copy();
         bool queued = transports[side]->SetMessageToSend(msg);
         // Update expected more data.
-        expect_more[side] = std::nullopt;
+        expect_more[side] = expect_more_next[side];
+        expect_more_next[side] = std::nullopt;
         // Verify consistency of GetBytesToSend after SetMessageToSend
         bytes_to_send_fn(/*side=*/side);
         if (queued) {
@@ -223,6 +236,7 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa
         // If all to-be-sent bytes were sent, move last_more data to expect_more data.
         if (send_now == bytes.size()) {
             expect_more[side] = last_more[side];
+            expect_more_next[side] = last_more_next[side];
         }
         // Remove the bytes from the last reported to-be-sent vector.
         assert(to_send[side].size() >= send_now);
@@ -251,6 +265,7 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa
             // Clear cached expected 'more' information: if certainly no more data was to be sent
             // before, receiving bytes makes this uncertain.
             if (expect_more[!side] == false) expect_more[!side] = std::nullopt;
+            if (expect_more_next[!side] == false) expect_more_next[!side] = std::nullopt;
             // Verify consistency of GetBytesToSend after ReceivedBytes
             bytes_to_send_fn(/*side=*/!side);
             bool progress = to_recv.size() < old_len;
@@ -320,6 +335,40 @@ std::unique_ptr<Transport> MakeV1Transport(NodeId nodeid) noexcept
     return std::make_unique<V1Transport>(nodeid, SER_NETWORK, INIT_PROTO_VERSION);
 }
 
+template<typename RNG>
+std::unique_ptr<Transport> MakeV2Transport(NodeId nodeid, bool initiator, RNG& rng, FuzzedDataProvider& provider)
+{
+    // Retrieve key
+    auto key = ConsumePrivateKey(provider);
+    if (!key.IsValid()) return {};
+    // Construct garbage
+    size_t garb_len = provider.ConsumeIntegralInRange<size_t>(0, V2Transport::MAX_GARBAGE_LEN);
+    std::vector<uint8_t> garb;
+    if (garb_len <= 64) {
+        // When the garbage length is up to 64 bytes, read it directly from the fuzzer input.
+        garb = provider.ConsumeBytes<uint8_t>(garb_len);
+        garb.resize(garb_len);
+    } else {
+        // If it's longer, generate it from the RNG. This avoids having large amounts of
+        // (hopefully) irrelevant data needing to be stored in the fuzzer data.
+        for (auto& v : garb) v = uint8_t(rng());
+    }
+    // Retrieve entropy
+    auto ent = provider.ConsumeBytes<std::byte>(32);
+    ent.resize(32);
+    // Use as entropy SHA256(ent || garbage). This prevents a situation where the fuzzer manages to
+    // include the garbage terminator (which is a function of both ellswift keys) in the garbage.
+    // This is extremely unlikely (~2^-116) with random keys/garbage, but the fuzzer can choose
+    // both non-randomly and dependently. Since the entropy is hashed anyway inside the ellswift
+    // computation, no coverage should be lost by using a hash as entropy, and it removes the
+    // possibility of garbage that happens to contain what is effectively a hash of the keys.
+    CSHA256().Write(UCharCast(ent.data()), ent.size())
+             .Write(garb.data(), garb.size())
+             .Finalize(UCharCast(ent.data()));
+
+    return std::make_unique<V2Transport>(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, garb);
+}
+
 } // namespace
 
 FUZZ_TARGET(p2p_transport_bidirectional, .init = initialize_p2p_transport_serialization)
@@ -332,3 +381,25 @@ FUZZ_TARGET(p2p_transport_bidirectional, .init = initialize_p2p_transport_serial
     if (!t1 || !t2) return;
     SimulationTest(*t1, *t2, rng, provider);
 }
+
+FUZZ_TARGET(p2p_transport_bidirectional_v2, .init = initialize_p2p_transport_serialization)
+{
+    // Test with two V2 transports talking to each other.
+    FuzzedDataProvider provider{buffer.data(), buffer.size()};
+    XoRoShiRo128PlusPlus rng(provider.ConsumeIntegral<uint64_t>());
+    auto t1 = MakeV2Transport(NodeId{0}, true, rng, provider);
+    auto t2 = MakeV2Transport(NodeId{1}, false, rng, provider);
+    if (!t1 || !t2) return;
+    SimulationTest(*t1, *t2, rng, provider);
+}
+
+FUZZ_TARGET(p2p_transport_bidirectional_v1v2, .init = initialize_p2p_transport_serialization)
+{
+    // Test with a V1 initiator talking to a V2 responder.
+    FuzzedDataProvider provider{buffer.data(), buffer.size()};
+    XoRoShiRo128PlusPlus rng(provider.ConsumeIntegral<uint64_t>());
+    auto t1 = MakeV1Transport(NodeId{0});
+    auto t2 = MakeV2Transport(NodeId{1}, false, rng, provider);
+    if (!t1 || !t2) return;
+    SimulationTest(*t1, *t2, rng, provider);
+}