[p2p] drop orphan reso candidates that are taking up too much of orph…

…anage

src/node/txdownload_impl.cpp

@@ -230,8 +230,12 @@ void TxDownloadImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::c
 {
     if (m_peer_info.count(peer) == 0) return;
     if (m_orphanage.HaveTx(gtxid) || (gtxid.IsWtxid() && m_orphanage.HaveTx(GenTxid::Txid(gtxid.GetHash())))) {
-        if (gtxid.IsWtxid() && CanAddOrphan(peer, gtxid.GetHash())) {
-            AddOrphanAnnouncer(peer, Wtxid::FromUint256(gtxid.GetHash()), now, /*already_added=*/false);
+        if (gtxid.IsWtxid()) {
+            const auto tx{m_orphanage.GetTx(Wtxid::FromUint256(gtxid.GetHash()))};
+            const auto parents{m_orphanage.GetParentTxids(Wtxid::FromUint256(gtxid.GetHash()))};
+            if (tx && parents && CanAddOrphan(peer, gtxid.GetHash(), tx->GetTotalSize(), parents->size())) {
+                AddOrphanAnnouncer(peer, Wtxid::FromUint256(gtxid.GetHash()), now, /*already_added=*/false);
+            }
         }
         return;
     }
@@ -365,7 +369,7 @@ void TxDownloadImpl::ReceivedNotFound(NodeId nodeid, const std::vector<uint256>&
     }
 }
 
-bool TxDownloadImpl::CanAddOrphan(NodeId nodeid, const uint256& orphan_wtxid)
+bool TxDownloadImpl::CanAddOrphan(NodeId nodeid, const uint256& orphan_wtxid, unsigned int tx_bytes, uint32_t num_parents)
 {
     AssertLockHeld(m_tx_download_mutex);
     if (Assume(m_peer_info.count(nodeid) != 0)) return false;
@@ -375,8 +379,14 @@ bool TxDownloadImpl::CanAddOrphan(NodeId nodeid, const uint256& orphan_wtxid)
 
     const auto& info = m_peer_info.at(nodeid).m_connection_info;
     if (!info.m_relay_permissions) {
-        // Too many queued orphan resolutions with this peer.
-        if (m_orphan_resolution_tracker.Count(nodeid) >= MAX_PEER_TX_ANNOUNCEMENTS) return false;
+        // If we added all parents to txrequest, there would be too many txrequests for this peer.
+        if (m_txrequest.Count(nodeid) + num_parents > MAX_PEER_TX_ANNOUNCEMENTS) return false;
+
+        // If we added this tx to the orphanage, there would be too many entries stored for this peer.
+        if (m_orphan_resolution_tracker.Count(nodeid) + 1 > MAX_ORPHANAGE_COUNT_PER_PER_PEER) return false;
+
+        // If we added this tx to the orphanage, there would be too many bytes stored for this peer.
+        if (m_orphanage.BytesFromPeer(nodeid) + tx_bytes > MAX_ORPHANAGE_BYTES_PER_PER_PEER) return false;
     }
     return true;
 }
@@ -432,14 +442,15 @@ std::pair<bool, std::vector<Txid>> TxDownloadImpl::NewOrphanTx(const CTransactio
             unique_parents.end());
     }
 
-    // Collect all orphan resolution candidates.
+    // Collect all orphan resolution candidates. Do this before attempting to add the new orphan.
     std::vector<NodeId> orphan_resolution_candidates;
