From 1e08195135bc54f7a8b28560ae10943b1fef0d83 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 16:58:35 +0100 Subject: [PATCH] [refactor] move new tx logic to txdownload Also delete external RecentRejectsReconsiderableFilter() access since it is no longer necessary. --- src/net_processing.cpp | 61 ++++++-------------------------- src/node/txdownloadman.h | 6 +++- src/node/txdownloadman_impl.cpp | 62 ++++++++++++++++++++++++++++++--- src/node/txdownloadman_impl.h | 2 ++ 4 files changed, 75 insertions(+), 56 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8d6b13fe608..3c223e1ea4c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -808,12 +808,6 @@ private: /** Stalling timeout for blocks in IBD */ std::atomic 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 index 514e1064101..164c4b706ef 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -125,7 +125,6 @@ public: // 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 @@ public: /** 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> 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 index 4d5bfad68c1..22bfff167d4 100644 --- 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> 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> 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 index 9843c815e18..019b930a850 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -167,6 +167,8 @@ public: 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> ReceivedTx(NodeId nodeid, const CTransactionRef& ptx); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H