From ed24e016969098c486f413f4f57dcffe35241785 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 26 Jun 2025 14:56:16 -0400 Subject: [PATCH] [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.