[refactor] move new tx logic to txdownload

glozow · glozow · commit 1e08195135bc · 2024-10-24T21:23:56.000-04:00
Also delete external RecentRejectsReconsiderableFilter() access since it
is no longer necessary.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -808,12 +808,6 @@ class PeerManagerImpl final : public PeerManager
     /** Stalling timeout for blocks in IBD */
     std::atomic<std::chrono::seconds> m_block_stalling_timeout{BLOCK_STALLING_TIMEOUT_DEFAULT};
 
-    CRollingBloomFilter& RecentRejectsReconsiderableFilter() EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex)
-    {
-        AssertLockHeld(m_tx_download_mutex);
-        return m_txdownloadman.RecentRejectsReconsiderableFilter();
-    }
-
     /**
      * For sending `inv`s to inbound peers, we use a single (exponentially
      * distributed) timer for all peers. If we used a separate timer for each
@@ -4239,24 +4233,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         LOCK2(cs_main, m_tx_download_mutex);
 
-        auto& m_txrequest = m_txdownloadman.GetTxRequestRef();
-
-        m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
-        if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
-
-        // We do the AlreadyHaveTx() check using wtxid, rather than txid - in the
-        // absence of witness malleation, this is strictly better, because the
-        // recent rejects filter may contain the wtxid but rarely contains
-        // the txid of a segwit transaction that has been rejected.
-        // In the presence of witness malleation, it's possible that by only
-        // doing the check with wtxid, we could overlook a transaction which
-        // was confirmed with a different witness, or exists in our mempool
-        // with a different witness, but this has limited downside:
-        // mempool validation does its own lookup of whether we have the txid
-        // already; and an adversary can already relay us old transactions
-        // (older than our recency filter) if trying to DoS us, without any need
-        // for witness malleation.
-        if (m_txdownloadman.AlreadyHaveTx(GenTxid::Wtxid(wtxid), /*include_reconsiderable=*/true)) {
+        const auto& [should_validate, package_to_validate] = m_txdownloadman.ReceivedTx(pfrom.GetId(), ptx);
+        if (!should_validate) {
             if (pfrom.HasPermission(NetPermissionFlags::ForceRelay)) {
                 // Always relay transactions received from peers with forcerelay
                 // permission, even if they were already in the mempool, allowing
@@ -4271,37 +4249,18 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                 }
             }
 
-            if (RecentRejectsReconsiderableFilter().contains(wtxid)) {
-                // When a transaction is already in m_lazy_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.
-                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{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");
-                    ProcessPackageResult(package_to_validate.value(), package_result);
-                }
+            if (package_to_validate) {
+                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.value(), package_result);
             }
-            // If a tx is detected by m_lazy_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.
-            //
-            // This means we won't penalize any peer subsequently relaying a DoSy
-            // tx (even if we penalized the first peer who gave it to us) because
-            // we have to account for m_lazy_recent_rejects showing false positives. In
-            // other words, we shouldn't penalize a peer if we aren't *sure* they
-            // submitted a DoSy tx.
-            //
-            // Note that m_lazy_recent_rejects doesn't just record DoSy or invalid
-            // transactions, but any tx not accepted by the mempool, which may be
-            // due to node policy (vs. consensus). So we can't blanket penalize a
-            // peer simply for relaying a tx that our m_lazy_recent_rejects has caught,
-            // regardless of false positives.
             return;
         }
 
+        // ReceivedTx should not be telling us to validate the tx and a package.
+        Assume(!package_to_validate.has_value());
+
         const MempoolAcceptResult result = m_chainman.ProcessTransaction(ptx);
         const TxValidationState& state = result.m_state;
 
diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h
@@ -125,7 +125,6 @@ class TxDownloadManager {
     // temporary and removed later once logic has been moved internally.
     TxOrphanage& GetOrphanageRef();
     TxRequestTracker& GetTxRequestRef();
-    CRollingBloomFilter& RecentRejectsReconsiderableFilter();
 
     // Responses to chain events. TxDownloadManager is not an actual client of ValidationInterface, these are called through PeerManager.
     void ActiveTipChange();
@@ -171,6 +170,11 @@ class TxDownloadManager {
 
     /** Respond to package rejected from mempool */
     void MempoolRejectedPackage(const Package& package);
+
+    /** Marks a tx as ReceivedResponse in txrequest and checks whether AlreadyHaveTx.
+     * Return a bool indicating whether this tx should be validated. If false, optionally, a
+     * PackageToValidate. */
+    std::pair<bool, std::optional<PackageToValidate>> ReceivedTx(NodeId nodeid, const CTransactionRef& ptx);
 };
 } // namespace node
 #endif // BITCOIN_NODE_TXDOWNLOADMAN_H
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
@@ -27,10 +27,6 @@ TxRequestTracker& TxDownloadManager::GetTxRequestRef()
 {
     return m_impl->m_txrequest;
 }
