backport: bitcoin#19988, #20162, partial #20171 - Overhaul transaction request logic

src/Makefile.am

@@ -406,6 +406,7 @@ BITCOIN_CORE_H = \
   txdb.h \
   txmempool.h \
   txorphanage.h \
+  txrequest.h \
   undo.h \
   unordered_lru_cache.h \
   util/asmap.h \
@@ -648,6 +649,7 @@ libbitcoin_node_a_SOURCES = \
   txdb.cpp \
   txmempool.cpp \
   txorphanage.cpp \
+  txrequest.cpp \
   validation.cpp \
   validationinterface.cpp \
   versionbits.cpp \

src/Makefile.test.include

@@ -197,6 +197,7 @@ BITCOIN_TESTS =\
   test/txindex_tests.cpp \
   test/txpackage_tests.cpp \
   test/txreconciliation_tests.cpp \
+  test/txrequest_tests.cpp \
   test/txvalidation_tests.cpp \
   test/txvalidationcache_tests.cpp \
   test/uint256_tests.cpp \
@@ -380,6 +381,7 @@ test_fuzz_fuzz_SOURCES = \
  test/fuzz/tx_out.cpp \
  test/fuzz/tx_pool.cpp \
  test/fuzz/txorphan.cpp \
+ test/fuzz/txrequest.cpp \
  test/fuzz/utxo_snapshot.cpp \
  test/fuzz/utxo_total_supply.cpp \
  test/fuzz/validation_load_mempool.cpp \

src/coinjoin/server.cpp

@@ -192,6 +192,7 @@ void CCoinJoinServer::ProcessDSQUEUE(NodeId from, CDataStream& vRecv)
         LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue, masternode=%s, queue=%s\n", dmn->proTxHash.ToString(), dsq.ToString());
 
         if (!m_queueman.TryAddQueue(dsq)) return;
+        WITH_LOCK(cs_main, m_peer_manager->PeerForgetObjectRequest(CInv{MSG_DSQ, dsq.GetHash()}));
         m_peer_manager->PeerRelayDSQ(dsq);
     }
 }

src/governance/net_governance.cpp

@@ -195,7 +195,9 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa
             return;
         }
 
-        if (!WITH_LOCK(::cs_main, return m_gov_manager.ProcessObject(peer.GetLogString(), nHash, govobj))) {
+        if (WITH_LOCK(::cs_main, return m_gov_manager.ProcessObject(peer.GetLogString(), nHash, govobj))) {
+            WITH_LOCK(::cs_main, m_peer_manager->PeerForgetObjectRequest(CInv{MSG_GOVERNANCE_OBJECT, nHash}));
+        } else {
             // apply node's ban score
             m_peer_manager->PeerMisbehaving(peer.GetId(), 20);
         }
@@ -244,6 +246,7 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa
         uint256 hashToRequest;
         if (m_gov_manager.ProcessVote(vote, exception, hashToRequest)) {
             LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- %s new\n", strHash);
+            WITH_LOCK(::cs_main, m_peer_manager->PeerForgetObjectRequest(CInv{MSG_GOVERNANCE_OBJECT_VOTE, nHash}));
             m_node_sync.BumpAssetLastTime("MNGOVERNANCEOBJECTVOTE");
 
             if (!m_node_sync.IsSynced()) {

src/instantsend/net_instantsend.cpp

@@ -288,6 +288,8 @@ void NetInstantSend::ProcessMessage(CNode& pfrom, const std::string& msg_type, C
                  islock->txid.ToString(), hash.ToString(), from);
 
         m_is_manager.EnqueueInstantSendLock(from, hash, std::move(islock));
+        // Enqueued locks count as AlreadyHave, no need to request them from anyone anymore.
+        WITH_LOCK(::cs_main, m_peer_manager->PeerForgetObjectRequest(CInv{MSG_ISDLOCK, hash}));
     }
 }
 

src/msg_result.h

@@ -51,7 +51,7 @@ struct MessageProcessingResult
     //! @m_transactions will relay transactions to peers which is ready to accept it (some peers does not accept transactions)
     std::vector<uint256> m_transactions;
 
-    //! @m_to_erase triggers EraseObjectRequest from PeerManager for this inventory if not nullopt
+    //! @m_to_erase completes the sending peer's pending request for this inventory if not nullopt
     std::optional<CInv> m_to_erase;
 
     MessageProcessingResult() = default;

src/net.h

@@ -16,7 +16,6 @@
 #include <crypto/siphash.h>
 #include <hash.h>
 #include <i2p.h>
-#include <limitedmap.h>
 #include <net_permissions.h>
 #include <netaddress.h>
 #include <netbase.h>

src/net_processing.h

@@ -64,14 +64,20 @@ class PeerManagerInternal
 public:
     virtual void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") = 0;
     virtual bool PeerIsBanned(const NodeId node_id) = 0;
-    /** Erase a pending object request for a peer and update global request tracking. */
+    /** Complete this peer's pending announcement of the inv, so it is not requested from them
+     *  again. Announcements of the same inv by other peers are unaffected: an invalid or unusable
+     *  object must not stop us from fetching it from honest peers.
+     *  Requires ::cs_main (see the PeerManagerImpl override). */
     virtual void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) = 0;
-    /** Consume this peer's pending request for the inv -- erase the matching per-peer announced /
-     *  in-flight state -- and return whether such a request existed (the peer announced the inv, or
-     *  we requested it from the peer). Any queued m_object_process_time entry is left in place; the
-     *  SendMessages drain skips it once the announced/in-flight state is gone.
+    /** Consume this peer's pending request for the inv and return whether such a request existed
+     *  (the peer announced the inv, or we requested it from the peer). Consuming completes the
+     *  announcement, so a second call without a re-announcement in between returns false.
      *  Requires ::cs_main (see the PeerManagerImpl override). */
     virtual bool PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) = 0;
+    /** Delete all peers' announcements of the inv. Call once the object is accepted (AlreadyHave
+     *  turns true), so it is not requested from anyone anymore.
+     *  Requires ::cs_main (see the PeerManagerImpl override). */
+    virtual void PeerForgetObjectRequest(const CInv& inv) = 0;
     virtual void PeerPushInventory(NodeId nodeid, const CInv& inv) = 0;
     virtual void PeerRelayInv(const CInv& inv) = 0;
     virtual void PeerRelayInvFiltered(const CInv& inv, const CTransaction& relatedTx) = 0;

src/protocol.h

@@ -536,6 +536,7 @@ class CInv
     SERIALIZE_METHODS(CInv, obj) { READWRITE(obj.type, obj.hash); }
 
     friend bool operator<(const CInv& a, const CInv& b);
+    friend bool operator==(const CInv& a, const CInv& b) { return a.type == b.type && a.hash == b.hash; }
 
     bool IsKnownType() const;
     std::string GetCommand() const;