p2p: Unify Send and Receive protocol versions

Summary:
```
CNode has two members to keep protocol version:

    nRecvVersion for received messages
    nSendVersion for messages to send

After exchanging with VERSION and VERACK messages via protocol version
INIT_PROTO_VERSION, both nodes set nRecvVersion and nSendVersion to the
same value which is the greatest common protocol version.

This PR:

    replaces two CNode members, nRecvVersion nSendVersion, with
m_greatest_common_version
    removes duplicated getter and setter

There is no change in behavior on the P2P network.
```

Backport of core [[bitcoin/bitcoin#17785 | PR17785]].

Depends on D9193.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9195

src/avalanche/processor.cpp

@@ -275,7 +275,7 @@ namespace {
 
 void Processor::sendResponse(CNode *pfrom, Response response) const {
     connman->PushMessage(
-        pfrom, CNetMsgMaker(pfrom->GetSendVersion())
+        pfrom, CNetMsgMaker(pfrom->GetCommonVersion())
                    .Make(NetMsgType::AVARESPONSE,
                          TCPResponse(std::move(response), sessionKey)));
 }
@@ -424,7 +424,7 @@ bool Processor::sendHello(CNode *pfrom) const {
         }
     }
 
-    connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion())
+    connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion())
                                     .Make(NetMsgType::AVAHELLO,
                                           Hello(peerData->delegation, sig)));
 
@@ -569,7 +569,7 @@ void Processor::runEventLoop() {
 
             // Send the query to the node.
             connman->PushMessage(
-                pnode, CNetMsgMaker(pnode->GetSendVersion())
+                pnode, CNetMsgMaker(pnode->GetCommonVersion())
                            .Make(NetMsgType::AVAPOLL,
                                  Poll(current_round, std::move(invs))));
             return true;

src/avalanche/test/processor_tests.cpp

@@ -102,7 +102,7 @@ struct AvalancheTestingSetup : public TestChain100Setup {
         auto node =
             new CNode(id++, ServiceFlags(NODE_NETWORK), 0, INVALID_SOCKET, addr,
                       0, 0, 0, CAddress(), "", ConnectionType::OUTBOUND);
-        node->SetSendVersion(PROTOCOL_VERSION);
+        node->SetCommonVersion(PROTOCOL_VERSION);
         node->nServices = nServices;
         m_node.peerman->InitializeNode(config, node);
         node->nVersion = 1;

src/net.cpp

@@ -647,32 +647,6 @@ bool CNode::ReceiveMsgBytes(const Config &config, const char *pch,
     return true;
 }
 
-void CNode::SetSendVersion(int nVersionIn) {
-    // Send version may only be changed in the version message, and only one
-    // version message is allowed per session. We can therefore treat this value
-    // as const and even atomic as long as it's only used once a version message
-    // has been successfully processed. Any attempt to set this twice is an
-    // error.
-    if (nSendVersion != 0) {
-        error("Send version already set for node: %i. Refusing to change from "
-              "%i to %i",
-              id, nSendVersion, nVersionIn);
-    } else {
-        nSendVersion = nVersionIn;
-    }
-}
-
-int CNode::GetSendVersion() const {
-    // The send version should always be explicitly set to INIT_PROTO_VERSION
-    // rather than using this value until SetSendVersion has been called.
-    if (nSendVersion == 0) {
-        error("Requesting unset send version for node: %i. Using %i", id,
-              INIT_PROTO_VERSION);
-        return INIT_PROTO_VERSION;
-    }
-    return nSendVersion;
-}
-
 int V1TransportDeserializer::readHeader(const Config &config, const char *pch,
                                         uint32_t nBytes) {
     // copy data to temporary parsing buffer
@@ -1280,9 +1254,10 @@ void CConnman::InactivityCheck(CNode *pnode) {
             LogPrintf("socket sending timeout: %is\n",
                       nTime - pnode->nLastSend);
             pnode->fDisconnect = true;
-        } else if (nTime - pnode->nLastRecv > (pnode->nVersion > BIP0031_VERSION
-                                                   ? TIMEOUT_INTERVAL
-                                                   : 90 * 60)) {
+        } else if (nTime - pnode->nLastRecv >
+                   (pnode->GetCommonVersion() > BIP0031_VERSION
+                        ? TIMEOUT_INTERVAL
+                        : 90 * 60)) {
             LogPrintf("socket receive timeout: %is\n",
                       nTime - pnode->nLastRecv);
             pnode->fDisconnect = true;

src/net.h

@@ -836,7 +836,6 @@ class CNode {
 
     std::deque<CInv> vRecvGetData;
     uint64_t nRecvBytes GUARDED_BY(cs_vRecv){0};
-    std::atomic<int> nRecvVersion{INIT_PROTO_VERSION};
 
     std::atomic<int64_t> nLastSend{0};
     std::atomic<int64_t> nLastRecv{0};
@@ -1035,6 +1034,7 @@ class CNode {
     const uint64_t nLocalHostNonce;
     const uint64_t nLocalExtraEntropy;
     const ConnectionType m_conn_type;
+    std::atomic<int> m_greatest_common_version{INIT_PROTO_VERSION};
 
     //! Services offered to this peer.
     //!
@@ -1054,7 +1054,6 @@ class CNode {
     const ServiceFlags nLocalServices;
 
     const int nMyStartingHeight;
-    int nSendVersion{0};
     NetPermissionFlags m_permissionFlags{PF_NONE};
     // Used only by SocketHandler thread
     std::list<CNetMessage> vRecvMsg;
@@ -1082,10 +1081,10 @@ class CNode {
     bool ReceiveMsgBytes(const Config &config, const char *pch, uint32_t nBytes,
                          bool &complete);
 
-    void SetRecvVersion(int nVersionIn) { nRecvVersion = nVersionIn; }
-    int GetRecvVersion() const { return nRecvVersion; }
-    void SetSendVersion(int nVersionIn);
-    int GetSendVersion() const;
+    void SetCommonVersion(int greatest_common_version) {
+        m_greatest_common_version = greatest_common_version;
+    }
+    int GetCommonVersion() const { return m_greatest_common_version; }
 
     CService GetAddrLocal() const;
     //! May not be called more than once

src/net_processing.cpp

@@ -810,15 +810,15 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid,
                 [&connman, nCMPCTBLOCKVersion](CNode *pnodeStop) {
                     AssertLockHeld(cs_main);
                     connman.PushMessage(
-                        pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion())
+                        pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion())
                                        .Make(NetMsgType::SENDCMPCT,
                                              /*fAnnounceUsingCMPCTBLOCK=*/false,
                                              nCMPCTBLOCKVersion));
                     return true;
                 });
             lNodesAnnouncingHeaderAndIDs.pop_front();
         }
-        connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion())
+        connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion())
                                        .Make(NetMsgType::SENDCMPCT,
                                              /*fAnnounceUsingCMPCTBLOCK=*/true,
                                              nCMPCTBLOCKVersion));
@@ -1605,7 +1605,8 @@ void PeerManager::NewPoWValidBlock(
         AssertLockHeld(cs_main);
 
         // TODO: Avoid the repeated-serialization here
-        if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect) {
+        if (pnode->GetCommonVersion() < INVALID_CB_NO_BAN_VERSION ||
+            pnode->fDisconnect) {
             return;
         }
         ProcessBlockAvailability(pnode->GetId());
@@ -1848,7 +1849,7 @@ static void ProcessGetBlockData(const Config &config, CNode &pfrom,
                      __func__, pfrom.GetId());
         }
     }