-CRollingBloomFilter& TxDownloadManager::RecentRejectsReconsiderableFilter()
-{
-    return m_impl->RecentRejectsReconsiderableFilter();
-}
 void TxDownloadManager::ActiveTipChange()
 {
     m_impl->ActiveTipChange();
@@ -83,6 +79,10 @@ void TxDownloadManager::MempoolRejectedPackage(const Package& package)
 {
     m_impl->MempoolRejectedPackage(package);
 }
+std::pair<bool, std::optional<PackageToValidate>> TxDownloadManager::ReceivedTx(NodeId nodeid, const CTransactionRef& ptx)
+{
+    return m_impl->ReceivedTx(nodeid, ptx);
+}
 
 // TxDownloadManagerImpl
 void TxDownloadManagerImpl::ActiveTipChange()
@@ -450,4 +450,58 @@ void TxDownloadManagerImpl::MempoolRejectedPackage(const Package& package)
 {
     RecentRejectsReconsiderableFilter().insert(GetPackageHash(package));
 }
+
+std::pair<bool, std::optional<PackageToValidate>> TxDownloadManagerImpl::ReceivedTx(NodeId nodeid, const CTransactionRef& ptx)
+{
+    const uint256& txid = ptx->GetHash();
+    const uint256& wtxid = ptx->GetWitnessHash();
+
+    // Mark that we have received a response
+    m_txrequest.ReceivedResponse(nodeid, txid);
+    if (ptx->HasWitness()) m_txrequest.ReceivedResponse(nodeid, wtxid);
+
+    // First check if we should drop this tx.
+    // We do the AlreadyHaveTx() check using wtxid, rather than txid - in the
+    // absence of witness malleation, this is strictly better, because the
+    // recent rejects filter may contain the wtxid but rarely contains
+    // the txid of a segwit transaction that has been rejected.
+    // In the presence of witness malleation, it's possible that by only
+    // doing the check with wtxid, we could overlook a transaction which
+    // was confirmed with a different witness, or exists in our mempool
+    // with a different witness, but this has limited downside:
+    // mempool validation does its own lookup of whether we have the txid
+    // already; and an adversary can already relay us old transactions
+    // (older than our recency filter) if trying to DoS us, without any need
+    // for witness malleation.
+    if (AlreadyHaveTx(GenTxid::Wtxid(wtxid), /*include_reconsiderable=*/true)) {
+
+        if (RecentRejectsReconsiderableFilter().contains(wtxid)) {
+            // When a transaction is already in m_lazy_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.
+            LogDebug(BCLog::TXPACKAGES, "found tx %s (wtxid=%s) in reconsiderable rejects, looking for child in orphanage\n",
+                     txid.ToString(), wtxid.ToString());
+            return std::make_pair(false, Find1P1CPackage(ptx, nodeid));
+        }
+
+        // If a tx is detected by m_lazy_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.
+        //
+        // This means we won't penalize any peer subsequently relaying a DoSy
+        // tx (even if we penalized the first peer who gave it to us) because
+        // we have to account for m_lazy_recent_rejects showing false positives. In
+        // other words, we shouldn't penalize a peer if we aren't *sure* they
+        // submitted a DoSy tx.
+        //
+        // Note that m_lazy_recent_rejects doesn't just record DoSy or invalid
+        // transactions, but any tx not accepted by the mempool, which may be
+        // due to node policy (vs. consensus). So we can't blanket penalize a
+        // peer simply for relaying a tx that our m_lazy_recent_rejects has caught,
+        // regardless of false positives.
+        return {false, std::nullopt};
+    }
+
+    return {true, std::nullopt};
+}
 } // namespace node
diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h
@@ -167,6 +167,8 @@ class TxDownloadManagerImpl {
     void MempoolAcceptedTx(const CTransactionRef& tx);
     RejectedTxTodo MempoolRejectedTx(const CTransactionRef& ptx, const TxValidationState& state, NodeId nodeid, bool first_time_failure);
     void MempoolRejectedPackage(const Package& package);
+
+    std::pair<bool, std::optional<PackageToValidate>> ReceivedTx(NodeId nodeid, const CTransactionRef& ptx);
 };
 } // namespace node
 #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H
