From 6bd4963c069bfd0af420e8a3fb724c3b693a1e76 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sun, 31 Jan 2021 23:42:00 +1000 Subject: txorphanage: Move functions and data into class Collects all the orphan handling globals into a single member var in net_processing, and ensures access is encapuslated into the interface functions. Also adds doxygen comments for methods. --- src/net_processing.cpp | 46 +++++++++---------------- src/test/denialofservice_tests.cpp | 53 +++++++++++++++++------------ src/txorphanage.cpp | 32 +++++++----------- src/txorphanage.h | 69 +++++++++++++++++++++++++------------- test/functional/p2p_invalid_tx.py | 2 +- 5 files changed, 106 insertions(+), 96 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f1b5c6f38d..98d9709eb2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -457,11 +457,12 @@ private: /** Number of peers from which we're downloading blocks. */ int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0; + /** Storage for orphan information */ + TxOrphanage m_orphanage; }; } // namespace namespace { - /** Number of preferable block download peers. */ int nPreferredDownload GUARDED_BY(cs_main) = 0; @@ -1003,7 +1004,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node, bool& fUpdateConnectionTim for (const QueuedBlock& entry : state->vBlocksInFlight) { mapBlocksInFlight.erase(entry.hash); } - WITH_LOCK(g_cs_orphans, EraseOrphansFor(nodeid)); + WITH_LOCK(g_cs_orphans, m_orphanage.EraseForPeer(nodeid)); m_txrequest.DisconnectedPeer(nodeid); nPreferredDownload -= state->fPreferredDownload; nPeersWithValidatedDownloads -= (state->nBlocksInFlightValidHeaders != 0); @@ -1080,11 +1081,6 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) return true; } -////////////////////////////////////////////////////////////////////////////// -// -// mapOrphanTransactions -// - static void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) { size_t max_extra_txn = gArgs.GetArg("-blockreconstructionextratxn", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN); @@ -1255,13 +1251,13 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn } /** - * Evict orphan txn pool entries (EraseOrphanTx) based on a newly connected + * Evict orphan txn pool entries based on a newly connected * block, remember the recently confirmed transactions, and delete tracked * announcements for them. Also save the time of the last tip update. */ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) { - EraseOrphansForBlock(*pblock); + m_orphanage.EraseForBlock(*pblock); m_last_tip_update = GetTime(); { @@ -1445,7 +1441,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) const uint256& hash = gtxid.GetHash(); - if (HaveOrphanTx(gtxid)) return true; + if (m_orphanage.HaveTx(gtxid)) return true; { LOCK(m_recent_confirmed_transactions_mutex); @@ -2040,7 +2036,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) const uint256 orphanHash = *orphan_work_set.begin(); orphan_work_set.erase(orphan_work_set.begin()); - const auto [porphanTx, from_peer] = GetOrphanTx(orphanHash); + const auto [porphanTx, from_peer] = m_orphanage.GetTx(orphanHash); if (porphanTx == nullptr) continue; const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), m_mempool, porphanTx, false /* bypass_limits */); @@ -2049,8 +2045,8 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanHash, porphanTx->GetWitnessHash(), m_connman); - AddChildrenToWorkSet(*porphanTx, orphan_work_set); - EraseOrphanTx(orphanHash); + m_orphanage.AddChildrenToWorkSet(*porphanTx, orphan_work_set); + m_orphanage.EraseTx(orphanHash); for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) { AddToCompactExtraTransactions(removedTx); } @@ -2097,7 +2093,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) recentRejects->insert(porphanTx->GetHash()); } } - EraseOrphanTx(orphanHash); + m_orphanage.EraseTx(orphanHash); break; } } @@ -3068,7 +3064,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), m_connman); - AddChildrenToWorkSet(tx, peer->m_orphan_work_set); + m_orphanage.AddChildrenToWorkSet(tx, peer->m_orphan_work_set); pfrom.nLastTXTime = GetTime(); @@ -3118,7 +3114,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (!AlreadyHaveTx(gtxid)) AddTxAnnouncement(pfrom, gtxid, current_time); } - if (OrphanageAddTx(ptx, pfrom.GetId())) { + if (m_orphanage.AddTx(ptx, pfrom.GetId())) { AddToCompactExtraTransactions(ptx); } @@ -3126,11 +3122,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); - // DoS prevention: do not allow mapOrphanTransactions to grow unbounded (see CVE-2012-3789) + // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, gArgs.GetArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS)); - unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTx); + unsigned int nEvicted = m_orphanage.LimitOrphans(nMaxOrphanTx); if (nEvicted > 0) { - LogPrint(BCLog::MEMPOOL, "mapOrphan overflow, removed %u tx\n", nEvicted); + LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted); } } else { LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s\n",tx.GetHash().ToString()); @@ -4744,15 +4740,3 @@ bool PeerManagerImpl::SendMessages(CNode* pto) return true; } -class CNetProcessingCleanup -{ -public: - CNetProcessingCleanup() {} - ~CNetProcessingCleanup() { - // orphan transactions - mapOrphanTransactions.clear(); - mapOrphanTransactionsByPrev.clear(); - g_orphans_by_wtxid.clear(); - } -}; -static CNetProcessingCleanup instance_of_cnetprocessingcleanup; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 83b2016cd4..35f16aa69d 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -284,15 +284,23 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) peerLogic->FinalizeNode(dummyNode, dummy); } -static CTransactionRef RandomOrphan() +class TxOrphanageTest : public TxOrphanage { - std::map::iterator it; - LOCK2(cs_main, g_cs_orphans); - it = mapOrphanTransactions.lower_bound(InsecureRand256()); - if (it == mapOrphanTransactions.end()) - it = mapOrphanTransactions.begin(); - return it->second.tx; -} +public: + inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) + { + return mapOrphanTransactions.size(); + } + + CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) + { + std::map::iterator it; + it = mapOrphanTransactions.lower_bound(InsecureRand256()); + if (it == mapOrphanTransactions.end()) + it = mapOrphanTransactions.begin(); + return it->second.tx; + } +}; static void MakeNewKeyWithFastRandomContext(CKey& key) { @@ -312,6 +320,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) // signature's R and S values have leading zeros. g_insecure_rand_ctx = FastRandomContext(ArithToUint256(arith_uint256(33))); + TxOrphanageTest orphanage; CKey key; MakeNewKeyWithFastRandomContext(key); FillableSigningProvider keystore; @@ -331,13 +340,13 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vout[0].nValue = 1*CENT; tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); - OrphanageAddTx(MakeTransactionRef(tx), i); + orphanage.AddTx(MakeTransactionRef(tx), i); } // ... and 50 that depend on other orphans: for (int i = 0; i < 50; i++) { - CTransactionRef txPrev = RandomOrphan(); + CTransactionRef txPrev = orphanage.RandomOrphan(); CMutableTransaction tx; tx.vin.resize(1); @@ -348,13 +357,13 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); BOOST_CHECK(SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL)); - OrphanageAddTx(MakeTransactionRef(tx), i); + orphanage.AddTx(MakeTransactionRef(tx), i); } // This really-big orphan should be ignored: for (int i = 0; i < 10; i++) { - CTransactionRef txPrev = RandomOrphan(); + CTransactionRef txPrev = orphanage.RandomOrphan(); CMutableTransaction tx; tx.vout.resize(1); @@ -372,24 +381,24 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) for (unsigned int j = 1; j < tx.vin.size(); j++) tx.vin[j].scriptSig = tx.vin[0].scriptSig; - BOOST_CHECK(!OrphanageAddTx(MakeTransactionRef(tx), i)); + BOOST_CHECK(!orphanage.AddTx(MakeTransactionRef(tx), i)); } // Test EraseOrphansFor: for (NodeId i = 0; i < 3; i++) { - size_t sizeBefore = mapOrphanTransactions.size(); - EraseOrphansFor(i); - BOOST_CHECK(mapOrphanTransactions.size() < sizeBefore); + size_t sizeBefore = orphanage.CountOrphans(); + orphanage.EraseForPeer(i); + BOOST_CHECK(orphanage.CountOrphans() < sizeBefore); } // Test LimitOrphanTxSize() function: - LimitOrphanTxSize(40); - BOOST_CHECK(mapOrphanTransactions.size() <= 40); - LimitOrphanTxSize(10); - BOOST_CHECK(mapOrphanTransactions.size() <= 10); - LimitOrphanTxSize(0); - BOOST_CHECK(mapOrphanTransactions.empty()); + orphanage.LimitOrphans(40); + BOOST_CHECK(orphanage.CountOrphans() <= 40); + orphanage.LimitOrphans(10); + BOOST_CHECK(orphanage.CountOrphans() <= 10); + orphanage.LimitOrphans(0); + BOOST_CHECK(orphanage.CountOrphans() == 0); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 45bc0885fa..f758107b82 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -17,14 +17,7 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; RecursiveMutex g_cs_orphans; -std::map mapOrphanTransactions GUARDED_BY(g_cs_orphans); -std::map::iterator> g_orphans_by_wtxid GUARDED_BY(g_cs_orphans); - - std::map::iterator, IteratorComparator>> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans); - - std::vector::iterator> g_orphan_list GUARDED_BY(g_cs_orphans); - -bool OrphanageAddTx(const CTransactionRef& tx, NodeId peer) +bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) { AssertLockHeld(g_cs_orphans); @@ -60,8 +53,9 @@ bool OrphanageAddTx(const CTransactionRef& tx, NodeId peer) return true; } -int EraseOrphanTx(const uint256& txid) +int TxOrphanage::EraseTx(const uint256& txid) { + AssertLockHeld(g_cs_orphans); std::map::iterator it = mapOrphanTransactions.find(txid); if (it == mapOrphanTransactions.end()) return 0; @@ -91,7 +85,7 @@ int EraseOrphanTx(const uint256& txid) return 1; } -void EraseOrphansFor(NodeId peer) +void TxOrphanage::EraseForPeer(NodeId peer) { AssertLockHeld(g_cs_orphans); @@ -102,13 +96,13 @@ void EraseOrphansFor(NodeId peer) std::map::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid if (maybeErase->second.fromPeer == peer) { - nErased += EraseOrphanTx(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); } -unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) +unsigned int TxOrphanage::LimitOrphans(unsigned int nMaxOrphans) { AssertLockHeld(g_cs_orphans); @@ -124,7 +118,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) { std::map::iterator maybeErase = iter++; if (maybeErase->second.nTimeExpire <= nNow) { - nErased += EraseOrphanTx(maybeErase->second.tx->GetHash()); + nErased += EraseTx(maybeErase->second.tx->GetHash()); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); } @@ -138,13 +132,13 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) { // Evict a random orphan: size_t randompos = rng.randrange(g_orphan_list.size()); - EraseOrphanTx(g_orphan_list[randompos]->first); + EraseTx(g_orphan_list[randompos]->first); ++nEvicted; } return nEvicted; } -void AddChildrenToWorkSet(const CTransaction& tx, std::set& orphan_work_set) +void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx, std::set& orphan_work_set) const { AssertLockHeld(g_cs_orphans); for (unsigned int i = 0; i < tx.vout.size(); i++) { @@ -157,7 +151,7 @@ void AddChildrenToWorkSet(const CTransaction& tx, std::set& orphan_work } } -bool HaveOrphanTx(const GenTxid& gtxid) +bool TxOrphanage::HaveTx(const GenTxid& gtxid) const { LOCK(g_cs_orphans); if (gtxid.IsWtxid()) { @@ -167,7 +161,7 @@ bool HaveOrphanTx(const GenTxid& gtxid) } } -std::pair GetOrphanTx(const uint256& txid) +std::pair TxOrphanage::GetTx(const uint256& txid) const { AssertLockHeld(g_cs_orphans); @@ -176,7 +170,7 @@ std::pair GetOrphanTx(const uint256& txid) return {it->second.tx, it->second.fromPeer}; } -void EraseOrphansForBlock(const CBlock& block) +void TxOrphanage::EraseForBlock(const CBlock& block) { LOCK(g_cs_orphans); @@ -201,7 +195,7 @@ void EraseOrphansForBlock(const CBlock& block) if (vOrphanErase.size()) { int nErased = 0; for (const uint256& orphanHash : vOrphanErase) { - nErased += EraseOrphanTx(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 f2ffbf3d67..6b9837815b 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -13,30 +13,48 @@ /** Guards orphan transactions and extra txs for compact blocks */ extern RecursiveMutex g_cs_orphans; -struct COrphanTx { - // When modifying, adapt the copy of this definition in tests/DoS_tests. - CTransactionRef tx; - NodeId fromPeer; - int64_t nTimeExpire; - size_t list_pos; -}; +/** Data structure to keep track of orphan transactions + */ +class TxOrphanage { +public: + /** Add a new orphan transaction */ + 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 EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans); + + /** Get the details of an orphan transaction (returns nullptr if not found) */ + std::pair GetTx(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -int EraseOrphanTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -void EraseOrphansFor(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -void EraseOrphansForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans); -unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -void AddChildrenToWorkSet(const CTransaction& tx, std::set& orphan_work_set) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -bool HaveOrphanTx(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans); -std::pair GetOrphanTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -bool OrphanageAddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); + /** Erase an orphan by txid */ + int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -/** Map from txid to orphan transaction record. Limited by - * -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */ -extern std::map mapOrphanTransactions GUARDED_BY(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); -/** Index from wtxid into the mapOrphanTransactions to lookup orphan - * transactions using their witness ids. */ -extern std::map::iterator> g_orphans_by_wtxid GUARDED_BY(g_cs_orphans); + /** Erase all orphans included in / invalidated by a new block */ + void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!g_cs_orphans); + + /** Limit the orphanage to the given maximum */ + unsigned int LimitOrphans(unsigned int nMaxOrphans) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); + + /** Add any orphans that list a particular tx as a parent into a peer's work set + * (ie orphans that may have found their final missing parent, and so should be reconsidered for the mempool) */ + void AddChildrenToWorkSet(const CTransaction& tx, std::set& orphan_work_set) const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); + +protected: + struct COrphanTx { + CTransactionRef tx; + NodeId fromPeer; + int64_t nTimeExpire; + size_t list_pos; + }; + + /** Map from txid to orphan transaction record. Limited by + * -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */ + std::map mapOrphanTransactions GUARDED_BY(g_cs_orphans); + + using OrphanMap = decltype(mapOrphanTransactions); struct IteratorComparator { @@ -49,9 +67,14 @@ extern std::map::iterator> g_orphans_by_wt /** Index from the parents' COutPoint into the mapOrphanTransactions. Used * to remove orphan transactions from the mapOrphanTransactions */ - extern std::map::iterator, IteratorComparator>> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans); + std::map> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans); /** Orphan transactions in vector for quick random eviction */ - extern std::vector::iterator> g_orphan_list GUARDED_BY(g_cs_orphans); + std::vector g_orphan_list GUARDED_BY(g_cs_orphans); + + /** Index from wtxid into the mapOrphanTransactions to lookup orphan + * transactions using their witness ids. */ + std::map g_orphans_by_wtxid GUARDED_BY(g_cs_orphans); +}; #endif // BITCOIN_TXORPHANAGE_H diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py index cca7390ae3..8783c244c3 100755 --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -154,7 +154,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): orphan_tx_pool[i].vin.append(CTxIn(outpoint=COutPoint(i, 333))) orphan_tx_pool[i].vout.append(CTxOut(nValue=11 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) - with node.assert_debug_log(['mapOrphan overflow, removed 1 tx']): + with node.assert_debug_log(['orphanage overflow, removed 1 tx']): node.p2ps[0].send_txs_and_test(orphan_tx_pool, node, success=False) rejected_parent = CTransaction() -- cgit v1.2.3