[refactor] make TxOrphanage keep itself trimmed

Author: glozow

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};

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",

src/node/txorphanage.cpp

@@ -172,11 +172,18 @@ class TxOrphanageImpl final : public TxOrphanage {
     template<typename Tag>
     void Erase(Iter<Tag> 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<ByWtxid> 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 @@ class TxOrphanageImpl final : public TxOrphanage {
     bool EraseTx(const Wtxid& wtxid) override;
     void EraseForPeer(NodeId peer) override;
     void EraseForBlock(const CBlock& block) override;
-    void LimitOrphans() override;
     std::vector<std::pair<Wtxid, NodeId>> AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng) override;
     bool HaveTxToReconsider(NodeId peer) override;
     std::vector<CTransactionRef> 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<ByWtxid>();
 
@@ -378,12 +390,21 @@ bool TxOrphanageImpl::EraseTx(const Wtxid& wtxid)
         Erase<ByWtxid>(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<ByPeer>().lower_bound(ByPeerView{peer, true, 0});
     return it != m_orphans.get<ByPeer>().end() && it->m_announcer == peer && it->m_reconsider;
 }
+
 void TxOrphanageImpl::EraseForBlock(const CBlock& block)
 {
     std::set<Wtxid> 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; }

src/node/txorphanage.h

@@ -88,9 +88,6 @@ class TxOrphanage {
     /** 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<std::pair<Wtxid, NodeId>> AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng) = 0;