From 5f9004e1550f726ca9dc9a08c865fa8f2e4b92e8 Mon Sep 17 00:00:00 2001 From: glozow Date: Sun, 1 May 2022 10:41:53 -0700 Subject: [PATCH 01/29] [refactor] add TxDownloadManager wrapping TxOrphanage, TxRequestTracker, and bloom filters This module is going to be responsible for managing everything related to transaction download, including txrequest, orphan transactions and package relay. It will be responsible for managing usage of the TxOrphanage and instructing PeerManager: - what tx or package-related messages to send to which peer - whether a tx or package-related message is allowed or useful - what transactions are available to try accepting to mempool Future commits will consolidate the interface and re-delegate interactions from PeerManager to TxDownloadManager. --- src/CMakeLists.txt | 1 + src/net_processing.cpp | 143 ++++++++------------------------ src/node/txdownloadman.h | 53 ++++++++++++ src/node/txdownloadman_impl.cpp | 34 ++++++++ src/node/txdownloadman_impl.h | 127 ++++++++++++++++++++++++++++ 5 files changed, 250 insertions(+), 108 deletions(-) create mode 100644 src/node/txdownloadman.h create mode 100644 src/node/txdownloadman_impl.cpp create mode 100644 src/node/txdownloadman_impl.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index d10638e887d..59d67d713a4 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -246,6 +246,7 @@ add_library(bitcoin_node STATIC EXCLUDE_FROM_ALL node/psbt.cpp node/timeoffsets.cpp node/transaction.cpp + node/txdownloadman_impl.cpp node/txreconciliation.cpp node/utxo_snapshot.cpp node/warnings.cpp diff --git a/src/net_processing.cpp b/src/net_processing.cpp index be16884011e..9554b080cc2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -781,7 +782,8 @@ class PeerManagerImpl final : public PeerManager * - Each data structure's limits hold (m_orphanage max size, m_txrequest per-peer limits, etc). */ Mutex m_tx_download_mutex ACQUIRED_BEFORE(m_mempool.cs); - TxRequestTracker m_txrequest GUARDED_BY(m_tx_download_mutex); + node::TxDownloadManager m_txdownloadman GUARDED_BY(m_tx_download_mutex); + std::unique_ptr m_txreconciliation; /** The height of the best chain */ @@ -862,112 +864,23 @@ class PeerManagerImpl final : public PeerManager bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex); - /** - * Filter for transactions that were recently rejected by the mempool. - * These are not rerequested until the chain tip changes, at which point - * the entire filter is reset. - * - * Without this filter we'd be re-requesting txs from each of our peers, - * increasing bandwidth consumption considerably. For instance, with 100 - * peers, half of which relay a tx we don't accept, that might be a 50x - * bandwidth increase. A flooding attacker attempting to roll-over the - * filter using minimum-sized, 60byte, transactions might manage to send - * 1000/sec if we have fast peers, so we pick 120,000 to give our peers a - * two minute window to send invs to us. - * - * Decreasing the false positive rate is fairly cheap, so we pick one in a - * million to make it highly unlikely for users to have issues with this - * filter. - * - * We typically only add wtxids to this filter. For non-segwit - * transactions, the txid == wtxid, so this only prevents us from - * re-downloading non-segwit transactions when communicating with - * non-wtxidrelay peers -- which is important for avoiding malleation - * attacks that could otherwise interfere with transaction relay from - * non-wtxidrelay peers. For communicating with wtxidrelay peers, having - * the reject filter store wtxids is exactly what we want to avoid - * redownload of a rejected transaction. - * - * In cases where we can tell that a segwit transaction will fail - * validation no matter the witness, we may add the txid of such - * transaction to the filter as well. This can be helpful when - * communicating with txid-relay peers or if we were to otherwise fetch a - * transaction via txid (eg in our orphan handling). - * - * Memory used: 1.3 MB - */ - std::unique_ptr m_lazy_recent_rejects GUARDED_BY(m_tx_download_mutex){nullptr}; CRollingBloomFilter& RecentRejectsFilter() EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex) { AssertLockHeld(m_tx_download_mutex); - - if (!m_lazy_recent_rejects) { - m_lazy_recent_rejects = std::make_unique(120'000, 0.000'001); - } - - return *m_lazy_recent_rejects; + return m_txdownloadman.RecentRejectsFilter(); } - /** - * Filter for: - * (1) wtxids of transactions that were recently rejected by the mempool but are - * eligible for reconsideration if submitted with other transactions. - * (2) packages (see GetPackageHash) we have already rejected before and should not retry. - * - * Similar to m_lazy_recent_rejects, this filter is used to save bandwidth when e.g. all of our peers - * have larger mempools and thus lower minimum feerates than us. - * - * When a transaction's error is TxValidationResult::TX_RECONSIDERABLE (in a package or by - * itself), add its wtxid to this filter. When a package fails for any reason, add the combined - * hash to this filter. - * - * Upon receiving an announcement for a transaction, if it exists in this filter, do not - * download the txdata. When considering packages, if it exists in this filter, drop it. - * - * Reset this filter when the chain tip changes. - * - * Parameters are picked to be the same as m_lazy_recent_rejects, with the same rationale. - */ - std::unique_ptr m_lazy_recent_rejects_reconsiderable GUARDED_BY(m_tx_download_mutex){nullptr}; - CRollingBloomFilter& RecentRejectsReconsiderableFilter() EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex) { AssertLockHeld(m_tx_download_mutex); - - if (!m_lazy_recent_rejects_reconsiderable) { - m_lazy_recent_rejects_reconsiderable = std::make_unique(120'000, 0.000'001); - } - - return *m_lazy_recent_rejects_reconsiderable; + return m_txdownloadman.RecentRejectsReconsiderableFilter(); } - /* - * Filter for transactions that have been recently confirmed. - * We use this to avoid requesting transactions that have already been - * confirnmed. - * - * Blocks don't typically have more than 4000 transactions, so this should - * be at least six blocks (~1 hr) worth of transactions that we can store, - * inserting both a txid and wtxid for every observed transaction. - * If the number of transactions appearing in a block goes up, or if we are - * seeing getdata requests more than an hour after initial announcement, we - * can increase this number. - * The false positive rate of 1/1M should come out to less than 1 - * transaction per day that would be inadvertently ignored (which is the - * same probability that we have in the reject filter). - */ - std::unique_ptr m_lazy_recent_confirmed_transactions GUARDED_BY(m_tx_download_mutex){nullptr}; - CRollingBloomFilter& RecentConfirmedTransactionsFilter() EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex) { AssertLockHeld(m_tx_download_mutex); - - if (!m_lazy_recent_confirmed_transactions) { - m_lazy_recent_confirmed_transactions = std::make_unique(48'000, 0.000'001); - } - - return *m_lazy_recent_confirmed_transactions; + return m_txdownloadman.RecentConfirmedTransactionsFilter(); } /** @@ -1104,9 +1017,6 @@ class PeerManagerImpl final : public PeerManager /** Number of peers from which we're downloading blocks. */ int m_peers_downloading_from GUARDED_BY(cs_main) = 0; - /** Storage for orphan information */ - TxOrphanage m_orphanage GUARDED_BY(m_tx_download_mutex); - void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction. @@ -1683,6 +1593,8 @@ void PeerManagerImpl::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, AssertLockHeld(::cs_main); // for State AssertLockHeld(m_tx_download_mutex); // For m_txrequest NodeId nodeid = node.GetId(); + + auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); if (!node.HasPermission(NetPermissionFlags::Relay) && m_txrequest.Count(nodeid) >= MAX_PEER_TX_ANNOUNCEMENTS) { // Too many queued announcements from this peer return; @@ -1722,7 +1634,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service } { LOCK(m_tx_download_mutex); - assert(m_txrequest.Count(nodeid) == 0); + assert(m_txdownloadman.GetTxRequestRef().Count(nodeid) == 0); } if (NetPermissions::HasFlag(node.m_permission_flags, NetPermissionFlags::BloomFilter)) { @@ -1791,8 +1703,8 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) } { LOCK(m_tx_download_mutex); - m_orphanage.EraseForPeer(nodeid); - m_txrequest.DisconnectedPeer(nodeid); + m_txdownloadman.GetOrphanageRef().EraseForPeer(nodeid); + m_txdownloadman.GetTxRequestRef().DisconnectedPeer(nodeid); } if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid); m_num_preferred_download_peers -= state->fPreferredDownload; @@ -1811,8 +1723,8 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) assert(m_outbound_peers_with_protect_from_disconnect == 0); assert(m_wtxid_relay_peers == 0); LOCK(m_tx_download_mutex); - assert(m_txrequest.Size() == 0); - assert(m_orphanage.Size() == 0); + assert(m_txdownloadman.GetTxRequestRef().Size() == 0); + assert(m_txdownloadman.GetOrphanageRef().Size() == 0); } } // cs_main if (node.fSuccessfullyConnected && @@ -1921,7 +1833,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c std::vector PeerManagerImpl::GetOrphanTransactions() { LOCK(m_tx_download_mutex); - return m_orphanage.GetOrphanTransactions(); + return m_txdownloadman.GetOrphanageRef().GetOrphanTransactions(); } PeerManagerInfo PeerManagerImpl::GetInfo() const @@ -2160,6 +2072,9 @@ void PeerManagerImpl::BlockConnected( return; } LOCK(m_tx_download_mutex); + auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); + auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); + m_orphanage.EraseForBlock(*pblock); for (const auto& ptx : pblock->vtx) { @@ -2324,6 +2239,8 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside { AssertLockHeld(m_tx_download_mutex); + auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); + const uint256& hash = gtxid.GetHash(); if (gtxid.IsWtxid()) { @@ -3213,6 +3130,9 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx AssertLockHeld(g_msgproc_mutex); AssertLockHeld(m_tx_download_mutex); + auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); + auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); + LogDebug(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), @@ -3278,6 +3198,9 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c AssertLockHeld(g_msgproc_mutex); AssertLockHeld(m_tx_download_mutex); + auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); + auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); + // As this version of the transaction was acceptable, we can forget about any requests for it. // No-op if the tx is not in txrequest. m_txrequest.ForgetTxHash(tx->GetHash()); @@ -3363,6 +3286,7 @@ std::optional PeerManagerImpl::Find1P1CPacka AssertLockHeld(m_tx_download_mutex); const auto& parent_wtxid{ptx->GetWitnessHash()}; + auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); Assume(RecentRejectsReconsiderableFilter().contains(parent_wtxid.ToUint256())); @@ -3417,7 +3341,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) CTransactionRef porphanTx = nullptr; - while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id)) { + while (CTransactionRef porphanTx = m_txdownloadman.GetOrphanageRef().GetTxToReconsider(peer.m_id)) { const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; const Txid& orphanHash = porphanTx->GetHash(); @@ -4565,6 +4489,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LOCK2(cs_main, m_tx_download_mutex); + auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); + auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); + m_txrequest.ReceivedResponse(pfrom.GetId(), txid); if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid); @@ -5325,7 +5252,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (inv.IsGenTxMsg()) { // If we receive a NOTFOUND message for a tx we requested, mark the announcement for it as // completed in TxRequestTracker. - m_txrequest.ReceivedResponse(pfrom.GetId(), inv.hash); + m_txdownloadman.GetTxRequestRef().ReceivedResponse(pfrom.GetId(), inv.hash); } } } @@ -5447,7 +5374,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt // the extra work may not be noticed, possibly resulting in an // unnecessary 100ms delay) LOCK(m_tx_download_mutex); - if (m_orphanage.HaveTxToReconsider(peer->m_id)) fMoreWork = true; + if (m_txdownloadman.GetOrphanageRef().HaveTxToReconsider(peer->m_id)) fMoreWork = true; } catch (const std::exception& e) { LogDebug(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size, e.what(), typeid(e).name()); } catch (...) { @@ -6344,7 +6271,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) { LOCK(m_tx_download_mutex); std::vector> expired; - auto requestable = m_txrequest.GetRequestable(pto->GetId(), current_time, &expired); + auto requestable = m_txdownloadman.GetTxRequestRef().GetRequestable(pto->GetId(), current_time, &expired); for (const auto& entry : expired) { LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx", entry.second.GetHash().ToString(), entry.first); @@ -6360,11 +6287,11 @@ bool PeerManagerImpl::SendMessages(CNode* pto) MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); vGetData.clear(); } - m_txrequest.RequestedTx(pto->GetId(), gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL); + m_txdownloadman.GetTxRequestRef().RequestedTx(pto->GetId(), gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL); } else { // We have already seen this transaction, no need to download. This is just a belt-and-suspenders, as // this should already be called whenever a transaction becomes AlreadyHaveTx(). - m_txrequest.ForgetTxHash(gtxid.GetHash()); + m_txdownloadman.GetTxRequestRef().ForgetTxHash(gtxid.GetHash()); } } } // release m_tx_download_mutex diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h new file mode 100644 index 00000000000..d9c1b7c134b --- /dev/null +++ b/src/node/txdownloadman.h @@ -0,0 +1,53 @@ +// Copyright (c) 2024 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_NODE_TXDOWNLOADMAN_H +#define BITCOIN_NODE_TXDOWNLOADMAN_H + +#include +#include + +class TxOrphanage; +class TxRequestTracker; +class CRollingBloomFilter; + +namespace node { +class TxDownloadManagerImpl; + +/** + * Class responsible for deciding what transactions to request and, once + * downloaded, whether and how to validate them. It is also responsible for + * deciding what transaction packages to validate and how to resolve orphan + * transactions. Its data structures include TxRequestTracker for scheduling + * requests, rolling bloom filters for remembering transactions that have + * already been {accepted, rejected, confirmed}, an orphanage, and a registry of + * each peer's transaction relay-related information. + * + * Caller needs to interact with TxDownloadManager: + * - ValidationInterface callbacks. + * - When a potential transaction relay peer connects or disconnects. + * - When a transaction or package is accepted or rejected from mempool + * - When a inv, notfound, or tx message is received + * - To get instructions for which getdata messages to send + * + * This class is not thread-safe. Access must be synchronized using an + * external mutex. + */ +class TxDownloadManager { + const std::unique_ptr m_impl; + +public: + explicit TxDownloadManager(); + ~TxDownloadManager(); + + // Get references to internal data structures. Outside access to these data structures should be + // temporary and removed later once logic has been moved internally. + TxOrphanage& GetOrphanageRef(); + TxRequestTracker& GetTxRequestRef(); + CRollingBloomFilter& RecentRejectsFilter(); + CRollingBloomFilter& RecentRejectsReconsiderableFilter(); + CRollingBloomFilter& RecentConfirmedTransactionsFilter(); +}; +} // namespace node +#endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp new file mode 100644 index 00000000000..4c301221d19 --- /dev/null +++ b/src/node/txdownloadman_impl.cpp @@ -0,0 +1,34 @@ +// Copyright (c) 2024 +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include + +namespace node { +TxDownloadManager::TxDownloadManager() : + m_impl{std::make_unique()} +{} +TxDownloadManager::~TxDownloadManager() = default; + +TxOrphanage& TxDownloadManager::GetOrphanageRef() +{ + return m_impl->m_orphanage; +} +TxRequestTracker& TxDownloadManager::GetTxRequestRef() +{ + return m_impl->m_txrequest; +} +CRollingBloomFilter& TxDownloadManager::RecentRejectsFilter() +{ + return m_impl->RecentRejectsFilter(); +} +CRollingBloomFilter& TxDownloadManager::RecentRejectsReconsiderableFilter() +{ + return m_impl->RecentRejectsReconsiderableFilter(); +} +CRollingBloomFilter& TxDownloadManager::RecentConfirmedTransactionsFilter() +{ + return m_impl->RecentConfirmedTransactionsFilter(); +} +} // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h new file mode 100644 index 00000000000..00a91176c21 --- /dev/null +++ b/src/node/txdownloadman_impl.h @@ -0,0 +1,127 @@ +// Copyright (c) 2024 +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. +#ifndef BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H +#define BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H + +#include + +#include +#include +#include +#include + +namespace node { +class TxDownloadManagerImpl { +public: + /** Manages unvalidated tx data (orphan transactions for which we are downloading ancestors). */ + TxOrphanage m_orphanage; + /** Tracks candidates for requesting and downloading transaction data. */ + TxRequestTracker m_txrequest; + + /** + * Filter for transactions that were recently rejected by the mempool. + * These are not rerequested until the chain tip changes, at which point + * the entire filter is reset. + * + * Without this filter we'd be re-requesting txs from each of our peers, + * increasing bandwidth consumption considerably. For instance, with 100 + * peers, half of which relay a tx we don't accept, that might be a 50x + * bandwidth increase. A flooding attacker attempting to roll-over the + * filter using minimum-sized, 60byte, transactions might manage to send + * 1000/sec if we have fast peers, so we pick 120,000 to give our peers a + * two minute window to send invs to us. + * + * Decreasing the false positive rate is fairly cheap, so we pick one in a + * million to make it highly unlikely for users to have issues with this + * filter. + * + * We typically only add wtxids to this filter. For non-segwit + * transactions, the txid == wtxid, so this only prevents us from + * re-downloading non-segwit transactions when communicating with + * non-wtxidrelay peers -- which is important for avoiding malleation + * attacks that could otherwise interfere with transaction relay from + * non-wtxidrelay peers. For communicating with wtxidrelay peers, having + * the reject filter store wtxids is exactly what we want to avoid + * redownload of a rejected transaction. + * + * In cases where we can tell that a segwit transaction will fail + * validation no matter the witness, we may add the txid of such + * transaction to the filter as well. This can be helpful when + * communicating with txid-relay peers or if we were to otherwise fetch a + * transaction via txid (eg in our orphan handling). + * + * Memory used: 1.3 MB + */ + std::unique_ptr m_lazy_recent_rejects{nullptr}; + + CRollingBloomFilter& RecentRejectsFilter() + { + if (!m_lazy_recent_rejects) { + m_lazy_recent_rejects = std::make_unique(120'000, 0.000'001); + } + + return *m_lazy_recent_rejects; + } + + /** + * Filter for: + * (1) wtxids of transactions that were recently rejected by the mempool but are + * eligible for reconsideration if submitted with other transactions. + * (2) packages (see GetPackageHash) we have already rejected before and should not retry. + * + * Similar to m_lazy_recent_rejects, this filter is used to save bandwidth when e.g. all of our peers + * have larger mempools and thus lower minimum feerates than us. + * + * When a transaction's error is TxValidationResult::TX_RECONSIDERABLE (in a package or by + * itself), add its wtxid to this filter. When a package fails for any reason, add the combined + * hash to this filter. + * + * Upon receiving an announcement for a transaction, if it exists in this filter, do not + * download the txdata. When considering packages, if it exists in this filter, drop it. + * + * Reset this filter when the chain tip changes. + * + * Parameters are picked to be the same as m_lazy_recent_rejects, with the same rationale. + */ + std::unique_ptr m_lazy_recent_rejects_reconsiderable{nullptr}; + + CRollingBloomFilter& RecentRejectsReconsiderableFilter() + { + if (!m_lazy_recent_rejects_reconsiderable) { + m_lazy_recent_rejects_reconsiderable = std::make_unique(120'000, 0.000'001); + } + + return *m_lazy_recent_rejects_reconsiderable; + } + + /* + * Filter for transactions that have been recently confirmed. + * We use this to avoid requesting transactions that have already been + * confirnmed. + * + * Blocks don't typically have more than 4000 transactions, so this should + * be at least six blocks (~1 hr) worth of transactions that we can store, + * inserting both a txid and wtxid for every observed transaction. + * If the number of transactions appearing in a block goes up, or if we are + * seeing getdata requests more than an hour after initial announcement, we + * can increase this number. + * The false positive rate of 1/1M should come out to less than 1 + * transaction per day that would be inadvertently ignored (which is the + * same probability that we have in the reject filter). + */ + std::unique_ptr m_lazy_recent_confirmed_transactions{nullptr}; + + CRollingBloomFilter& RecentConfirmedTransactionsFilter() + { + if (!m_lazy_recent_confirmed_transactions) { + m_lazy_recent_confirmed_transactions = std::make_unique(48'000, 0.000'001); + } + + return *m_lazy_recent_confirmed_transactions; + } + + TxDownloadManagerImpl() = default; +}; +} // namespace node +#endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From f6c860efb1221e1eadc3acebd6b0b885b9cc291a Mon Sep 17 00:00:00 2001 From: glozow Date: Sun, 29 Sep 2024 10:47:43 -0400 Subject: [PATCH 02/29] [doc] fix typo in m_lazy_recent_confirmed_transactions doc --- src/node/txdownloadman_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 00a91176c21..aee895dbc9f 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -98,7 +98,7 @@ class TxDownloadManagerImpl { /* * Filter for transactions that have been recently confirmed. * We use this to avoid requesting transactions that have already been - * confirnmed. + * confirmed. * * Blocks don't typically have more than 4000 transactions, so this should * be at least six blocks (~1 hr) worth of transactions that we can store, From af918349de52e654927d50279de64f548a8b53d6 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 15:31:35 +0100 Subject: [PATCH 03/29] [refactor] move ValidationInterface functions to TxDownloadManager This is move-only. --- src/net_processing.cpp | 27 ++--------------- src/node/txdownloadman.h | 9 ++++-- src/node/txdownloadman_impl.cpp | 52 +++++++++++++++++++++++++++++++++ src/node/txdownloadman_impl.h | 5 ++++ 4 files changed, 67 insertions(+), 26 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9554b080cc2..665f6438b80 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2036,8 +2036,7 @@ void PeerManagerImpl::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd) // If the chain tip has changed, previously rejected transactions might now be valid, e.g. due // to a timelock. Reset the rejection filters to give those transactions another chance if we // see them again. - RecentRejectsFilter().reset(); - RecentRejectsReconsiderableFilter().reset(); + m_txdownloadman.ActiveTipChange(); } } @@ -2072,33 +2071,13 @@ void PeerManagerImpl::BlockConnected( return; } LOCK(m_tx_download_mutex); - auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); - auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); - - m_orphanage.EraseForBlock(*pblock); - - for (const auto& ptx : pblock->vtx) { - RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256()); - if (ptx->HasWitness()) { - RecentConfirmedTransactionsFilter().insert(ptx->GetWitnessHash().ToUint256()); - } - m_txrequest.ForgetTxHash(ptx->GetHash()); - m_txrequest.ForgetTxHash(ptx->GetWitnessHash()); - } + m_txdownloadman.BlockConnected(pblock); } void PeerManagerImpl::BlockDisconnected(const std::shared_ptr &block, const CBlockIndex* pindex) { - // To avoid relay problems with transactions that were previously - // confirmed, clear our filter of recently confirmed transactions whenever - // there's a reorg. - // This means that in a 1-block reorg (where 1 block is disconnected and - // then another block reconnected), our filter will drop to having only one - // block's worth of transactions in it, but that should be fine, since - // presumably the most common case of relaying a confirmed transaction - // should be just after a new block containing it is found. LOCK(m_tx_download_mutex); - RecentConfirmedTransactionsFilter().reset(); + m_txdownloadman.BlockDisconnected(); } /** diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index d9c1b7c134b..3e010cda643 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -8,10 +8,10 @@ #include #include +class CBlock; +class CRollingBloomFilter; class TxOrphanage; class TxRequestTracker; -class CRollingBloomFilter; - namespace node { class TxDownloadManagerImpl; @@ -48,6 +48,11 @@ class TxDownloadManager { CRollingBloomFilter& RecentRejectsFilter(); CRollingBloomFilter& RecentRejectsReconsiderableFilter(); CRollingBloomFilter& RecentConfirmedTransactionsFilter(); + + // Responses to chain events. TxDownloadManager is not an actual client of ValidationInterface, these are called through PeerManager. + void ActiveTipChange(); + void BlockConnected(const std::shared_ptr& pblock); + void BlockDisconnected(); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 4c301221d19..f0a1256c4fe 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -5,7 +5,13 @@ #include #include +#include +#include +#include +#include + namespace node { +// TxDownloadManager wrappers TxDownloadManager::TxDownloadManager() : m_impl{std::make_unique()} {} @@ -31,4 +37,50 @@ CRollingBloomFilter& TxDownloadManager::RecentConfirmedTransactionsFilter() { return m_impl->RecentConfirmedTransactionsFilter(); } +void TxDownloadManager::ActiveTipChange() +{ + m_impl->ActiveTipChange(); +} +void TxDownloadManager::BlockConnected(const std::shared_ptr& pblock) +{ + m_impl->BlockConnected(pblock); +} +void TxDownloadManager::BlockDisconnected() +{ + m_impl->BlockDisconnected(); +} + +// TxDownloadManagerImpl +void TxDownloadManagerImpl::ActiveTipChange() +{ + RecentRejectsFilter().reset(); + RecentRejectsReconsiderableFilter().reset(); +} + +void TxDownloadManagerImpl::BlockConnected(const std::shared_ptr& pblock) +{ + m_orphanage.EraseForBlock(*pblock); + + for (const auto& ptx : pblock->vtx) { + RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256()); + if (ptx->HasWitness()) { + RecentConfirmedTransactionsFilter().insert(ptx->GetWitnessHash().ToUint256()); + } + m_txrequest.ForgetTxHash(ptx->GetHash()); + m_txrequest.ForgetTxHash(ptx->GetWitnessHash()); + } +} + +void TxDownloadManagerImpl::BlockDisconnected() +{ + // To avoid relay problems with transactions that were previously + // confirmed, clear our filter of recently confirmed transactions whenever + // there's a reorg. + // This means that in a 1-block reorg (where 1 block is disconnected and + // then another block reconnected), our filter will drop to having only one + // block's worth of transactions in it, but that should be fine, since + // presumably the most common case of relaying a confirmed transaction + // should be just after a new block containing it is found. + RecentConfirmedTransactionsFilter().reset(); +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index aee895dbc9f..d55d218c917 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -122,6 +123,10 @@ class TxDownloadManagerImpl { } TxDownloadManagerImpl() = default; + + void ActiveTipChange(); + void BlockConnected(const std::shared_ptr& pblock); + void BlockDisconnected(); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From 84e4ef843db3443278d6eb70ff89fa254fcc6631 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 17:04:00 +0100 Subject: [PATCH 04/29] [txdownload] add read-only reference to mempool This will become necessary in later commits that query mempool. We also introduce the TxDownloadOptions in this commit to make the later diff easier to review. --- src/net_processing.cpp | 1 + src/node/txdownloadman.h | 8 +++++++- src/node/txdownloadman_impl.cpp | 5 +++-- src/node/txdownloadman_impl.h | 5 ++++- 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 665f6438b80..fb903d061cc 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2000,6 +2000,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, m_banman(banman), m_chainman(chainman), m_mempool(pool), + m_txdownloadman(node::TxDownloadOptions{pool}), m_warnings{warnings}, m_opts{opts} { diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 3e010cda643..b305b91616b 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -10,11 +10,17 @@ class CBlock; class CRollingBloomFilter; +class CTxMemPool; class TxOrphanage; class TxRequestTracker; namespace node { class TxDownloadManagerImpl; +struct TxDownloadOptions { + /** Read-only reference to mempool. */ + const CTxMemPool& m_mempool; +}; + /** * Class responsible for deciding what transactions to request and, once * downloaded, whether and how to validate them. It is also responsible for @@ -38,7 +44,7 @@ class TxDownloadManager { const std::unique_ptr m_impl; public: - explicit TxDownloadManager(); + explicit TxDownloadManager(const TxDownloadOptions& options); ~TxDownloadManager(); // Get references to internal data structures. Outside access to these data structures should be diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index f0a1256c4fe..984e6cf04c8 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -7,13 +7,14 @@ #include #include +#include #include #include namespace node { // TxDownloadManager wrappers -TxDownloadManager::TxDownloadManager() : - m_impl{std::make_unique()} +TxDownloadManager::TxDownloadManager(const TxDownloadOptions& options) : + m_impl{std::make_unique(options)} {} TxDownloadManager::~TxDownloadManager() = default; diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index d55d218c917..df7824cdb46 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -12,9 +12,12 @@ #include #include +class CTxMemPool; namespace node { class TxDownloadManagerImpl { public: + TxDownloadOptions m_opts; + /** Manages unvalidated tx data (orphan transactions for which we are downloading ancestors). */ TxOrphanage m_orphanage; /** Tracks candidates for requesting and downloading transaction data. */ @@ -122,7 +125,7 @@ class TxDownloadManagerImpl { return *m_lazy_recent_confirmed_transactions; } - TxDownloadManagerImpl() = default; + TxDownloadManagerImpl(const TxDownloadOptions& options) : m_opts{options} {} void ActiveTipChange(); void BlockConnected(const std::shared_ptr& pblock); From f61d9e4b4b80842d520c490a1012044c0816679a Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 17:06:56 +0100 Subject: [PATCH 05/29] [refactor] move AlreadyHaveTx to TxDownload This is move-only. Also delete external RecentConfirmedTransactionsFilter() access since it is no longer necessary. --- src/net_processing.cpp | 59 +++------------------------------ src/node/txdownloadman.h | 11 +++++- src/node/txdownloadman_impl.cpp | 37 ++++++++++++++++++--- src/node/txdownloadman_impl.h | 3 ++ 4 files changed, 50 insertions(+), 60 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fb903d061cc..13f9c8a2cc2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -854,17 +854,6 @@ class PeerManagerImpl final : public PeerManager /** Stalling timeout for blocks in IBD */ std::atomic m_block_stalling_timeout{BLOCK_STALLING_TIMEOUT_DEFAULT}; - /** Check whether we already have this gtxid in: - * - mempool - * - orphanage - * - m_lazy_recent_rejects - * - m_lazy_recent_rejects_reconsiderable (if include_reconsiderable = true) - * - m_lazy_recent_confirmed_transactions - * */ - bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) - EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex); - - CRollingBloomFilter& RecentRejectsFilter() EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex) { AssertLockHeld(m_tx_download_mutex); @@ -877,12 +866,6 @@ class PeerManagerImpl final : public PeerManager return m_txdownloadman.RecentRejectsReconsiderableFilter(); } - CRollingBloomFilter& RecentConfirmedTransactionsFilter() EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex) - { - AssertLockHeld(m_tx_download_mutex); - return m_txdownloadman.RecentConfirmedTransactionsFilter(); - } - /** * 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 @@ -2214,40 +2197,6 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta // Messages // - -bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) -{ - AssertLockHeld(m_tx_download_mutex); - - auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); - - const uint256& hash = gtxid.GetHash(); - - if (gtxid.IsWtxid()) { - // Normal query by wtxid. - if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; - } else { - // Never query by txid: it is possible that the transaction in the orphanage has the same - // txid but a different witness, which would give us a false positive result. If we decided - // not to request the transaction based on this result, an attacker could prevent us from - // downloading a transaction by intentionally creating a malleated version of it. While - // only one (or none!) of these transactions can ultimately be confirmed, we have no way of - // discerning which one that is, so the orphanage can store multiple transactions with the - // same txid. - // - // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid. - // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will - // help us find non-segwit transactions, saving bandwidth, and should have no false positives. - if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; - } - - if (include_reconsiderable && RecentRejectsReconsiderableFilter().contains(hash)) return true; - - if (RecentConfirmedTransactionsFilter().contains(hash)) return true; - - return RecentRejectsFilter().contains(hash) || m_mempool.exists(gtxid); -} - bool PeerManagerImpl::AlreadyHaveBlock(const uint256& block_hash) { return m_chainman.m_blockman.LookupBlockIndex(block_hash) != nullptr; @@ -4172,7 +4121,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } const GenTxid gtxid = ToGenTxid(inv); - const bool fAlreadyHave = AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true); + const bool fAlreadyHave = m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true); LogDebug(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); AddKnownTx(*peer, inv.hash); @@ -4487,7 +4436,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // 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 (m_txdownloadman.AlreadyHaveTx(GenTxid::Wtxid(wtxid), /*include_reconsiderable=*/true)) { if (pfrom.HasPermission(NetPermissionFlags::ForceRelay)) { // Always relay transactions received from peers with forcerelay // permission, even if they were already in the mempool, allowing @@ -4586,7 +4535,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, AddKnownTx(*peer, parent_txid); // Exclude m_lazy_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_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) AddTxAnnouncement(pfrom, gtxid, current_time); } if (m_orphanage.AddTx(ptx, pfrom.GetId())) { @@ -6259,7 +6208,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) for (const GenTxid& gtxid : requestable) { // Exclude m_lazy_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)) { + if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { LogDebug(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()); diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index b305b91616b..f573bc54d4b 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -11,6 +11,7 @@ class CBlock; class CRollingBloomFilter; class CTxMemPool; +class GenTxid; class TxOrphanage; class TxRequestTracker; namespace node { @@ -53,12 +54,20 @@ class TxDownloadManager { TxRequestTracker& GetTxRequestRef(); CRollingBloomFilter& RecentRejectsFilter(); CRollingBloomFilter& RecentRejectsReconsiderableFilter(); - CRollingBloomFilter& RecentConfirmedTransactionsFilter(); // Responses to chain events. TxDownloadManager is not an actual client of ValidationInterface, these are called through PeerManager. void ActiveTipChange(); void BlockConnected(const std::shared_ptr& pblock); void BlockDisconnected(); + + /** Check whether we already have this gtxid in: + * - mempool + * - orphanage + * - m_recent_rejects + * - m_recent_rejects_reconsiderable (if include_reconsiderable = true) + * - m_recent_confirmed_transactions + * */ + bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 984e6cf04c8..6d417ac65b6 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -34,10 +34,6 @@ CRollingBloomFilter& TxDownloadManager::RecentRejectsReconsiderableFilter() { return m_impl->RecentRejectsReconsiderableFilter(); } -CRollingBloomFilter& TxDownloadManager::RecentConfirmedTransactionsFilter() -{ - return m_impl->RecentConfirmedTransactionsFilter(); -} void TxDownloadManager::ActiveTipChange() { m_impl->ActiveTipChange(); @@ -50,6 +46,10 @@ void TxDownloadManager::BlockDisconnected() { m_impl->BlockDisconnected(); } +bool TxDownloadManager::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) +{ + return m_impl->AlreadyHaveTx(gtxid, include_reconsiderable); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -84,4 +84,33 @@ void TxDownloadManagerImpl::BlockDisconnected() // should be just after a new block containing it is found. RecentConfirmedTransactionsFilter().reset(); } + +bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) +{ + const uint256& hash = gtxid.GetHash(); + + if (gtxid.IsWtxid()) { + // Normal query by wtxid. + if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; + } else { + // Never query by txid: it is possible that the transaction in the orphanage has the same + // txid but a different witness, which would give us a false positive result. If we decided + // not to request the transaction based on this result, an attacker could prevent us from + // downloading a transaction by intentionally creating a malleated version of it. While + // only one (or none!) of these transactions can ultimately be confirmed, we have no way of + // discerning which one that is, so the orphanage can store multiple transactions with the + // same txid. + // + // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid. + // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will + // help us find non-segwit transactions, saving bandwidth, and should have no false positives. + if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; + } + + if (include_reconsiderable && RecentRejectsReconsiderableFilter().contains(hash)) return true; + + if (RecentConfirmedTransactionsFilter().contains(hash)) return true; + + return RecentRejectsFilter().contains(hash) || m_opts.m_mempool.exists(gtxid); +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index df7824cdb46..ad046bef9e7 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -130,6 +131,8 @@ class TxDownloadManagerImpl { void ActiveTipChange(); void BlockConnected(const std::shared_ptr& pblock); void BlockDisconnected(); + + bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From f48d36cd97e9b27dfa105c35e0fe67cba47056d1 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 16:46:36 +0100 Subject: [PATCH 06/29] [refactor] move peer (dis)connection logic to TxDownload The information stored in TxDownloadConnectionInfo isn't used until the next commit. --- src/net_processing.cpp | 13 +++++++++++-- src/node/txdownloadman.h | 16 ++++++++++++++++ src/node/txdownloadman_impl.cpp | 29 +++++++++++++++++++++++++++++ src/node/txdownloadman_impl.h | 17 +++++++++++++++++ 4 files changed, 73 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 13f9c8a2cc2..a0f0ca10f95 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1686,8 +1686,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) } { LOCK(m_tx_download_mutex); - m_txdownloadman.GetOrphanageRef().EraseForPeer(nodeid); - m_txdownloadman.GetTxRequestRef().DisconnectedPeer(nodeid); + m_txdownloadman.DisconnectedPeer(nodeid); } if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid); m_num_preferred_download_peers -= state->fPreferredDownload; @@ -3852,6 +3851,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, tx_relay->m_next_inv_send_time == 0s)); } + { + LOCK2(::cs_main, m_tx_download_mutex); + const CNodeState* state = State(pfrom.GetId()); + m_txdownloadman.ConnectedPeer(pfrom.GetId(), node::TxDownloadConnectionInfo { + .m_preferred = state->fPreferredDownload, + .m_relay_permissions = pfrom.HasPermission(NetPermissionFlags::Relay), + .m_wtxid_relay = peer->m_wtxid_relay, + }); + } + pfrom.fSuccessfullyConnected = true; return; } diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index f573bc54d4b..93c35b4d24a 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_NODE_TXDOWNLOADMAN_H #define BITCOIN_NODE_TXDOWNLOADMAN_H +#include + #include #include @@ -21,6 +23,14 @@ struct TxDownloadOptions { /** Read-only reference to mempool. */ const CTxMemPool& m_mempool; }; +struct TxDownloadConnectionInfo { + /** Whether this peer is preferred for transaction download. */ + const bool m_preferred; + /** Whether this peer has Relay permissions. */ + const bool m_relay_permissions; + /** Whether this peer supports wtxid relay. */ + const bool m_wtxid_relay; +}; /** * Class responsible for deciding what transactions to request and, once @@ -68,6 +78,12 @@ class TxDownloadManager { * - m_recent_confirmed_transactions * */ bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable); + + /** Creates a new PeerInfo. Saves the connection info to calculate tx announcement delays later. */ + void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info); + + /** Deletes all txrequest announcements and orphans for a given peer. */ + void DisconnectedPeer(NodeId nodeid); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 6d417ac65b6..6a00e1c70f2 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -50,6 +50,14 @@ bool TxDownloadManager::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsi { return m_impl->AlreadyHaveTx(gtxid, include_reconsiderable); } +void TxDownloadManager::ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info) +{ + m_impl->ConnectedPeer(nodeid, info); +} +void TxDownloadManager::DisconnectedPeer(NodeId nodeid) +{ + m_impl->DisconnectedPeer(nodeid); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -113,4 +121,25 @@ bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_rec return RecentRejectsFilter().contains(hash) || m_opts.m_mempool.exists(gtxid); } + +void TxDownloadManagerImpl::ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info) +{ + // If already connected (shouldn't happen in practice), exit early. + if (m_peer_info.contains(nodeid)) return; + + m_peer_info.try_emplace(nodeid, info); + if (info.m_wtxid_relay) m_num_wtxid_peers += 1; +} + +void TxDownloadManagerImpl::DisconnectedPeer(NodeId nodeid) +{ + m_orphanage.EraseForPeer(nodeid); + m_txrequest.DisconnectedPeer(nodeid); + + if (auto it = m_peer_info.find(nodeid); it != m_peer_info.end()) { + if (it->second.m_connection_info.m_wtxid_relay) m_num_wtxid_peers -= 1; + m_peer_info.erase(it); + } + +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index ad046bef9e7..fc57f690240 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -128,11 +128,28 @@ class TxDownloadManagerImpl { TxDownloadManagerImpl(const TxDownloadOptions& options) : m_opts{options} {} + struct PeerInfo { + /** Information relevant to scheduling tx requests. */ + const TxDownloadConnectionInfo m_connection_info; + + PeerInfo(const TxDownloadConnectionInfo& info) : m_connection_info{info} {} + }; + + /** Information for all of the peers we may download transactions from. This is not necessarily + * all peers we are connected to (no block-relay-only and temporary connections). */ + std::map m_peer_info; + + /** Number of wtxid relay peers we have in m_peer_info. */ + uint32_t m_num_wtxid_peers{0}; + void ActiveTipChange(); void BlockConnected(const std::shared_ptr& pblock); void BlockDisconnected(); bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable); + + void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info); + void DisconnectedPeer(NodeId nodeid); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From 288865338f50d5b00758236aa4a59546a41c88c1 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 21 Aug 2024 11:29:24 +0100 Subject: [PATCH 07/29] [refactor] rename maybe_add_extra_compact_tx to first_time_failure The usage of this bool will increase in scope in the next commit. For this commit, the value of this bool is accurate at each ProcessInvalidTx callsite: - ProcessOrphanTx -> this tx is an orphan i.e. has been rejected before - ProcessPackageResult -> 1p1c only, each transaction is either an orphan or in m_lazy_recent_rejects_reconsiderable - ProcessMessage -> tx was received over p2p and validated for the first time --- src/net_processing.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a0f0ca10f95..8013b76bca8 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -581,12 +581,12 @@ class PeerManagerImpl final : public PeerManager bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer); /** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID. - * @param[in] maybe_add_extra_compact_tx Whether this tx should be added to vExtraTxnForCompact. + * @param[in] first_time_failure Whether this tx should be added to vExtraTxnForCompact. * Set to false if the tx has already been rejected before, * e.g. is an orphan, to avoid adding duplicate entries. * Updates m_txrequest, m_lazy_recent_rejects, m_lazy_recent_rejects_reconsiderable, m_orphanage, and vExtraTxnForCompact. */ void ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result, - bool maybe_add_extra_compact_tx) + 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. @@ -3052,7 +3052,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, } void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx, const TxValidationState& state, - bool maybe_add_extra_compact_tx) + bool first_time_failure) { AssertLockNotHeld(m_peer_mutex); AssertLockHeld(g_msgproc_mutex); @@ -3106,7 +3106,7 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx RecentRejectsFilter().insert(ptx->GetHash().ToUint256()); m_txrequest.ForgetTxHash(ptx->GetHash()); } - if (maybe_add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) { + if (first_time_failure && RecursiveDynamicUsage(*ptx) < 100000) { AddToCompactExtraTransactions(ptx); } } @@ -3191,7 +3191,7 @@ void PeerManagerImpl::ProcessPackageResult(const PackageToValidate& package_to_v // 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); + ProcessInvalidTx(nodeid, tx, tx_result.m_state, /*first_time_failure=*/false); break; } case MempoolAcceptResult::ResultType::MEMPOOL_ENTRY: @@ -3290,7 +3290,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) state.GetResult() != TxValidationResult::TX_UNKNOWN && state.GetResult() != TxValidationResult::TX_NO_MEMPOOL && state.GetResult() != TxValidationResult::TX_RESULT_UNSET)) { - ProcessInvalidTx(peer.m_id, porphanTx, state, /*maybe_add_extra_compact_tx=*/false); + ProcessInvalidTx(peer.m_id, porphanTx, state, /*first_time_failure=*/false); } return true; } @@ -4574,7 +4574,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } } if (state.IsInvalid()) { - ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true); + ProcessInvalidTx(pfrom.GetId(), ptx, state, /*first_time_failure=*/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. From 58e09f244b4bf07d31bc8dd4e939c2dc4dc74f3a Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 14 Aug 2024 10:33:21 +0100 Subject: [PATCH 08/29] [p2p] don't log tx invs when in IBD These invs are ignored anyway, and this allows us to more easily move the inv handling to TxDownloadManager in the next commit. --- src/net_processing.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8013b76bca8..1ba1e853e71 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4130,12 +4130,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } const GenTxid gtxid = ToGenTxid(inv); - const bool fAlreadyHave = m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true); - LogDebug(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); - AddKnownTx(*peer, inv.hash); - if (!fAlreadyHave && !m_chainman.IsInitialBlockDownload()) { - AddTxAnnouncement(pfrom, gtxid, current_time); + + if (!m_chainman.IsInitialBlockDownload()) { + const bool fAlreadyHave = m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true); + LogDebug(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); + if (!fAlreadyHave) { + AddTxAnnouncement(pfrom, gtxid, current_time); + } } } else { LogDebug(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId()); From 042a97ce7fc672021cdb1dee62a550ef19c208fb Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 25 Apr 2024 11:43:04 +0100 Subject: [PATCH 09/29] [refactor] move tx inv/getdata handling to txdownload --- src/net_processing.cpp | 94 +++++---------------------------- src/node/txdownloadman.h | 24 +++++++++ src/node/txdownloadman_impl.cpp | 63 ++++++++++++++++++++++ src/node/txdownloadman_impl.h | 6 +++ 4 files changed, 105 insertions(+), 82 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1ba1e853e71..71162e957db 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -89,22 +89,6 @@ static constexpr auto PING_INTERVAL{2min}; static const unsigned int MAX_LOCATOR_SZ = 101; /** The maximum number of entries in an 'inv' protocol message */ static const unsigned int MAX_INV_SZ = 50000; -/** Maximum number of in-flight transaction requests from a peer. It is not a hard limit, but the threshold at which - * point the OVERLOADED_PEER_TX_DELAY kicks in. */ -static constexpr int32_t MAX_PEER_TX_REQUEST_IN_FLIGHT = 100; -/** Maximum number of transactions to consider for requesting, per peer. It provides a reasonable DoS limit to - * per-peer memory usage spent on announcements, while covering peers continuously sending INVs at the maximum - * rate (by our own policy, see INVENTORY_BROADCAST_PER_SECOND) for several minutes, while not receiving - * the actual transaction (from any peer) in response to requests for them. */ -static constexpr int32_t MAX_PEER_TX_ANNOUNCEMENTS = 5000; -/** How long to delay requesting transactions via txids, if we have wtxid-relaying peers */ -static constexpr auto TXID_RELAY_DELAY{2s}; -/** How long to delay requesting transactions from non-preferred peers */ -static constexpr auto NONPREF_PEER_TX_DELAY{2s}; -/** How long to delay requesting transactions from overloaded peers (see MAX_PEER_TX_REQUEST_IN_FLIGHT). */ -static constexpr auto OVERLOADED_PEER_TX_DELAY{2s}; -/** How long to wait before downloading a transaction from an additional peer */ -static constexpr auto GETDATA_TX_INTERVAL{60s}; /** Limit to avoid sending big packets. Not used in processing incoming GETDATA for compatibility */ static const unsigned int MAX_GETDATA_SZ = 1000; /** Number of blocks that can be requested at any given time from a single peer. */ @@ -156,7 +140,7 @@ static constexpr unsigned int INVENTORY_BROADCAST_TARGET = INVENTORY_BROADCAST_P /** Maximum number of inventory items to send per transmission. */ static constexpr unsigned int INVENTORY_BROADCAST_MAX = 1000; static_assert(INVENTORY_BROADCAST_MAX >= INVENTORY_BROADCAST_TARGET, "INVENTORY_BROADCAST_MAX too low"); -static_assert(INVENTORY_BROADCAST_MAX <= MAX_PEER_TX_ANNOUNCEMENTS, "INVENTORY_BROADCAST_MAX too high"); +static_assert(INVENTORY_BROADCAST_MAX <= node::MAX_PEER_TX_ANNOUNCEMENTS, "INVENTORY_BROADCAST_MAX too high"); /** Average delay between feefilter broadcasts in seconds. */ static constexpr auto AVG_FEEFILTER_BROADCAST_INTERVAL{10min}; /** Maximum feefilter broadcast delay after significant change. */ @@ -720,12 +704,6 @@ class PeerManagerImpl final : public PeerManager void SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req); - /** Register with TxRequestTracker that an INV has been received from a - * peer. The announcement parameters are decided in PeerManager and then - * passed to TxRequestTracker. */ - void AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std::chrono::microseconds current_time) - EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_tx_download_mutex); - /** Send a message to a peer */ void PushMessage(CNode& node, CSerializedNetMsg&& msg) const { m_connman.PushMessage(&node, std::move(msg)); } template @@ -1571,36 +1549,6 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) } } -void PeerManagerImpl::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std::chrono::microseconds current_time) -{ - AssertLockHeld(::cs_main); // for State - AssertLockHeld(m_tx_download_mutex); // For m_txrequest - NodeId nodeid = node.GetId(); - - auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); - if (!node.HasPermission(NetPermissionFlags::Relay) && m_txrequest.Count(nodeid) >= MAX_PEER_TX_ANNOUNCEMENTS) { - // Too many queued announcements from this peer - return; - } - const CNodeState* state = State(nodeid); - - // Decide the TxRequestTracker parameters for this announcement: - // - "preferred": if fPreferredDownload is set (= outbound, or NetPermissionFlags::NoBan permission) - // - "reqtime": current time plus delays for: - // - NONPREF_PEER_TX_DELAY for announcements from non-preferred connections - // - TXID_RELAY_DELAY for txid announcements while wtxid peers are available - // - OVERLOADED_PEER_TX_DELAY for announcements from peers which have at least - // MAX_PEER_TX_REQUEST_IN_FLIGHT requests in flight (and don't have NetPermissionFlags::Relay). - auto delay{0us}; - const bool preferred = state->fPreferredDownload; - if (!preferred) delay += NONPREF_PEER_TX_DELAY; - if (!gtxid.IsWtxid() && m_wtxid_relay_peers > 0) delay += TXID_RELAY_DELAY; - const bool overloaded = !node.HasPermission(NetPermissionFlags::Relay) && - m_txrequest.CountInFlight(nodeid) >= MAX_PEER_TX_REQUEST_IN_FLIGHT; - if (overloaded) delay += OVERLOADED_PEER_TX_DELAY; - m_txrequest.ReceivedInv(nodeid, gtxid, preferred, current_time + delay); -} - void PeerManagerImpl::UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) { LOCK(cs_main); @@ -4133,11 +4081,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, AddKnownTx(*peer, inv.hash); if (!m_chainman.IsInitialBlockDownload()) { - const bool fAlreadyHave = m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true); + const bool fAlreadyHave{m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time, /*p2p_inv=*/true)}; LogDebug(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); - if (!fAlreadyHave) { - AddTxAnnouncement(pfrom, gtxid, current_time); - } } } else { LogDebug(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId()); @@ -4546,7 +4491,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, AddKnownTx(*peer, parent_txid); // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been // previously rejected for being too low feerate. This orphan might CPFP it. - if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) AddTxAnnouncement(pfrom, gtxid, current_time); + if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { + m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time, /*p2p_inv=*/false); + } } if (m_orphanage.AddTx(ptx, pfrom.GetId())) { @@ -5186,7 +5133,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (msg_type == NetMsgType::NOTFOUND) { std::vector vInv; vRecv >> vInv; - if (vInv.size() <= MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) { + if (vInv.size() <= node::MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) { LOCK(m_tx_download_mutex); for (CInv &inv : vInv) { if (inv.IsGenTxMsg()) { @@ -6210,31 +6157,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // { LOCK(m_tx_download_mutex); - std::vector> expired; - auto requestable = m_txdownloadman.GetTxRequestRef().GetRequestable(pto->GetId(), current_time, &expired); - for (const auto& entry : expired) { - LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx", - entry.second.GetHash().ToString(), entry.first); - } - for (const GenTxid& gtxid : requestable) { - // Exclude m_lazy_recent_rejects_reconsiderable: we may be requesting a missing parent - // that was previously rejected for being too low feerate. - if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { - LogDebug(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()); - if (vGetData.size() >= MAX_GETDATA_SZ) { - MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); - vGetData.clear(); - } - m_txdownloadman.GetTxRequestRef().RequestedTx(pto->GetId(), gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL); - } else { - // We have already seen this transaction, no need to download. This is just a belt-and-suspenders, as - // this should already be called whenever a transaction becomes AlreadyHaveTx(). - m_txdownloadman.GetTxRequestRef().ForgetTxHash(gtxid.GetHash()); + for (const GenTxid& gtxid : m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time)) { + vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.GetHash()); + if (vGetData.size() >= MAX_GETDATA_SZ) { + MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); + vGetData.clear(); } } - } // release m_tx_download_mutex + } if (!vGetData.empty()) MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 93c35b4d24a..ce0c9594281 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -19,6 +19,22 @@ class TxRequestTracker; namespace node { class TxDownloadManagerImpl; +/** Maximum number of in-flight transaction requests from a peer. It is not a hard limit, but the threshold at which + * point the OVERLOADED_PEER_TX_DELAY kicks in. */ +static constexpr int32_t MAX_PEER_TX_REQUEST_IN_FLIGHT = 100; +/** Maximum number of transactions to consider for requesting, per peer. It provides a reasonable DoS limit to + * per-peer memory usage spent on announcements, while covering peers continuously sending INVs at the maximum + * rate (by our own policy, see INVENTORY_BROADCAST_PER_SECOND) for several minutes, while not receiving + * the actual transaction (from any peer) in response to requests for them. */ +static constexpr int32_t MAX_PEER_TX_ANNOUNCEMENTS = 5000; +/** How long to delay requesting transactions via txids, if we have wtxid-relaying peers */ +static constexpr auto TXID_RELAY_DELAY{2s}; +/** How long to delay requesting transactions from non-preferred peers */ +static constexpr auto NONPREF_PEER_TX_DELAY{2s}; +/** How long to delay requesting transactions from overloaded peers (see MAX_PEER_TX_REQUEST_IN_FLIGHT). */ +static constexpr auto OVERLOADED_PEER_TX_DELAY{2s}; +/** How long to wait before downloading a transaction from an additional peer */ +static constexpr auto GETDATA_TX_INTERVAL{60s}; struct TxDownloadOptions { /** Read-only reference to mempool. */ const CTxMemPool& m_mempool; @@ -84,6 +100,14 @@ class TxDownloadManager { /** Deletes all txrequest announcements and orphans for a given peer. */ void DisconnectedPeer(NodeId nodeid); + + /** New inv has been received. May be added as a candidate to txrequest. + * @param[in] p2p_inv When true, only add this announcement if we don't already have the tx. + * Returns true if this was a dropped inv (p2p_inv=true and we already have the tx), false otherwise. */ + bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv); + + /** Get getdata requests to send. */ + std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 6a00e1c70f2..d0125f69ec0 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -58,6 +59,14 @@ void TxDownloadManager::DisconnectedPeer(NodeId nodeid) { m_impl->DisconnectedPeer(nodeid); } +bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv) +{ + return m_impl->AddTxAnnouncement(peer, gtxid, now, p2p_inv); +} +std::vector TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) +{ + return m_impl->GetRequestsToSend(nodeid, current_time); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -142,4 +151,58 @@ void TxDownloadManagerImpl::DisconnectedPeer(NodeId nodeid) } } + +bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv) +{ + // If this is an inv received from a peer and we already have it, we can drop it. + if (p2p_inv && AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) return true; + + auto it = m_peer_info.find(peer); + if (it == m_peer_info.end()) return false; + const auto& info = it->second.m_connection_info; + if (!info.m_relay_permissions && m_txrequest.Count(peer) >= MAX_PEER_TX_ANNOUNCEMENTS) { + // Too many queued announcements for this peer + return false; + } + // Decide the TxRequestTracker parameters for this announcement: + // - "preferred": if fPreferredDownload is set (= outbound, or NetPermissionFlags::NoBan permission) + // - "reqtime": current time plus delays for: + // - NONPREF_PEER_TX_DELAY for announcements from non-preferred connections + // - TXID_RELAY_DELAY for txid announcements while wtxid peers are available + // - OVERLOADED_PEER_TX_DELAY for announcements from peers which have at least + // MAX_PEER_TX_REQUEST_IN_FLIGHT requests in flight (and don't have NetPermissionFlags::Relay). + auto delay{0us}; + if (!info.m_preferred) delay += NONPREF_PEER_TX_DELAY; + if (!gtxid.IsWtxid() && m_num_wtxid_peers > 0) delay += TXID_RELAY_DELAY; + const bool overloaded = !info.m_relay_permissions && m_txrequest.CountInFlight(peer) >= MAX_PEER_TX_REQUEST_IN_FLIGHT; + if (overloaded) delay += OVERLOADED_PEER_TX_DELAY; + + m_txrequest.ReceivedInv(peer, gtxid, info.m_preferred, now + delay); + + return false; +} + +std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) +{ + std::vector requests; + std::vector> expired; + auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired); + for (const auto& entry : expired) { + LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx", + entry.second.GetHash().ToString(), entry.first); + } + for (const GenTxid& gtxid : requestable) { + if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { + LogDebug(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx", + gtxid.GetHash().ToString(), nodeid); + requests.emplace_back(gtxid); + m_txrequest.RequestedTx(nodeid, gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL); + } else { + // We have already seen this transaction, no need to download. This is just a belt-and-suspenders, as + // this should already be called whenever a transaction becomes AlreadyHaveTx(). + m_txrequest.ForgetTxHash(gtxid.GetHash()); + } + } + return requests; +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index fc57f690240..49948908d41 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -150,6 +150,12 @@ class TxDownloadManagerImpl { void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info); void DisconnectedPeer(NodeId nodeid); + + /** New inv has been received. May be added as a candidate to txrequest. */ + bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv); + + /** Get getdata requests to send. */ + std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From 3a41926d1b59dc9bbabc38cdc461c169426d94e7 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 25 Apr 2024 11:48:43 +0100 Subject: [PATCH 10/29] [refactor] move notfound processing to txdownload --- src/net_processing.cpp | 8 ++++---- src/node/txdownloadman.h | 3 +++ src/node/txdownloadman_impl.cpp | 13 +++++++++++++ src/node/txdownloadman_impl.h | 3 +++ 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 71162e957db..3f4f4f6af38 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5133,16 +5133,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (msg_type == NetMsgType::NOTFOUND) { std::vector vInv; vRecv >> vInv; + std::vector tx_invs; if (vInv.size() <= node::MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) { - LOCK(m_tx_download_mutex); for (CInv &inv : vInv) { if (inv.IsGenTxMsg()) { - // If we receive a NOTFOUND message for a tx we requested, mark the announcement for it as - // completed in TxRequestTracker. - m_txdownloadman.GetTxRequestRef().ReceivedResponse(pfrom.GetId(), inv.hash); + tx_invs.emplace_back(inv.hash); } } } + LOCK(m_tx_download_mutex); + m_txdownloadman.ReceivedNotFound(pfrom.GetId(), tx_invs); return; } diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index ce0c9594281..6995725ecc6 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -108,6 +108,9 @@ class TxDownloadManager { /** Get getdata requests to send. */ std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); + + /** Should be called when a notfound for a tx has been received. */ + void ReceivedNotFound(NodeId nodeid, const std::vector& txhashes); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index d0125f69ec0..6b505af22f2 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -67,6 +67,10 @@ std::vector TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::ch { return m_impl->GetRequestsToSend(nodeid, current_time); } +void TxDownloadManager::ReceivedNotFound(NodeId nodeid, const std::vector& txhashes) +{ + m_impl->ReceivedNotFound(nodeid, txhashes); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -205,4 +209,13 @@ std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std } return requests; } + +void TxDownloadManagerImpl::ReceivedNotFound(NodeId nodeid, const std::vector& txhashes) +{ + for (const auto& txhash : txhashes) { + // If we receive a NOTFOUND message for a tx we requested, mark the announcement for it as + // completed in TxRequestTracker. + m_txrequest.ReceivedResponse(nodeid, txhash); + } +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 49948908d41..a2af7506e7b 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -156,6 +156,9 @@ class TxDownloadManagerImpl { /** Get getdata requests to send. */ std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); + + /** Marks a tx as ReceivedResponse in txrequest. */ + void ReceivedNotFound(NodeId nodeid, const std::vector& txhashes); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From c8e67b9169bddc0bdfefa10e9cf7f9c22847e237 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 15 May 2024 16:00:32 +0100 Subject: [PATCH 11/29] [refactor] move ProcessInvalidTx and ProcessValidTx definitions down ProcessInvalidTx will return a PackageToValidate, so it needs to be defined afterward. --- src/net_processing.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3f4f4f6af38..95fdf862f12 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -564,20 +564,6 @@ class PeerManagerImpl final : public PeerManager */ bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer); - /** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID. - * @param[in] first_time_failure Whether this tx should be added to vExtraTxnForCompact. - * Set to false if the tx has already been rejected before, - * e.g. is an orphan, to avoid adding duplicate entries. - * Updates m_txrequest, m_lazy_recent_rejects, m_lazy_recent_rejects_reconsiderable, m_orphanage, and vExtraTxnForCompact. */ - void 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. - * Updates m_txrequest, m_orphanage, and vExtraTxnForCompact. Also queues the tx for relay. */ - void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list& replaced_transactions) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); - struct PackageToValidate { const Package m_txns; const std::vector m_senders; @@ -602,6 +588,20 @@ class PeerManagerImpl final : public PeerManager } }; + /** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID. + * @param[in] first_time_failure Whether this tx should be added to vExtraTxnForCompact. + * Set to false if the tx has already been rejected before, + * e.g. is an orphan, to avoid adding duplicate entries. + * Updates m_txrequest, m_lazy_recent_rejects, m_lazy_recent_rejects_reconsiderable, m_orphanage, and vExtraTxnForCompact. */ + void 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. + * Updates m_txrequest, m_orphanage, and vExtraTxnForCompact. Also queues the tx for relay. */ + void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list& replaced_transactions) + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); + /** Handle the results of package validation: calls ProcessValidTx and ProcessInvalidTx for * individual transactions, and caches rejection for the package as a group. */ From 416fbc952b209817a37e76c09fff5d17be7a72d0 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 30 Apr 2024 10:54:00 +0100 Subject: [PATCH 12/29] [refactor] move new orphan handling to ProcessInvalidTx --- src/net_processing.cpp | 159 +++++++++++++++++++++-------------------- 1 file changed, 81 insertions(+), 78 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 95fdf862f12..274653dae65 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3006,8 +3006,11 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx AssertLockHeld(g_msgproc_mutex); AssertLockHeld(m_tx_download_mutex); + PeerRef peer{GetPeerRef(nodeid)}; + auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); + const CTransaction& tx{*ptx}; LogDebug(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n", ptx->GetHash().ToString(), @@ -3016,6 +3019,84 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx state.ToString()); 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 + // already in orphanage or from 1p1c processing. + if (first_time_failure) { + bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected + + // Deduplicate parent txids, so that we don't have to loop over + // the same parent txid more than once down below. + std::vector unique_parents; + unique_parents.reserve(tx.vin.size()); + for (const CTxIn& txin : tx.vin) { + // We start with all parents, and then remove duplicates below. + unique_parents.push_back(txin.prevout.hash); + } + 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_lazy_recent_rejects and m_lazy_recent_rejects_reconsiderable. + // We can tolerate having up to 1 parent in m_lazy_recent_rejects_reconsiderable since we + // submit 1p1c packages. However, fail immediately if any are in m_lazy_recent_rejects. + std::optional rejected_parent_reconsiderable; + for (const uint256& parent_txid : unique_parents) { + if (RecentRejectsFilter().contains(parent_txid)) { + fRejectedParents = true; + break; + } else if (RecentRejectsReconsiderableFilter().contains(parent_txid) && !m_mempool.exists(GenTxid::Txid(parent_txid))) { + // More than 1 parent in m_lazy_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) { + const auto current_time{GetTime()}; + + for (const uint256& parent_txid : unique_parents) { + // Here, we only have the txid (and not wtxid) of the + // inputs, so we only request in txid mode, even for + // wtxidrelay peers. + // Eventually we should replace this with an improved + // protocol for getting all unconfirmed parents. + const auto gtxid{GenTxid::Txid(parent_txid)}; + if (peer) AddKnownTx(*peer, parent_txid); + // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been + // previously rejected for being too low feerate. This orphan might CPFP it. + if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { + m_txdownloadman.AddTxAnnouncement(nodeid, gtxid, current_time, /*p2p_inv=*/false); + } + } + + if (m_orphanage.AddTx(ptx, nodeid)) { + AddToCompactExtraTransactions(ptx); + } + + // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore. + m_txrequest.ForgetTxHash(tx.GetHash()); + m_txrequest.ForgetTxHash(tx.GetWitnessHash()); + + // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) + m_orphanage.LimitOrphans(m_opts.max_orphan_txs, m_rng); + } else { + LogDebug(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", + tx.GetHash().ToString(), + tx.GetWitnessHash().ToString()); + // We will continue to reject this tx since it has rejected + // parents so avoid re-requesting it from other peers. + // Here we add both the txid and the wtxid, as we know that + // regardless of what witness is provided, we will not accept + // this, so we don't need to allow for redownload of this txid + // from any of our non-wtxidrelay peers. + RecentRejectsFilter().insert(tx.GetHash().ToUint256()); + RecentRejectsFilter().insert(tx.GetWitnessHash().ToUint256()); + m_txrequest.ForgetTxHash(tx.GetHash()); + m_txrequest.ForgetTxHash(tx.GetWitnessHash()); + } + } return; } else if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) { // We can add the wtxid of this transaction to our reject filter. @@ -4375,7 +4456,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LOCK2(cs_main, m_tx_download_mutex); auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); - auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); m_txrequest.ReceivedResponse(pfrom.GetId(), txid); if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid); @@ -4445,83 +4525,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions); pfrom.m_last_tx_time = GetTime(); } - else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) - { - bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected - - // Deduplicate parent txids, so that we don't have to loop over - // the same parent txid more than once down below. - std::vector unique_parents; - unique_parents.reserve(tx.vin.size()); - for (const CTxIn& txin : tx.vin) { - // We start with all parents, and then remove duplicates below. - unique_parents.push_back(txin.prevout.hash); - } - 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_lazy_recent_rejects and m_lazy_recent_rejects_reconsiderable. - // We can tolerate having up to 1 parent in m_lazy_recent_rejects_reconsiderable since we - // submit 1p1c packages. However, fail immediately if any are in m_lazy_recent_rejects. - std::optional rejected_parent_reconsiderable; - for (const uint256& parent_txid : unique_parents) { - if (RecentRejectsFilter().contains(parent_txid)) { - fRejectedParents = true; - break; - } else if (RecentRejectsReconsiderableFilter().contains(parent_txid) && !m_mempool.exists(GenTxid::Txid(parent_txid))) { - // More than 1 parent in m_lazy_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) { - const auto current_time{GetTime()}; - - for (const uint256& parent_txid : unique_parents) { - // Here, we only have the txid (and not wtxid) of the - // inputs, so we only request in txid mode, even for - // wtxidrelay peers. - // Eventually we should replace this with an improved - // protocol for getting all unconfirmed parents. - const auto gtxid{GenTxid::Txid(parent_txid)}; - AddKnownTx(*peer, parent_txid); - // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been - // previously rejected for being too low feerate. This orphan might CPFP it. - if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { - m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time, /*p2p_inv=*/false); - } - } - - if (m_orphanage.AddTx(ptx, pfrom.GetId())) { - AddToCompactExtraTransactions(ptx); - } - - // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore. - m_txrequest.ForgetTxHash(tx.GetHash()); - m_txrequest.ForgetTxHash(tx.GetWitnessHash()); - - // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) - m_orphanage.LimitOrphans(m_opts.max_orphan_txs, m_rng); - } else { - LogDebug(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", - tx.GetHash().ToString(), - tx.GetWitnessHash().ToString()); - // We will continue to reject this tx since it has rejected - // parents so avoid re-requesting it from other peers. - // Here we add both the txid and the wtxid, as we know that - // regardless of what witness is provided, we will not accept - // this, so we don't need to allow for redownload of this txid - // from any of our non-wtxidrelay peers. - RecentRejectsFilter().insert(tx.GetHash().ToUint256()); - RecentRejectsFilter().insert(tx.GetWitnessHash().ToUint256()); - m_txrequest.ForgetTxHash(tx.GetHash()); - m_txrequest.ForgetTxHash(tx.GetWitnessHash()); - } - } if (state.IsInvalid()) { ProcessInvalidTx(pfrom.GetId(), ptx, state, /*first_time_failure=*/true); } From 798cc8f5aac9bf2111ea88d4a4c3817d34e089e2 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 15 May 2024 16:10:24 +0100 Subject: [PATCH 13/29] [refactor] move Find1P1CPackage into ProcessInvalidTx --- src/net_processing.cpp | 53 +++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 274653dae65..1ffeb3a3ce1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -565,8 +565,8 @@ class PeerManagerImpl final : public PeerManager bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer); struct PackageToValidate { - const Package m_txns; - const std::vector m_senders; + Package m_txns; + std::vector m_senders; /** Construct a 1-parent-1-child package. */ explicit PackageToValidate(const CTransactionRef& parent, const CTransactionRef& child, @@ -576,6 +576,16 @@ class PeerManagerImpl final : public PeerManager 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)", @@ -589,11 +599,17 @@ class PeerManagerImpl final : public PeerManager }; /** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID. - * @param[in] first_time_failure Whether this tx should be added to vExtraTxnForCompact. + * @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. + * Set to true for transactions just received over p2p. * Set to false if the tx has already been rejected before, - * e.g. is an orphan, to avoid adding duplicate entries. - * Updates m_txrequest, m_lazy_recent_rejects, m_lazy_recent_rejects_reconsiderable, m_orphanage, and vExtraTxnForCompact. */ - void ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result, + * e.g. is already in the orphanage, to avoid adding duplicate entries. + * Updates m_txrequest, m_lazy_recent_rejects, m_lazy_recent_rejects_reconsiderable, m_orphanage, and vExtraTxnForCompact. + * + * @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) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); @@ -2999,7 +3015,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, return; } -void 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); @@ -3017,6 +3033,8 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx ptx->GetWitnessHash().ToString(), nodeid, state.ToString()); + // Populated if failure is reconsiderable and eligible package is found. + 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 @@ -3097,7 +3115,7 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx m_txrequest.ForgetTxHash(tx.GetWitnessHash()); } } - return; + return std::nullopt; } else if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) { // We can add the wtxid of this transaction to our reject filter. // Do not add txids of witness transactions or witness-stripped @@ -3117,6 +3135,14 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx // because we should not download or submit this transaction by itself again, but may // submit it as part of a package later. RecentRejectsReconsiderableFilter().insert(ptx->GetWitnessHash().ToUint256()); + + if (first_time_failure) { + // 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. + 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); + } } else { RecentRejectsFilter().insert(ptx->GetWitnessHash().ToUint256()); } @@ -3147,6 +3173,8 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx if (Assume(state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) { LogDebug(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); } + + return package_to_validate; } void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list& replaced_transactions) @@ -4526,14 +4554,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, pfrom.m_last_tx_time = GetTime(); } if (state.IsInvalid()) { - ProcessInvalidTx(pfrom.GetId(), ptx, state, /*first_time_failure=*/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) { - LogDebug(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())}) { + if (auto package_to_validate{ProcessInvalidTx(pfrom.GetId(), ptx, state, /*first_time_failure=*/true)}) { 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"); From 6797bc42a762f431a986852fa74b1775aea8ba38 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 15 Oct 2024 17:57:31 +0100 Subject: [PATCH 14/29] [p2p] restrict RecursiveDynamicUsage of orphans added to vExtraTxnForCompact There does not appear to be any reason why orphan transactions should be given special treatment. --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1ffeb3a3ce1..db31c3e45ae 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3089,7 +3089,7 @@ std::optional PeerManagerImpl::ProcessInvali } } - if (m_orphanage.AddTx(ptx, nodeid)) { + if (m_orphanage.AddTx(ptx, nodeid) && RecursiveDynamicUsage(*ptx) < 100000) { AddToCompactExtraTransactions(ptx); } From f497414ce76a4cf44fa669e3665746cc17710fc6 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 30 Apr 2024 13:12:21 +0100 Subject: [PATCH 15/29] [refactor] put peerman tasks at the end of ProcessInvalidTx --- src/net_processing.cpp | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index db31c3e45ae..5e4a77cf468 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -573,7 +573,7 @@ class PeerManagerImpl final : public PeerManager NodeId parent_sender, NodeId child_sender) : m_txns{parent, child}, - m_senders {parent_sender, child_sender} + m_senders{parent_sender, child_sender} {} // Move ctor @@ -3033,6 +3033,11 @@ std::optional PeerManagerImpl::ProcessInvali ptx->GetWitnessHash().ToString(), nodeid, state.ToString()); + + // Whether we should call AddToCompactExtraTransactions at the end + bool add_extra_compact_tx{first_time_failure}; + // 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; @@ -3044,7 +3049,6 @@ std::optional PeerManagerImpl::ProcessInvali // Deduplicate parent txids, so that we don't have to loop over // the same parent txid more than once down below. - std::vector unique_parents; unique_parents.reserve(tx.vin.size()); for (const CTxIn& txin : tx.vin) { // We start with all parents, and then remove duplicates below. @@ -3081,7 +3085,6 @@ std::optional PeerManagerImpl::ProcessInvali // Eventually we should replace this with an improved // protocol for getting all unconfirmed parents. const auto gtxid{GenTxid::Txid(parent_txid)}; - if (peer) AddKnownTx(*peer, parent_txid); // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been // previously rejected for being too low feerate. This orphan might CPFP it. if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { @@ -3089,9 +3092,8 @@ std::optional PeerManagerImpl::ProcessInvali } } - if (m_orphanage.AddTx(ptx, nodeid) && RecursiveDynamicUsage(*ptx) < 100000) { - AddToCompactExtraTransactions(ptx); - } + // Potentially flip add_extra_compact_tx to false if AddTx returns false because the tx was already there + add_extra_compact_tx &= m_orphanage.AddTx(ptx, nodeid); // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore. m_txrequest.ForgetTxHash(tx.GetHash()); @@ -3100,6 +3102,7 @@ std::optional PeerManagerImpl::ProcessInvali // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) m_orphanage.LimitOrphans(m_opts.max_orphan_txs, m_rng); } else { + unique_parents.clear(); LogDebug(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", tx.GetHash().ToString(), tx.GetWitnessHash().ToString()); @@ -3115,8 +3118,9 @@ std::optional PeerManagerImpl::ProcessInvali m_txrequest.ForgetTxHash(tx.GetWitnessHash()); } } - return std::nullopt; - } else if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) { + } else if (state.GetResult() == TxValidationResult::TX_WITNESS_STRIPPED) { + add_extra_compact_tx = false; + } else { // We can add the wtxid of this transaction to our reject filter. // Do not add txids of witness transactions or witness-stripped // transactions to the filter, as they can have been malleated; @@ -3161,16 +3165,19 @@ std::optional PeerManagerImpl::ProcessInvali RecentRejectsFilter().insert(ptx->GetHash().ToUint256()); m_txrequest.ForgetTxHash(ptx->GetHash()); } - if (first_time_failure && RecursiveDynamicUsage(*ptx) < 100000) { - AddToCompactExtraTransactions(ptx); - } + } + if (add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) { + AddToCompactExtraTransactions(ptx); + } + for (const uint256& parent_txid : unique_parents) { + if (peer) AddKnownTx(*peer, parent_txid); } MaybePunishNodeForTx(nodeid, state); // If the tx failed in ProcessOrphanTx, it should be removed from the orphanage unless the // tx was still missing inputs. If the tx was not in the orphanage, EraseTx does nothing and returns 0. - if (Assume(state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) { + if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) { LogDebug(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); } From a8cf3b6e845741e4b992beced564397779bfb7da Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 25 Apr 2024 11:24:31 +0100 Subject: [PATCH 16/29] [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 @@ class PeerManagerImpl final : public PeerManager */ 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 @@ 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 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 @@ 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 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 @@ class TxDownloadManager { /** 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 @@ class TxDownloadManagerImpl { /** 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 From c6b21749ca0aea70908773d865e67511ca141ae6 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 15:45:33 +0100 Subject: [PATCH 17/29] [refactor] move valid tx processing to TxDownload --- src/net_processing.cpp | 12 +----------- src/node/txdownloadman.h | 3 +++ src/node/txdownloadman_impl.cpp | 16 ++++++++++++++++ src/node/txdownloadman_impl.h | 2 ++ 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 57e6fc7b2fa..6cf76d3f42d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3150,17 +3150,7 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c AssertLockHeld(g_msgproc_mutex); AssertLockHeld(m_tx_download_mutex); - auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); - auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); - - // As this version of the transaction was acceptable, we can forget about any requests for it. - // No-op if the tx is not in txrequest. - m_txrequest.ForgetTxHash(tx->GetHash()); - m_txrequest.ForgetTxHash(tx->GetWitnessHash()); - - m_orphanage.AddChildrenToWorkSet(*tx); - // If it came from the orphanage, remove it. No-op if the tx is not in txorphanage. - m_orphanage.EraseTx(tx->GetWitnessHash()); + m_txdownloadman.MempoolAcceptedTx(tx); LogDebug(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (wtxid=%s) (poolsz %u txn, %u kB)\n", nodeid, diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 1a1f0a6da71..4c2e35e23ac 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -152,6 +152,9 @@ class TxDownloadManager { * 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); + + /** Respond to successful transaction submission to mempool */ + void MempoolAcceptedTx(const CTransactionRef& tx); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 2a62b090c00..1a8de26983f 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -75,6 +75,10 @@ std::optional TxDownloadManager::Find1P1CPackage(const CTrans { return m_impl->Find1P1CPackage(ptx, nodeid); } +void TxDownloadManager::MempoolAcceptedTx(const CTransactionRef& tx) +{ + m_impl->MempoolAcceptedTx(tx); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -272,4 +276,16 @@ std::optional TxDownloadManagerImpl::Find1P1CPackage(const CT } return std::nullopt; } + +void TxDownloadManagerImpl::MempoolAcceptedTx(const CTransactionRef& tx) +{ + // As this version of the transaction was acceptable, we can forget about any requests for it. + // No-op if the tx is not in txrequest. + m_txrequest.ForgetTxHash(tx->GetHash()); + m_txrequest.ForgetTxHash(tx->GetWitnessHash()); + + m_orphanage.AddChildrenToWorkSet(*tx); + // If it came from the orphanage, remove it. No-op if the tx is not in txorphanage. + m_orphanage.EraseTx(tx->GetWitnessHash()); +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 730a9642ad8..b9a89f1f63d 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -162,6 +162,8 @@ class TxDownloadManagerImpl { void ReceivedNotFound(NodeId nodeid, const std::vector& txhashes); std::optional Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid); + + void MempoolAcceptedTx(const CTransactionRef& tx); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From c4ce0c1218d0a3a2e9b22701f26391b8a9107196 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 15:55:57 +0100 Subject: [PATCH 18/29] [refactor] move invalid tx processing to TxDownload Move-only. Also delete external RecentRejectsFilter() access since it is no longer necessary. --- src/net_processing.cpp | 150 +---------------------------- src/node/txdownloadman.h | 15 ++- src/node/txdownloadman_impl.cpp | 161 +++++++++++++++++++++++++++++++- src/node/txdownloadman_impl.h | 2 + 4 files changed, 175 insertions(+), 153 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6cf76d3f42d..5ce7317ab50 100644 --- 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 m_block_stalling_timeout{BLOCK_STALLING_TIMEOUT_DEFAULT}; - CRollingBloomFilter& RecentRejectsFilter() EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex) - { - AssertLockHeld(m_tx_download_mutex); - return m_txdownloadman.RecentRejectsFilter(); - } - CRollingBloomFilter& RecentRejectsReconsiderableFilter() EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex) { AssertLockHeld(m_tx_download_mutex); @@ -1906,7 +1900,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, m_banman(banman), m_chainman(chainman), m_mempool(pool), - m_txdownloadman(node::TxDownloadOptions{pool, m_rng}), + m_txdownloadman(node::TxDownloadOptions{pool, m_rng, opts.max_orphan_txs}), m_warnings{warnings}, m_opts{opts} { @@ -2984,148 +2978,14 @@ std::optional PeerManagerImpl::ProcessInvalidTx(NodeId PeerRef peer{GetPeerRef(nodeid)}; - auto& m_orphanage = m_txdownloadman.GetOrphanageRef(); - auto& m_txrequest = m_txdownloadman.GetTxRequestRef(); - const CTransaction& tx{*ptx}; - LogDebug(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), nodeid, state.ToString()); - // Whether we should call AddToCompactExtraTransactions at the end - bool add_extra_compact_tx{first_time_failure}; - // 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; - - 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 - // already in orphanage or from 1p1c processing. - if (first_time_failure) { - bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected - - // Deduplicate parent txids, so that we don't have to loop over - // the same parent txid more than once down below. - unique_parents.reserve(tx.vin.size()); - for (const CTxIn& txin : tx.vin) { - // We start with all parents, and then remove duplicates below. - unique_parents.push_back(txin.prevout.hash); - } - 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_lazy_recent_rejects and m_lazy_recent_rejects_reconsiderable. - // We can tolerate having up to 1 parent in m_lazy_recent_rejects_reconsiderable since we - // submit 1p1c packages. However, fail immediately if any are in m_lazy_recent_rejects. - std::optional rejected_parent_reconsiderable; - for (const uint256& parent_txid : unique_parents) { - if (RecentRejectsFilter().contains(parent_txid)) { - fRejectedParents = true; - break; - } else if (RecentRejectsReconsiderableFilter().contains(parent_txid) && !m_mempool.exists(GenTxid::Txid(parent_txid))) { - // More than 1 parent in m_lazy_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) { - const auto current_time{GetTime()}; - - for (const uint256& parent_txid : unique_parents) { - // Here, we only have the txid (and not wtxid) of the - // inputs, so we only request in txid mode, even for - // wtxidrelay peers. - // Eventually we should replace this with an improved - // protocol for getting all unconfirmed parents. - const auto gtxid{GenTxid::Txid(parent_txid)}; - // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been - // previously rejected for being too low feerate. This orphan might CPFP it. - if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { - m_txdownloadman.AddTxAnnouncement(nodeid, gtxid, current_time, /*p2p_inv=*/false); - } - } + const auto& [add_extra_compact_tx, unique_parents, package_to_validate] = m_txdownloadman.MempoolRejectedTx(ptx, state, nodeid, first_time_failure); - // Potentially flip add_extra_compact_tx to false if AddTx returns false because the tx was already there - add_extra_compact_tx &= m_orphanage.AddTx(ptx, nodeid); - - // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore. - m_txrequest.ForgetTxHash(tx.GetHash()); - m_txrequest.ForgetTxHash(tx.GetWitnessHash()); - - // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) - m_orphanage.LimitOrphans(m_opts.max_orphan_txs, m_rng); - } else { - unique_parents.clear(); - LogDebug(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", - tx.GetHash().ToString(), - tx.GetWitnessHash().ToString()); - // We will continue to reject this tx since it has rejected - // parents so avoid re-requesting it from other peers. - // Here we add both the txid and the wtxid, as we know that - // regardless of what witness is provided, we will not accept - // this, so we don't need to allow for redownload of this txid - // from any of our non-wtxidrelay peers. - RecentRejectsFilter().insert(tx.GetHash().ToUint256()); - RecentRejectsFilter().insert(tx.GetWitnessHash().ToUint256()); - m_txrequest.ForgetTxHash(tx.GetHash()); - m_txrequest.ForgetTxHash(tx.GetWitnessHash()); - } - } - } else if (state.GetResult() == TxValidationResult::TX_WITNESS_STRIPPED) { - add_extra_compact_tx = false; - } else { - // We can add the wtxid of this transaction to our reject filter. - // Do not add txids of witness transactions or witness-stripped - // transactions to the filter, as they can have been malleated; - // adding such txids to the reject filter would potentially - // interfere with relay of valid transactions from peers that - // do not support wtxid-based relay. See - // https://github.com/bitcoin/bitcoin/issues/8279 for details. - // We can remove this restriction (and always add wtxids to - // the filter even for witness stripped transactions) once - // wtxid-based relay is broadly deployed. - // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 - // for concerns around weakening security of unupgraded nodes - // if we start doing this too early. - if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { - // If the result is TX_RECONSIDERABLE, add it to m_lazy_recent_rejects_reconsiderable - // because we should not download or submit this transaction by itself again, but may - // submit it as part of a package later. - RecentRejectsReconsiderableFilter().insert(ptx->GetWitnessHash().ToUint256()); - - if (first_time_failure) { - // 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. - 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 = m_txdownloadman.Find1P1CPackage(ptx, nodeid); - } - } else { - RecentRejectsFilter().insert(ptx->GetWitnessHash().ToUint256()); - } - m_txrequest.ForgetTxHash(ptx->GetWitnessHash()); - // If the transaction failed for TX_INPUTS_NOT_STANDARD, - // then we know that the witness was irrelevant to the policy - // failure, since this check depends only on the txid - // (the scriptPubKey being spent is covered by the txid). - // Add the txid to the reject filter to prevent repeated - // processing of this transaction in the event that child - // transactions are later received (resulting in - // parent-fetching by txid via the orphan-handling logic). - // We only add the txid if it differs from the wtxid, to avoid wasting entries in the - // rolling bloom filter. - if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && ptx->HasWitness()) { - RecentRejectsFilter().insert(ptx->GetHash().ToUint256()); - m_txrequest.ForgetTxHash(ptx->GetHash()); - } - } if (add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) { AddToCompactExtraTransactions(ptx); } @@ -3135,12 +2995,6 @@ std::optional PeerManagerImpl::ProcessInvalidTx(NodeId MaybePunishNodeForTx(nodeid, state); - // If the tx failed in ProcessOrphanTx, it should be removed from the orphanage unless the - // tx was still missing inputs. If the tx was not in the orphanage, EraseTx does nothing and returns 0. - if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) { - LogDebug(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); - } - return package_to_validate; } diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 4c2e35e23ac..5b797f5d9b1 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -41,6 +41,8 @@ struct TxDownloadOptions { const CTxMemPool& m_mempool; /** RNG provided by caller. */ FastRandomContext& m_rng; + /** Maximum number of transactions allowed in orphanage. */ + const uint32_t m_max_orphan_txs; }; struct TxDownloadConnectionInfo { /** Whether this peer is preferred for transaction download. */ @@ -64,6 +66,8 @@ struct PackageToValidate { // Move ctor PackageToValidate(PackageToValidate&& other) : m_txns{std::move(other.m_txns)}, m_senders{std::move(other.m_senders)} {} + // Copy ctor + PackageToValidate(const PackageToValidate& other) = default; // Move assignment PackageToValidate& operator=(PackageToValidate&& other) { @@ -83,6 +87,13 @@ struct PackageToValidate { m_senders.back()); } }; +struct RejectedTxTodo +{ + bool m_should_add_extra_compact_tx; + std::vector m_unique_parents; + std::optional m_package_to_validate; +}; + /** * Class responsible for deciding what transactions to request and, once @@ -114,7 +125,6 @@ class TxDownloadManager { // temporary and removed later once logic has been moved internally. TxOrphanage& GetOrphanageRef(); TxRequestTracker& GetTxRequestRef(); - CRollingBloomFilter& RecentRejectsFilter(); CRollingBloomFilter& RecentRejectsReconsiderableFilter(); // Responses to chain events. TxDownloadManager is not an actual client of ValidationInterface, these are called through PeerManager. @@ -155,6 +165,9 @@ class TxDownloadManager { /** Respond to successful transaction submission to mempool */ void MempoolAcceptedTx(const CTransactionRef& tx); + + /** Respond to transaction rejected from mempool */ + RejectedTxTodo MempoolRejectedTx(const CTransactionRef& ptx, const TxValidationState& state, NodeId nodeid, bool first_time_failure); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 1a8de26983f..13000dc5755 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::RecentRejectsFilter() -{ - return m_impl->RecentRejectsFilter(); -} CRollingBloomFilter& TxDownloadManager::RecentRejectsReconsiderableFilter() { return m_impl->RecentRejectsReconsiderableFilter(); @@ -79,6 +75,10 @@ void TxDownloadManager::MempoolAcceptedTx(const CTransactionRef& tx) { m_impl->MempoolAcceptedTx(tx); } +RejectedTxTodo TxDownloadManager::MempoolRejectedTx(const CTransactionRef& ptx, const TxValidationState& state, NodeId nodeid, bool first_time_failure) +{ + return m_impl->MempoolRejectedTx(ptx, state, nodeid, first_time_failure); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -167,6 +167,8 @@ void TxDownloadManagerImpl::DisconnectedPeer(NodeId nodeid) bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv) { // If this is an inv received from a peer and we already have it, we can drop it. + // If this is a request for the parent of an orphan, we don't drop transactions that we already have. In particular, + // we *do* want to request parents that are in m_lazy_recent_rejects_reconsiderable, since they can be CPFP'd. if (p2p_inv && AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) return true; auto it = m_peer_info.find(peer); @@ -288,4 +290,155 @@ void TxDownloadManagerImpl::MempoolAcceptedTx(const CTransactionRef& tx) // If it came from the orphanage, remove it. No-op if the tx is not in txorphanage. m_orphanage.EraseTx(tx->GetWitnessHash()); } + +node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransactionRef& ptx, const TxValidationState& state, NodeId nodeid, bool first_time_failure) +{ + const CTransaction& tx{*ptx}; + // Results returned to caller + // Whether we should call AddToCompactExtraTransactions at the end + bool add_extra_compact_tx{first_time_failure}; + // 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; + + 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 + // already in orphanage or from 1p1c processing. + if (first_time_failure) { + bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected + + // Deduplicate parent txids, so that we don't have to loop over + // the same parent txid more than once down below. + unique_parents.reserve(tx.vin.size()); + for (const CTxIn& txin : tx.vin) { + // We start with all parents, and then remove duplicates below. + unique_parents.push_back(txin.prevout.hash); + } + 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_lazy_recent_rejects and m_lazy_recent_rejects_reconsiderable. + // We can tolerate having up to 1 parent in m_lazy_recent_rejects_reconsiderable since we + // submit 1p1c packages. However, fail immediately if any are in m_lazy_recent_rejects. + std::optional rejected_parent_reconsiderable; + for (const uint256& parent_txid : unique_parents) { + if (RecentRejectsFilter().contains(parent_txid)) { + fRejectedParents = true; + break; + } else if (RecentRejectsReconsiderableFilter().contains(parent_txid) && + !m_opts.m_mempool.exists(GenTxid::Txid(parent_txid))) { + // More than 1 parent in m_lazy_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) { + const auto current_time{GetTime()}; + + for (const uint256& parent_txid : unique_parents) { + // Here, we only have the txid (and not wtxid) of the + // inputs, so we only request in txid mode, even for + // wtxidrelay peers. + // Eventually we should replace this with an improved + // protocol for getting all unconfirmed parents. + const auto gtxid{GenTxid::Txid(parent_txid)}; + // Exclude m_lazy_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(nodeid, gtxid, current_time, /*p2p_inv=*/false); + } + } + + // Potentially flip add_extra_compact_tx to false if AddTx returns false because the tx was already there + add_extra_compact_tx &= m_orphanage.AddTx(ptx, nodeid); + + // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore. + m_txrequest.ForgetTxHash(tx.GetHash()); + m_txrequest.ForgetTxHash(tx.GetWitnessHash()); + + // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) + m_orphanage.LimitOrphans(m_opts.m_max_orphan_txs, m_opts.m_rng); + } else { + unique_parents.clear(); + LogDebug(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", + tx.GetHash().ToString(), + tx.GetWitnessHash().ToString()); + // We will continue to reject this tx since it has rejected + // parents so avoid re-requesting it from other peers. + // Here we add both the txid and the wtxid, as we know that + // regardless of what witness is provided, we will not accept + // this, so we don't need to allow for redownload of this txid + // from any of our non-wtxidrelay peers. + RecentRejectsFilter().insert(tx.GetHash().ToUint256()); + RecentRejectsFilter().insert(tx.GetWitnessHash().ToUint256()); + m_txrequest.ForgetTxHash(tx.GetHash()); + m_txrequest.ForgetTxHash(tx.GetWitnessHash()); + } + } + } else if (state.GetResult() == TxValidationResult::TX_WITNESS_STRIPPED) { + add_extra_compact_tx = false; + } else { + // We can add the wtxid of this transaction to our reject filter. + // Do not add txids of witness transactions or witness-stripped + // transactions to the filter, as they can have been malleated; + // adding such txids to the reject filter would potentially + // interfere with relay of valid transactions from peers that + // do not support wtxid-based relay. See + // https://github.com/bitcoin/bitcoin/issues/8279 for details. + // We can remove this restriction (and always add wtxids to + // the filter even for witness stripped transactions) once + // wtxid-based relay is broadly deployed. + // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 + // for concerns around weakening security of unupgraded nodes + // if we start doing this too early. + if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { + // If the result is TX_RECONSIDERABLE, add it to m_lazy_recent_rejects_reconsiderable + // because we should not download or submit this transaction by itself again, but may + // submit it as part of a package later. + RecentRejectsReconsiderableFilter().insert(ptx->GetWitnessHash().ToUint256()); + + if (first_time_failure) { + // 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. + 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); + } + } else { + RecentRejectsFilter().insert(ptx->GetWitnessHash().ToUint256()); + } + m_txrequest.ForgetTxHash(ptx->GetWitnessHash()); + // If the transaction failed for TX_INPUTS_NOT_STANDARD, + // then we know that the witness was irrelevant to the policy + // failure, since this check depends only on the txid + // (the scriptPubKey being spent is covered by the txid). + // Add the txid to the reject filter to prevent repeated + // processing of this transaction in the event that child + // transactions are later received (resulting in + // parent-fetching by txid via the orphan-handling logic). + // We only add the txid if it differs from the wtxid, to avoid wasting entries in the + // rolling bloom filter. + if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && ptx->HasWitness()) { + RecentRejectsFilter().insert(ptx->GetHash().ToUint256()); + m_txrequest.ForgetTxHash(ptx->GetHash()); + } + } + + // If the tx failed in ProcessOrphanTx, it should be removed from the orphanage unless the + // tx was still missing inputs. If the tx was not in the orphanage, EraseTx does nothing and returns 0. + if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) { + LogDebug(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); + } + + return RejectedTxTodo{ + .m_should_add_extra_compact_tx = add_extra_compact_tx, + .m_unique_parents = std::move(unique_parents), + .m_package_to_validate = std::move(package_to_validate) + }; +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index b9a89f1f63d..496f2891847 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -164,6 +165,7 @@ class TxDownloadManagerImpl { std::optional Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid); void MempoolAcceptedTx(const CTransactionRef& tx); + RejectedTxTodo MempoolRejectedTx(const CTransactionRef& ptx, const TxValidationState& state, NodeId nodeid, bool first_time_failure); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From 257568eab5baba07571fe2c68759e843d215d4a9 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 16:35:07 +0100 Subject: [PATCH 19/29] [refactor] move invalid package processing to TxDownload --- src/net_processing.cpp | 2 +- src/node/txdownloadman.h | 3 +++ src/node/txdownloadman_impl.cpp | 9 +++++++++ src/node/txdownloadman_impl.h | 1 + 4 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5ce7317ab50..8d6b13fe608 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3029,7 +3029,7 @@ void PeerManagerImpl::ProcessPackageResult(const node::PackageToValidate& packag const auto& senders = package_to_validate.m_senders; if (package_result.m_state.IsInvalid()) { - RecentRejectsReconsiderableFilter().insert(GetPackageHash(package)); + m_txdownloadman.MempoolRejectedPackage(package); } // We currently only expect to process 1-parent-1-child packages. Remove if this changes. if (!Assume(package.size() == 2)) return; diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 5b797f5d9b1..514e1064101 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -168,6 +168,9 @@ class TxDownloadManager { /** Respond to transaction rejected from mempool */ RejectedTxTodo MempoolRejectedTx(const CTransactionRef& ptx, const TxValidationState& state, NodeId nodeid, bool first_time_failure); + + /** Respond to package rejected from mempool */ + void MempoolRejectedPackage(const Package& package); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 13000dc5755..4d5bfad68c1 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -79,6 +79,10 @@ RejectedTxTodo TxDownloadManager::MempoolRejectedTx(const CTransactionRef& ptx, { return m_impl->MempoolRejectedTx(ptx, state, nodeid, first_time_failure); } +void TxDownloadManager::MempoolRejectedPackage(const Package& package) +{ + m_impl->MempoolRejectedPackage(package); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -441,4 +445,9 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction .m_package_to_validate = std::move(package_to_validate) }; } + +void TxDownloadManagerImpl::MempoolRejectedPackage(const Package& package) +{ + RecentRejectsReconsiderableFilter().insert(GetPackageHash(package)); +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 496f2891847..9843c815e18 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -166,6 +166,7 @@ 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); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From 1e08195135bc54f7a8b28560ae10943b1fef0d83 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 16 Apr 2024 16:58:35 +0100 Subject: [PATCH 20/29] [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 @@ class PeerManagerImpl final : public PeerManager /** 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 @@ 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> 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 @@ 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> ReceivedTx(NodeId nodeid, const CTransactionRef& ptx); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From f150fb94e7dbb3c1f4fca32a0abf063943ca676d Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 15 May 2024 16:57:03 +0100 Subject: [PATCH 21/29] [refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownloadImpl --- src/node/txdownloadman.h | 14 -------------- src/node/txdownloadman_impl.cpp | 8 -------- src/node/txdownloadman_impl.h | 10 ++++++++++ 3 files changed, 10 insertions(+), 22 deletions(-) diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 164c4b706ef..e921ac3c2d4 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -131,15 +131,6 @@ class TxDownloadManager { void BlockConnected(const std::shared_ptr& pblock); void BlockDisconnected(); - /** Check whether we already have this gtxid in: - * - mempool - * - orphanage - * - m_recent_rejects - * - m_recent_rejects_reconsiderable (if include_reconsiderable = true) - * - m_recent_confirmed_transactions - * */ - bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable); - /** Creates a new PeerInfo. Saves the connection info to calculate tx announcement delays later. */ void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info); @@ -157,11 +148,6 @@ class TxDownloadManager { /** 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); - /** Respond to successful transaction submission to mempool */ void MempoolAcceptedTx(const CTransactionRef& tx); diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 22bfff167d4..e6b1873e28f 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -39,10 +39,6 @@ void TxDownloadManager::BlockDisconnected() { m_impl->BlockDisconnected(); } -bool TxDownloadManager::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) -{ - return m_impl->AlreadyHaveTx(gtxid, include_reconsiderable); -} void TxDownloadManager::ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info) { m_impl->ConnectedPeer(nodeid, info); @@ -63,10 +59,6 @@ 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); -} void TxDownloadManager::MempoolAcceptedTx(const CTransactionRef& tx) { m_impl->MempoolAcceptedTx(tx); diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 019b930a850..fe04770f650 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -148,6 +148,13 @@ class TxDownloadManagerImpl { void BlockConnected(const std::shared_ptr& pblock); void BlockDisconnected(); + /** Check whether we already have this gtxid in: + * - mempool + * - orphanage + * - m_recent_rejects + * - m_recent_rejects_reconsiderable (if include_reconsiderable = true) + * - m_recent_confirmed_transactions + * */ bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable); void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info); @@ -162,6 +169,9 @@ class TxDownloadManagerImpl { /** Marks a tx as ReceivedResponse in txrequest. */ 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); void MempoolAcceptedTx(const CTransactionRef& tx); From 969b07237b990b7eb6f3d24914ccc872202d8a0f Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 26 Apr 2024 11:47:47 +0100 Subject: [PATCH 22/29] [refactor] wrap {Have,Get}TxToReconsider in txdownload --- src/net_processing.cpp | 4 ++-- src/node/txdownloadman.h | 6 ++++++ src/node/txdownloadman_impl.cpp | 19 +++++++++++++++++++ src/node/txdownloadman_impl.h | 3 +++ 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3c223e1ea4c..bbd24d80928 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3076,7 +3076,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) CTransactionRef porphanTx = nullptr; - while (CTransactionRef porphanTx = m_txdownloadman.GetOrphanageRef().GetTxToReconsider(peer.m_id)) { + while (CTransactionRef porphanTx = m_txdownloadman.GetTxToReconsider(peer.m_id)) { const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; const Txid& orphanHash = porphanTx->GetHash(); @@ -5000,7 +5000,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt // the extra work may not be noticed, possibly resulting in an // unnecessary 100ms delay) LOCK(m_tx_download_mutex); - if (m_txdownloadman.GetOrphanageRef().HaveTxToReconsider(peer->m_id)) fMoreWork = true; + if (m_txdownloadman.HaveMoreWork(peer->m_id)) fMoreWork = true; } catch (const std::exception& e) { LogDebug(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size, e.what(), typeid(e).name()); } catch (...) { diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index e921ac3c2d4..aaff966cd56 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -161,6 +161,12 @@ class TxDownloadManager { * Return a bool indicating whether this tx should be validated. If false, optionally, a * PackageToValidate. */ std::pair> ReceivedTx(NodeId nodeid, const CTransactionRef& ptx); + + /** Whether there are any orphans to reconsider for this peer. */ + bool HaveMoreWork(NodeId nodeid) const; + + /** Returns next orphan tx to consider, or nullptr if none exist. */ + CTransactionRef GetTxToReconsider(NodeId nodeid); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index e6b1873e28f..935ec2d6b74 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -75,6 +75,14 @@ std::pair> TxDownloadManager::ReceivedTx( { return m_impl->ReceivedTx(nodeid, ptx); } +bool TxDownloadManager::HaveMoreWork(NodeId nodeid) const +{ + return m_impl->HaveMoreWork(nodeid); +} +CTransactionRef TxDownloadManager::GetTxToReconsider(NodeId nodeid) +{ + return m_impl->GetTxToReconsider(nodeid); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -496,4 +504,15 @@ std::pair> TxDownloadManagerImpl::Receive return {true, std::nullopt}; } + +bool TxDownloadManagerImpl::HaveMoreWork(NodeId nodeid) +{ + return m_orphanage.HaveTxToReconsider(nodeid); +} + +CTransactionRef TxDownloadManagerImpl::GetTxToReconsider(NodeId nodeid) +{ + return m_orphanage.GetTxToReconsider(nodeid); +} + } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index fe04770f650..85841777d05 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -179,6 +179,9 @@ class TxDownloadManagerImpl { void MempoolRejectedPackage(const Package& package); std::pair> ReceivedTx(NodeId nodeid, const CTransactionRef& ptx); + + bool HaveMoreWork(NodeId nodeid); + CTransactionRef GetTxToReconsider(NodeId nodeid); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From fa7027d0fc1fb2eb4148ba9741e1736f61d7e164 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 26 Apr 2024 11:56:24 +0100 Subject: [PATCH 23/29] [refactor] add CheckIsEmpty and GetOrphanTransactions, remove access to TxDownloadMan internals --- src/net_processing.cpp | 11 +++-------- src/node/txdownloadman.h | 16 ++++++++++------ src/node/txdownloadman_impl.cpp | 34 +++++++++++++++++++++++++-------- src/node/txdownloadman_impl.h | 5 +++++ 4 files changed, 44 insertions(+), 22 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index bbd24d80928..063dd441c9f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1527,10 +1527,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service LOCK(cs_main); // For m_node_states m_node_states.try_emplace(m_node_states.end(), nodeid); } - { - LOCK(m_tx_download_mutex); - assert(m_txdownloadman.GetTxRequestRef().Count(nodeid) == 0); - } + WITH_LOCK(m_tx_download_mutex, m_txdownloadman.CheckIsEmpty(nodeid)); if (NetPermissions::HasFlag(node.m_permission_flags, NetPermissionFlags::BloomFilter)) { our_services = static_cast(our_services | NODE_BLOOM); @@ -1616,9 +1613,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) assert(m_peers_downloading_from == 0); assert(m_outbound_peers_with_protect_from_disconnect == 0); assert(m_wtxid_relay_peers == 0); - LOCK(m_tx_download_mutex); - assert(m_txdownloadman.GetTxRequestRef().Size() == 0); - assert(m_txdownloadman.GetOrphanageRef().Size() == 0); + WITH_LOCK(m_tx_download_mutex, m_txdownloadman.CheckIsEmpty()); } } // cs_main if (node.fSuccessfullyConnected && @@ -1727,7 +1722,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c std::vector PeerManagerImpl::GetOrphanTransactions() { LOCK(m_tx_download_mutex); - return m_txdownloadman.GetOrphanageRef().GetOrphanTransactions(); + return m_txdownloadman.GetOrphanTransactions(); } PeerManagerInfo PeerManagerImpl::GetInfo() const diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index aaff966cd56..c2a76b3a4f2 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -15,7 +16,6 @@ class CBlock; class CRollingBloomFilter; class CTxMemPool; class GenTxid; -class TxOrphanage; class TxRequestTracker; namespace node { class TxDownloadManagerImpl; @@ -121,11 +121,6 @@ class TxDownloadManager { explicit TxDownloadManager(const TxDownloadOptions& options); ~TxDownloadManager(); - // Get references to internal data structures. Outside access to these data structures should be - // temporary and removed later once logic has been moved internally. - TxOrphanage& GetOrphanageRef(); - TxRequestTracker& GetTxRequestRef(); - // Responses to chain events. TxDownloadManager is not an actual client of ValidationInterface, these are called through PeerManager. void ActiveTipChange(); void BlockConnected(const std::shared_ptr& pblock); @@ -167,6 +162,15 @@ class TxDownloadManager { /** Returns next orphan tx to consider, or nullptr if none exist. */ CTransactionRef GetTxToReconsider(NodeId nodeid); + + /** Check that all data structures are empty. */ + void CheckIsEmpty() const; + + /** Check that all data structures that track per-peer information have nothing for this peer. */ + void CheckIsEmpty(NodeId nodeid) const; + + /** Wrapper for TxOrphanage::GetOrphanTransactions */ + std::vector GetOrphanTransactions() const; }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 935ec2d6b74..89125982dd6 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -19,14 +19,6 @@ TxDownloadManager::TxDownloadManager(const TxDownloadOptions& options) : {} TxDownloadManager::~TxDownloadManager() = default; -TxOrphanage& TxDownloadManager::GetOrphanageRef() -{ - return m_impl->m_orphanage; -} -TxRequestTracker& TxDownloadManager::GetTxRequestRef() -{ - return m_impl->m_txrequest; -} void TxDownloadManager::ActiveTipChange() { m_impl->ActiveTipChange(); @@ -83,6 +75,18 @@ CTransactionRef TxDownloadManager::GetTxToReconsider(NodeId nodeid) { return m_impl->GetTxToReconsider(nodeid); } +void TxDownloadManager::CheckIsEmpty() const +{ + m_impl->CheckIsEmpty(); +} +void TxDownloadManager::CheckIsEmpty(NodeId nodeid) const +{ + m_impl->CheckIsEmpty(nodeid); +} +std::vector TxDownloadManager::GetOrphanTransactions() const +{ + return m_impl->GetOrphanTransactions(); +} // TxDownloadManagerImpl void TxDownloadManagerImpl::ActiveTipChange() @@ -515,4 +519,18 @@ CTransactionRef TxDownloadManagerImpl::GetTxToReconsider(NodeId nodeid) return m_orphanage.GetTxToReconsider(nodeid); } +void TxDownloadManagerImpl::CheckIsEmpty(NodeId nodeid) +{ + assert(m_txrequest.Count(nodeid) == 0); +} +void TxDownloadManagerImpl::CheckIsEmpty() +{ + assert(m_orphanage.Size() == 0); + assert(m_txrequest.Size() == 0); + assert(m_num_wtxid_peers == 0); +} +std::vector TxDownloadManagerImpl::GetOrphanTransactions() const +{ + return m_orphanage.GetOrphanTransactions(); +} } // namespace node diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 85841777d05..c3e2417aa54 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -182,6 +182,11 @@ class TxDownloadManagerImpl { bool HaveMoreWork(NodeId nodeid); CTransactionRef GetTxToReconsider(NodeId nodeid); + + void CheckIsEmpty(); + void CheckIsEmpty(NodeId nodeid); + + std::vector GetOrphanTransactions() const; }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From 2266eba43a973345351f2b0a8296523fb7de5576 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 17 Sep 2024 19:26:57 -0400 Subject: [PATCH 24/29] [p2p] don't find 1p1cs for reconsiderable txns that are AlreadyHaveTx This is a slight behavior change: if a transaction is in both reconsiderable rejects and AlreadyHaveTx in another way, we don't try to return a 1p1c package. This is the correct thing to do, as we don't want to reconsider transactions that have multiple things wrong with them. For example, if a transaction is low feerate, and then later found to have a bad signature, we shouldn't try it again in a package. --- src/node/txdownloadman_impl.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 89125982dd6..ccc6cff8940 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -477,17 +477,7 @@ std::pair> TxDownloadManagerImpl::Receive // 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 (AlreadyHaveTx(GenTxid::Wtxid(wtxid), /*include_reconsiderable=*/false)) { // 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. @@ -504,8 +494,16 @@ std::pair> TxDownloadManagerImpl::Receive // peer simply for relaying a tx that our m_lazy_recent_rejects has caught, // regardless of false positives. return {false, std::nullopt}; + } else 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 {false, Find1P1CPackage(ptx, nodeid)}; } + return {true, std::nullopt}; } From 5269d57e6d78e90baa0b40629f60a2d1d63e2992 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 23 Aug 2024 16:05:19 +0100 Subject: [PATCH 25/29] [p2p] don't process orphan if in recent rejects This should never happen normally, but just in case. --- src/node/txdownloadman_impl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index ccc6cff8940..6078f91c0a3 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -313,7 +313,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction 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 // already in orphanage or from 1p1c processing. - if (first_time_failure) { + if (first_time_failure && !RecentRejectsFilter().contains(ptx->GetWitnessHash().ToUint256())) { bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected // Deduplicate parent txids, so that we don't have to loop over From f803c8ce8dd88d9d0fd7857f63d76045b1e2bcaa Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 15 Oct 2024 18:22:11 +0100 Subject: [PATCH 26/29] [p2p] filter 1p1c for child txid in recent rejects Avoid the fuzzer situation where: 1. Orphanage has 2 transactions with the same txid, one with witness, one without witness. 2. The transaction with witness is found to have `TX_INPUTS_NOT_STANDARD` error. The txid is added to recent rejects filter, and the tx with witness is deleted from orphanage. 3. A low feerate parent is found. Find1P1CPackage finds the transaction with no witness in orphanage, and returns the package. 4. net_processing has just been handed a package in which the child is already in recent rejects. --- src/node/txdownloadman_impl.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 6078f91c0a3..f9635d049ad 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -253,7 +253,8 @@ std::optional TxDownloadManagerImpl::Find1P1CPackage(const CT // 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))) { + if (!RecentRejectsReconsiderableFilter().contains(GetPackageHash(maybe_cpfp_package)) && + !RecentRejectsFilter().contains(child->GetHash().ToUint256())) { return PackageToValidate{ptx, child, nodeid, nodeid}; } } @@ -280,7 +281,8 @@ std::optional TxDownloadManagerImpl::Find1P1CPackage(const CT // 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))) { + if (!RecentRejectsReconsiderableFilter().contains(GetPackageHash(maybe_cpfp_package)) && + !RecentRejectsFilter().contains(child_tx->GetHash().ToUint256())) { return PackageToValidate{ptx, child_tx, nodeid, child_sender}; } } From fa584cbe727b62853a410623b3d7c738e11cbffd Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 27 Aug 2024 12:12:12 +0100 Subject: [PATCH 27/29] [p2p] add TxDownloadOptions bool to make TxRequestTracker deterministic Forward this bool to the TxRequestTracker ctor. This is needed for stablity in TxDownloadManager fuzzers --- src/net_processing.cpp | 2 +- src/node/txdownloadman.h | 2 ++ src/node/txdownloadman_impl.h | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 063dd441c9f..170352f7299 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1889,7 +1889,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, m_banman(banman), m_chainman(chainman), m_mempool(pool), - m_txdownloadman(node::TxDownloadOptions{pool, m_rng, opts.max_orphan_txs}), + m_txdownloadman(node::TxDownloadOptions{pool, m_rng, opts.max_orphan_txs, opts.deterministic_rng}), m_warnings{warnings}, m_opts{opts} { diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index c2a76b3a4f2..28ca90c5542 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -43,6 +43,8 @@ struct TxDownloadOptions { FastRandomContext& m_rng; /** Maximum number of transactions allowed in orphanage. */ const uint32_t m_max_orphan_txs; + /** Instantiate TxRequestTracker as deterministic (used for tests). */ + bool m_deterministic_txrequest{false}; }; struct TxDownloadConnectionInfo { /** Whether this peer is preferred for transaction download. */ diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index c3e2417aa54..48f02e607af 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -128,7 +128,7 @@ class TxDownloadManagerImpl { return *m_lazy_recent_confirmed_transactions; } - TxDownloadManagerImpl(const TxDownloadOptions& options) : m_opts{options} {} + TxDownloadManagerImpl(const TxDownloadOptions& options) : m_opts{options}, m_txrequest{options.m_deterministic_txrequest} {} struct PeerInfo { /** Information relevant to scheduling tx requests. */ From 699643f23a1bd0346e36bd90c83ba1b0b0a5c3fe Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 29 Apr 2024 11:58:54 +0100 Subject: [PATCH 28/29] [unit test] MempoolRejectedTx --- src/test/CMakeLists.txt | 1 + src/test/txdownload_tests.cpp | 337 ++++++++++++++++++++++++++++++++++ 2 files changed, 338 insertions(+) create mode 100644 src/test/txdownload_tests.cpp diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index c23fbae92fe..c376c1905a2 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -125,6 +125,7 @@ add_executable(test_bitcoin torcontrol_tests.cpp transaction_tests.cpp translation_tests.cpp + txdownload_tests.cpp txindex_tests.cpp txpackage_tests.cpp txreconciliation_tests.cpp diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp new file mode 100644 index 00000000000..3eb57a63537 --- /dev/null +++ b/src/test/txdownload_tests.cpp @@ -0,0 +1,337 @@ +// Copyright (c) 2011-2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include