[refactor] move parent inv-adding to OrphanResolutionCandidate

Deduplicate the logic of adding the parents as announcements to
txrequest. The function can return a bool (indicating whether we're
attempting orphan resolution) instead of the delay.

Author: glozow

src/node/txdownloadman_impl.cpp

@@ -187,15 +187,8 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid,
 
             if (unique_parents.empty()) return true;
 
-            if (auto delay{OrphanResolutionCandidate(peer, Wtxid::FromUint256(gtxid.GetHash()), unique_parents.size())}) {
-                m_orphanage.AddAnnouncer(Wtxid::FromUint256(gtxid.GetHash()), peer);
-
-                const auto& info = m_peer_info.at(peer).m_connection_info;
-                for (const auto& parent_txid : unique_parents) {
-                    m_txrequest.ReceivedInv(peer, GenTxid::Txid(parent_txid), info.m_preferred, now + *delay);
-                }
-
-                LogDebug(BCLog::TXPACKAGES, "added peer=%d as a candidate for resolving orphan %s\n", peer, gtxid.GetHash().ToString());
+            if (OrphanResolutionCandidate(unique_parents, orphan_tx->GetWitnessHash(), peer, now)) {
+                m_orphanage.AddAnnouncer(orphan_tx->GetWitnessHash(), peer);
             }
 
             // Return even if the peer isn't an orphan resolution candidate. This would be caught by AlreadyHaveTx.
@@ -231,21 +224,23 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid,
     return false;
 }
 
-std::optional<std::chrono::seconds> TxDownloadManagerImpl::OrphanResolutionCandidate(NodeId nodeid, const Wtxid& orphan_wtxid, size_t num_parents)
+bool TxDownloadManagerImpl::OrphanResolutionCandidate(const std::vector<Txid>& unique_parents, const Wtxid& wtxid, NodeId nodeid, std::chrono::microseconds now)
 {
-    if (m_peer_info.count(nodeid) == 0) return std::nullopt;
-    if (m_orphanage.HaveTxFromPeer(orphan_wtxid, nodeid)) return std::nullopt;
+    auto it_peer = m_peer_info.find(nodeid);
+    if (it_peer == m_peer_info.end()) return false;
+    if (m_orphanage.HaveTxFromPeer(wtxid, nodeid)) return false;
 
     const auto& peer_entry = m_peer_info.at(nodeid);
     const auto& info = peer_entry.m_connection_info;
+
     // TODO: add delays and limits based on the amount of orphan resolution we are already doing
     // with this peer, how much they are using the orphanage, etc.
     if (!info.m_relay_permissions) {
         // This mirrors the delaying and dropping behavior in AddTxAnnouncement in order to preserve
         // existing behavior: drop if we are tracking too many invs for this peer already. Each
         // orphan resolution involves at least 1 transaction request which may or may not be
         // currently tracked in m_txrequest, so we include that in the count.
-        if (m_txrequest.Count(nodeid) + num_parents > MAX_PEER_TX_ANNOUNCEMENTS) return std::nullopt;
+        if (m_txrequest.Count(nodeid) + unique_parents.size() > MAX_PEER_TX_ANNOUNCEMENTS) return false;
     }
 
     std::chrono::seconds delay{0s};
@@ -258,7 +253,13 @@ std::optional<std::chrono::seconds> TxDownloadManagerImpl::OrphanResolutionCandi
     const bool overloaded = !info.m_relay_permissions && m_txrequest.CountInFlight(nodeid) >= MAX_PEER_TX_REQUEST_IN_FLIGHT;
     if (overloaded) delay += OVERLOADED_PEER_TX_DELAY;
 
-    return delay;
+    // Treat finding orphan resolution candidate as equivalent to the peer announcing all missing parents.
+    // In the future, orphan resolution may include more explicit steps
+    for (const auto& parent_txid : unique_parents) {
+        m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(parent_txid), info.m_preferred, now + delay);
+    }
+    LogDebug(BCLog::TXPACKAGES, "added peer=%d as a candidate for resolving orphan %s\n", nodeid, wtxid.ToString());
+    return true;
 }
 
 std::vector<GenTxid> TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time)
@@ -401,17 +402,8 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
                 add_extra_compact_tx &= !m_orphanage.HaveTx(wtxid);
 
                 auto add_orphan_reso_candidate = [&](const CTransactionRef& orphan_tx, const std::vector<Txid>& unique_parents, NodeId nodeid, std::chrono::microseconds now) {
-                    const auto& wtxid = orphan_tx->GetWitnessHash();
-                    if (auto delay{OrphanResolutionCandidate(nodeid, wtxid, unique_parents.size())}) {
-                        const auto& info = m_peer_info.at(nodeid).m_connection_info;
+                    if (OrphanResolutionCandidate(unique_parents, orphan_tx->GetWitnessHash(), nodeid, now)) {
                         m_orphanage.AddTx(orphan_tx, nodeid);
-
-                        // Treat finding orphan resolution candidate as equivalent to the peer announcing all missing parents
-                        // In the future, orphan resolution may include more explicit steps
-                        for (const auto& parent_txid : unique_parents) {
-                            m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(parent_txid), info.m_preferred, now + *delay);
-                        }
-                        LogDebug(BCLog::TXPACKAGES, "added peer=%d as a candidate for resolving orphan %s\n", nodeid, wtxid.ToString());
                     }
                 };
 

src/node/txdownloadman_impl.h

@@ -194,11 +194,11 @@ class TxDownloadManagerImpl {
     /** Helper for getting deduplicated vector of Txids in vin. */
     std::vector<Txid> GetUniqueParents(const CTransaction& tx);
 
-    /** Determine candidacy (and delay) for potential orphan resolution candidate.
-     * @returns delay for orphan resolution if this peer is a good candidate for orphan resolution,
-     * std::nullopt if this peer cannot be added because it has reached download/orphanage limits.
+    /** If this peer is an orphan resolution candidate for this transaction, treat the unique_parents as announced by
+     * this peer; add them as new invs to m_txrequest.
+     * @returns whether this transaction was a valid orphan resolution candidate.
      * */
-    std::optional<std::chrono::seconds> OrphanResolutionCandidate(NodeId nodeid, const Wtxid& orphan_wtxid, size_t num_parents);
+    bool OrphanResolutionCandidate(const std::vector<Txid>& unique_parents, const Wtxid& wtxid, NodeId nodeid, std::chrono::microseconds now);
 };
 } // namespace node
 #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H