merge bitcoin#26295: Replace global g_cs_orphans lock with local

kwvg · kwvg · commit 41721029f5ea · 2025-07-30T07:52:06.000Z
This commit will not compile as-is as block based orphan reprocessing, a
Dash-ism, has not been adapted to work with this refactor. That has been
segmented out into a separate commit to aid in review.
diff --git a/src/init.cpp b/src/init.cpp
@@ -62,7 +62,6 @@
 #include <torcontrol.h>
 #include <txdb.h>
 #include <txmempool.h>
-#include <txorphanage.h>
 #include <util/asmap.h>
 #include <util/error.h>
 #include <util/moneystr.h>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -405,9 +405,6 @@ struct Peer {
     /** Total number of addresses that were processed (excludes rate-limited ones). */
     std::atomic<uint64_t> m_addr_processed{0};
 
-    /** Set of txids to reconsider once their parent transactions have been accepted **/
-    std::set<uint256> m_orphan_work_set GUARDED_BY(g_cs_orphans);
-
     /** Whether we've sent this peer a getheaders in response to an inv prior to initial-headers-sync completing */
     bool m_inv_triggered_getheaders_before_sync GUARDED_BY(NetEventsInterface::g_msgproc_mutex){false};
 
@@ -711,8 +708,17 @@ class PeerManagerImpl final : public PeerManager
      */
     bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);
 
-    void ProcessOrphanTx(std::set<uint256>& orphan_work_set) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans)
-        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+    /**
+     * Reconsider orphan transactions after a parent has been accepted to the mempool.
+     *
+     * @peer[in]  peer     The peer whose orphan transactions we will reconsider. Generally only one
+     *                     orphan will be reconsidered on each call of this function. This set
+     *                     may be added to if accepting an orphan causes its children to be
+     *                     reconsidered.
+     * @return             True if there are still orphans in this peer's work set.
+     */
+    bool ProcessOrphanTx(Peer& peer)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
     /** Process a single headers message from a peer. */
     void ProcessHeadersMessage(CNode& pfrom, Peer& peer,
                                const std::vector<CBlockHeader>& headers,
@@ -1040,14 +1046,14 @@ class PeerManagerImpl final : public PeerManager
     /** Storage for orphan information */
     TxOrphanage m_orphanage;
 
-    void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
+    void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
 
     /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction.
      *  The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of
      *  these are kept in a ring buffer */
-    std::vector<std::pair<uint256, CTransactionRef>> vExtraTxnForCompact GUARDED_BY(g_cs_orphans);
+    std::vector<std::pair<uint256, CTransactionRef>> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex);
     /** Offset into vExtraTxnForCompact to insert the next tx */
-    size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0;
+    size_t vExtraTxnForCompactIt GUARDED_BY(g_msgproc_mutex) = 0;
 };
 
 // Keeps track of the time (in microseconds) when transactions were requested last time
@@ -1674,7 +1680,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) {
     for (const QueuedBlock& entry : state->vBlocksInFlight) {
         mapBlocksInFlight.erase(entry.pindex->GetBlockHash());
     }
-    WITH_LOCK(g_cs_orphans, m_orphanage.EraseForPeer(nodeid));
+    m_orphanage.EraseForPeer(nodeid);
     if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid);
     m_num_preferred_download_peers -= state->fPreferredDownload;
     m_peers_downloading_from -= (state->nBlocksInFlight != 0);
@@ -1768,7 +1774,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c
     return true;
 }
 
-void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
+void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
 {
     size_t max_extra_txn = gArgs.GetIntArg("-blockreconstructionextratxn", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN);
     if (max_extra_txn <= 0)
@@ -2007,7 +2013,7 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler)
 void PeerManagerImpl::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex)
 {
     {
-        LOCK2(::cs_main, g_cs_orphans);
+        LOCK2(::cs_main, m_mutex);
 
         auto orphanWorkSet = m_orphanage.GetCandidatesForBlock(*pblock);
         while (!orphanWorkSet.empty()) {
@@ -3197,33 +3203,24 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
     return;
 }
 
-/**
- * Reconsider orphan transactions after a parent has been accepted to the mempool.
- *
- * @param[in,out]  orphan_work_set  The set of orphan transactions to reconsider. Generally only one
- *                                  orphan will be reconsidered on each call of this function. This set
- *                                  may be added to if accepting an orphan causes its children to be
- *                                  reconsidered.
- */
-void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
+bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
 {
+    AssertLockHeld(g_msgproc_mutex);
     AssertLockHeld(cs_main);
-    AssertLockHeld(g_cs_orphans);
-
-    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());
             _RelayTransaction(porphanTx->GetHash());
-            m_orphanage.AddChildrenToWorkSet(*porphanTx, orphan_work_set);
+            m_orphanage.AddChildrenToWorkSet(*porphanTx, peer.m_id);
             m_orphanage.EraseTx(orphanHash);
             break;
         } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) {
@@ -3243,6 +3240,8 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
             break;
         }
     }