-    const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
+    const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
     // Disconnect node in case we have reached the outbound limit for serving
     // historical blocks.
     // Never disconnect whitelisted nodes.
@@ -2017,7 +2018,7 @@ static void ProcessGetData(const Config &config, CNode &pfrom,
 
     std::deque<CInv>::iterator it = pfrom.vRecvGetData.begin();
     std::vector<CInv> vNotFound;
-    const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
+    const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
 
     // mempool entries added before this time have likely expired from mapRelay
     const std::chrono::seconds longlived_mempool_time =
@@ -2105,7 +2106,7 @@ void PeerManager::SendBlockTransactions(CNode &pfrom, const CBlock &block,
         resp.txn[i] = block.vtx[req.indices[i]];
     }
     LOCK(cs_main);
-    const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
+    const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
     int nSendFlags = 0;
     m_connman.PushMessage(
         &pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp));
@@ -2114,7 +2115,7 @@ void PeerManager::SendBlockTransactions(CNode &pfrom, const CBlock &block,
 void PeerManager::ProcessHeadersMessage(
     const Config &config, CNode &pfrom,
     const std::vector<CBlockHeader> &headers, bool via_compact_block) {
-    const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
+    const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
     size_t nCount = headers.size();
 
     if (nCount == 0) {
@@ -2531,7 +2532,7 @@ static void ProcessGetCFilters(CNode &peer, CDataStream &vRecv,
     }
 
     for (const auto &filter : filters) {
-        CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
+        CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion())
                                     .Make(NetMsgType::CFILTER, filter);
         connman.PushMessage(&peer, std::move(msg));
     }
@@ -2593,7 +2594,7 @@ static void ProcessGetCFHeaders(CNode &peer, CDataStream &vRecv,
     }
 
     CSerializedNetMsg msg =
-        CNetMsgMaker(peer.GetSendVersion())
+        CNetMsgMaker(peer.GetCommonVersion())
             .Make(NetMsgType::CFHEADERS, filter_type_ser,
                   stop_index->GetBlockHash(), prev_header, filter_hashes);
     connman.PushMessage(&peer, std::move(msg));
@@ -2647,7 +2648,7 @@ static void ProcessGetCFCheckPt(CNode &peer, CDataStream &vRecv,
         }
     }
 
-    CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
+    CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion())
                                 .Make(NetMsgType::CFCHECKPT, filter_type_ser,
                                       stop_index->GetBlockHash(), headers);
     connman.PushMessage(&peer, std::move(msg));
