refactor: P2P transport without serialize version and type

Author: MarcoFalke

src/net.cpp

@@ -683,8 +683,8 @@ bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)
     return true;
 }
 
-V1Transport::V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept :
-    m_magic_bytes{Params().MessageStart()}, m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn)
+V1Transport::V1Transport(const NodeId node_id) noexcept
+    : m_magic_bytes{Params().MessageStart()}, m_node_id{node_id}
 {
     LOCK(m_recv_mutex);
     Reset();
@@ -968,12 +968,12 @@ void V2Transport::StartSendingHandshake() noexcept
     // We cannot wipe m_send_garbage as it will still be used as AAD later in the handshake.
 }
 
-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}
+V2Transport::V2Transport(NodeId nodeid, bool initiating, 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},
+      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}
 {
     Assume(m_send_garbage.size() <= MAX_GARBAGE_LEN);
     // Start sending immediately if we're the initiator of the connection.
@@ -983,9 +983,9 @@ V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int versio
     }
 }
 
-V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept :
-    V2Transport{nodeid, initiating, type_in, version_in, GenerateRandomKey(),
-                MakeByteSpan(GetRandHash()), GenerateRandomGarbage()} { }
+V2Transport::V2Transport(NodeId nodeid, bool initiating) noexcept
+    : V2Transport{nodeid, initiating, GenerateRandomKey(),
+                  MakeByteSpan(GetRandHash()), GenerateRandomGarbage()} {}
 
 void V2Transport::SetReceiveState(RecvState recv_state) noexcept
 {
@@ -1429,8 +1429,7 @@ CNetMessage V2Transport::GetReceivedMessage(std::chrono::microseconds time, bool
     Assume(m_recv_state == RecvState::APP_READY);
     Span<const uint8_t> contents{m_recv_decode_buffer};
     auto msg_type = GetMessageType(contents);
-    CDataStream ret(m_recv_type, m_recv_version);
-    CNetMessage msg{std::move(ret)};
+    CNetMessage msg{DataStream{}};
     // Note that BIP324Cipher::EXPANSION also includes the length descriptor size.
     msg.m_raw_message_size = m_recv_decode_buffer.size() + BIP324Cipher::EXPANSION;
     if (msg_type) {
@@ -3638,9 +3637,9 @@ ServiceFlags CConnman::GetLocalServices() const
 static std::unique_ptr<Transport> MakeTransport(NodeId id, bool use_v2transport, bool inbound) noexcept
 {
     if (use_v2transport) {
-        return std::make_unique<V2Transport>(id, /*initiating=*/!inbound, SER_NETWORK, INIT_PROTO_VERSION);
+        return std::make_unique<V2Transport>(id, /*initiating=*/!inbound);
     } else {
-        return std::make_unique<V1Transport>(id, SER_NETWORK, INIT_PROTO_VERSION);
+        return std::make_unique<V1Transport>(id);
     }
 }
 

src/net.h

@@ -232,15 +232,16 @@ class CNodeStats
  * Ideally it should only contain receive time, payload,
  * type and size.
  */
-class CNetMessage {
+class CNetMessage
+{
 public:
-    CDataStream m_recv;                  //!< received message data
+    DataStream m_recv;                   //!< received message data
     std::chrono::microseconds m_time{0}; //!< time of message receipt
     uint32_t m_message_size{0};          //!< size of the payload
     uint32_t m_raw_message_size{0};      //!< used wire size of the message (including header/checksum)
     std::string m_type;
 
-    CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {}
+    explicit CNetMessage(DataStream&& recv_in) : m_recv(std::move(recv_in)) {}
     // Only one CNetMessage object will exist for the same message on either
     // the receive or processing queue. For performance reasons we therefore
     // delete the copy constructor and assignment operator to avoid the
@@ -249,11 +250,6 @@ class CNetMessage {
     CNetMessage(const CNetMessage&) = delete;
     CNetMessage& operator=(CNetMessage&&) = default;
     CNetMessage& operator=(const CNetMessage&) = delete;
-
-    void SetVersion(int nVersionIn)
-    {
-        m_recv.SetVersion(nVersionIn);
-    }
 };
 
 /** The Transport converts one connection's sent messages to wire bytes, and received bytes back. */
@@ -379,9 +375,9 @@ class V1Transport final : public Transport
     mutable CHash256 hasher GUARDED_BY(m_recv_mutex);
     mutable uint256 data_hash GUARDED_BY(m_recv_mutex);
     bool in_data GUARDED_BY(m_recv_mutex); // parsing header (false) or data (true)
-    CDataStream hdrbuf GUARDED_BY(m_recv_mutex); // partially received header
+    DataStream hdrbuf GUARDED_BY(m_recv_mutex){}; // partially received header
     CMessageHeader hdr GUARDED_BY(m_recv_mutex); // complete header
-    CDataStream vRecv GUARDED_BY(m_recv_mutex); // received message data
+    DataStream vRecv GUARDED_BY(m_recv_mutex){}; // received message data
     unsigned int nHdrPos GUARDED_BY(m_recv_mutex);
     unsigned int nDataPos GUARDED_BY(m_recv_mutex);
 
@@ -420,7 +416,7 @@ class V1Transport final : public Transport
     size_t m_bytes_sent GUARDED_BY(m_send_mutex) {0};
 
 public:
-    V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept;
+    explicit V1Transport(const NodeId node_id) noexcept;
 
     bool ReceivedMessageComplete() const override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex)
     {
@@ -598,10 +594,6 @@ class V2Transport final : public Transport
     std::vector<uint8_t> m_recv_aad GUARDED_BY(m_recv_mutex);
     /** Buffer to put decrypted contents in, for converting to CNetMessage. */
     std::vector<uint8_t> m_recv_decode_buffer GUARDED_BY(m_recv_mutex);
-    /** Deserialization type. */
-    const int m_recv_type;
-    /** Deserialization version number. */
-    const int m_recv_version;
     /** Current receiver state. */
     RecvState m_recv_state GUARDED_BY(m_recv_mutex);
 
@@ -647,13 +639,11 @@ class V2Transport final : public Transport
      *
      * @param[in] nodeid      the node's NodeId (only for debug log output).
      * @param[in] initiating  whether we are the initiator side.
-     * @param[in] type_in     the serialization type of returned CNetMessages.
-     * @param[in] version_in  the serialization version of returned CNetMessages.
      */
-    V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept;
+    V2Transport(NodeId nodeid, bool initiating) 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, std::vector<uint8_t> garbage) noexcept;
+    V2Transport(NodeId nodeid, bool initiating, 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/net_processing.cpp

@@ -515,7 +515,7 @@ class PeerManagerImpl final : public PeerManager
     void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void SetBestHeight(int height) override { m_best_height = height; };
     void UnitTestMisbehaving(NodeId peer_id, int howmuch) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), howmuch, ""); };
-    void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
+    void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv,
                         const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
     void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override;
@@ -1033,7 +1033,7 @@ class PeerManagerImpl final : public PeerManager
      * @param[in]   peer            The peer that we received the request from
      * @param[in]   vRecv           The raw message received
      */
-    void ProcessGetCFilters(CNode& node, Peer& peer, CDataStream& vRecv);
+    void ProcessGetCFilters(CNode& node, Peer& peer, DataStream& vRecv);
 
     /**
      * Handle a cfheaders request.
@@ -1044,7 +1044,7 @@ class PeerManagerImpl final : public PeerManager
      * @param[in]   peer            The peer that we received the request from
      * @param[in]   vRecv           The raw message received
      */
-    void ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& vRecv);
+    void ProcessGetCFHeaders(CNode& node, Peer& peer, DataStream& vRecv);
 
     /**
      * Handle a getcfcheckpt request.
@@ -1055,7 +1055,7 @@ class PeerManagerImpl final : public PeerManager
      * @param[in]   peer            The peer that we received the request from
      * @param[in]   vRecv           The raw message received
      */
-    void ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv);
+    void ProcessGetCFCheckPt(CNode& node, Peer& peer, DataStream& vRecv);
 
     /** Checks if address relay is permitted with peer. If needed, initializes
      * the m_addr_known bloom filter and sets m_addr_relay_enabled to true.
@@ -3130,7 +3130,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer,
     return true;
 }
 
-void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vRecv)
+void PeerManagerImpl::ProcessGetCFilters(CNode& node, Peer& peer, DataStream& vRecv)
 {
     uint8_t filter_type_ser;
     uint32_t start_height;
@@ -3159,7 +3159,7 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vR
     }
 }
 
-void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& vRecv)
+void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, DataStream& vRecv)
 {
     uint8_t filter_type_ser;
     uint32_t start_height;
@@ -3201,7 +3201,7 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream&
               filter_hashes);
 }
 
-void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv)
+void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, DataStream& vRecv)
 {
     uint8_t filter_type_ser;
     uint256 stop_hash;
@@ -3342,7 +3342,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
     return;
 }
 
-void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
+void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv,
                                      const std::chrono::microseconds time_received,
                                      const std::atomic<bool>& interruptMsgProc)
 {
@@ -5056,8 +5056,6 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
         CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /*is_incoming=*/true);
     }
 
