[refactor] move Find1P1CPackage to txdownload

Move-only.

Author: glozow

src/net_processing.cpp

@@ -564,40 +564,6 @@ class PeerManagerImpl final : public PeerManager
      */
     bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);
 
-    struct PackageToValidate {
-        Package m_txns;
-        std::vector<NodeId> 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 @@ class PeerManagerImpl final : public PeerManager
      * @returns a PackageToValidate if this transaction has a reconsiderable failure and an eligible package was found,
      * or std::nullopt otherwise.
      */
-    std::optional<PackageToValidate> ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result,
-                          bool first_time_failure)
+    std::optional<node::PackageToValidate> 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 @@ class PeerManagerImpl final : public PeerManager
     /** 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<PackageToValidate> 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::PackageToValidate> PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx, const TxValidationState& state,
+std::optional<node::PackageToValidate> 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::PackageToValidate> PeerManagerImpl::ProcessInvali
     // Hashes to pass to AddKnownTx later
     std::vector<uint256> unique_parents;
     // Populated if failure is reconsiderable and eligible package is found.
-    std::optional<PackageToValidate> package_to_validate;
+    std::optional<node::PackageToValidate> 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::PackageToValidate> 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::PackageToValidate> 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<size_t> 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");

src/node/txdownloadman.h

@@ -6,6 +6,7 @@
 #define BITCOIN_NODE_TXDOWNLOADMAN_H
 
 #include <net.h>
+#include <policy/packages.h>
 
 #include <cstdint>
 #include <memory>
@@ -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<NodeId> 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 @@ class TxDownloadManager {
 
     /** Should be called when a notfound for a tx has been received. */
     void ReceivedNotFound(NodeId nodeid, const std::vector<uint256>& 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<PackageToValidate> Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid);
 };
 } // namespace node
 #endif // BITCOIN_NODE_TXDOWNLOADMAN_H

src/node/txdownloadman_impl.cpp

@@ -71,6 +71,10 @@ void TxDownloadManager::ReceivedNotFound(NodeId nodeid, const std::vector<uint25
 {
     m_impl->ReceivedNotFound(nodeid, txhashes);
 }
+std::optional<PackageToValidate> 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<ui
         m_txrequest.ReceivedResponse(nodeid, txhash);
     }
 }
+
+std::optional<PackageToValidate> 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<size_t> 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

src/node/txdownloadman_impl.h

@@ -10,6 +10,7 @@
 #include <kernel/chain.h>
 #include <net.h>
 #include <primitives/transaction.h>
+#include <policy/packages.h>
 #include <txorphanage.h>
 #include <txrequest.h>
 
@@ -159,6 +160,8 @@ class TxDownloadManagerImpl {
 
     /** Marks a tx as ReceivedResponse in txrequest. */
     void ReceivedNotFound(NodeId nodeid, const std::vector<uint256>& txhashes);
+
+    std::optional<PackageToValidate> Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid);
 };
 } // namespace node
 #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H