@@ -2691,14 +2692,12 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
         uint64_t nServiceInt;
         ServiceFlags nServices;
         int nVersion;
-        int nSendVersion;
         std::string cleanSubVer;
         int nStartingHeight = -1;
         bool fRelay = true;
         uint64_t nExtraEntropy = 1;
 
         vRecv >> nVersion >> nServiceInt >> nTime >> addrMe;
-        nSendVersion = std::min(nVersion, PROTOCOL_VERSION);
         nServices = ServiceFlags(nServiceInt);
         if (!pfrom.IsInboundConn()) {
             m_connman.SetServices(pfrom.addr, nServices);
@@ -2757,8 +2756,15 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
             PushNodeVersion(config, pfrom, m_connman, GetAdjustedTime());
         }
 
+        // Change version
+        const int greatest_common_version =
+            std::min(nVersion, PROTOCOL_VERSION);
+        pfrom.SetCommonVersion(greatest_common_version);
+        pfrom.nVersion = nVersion;
+
         m_connman.PushMessage(
-            &pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK));
+            &pfrom,
+            CNetMsgMaker(greatest_common_version).Make(NetMsgType::VERACK));
 
         pfrom.nServices = nServices;
         pfrom.SetAddrLocal(addrMe);
@@ -2784,9 +2790,6 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
             pfrom.m_tx_relay->fRelayTxes = fRelay;
         }
 
-        // Change version
-        pfrom.SetSendVersion(nSendVersion);
-        pfrom.nVersion = nVersion;
         pfrom.nRemoteHostNonce = nNonce;
         pfrom.nRemoteExtraEntropy = nExtraEntropy;
 
@@ -2817,8 +2820,8 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
             }
 
             // Get recent addresses
-            m_connman.PushMessage(
-                &pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR));
+            m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version)
+                                              .Make(NetMsgType::GETADDR));
             pfrom.fGetAddr = true;
             m_connman.MarkAddressGood(pfrom.addr);
         }
@@ -2861,11 +2864,9 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
     }
 
     // At this point, the outgoing message serialization version can't change.
-    const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
+    const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
 
     if (msg_type == NetMsgType::VERACK) {
-        pfrom.SetRecvVersion(std::min(pfrom.nVersion.load(), PROTOCOL_VERSION));
-
         if (!pfrom.IsInboundConn()) {
             // Mark this node as currently connected, so we update its timestamp
             // later.
@@ -2880,7 +2881,7 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
                 pfrom.m_tx_relay == nullptr ? "block-relay" : "full-relay");
         }
 
-        if (pfrom.nVersion >= SENDHEADERS_VERSION) {
+        if (pfrom.GetCommonVersion() >= SENDHEADERS_VERSION) {
             // Tell our peer we prefer to receive headers rather than inv's
             // We send this to non-NODE NETWORK peers as well, because even
             // non-NODE NETWORK peers can announce blocks (such as pruning
@@ -2889,7 +2890,7 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
                                   msgMaker.Make(NetMsgType::SENDHEADERS));
         }
 
