From a8cf3b6e845741e4b992beced564397779bfb7da Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 25 Apr 2024 11:24:31 +0100 Subject: [PATCH] [refactor] move Find1P1CPackage to txdownload Move-only. --- src/net_processing.cpp | 113 +++----------------------------- src/node/txdownloadman.h | 41 ++++++++++++ src/node/txdownloadman_impl.cpp | 54 +++++++++++++++ src/node/txdownloadman_impl.h | 3 + 4 files changed, 107 insertions(+), 104 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5e4a77cf468..57e6fc7b2fa 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -564,40 +564,6 @@ private: */ bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer); - struct PackageToValidate { - Package m_txns; - std::vector m_senders; - /** Construct a 1-parent-1-child package. */ - explicit PackageToValidate(const CTransactionRef& parent, - const CTransactionRef& child, - NodeId parent_sender, - NodeId child_sender) : - m_txns{parent, child}, - m_senders{parent_sender, child_sender} - {} - - // Move ctor - PackageToValidate(PackageToValidate&& other) : m_txns{std::move(other.m_txns)}, m_senders{std::move(other.m_senders)} {} - - // Move assignment - PackageToValidate& operator=(PackageToValidate&& other) { - this->m_txns = std::move(other.m_txns); - this->m_senders = std::move(other.m_senders); - return *this; - } - - std::string ToString() const { - Assume(m_txns.size() == 2); - return strprintf("parent %s (wtxid=%s, sender=%d) + child %s (wtxid=%s, sender=%d)", - m_txns.front()->GetHash().ToString(), - m_txns.front()->GetWitnessHash().ToString(), - m_senders.front(), - m_txns.back()->GetHash().ToString(), - m_txns.back()->GetWitnessHash().ToString(), - m_senders.back()); - } - }; - /** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID. * @param[in] first_time_failure Whether we should consider inserting into vExtraTxnForCompact, adding * a new orphan to resolve, or looking for a package to submit. @@ -609,8 +575,8 @@ private: * @returns a PackageToValidate if this transaction has a reconsiderable failure and an eligible package was found, * or std::nullopt otherwise. */ - std::optional ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result, - bool first_time_failure) + std::optional ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result, + bool first_time_failure) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); /** Handle a transaction whose result was MempoolAcceptResult::ResultType::VALID. @@ -621,13 +587,7 @@ private: /** Handle the results of package validation: calls ProcessValidTx and ProcessInvalidTx for * individual transactions, and caches rejection for the package as a group. */ - void ProcessPackageResult(const PackageToValidate& package_to_validate, const PackageMempoolAcceptResult& package_result) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); - - /** Look for a child of this transaction in the orphanage to form a 1-parent-1-child package, - * skipping any combinations that have already been tried. Return the resulting package along with - * the senders of its respective transactions, or std::nullopt if no package is found. */ - std::optional Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid) + void ProcessPackageResult(const node::PackageToValidate& package_to_validate, const PackageMempoolAcceptResult& package_result) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); /** @@ -1946,7 +1906,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, m_banman(banman), m_chainman(chainman), m_mempool(pool), - m_txdownloadman(node::TxDownloadOptions{pool}), + m_txdownloadman(node::TxDownloadOptions{pool, m_rng}), m_warnings{warnings}, m_opts{opts} { @@ -3015,7 +2975,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, return; } -std::optional PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx, const TxValidationState& state, +std::optional PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx, const TxValidationState& state, bool first_time_failure) { AssertLockNotHeld(m_peer_mutex); @@ -3039,7 +2999,7 @@ std::optional PeerManagerImpl::ProcessInvali // Hashes to pass to AddKnownTx later std::vector unique_parents; // Populated if failure is reconsiderable and eligible package is found. - std::optional package_to_validate; + std::optional package_to_validate; if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { // Only process a new orphan if this is a first time failure, as otherwise it must be either @@ -3145,7 +3105,7 @@ std::optional PeerManagerImpl::ProcessInvali // orphanage, as it is possible that they succeed as a package. LogDebug(BCLog::TXPACKAGES, "tx %s (wtxid=%s) failed but reconsiderable, looking for child in orphanage\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); - package_to_validate = Find1P1CPackage(ptx, nodeid); + package_to_validate = m_txdownloadman.Find1P1CPackage(ptx, nodeid); } } else { RecentRejectsFilter().insert(ptx->GetWitnessHash().ToUint256()); @@ -3215,7 +3175,7 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c } } -void PeerManagerImpl::ProcessPackageResult(const PackageToValidate& package_to_validate, const PackageMempoolAcceptResult& package_result) +void PeerManagerImpl::ProcessPackageResult(const node::PackageToValidate& package_to_validate, const PackageMempoolAcceptResult& package_result) { AssertLockNotHeld(m_peer_mutex); AssertLockHeld(g_msgproc_mutex); @@ -3271,61 +3231,6 @@ void PeerManagerImpl::ProcessPackageResult(const PackageToValidate& package_to_v } } -std::optional PeerManagerImpl::Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid) -{ - AssertLockNotHeld(m_peer_mutex); - AssertLockHeld(g_msgproc_mutex); - AssertLockHeld(m_tx_download_mutex); - - const auto& parent_wtxid{ptx->GetWitnessHash()}; - auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); - - Assume(RecentRejectsReconsiderableFilter().contains(parent_wtxid.ToUint256())); - - // Prefer children from this peer. This helps prevent censorship attempts in which an attacker - // sends lots of fake children for the parent, and we (unluckily) keep selecting the fake - // children instead of the real one provided by the honest peer. - const auto cpfp_candidates_same_peer{m_orphanage.GetChildrenFromSamePeer(ptx, nodeid)}; - - // These children should be sorted from newest to oldest. In the (probably uncommon) case - // of children that replace each other, this helps us accept the highest feerate (probably the - // most recent) one efficiently. - for (const auto& child : cpfp_candidates_same_peer) { - Package maybe_cpfp_package{ptx, child}; - if (!RecentRejectsReconsiderableFilter().contains(GetPackageHash(maybe_cpfp_package))) { - return PeerManagerImpl::PackageToValidate{ptx, child, nodeid, nodeid}; - } - } - - // If no suitable candidate from the same peer is found, also try children that were provided by - // a different peer. This is useful because sometimes multiple peers announce both transactions - // to us, and we happen to download them from different peers (we wouldn't have known that these - // 2 transactions are related). We still want to find 1p1c packages then. - // - // If we start tracking all announcers of orphans, we can restrict this logic to parent + child - // pairs in which both were provided by the same peer, i.e. delete this step. - const auto cpfp_candidates_different_peer{m_orphanage.GetChildrenFromDifferentPeer(ptx, nodeid)}; - - // Find the first 1p1c that hasn't already been rejected. We randomize the order to not - // create a bias that attackers can use to delay package acceptance. - // - // Create a random permutation of the indices. - std::vector tx_indices(cpfp_candidates_different_peer.size()); - std::iota(tx_indices.begin(), tx_indices.end(), 0); - std::shuffle(tx_indices.begin(), tx_indices.end(), m_rng); - - for (const auto index : tx_indices) { - // If we already tried a package and failed for any reason, the combined hash was - // cached in m_lazy_recent_rejects_reconsiderable. - const auto [child_tx, child_sender] = cpfp_candidates_different_peer.at(index); - Package maybe_cpfp_package{ptx, child_tx}; - if (!RecentRejectsReconsiderableFilter().contains(GetPackageHash(maybe_cpfp_package))) { - return PeerManagerImpl::PackageToValidate{ptx, child_tx, nodeid, child_sender}; - } - } - return std::nullopt; -} - bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) { AssertLockHeld(g_msgproc_mutex); @@ -4528,7 +4433,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // possible that they succeed as a package. LogDebug(BCLog::TXPACKAGES, "found tx %s (wtxid=%s) in reconsiderable rejects, looking for child in orphanage\n", txid.ToString(), wtxid.ToString()); - if (auto package_to_validate{Find1P1CPackage(ptx, pfrom.GetId())}) { + if (auto package_to_validate{m_txdownloadman.Find1P1CPackage(ptx, pfrom.GetId())}) { const auto package_result{ProcessNewPackage(m_chainman.ActiveChainstate(), m_mempool, package_to_validate->m_txns, /*test_accept=*/false, /*client_maxfeerate=*/std::nullopt)}; LogDebug(BCLog::TXPACKAGES, "package evaluation for %s: %s\n", package_to_validate->ToString(), package_result.m_state.IsValid() ? "package accepted" : "package rejected"); diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 6995725ecc6..1a1f0a6da71 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -6,6 +6,7 @@ #define BITCOIN_NODE_TXDOWNLOADMAN_H #include +#include #include #include @@ -38,6 +39,8 @@ static constexpr auto GETDATA_TX_INTERVAL{60s}; struct TxDownloadOptions { /** Read-only reference to mempool. */ const CTxMemPool& m_mempool; + /** RNG provided by caller. */ + FastRandomContext& m_rng; }; struct TxDownloadConnectionInfo { /** Whether this peer is preferred for transaction download. */ @@ -47,6 +50,39 @@ struct TxDownloadConnectionInfo { /** Whether this peer supports wtxid relay. */ const bool m_wtxid_relay; }; +struct PackageToValidate { + Package m_txns; + std::vector m_senders; + /** Construct a 1-parent-1-child package. */ + explicit PackageToValidate(const CTransactionRef& parent, + const CTransactionRef& child, + NodeId parent_sender, + NodeId child_sender) : + m_txns{parent, child}, + m_senders{parent_sender, child_sender} + {} + + // Move ctor + PackageToValidate(PackageToValidate&& other) : m_txns{std::move(other.m_txns)}, m_senders{std::move(other.m_senders)} {} + + // Move assignment + PackageToValidate& operator=(PackageToValidate&& other) { + this->m_txns = std::move(other.m_txns); + this->m_senders = std::move(other.m_senders); + return *this; + } + + std::string ToString() const { + Assume(m_txns.size() == 2); + return strprintf("parent %s (wtxid=%s, sender=%d) + child %s (wtxid=%s, sender=%d)", + m_txns.front()->GetHash().ToString(), + m_txns.front()->GetWitnessHash().ToString(), + m_senders.front(), + m_txns.back()->GetHash().ToString(), + m_txns.back()->GetWitnessHash().ToString(), + m_senders.back()); + } +}; /** * Class responsible for deciding what transactions to request and, once @@ -111,6 +147,11 @@ public: /** Should be called when a notfound for a tx has been received. */ void ReceivedNotFound(NodeId nodeid, const std::vector& txhashes); + + /** Look for a child of this transaction in the orphanage to form a 1-parent-1-child package, + * skipping any combinations that have already been tried. Return the resulting package along with + * the senders of its respective transactions, or std::nullopt if no package is found. */ + std::optional Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 6b505af22f2..2a62b090c00 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -71,6 +71,10 @@ void TxDownloadManager::ReceivedNotFound(NodeId nodeid, const std::vectorReceivedNotFound(nodeid, txhashes); } +std::optional TxDownloadManager::Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid) +{ + return m_impl->Find1P1CPackage(ptx, nodeid); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -218,4 +222,54 @@ void TxDownloadManagerImpl::ReceivedNotFound(NodeId nodeid, const std::vector TxDownloadManagerImpl::Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid) +{ + const auto& parent_wtxid{ptx->GetWitnessHash()}; + + Assume(RecentRejectsReconsiderableFilter().contains(parent_wtxid.ToUint256())); + + // Prefer children from this peer. This helps prevent censorship attempts in which an attacker + // sends lots of fake children for the parent, and we (unluckily) keep selecting the fake + // children instead of the real one provided by the honest peer. + const auto cpfp_candidates_same_peer{m_orphanage.GetChildrenFromSamePeer(ptx, nodeid)}; + + // These children should be sorted from newest to oldest. In the (probably uncommon) case + // of children that replace each other, this helps us accept the highest feerate (probably the + // most recent) one efficiently. + for (const auto& child : cpfp_candidates_same_peer) { + Package maybe_cpfp_package{ptx, child}; + if (!RecentRejectsReconsiderableFilter().contains(GetPackageHash(maybe_cpfp_package))) { + return PackageToValidate{ptx, child, nodeid, nodeid}; + } + } + + // If no suitable candidate from the same peer is found, also try children that were provided by + // a different peer. This is useful because sometimes multiple peers announce both transactions + // to us, and we happen to download them from different peers (we wouldn't have known that these + // 2 transactions are related). We still want to find 1p1c packages then. + // + // If we start tracking all announcers of orphans, we can restrict this logic to parent + child + // pairs in which both were provided by the same peer, i.e. delete this step. + const auto cpfp_candidates_different_peer{m_orphanage.GetChildrenFromDifferentPeer(ptx, nodeid)}; + + // Find the first 1p1c that hasn't already been rejected. We randomize the order to not + // create a bias that attackers can use to delay package acceptance. + // + // Create a random permutation of the indices. + std::vector tx_indices(cpfp_candidates_different_peer.size()); + std::iota(tx_indices.begin(), tx_indices.end(), 0); + std::shuffle(tx_indices.begin(), tx_indices.end(), m_opts.m_rng); + + for (const auto index : tx_indices) { + // If we already tried a package and failed for any reason, the combined hash was + // cached in m_lazy_recent_rejects_reconsiderable. + const auto [child_tx, child_sender] = cpfp_candidates_different_peer.at(index); + Package maybe_cpfp_package{ptx, child_tx}; + if (!RecentRejectsReconsiderableFilter().contains(GetPackageHash(maybe_cpfp_package))) { + return PackageToValidate{ptx, child_tx, nodeid, child_sender}; + } + } + return std::nullopt; +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index a2af7506e7b..730a9642ad8 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -159,6 +160,8 @@ public: /** Marks a tx as ReceivedResponse in txrequest. */ void ReceivedNotFound(NodeId nodeid, const std::vector& txhashes); + + std::optional Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H