Merge #19590: p2p, refactor: add CInv transaction message helpers; use in net processing

c251d71 p2p, refactoring: use CInv helpers in net_processing.cpp (Jon Atack)
4254cd9 p2p: add CInv transaction message helper methods (Jon Atack)

Pull request description:

  Following the merge of wtxid relay in #18044, this is the first of three refactoring PRs (this one, #19610, and #19611) with no change in behavior, tightly scoped to ease review, to simplify the net processing code and improve encapsulation:

  - add `CInv` transaction message helper methods, defined in the class

  - use the new helpers in `net_processing.cpp` to simplify the code and improve encapsulation

  Test coverage is provided by the functional p2p tests, notably (from seeing which tests failed when breaking things to test coverage) `p2p_segwit`, `p2p_tx_download`, `p2p_feefilter`, and `p2p_permissions`.

ACKs for top commit:
  fjahr:
    Code review ACK c251d71
  laanwj:
    Code review ACK c251d71
  vasild:
    ACK c251d71
  theStack:
    Code-Review ACK c251d71
  hebasto:
    ACK c251d71, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: ead034b3c9e438909b4c5010c570d7930e69063c114290b051b7cebfd9bd5b19f573218bebe8a521256d32e830797f997adad3d85b4539c64ac5762b698e656d

Author: laanwj

src/net_processing.cpp

@@ -1441,9 +1441,9 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO
 
             {
                 LOCK(g_cs_orphans);
-                if (inv.type != MSG_WTX && mapOrphanTransactions.count(inv.hash)) {
+                if (!inv.IsMsgWtx() && mapOrphanTransactions.count(inv.hash)) {
                     return true;
-                } else if (inv.type == MSG_WTX && g_orphans_by_wtxid.count(inv.hash)) {
+                } else if (inv.IsMsgWtx() && g_orphans_by_wtxid.count(inv.hash)) {
                     return true;
                 }
             }
@@ -1453,8 +1453,7 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO
                 if (g_recent_confirmed_transactions->contains(inv.hash)) return true;
             }
 
-            const bool by_wtxid = (inv.type == MSG_WTX);
-            return recentRejects->contains(inv.hash) || mempool.exists(inv.hash, by_wtxid);
+            return recentRejects->contains(inv.hash) || mempool.exists(inv.hash, inv.IsMsgWtx());
         }
     case MSG_BLOCK:
     case MSG_WITNESS_BLOCK:
@@ -1715,7 +1714,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
     // Process as many TX items from the front of the getdata queue as
     // possible, since they're common and it's efficient to batch process
     // them.
-    while (it != pfrom.vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX || it->type == MSG_WTX)) {
+    while (it != pfrom.vRecvGetData.end() && it->IsGenTxMsg()) {
         if (interruptMsgProc) return;
         // The send buffer provides backpressure. If there's no space in
         // the buffer, pause processing until the next call.
@@ -1728,10 +1727,10 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
             continue;
         }
 
-        CTransactionRef tx = FindTxForGetData(pfrom, inv.hash, inv.type == MSG_WTX, mempool_req, now);
+        CTransactionRef tx = FindTxForGetData(pfrom, inv.hash, inv.IsMsgWtx(), mempool_req, now);
         if (tx) {
             // WTX and WITNESS_TX imply we serialize with witness
-            int nSendFlags = (inv.type == MSG_TX ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
+            int nSendFlags = (inv.IsMsgTx() ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
             connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *tx));
             mempool.RemoveUnbroadcastTx(tx->GetHash());
             // As we're going to send tx, make sure its unconfirmed parents are made requestable.
@@ -2650,15 +2649,15 @@ void ProcessMessage(
 
             // ignore INVs that don't match wtxidrelay setting
             if (State(pfrom.GetId())->m_wtxid_relay) {
-                if (inv.type == MSG_TX) continue;
+                if (inv.IsMsgTx()) continue;
             } else {
-                if (inv.type == MSG_WTX) continue;
+                if (inv.IsMsgWtx()) continue;
             }
 
             bool fAlreadyHave = AlreadyHave(inv, mempool);
             LogPrint(BCLog::NET, "got inv: %s  %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId());
 
-            if (inv.type == MSG_TX) {
+            if (inv.IsMsgTx()) {
                 inv.type |= nFetchFlags;
             }
 
@@ -3690,7 +3689,7 @@ void ProcessMessage(
         vRecv >> vInv;
         if (vInv.size() <= MAX_PEER_TX_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
             for (CInv &inv : vInv) {
-                if (inv.type == MSG_TX || inv.type == MSG_WITNESS_TX || inv.type == MSG_WTX) {
+                if (inv.IsGenTxMsg()) {
                     // If we receive a NOTFOUND message for a txid we requested, erase
                     // it from our data structures for this peer.
                     auto in_flight_it = state->m_tx_download.m_tx_in_flight.find(inv.hash);

src/protocol.h

@@ -430,6 +430,14 @@ class CInv
     std::string GetCommand() const;
     std::string ToString() const;
 
+    // Single-message helper methods
+    bool IsMsgTx()        const { return type == MSG_TX; }
+    bool IsMsgWtx()       const { return type == MSG_WTX; }
+    bool IsMsgWitnessTx() const { return type == MSG_WITNESS_TX; }
+
+    // Combined-message helper methods
+    bool IsGenTxMsg()     const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }
+
     int type;
     uint256 hash;
 };