-        if (pfrom.nVersion >= SHORT_IDS_BLOCKS_VERSION) {
+        if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION) {
             // Tell our peer we are willing to provide version 1 or 2
             // cmpctblocks. However, we do not request new block announcements
             // using cmpctblock messages. We send this to non-NODE NETWORK peers
@@ -4196,7 +4197,7 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
     }
 
     if (msg_type == NetMsgType::PING) {
-        if (pfrom.nVersion > BIP0031_VERSION) {
+        if (pfrom.GetCommonVersion() > BIP0031_VERSION) {
             uint64_t nonce = 0;
             vRecv >> nonce;
             // Echo the message back with the nonce. This allows for two useful
@@ -4502,7 +4503,7 @@ bool PeerManager::ProcessMessages(const Config &config, CNode *pfrom,
     }
     CNetMessage &msg(msgs.front());
 
-    msg.SetVersion(pfrom->GetRecvVersion());
+    msg.SetVersion(pfrom->GetCommonVersion());
 
     // Check network magic
     if (!msg.m_valid_netmagic) {
@@ -4570,7 +4571,7 @@ void PeerManager::ConsiderEviction(CNode &pto, int64_t time_in_seconds) {
     AssertLockHeld(cs_main);
 
     CNodeState &state = *State(pto.GetId());
-    const CNetMsgMaker msgMaker(pto.GetSendVersion());
+    const CNetMsgMaker msgMaker(pto.GetCommonVersion());
 
     if (!state.m_chain_sync.m_protect && pto.IsOutboundOrBlockRelayConn() &&
         state.fSyncStarted) {
@@ -4795,7 +4796,7 @@ bool PeerManager::SendMessages(const Config &config, CNode *pto,
 
     // If we get here, the outgoing message serialization version is set and
     // can't change.
-    const CNetMsgMaker msgMaker(pto->GetSendVersion());
+    const CNetMsgMaker msgMaker(pto->GetCommonVersion());
 
     //
     // Message: ping
@@ -4817,7 +4818,7 @@ bool PeerManager::SendMessages(const Config &config, CNode *pto,
         }
         pto->fPingQueued = false;
         pto->m_ping_start = GetTime<std::chrono::microseconds>();
-        if (pto->nVersion > BIP0031_VERSION) {
+        if (pto->GetCommonVersion() > BIP0031_VERSION) {
             pto->nPingNonceSent = nonce;
             m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::PING, nonce));
         } else {
@@ -5468,7 +5469,8 @@ bool PeerManager::SendMessages(const Config &config, CNode *pto,
         //
         // We don't want white listed peers to filter txs to us if we have
         // -whitelistforcerelay
-        if (pto->m_tx_relay != nullptr && pto->nVersion >= FEEFILTER_VERSION &&
+        if (pto->m_tx_relay != nullptr &&
+            pto->GetCommonVersion() >= FEEFILTER_VERSION &&
             gArgs.GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
             !pto->HasPermission(PF_FORCERELAY)) {
             Amount currentFilter =

src/test/denialofservice_tests.cpp

@@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) {
     CAddress addr1(ip(0xa0b0c001), NODE_NONE);
     CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK), 0, INVALID_SOCKET, addr1,
                      0, 0, 0, CAddress(), "", ConnectionType::OUTBOUND);
-    dummyNode1.SetSendVersion(PROTOCOL_VERSION);
+    dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
 
     peerLogic->InitializeNode(config, &dummyNode1);
     dummyNode1.nVersion = 1;
@@ -149,7 +149,7 @@ static void AddRandomOutboundPeer(const Config &config,
                                   INVALID_SOCKET, addr, 0, 0, 0, CAddress(), "",
                                   ConnectionType::OUTBOUND));
     CNode &node = *vNodes.back();
-    node.SetSendVersion(PROTOCOL_VERSION);
+    node.SetCommonVersion(PROTOCOL_VERSION);
 
     peerLogic.InitializeNode(config, &node);
     node.nVersion = 1;
@@ -250,7 +250,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) {
     CAddress addr1(ip(0xa0b0c001), NODE_NONE);
     CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, 0, 0, 0,
                      CAddress(), "", ConnectionType::INBOUND);
-    dummyNode1.SetSendVersion(PROTOCOL_VERSION);
+    dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
     peerLogic->InitializeNode(config, &dummyNode1);
     dummyNode1.nVersion = 1;
     dummyNode1.fSuccessfullyConnected = true;
@@ -269,7 +269,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) {
     CAddress addr2(ip(0xa0b0c002), NODE_NONE);
     CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, 1, 1, 1,
                      CAddress(), "", ConnectionType::INBOUND);
-    dummyNode2.SetSendVersion(PROTOCOL_VERSION);
+    dummyNode2.SetCommonVersion(PROTOCOL_VERSION);
     peerLogic->InitializeNode(config, &dummyNode2);
     dummyNode2.nVersion = 1;
     dummyNode2.fSuccessfullyConnected = true;
@@ -319,7 +319,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) {
     CAddress addr(ip(0xa0b0c001), NODE_NONE);
     CNode dummyNode(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr, 4, 4, 4,
                     CAddress(), "", ConnectionType::INBOUND);
-    dummyNode.SetSendVersion(PROTOCOL_VERSION);
+    dummyNode.SetCommonVersion(PROTOCOL_VERSION);
     peerLogic->InitializeNode(config, &dummyNode);
     dummyNode.nVersion = 1;
     dummyNode.fSuccessfullyConnected = true;