Move all g_cs_orphans locking to txorphanage

src/net_processing.cpp

@@ -595,8 +595,8 @@ private:
      *                     reconsidered.
      * @return             True if there are still orphans in this peer's work set.
      */
-    bool ProcessOrphanTx(Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans)
+    bool ProcessOrphanTx(Peer& peer)
-        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex);
+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
     /** Process a single headers message from a peer.
      *
      * @param[in]   pfrom     CNode of the peer
@@ -1501,7 +1501,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);
     m_txrequest.DisconnectedPeer(nodeid);
     m_num_preferred_download_peers -= state->fPreferredDownload;
     m_peers_downloading_from -= (state->nBlocksInFlight != 0);
@@ -2885,7 +2885,6 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
 {
     AssertLockHeld(g_msgproc_mutex);
     AssertLockHeld(cs_main);
-    AssertLockHeld(g_cs_orphans);
 
     CTransactionRef porphanTx = nullptr;
     NodeId from_peer = -1;
@@ -3903,7 +3902,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             AddKnownTx(*peer, txid);
         }
 
-        LOCK2(cs_main, g_cs_orphans);
+        LOCK(cs_main);
 
         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
@@ -4139,7 +4138,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         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());
@@ -4769,7 +4768,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
 
     bool has_more_orphans;
     {
-        LOCK2(cs_main, g_cs_orphans);
+        LOCK(cs_main);
         has_more_orphans = ProcessOrphanTx(*peer);
     }
 

src/test/fuzz/txorphan.cpp

@@ -85,12 +85,10 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
             CallOneOf(
                 fuzzed_data_provider,
                 [&] {
-                    LOCK(g_cs_orphans);
                     orphanage.AddChildrenToWorkSet(*tx, peer_id);
                 },
                 [&] {
                     {
-                        LOCK(g_cs_orphans);
                         NodeId originator;
                         bool more = true;
                         CTransactionRef ref = orphanage.GetTxToReconsider(peer_id, originator, more);
@@ -107,14 +105,12 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
                     // AddTx should return false if tx is too big or already have it
                     // tx weight 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(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(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 == (GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT));
@@ -125,25 +121,22 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
                     bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(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(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(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);
                 });
         }

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(!g_cs_orphans)
     {
+        LOCK(g_cs_orphans);
         return m_orphans.size();
     }
 
-    CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
+    CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans)
     {
+        LOCK(g_cs_orphans);
         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++)
     {

src/txorphanage.cpp

@@ -15,11 +15,11 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60;
 /** Minimum time between orphan transactions expire time checks in seconds */
 static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60;
 
-RecursiveMutex g_cs_orphans;
+RecursiveMutex TxOrphanage::g_cs_orphans;
 
 bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
 {
-    AssertLockHeld(g_cs_orphans);
+    LOCK(g_cs_orphans);
 
     const uint256& hash = tx->GetHash();
     if (m_orphans.count(hash))
@@ -54,6 +54,12 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
 }
 
 int TxOrphanage::EraseTx(const uint256& txid)
+{
+    LOCK(g_cs_orphans);
+    return _EraseTx(txid);
+}
+
+int TxOrphanage::_EraseTx(const uint256& txid)
 {
     AssertLockHeld(g_cs_orphans);
     std::map<uint256, OrphanTx>::iterator it = m_orphans.find(txid);
@@ -87,7 +93,7 @@ int TxOrphanage::EraseTx(const uint256& txid)
 
 void TxOrphanage::EraseForPeer(NodeId peer)
 {
-    AssertLockHeld(g_cs_orphans);
+    LOCK(g_cs_orphans);
 
     m_peer_work_set.erase(peer);
 
@@ -98,7 +104,7 @@ void TxOrphanage::EraseForPeer(NodeId peer)
         std::map<uint256, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
         if (maybeErase->second.fromPeer == peer)
         {
-            nErased += EraseTx(maybeErase->second.tx->GetHash());
+            nErased += _EraseTx(maybeErase->second.tx->GetHash());
         }
     }
     if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx from peer=%d\n", nErased, peer);
@@ -106,7 +112,7 @@ void TxOrphanage::EraseForPeer(NodeId peer)
 
 void TxOrphanage::LimitOrphans(unsigned int max_orphans)
 {
-    AssertLockHeld(g_cs_orphans);
+    LOCK(g_cs_orphans);
 
     unsigned int nEvicted = 0;
     static int64_t nNextSweep;
@@ -120,7 +126,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans)
         {
             std::map<uint256, OrphanTx>::iterator maybeErase = iter++;
             if (maybeErase->second.nTimeExpire <= nNow) {
-                nErased += EraseTx(maybeErase->second.tx->GetHash());
+                nErased += _EraseTx(maybeErase->second.tx->GetHash());
             } else {
                 nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime);
             }
@@ -134,7 +140,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans)
     {
         // Evict a random orphan:
         size_t randompos = rng.randrange(m_orphan_list.size());
-        EraseTx(m_orphan_list[randompos]->first);
+        _EraseTx(m_orphan_list[randompos]->first);
         ++nEvicted;
     }
     if (nEvicted > 0) LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted);
