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 @@ private: /** 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 @@ private: 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 @@ public: 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 @@ public: 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