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.