Merge remote-tracking branch 'upstream/master'

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/chain.h

@@ -388,6 +388,14 @@ const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex*
 /** Used to marshal pointers into hashes for db storage. */
 class CDiskBlockIndex : public CBlockIndex
 {
+    /** Historically CBlockLocator's version field has been written to disk
+     * streams as the client version, but the value has never been used.
+     *
+     * Hard-code to the highest client version ever written.
+     * SerParams can be used if the field requires any meaning in the future.
+     **/
+    static constexpr int DUMMY_VERSION = 259900;
+
 public:
     uint256 hashPrev;
 
@@ -404,8 +412,8 @@ class CDiskBlockIndex : public CBlockIndex
     SERIALIZE_METHODS(CDiskBlockIndex, obj)
     {
         LOCK(::cs_main);
-        int _nVersion = s.GetVersion();
-        if (!(s.GetType() & SER_GETHASH)) READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
+        int _nVersion = DUMMY_VERSION;
+        READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
 
         READWRITE(VARINT_MODE(obj.nHeight, VarIntMode::NONNEGATIVE_SIGNED));
         READWRITE(VARINT(obj.nStatus));

src/consensus/validation.h

@@ -158,10 +158,6 @@ static inline int64_t GetTransactionInputWeight(const CTxIn& txin)
     // scriptWitness size is added here because witnesses and txins are split up in segwit serialization.
     return ::GetSerializeSize(txin, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(txin, PROTOCOL_VERSION) + ::GetSerializeSize(txin.scriptWitness.stack, PROTOCOL_VERSION);
 }
-static inline int64_t GetTransactionOutputWeight(const CTxOut& txout)
-{
-    return ::GetSerializeSize(txout, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(txout, PROTOCOL_VERSION);
-}
 
 /** Compute at which vout of the block's coinbase transaction the witness commitment occurs, or -1 if not found */
 inline int GetWitnessCommitmentIndex(const CBlock& block)

src/dbwrapper.h

@@ -6,7 +6,6 @@
 #define BITCOIN_DBWRAPPER_H
 
 #include <attributes.h>
-#include <clientversion.h>
 #include <serialize.h>
 #include <span.h>
 #include <streams.h>
@@ -167,7 +166,7 @@ class CDBIterator
 
     template<typename V> bool GetValue(V& value) {
         try {
-            CDataStream ssValue{GetValueImpl(), SER_DISK, CLIENT_VERSION};
+            DataStream ssValue{GetValueImpl()};
             ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent));
             ssValue >> value;
         } catch (const std::exception&) {
@@ -229,7 +228,7 @@ class CDBWrapper
             return false;
         }
         try {
-            CDataStream ssValue{MakeByteSpan(*strValue), SER_DISK, CLIENT_VERSION};
+            DataStream ssValue{MakeByteSpan(*strValue)};
             ssValue.Xor(obfuscate_key);
             ssValue >> value;
         } catch (const std::exception&) {

src/index/blockfilterindex.cpp

@@ -4,6 +4,7 @@
 
 #include <map>
 
+#include <clientversion.h>
 #include <common/args.h>
 #include <dbwrapper.h>
 #include <hash.h>

src/index/txindex.cpp

@@ -4,6 +4,7 @@
 
 #include <index/txindex.h>
 
+#include <clientversion.h>
 #include <common/args.h>
 #include <index/disktxpos.h>
 #include <logging.h>

src/init.cpp

@@ -19,6 +19,7 @@
 #include <chain.h>
 #include <chainparams.h>
 #include <chainparamsbase.h>
+#include <clientversion.h>
 #include <common/args.h>
 #include <common/system.h>
 #include <consensus/amount.h>

src/net.cpp

@@ -980,36 +980,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);
@@ -1088,9 +1108,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);
@@ -1150,7 +1171,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(),
@@ -1161,9 +1181,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());
@@ -1533,9 +1556,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),
@@ -1554,10 +1575,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

@@ -541,25 +541,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.
@@ -602,6 +602,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. */
@@ -615,6 +617,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. */
@@ -637,7 +641,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/primitives/block.h

@@ -118,6 +118,15 @@ class CBlock : public CBlockHeader
  */
 struct CBlockLocator
 {
+    /** Historically CBlockLocator's version field has been written to network
+     * streams as the negotiated protocol version and to disk streams as the
+     * client version, but the value has never been used.
+     *
+     * Hard-code to the highest protocol version ever written to a network stream.
+     * SerParams can be used if the field requires any meaning in the future,
+     **/
+    static constexpr int DUMMY_VERSION = 70016;
+
     std::vector<uint256> vHave;
 
     CBlockLocator() {}
@@ -126,9 +135,8 @@ struct CBlockLocator
 
     SERIALIZE_METHODS(CBlockLocator, obj)
     {
-        int nVersion = s.GetVersion();
-        if (!(s.GetType() & SER_GETHASH))
-            READWRITE(nVersion);
+        int nVersion = DUMMY_VERSION;
+        READWRITE(nVersion);
         READWRITE(obj.vHave);
     }
 

src/test/blockmanager_tests.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chainparams.h>
+#include <clientversion.h>
 #include <node/blockstorage.h>
 #include <node/context.h>
 #include <node/kernel_notifications.h>

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:

src/validation.cpp

@@ -11,6 +11,7 @@
 #include <arith_uint256.h>
 #include <chain.h>
 #include <checkqueue.h>
+#include <clientversion.h>
 #include <consensus/amount.h>
 #include <consensus/consensus.h>
 #include <consensus/merkle.h>

src/wallet/spend.cpp

@@ -144,7 +144,7 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *walle
     if (is_segwit) weight += 2;
 
     // Add the size of the transaction outputs.
-    for (const auto& txo : tx.vout) weight += GetTransactionOutputWeight(txo);
+    for (const auto& txo : tx.vout) weight += GetSerializeSize(txo) * WITNESS_SCALE_FACTOR;
 
     // Add the size of the transaction inputs as if they were signed.
     for (uint32_t i = 0; i < txouts.size(); i++) {