From 733d85f79cde353d8c9b54370f296b1031fa33d9 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Fri, 7 Oct 2022 14:25:47 +1000 Subject: [PATCH] Move all g_cs_orphans locking to txorphanage --- src/net_processing.cpp | 13 ++++++------- src/test/fuzz/txorphan.cpp | 9 +-------- src/test/orphanage_tests.cpp | 8 ++++---- src/txorphanage.cpp | 26 ++++++++++++++++---------- src/txorphanage.h | 29 ++++++++++++++++------------- 5 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f4b2d2e38f..dc839cd883 100644 --- a/src/net_processing.cpp +++ b/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) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex); + bool ProcessOrphanTx(Peer& peer) + 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& interrupt bool has_more_orphans; { - LOCK2(cs_main, g_cs_orphans); + LOCK(cs_main); has_more_orphans = ProcessOrphanTx(*peer); } diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index b200f94144..02743051e8 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/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(); - 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 index 842daa8bd4..84609f453c 100644 --- 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(!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::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 index 4cccb1affb..bdebfc161f 100644 --- a/src/txorphanage.cpp +++ b/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::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::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::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& 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); } diff --git a/src/txorphanage.h b/src/txorphanage.h index 5af1f2f0e0..91cfb7786d 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -13,9 +13,6 @@ #include #include -/** 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 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