+
+    return more;
 }
 
 bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer,
@@ -4469,7 +4468,7 @@ void PeerManagerImpl::ProcessMessage(
            }
         }
 
-        LOCK2(cs_main, g_cs_orphans);
+        LOCK(cs_main);
 
         if (AlreadyHave(inv)) {
             if (pfrom.HasPermission(NetPermissionFlags::ForceRelay)) {
@@ -4499,7 +4498,7 @@ void PeerManagerImpl::ProcessMessage(
             }
 
             _RelayTransaction(tx.GetHash());
-            m_orphanage.AddChildrenToWorkSet(tx, peer->m_orphan_work_set);
+            m_orphanage.AddChildrenToWorkSet(tx, peer->m_id);
 
             pfrom.m_last_tx_time = GetTime<std::chrono::seconds>();
 
@@ -4509,7 +4508,7 @@ void PeerManagerImpl::ProcessMessage(
                      m_mempool.size(), m_mempool.DynamicMemoryUsage() / 1000);
 
             // Recursively process any orphan transactions that depended on this one
-            ProcessOrphanTx(peer->m_orphan_work_set);
+            ProcessOrphanTx(*peer);
         }
         else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS)
         {
@@ -4648,7 +4647,7 @@ void PeerManagerImpl::ProcessMessage(
         bool fBlockReconstructed = false;
 
         {
-        LOCK2(cs_main, g_cs_orphans);
+        LOCK(cs_main);
         // If AcceptBlockHeader returned true, it set pindex
         assert(pindex);
         UpdateBlockAvailability(pfrom.GetId(), pindex->GetBlockHash());
@@ -5351,28 +5350,24 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
         }
     }
 
+    bool has_more_orphans;
     {
-        LOCK2(cs_main, g_cs_orphans);
-        if (!peer->m_orphan_work_set.empty()) {
-            ProcessOrphanTx(peer->m_orphan_work_set);
-        }
+        LOCK(cs_main);
+        has_more_orphans = ProcessOrphanTx(*peer);
     }
 
     if (pfrom->fDisconnect)
         return false;
 
+    if (has_more_orphans) return true;
+
     // this maintains the order of responses
     // and prevents m_getdata_requests to grow unbounded
     {
         LOCK(peer->m_getdata_requests_mutex);
         if (!peer->m_getdata_requests.empty()) return true;
     }
 
-    {
-        LOCK(g_cs_orphans);
-        if (!peer->m_orphan_work_set.empty()) return true;
-    }
-
     // Don't bother if send buffer is too full to respond anyway
     if (pfrom->fPauseSend) return false;
 
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
@@ -19,7 +19,6 @@
 #include <test/util/net.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
-#include <txorphanage.h>
 #include <validationinterface.h>
 #include <version.h>
 
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
@@ -13,7 +13,6 @@
 #include <test/util/net.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
-#include <txorphanage.h>
 #include <validation.h>
 #include <validationinterface.h>
 
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
@@ -40,7 +40,6 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
 
     TxOrphanage orphanage;
-    std::set<uint256> orphan_work_set;
     std::vector<COutPoint> outpoints;
     // initial outpoints used to construct transactions later
     for (uint8_t i = 0; i < 4; i++) {
@@ -90,30 +89,32 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
             CallOneOf(
                 fuzzed_data_provider,
                 [&] {
-                    LOCK(g_cs_orphans);
-                    orphanage.AddChildrenToWorkSet(*tx, orphan_work_set);
+                    orphanage.AddChildrenToWorkSet(*tx, peer_id);
                 },
                 [&] {
-                    bool have_tx = orphanage.HaveTx(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(ref->GetHash());
+                            Assert(have_tx);
+                        }
                     }
                 },
                 [&] {
                     bool have_tx = orphanage.HaveTx(tx->GetHash());
                     // AddTx should return false if tx is too big or already have it
                     // tx size is unknown, we only check when tx is already in orphanage
                     {
-                        LOCK(g_cs_orphans);
                         bool add_tx = orphanage.AddTx(tx, peer_id);
                         // have_tx == true -> add_tx == false
                         Assert(!have_tx || !add_tx);
                     }
                     have_tx = orphanage.HaveTx(tx->GetHash());
                     {
-                        LOCK(g_cs_orphans);
                         bool add_tx = orphanage.AddTx(tx, peer_id);
                         // if have_tx is still false, it must be too big
                         Assert(!have_tx == (::GetSerializeSize(*tx, PROTOCOL_VERSION) > MAX_STANDARD_TX_SIZE));
@@ -124,25 +125,22 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
                     bool have_tx = orphanage.HaveTx(tx->GetHash());
                     // EraseTx should return 0 if m_orphans doesn't have the tx
                     {
-                        LOCK(g_cs_orphans);
                         Assert(have_tx == orphanage.EraseTx(tx->GetHash()));
                     }
                     have_tx = orphanage.HaveTx(tx->GetHash());
                     // have_tx should be false and EraseTx should fail
                     {
-                        LOCK(g_cs_orphans);
                         Assert(!have_tx && !orphanage.EraseTx(tx->GetHash()));
                     }
                 },
                 [&] {
-                    LOCK(g_cs_orphans);
                     orphanage.EraseForPeer(peer_id);
                 },
                 [&] {
                     // test mocktime and expiry
                     SetMockTime(ConsumeTime(fuzzed_data_provider));
                     auto limit = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
-                    WITH_LOCK(g_cs_orphans, orphanage.LimitOrphans(limit));
+                    orphanage.LimitOrphans(limit);
                     Assert(orphanage.Size() <= limit);
                 });
         }
diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp
@@ -20,13 +20,15 @@ BOOST_FIXTURE_TEST_SUITE(orphanage_tests, TestingSetup)
 class TxOrphanageTest : public TxOrphanage
 {
 public:
-    inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
+    inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
+        LOCK(m_mutex);
         return m_orphans.size();
     }
 
-    CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
+    CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
+        LOCK(m_mutex);
         std::map<uint256, OrphanTx>::iterator it;
         it = m_orphans.lower_bound(InsecureRand256());
         if (it == m_orphans.end())
@@ -59,8 +61,6 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
     FillableSigningProvider keystore;
     BOOST_CHECK(keystore.AddKey(key));
 
-    LOCK(g_cs_orphans);
-
     // 50 orphan transactions:
     for (int i = 0; i < 50; i++)
     {
diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp
diff --git a/src/txorphanage.h b/src/txorphanage.h

Original file line number	Diff line number	Diff line change
`@@ -20,13 +20,15 @@ BOOST_FIXTURE_TEST_SUITE(orphanage_tests, TestingSetup)`
`20`	`20`	`class TxOrphanageTest : public TxOrphanage`
`21`	`21`	`{`
`22`	`22`	`public:`
`23`		`- inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)`
	`23`	`+ inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)`
`24`	`24`	`{`
	`25`	`+ LOCK(m_mutex);`
`25`	`26`	`return m_orphans.size();`
`26`	`27`	`}`
`27`	`28`
`28`		`- CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)`
	`29`	`+ CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)`
`29`	`30`	`{`
	`31`	`+ LOCK(m_mutex);`
`30`	`32`	`std::map<uint256, OrphanTx>::iterator it;`
`31`	`33`	`it = m_orphans.lower_bound(InsecureRand256());`
`32`	`34`	`if (it == m_orphans.end())`
`@@ -59,8 +61,6 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)`
`59`	`61`	`FillableSigningProvider keystore;`
`60`	`62`	`BOOST_CHECK(keystore.AddKey(key));`
`61`	`63`
`62`		`- LOCK(g_cs_orphans);`
`63`		`-`
`64`	`64`	`// 50 orphan transactions:`
`65`	`65`	`for (int i = 0; i < 50; i++)`
`66`	`66`	`{`