From 57221ad97971d399a90d509c5e7b64227f6b2b5e Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 15 Jan 2025 16:53:02 -0500 Subject: [PATCH] [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. --- src/node/txdownloadman_impl.cpp | 40 +++++++++++++-------------------- src/node/txdownloadman_impl.h | 8 +++---- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index fe961048d34..01f34961ad0 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/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,13 +224,15 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, return false; } -std::optional TxDownloadManagerImpl::OrphanResolutionCandidate(NodeId nodeid, const Wtxid& orphan_wtxid, size_t num_parents) +bool TxDownloadManagerImpl::OrphanResolutionCandidate(const std::vector& 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) { @@ -245,7 +240,7 @@ std::optional TxDownloadManagerImpl::OrphanResolutionCandi // 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 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 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& 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()); } }; diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index ab563b22415..dc8a12c372a 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -194,11 +194,11 @@ protected: /** Helper for getting deduplicated vector of Txids in vin. */ std::vector 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 OrphanResolutionCandidate(NodeId nodeid, const Wtxid& orphan_wtxid, size_t num_parents); + bool OrphanResolutionCandidate(const std::vector& unique_parents, const Wtxid& wtxid, NodeId nodeid, std::chrono::microseconds now); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H