[p2p] opportunistically accept 1-parent-1-child packages

src/net_processing.cpp

@@ -596,6 +596,45 @@ private:
     void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
 
+    /** Handle the results of package validation: calls ProcessValidTx and ProcessInvalidTx for
+     * individual transactions, and caches rejection for the package as a group.
+     * @param[in]   senders     Must contain the nodeids of the peers that provided each transaction
+     *                          in package, in the same order.
+     *   */
+    void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector<NodeId>& senders)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
+
+    /** A package to validate  */
+    struct PackageToValidate {
+        const Package m_txns;
+        const 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}
+        {}
+
+        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());
+        }
+    };
+
+    /** 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)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
+
     /**
      * Reconsider orphan transactions after a parent has been accepted to the mempool.
      *
@@ -3198,6 +3237,117 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c
     }
 }
 
+void PeerManagerImpl::ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector<NodeId>& senders)
+{
+    AssertLockNotHeld(m_peer_mutex);
+    AssertLockHeld(g_msgproc_mutex);
+    AssertLockHeld(cs_main);
+
+    if (package_result.m_state.IsInvalid()) {
+        m_recent_rejects_reconsiderable.insert(GetPackageHash(package));
+    }
+    // We currently only expect to process 1-parent-1-child packages. Remove if this changes.
+    if (!Assume(package.size() == 2)) return;
+
+    // No package results to look through for PCKG_POLICY or PCKG_MEMPOOL_ERROR
+    if (package_result.m_state.GetResult() == PackageValidationResult::PCKG_POLICY ||
+        package_result.m_state.GetResult() == PackageValidationResult::PCKG_MEMPOOL_ERROR) return;
+
+    // Iterate backwards to erase in-package descendants from the orphanage before they become
+    // relevant in AddChildrenToWorkSet.
+    auto package_iter = package.rbegin();
+    auto senders_iter = senders.rbegin();
+    while (package_iter != package.rend()) {
+        const auto& tx = *package_iter;
+        const NodeId nodeid = *senders_iter;
+        const auto it_result{package_result.m_tx_results.find(tx->GetWitnessHash())};
+        if (Assume(it_result != package_result.m_tx_results.end())) {
+            const auto& tx_result = it_result->second;
+            switch (tx_result.m_result_type) {
+                case MempoolAcceptResult::ResultType::VALID:
+                {
+                    Assume(tx_result.m_replaced_transactions.has_value());
+                    std::list<CTransactionRef> empty_replacement_list;
+                    ProcessValidTx(nodeid, tx, tx_result.m_replaced_transactions.value_or(empty_replacement_list));
+                    break;
+                }
+                case MempoolAcceptResult::ResultType::INVALID:
+                case MempoolAcceptResult::ResultType::DIFFERENT_WITNESS:
+                {
+                    // Don't add to vExtraTxnForCompact, as these transactions should have already been
+                    // added there when added to the orphanage or rejected for TX_RECONSIDERABLE.
+                    // This should be updated if package submission is ever used for transactions
+                    // that haven't already been validated before.
+                    ProcessInvalidTx(nodeid, tx, tx_result.m_state, /*maybe_add_extra_compact_tx=*/false);
+                    break;
+                }
+                case MempoolAcceptResult::ResultType::MEMPOOL_ENTRY:
+                {
+                    // AlreadyHaveTx() should be catching transactions that are already in mempool.
+                    Assume(false);
+                    break;
+                }
+            }
+        }
+        package_iter++;
+        senders_iter++;
+    }
+}
+
+std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid)
+{
+    AssertLockNotHeld(m_peer_mutex);
+    AssertLockHeld(g_msgproc_mutex);
+    AssertLockHeld(cs_main);
+
+    const auto& parent_wtxid{ptx->GetWitnessHash()};
+
+    Assume(m_recent_rejects_reconsiderable.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 (!m_recent_rejects_reconsiderable.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);
+    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_recent_rejects_reconsiderable.
+        const auto [child_tx, child_sender] = cpfp_candidates_different_peer.at(index);
+        Package maybe_cpfp_package{ptx, child_tx};
+        if (!m_recent_rejects_reconsiderable.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);
@@ -4377,6 +4527,20 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                     RelayTransaction(tx.GetHash(), tx.GetWitnessHash());
                 }
             }
