From 77ebe8f2801215162fe7c00f2dfd35366c4a91f7 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 16 May 2025 10:36:05 -0400 Subject: [PATCH] [prep/test] have TxOrphanage remember its own limits in LimitOrphans Move towards a model where TxOrphanage is initialized with limits that it remembers throughout its lifetime. Remove the param. Limiting by number of unique orphans will be removed in a later commit. Now that -maxorphantx is gone, this does not change the node behavior. The parameter is only used in tests. --- src/net_processing.cpp | 2 +- src/net_processing.h | 2 -- src/node/txdownloadman.h | 2 -- src/node/txdownloadman_impl.cpp | 2 +- src/node/txorphanage.cpp | 4 ++-- src/node/txorphanage.h | 4 ++-- src/test/fuzz/txdownloadman.cpp | 7 +++---- src/test/fuzz/txorphan.cpp | 5 ++--- src/test/orphanage_tests.cpp | 23 ++++++----------------- src/test/txdownload_tests.cpp | 4 ++-- 10 files changed, 19 insertions(+), 36 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7186f2aa1e9..10fbae5e7ed 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1925,7 +1925,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, m_banman(banman), m_chainman(chainman), m_mempool(pool), - m_txdownloadman(node::TxDownloadOptions{pool, m_rng, opts.max_orphan_txs, opts.deterministic_rng}), + m_txdownloadman(node::TxDownloadOptions{pool, m_rng, opts.deterministic_rng}), m_warnings{warnings}, m_opts{opts} { diff --git a/src/net_processing.h b/src/net_processing.h index fea7c6fa1db..9cf79512172 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -76,8 +76,6 @@ public: bool ignore_incoming_txs{DEFAULT_BLOCKSONLY}; //! Whether transaction reconciliation protocol is enabled bool reconcile_txs{DEFAULT_TXRECONCILIATION_ENABLE}; - //! Maximum number of orphan transactions kept in memory - uint32_t max_orphan_txs{node::DEFAULT_MAX_ORPHAN_TRANSACTIONS}; //! Number of non-mempool transactions to keep around for block reconstruction. Includes //! orphan, replaced, and rejected transactions. uint32_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN}; diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 0b9d828616f..3ebf66324a8 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -41,8 +41,6 @@ struct TxDownloadOptions { const CTxMemPool& m_mempool; /** RNG provided by caller. */ FastRandomContext& m_rng; - /** Maximum number of transactions allowed in orphanage. */ - const uint32_t m_max_orphan_txs; /** Instantiate TxRequestTracker as deterministic (used for tests). */ bool m_deterministic_txrequest{false}; }; diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 5bb435f5321..1c106c9eb7c 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -422,7 +422,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) // Note that, if the orphanage reaches capacity, it's possible that we immediately evict // the transaction we just added. - m_orphanage.LimitOrphans(m_opts.m_max_orphan_txs, m_opts.m_rng); + m_orphanage.LimitOrphans(m_opts.m_rng); } else { unique_parents.clear(); LogDebug(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index 6c6c4a48840..86e7c04332a 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -141,7 +141,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) if (nErased > 0) LogDebug(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", nErased, peer); } -void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) +void TxOrphanage::LimitOrphans(FastRandomContext& rng) { unsigned int nEvicted = 0; auto nNow{Now()}; @@ -163,7 +163,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) m_next_sweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL; if (nErased > 0) LogDebug(BCLog::TXPACKAGES, "Erased %d orphan tx due to expiration\n", nErased); } - while (m_orphans.size() > max_orphans) + while (m_orphans.size() > DEFAULT_MAX_ORPHAN_TRANSACTIONS) { // Evict a random orphan: size_t randompos = rng.randrange(m_orphan_list.size()); diff --git a/src/node/txorphanage.h b/src/node/txorphanage.h index f168d2e1055..48eafd081db 100644 --- a/src/node/txorphanage.h +++ b/src/node/txorphanage.h @@ -62,8 +62,8 @@ public: /** Erase all orphans included in or invalidated by a new block */ void EraseForBlock(const CBlock& block); - /** Limit the orphanage to the given maximum */ - void LimitOrphans(unsigned int max_orphans, FastRandomContext& rng); + /** Limit the orphanage to DEFAULT_MAX_ORPHAN_TRANSACTIONS. */ + void LimitOrphans(FastRandomContext& rng); /** Add any orphans that list a particular tx as a parent into the from peer's work set */ void AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng); diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index c5d66e9fa0a..277fc7161d6 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -173,9 +173,8 @@ FUZZ_TARGET(txdownloadman, .init = initialize) // Initialize txdownloadman bilingual_str error; CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error}; - const auto max_orphan_count = fuzzed_data_provider.ConsumeIntegralInRange(0, 300); FastRandomContext det_rand{true}; - node::TxDownloadManager txdownloadman{node::TxDownloadOptions{pool, det_rand, max_orphan_count, true}}; + node::TxDownloadManager txdownloadman{node::TxDownloadOptions{pool, det_rand, true}}; std::chrono::microseconds time{244466666}; @@ -304,9 +303,9 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) // Initialize a TxDownloadManagerImpl bilingual_str error; CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error}; - const auto max_orphan_count = fuzzed_data_provider.ConsumeIntegralInRange(0, 300); + const auto max_orphan_count = node::DEFAULT_MAX_ORPHAN_TRANSACTIONS; FastRandomContext det_rand{true}; - node::TxDownloadManagerImpl txdownload_impl{node::TxDownloadOptions{pool, det_rand, max_orphan_count, true}}; + node::TxDownloadManagerImpl txdownload_impl{node::TxDownloadOptions{pool, det_rand, true}}; std::chrono::microseconds time{244466666}; diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 26486af7635..c3521ff5205 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -217,9 +217,8 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) [&] { // test mocktime and expiry SetMockTime(ConsumeTime(fuzzed_data_provider)); - auto limit = fuzzed_data_provider.ConsumeIntegral(); - orphanage.LimitOrphans(limit, orphanage_rng); - Assert(orphanage.Size() <= limit); + orphanage.LimitOrphans(orphanage_rng); + Assert(orphanage.Size() <= node::DEFAULT_MAX_ORPHAN_TRANSACTIONS); }); } diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 68cffcfb422..1ab22cb3e88 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -181,34 +181,23 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) // Test LimitOrphanTxSize() function, nothing should timeout: FastRandomContext rng{/*fDeterministic=*/true}; - orphanage.LimitOrphans(/*max_orphans=*/expected_num_orphans, rng); + orphanage.LimitOrphans(rng); BOOST_CHECK_EQUAL(orphanage.Size(), expected_num_orphans); - expected_num_orphans -= 1; - orphanage.LimitOrphans(/*max_orphans=*/expected_num_orphans, rng); - BOOST_CHECK_EQUAL(orphanage.Size(), expected_num_orphans); - assert(expected_num_orphans > 40); - orphanage.LimitOrphans(40, rng); - BOOST_CHECK_EQUAL(orphanage.Size(), 40); - orphanage.LimitOrphans(10, rng); - BOOST_CHECK_EQUAL(orphanage.Size(), 10); - orphanage.LimitOrphans(0, rng); - BOOST_CHECK_EQUAL(orphanage.Size(), 0); // Add one more orphan, check timeout logic auto timeout_tx = MakeTransactionSpending(/*outpoints=*/{}, rng); orphanage.AddTx(timeout_tx, 0); - orphanage.LimitOrphans(1, rng); - BOOST_CHECK_EQUAL(orphanage.Size(), 1); + expected_num_orphans += 1; + BOOST_CHECK_EQUAL(orphanage.Size(), expected_num_orphans); // One second shy of expiration SetMockTime(now + node::ORPHAN_TX_EXPIRE_TIME - 1s); - orphanage.LimitOrphans(1, rng); - BOOST_CHECK_EQUAL(orphanage.Size(), 1); + orphanage.LimitOrphans(rng); + BOOST_CHECK_EQUAL(orphanage.Size(), expected_num_orphans); // Jump one more second, orphan should be timed out on limiting SetMockTime(now + node::ORPHAN_TX_EXPIRE_TIME); - BOOST_CHECK_EQUAL(orphanage.Size(), 1); - orphanage.LimitOrphans(1, rng); + orphanage.LimitOrphans(rng); BOOST_CHECK_EQUAL(orphanage.Size(), 0); } diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index e5e0d652d60..8130e443ed6 100644 --- a/src/test/txdownload_tests.cpp +++ b/src/test/txdownload_tests.cpp @@ -114,7 +114,7 @@ BOOST_FIXTURE_TEST_CASE(tx_rejection_types, TestChain100Setup) { CTxMemPool& pool = *Assert(m_node.mempool); FastRandomContext det_rand{true}; - node::TxDownloadOptions DEFAULT_OPTS{pool, det_rand, node::DEFAULT_MAX_ORPHAN_TRANSACTIONS, true}; + node::TxDownloadOptions DEFAULT_OPTS{pool, det_rand, true}; // A new TxDownloadManagerImpl is created for each tx so we can just reuse the same one. TxValidationState state; @@ -172,7 +172,7 @@ BOOST_FIXTURE_TEST_CASE(handle_missing_inputs, TestChain100Setup) { CTxMemPool& pool = *Assert(m_node.mempool); FastRandomContext det_rand{true}; - node::TxDownloadOptions DEFAULT_OPTS{pool, det_rand, node::DEFAULT_MAX_ORPHAN_TRANSACTIONS, true}; + node::TxDownloadOptions DEFAULT_OPTS{pool, det_rand, true}; NodeId nodeid{1}; node::TxDownloadConnectionInfo DEFAULT_CONN{/*m_preferred=*/false, /*m_relay_permissions=*/false, /*m_wtxid_relay=*/true};