diff --git a/src/net_processing.cpp b/src/net_processing.cpp --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -808,6 +808,9 @@ /** Number of peers from which we're downloading blocks. */ int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0; + /** Storage for orphan information */ + TxOrphanage m_orphanage; + /** * Checks if address relay is permitted with peer. If needed, initializes * the m_addr_known bloom filter and sets m_addr_relay_enabled to true. @@ -1640,7 +1643,7 @@ 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 -= @@ -1731,11 +1734,6 @@ return true; } -////////////////////////////////////////////////////////////////////////////// -// -// mapOrphanTransactions -// - static void AddToCompactExtraTransactions(const CTransactionRef &tx) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) { size_t max_extra_txn = gArgs.GetArg("-blockreconstructionextratxn", @@ -1962,13 +1960,13 @@ } /** - * 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(); { @@ -2159,7 +2157,7 @@ recentRejects->reset(); } - if (HaveOrphanTx(txid)) { + if (m_orphanage.HaveTx(txid)) { return true; } @@ -2919,7 +2917,7 @@ const TxId orphanTxId = *orphan_work_set.begin(); orphan_work_set.erase(orphan_work_set.begin()); - const auto [porphanTx, from_peer] = GetOrphanTx(orphanTxId); + const auto [porphanTx, from_peer] = m_orphanage.GetTx(orphanTxId); if (porphanTx == nullptr) { continue; } @@ -2930,8 +2928,8 @@ LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanTxId.ToString()); RelayTransaction(orphanTxId, m_connman); - AddChildrenToWorkSet(*porphanTx, orphan_work_set); - EraseOrphanTx(orphanTxId); + m_orphanage.AddChildrenToWorkSet(*porphanTx, orphan_work_set); + m_orphanage.EraseTx(orphanTxId); break; } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { if (state.IsInvalid()) { @@ -2949,7 +2947,7 @@ assert(recentRejects); recentRejects->insert(orphanTxId); - EraseOrphanTx(orphanTxId); + m_orphanage.EraseTx(orphanTxId); break; } } @@ -4231,7 +4229,7 @@ // about any requests for it. m_txrequest.ForgetInvId(tx.GetId()); RelayTransaction(tx.GetId(), m_connman); - AddChildrenToWorkSet(tx, peer->m_orphan_work_set); + m_orphanage.AddChildrenToWorkSet(tx, peer->m_orphan_work_set); pfrom.m_last_tx_time = GetTime(); @@ -4278,7 +4276,7 @@ } } - if (OrphanageAddTx(ptx, pfrom.GetId())) { + if (m_orphanage.AddTx(ptx, pfrom.GetId())) { AddToCompactExtraTransactions(ptx); } @@ -4286,15 +4284,15 @@ // AlreadyHave, and we shouldn't request it anymore. m_txrequest.ForgetInvId(tx.GetId()); - // DoS prevention: do not allow mapOrphanTransactions to grow + // 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); + "orphanage overflow, removed %u tx\n", nEvicted); } } else { LogPrint(BCLog::MEMPOOL, @@ -6808,14 +6806,3 @@ } // release cs_main return true; } - -class CNetProcessingCleanup { -public: - CNetProcessingCleanup() {} - ~CNetProcessingCleanup() { - // orphan transactions - mapOrphanTransactions.clear(); - mapOrphanTransactionsByPrev.clear(); - } -}; -static CNetProcessingCleanup instance_of_cnetprocessingcleanup; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -334,16 +334,24 @@ peerLogic->FinalizeNode(config, dummyNode, dummy); } -static CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) { - std::map::iterator it; - it = mapOrphanTransactions.lower_bound(TxId{InsecureRand256()}); - if (it == mapOrphanTransactions.end()) { - it = mapOrphanTransactions.begin(); +class TxOrphanageTest : public TxOrphanage { +public: + inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) { + return mapOrphanTransactions.size(); } - return it->second.tx; -} + + CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) { + std::map::iterator it; + it = mapOrphanTransactions.lower_bound(TxId{InsecureRand256()}); + if (it == mapOrphanTransactions.end()) { + it = mapOrphanTransactions.begin(); + } + return it->second.tx; + } +}; BOOST_AUTO_TEST_CASE(DoS_mapOrphans) { + TxOrphanageTest orphanage; CKey key; key.MakeNewKey(true); FillableSigningProvider keystore; @@ -362,12 +370,12 @@ 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); @@ -379,12 +387,12 @@ BOOST_CHECK(SignSignature(keystore, *txPrev, tx, 0, SigHashType().withForkId())); - 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); @@ -403,23 +411,23 @@ 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.h b/src/txorphanage.h --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -13,50 +13,77 @@ /** 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); -int EraseOrphanTx(const TxId &txid) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -void EraseOrphansFor(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -void EraseOrphansForBlock(const CBlock &block) LOCKS_EXCLUDED(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 TxId &txid) LOCKS_EXCLUDED(g_cs_orphans); -std::pair GetOrphanTx(const TxId &txid) - EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -bool OrphanageAddTx(const CTransactionRef &tx, NodeId peer) - 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); - -struct IteratorComparator { - template bool operator()(const I &a, const I &b) const { - return &(*a) < &(*b); - } -}; + /** Check if we already have an orphan transaction */ + bool HaveTx(const TxId &txid) const LOCKS_EXCLUDED(g_cs_orphans); + + /** + * Get the details of an orphan transaction (returns nullptr if not found) + */ + std::pair GetTx(const TxId &txid) const + EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); + + /** Erase an orphan by txid */ + int EraseTx(const TxId &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); + + /** Erase all orphans included in / invalidated by a new block */ + void EraseForBlock(const CBlock &block) LOCKS_EXCLUDED(g_cs_orphans); + + /** Limit the orphanage to the given maximum */ + unsigned int LimitOrphans(unsigned int nMaxOrphans) + EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); -/** - * 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); - -/** Orphan transactions in vector for quick random eviction */ -extern std::vector::iterator> - g_orphan_list GUARDED_BY(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 { + template bool operator()(const I &a, const I &b) const { + return &(*a) < &(*b); + } + }; + + /** + * Index from the parents' COutPoint into the mapOrphanTransactions. Used + * to remove orphan transactions from the mapOrphanTransactions + */ + std::map> + mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans); + + /** Orphan transactions in vector for quick random eviction */ + std::vector g_orphan_list GUARDED_BY(g_cs_orphans); +}; #endif // BITCOIN_TXORPHANAGE_H diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -17,16 +17,7 @@ RecursiveMutex g_cs_orphans; -std::map mapOrphanTransactions 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); const TxId &txid = tx->GetId(); @@ -63,7 +54,8 @@ return true; } -int EraseOrphanTx(const TxId &txid) { +int TxOrphanage::EraseTx(const TxId &txid) { + AssertLockHeld(g_cs_orphans); std::map::iterator it = mapOrphanTransactions.find(txid); if (it == mapOrphanTransactions.end()) { return 0; @@ -94,7 +86,7 @@ return 1; } -void EraseOrphansFor(NodeId peer) { +void TxOrphanage::EraseForPeer(NodeId peer) { AssertLockHeld(g_cs_orphans); int nErased = 0; @@ -103,7 +95,7 @@ std::map::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid if (maybeErase->second.fromPeer == peer) { - nErased += EraseOrphanTx(maybeErase->second.tx->GetId()); + nErased += EraseTx(maybeErase->second.tx->GetId()); } } if (nErased > 0) { @@ -112,7 +104,7 @@ } } -unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) { +unsigned int TxOrphanage::LimitOrphans(unsigned int nMaxOrphans) { AssertLockHeld(g_cs_orphans); unsigned int nEvicted = 0; @@ -128,7 +120,7 @@ while (iter != mapOrphanTransactions.end()) { std::map::iterator maybeErase = iter++; if (maybeErase->second.nTimeExpire <= nNow) { - nErased += EraseOrphanTx(maybeErase->second.tx->GetId()); + nErased += EraseTx(maybeErase->second.tx->GetId()); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); @@ -146,14 +138,14 @@ while (mapOrphanTransactions.size() > 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 (size_t i = 0; i < tx.vout.size(); i++) { const auto it_by_prev = @@ -166,12 +158,12 @@ } } -bool HaveOrphanTx(const TxId &txid) { +bool TxOrphanage::HaveTx(const TxId &txid) const { LOCK(g_cs_orphans); return mapOrphanTransactions.count(txid); } -std::pair GetOrphanTx(const TxId &txid) { +std::pair TxOrphanage::GetTx(const TxId &txid) const { AssertLockHeld(g_cs_orphans); const auto it = mapOrphanTransactions.find(txid); @@ -181,7 +173,7 @@ return {it->second.tx, it->second.fromPeer}; } -void EraseOrphansForBlock(const CBlock &block) { +void TxOrphanage::EraseForBlock(const CBlock &block) { LOCK(g_cs_orphans); std::vector vOrphanErase; @@ -209,7 +201,7 @@ if (vOrphanErase.size()) { int nErased = 0; for (const auto &orphanId : vOrphanErase) { - nErased += EraseOrphanTx(orphanId); + nErased += EraseTx(orphanId); } LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx included or conflicted by block\n", diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -189,7 +189,7 @@ scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) pad_tx(orphan_tx_pool[i]) - 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()