[refactor] move AlreadyHaveTx to TxDownload

This is move-only.
Also delete external RecentConfirmedTransactionsFilter() access since it
is no longer necessary.

Author: glozow

src/net_processing.cpp

@@ -854,17 +854,6 @@ class PeerManagerImpl final : public PeerManager
     /** Stalling timeout for blocks in IBD */
     std::atomic<std::chrono::seconds> m_block_stalling_timeout{BLOCK_STALLING_TIMEOUT_DEFAULT};
 
-    /** 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());

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<const CBlock>& 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

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

src/node/txdownloadman_impl.h

@@ -9,6 +9,7 @@
 #include <common/bloom.h>
 #include <kernel/chain.h>
 #include <net.h>
+#include <primitives/transaction.h>
 #include <txorphanage.h>
 #include <txrequest.h>
 
@@ -130,6 +131,8 @@ class TxDownloadManagerImpl {
     void ActiveTipChange();
     void BlockConnected(const std::shared_ptr<const CBlock>& pblock);
     void BlockDisconnected();
+
+    bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable);
 };
 } // namespace node
 #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H