[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.

Author: sipa

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<COutPoint, std::set<Wtxid>, SaltedOutpointHasher> m_outpoint_to_orphan_it;
 
+    /** Set of Wtxids for which (exactly) one announcement with m_reconsider=true exists. */
+    std::set<Wtxid> 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<Tag> 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<Tag>().erase(it);
 }
 
@@ -521,6 +529,9 @@ std::vector<std::pair<Wtxid, NodeId>> 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<std::pair<Wtxid, NodeId>> 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<ByPeer>().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<NodeId, PeerDoSInfo> reconstructed_peer_info;
     std::map<Wtxid, std::pair<TxOrphanage::Usage, TxOrphanage::Count>> unique_wtxids_to_scores;
     std::set<COutPoint> all_outpoints;
+    std::set<Wtxid> 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.

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<Wtxid, bool> child_wtxids;
+                /** Set of not-already-reconsiderable child wtxids. */
+                std::set<Wtxid> 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.