-    if (CanAddOrphan(nodeid, wtxid)) orphan_resolution_candidates.emplace_back(nodeid);
+    const auto tx_bytes{tx->GetTotalSize()};
+    if (CanAddOrphan(nodeid, wtxid, tx_bytes, unique_parents.size())) orphan_resolution_candidates.emplace_back(nodeid);
     for (const auto candidate : m_txrequest.GetCandidatePeers(wtxid)) {
-        if (CanAddOrphan(candidate, wtxid)) orphan_resolution_candidates.emplace_back(candidate);
+        if (CanAddOrphan(candidate, wtxid, tx_bytes, unique_parents.size())) orphan_resolution_candidates.emplace_back(candidate);
     }
     for (const auto candidate : m_txrequest.GetCandidatePeers(txid)) {
-        if (CanAddOrphan(candidate, wtxid)) orphan_resolution_candidates.emplace_back(candidate);
+        if (CanAddOrphan(candidate, wtxid, tx_bytes, unique_parents.size())) orphan_resolution_candidates.emplace_back(candidate);
     }
 
     if (!orphan_resolution_candidates.empty() && !already_in_orphanage) {

src/node/txdownload_impl.h

@@ -29,6 +29,16 @@ static constexpr auto NONPREF_PEER_TX_DELAY{2s};
 static constexpr auto OVERLOADED_PEER_TX_DELAY{2s};
 /** How long to wait before downloading a transaction from an additional peer */
 static constexpr auto GETDATA_TX_INTERVAL{60s};
+/** Maximum number of orphans for which a single peer may be a resolution candidate, unless they
+ * have Relay permissions. Equivalent to 20% of the maximum total allowed orphans. We will not
+ * consider another (tx, peer) for orphan resolution if it would make the peer exceed this limit.
+ * However, we may consider other peers as candidates for the tx if they are eligible. */
+static constexpr unsigned int MAX_ORPHANAGE_COUNT_PER_PER_PEER{20};
+/** Maximum total bytes of orphans for which a single peer may be a resolution candidate, unless
+ * they have Relay permissions. Equivalent to 10% of the maximum total orphanage capacity. We will
+ * not consider another (tx, peer) for orphan resolution if it would make the peer exceed this
+ * limit. However, we may consider other peers as candidates for the tx if they are eligible. */
+static constexpr unsigned int MAX_ORPHANAGE_BYTES_PER_PER_PEER{10*400'000};
 
 struct TxDownloadOptions {
     /** Global maximum number of orphan transactions to keep. Enforced with LimitOrphans. */
@@ -154,7 +164,8 @@ class TxDownloadImpl {
 protected:
     /** Whether this peer can be a candidate for orphan resolution. Returns false if the peer
      * doesn't exist, is already a candidate for this tx, or has reached orphan limits. */
-    bool CanAddOrphan(NodeId nodeid, const uint256& orphan_wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex);
+    bool CanAddOrphan(NodeId nodeid, const uint256& orphan_wtxid, unsigned int tx_bytes, uint32_t num_parents)
+        EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex);
 
     /** Maybe adds an inv to txrequest. */
     void AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now)

src/test/fuzz/txdownloadman.cpp

@@ -281,10 +281,16 @@ static void CheckInvariants(const node::TxDownloadImpl& txdownload_impl, size_t
         }
     }
 
-    // We should never have more than the maximum in-flight requests out for a peer.
+    // For each peer, we should never exceed:
+    // - MAX_PEER_TX_REQUEST_IN_FLIGHT total in-flight requests per peer (which includes tx requests
+    // for orphan ancestors)
+    // - MAX_ORPHANAGE_COUNT_PER_PER_PEER number of transactions in the orphanage
+    // - MAX_ORPHANAGE_BYTES_PER_PER_PEER total size, in bytes, of transactions in the orphanage
     for (NodeId peer = 0; peer < NUM_PEERS; ++peer) {
         if (!HasRelayPermissions(peer)) {
             Assert(txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT);
+            Assert(txdownload_impl.m_orphan_resolution_tracker.CountInFlight(peer) <= node::MAX_ORPHANAGE_COUNT_PER_PER_PEER);
+            Assert(txdownload_impl.m_orphanage.BytesFromPeer(peer) <= node::MAX_ORPHANAGE_BYTES_PER_PER_PEER);
         }
     }
 }