txorphanage: make m_peer_work_set private

Author: ajtowns

src/net_processing.cpp

@@ -2887,20 +2887,14 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
     AssertLockHeld(cs_main);
     AssertLockHeld(g_cs_orphans);
 
-    auto work_set_it = m_orphanage.m_peer_work_set.find(peer.m_id);
-    if (work_set_it == m_orphanage.m_peer_work_set.end()) return false;
-
-    std::set<uint256>& orphan_work_set = work_set_it->second;
-
-    while (!orphan_work_set.empty()) {
-        const uint256 orphanHash = *orphan_work_set.begin();
-        orphan_work_set.erase(orphan_work_set.begin());
-
-        const auto [porphanTx, from_peer] = m_orphanage.GetTx(orphanHash);
-        if (porphanTx == nullptr) continue;
+    CTransactionRef porphanTx = nullptr;
+    NodeId from_peer = -1;
+    bool more = false;
 
+    while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id, from_peer, more)) {
         const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx);
         const TxValidationState& state = result.m_state;
+        const uint256& orphanHash = porphanTx->GetHash();
 
         if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
             LogPrint(BCLog::MEMPOOL, "   accepted orphan tx %s\n", orphanHash.ToString());
@@ -2957,7 +2951,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
         }
     }
 
-    return !orphan_work_set.empty();
+    return more;
 }
 
 bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer,

src/test/fuzz/txorphan.cpp

@@ -89,11 +89,17 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
                     orphanage.AddChildrenToWorkSet(*tx, peer_id);
                 },
                 [&] {
-                    bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash()));
                     {
                         LOCK(g_cs_orphans);
-                        bool get_tx = orphanage.GetTx(tx->GetHash()).first != nullptr;
-                        Assert(have_tx == get_tx);
+                        NodeId originator;
+                        bool more = true;
+                        CTransactionRef ref = orphanage.GetTxToReconsider(peer_id, originator, more);
+                        if (!ref) {
+                            Assert(!more);
+                        } else {
+                            bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash()));
+                            Assert(have_tx);
+                        }
                     }
                 },
                 [&] {

src/txorphanage.cpp

@@ -167,13 +167,27 @@ bool TxOrphanage::HaveTx(const GenTxid& gtxid) const
     }
 }
 
-std::pair<CTransactionRef, NodeId> TxOrphanage::GetTx(const uint256& txid) const
+CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, NodeId& originator, bool& more)
 {
     AssertLockHeld(g_cs_orphans);
 
-    const auto it = m_orphans.find(txid);
-    if (it == m_orphans.end()) return {nullptr, -1};
-    return {it->second.tx, it->second.fromPeer};
+    auto work_set_it = m_peer_work_set.find(peer);
+    if (work_set_it != m_peer_work_set.end()) {
+        auto& work_set = work_set_it->second;
+        while (!work_set.empty()) {
+            uint256 txid = *work_set.begin();
+            work_set.erase(work_set.begin());
+
+            const auto orphan_it = m_orphans.find(txid);
+            if (orphan_it != m_orphans.end()) {
+                more = !work_set.empty();
+                originator = orphan_it->second.fromPeer;
+                return orphan_it->second.tx;
+            }
+        }
+    }
+    more = false;
+    return nullptr;
 }
 
 void TxOrphanage::EraseForBlock(const CBlock& block)

src/txorphanage.h

@@ -10,6 +10,9 @@
 #include <primitives/transaction.h>
 #include <sync.h>
 
+#include <map>
+#include <set>
+
 /** Guards orphan transactions */
 extern RecursiveMutex g_cs_orphans;
 
@@ -26,10 +29,14 @@ class TxOrphanage {
     /** Check if we already have an orphan transaction (by txid or wtxid) */
     bool HaveTx(const GenTxid& gtxid) const LOCKS_EXCLUDED(::g_cs_orphans);
 
-    /** Get an orphan transaction and its originating peer
-     * (Transaction ref will be nullptr if not found)
+    /** Extract a transaction from a peer's work set
+     *  Returns nullptr and sets more to false if there are no transactions
+     *  to work on. Otherwise returns the transaction reference, removes
+     *  the transaction from the work set, and populates its arguments with
+     *  the originating peer, and whether there are more orphans for this peer
+     *  to work on after this tx.
      */
-    std::pair<CTransactionRef, NodeId> GetTx(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
+    CTransactionRef GetTxToReconsider(NodeId peer, NodeId& originator, bool& more) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
 
     /** Erase an orphan by txid */
     int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
@@ -43,9 +50,6 @@ class TxOrphanage {
     /** Limit the orphanage to the given maximum */
     void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
 
-    /** Which peer provided a parent tx of orphans that need to be reconsidered */
-    std::map<NodeId, std::set<uint256>> m_peer_work_set GUARDED_BY(g_cs_orphans);
-
     /** Add any orphans that list a particular tx as a parent into a peer's work set */
     void AddChildrenToWorkSet(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
 
@@ -68,6 +72,9 @@ class TxOrphanage {
      *  -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */
     std::map<uint256, OrphanTx> m_orphans GUARDED_BY(g_cs_orphans);
 
+    /** Which peer provided a parent tx of orphans that need to be reconsidered */
+    std::map<NodeId, std::set<uint256>> m_peer_work_set GUARDED_BY(g_cs_orphans);
+
     using OrphanMap = decltype(m_orphans);
 
     struct IteratorComparator