@@ -142,7 +148,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans)
 
 void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx, NodeId peer)
 {
-    AssertLockHeld(g_cs_orphans);
+    LOCK(g_cs_orphans);
 
     // Get this peer's work set, emplacing an empty set it didn't exist
     std::set<uint256>& orphan_work_set = m_peer_work_set.try_emplace(peer).first->second;
@@ -169,7 +175,7 @@ bool TxOrphanage::HaveTx(const GenTxid& gtxid) const
 
 CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, NodeId& originator, bool& more)
 {
-    AssertLockHeld(g_cs_orphans);
+    LOCK(g_cs_orphans);
 
     auto work_set_it = m_peer_work_set.find(peer);
     if (work_set_it != m_peer_work_set.end()) {
@@ -215,7 +221,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
     if (vOrphanErase.size()) {
         int nErased = 0;
         for (const uint256& orphanHash : vOrphanErase) {
-            nErased += EraseTx(orphanHash);
+            nErased += _EraseTx(orphanHash);
         }
         LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx included or conflicted by block\n", nErased);
     }

src/txorphanage.h

@@ -13,9 +13,6 @@
 #include <map>
 #include <set>
 
-/** Guards orphan transactions */
-extern RecursiveMutex g_cs_orphans;
-
 /** A class to track orphan transactions (failed on TX_MISSING_INPUTS)
  * Since we cannot distinguish orphans from bad transactions with
  * non-existent inputs, we heavily limit the number of orphans
@@ -24,10 +21,10 @@ extern RecursiveMutex g_cs_orphans;
 class TxOrphanage {
 public:
     /** Add a new orphan transaction */
-    bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
+    bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans);
 
     /** Check if we already have an orphan transaction (by txid or wtxid) */
-    bool HaveTx(const GenTxid& gtxid) const LOCKS_EXCLUDED(::g_cs_orphans);
+    bool HaveTx(const GenTxid& gtxid) const EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans);
 
     /** Extract a transaction from a peer's work set
      *  Returns nullptr and sets more to false if there are no transactions
@@ -36,31 +33,34 @@ public:
      *  the originating peer, and whether there are more orphans for this peer
      *  to work on after this tx.
      */
-    CTransactionRef GetTxToReconsider(NodeId peer, NodeId& originator, bool& more) 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);
+    int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans);
 
     /** Erase all orphans announced by a peer (eg, after that peer disconnects) */
-    void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
+    void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans);
 
     /** Erase all orphans included in or invalidated by a new block */
-    void EraseForBlock(const CBlock& block) LOCKS_EXCLUDED(::g_cs_orphans);
+    void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans);
 
     /** Limit the orphanage to the given maximum */
-    void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
+    void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!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);
+    void AddChildrenToWorkSet(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans);
 
     /** Return how many entries exist in the orphange */
-    size_t Size() LOCKS_EXCLUDED(::g_cs_orphans)
+    size_t Size() EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans)
     {
-        LOCK(::g_cs_orphans);
+        LOCK(g_cs_orphans);
         return m_orphans.size();
     }
 
 protected:
+    /** Guards orphan transactions */
+    static RecursiveMutex g_cs_orphans;
+
     struct OrphanTx {
         CTransactionRef tx;
         NodeId fromPeer;
@@ -96,6 +96,9 @@ protected:
     /** Index from wtxid into the m_orphans to lookup orphan
      *  transactions using their witness ids. */
     std::map<uint256, OrphanMap::iterator> m_wtxid_to_orphan_it GUARDED_BY(g_cs_orphans);
+
+    /** Erase an orphan by txid */
+    int _EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
 };
 
 #endif // BITCOIN_TXORPHANAGE_H