-    msg.SetVersion(pfrom->GetCommonVersion());
-
     try {
         ProcessMessage(*pfrom, msg.m_type, msg.m_recv, msg.m_time, interruptMsgProc);
         if (interruptMsgProc) return false;

src/net_processing.h

@@ -105,7 +105,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
     virtual void CheckForStaleTipAndEvictPeers() = 0;
 
     /** Process a single message from a peer. Public for fuzz testing */
-    virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
+    virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv,
                                 const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
 
     /** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */

src/test/fuzz/p2p_transport_serialization.cpp

@@ -36,8 +36,8 @@ void initialize_p2p_transport_serialization()
 FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serialization)
 {
     // Construct transports for both sides, with dummy NodeIds.
-    V1Transport recv_transport{NodeId{0}, SER_NETWORK, INIT_PROTO_VERSION};
-    V1Transport send_transport{NodeId{1}, SER_NETWORK, INIT_PROTO_VERSION};
+    V1Transport recv_transport{NodeId{0}};
+    V1Transport send_transport{NodeId{1}};
 
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
 
@@ -335,7 +335,7 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa
 
 std::unique_ptr<Transport> MakeV1Transport(NodeId nodeid) noexcept
 {
-    return std::make_unique<V1Transport>(nodeid, SER_NETWORK, INIT_PROTO_VERSION);
+    return std::make_unique<V1Transport>(nodeid);
 }
 
 template<typename RNG>
@@ -369,7 +369,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, std::move(garb));
+    return std::make_unique<V2Transport>(nodeid, initiator, key, ent, std::move(garb));
 }
 
 } // namespace

src/test/net_tests.cpp

@@ -1046,10 +1046,10 @@ class V2TransportTester
 
 public:
     /** 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) {}
+    explicit V2TransportTester(bool test_initiator)
+        : m_transport{0, test_initiator},
+          m_cipher{GenerateRandomTestKey(), MakeByteSpan(InsecureRand256())},
+          m_test_initiator(test_initiator) {}
 
     /** Data type returned by Interact:
      *