From d1fac25ff3c3ac090b68e370efc6dd9374b6ad3b Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 30 Jun 2025 09:59:33 -0400 Subject: [PATCH 01/12] [doc] 31829 release note --- doc/release-notes-31829.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 doc/release-notes-31829.md diff --git a/doc/release-notes-31829.md b/doc/release-notes-31829.md new file mode 100644 index 00000000000..9c20a909785 --- /dev/null +++ b/doc/release-notes-31829.md @@ -0,0 +1,11 @@ +P2P + +- The transaction orphanage, which holds transactions with missing inputs temporarily while the node attempts to fetch +its parents, now has improved Denial of Service protections. Previously, it enforced a maximum number of unique +transactions (default 100, configurable using `-maxorphantx`). Now, its limits are as follows: the number of entries +(unique by wtxid and peer), plus each unique transaction's input count divided by 10, must not exceed 3,000. The total +weight of unique transactions must not exceed 404,000 Wu multiplied by the number of peers. + +- The `-maxorphantx` option no longer has any effect, since the orphanage no longer limits the number of unique +transactions. Users should remove this configuration option if they were using it, as the setting will cause an +error in future versions when it is no longer recognized. From af7402ccfa7f19177b5f422f596a3ab2bd1e9633 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 3 Jul 2025 16:06:21 -0400 Subject: [PATCH 02/12] [refactor] make TxOrphanage keep itself trimmed --- src/bench/txorphanage.cpp | 2 - src/node/txdownloadman_impl.cpp | 4 - src/node/txorphanage.cpp | 41 ++++++- src/node/txorphanage.h | 3 - src/test/fuzz/txorphan.cpp | 183 ++++++++++++-------------------- src/test/orphanage_tests.cpp | 58 ++++------ 6 files changed, 125 insertions(+), 166 deletions(-) diff --git a/src/bench/txorphanage.cpp b/src/bench/txorphanage.cpp index b9bb7b41c22..d2ded290e66 100644 --- a/src/bench/txorphanage.cpp +++ b/src/bench/txorphanage.cpp @@ -97,7 +97,6 @@ static void OrphanageSinglePeerEviction(benchmark::Bench& bench) // Lastly, add the large transaction. const auto num_announcements_before_trim{orphanage->CountAnnouncements()}; assert(orphanage->AddTx(large_tx, peer)); - orphanage->LimitOrphans(); // If there are multiple peers, note that they all have the same DoS score. We will evict only 1 item at a time for each new DoSiest peer. const auto num_announcements_after_trim{orphanage->CountAnnouncements()}; @@ -178,7 +177,6 @@ static void OrphanageMultiPeerEviction(benchmark::Bench& bench) const auto num_announcements_before_trim{orphanage->CountAnnouncements()}; // There is a small gap between the total usage and the max usage. Add a transaction to fill it. assert(orphanage->AddTx(last_tx, 0)); - orphanage->LimitOrphans(); // If there are multiple peers, note that they all have the same DoS score. We will evict only 1 item at a time for each new DoSiest peer. const auto num_evicted{num_announcements_before_trim - orphanage->CountAnnouncements() + 1}; diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 3822cc81fdc..99fa036bda3 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -188,7 +188,6 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, if (MaybeAddOrphanResolutionCandidate(unique_parents, *wtxid, peer, now)) { m_orphanage->AddAnnouncer(orphan_tx->GetWitnessHash(), peer); - m_orphanage->LimitOrphans(); } // Return even if the peer isn't an orphan resolution candidate. This would be caught by AlreadyHaveTx. @@ -419,9 +418,6 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore. m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); - - // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) - m_orphanage->LimitOrphans(); } 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 6093be46ce4..9b95b52b477 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -172,11 +172,18 @@ class TxOrphanageImpl final : public TxOrphanage { template void Erase(Iter it); + /** Erase by wtxid. */ + bool EraseTxInternal(const Wtxid& wtxid); + /** Check if there is exactly one announcement with the same wtxid as it. */ bool IsUnique(Iter it) const; /** Check if the orphanage needs trimming. */ bool NeedsTrim() const; + + /** Limit the orphanage to MaxGlobalLatencyScore and MaxGlobalUsage. */ + void LimitOrphans(); + public: TxOrphanageImpl() = default; TxOrphanageImpl(Count max_global_ann, Usage reserved_peer_usage) : @@ -216,7 +223,6 @@ public: bool EraseTx(const Wtxid& wtxid) override; void EraseForPeer(NodeId peer) override; void EraseForBlock(const CBlock& block) override; - void LimitOrphans() override; std::vector> AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng) override; bool HaveTxToReconsider(NodeId peer) override; std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const override; @@ -332,6 +338,9 @@ bool TxOrphanageImpl::AddTx(const CTransactionRef& tx, NodeId peer) peer, txid.ToString(), wtxid.ToString()); Assume(!IsUnique(iter)); } + + // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) + LimitOrphans(); return brand_new; } @@ -360,10 +369,13 @@ bool TxOrphanageImpl::AddAnnouncer(const Wtxid& wtxid, NodeId peer) peer, txid.ToString(), wtxid.ToString()); Assume(!IsUnique(iter)); + + // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) + LimitOrphans(); return true; } -bool TxOrphanageImpl::EraseTx(const Wtxid& wtxid) +bool TxOrphanageImpl::EraseTxInternal(const Wtxid& wtxid) { auto& index_by_wtxid = m_orphans.get(); @@ -378,12 +390,21 @@ bool TxOrphanageImpl::EraseTx(const Wtxid& wtxid) Erase(it++); num_ann += 1; } - LogDebug(BCLog::TXPACKAGES, "removed orphan tx %s (wtxid=%s) (%u announcements)\n", txid.ToString(), wtxid.ToString(), num_ann); return true; } +bool TxOrphanageImpl::EraseTx(const Wtxid& wtxid) +{ + const auto ret = EraseTxInternal(wtxid); + + // Deletions can cause the orphanage's MaxGlobalUsage to decrease, so we may need to trim here. + LimitOrphans(); + + return ret; +} + /** Erase all entries by this peer. */ void TxOrphanageImpl::EraseForPeer(NodeId peer) { @@ -400,6 +421,9 @@ void TxOrphanageImpl::EraseForPeer(NodeId peer) Assume(!m_peer_orphanage_info.contains(peer)); if (num_ann > 0) LogDebug(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", num_ann, peer); + + // Deletions can cause the orphanage's MaxGlobalUsage to decrease, so we may need to trim here. + LimitOrphans(); } /** If the data structure needs trimming, evicts announcements by selecting the DoSiest peer and evicting its oldest @@ -565,6 +589,7 @@ bool TxOrphanageImpl::HaveTxToReconsider(NodeId peer) auto it = m_orphans.get().lower_bound(ByPeerView{peer, true, 0}); return it != m_orphans.get().end() && it->m_announcer == peer && it->m_reconsider; } + void TxOrphanageImpl::EraseForBlock(const CBlock& block) { std::set wtxids_to_erase; @@ -583,13 +608,19 @@ void TxOrphanageImpl::EraseForBlock(const CBlock& block) unsigned int num_erased{0}; for (const auto& wtxid : wtxids_to_erase) { - num_erased += EraseTx(wtxid) ? 1 : 0; + // Don't use EraseTx here because it calls LimitOrphans and announcements deleted in that call are not reflected + // in its return result. Waiting until the end to do LimitOrphans helps save repeated computation and allows us + // to check that num_erased is what we expect. + num_erased += EraseTxInternal(wtxid) ? 1 : 0; } if (num_erased != 0) { LogDebug(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) included or conflicted by block\n", num_erased); } Assume(wtxids_to_erase.size() == num_erased); + + // Deletions can cause the orphanage's MaxGlobalUsage to decrease, so we may need to trim here. + LimitOrphans(); } /** Get all children that spend from this tx and were received from nodeid. Sorted from most @@ -697,6 +728,8 @@ void TxOrphanageImpl::SanityCheck() const const auto summed_peer_latency_score = std::accumulate(m_peer_orphanage_info.begin(), m_peer_orphanage_info.end(), TxOrphanage::Count{0}, [](TxOrphanage::Count sum, const auto pair) { return sum + pair.second.m_total_latency_score; }); assert(summed_peer_latency_score >= m_unique_rounded_input_scores + m_orphans.size()); + + assert(!NeedsTrim()); } TxOrphanage::Count TxOrphanageImpl::MaxGlobalLatencyScore() const { return m_max_global_latency_score; } diff --git a/src/node/txorphanage.h b/src/node/txorphanage.h index ae639213944..584fb390d34 100644 --- a/src/node/txorphanage.h +++ b/src/node/txorphanage.h @@ -88,9 +88,6 @@ public: /** Erase all orphans included in or invalidated by a new block */ virtual void EraseForBlock(const CBlock& block) = 0; - /** Limit the orphanage to MaxGlobalLatencyScore and MaxGlobalUsage. */ - virtual void LimitOrphans() = 0; - /** Add any orphans that list a particular tx as a parent into the from peer's work set */ virtual std::vector> AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng) = 0; diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 36d7af6cc7b..a9bd762e645 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -122,37 +122,34 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) }, [&] { bool have_tx = orphanage->HaveTx(tx->GetWitnessHash()); + bool have_tx_and_peer = orphanage->HaveTxFromPeer(wtxid, peer_id); // 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 { bool add_tx = orphanage->AddTx(tx, peer_id); // have_tx == true -> add_tx == false Assert(!have_tx || !add_tx); + // have_tx_and_peer == true -> add_tx == false + Assert(!have_tx_and_peer || !add_tx); + // After AddTx, the orphanage may trim itself, so the peer's usage may have gone up or down. if (add_tx) { - Assert(orphanage->UsageByPeer(peer_id) == tx_weight + total_peer_bytes_start); - Assert(orphanage->TotalOrphanUsage() == tx_weight + total_bytes_start); Assert(tx_weight <= MAX_STANDARD_TX_WEIGHT); } else { // Peer may have been added as an announcer. - if (orphanage->UsageByPeer(peer_id) == tx_weight + total_peer_bytes_start) { + if (orphanage->UsageByPeer(peer_id) > total_peer_bytes_start) { Assert(orphanage->HaveTxFromPeer(wtxid, peer_id)); - } else { - // Otherwise, there must not be any change to the peer byte count. - Assert(orphanage->UsageByPeer(peer_id) == total_peer_bytes_start); } - // Regardless, total bytes should not have changed. - Assert(orphanage->TotalOrphanUsage() == total_bytes_start); + // If announcement was added, total bytes does not increase. + // However, if eviction was triggered, the value may decrease. + Assert(orphanage->TotalOrphanUsage() <= total_bytes_start); } } - have_tx = orphanage->HaveTx(tx->GetWitnessHash()); - { - bool add_tx = orphanage->AddTx(tx, peer_id); - // if have_tx is still false, it must be too big - Assert(!have_tx == (tx_weight > MAX_STANDARD_TX_WEIGHT)); - Assert(!have_tx || !add_tx); - } + // We are not guaranteed to have_tx after AddTx. There are a few possibile reasons: + // - tx itself exceeds the per-peer memory usage limit, so LimitOrphans had to remove it immediately + // - tx itself exceeds the per-peer latency score limit, so LimitOrphans had to remove it immediately + // - the orphanage needed trim and all other announcements from this peer are reconsiderable }, [&] { bool have_tx = orphanage->HaveTx(tx->GetWitnessHash()); @@ -165,14 +162,9 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) // have_tx_and_peer == true -> added_announcer == false Assert(!have_tx_and_peer || !added_announcer); - // Total bytes should not have changed. If peer was added as announcer, byte - // accounting must have been updated. - Assert(orphanage->TotalOrphanUsage() == total_bytes_start); - if (added_announcer) { - Assert(orphanage->UsageByPeer(peer_id) == tx_weight + total_peer_bytes_start); - } else { - Assert(orphanage->UsageByPeer(peer_id) == total_peer_bytes_start); - } + // If announcement was added, total bytes does not increase. + // However, if eviction was triggered, the value may decrease. + Assert(orphanage->TotalOrphanUsage() <= total_bytes_start); } }, [&] { @@ -182,15 +174,11 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) { auto bytes_from_peer_before{orphanage->UsageByPeer(peer_id)}; Assert(have_tx == orphanage->EraseTx(tx->GetWitnessHash())); + // After EraseTx, the orphanage may trim itself, so all peers' usage may have gone up or down. if (have_tx) { - Assert(orphanage->TotalOrphanUsage() == total_bytes_start - tx_weight); - if (have_tx_and_peer) { - Assert(orphanage->UsageByPeer(peer_id) == bytes_from_peer_before - tx_weight); - } else { + if (!have_tx_and_peer) { Assert(orphanage->UsageByPeer(peer_id) == bytes_from_peer_before); } - } else { - Assert(orphanage->TotalOrphanUsage() == total_bytes_start); } } have_tx = orphanage->HaveTx(tx->GetWitnessHash()); @@ -218,13 +206,8 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) Assert(!orphanage->HaveTx(tx_removed->GetWitnessHash())); Assert(!orphanage->HaveTxFromPeer(tx_removed->GetWitnessHash(), peer_id)); } - }, - [&] { - // test mocktime and expiry - SetMockTime(ConsumeTime(fuzzed_data_provider)); - orphanage->LimitOrphans(); - }); - + } + ); } // Set tx as potential parent to be used for future GetChildren() calls. @@ -369,33 +352,8 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage) Assert(orphanage->LatencyScoreFromPeer(peer_id) == 0); Assert(orphanage->AnnouncementsFromPeer(peer_id) == 0); } - }, - [&] { // LimitOrphans - // Assert that protected peers are never affected by LimitOrphans. - unsigned int protected_count = 0; - unsigned int protected_bytes = 0; - for (unsigned int peer = 0; peer < num_peers; ++peer) { - if (protected_peers[peer]) { - protected_count += orphanage->LatencyScoreFromPeer(peer); - protected_bytes += orphanage->UsageByPeer(peer); - } - } - orphanage->LimitOrphans(); - Assert(orphanage->TotalLatencyScore() <= global_latency_score_limit); - Assert(orphanage->TotalOrphanUsage() <= per_peer_weight_reservation * num_peers); - - // Number of announcements and usage should never differ before and after since - // we've never exceeded the per-peer reservations. - for (unsigned int peer = 0; peer < num_peers; ++peer) { - if (protected_peers[peer]) { - protected_count -= orphanage->LatencyScoreFromPeer(peer); - protected_bytes -= orphanage->UsageByPeer(peer); - } - } - Assert(protected_count == 0); - Assert(protected_bytes == 0); - }); - + } + ); } } @@ -727,59 +685,56 @@ FUZZ_TARGET(txorphanage_sim) assert(!have_reconsider_fn(peer)); } break; - } else if (command-- == 0) { - // LimitOrphans - const auto max_ann = max_global_ann / std::max(1, count_peers_fn()); - const auto max_mem = reserved_peer_usage; - while (true) { - // Count global usage and number of peers. - node::TxOrphanage::Usage total_usage{0}; - node::TxOrphanage::Count total_latency_score = sim_announcements.size(); - for (unsigned tx = 0; tx < NUM_TX; ++tx) { - if (have_tx_fn(tx)) { - total_usage += GetTransactionWeight(*txn[tx]); - total_latency_score += txn[tx]->vin.size() / 10; - } - } - auto num_peers = count_peers_fn(); - bool oversized = (total_usage > reserved_peer_usage * num_peers) || - (total_latency_score > real->MaxGlobalLatencyScore()); - if (!oversized) break; - // Find worst peer. - FeeFrac worst_dos_score{0, 1}; - unsigned worst_peer = unsigned(-1); - for (unsigned peer = 0; peer < NUM_PEERS; ++peer) { - auto dos_score = dos_score_fn(peer, max_ann, max_mem); - // Use >= so that the more recent peer (higher NodeId) wins in case of - // ties. - if (dos_score >= worst_dos_score) { - worst_dos_score = dos_score; - worst_peer = peer; - } - } - assert(worst_peer != unsigned(-1)); - assert(worst_dos_score >> FeeFrac(1, 1)); - // Find oldest announcement from worst_peer, preferring non-reconsiderable ones. - bool done{false}; - for (int reconsider = 0; reconsider < 2; ++reconsider) { - for (auto it = sim_announcements.begin(); it != sim_announcements.end(); ++it) { - if (it->announcer != worst_peer || it->reconsider != reconsider) continue; - sim_announcements.erase(it); - done = true; - break; - } - if (done) break; - } - assert(done); - } - real->LimitOrphans(); - // We must now be within limits, otherwise LimitOrphans should have continued further). - // We don't check the contents of the orphanage until the end to make fuzz runs faster. - assert(real->TotalLatencyScore() <= real->MaxGlobalLatencyScore()); - assert(real->TotalOrphanUsage() <= real->MaxGlobalUsage()); - break; } } + // Always trim after each command if needed. + const auto max_ann = max_global_ann / std::max(1, count_peers_fn()); + const auto max_mem = reserved_peer_usage; + while (true) { + // Count global usage and number of peers. + node::TxOrphanage::Usage total_usage{0}; + node::TxOrphanage::Count total_latency_score = sim_announcements.size(); + for (unsigned tx = 0; tx < NUM_TX; ++tx) { + if (have_tx_fn(tx)) { + total_usage += GetTransactionWeight(*txn[tx]); + total_latency_score += txn[tx]->vin.size() / 10; + } + } + auto num_peers = count_peers_fn(); + bool oversized = (total_usage > reserved_peer_usage * num_peers) || + (total_latency_score > real->MaxGlobalLatencyScore()); + if (!oversized) break; + // Find worst peer. + FeeFrac worst_dos_score{0, 1}; + unsigned worst_peer = unsigned(-1); + for (unsigned peer = 0; peer < NUM_PEERS; ++peer) { + auto dos_score = dos_score_fn(peer, max_ann, max_mem); + // Use >= so that the more recent peer (higher NodeId) wins in case of + // ties. + if (dos_score >= worst_dos_score) { + worst_dos_score = dos_score; + worst_peer = peer; + } + } + assert(worst_peer != unsigned(-1)); + assert(worst_dos_score >> FeeFrac(1, 1)); + // Find oldest announcement from worst_peer, preferring non-reconsiderable ones. + bool done{false}; + for (int reconsider = 0; reconsider < 2; ++reconsider) { + for (auto it = sim_announcements.begin(); it != sim_announcements.end(); ++it) { + if (it->announcer != worst_peer || it->reconsider != reconsider) continue; + sim_announcements.erase(it); + done = true; + break; + } + if (done) break; + } + assert(done); + } + // We must now be within limits, otherwise LimitOrphans should have continued further). + // We don't check the contents of the orphanage until the end to make fuzz runs faster. + assert(real->TotalLatencyScore() <= real->MaxGlobalLatencyScore()); + assert(real->TotalOrphanUsage() <= real->MaxGlobalUsage()); } // diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index d100f8bbee3..905185a78ed 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -72,15 +72,6 @@ static bool EqualTxns(const std::set& set_txns, const std::vect return true; } -unsigned int CheckNumEvictions(node::TxOrphanage& orphanage) -{ - const auto original_total_count{orphanage.CountAnnouncements()}; - orphanage.LimitOrphans(); - assert(orphanage.TotalLatencyScore() <= orphanage.MaxGlobalLatencyScore()); - assert(orphanage.TotalOrphanUsage() <= orphanage.MaxGlobalUsage()); - return original_total_count - orphanage.CountAnnouncements(); -} - BOOST_AUTO_TEST_CASE(peer_dos_limits) { FastRandomContext det_rand{true}; @@ -106,30 +97,22 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_ann=*/1, /*reserved_peer_usage=*/TX_SIZE * 10); auto orphanage_low_mem = node::MakeTxOrphanage(/*max_global_ann=*/10, /*reserved_peer_usage=*/TX_SIZE); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_mem), 0); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_ann), 0); - // Add the first transaction orphanage_low_ann->AddTx(txns.at(0), peer); orphanage_low_mem->AddTx(txns.at(0), peer); // Add more. One of the limits is exceeded, so LimitOrphans evicts 1. orphanage_low_ann->AddTx(txns.at(1), peer); - BOOST_CHECK(orphanage_low_ann->TotalLatencyScore() > orphanage_low_ann->MaxGlobalLatencyScore()); - BOOST_CHECK(orphanage_low_ann->TotalOrphanUsage() <= orphanage_low_ann->MaxGlobalUsage()); - orphanage_low_mem->AddTx(txns.at(1), peer); - BOOST_CHECK(orphanage_low_mem->TotalLatencyScore() <= orphanage_low_mem->MaxGlobalLatencyScore()); - BOOST_CHECK(orphanage_low_mem->TotalOrphanUsage() > orphanage_low_mem->MaxGlobalUsage()); - - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_mem), 1); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_ann), 1); // The older transaction is evicted. BOOST_CHECK(!orphanage_low_ann->HaveTx(txns.at(0)->GetWitnessHash())); BOOST_CHECK(!orphanage_low_mem->HaveTx(txns.at(0)->GetWitnessHash())); BOOST_CHECK(orphanage_low_ann->HaveTx(txns.at(1)->GetWitnessHash())); BOOST_CHECK(orphanage_low_mem->HaveTx(txns.at(1)->GetWitnessHash())); + + orphanage_low_ann->SanityCheck(); + orphanage_low_mem->SanityCheck(); } // Single peer: latency score includes inputs @@ -138,8 +121,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) NodeId peer{10}; auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_ann=*/5, /*reserved_peer_usage=*/TX_SIZE * 1000); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_ann), 0); - // Add the first transaction orphanage_low_ann->AddTx(txns.at(0), peer); @@ -151,14 +132,11 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) auto ptx = MakeTransactionSpending(outpoints_45, det_rand); orphanage_low_ann->AddTx(ptx, peer); - BOOST_CHECK(orphanage_low_ann->TotalLatencyScore() > orphanage_low_ann->MaxGlobalLatencyScore()); - BOOST_CHECK(orphanage_low_ann->LatencyScoreFromPeer(peer) > orphanage_low_ann->MaxPeerLatencyScore()); - - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_ann), 1); - // The older transaction is evicted. BOOST_CHECK(!orphanage_low_ann->HaveTx(txns.at(0)->GetWitnessHash())); BOOST_CHECK(orphanage_low_ann->HaveTx(ptx->GetWitnessHash())); + + orphanage_low_ann->SanityCheck(); } // Single peer: eviction order is FIFO on non-reconsiderable, then reconsiderable orphans. @@ -191,15 +169,14 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Add 1 more orphan, causing the orphanage to be oversize. child1 is evicted. orphanage->AddTx(children.at(3), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(orphanage->HaveTx(children.at(0)->GetWitnessHash())); BOOST_CHECK(!orphanage->HaveTx(children.at(1)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(2)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(3)->GetWitnessHash())); + orphanage->SanityCheck(); // Add 1 more... child2 is evicted. orphanage->AddTx(children.at(4), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(orphanage->HaveTx(children.at(0)->GetWitnessHash())); BOOST_CHECK(!orphanage->HaveTx(children.at(2)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(3)->GetWitnessHash())); @@ -213,7 +190,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // child5 is evicted immediately because it is the only non-reconsiderable orphan. orphanage->AddTx(children.at(5), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(orphanage->HaveTx(children.at(0)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(3)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(4)->GetWitnessHash())); @@ -222,7 +198,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Transactions are marked non-reconsiderable again when returned through GetTxToReconsider BOOST_CHECK_EQUAL(orphanage->GetTxToReconsider(peer), children.at(0)); orphanage->AddTx(children.at(6), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(!orphanage->HaveTx(children.at(0)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(3)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(4)->GetWitnessHash())); @@ -232,6 +207,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // reconsideration earlier. BOOST_CHECK_EQUAL(orphanage->GetTxToReconsider(peer), children.at(3)); BOOST_CHECK_EQUAL(orphanage->GetTxToReconsider(peer), children.at(4)); + + orphanage->SanityCheck(); } // Multiple peers: when limit is exceeded, we choose the DoSiest peer and evict their oldest transaction. @@ -247,8 +224,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // No evictions happen before the global limit is reached. for (unsigned int i{0}; i < max_announcements; ++i) { orphanage->AddTx(txns.at(i), peer_dosy); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 0); } + orphanage->SanityCheck(); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer_dosy), max_announcements); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer1), 0); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer2), 0); @@ -260,7 +237,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) orphanage->AddTx(txns.at(max_announcements + i), peer1); // The announcement limit per peer has halved, but LimitOrphans does not evict beyond what is necessary to // bring the total announcements within its global limit. - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(orphanage->AnnouncementsFromPeer(peer_dosy) > orphanage->MaxPeerLatencyScore()); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer1), i + 1); @@ -276,9 +252,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) BOOST_CHECK(orphanage->HaveTxFromPeer(txns.at(i)->GetWitnessHash(), peer_dosy)); orphanage->AddTx(txns.at(i), peer2); - // Announcement limit is by entry, not by unique orphans - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); - // peer_dosy is still the only one getting evicted BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer_dosy), max_announcements - i - 1); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer1), num_from_peer1); @@ -291,15 +264,18 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // With 6 peers, each can add 10, and still only peer_dosy's orphans are evicted. const unsigned int max_per_peer{max_announcements / 6}; + const unsigned int num_announcements{orphanage->CountAnnouncements()}; for (NodeId peer{3}; peer < 6; ++peer) { for (unsigned int i{0}; i < max_per_peer; ++i) { + // Each addition causes 1 eviction. orphanage->AddTx(txns.at(peer * max_per_peer + i), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); + BOOST_CHECK_EQUAL(orphanage->CountAnnouncements(), num_announcements); } } for (NodeId peer{0}; peer < 6; ++peer) { BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer), max_per_peer); } + orphanage->SanityCheck(); } // Limits change as more peers are added. @@ -355,6 +331,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) BOOST_CHECK_EQUAL(orphanage->ReservedPeerUsage(), node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER); BOOST_CHECK_EQUAL(orphanage->MaxGlobalUsage(), node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER); BOOST_CHECK_EQUAL(orphanage->MaxPeerLatencyScore(), node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE); + + orphanage->SanityCheck(); } // Test eviction of multiple transactions at a time @@ -378,17 +356,17 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) } BOOST_CHECK(orphanage->TotalLatencyScore() <= orphanage->MaxGlobalLatencyScore()); BOOST_CHECK(orphanage->TotalOrphanUsage() <= orphanage->MaxGlobalUsage()); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 0); // Add the large transaction. This should cause evictions of all the previous 10 transactions from that peer. orphanage->AddTx(ptx_large, peer_large); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 10); // peer_normal should still have 10 transactions, and peer_large should have 1. BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer_normal), 10); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer_large), 1); BOOST_CHECK(orphanage->HaveTxFromPeer(ptx_large->GetWitnessHash(), peer_large)); BOOST_CHECK_EQUAL(orphanage->CountAnnouncements(), 11); + + orphanage->SanityCheck(); } // Test that latency score includes number of inputs. @@ -432,6 +410,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Peer 1 sent 5 of the 10 transactions with many inputs BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(1), 5); BOOST_CHECK_EQUAL(orphanage->LatencyScoreFromPeer(1), 5 + 5 * 5); + + orphanage->SanityCheck(); } } BOOST_AUTO_TEST_CASE(DoS_mapOrphans) From ed24e016969098c486f413f4f57dcffe35241785 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 26 Jun 2025 14:56:16 -0400 Subject: [PATCH 03/12] [optimization] Maintain at most 1 reconsiderable announcement per wtxid This introduces an invariant that TxOrphanageImpl never holds more than one announcement with m_reconsider=true for a given wtxid. This avoids duplicate work, both in the caller might otherwise reconsider the same transaction multiple times before it is ready, and internally in AddChildrenToWorkSet, which might otherwise iterate over all announcements multiple times. --- src/node/txorphanage.cpp | 35 +++++++++++++++++++++++++++++++---- src/test/fuzz/txorphan.cpp | 19 +++++++------------ 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index 9b95b52b477..33d2da32427 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -119,6 +119,9 @@ class TxOrphanageImpl final : public TxOrphanage { * a transaction that can be reconsidered and to remove entries that conflict with a block.*/ std::unordered_map, SaltedOutpointHasher> m_outpoint_to_orphan_it; + /** Set of Wtxids for which (exactly) one announcement with m_reconsider=true exists. */ + std::set m_reconsiderable_wtxids; + struct PeerDoSInfo { TxOrphanage::Usage m_total_usage{0}; TxOrphanage::Count m_count_announcements{0}; @@ -261,6 +264,11 @@ void TxOrphanageImpl::Erase(Iter it) } } } + + // If this was the (unique) reconsiderable announcement for its wtxid, then the wtxid won't + // have any reconsiderable announcements left after erasing. + if (it->m_reconsider) m_reconsiderable_wtxids.erase(it->m_tx->GetWitnessHash()); + m_orphans.get().erase(it); } @@ -521,6 +529,9 @@ std::vector> TxOrphanageImpl::AddChildrenToWorkSet(cons const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i)); if (it_by_prev != m_outpoint_to_orphan_it.end()) { for (const auto& wtxid : it_by_prev->second) { + // If a reconsiderable announcement for this wtxid already exists, skip it. + if (m_reconsiderable_wtxids.contains(wtxid)) continue; + // Belt and suspenders, each entry in m_outpoint_to_orphan_it should always have at least 1 announcement. auto it = index_by_wtxid.lower_bound(ByWtxidView{wtxid, MIN_PEER}); if (!Assume(it != index_by_wtxid.end())) continue; @@ -537,10 +548,10 @@ std::vector> TxOrphanageImpl::AddChildrenToWorkSet(cons // Mark this orphan as ready to be reconsidered. static constexpr auto mark_reconsidered_modifier = [](auto& ann) { ann.m_reconsider = true; }; - if (!it->m_reconsider) { - index_by_wtxid.modify(it, mark_reconsidered_modifier); - ret.emplace_back(wtxid, it->m_announcer); - } + Assume(!it->m_reconsider); + index_by_wtxid.modify(it, mark_reconsidered_modifier); + ret.emplace_back(wtxid, it->m_announcer); + m_reconsiderable_wtxids.insert(wtxid); LogDebug(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n", it->m_tx->GetHash().ToString(), it->m_tx->GetWitnessHash().ToString(), it->m_announcer); @@ -578,6 +589,9 @@ CTransactionRef TxOrphanageImpl::GetTxToReconsider(NodeId peer) // reconsidered again until there is a new reason to do so. static constexpr auto mark_reconsidered_modifier = [](auto& ann) { ann.m_reconsider = false; }; m_orphans.get().modify(it, mark_reconsidered_modifier); + // As there is exactly one m_reconsider announcement per reconsiderable wtxids, flipping + // the m_reconsider flag means the wtxid is no longer reconsiderable. + m_reconsiderable_wtxids.erase(it->m_tx->GetWitnessHash()); return it->m_tx; } return nullptr; @@ -680,6 +694,7 @@ void TxOrphanageImpl::SanityCheck() const std::unordered_map reconstructed_peer_info; std::map> unique_wtxids_to_scores; std::set all_outpoints; + std::set reconstructed_reconsiderable_wtxids; for (auto it = m_orphans.begin(); it != m_orphans.end(); ++it) { for (const auto& input : it->m_tx->vin) { @@ -691,9 +706,21 @@ void TxOrphanageImpl::SanityCheck() const peer_info.m_total_usage += it->GetMemUsage(); peer_info.m_count_announcements += 1; peer_info.m_total_latency_score += it->GetLatencyScore(); + + if (it->m_reconsider) { + auto [_, added] = reconstructed_reconsiderable_wtxids.insert(it->m_tx->GetWitnessHash()); + // Check that there is only ever 1 announcement per wtxid with m_reconsider set. + assert(added); + } } assert(reconstructed_peer_info.size() == m_peer_orphanage_info.size()); + // Recalculated per-peer stats are identical to m_peer_orphanage_info + assert(reconstructed_peer_info == m_peer_orphanage_info); + + // Recalculated set of reconsiderable wtxids must match. + assert(m_reconsiderable_wtxids == reconstructed_reconsiderable_wtxids); + // All outpoints exist in m_outpoint_to_orphan_it, all keys in m_outpoint_to_orphan_it correspond to some // orphan, and all wtxids referenced in m_outpoint_to_orphan_it are also in m_orphans. // This ensures m_outpoint_to_orphan_it is cleaned up. diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index a9bd762e645..82030390f05 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -626,11 +626,11 @@ FUZZ_TARGET(txorphanage_sim) auto tx = read_tx_fn(); FastRandomContext rand_ctx(rng.rand256()); auto added = real->AddChildrenToWorkSet(*txn[tx], rand_ctx); - /** Map of all child wtxids, with value whether they already have a reconsiderable - announcement from some peer. */ - std::map child_wtxids; + /** Set of not-already-reconsiderable child wtxids. */ + std::set child_wtxids; for (unsigned child_tx = 0; child_tx < NUM_TX; ++child_tx) { if (!have_tx_fn(child_tx)) continue; + if (have_reconsiderable_fn(child_tx)) continue; bool child_of = false; for (auto& txin : txn[child_tx]->vin) { if (txin.prevout.hash == txn[tx]->GetHash()) { @@ -639,11 +639,11 @@ FUZZ_TARGET(txorphanage_sim) } } if (child_of) { - child_wtxids[txn[child_tx]->GetWitnessHash()] = have_reconsiderable_fn(child_tx); + child_wtxids.insert(txn[child_tx]->GetWitnessHash()); } } for (auto& [wtxid, peer] : added) { - // Wtxid must be a child of tx. + // Wtxid must be a child of tx that is not yet reconsiderable. auto child_wtxid_it = child_wtxids.find(wtxid); assert(child_wtxid_it != child_wtxids.end()); // Announcement must exist. @@ -653,18 +653,13 @@ FUZZ_TARGET(txorphanage_sim) assert(sim_ann_it->reconsider == false); // Make reconsiderable. sim_ann_it->reconsider = true; - } - for (auto& [wtxid, peer] : added) { - // Remove from child_wtxids map, so we can check that only already-reconsiderable - // ones are missing from the result. + // Remove from child_wtxids map, to disallow it being reported a second time in added. child_wtxids.erase(wtxid); } // Verify that AddChildrenToWorkSet does not select announcements that were already reconsiderable: // Check all child wtxids which did not occur at least once in the result were already reconsiderable // due to a previous AddChildrenToWorkSet. - for (auto& [wtxid, already_reconsider] : child_wtxids) { - assert(already_reconsider); - } + assert(child_wtxids.empty()); break; } else if (command-- == 0) { // GetTxToReconsider. From cc50f2f0df6e6e2cc9b9aeb3c3c8e1c78fa5be1d Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 5 Jun 2025 14:13:28 -0400 Subject: [PATCH 04/12] [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans --- src/node/txdownloadman_impl.cpp | 2 +- src/node/txorphanage.cpp | 1 - src/node/txorphanage.h | 3 --- src/test/orphanage_tests.cpp | 28 ++++++++++++++-------------- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 99fa036bda3..38b949c654a 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -572,7 +572,7 @@ void TxDownloadManagerImpl::CheckIsEmpty(NodeId nodeid) void TxDownloadManagerImpl::CheckIsEmpty() { assert(m_orphanage->TotalOrphanUsage() == 0); - assert(m_orphanage->Size() == 0); + assert(m_orphanage->CountUniqueOrphans() == 0); assert(m_txrequest.Size() == 0); assert(m_num_wtxid_peers == 0); } diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index 33d2da32427..9d386b9dc0b 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -229,7 +229,6 @@ public: std::vector> AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng) override; bool HaveTxToReconsider(NodeId peer) override; std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const override; - size_t Size() const override { return m_unique_orphans; } std::vector GetOrphanTransactions() const override; TxOrphanage::Usage TotalOrphanUsage() const override; void SanityCheck() const override; diff --git a/src/node/txorphanage.h b/src/node/txorphanage.h index 584fb390d34..24fa20bfe78 100644 --- a/src/node/txorphanage.h +++ b/src/node/txorphanage.h @@ -98,9 +98,6 @@ public: * recent to least recent. */ virtual std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const = 0; - /** Return how many entries exist in the orphange */ - virtual size_t Size() const = 0; - /** Get all orphan transactions */ virtual std::vector GetOrphanTransactions() const = 0; diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 905185a78ed..2de8693a5df 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -498,11 +498,11 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) BOOST_CHECK(!orphanage->AddTx(MakeTransactionRef(tx), i)); } - size_t expected_num_orphans = orphanage->Size(); + size_t expected_num_orphans = orphanage->CountUniqueOrphans(); // Non-existent peer; nothing should be deleted orphanage->EraseForPeer(/*peer=*/-1); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_num_orphans); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_num_orphans); // Each of first three peers stored // two transactions each. @@ -510,7 +510,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) { orphanage->EraseForPeer(i); expected_num_orphans -= 2; - BOOST_CHECK(orphanage->Size() == expected_num_orphans); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_num_orphans); } } @@ -732,7 +732,7 @@ BOOST_AUTO_TEST_CASE(process_block) BOOST_CHECK(!orphanage->HaveTx(expected_removed_wtxid)); } // Only remaining tx is control_tx - BOOST_CHECK_EQUAL(orphanage->Size(), 1); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), 1); BOOST_CHECK(orphanage->HaveTx(control_tx->GetWitnessHash())); } @@ -753,11 +753,11 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) BOOST_CHECK(orphanage->AddTx(ptx, node0)); BOOST_CHECK(orphanage->HaveTx(wtxid)); expected_total_count += 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // Adding again should do nothing. BOOST_CHECK(!orphanage->AddTx(ptx, node0)); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // We can add another tx with the same txid but different witness. auto ptx_mutated{MakeMutation(ptx)}; @@ -769,7 +769,7 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) // Adding a new announcer should not change overall accounting. BOOST_CHECK(orphanage->AddAnnouncer(ptx->GetWitnessHash(), node2)); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // If we already have this announcer, AddAnnouncer returns false. BOOST_CHECK(orphanage->HaveTxFromPeer(ptx->GetWitnessHash(), node2)); @@ -777,7 +777,7 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) // Same with using AddTx for an existing tx, which is equivalent to using AddAnnouncer BOOST_CHECK(!orphanage->AddTx(ptx, node1)); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // if EraseForPeer is called for an orphan with multiple announcers, the orphanage should only // erase that peer from the announcers set. @@ -787,15 +787,15 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) // node0 is the only one that announced ptx_mutated BOOST_CHECK(!orphanage->HaveTx(ptx_mutated->GetWitnessHash())); expected_total_count -= 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // EraseForPeer should delete the orphan if it's the only announcer left. orphanage->EraseForPeer(node1); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); BOOST_CHECK(orphanage->HaveTx(ptx->GetWitnessHash())); orphanage->EraseForPeer(node2); expected_total_count -= 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); BOOST_CHECK(!orphanage->HaveTx(ptx->GetWitnessHash())); } @@ -809,13 +809,13 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) expected_total_count += 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); orphanage->EraseForBlock(block); expected_total_count -= 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); } } BOOST_AUTO_TEST_CASE(peer_worksets) @@ -863,7 +863,7 @@ BOOST_AUTO_TEST_CASE(peer_worksets) // Delete this tx, clearing the orphanage. BOOST_CHECK_EQUAL(orphanage->EraseTx(orphan_wtxid), 1); - BOOST_CHECK_EQUAL(orphanage->Size(), 0); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), 0); for (NodeId node = node0; node <= node2; ++node) { BOOST_CHECK_EQUAL(orphanage->GetTxToReconsider(node), nullptr); BOOST_CHECK(!orphanage->HaveTxFromPeer(orphan_wtxid, node)); From 8a58d0e87d70580ae47da228e2f88cd53c40c675 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 7 Jul 2025 13:12:01 -0400 Subject: [PATCH 05/12] scripted-diff: rename OrphanTxBase to OrphanInfo -BEGIN VERIFY SCRIPT- sed -i 's/OrphanTxBase/OrphanInfo/g' $(git grep -l 'OrphanTxBase') -END VERIFY SCRIPT- --- src/net_processing.cpp | 4 ++-- src/net_processing.h | 2 +- src/node/txdownloadman.h | 2 +- src/node/txdownloadman_impl.cpp | 4 ++-- src/node/txdownloadman_impl.h | 2 +- src/node/txorphanage.cpp | 8 ++++---- src/node/txorphanage.h | 6 +++--- src/rpc/mempool.cpp | 4 ++-- 8 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 10fbae5e7ed..65a70da089b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -533,7 +533,7 @@ public: std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - std::vector GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); + std::vector GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void RelayTransaction(const Txid& txid, const Wtxid& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); @@ -1754,7 +1754,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c return true; } -std::vector PeerManagerImpl::GetOrphanTransactions() +std::vector PeerManagerImpl::GetOrphanTransactions() { LOCK(m_tx_download_mutex); return m_txdownloadman.GetOrphanTransactions(); diff --git a/src/net_processing.h b/src/net_processing.h index 9cf79512172..8c140d98ad6 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -109,7 +109,7 @@ public: /** Get statistics from node state */ virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0; - virtual std::vector GetOrphanTransactions() = 0; + virtual std::vector GetOrphanTransactions() = 0; /** Get peer manager info. */ virtual PeerManagerInfo GetInfo() const = 0; diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 3ebf66324a8..90dc0e343af 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -170,7 +170,7 @@ public: void CheckIsEmpty(NodeId nodeid) const; /** Wrapper for TxOrphanage::GetOrphanTransactions */ - std::vector GetOrphanTransactions() const; + std::vector GetOrphanTransactions() const; }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 38b949c654a..0105fe3d609 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -83,7 +83,7 @@ void TxDownloadManager::CheckIsEmpty(NodeId nodeid) const { m_impl->CheckIsEmpty(nodeid); } -std::vector TxDownloadManager::GetOrphanTransactions() const +std::vector TxDownloadManager::GetOrphanTransactions() const { return m_impl->GetOrphanTransactions(); } @@ -576,7 +576,7 @@ void TxDownloadManagerImpl::CheckIsEmpty() assert(m_txrequest.Size() == 0); assert(m_num_wtxid_peers == 0); } -std::vector TxDownloadManagerImpl::GetOrphanTransactions() const +std::vector TxDownloadManagerImpl::GetOrphanTransactions() const { return m_orphanage->GetOrphanTransactions(); } diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index b10c71d5d24..8af0cb227f5 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -188,7 +188,7 @@ public: void CheckIsEmpty(); void CheckIsEmpty(NodeId nodeid); - std::vector GetOrphanTransactions() const; + std::vector GetOrphanTransactions() const; protected: /** Helper for getting deduplicated vector of Txids in vin. */ diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index 9d386b9dc0b..3e07652abcc 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -229,7 +229,7 @@ public: std::vector> AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng) override; bool HaveTxToReconsider(NodeId peer) override; std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const override; - std::vector GetOrphanTransactions() const override; + std::vector GetOrphanTransactions() const override; TxOrphanage::Usage TotalOrphanUsage() const override; void SanityCheck() const override; }; @@ -665,9 +665,9 @@ std::vector TxOrphanageImpl::GetChildrenFromSamePeer(const CTra return children_found; } -std::vector TxOrphanageImpl::GetOrphanTransactions() const +std::vector TxOrphanageImpl::GetOrphanTransactions() const { - std::vector result; + std::vector result; result.reserve(m_unique_orphans); auto& index_by_wtxid = m_orphans.get(); @@ -675,7 +675,7 @@ std::vector TxOrphanageImpl::GetOrphanTransactions() std::set this_orphan_announcers; while (it != index_by_wtxid.end()) { this_orphan_announcers.insert(it->m_announcer); - // If this is the last entry, or the next entry has a different wtxid, build a OrphanTxBase. + // If this is the last entry, or the next entry has a different wtxid, build a OrphanInfo. if (std::next(it) == index_by_wtxid.end() || std::next(it)->m_tx->GetWitnessHash() != it->m_tx->GetWitnessHash()) { result.emplace_back(it->m_tx, std::move(this_orphan_announcers)); this_orphan_announcers.clear(); diff --git a/src/node/txorphanage.h b/src/node/txorphanage.h index 24fa20bfe78..146e6730e89 100644 --- a/src/node/txorphanage.h +++ b/src/node/txorphanage.h @@ -41,13 +41,13 @@ public: using Count = unsigned int; /** Allows providing orphan information externally */ - struct OrphanTxBase { + struct OrphanInfo { CTransactionRef tx; /** Peers added with AddTx or AddAnnouncer. */ std::set announcers; // Constructor with moved announcers - OrphanTxBase(CTransactionRef tx, std::set&& announcers) : + OrphanInfo(CTransactionRef tx, std::set&& announcers) : tx(std::move(tx)), announcers(std::move(announcers)) {} @@ -99,7 +99,7 @@ public: virtual std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const = 0; /** Get all orphan transactions */ - virtual std::vector GetOrphanTransactions() const = 0; + virtual std::vector GetOrphanTransactions() const = 0; /** Get the total usage (weight) of all orphans. If an orphan has multiple announcers, its usage is * only counted once within this total. */ diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 55104e4c766..d0c5ec65dbf 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -839,7 +839,7 @@ static std::vector OrphanDescription() }; } -static UniValue OrphanToJSON(const node::TxOrphanage::OrphanTxBase& orphan) +static UniValue OrphanToJSON(const node::TxOrphanage::OrphanInfo& orphan) { UniValue o(UniValue::VOBJ); o.pushKV("txid", orphan.tx->GetHash().ToString()); @@ -895,7 +895,7 @@ static RPCHelpMan getorphantxs() { const NodeContext& node = EnsureAnyNodeContext(request.context); PeerManager& peerman = EnsurePeerman(node); - std::vector orphanage = peerman.GetOrphanTransactions(); + std::vector orphanage = peerman.GetOrphanTransactions(); int verbosity{ParseVerbosity(request.params[0], /*default_verbosity=*/0, /*allow_bool*/false)}; From edb97bb3f151600f00c94a2732d2595446011295 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 15 Jul 2025 16:24:20 -0400 Subject: [PATCH 06/12] [logging] add logs for inner loop of LimitOrphans --- src/node/txorphanage.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index 3e07652abcc..e15de3a3365 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -494,17 +494,21 @@ void TxOrphanageImpl::LimitOrphans() // The number of inner loop iterations is bounded by the total number of announcements. const auto& dos_threshold = heap_peer_dos.empty() ? FeeFrac{1, 1} : heap_peer_dos.front().second; auto it_ann = m_orphans.get().lower_bound(ByPeerView{worst_peer, false, 0}); + unsigned int num_erased_this_round{0}; + unsigned int starting_num_ann{it_worst_peer->second.m_count_announcements}; while (NeedsTrim()) { if (!Assume(it_ann->m_announcer == worst_peer)) break; if (!Assume(it_ann != m_orphans.get().end())) break; Erase(it_ann++); num_erased += 1; + num_erased_this_round += 1; // If we erased the last orphan from this peer, it_worst_peer will be invalidated. it_worst_peer = m_peer_orphanage_info.find(worst_peer); if (it_worst_peer == m_peer_orphanage_info.end() || it_worst_peer->second.GetDosScore(max_ann, max_mem) <= dos_threshold) break; } + LogDebug(BCLog::TXPACKAGES, "peer=%d orphanage overflow, removed %u of %u announcements\n", worst_peer, num_erased_this_round, starting_num_ann); if (!NeedsTrim()) break; From cfd71c67043a2a46950fd3f055afbe4a93922f75 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 16 Jul 2025 13:34:18 -0400 Subject: [PATCH 07/12] scripted-diff: rename TxOrphanage outpoints index -BEGIN VERIFY SCRIPT- sed -i 's/m_outpoint_to_orphan_it/m_outpoint_to_orphan_wtxids/g' src/node/txorphanage.cpp -END VERIFY SCRIPT- --- src/node/txorphanage.cpp | 44 ++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index e15de3a3365..cda23b250d5 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -50,9 +50,9 @@ class TxOrphanageImpl final : public TxOrphanage { { } /** Get an approximation for "memory usage". The total memory is a function of the memory used to store the - * transaction itself, each entry in m_orphans, and each entry in m_outpoint_to_orphan_it. We use weight because + * transaction itself, each entry in m_orphans, and each entry in m_outpoint_to_orphan_wtxids. We use weight because * it is often higher than the actual memory usage of the tranaction. This metric conveniently encompasses - * m_outpoint_to_orphan_it usage since input data does not get the witness discount, and makes it easier to + * m_outpoint_to_orphan_wtxids usage since input data does not get the witness discount, and makes it easier to * reason about each peer's limits using well-understood transaction attributes. */ TxOrphanage::Usage GetMemUsage() const { return GetTransactionWeight(*m_tx); @@ -60,7 +60,7 @@ class TxOrphanageImpl final : public TxOrphanage { /** Get an approximation of how much this transaction contributes to latency in EraseForBlock and EraseForPeer. * The computation time is a function of the number of entries in m_orphans (thus 1 per announcement) and the - * number of entries in m_outpoint_to_orphan_it (thus an additional 1 for every 10 inputs). Transactions with a + * number of entries in m_outpoint_to_orphan_wtxids (thus an additional 1 for every 10 inputs). Transactions with a * small number of inputs (9 or fewer) are counted as 1 to make it easier to reason about each peer's limits in * terms of "normal" transactions. */ TxOrphanage::Count GetLatencyScore() const { @@ -117,7 +117,7 @@ class TxOrphanageImpl final : public TxOrphanage { /** Index from the parents' outputs to wtxids that exist in m_orphans. Used to find children of * a transaction that can be reconsidered and to remove entries that conflict with a block.*/ - std::unordered_map, SaltedOutpointHasher> m_outpoint_to_orphan_it; + std::unordered_map, SaltedOutpointHasher> m_outpoint_to_orphan_wtxids; /** Set of Wtxids for which (exactly) one announcement with m_reconsider=true exists. */ std::set m_reconsiderable_wtxids; @@ -250,15 +250,15 @@ void TxOrphanageImpl::Erase(Iter it) m_unique_rounded_input_scores -= it->GetLatencyScore() - 1; m_unique_orphan_usage -= it->GetMemUsage(); - // Remove references in m_outpoint_to_orphan_it + // Remove references in m_outpoint_to_orphan_wtxids const auto& wtxid{it->m_tx->GetWitnessHash()}; for (const auto& input : it->m_tx->vin) { - auto it_prev = m_outpoint_to_orphan_it.find(input.prevout); - if (it_prev != m_outpoint_to_orphan_it.end()) { + auto it_prev = m_outpoint_to_orphan_wtxids.find(input.prevout); + if (it_prev != m_outpoint_to_orphan_wtxids.end()) { it_prev->second.erase(wtxid); // Clean up keys if they point to an empty set. if (it_prev->second.empty()) { - m_outpoint_to_orphan_it.erase(it_prev); + m_outpoint_to_orphan_wtxids.erase(it_prev); } } } @@ -326,10 +326,10 @@ bool TxOrphanageImpl::AddTx(const CTransactionRef& tx, NodeId peer) auto& peer_info = m_peer_orphanage_info.try_emplace(peer).first->second; peer_info.Add(*iter); - // Add links in m_outpoint_to_orphan_it + // Add links in m_outpoint_to_orphan_wtxids if (brand_new) { for (const auto& input : tx->vin) { - auto& wtxids_for_prevout = m_outpoint_to_orphan_it.try_emplace(input.prevout).first->second; + auto& wtxids_for_prevout = m_outpoint_to_orphan_wtxids.try_emplace(input.prevout).first->second; wtxids_for_prevout.emplace(wtxid); } @@ -338,7 +338,7 @@ bool TxOrphanageImpl::AddTx(const CTransactionRef& tx, NodeId peer) m_unique_rounded_input_scores += iter->GetLatencyScore() - 1; LogDebug(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s), weight: %u (mapsz %u outsz %u)\n", - txid.ToString(), wtxid.ToString(), sz, m_orphans.size(), m_outpoint_to_orphan_it.size()); + txid.ToString(), wtxid.ToString(), sz, m_orphans.size(), m_outpoint_to_orphan_wtxids.size()); Assume(IsUnique(iter)); } else { LogDebug(BCLog::TXPACKAGES, "added peer=%d as announcer of orphan tx %s (wtxid=%s)\n", @@ -421,7 +421,7 @@ void TxOrphanageImpl::EraseForPeer(NodeId peer) unsigned int num_ann{0}; while (it != index_by_peer.end() && it->m_announcer == peer) { - // Delete item, cleaning up m_outpoint_to_orphan_it iff this entry is unique by wtxid. + // Delete item, cleaning up m_outpoint_to_orphan_wtxids iff this entry is unique by wtxid. Erase(it++); num_ann += 1; } @@ -529,13 +529,13 @@ std::vector> TxOrphanageImpl::AddChildrenToWorkSet(cons std::vector> ret; auto& index_by_wtxid = m_orphans.get(); for (unsigned int i = 0; i < tx.vout.size(); i++) { - const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i)); - if (it_by_prev != m_outpoint_to_orphan_it.end()) { + const auto it_by_prev = m_outpoint_to_orphan_wtxids.find(COutPoint(tx.GetHash(), i)); + if (it_by_prev != m_outpoint_to_orphan_wtxids.end()) { for (const auto& wtxid : it_by_prev->second) { // If a reconsiderable announcement for this wtxid already exists, skip it. if (m_reconsiderable_wtxids.contains(wtxid)) continue; - // Belt and suspenders, each entry in m_outpoint_to_orphan_it should always have at least 1 announcement. + // Belt and suspenders, each entry in m_outpoint_to_orphan_wtxids should always have at least 1 announcement. auto it = index_by_wtxid.lower_bound(ByWtxidView{wtxid, MIN_PEER}); if (!Assume(it != index_by_wtxid.end())) continue; @@ -615,8 +615,8 @@ void TxOrphanageImpl::EraseForBlock(const CBlock& block) // Which orphan pool entries must we evict? for (const auto& input : block_tx.vin) { - auto it_prev = m_outpoint_to_orphan_it.find(input.prevout); - if (it_prev != m_outpoint_to_orphan_it.end()) { + auto it_prev = m_outpoint_to_orphan_wtxids.find(input.prevout); + if (it_prev != m_outpoint_to_orphan_wtxids.end()) { // Copy all wtxids to wtxids_to_erase. std::copy(it_prev->second.cbegin(), it_prev->second.cend(), std::inserter(wtxids_to_erase, wtxids_to_erase.end())); } @@ -724,11 +724,11 @@ void TxOrphanageImpl::SanityCheck() const // Recalculated set of reconsiderable wtxids must match. assert(m_reconsiderable_wtxids == reconstructed_reconsiderable_wtxids); - // All outpoints exist in m_outpoint_to_orphan_it, all keys in m_outpoint_to_orphan_it correspond to some - // orphan, and all wtxids referenced in m_outpoint_to_orphan_it are also in m_orphans. - // This ensures m_outpoint_to_orphan_it is cleaned up. - assert(all_outpoints.size() == m_outpoint_to_orphan_it.size()); - for (const auto& [outpoint, wtxid_set] : m_outpoint_to_orphan_it) { + // All outpoints exist in m_outpoint_to_orphan_wtxids, all keys in m_outpoint_to_orphan_wtxids correspond to some + // orphan, and all wtxids referenced in m_outpoint_to_orphan_wtxids are also in m_orphans. + // This ensures m_outpoint_to_orphan_wtxids is cleaned up. + assert(all_outpoints.size() == m_outpoint_to_orphan_wtxids.size()); + for (const auto& [outpoint, wtxid_set] : m_outpoint_to_orphan_wtxids) { assert(all_outpoints.contains(outpoint)); for (const auto& wtxid : wtxid_set) { assert(unique_wtxids_to_scores.contains(wtxid)); From b10c55b298d4d2b7dddfecdbeb0edc624b8e6eb2 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 17 Jul 2025 14:52:35 -0400 Subject: [PATCH 08/12] fix up TxOrphanage lower_bound sanity checks Co-authored-by: Sebastian Falbesoner --- src/node/txorphanage.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index cda23b250d5..931bbdc2495 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -497,8 +497,8 @@ void TxOrphanageImpl::LimitOrphans() unsigned int num_erased_this_round{0}; unsigned int starting_num_ann{it_worst_peer->second.m_count_announcements}; while (NeedsTrim()) { - if (!Assume(it_ann->m_announcer == worst_peer)) break; if (!Assume(it_ann != m_orphans.get().end())) break; + if (!Assume(it_ann->m_announcer == worst_peer)) break; Erase(it_ann++); num_erased += 1; @@ -537,7 +537,7 @@ std::vector> TxOrphanageImpl::AddChildrenToWorkSet(cons // Belt and suspenders, each entry in m_outpoint_to_orphan_wtxids should always have at least 1 announcement. auto it = index_by_wtxid.lower_bound(ByWtxidView{wtxid, MIN_PEER}); - if (!Assume(it != index_by_wtxid.end())) continue; + if (!Assume(it != index_by_wtxid.end() && it->m_tx->GetWitnessHash() == wtxid)) continue; // Select a random peer to assign orphan processing, reducing wasted work if the orphan is still missing // inputs. However, we don't want to create an issue in which the assigned peer can purposefully stop us From 1384dbaf6d0bfcdb05f97e1e3cb3d5e498bee505 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 23 Jul 2025 13:49:22 -0400 Subject: [PATCH 09/12] [config] emit warning for -maxorphantx, but allow it to be set --- src/init.cpp | 7 +++++++ test/functional/p2p_orphan_handling.py | 11 +++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 63244c802ea..ac4c3f73f1c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -490,6 +490,8 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + // TODO: remove in v31.0 + argsman.AddArg("-maxorphantx=", strprintf("(Removed option, see release notes)"), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-mempoolexpiry=", strprintf("Do not keep transactions in the mempool longer than hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY_HOURS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-minimumchainwork=", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet3: %s, testnet4: %s, signet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnet4ChainParams->GetConsensus().nMinimumChainWork.GetHex(), signetChainParams->GetConsensus().nMinimumChainWork.GetHex()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-par=", strprintf("Set the number of script verification threads (0 = auto, up to %d, <0 = leave that many cores free, default: %d)", @@ -893,6 +895,11 @@ bool AppInitParameterInteraction(const ArgsManager& args) InitWarning(_("Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated. They will be removed in a future version.")); } + // We no longer limit the orphanage based on number of transactions but keep the option to warn users who still have it in their config. + if (args.IsArgSet("-maxorphantx")) { + InitWarning(_("Option '-maxorphantx' is set but no longer has any effect (see release notes). Please remove it from your configuration.")); + } + // Error if network-specific options (-addnode, -connect, etc) are // specified in default section of config file, but not overridden // on the command line or in this chain's section of the config file. diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index ddaa8ac0ac9..f2eed48603c 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -830,6 +830,16 @@ class OrphanHandlingTest(BitcoinTestFramework): assert orphan["txid"] in final_mempool assert tx_replacer_C["txid"] in final_mempool + @cleanup + def test_maxorphantx_option(self): + # This test should be removed when -maxorphantx is removed. + self.log.info("Test that setting the -maxorphantx option does not error") + warning = "Warning: Option '-maxorphantx' is set but no longer has any effect (see release notes). Please remove it from your configuration." + self.restart_node(0, extra_args=["-maxorphantx=5"]) + assert_equal(self.nodes[0].getorphantxs(), []) + self.stop_node(0, expected_stderr=warning) + self.restart_node(0) + def run_test(self): self.nodes[0].setmocktime(int(time.time())) self.wallet_nonsegwit = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK) @@ -850,6 +860,7 @@ class OrphanHandlingTest(BitcoinTestFramework): self.test_announcers_before_and_after() self.test_parents_change() self.test_maximal_package_protected() + self.test_maxorphantx_option() if __name__ == '__main__': From 3b924489238220710326e9031c7aaa0d606c9064 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 23 Jul 2025 13:51:29 -0400 Subject: [PATCH 10/12] [doc] comment fixups for orphanage changes --- src/node/txorphanage.cpp | 22 ++++++++++------------ src/node/txorphanage.h | 4 ++-- src/test/fuzz/txorphan.cpp | 4 ++-- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index 931bbdc2495..6dbcbc32371 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -51,7 +51,7 @@ class TxOrphanageImpl final : public TxOrphanage { /** Get an approximation for "memory usage". The total memory is a function of the memory used to store the * transaction itself, each entry in m_orphans, and each entry in m_outpoint_to_orphan_wtxids. We use weight because - * it is often higher than the actual memory usage of the tranaction. This metric conveniently encompasses + * it is often higher than the actual memory usage of the transaction. This metric conveniently encompasses * m_outpoint_to_orphan_wtxids usage since input data does not get the witness discount, and makes it easier to * reason about each peer's limits using well-understood transaction attributes. */ TxOrphanage::Usage GetMemUsage() const { @@ -158,13 +158,13 @@ class TxOrphanageImpl final : public TxOrphanage { * do not trim unless the orphanage exceeds global limits, but it means that this peer will * be selected for trimming sooner. If the global latency score or global memory usage * limits are exceeded, it must be that there is a peer whose DoS score > 1. */ - FeeFrac GetDosScore(TxOrphanage::Count max_peer_latency_score, TxOrphanage::Usage max_peer_bytes) const + FeeFrac GetDosScore(TxOrphanage::Count max_peer_latency_score, TxOrphanage::Usage max_peer_memory) const { assert(max_peer_latency_score > 0); - assert(max_peer_bytes > 0); - const FeeFrac cpu_score(m_total_latency_score, max_peer_latency_score); - const FeeFrac mem_score(m_total_usage, max_peer_bytes); - return std::max(cpu_score, mem_score); + assert(max_peer_memory > 0); + const FeeFrac latency_score(m_total_latency_score, max_peer_latency_score); + const FeeFrac mem_score(m_total_usage, max_peer_memory); + return std::max(latency_score, mem_score); } }; /** Store per-peer statistics. Used to determine each peer's DoS score. The size of this map is used to determine the @@ -205,7 +205,7 @@ public: TxOrphanage::Count TotalLatencyScore() const override; TxOrphanage::Usage ReservedPeerUsage() const override; - /** Maximum allowed (deduplicated) latency score for all tranactions (see Announcement::GetLatencyScore()). Dynamic + /** Maximum allowed (deduplicated) latency score for all transactions (see Announcement::GetLatencyScore()). Dynamic * based on number of peers. Each peer has an equal amount, but the global maximum latency score stays constant. The * number of peers times MaxPeerLatencyScore() (rounded) adds up to MaxGlobalLatencyScore(). As long as every peer's * m_total_latency_score / MaxPeerLatencyScore() < 1, MaxGlobalLatencyScore() is not exceeded. */ @@ -469,11 +469,11 @@ void TxOrphanageImpl::LimitOrphans() std::make_heap(heap_peer_dos.begin(), heap_peer_dos.end(), compare_score); unsigned int num_erased{0}; - // This outer loop finds the peer with the highest DoS score, which is a fraction of {usage, announcements} used + // This outer loop finds the peer with the highest DoS score, which is a fraction of memory and latency scores // over the respective allowances. We continue until the orphanage is within global limits. That means some peers // might still have a DoS score > 1 at the end. - // Note: if ratios are the same, FeeFrac tiebreaks by denominator. In practice, since the CPU denominator (number of - // announcements) is always lower, this means that a peer with only high number of announcements will be targeted + // Note: if ratios are the same, FeeFrac tiebreaks by denominator. In practice, since the latency denominator (number of + // announcements and inputs) is always lower, this means that a peer with only high latency scores will be targeted // before a peer using a lot of memory, even if they have the same ratios. do { Assume(!heap_peer_dos.empty()); @@ -640,8 +640,6 @@ void TxOrphanageImpl::EraseForBlock(const CBlock& block) LimitOrphans(); } -/** Get all children that spend from this tx and were received from nodeid. Sorted from most - * recent to least recent. */ std::vector TxOrphanageImpl::GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId peer) const { std::vector children_found; diff --git a/src/node/txorphanage.h b/src/node/txorphanage.h index 146e6730e89..2160879f287 100644 --- a/src/node/txorphanage.h +++ b/src/node/txorphanage.h @@ -94,8 +94,8 @@ public: /** Does this peer have any work to do? */ virtual bool HaveTxToReconsider(NodeId peer) = 0; - /** Get all children that spend from this tx and were received from nodeid. Sorted from most - * recent to least recent. */ + /** Get all children that spend from this tx and were received from nodeid. Sorted + * reconsiderable before non-reconsiderable, then from most recent to least recent. */ virtual std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const = 0; /** Get all orphan transactions */ diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 82030390f05..5bc26f74640 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -367,7 +367,7 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage) FUZZ_TARGET(txorphanage_sim) { SeedRandomStateForTest(SeedRand::ZEROS); - // This is a comphehensive simulation fuzz test, which runs through a scenario involving up to + // This is a comprehensive simulation fuzz test, which runs through a scenario involving up to // 16 transactions (which may have simple or complex topology, and may have duplicate txids // with distinct wtxids, and up to 16 peers. The scenario is performed both on a real // TxOrphanage object and the behavior is compared with a naive reimplementation (just a vector @@ -726,7 +726,7 @@ FUZZ_TARGET(txorphanage_sim) } assert(done); } - // We must now be within limits, otherwise LimitOrphans should have continued further). + // We must now be within limits, otherwise LimitOrphans should have continued further. // We don't check the contents of the orphanage until the end to make fuzz runs faster. assert(real->TotalLatencyScore() <= real->MaxGlobalLatencyScore()); assert(real->TotalOrphanUsage() <= real->MaxGlobalUsage()); From 3d4d4f0d92d42809e74377e4380abdc70f74de5d Mon Sep 17 00:00:00 2001 From: monlovesmango Date: Thu, 24 Jul 2025 18:49:48 +0000 Subject: [PATCH 11/12] scripted-diff: rename "ann" variables to "latency_score" -BEGIN VERIFY SCRIPT- sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.cpp sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.h sed -i 's/max_global_ann/max_global_latency_score/g' src/test/orphanage_tests.cpp sed -i 's/max_global_ann/max_global_latency_score/g' src/test/fuzz/txorphan.cpp sed -i 's/max_global_ann/max_global_latency_score/g' src/bench/txorphanage.cpp sed -i 's/max_ann/max_lat/g' src/node/txorphanage.cpp -END VERIFY SCRIPT- --- src/bench/txorphanage.cpp | 6 +++--- src/node/txorphanage.cpp | 16 ++++++++-------- src/node/txorphanage.h | 2 +- src/test/fuzz/txorphan.cpp | 10 +++++----- src/test/orphanage_tests.cpp | 8 ++++---- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/bench/txorphanage.cpp b/src/bench/txorphanage.cpp index d2ded290e66..dd614fa3f9d 100644 --- a/src/bench/txorphanage.cpp +++ b/src/bench/txorphanage.cpp @@ -68,7 +68,7 @@ static void OrphanageSinglePeerEviction(benchmark::Bench& bench) auto large_tx = MakeTransactionBulkedTo(1, MAX_STANDARD_TX_WEIGHT, det_rand); assert(GetTransactionWeight(*large_tx) <= MAX_STANDARD_TX_WEIGHT); - const auto orphanage{node::MakeTxOrphanage(/*max_global_ann=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; + const auto orphanage{node::MakeTxOrphanage(/*max_global_latency_score=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; // Populate the orphanage. To maximize the number of evictions, first fill up with tiny transactions, then add a huge one. NodeId peer{0}; @@ -131,7 +131,7 @@ static void OrphanageMultiPeerEviction(benchmark::Bench& bench) indexes.resize(NUM_UNIQUE_TXNS); std::iota(indexes.begin(), indexes.end(), 0); - const auto orphanage{node::MakeTxOrphanage(/*max_global_ann=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; + const auto orphanage{node::MakeTxOrphanage(/*max_global_latency_score=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; // Every peer sends the same transactions, all from shared_txs. // Each peer has 1 or 2 assigned transactions, which they must place as the last and second-to-last positions. // The assignments ensure that every transaction is in some peer's last 2 transactions, and is thus remains in the orphanage until the end of LimitOrphans. @@ -189,7 +189,7 @@ static void OrphanageMultiPeerEviction(benchmark::Bench& bench) static void OrphanageEraseAll(benchmark::Bench& bench, bool block_or_disconnect) { FastRandomContext det_rand{true}; - const auto orphanage{node::MakeTxOrphanage(/*max_global_ann=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; + const auto orphanage{node::MakeTxOrphanage(/*max_global_latency_score=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; // This is an unrealistically large number of inputs for a block, as there is almost no room given to witness data, // outputs, and overhead for individual transactions. The entire block is 1 transaction with 20,000 inputs. constexpr unsigned int NUM_BLOCK_INPUTS{MAX_BLOCK_WEIGHT / APPROX_WEIGHT_PER_INPUT}; diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index 6dbcbc32371..43efcda77e6 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -189,8 +189,8 @@ class TxOrphanageImpl final : public TxOrphanage { public: TxOrphanageImpl() = default; - TxOrphanageImpl(Count max_global_ann, Usage reserved_peer_usage) : - m_max_global_latency_score{max_global_ann}, + TxOrphanageImpl(Count max_global_latency_score, Usage reserved_peer_usage) : + m_max_global_latency_score{max_global_latency_score}, m_reserved_usage_per_peer{reserved_peer_usage} {} ~TxOrphanageImpl() noexcept override = default; @@ -447,7 +447,7 @@ void TxOrphanageImpl::LimitOrphans() // Even though it's possible for MaxPeerLatencyScore to increase within this call to LimitOrphans // (e.g. if a peer's orphans are removed entirely, changing the number of peers), use consistent limits throughout. - const auto max_ann{MaxPeerLatencyScore()}; + const auto max_lat{MaxPeerLatencyScore()}; const auto max_mem{ReservedPeerUsage()}; // We have exceeded the global limit(s). Now, identify who is using too much and evict their orphans. @@ -456,7 +456,7 @@ void TxOrphanageImpl::LimitOrphans() heap_peer_dos.reserve(m_peer_orphanage_info.size()); for (const auto& [nodeid, entry] : m_peer_orphanage_info) { // Performance optimization: only consider peers with a DoS score > 1. - const auto dos_score = entry.GetDosScore(max_ann, max_mem); + const auto dos_score = entry.GetDosScore(max_lat, max_mem); if (dos_score >> FeeFrac{1, 1}) { heap_peer_dos.emplace_back(nodeid, dos_score); } @@ -506,7 +506,7 @@ void TxOrphanageImpl::LimitOrphans() // If we erased the last orphan from this peer, it_worst_peer will be invalidated. it_worst_peer = m_peer_orphanage_info.find(worst_peer); - if (it_worst_peer == m_peer_orphanage_info.end() || it_worst_peer->second.GetDosScore(max_ann, max_mem) <= dos_threshold) break; + if (it_worst_peer == m_peer_orphanage_info.end() || it_worst_peer->second.GetDosScore(max_lat, max_mem) <= dos_threshold) break; } LogDebug(BCLog::TXPACKAGES, "peer=%d orphanage overflow, removed %u of %u announcements\n", worst_peer, num_erased_this_round, starting_num_ann); @@ -515,7 +515,7 @@ void TxOrphanageImpl::LimitOrphans() // Unless this peer is empty, put it back in the heap so we continue to consider evicting its orphans. // We may select this peer for evictions again if there are multiple DoSy peers. if (it_worst_peer != m_peer_orphanage_info.end() && it_worst_peer->second.m_count_announcements > 0) { - heap_peer_dos.emplace_back(worst_peer, it_worst_peer->second.GetDosScore(max_ann, max_mem)); + heap_peer_dos.emplace_back(worst_peer, it_worst_peer->second.GetDosScore(max_lat, max_mem)); std::push_heap(heap_peer_dos.begin(), heap_peer_dos.end(), compare_score); } } while (true); @@ -774,8 +774,8 @@ std::unique_ptr MakeTxOrphanage() noexcept { return std::make_unique(); } -std::unique_ptr MakeTxOrphanage(TxOrphanage::Count max_global_ann, TxOrphanage::Usage reserved_peer_usage) noexcept +std::unique_ptr MakeTxOrphanage(TxOrphanage::Count max_global_latency_score, TxOrphanage::Usage reserved_peer_usage) noexcept { - return std::make_unique(max_global_ann, reserved_peer_usage); + return std::make_unique(max_global_latency_score, reserved_peer_usage); } } // namespace node diff --git a/src/node/txorphanage.h b/src/node/txorphanage.h index 2160879f287..5cbf3451d0d 100644 --- a/src/node/txorphanage.h +++ b/src/node/txorphanage.h @@ -146,6 +146,6 @@ public: /** Create a new TxOrphanage instance */ std::unique_ptr MakeTxOrphanage() noexcept; -std::unique_ptr MakeTxOrphanage(TxOrphanage::Count max_global_ann, TxOrphanage::Usage reserved_peer_usage) noexcept; +std::unique_ptr MakeTxOrphanage(TxOrphanage::Count max_global_latency_score, TxOrphanage::Usage reserved_peer_usage) noexcept; } // namespace node #endif // BITCOIN_NODE_TXORPHANAGE_H diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 5bc26f74640..3c795430570 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -470,9 +470,9 @@ FUZZ_TARGET(txorphanage_sim) // 3. Initialize real orphanage // - auto max_global_ann = provider.ConsumeIntegralInRange(NUM_PEERS, MAX_ANN); + auto max_global_latency_score = provider.ConsumeIntegralInRange(NUM_PEERS, MAX_ANN); auto reserved_peer_usage = provider.ConsumeIntegralInRange(1, total_usage); - auto real = node::MakeTxOrphanage(max_global_ann, reserved_peer_usage); + auto real = node::MakeTxOrphanage(max_global_latency_score, reserved_peer_usage); // // 4. Functions and data structures for the simulation. @@ -683,7 +683,7 @@ FUZZ_TARGET(txorphanage_sim) } } // Always trim after each command if needed. - const auto max_ann = max_global_ann / std::max(1, count_peers_fn()); + const auto max_ann = max_global_latency_score / std::max(1, count_peers_fn()); const auto max_mem = reserved_peer_usage; while (true) { // Count global usage and number of peers. @@ -813,12 +813,12 @@ FUZZ_TARGET(txorphanage_sim) // CountUniqueOrphans assert(unique_orphans == real->CountUniqueOrphans()); // MaxGlobalLatencyScore - assert(max_global_ann == real->MaxGlobalLatencyScore()); + assert(max_global_latency_score == real->MaxGlobalLatencyScore()); // ReservedPeerUsage assert(reserved_peer_usage == real->ReservedPeerUsage()); // MaxPeerLatencyScore auto present_peers = count_peers_fn(); - assert(max_global_ann / std::max(1, present_peers) == real->MaxPeerLatencyScore()); + assert(max_global_latency_score / std::max(1, present_peers) == real->MaxPeerLatencyScore()); // MaxGlobalUsage assert(reserved_peer_usage * std::max(1, present_peers) == real->MaxGlobalUsage()); // TotalLatencyScore. diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 2de8693a5df..17ddc71a207 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -94,8 +94,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) { // Test announcement limits NodeId peer{8}; - auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_ann=*/1, /*reserved_peer_usage=*/TX_SIZE * 10); - auto orphanage_low_mem = node::MakeTxOrphanage(/*max_global_ann=*/10, /*reserved_peer_usage=*/TX_SIZE); + auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_latency_score=*/1, /*reserved_peer_usage=*/TX_SIZE * 10); + auto orphanage_low_mem = node::MakeTxOrphanage(/*max_global_latency_score=*/10, /*reserved_peer_usage=*/TX_SIZE); // Add the first transaction orphanage_low_ann->AddTx(txns.at(0), peer); @@ -119,7 +119,7 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) { // Test latency score limits NodeId peer{10}; - auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_ann=*/5, /*reserved_peer_usage=*/TX_SIZE * 1000); + auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_latency_score=*/5, /*reserved_peer_usage=*/TX_SIZE * 1000); // Add the first transaction orphanage_low_ann->AddTx(txns.at(0), peer); @@ -153,7 +153,7 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Test announcement limits NodeId peer{9}; - auto orphanage = node::MakeTxOrphanage(/*max_global_ann=*/3, /*reserved_peer_usage=*/TX_SIZE * 10); + auto orphanage = node::MakeTxOrphanage(/*max_global_latency_score=*/3, /*reserved_peer_usage=*/TX_SIZE * 10); // First add a tx which will be made reconsiderable. orphanage->AddTx(children.at(0), peer); From c0642e558a02319ade33dc1014e7ae981663ea46 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 4 Aug 2025 10:47:48 -0400 Subject: [PATCH 12/12] [fuzz] fix latency score check in txorphan_protected --- src/test/fuzz/txorphan.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 3c795430570..e6b3444749d 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -328,7 +328,7 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage) { if (peer_is_protected && !have_tx_and_peer && (orphanage->UsageByPeer(peer_id) + tx_weight > honest_mem_limit || - orphanage->LatencyScoreFromPeer(peer_id) + (tx->vin.size()) + 1 > honest_latency_limit)) { + orphanage->LatencyScoreFromPeer(peer_id) + (tx->vin.size() / 10) + 1 > honest_latency_limit)) { // We never want our protected peer oversized } else { orphanage->AddAnnouncer(tx->GetWitnessHash(), peer_id);