+
+            if (m_recent_rejects_reconsiderable.contains(wtxid)) {
+                // When a transaction is already in m_recent_rejects_reconsiderable, we shouldn't submit
+                // it by itself again. However, look for a matching child in the orphanage, as it is
+                // possible that they succeed as a package.
+                LogPrint(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())}) {
+                    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");
+                    ProcessPackageResult(package_to_validate->m_txns, package_result, package_to_validate->m_senders);
+                }
+            }
             // If a tx is detected by m_recent_rejects it is ignored. Because we haven't
             // submitted the tx to our mempool, we won't have computed a DoS
             // score for it or determined exactly why we consider it invalid.
@@ -4418,10 +4582,23 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             }
             std::sort(unique_parents.begin(), unique_parents.end());
             unique_parents.erase(std::unique(unique_parents.begin(), unique_parents.end()), unique_parents.end());
+
+            // Distinguish between parents in m_recent_rejects and m_recent_rejects_reconsiderable.
+            // We can tolerate having up to 1 parent in m_recent_rejects_reconsiderable since we
+            // submit 1p1c packages. However, fail immediately if any are in m_recent_rejects.
+            std::optional<uint256> rejected_parent_reconsiderable;
             for (const uint256& parent_txid : unique_parents) {
                 if (m_recent_rejects.contains(parent_txid)) {
                     fRejectedParents = true;
                     break;
+                } else if (m_recent_rejects_reconsiderable.contains(parent_txid) && !m_mempool.exists(GenTxid::Txid(parent_txid))) {
+                    // More than 1 parent in m_recent_rejects_reconsiderable: 1p1c will not be
+                    // sufficient to accept this package, so just give up here.
+                    if (rejected_parent_reconsiderable.has_value()) {
+                        fRejectedParents = true;
+                        break;
+                    }
+                    rejected_parent_reconsiderable = parent_txid;
                 }
             }
             if (!fRejectedParents) {
@@ -4435,7 +4612,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                     // protocol for getting all unconfirmed parents.
                     const auto gtxid{GenTxid::Txid(parent_txid)};
                     AddKnownTx(*peer, parent_txid);
-                    if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) AddTxAnnouncement(pfrom, gtxid, current_time);
+                    // Exclude m_recent_rejects_reconsiderable: the missing parent may have been
+                    // previously rejected for being too low feerate. This orphan might CPFP it.
+                    if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) AddTxAnnouncement(pfrom, gtxid, current_time);
                 }
 
                 if (m_orphanage.AddTx(ptx, pfrom.GetId())) {
@@ -4467,6 +4646,19 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         if (state.IsInvalid()) {
             ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true);
         }
+        // When a transaction fails for TX_RECONSIDERABLE, look for a matching child in the
+        // orphanage, as it is possible that they succeed as a package.
+        if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) {
+            LogPrint(BCLog::TXPACKAGES, "tx %s (wtxid=%s) failed but reconsiderable, looking for child in orphanage\n",
+                     txid.ToString(), wtxid.ToString());
+            if (auto package_to_validate{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");
+                ProcessPackageResult(package_to_validate->m_txns, package_result, package_to_validate->m_senders);
+            }
+        }
+
         return;
     }
 
@@ -6076,7 +6268,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                 entry.second.GetHash().ToString(), entry.first);
         }
         for (const GenTxid& gtxid : requestable) {
-            if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) {
+            // Exclude m_recent_rejects_reconsiderable: we may be requesting a missing parent
+            // that was previously rejected for being too low feerate.
+            if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
                 LogPrint(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx",
                     gtxid.GetHash().ToString(), pto->GetId());
                 vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.GetHash());