refactor: TxDownloadManager + fuzzing #30110

27+static constexpr auto NONPREF_PEER_TX_DELAY{2s};
28+/** How long to delay requesting transactions from overloaded peers (see MAX_PEER_TX_REQUEST_IN_FLIGHT). */
29+static constexpr auto OVERLOADED_PEER_TX_DELAY{2s};
30+/** How long to wait before downloading a transaction from an additional peer */
31+static constexpr auto GETDATA_TX_INTERVAL{60s};
32+struct TxDownloadOptions {

50+    m_recent_confirmed_transactions.reset();
51+}
52+
53+bool TxDownloadImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable)
54+{
55+    const uint256& hash = gtxid.GetHash();

11+#include <validationinterface.h>
12+
13 namespace node {
14+void TxDownloadImpl::ActiveTipChange()
15+{
16+    // If the chain tip has changed, previously rejected transactions might now be invalid, e.g. due

75@@ -75,4 +76,23 @@ bool TxDownloadImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsider
76 
77     return RecentRejectsFilter().contains(hash) || m_opts.m_mempool.exists(gtxid);
78 }
79+
80+void TxDownloadImpl::ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info)
81+{
82+    // If already connected (shouldn't happen in practice), exit early.
83+    if (m_peer_info.count(nodeid) > 0) return;

4072-                if (!fAlreadyHave && !m_chainman.IsInitialBlockDownload()) {
4073-                    AddTxAnnouncement(pfrom, gtxid, current_time);
4074+                if (!m_chainman.IsInitialBlockDownload()) {
4075+                    const bool fAlreadyHave{m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time, /*p2p_inv=*/true)};
4076+                    LogPrint(BCLog::NET, "got inv: %s  %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId());
4077                 }

4477@@ -4531,7 +4478,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4478                     AddKnownTx(*peer, parent_txid);
4479                     // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been
4480                     // previously rejected for being too low feerate. This orphan might CPFP it.
4481-                    if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) AddTxAnnouncement(pfrom, gtxid, current_time);
4482+                    if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
4483+                        m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time, /*p2p_inv=*/false);

43@@ -44,4 +44,33 @@ void TxDownloadImpl::BlockDisconnected()
44     // should be just after a new block containing it is found.
45     RecentConfirmedTransactionsFilter().reset();
46 }
47+
48+bool TxDownloadImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable)

3839@@ -3841,6 +3840,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3840                        tx_relay->m_next_inv_send_time == 0s));
3841         }
3842 
3843+        LOCK2(::cs_main, m_tx_download_mutex);

3006         ptx->GetWitnessHash().ToString(),
3007         nodeid,
3008         state.ToString());
3009 
3010-    if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
3011+    // Only process a new orphan if maybe_add_extra_compact_tx, as otherwise it must be either

3078+            // parents so avoid re-requesting it from other peers.
3079+            // Here we add both the txid and the wtxid, as we know that
3080+            // regardless of what witness is provided, we will not accept
3081+            // this, so we don't need to allow for redownload of this txid
3082+            // from any of our non-wtxidrelay peers.
3083+            RecentRejectsFilter().insert(tx.GetHash().ToUint256());

603@@ -594,8 +604,8 @@ class PeerManagerImpl final : public PeerManager
604      *                                            Set to false if the tx has already been rejected before,
605      *                                            e.g. is an orphan, to avoid adding duplicate entries.
606      * Updates m_txrequest, m_lazy_recent_rejects, m_lazy_recent_rejects_reconsiderable, m_orphanage, and vExtraTxnForCompact. */
607-    void ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result,
608-                          bool maybe_add_extra_compact_tx)
609+    std::optional<PackageToValidate> ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result,

3116@@ -3104,6 +3117,14 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx
3117             // because we should not download or submit this transaction by itself again, but may
3118             // submit it as part of a package later.
3119             RecentRejectsReconsiderableFilter().insert(ptx->GetWitnessHash().ToUint256());
3120+
3121+            if (maybe_add_extra_compact_tx) {

3138-    m_txrequest.ForgetTxHash(tx->GetWitnessHash());
3139-
3140-    m_orphanage.AddChildrenToWorkSet(*tx);
3141-    // If it came from the orphanage, remove it. No-op if the tx is not in txorphanage.
3142-    m_orphanage.EraseTx(tx->GetWitnessHash());
3143+    m_txdownloadman.MempoolAcceptedTx(tx);

4231-        // already; and an adversary can already relay us old transactions
4232-        // (older than our recency filter) if trying to DoS us, without any need
4233-        // for witness malleation.
4234-        if (m_txdownloadman.AlreadyHaveTx(GenTxid::Wtxid(wtxid), /*include_reconsiderable=*/true)) {
4235+        const auto& [should_validate, package_to_validate] = m_txdownloadman.ReceivedTx(pfrom.GetId(), ptx);
4236+        if (!should_validate) {

403+            LogPrint(BCLog::TXPACKAGES, "found tx %s (wtxid=%s) in reconsiderable rejects, looking for child in orphanage\n",
404+                     txid.ToString(), wtxid.ToString());
405+            return std::make_pair(false, Find1P1CPackage(ptx, nodeid));
406+        }
407+
408+        // If a tx is detected by m_lazy_recent_rejects it is ignored. Because we haven't

424@@ -425,4 +425,15 @@ std::pair<bool, std::optional<PackageToValidate>> TxDownloadImpl::ReceivedTx(Nod
425 
426     return std::make_pair(true, std::nullopt);
427 }
428+
429+bool TxDownloadImpl::HaveMoreWork(NodeId nodeid)

435@@ -436,4 +436,13 @@ CTransactionRef TxDownloadImpl::GetTxToReconsider(NodeId nodeid)
436     return m_orphanage.GetTxToReconsider(nodeid);
437 }
438 
439+void TxDownloadImpl::CheckIsEmpty(NodeId nodeid)

65+    // Ensure every transaction has a different txid by having each one spend the previous one.
66+    static Txid prevout_hash{};
67+
68+    std::vector<CTransactionRef> txns;
69+    txns.reserve(num_txns);
70+    // Simplest spk for every tx

82+    return txns;
83+}
84+static CTransactionRef GetNewTransaction()
85+{
86+    // Create 2 transactions and return the second one. Transactions are created as a chain to
87+    // ensure uniqueness. Here, we want to avoid direct parent/child relationships between transactions.

 96+
 97+    TxValidationState state;
 98+
 99+    for (const auto& [result, expected_behavior] : expected_behaviors) {
100+        node::TxDownloadImpl txdownload_impl{node::TxDownloadOptions{pool, det_rand, DEFAULT_MAX_ORPHAN_TRANSACTIONS}};
101+        const auto ptx = GetNewTransaction();

81+    }
82+    return txns;
83+}
84+static CTransactionRef GetNewTransaction()
85+{
86+    // Create 2 transactions and return the second one. Transactions are created as a chain to

149+    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
150+    const auto max_orphan_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 300);
151+    FastRandomContext det_rand{true};
152+    node::TxDownloadManager txdownloadman{node::TxDownloadOptions{pool, det_rand, max_orphan_count}};
153+
154+    std::chrono::microseconds time{244466666};

158+        NodeId rand_peer = fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(0, NUM_PEERS);
159+
160+        // Transaction can be one of the premade ones or a randomly generated one
161+        auto rand_tx = fuzzed_data_provider.ConsumeBool() ?
162+            MakeTransactionSpending({fuzzed_data_provider.PickValueInArray(COINS)},
163+                                    /*num_outputs=*/fuzzed_data_provider.ConsumeIntegralInRange(1, 500),

210+                txdownloadman.ReceivedTx(rand_peer, rand_tx);
211+                const auto& [should_validate, maybe_package] = txdownloadman.ReceivedTx(rand_peer, rand_tx);
212+                Assert(should_validate || !maybe_package.has_value());
213+                if (maybe_package.has_value()) {
214+                    Assert(maybe_package->m_senders.size() == 2);
215+                    Assert(maybe_package->m_senders.front() == rand_peer);

387+                    txdownload_impl.MempoolRejectedTx(ptx, state_missing_inputs, rand_peer, fuzzed_data_provider.ConsumeBool());
388+                }
389+            }
390+        );
391+
392+        // Jump ahead in time

300+                txdownload_impl.DisconnectedPeer(rand_peer);
301+                txdownload_impl.CheckIsEmpty(rand_peer);
302+            },
303+            [&] {
304+                txdownload_impl.ActiveTipChange();
305+                // After a block update, nothing should be in the rejection caches

257+    // We should never have more than the maximum in-flight requests out for a peer.
258+    for (NodeId peer = 0; peer < NUM_PEERS; ++peer) {
259+        if (!HasRelayPermissions(peer)) {
260+            Assert(txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT);
261+        }
262+    }

323+            [&] {
324+                txdownload_impl.MempoolAcceptedTx(rand_tx);
325+            },
326+            [&] {
327+                TxValidationState state;
328+                state.Invalid(fuzzed_data_provider.PickValueInArray(TESTED_TX_RESULTS), "");

353+                    Assert(maybe_package->m_senders.size() == 2);
354+                    Assert(maybe_package->m_senders.front() == rand_peer);
355+
356+                    const auto& package = maybe_package->m_txns;
357+                    Assert(package.size() == 2);
358+                    // Parent is in m_lazy_recent_rejects_reconsiderable and child is in m_orphanage

369+                txdownload_impl.ReceivedNotFound(rand_peer, {rand_tx->GetWitnessHash()});
370+            },
371+            [&] {
372+                const bool expect_work{txdownload_impl.HaveMoreWork(rand_peer)};
373+                const auto ptx = txdownload_impl.GetTxToReconsider(rand_peer);
374+                // expect_work=true doesn't necessarily mean the next item from the workset isn't a

165+    TestPeer peer;
166+    peer.m_nodeid = nodeid;
167+    peer.m_good = force_good || fuzzed_data_provider.ConsumeBool();
168+    peer.m_connection_preferred = preferred;
169+    peer.m_relay_permissions = false;
170+    peer.m_wtxid_relay = force_good || fuzzed_data_provider.ConsumeBool();

109+    bool m_good;
110+
111+    // for TxDownloadConnectionInfo
112+    // Note that this is used to indicate when somebody is an outbound peer.
113+    bool m_connection_preferred{false};
114+    bool m_relay_permissions{false};

227+
228+    // Reconsiderable parent of a tx already in orphanage: the only time PackageToValidate is returned.
229+    {
230+        node::TxDownloadImpl txdownload_impl{node::TxDownloadOptions{pool, det_rand, DEFAULT_MAX_ORPHAN_TRANSACTIONS}};
231+
232+        // Child missing inputs, should be added to orphanage.

207+                txdownloadman.GetRequestsToSend(rand_peer, time);
208+            },
209+            [&] {
210+                txdownloadman.ReceivedTx(rand_peer, rand_tx);
211+                const auto& [should_validate, maybe_package] = txdownloadman.ReceivedTx(rand_peer, rand_tx);
212+                Assert(should_validate || !maybe_package.has_value());

 96+    std::optional<PackageToValidate> m_package_to_validate;
 97+};
 98+
 99+
100+class TxDownloadManager {
101+    const std::unique_ptr<TxDownloadImpl> m_impl;

221+// malicious, we'll always be able to download a transaction.
222+// Connect to 8 outbounds and 0-120 inbound peers. 1 outbound is guaranteed to be good (see m_good
223+// above for definition), while the rest may or may not be.
224+// A non-good node has a superset of the actions available to a good node: it may stall a getdata,
225+// send a malleated or witness stripped tx/inv, send a notfound, etc.
226+FUZZ_TARGET(tx_download_one_honest_peer, .init = initialize)

275+        }
276+    }
277+    txdownload_impl.m_txrequest.SanityCheck();
278+}
279+
280+FUZZ_TARGET(txdownload_impl, .init = initialize)

279+    // within the first round and will never be disconnected.
280+    int counter = 0;
281+
282+    LIMITED_WHILE(!got_tx, 100000)
283+    {
284+        std::shuffle(indexes.begin(), indexes.end(), det_rand);

10+TxDownloadManager::TxDownloadManager(const TxDownloadOptions& options) :
11+    m_impl{std::make_unique<TxDownloadImpl>(options)}
12+{}
13+TxDownloadManager::~TxDownloadManager() = default;
14+
15+void TxDownloadManager::ActiveTipChange()

3082             }
3083 
3084-            if (m_orphanage.AddTx(ptx, nodeid)) {
3085-                AddToCompactExtraTransactions(ptx);
3086-            }
3087+            add_extra_compact_tx &= m_orphanage.AddTx(ptx, nodeid);

11+class TxOrphanage;
12+class TxRequestTracker;
13+class CRollingBloomFilter;
14+
15+namespace node {
16+class TxDownloadImpl;

0@@ -0,0 +1,34 @@
1+// Copyright (c) 2024
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+

1581@@ -1672,6 +1582,8 @@ void PeerManagerImpl::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid,
1582     AssertLockHeld(::cs_main); // for State
1583     AssertLockHeld(m_tx_download_mutex); // For m_txrequest

94+
95+    // A new TxDownloadManagerImpl is created for each tx so we can just reuse the same one.
96+    TxValidationState state;
97+    NodeId nodeid{0};
98+    std::chrono::microseconds now{GetTime()};
99+    node::TxDownloadConnectionInfo connection_info{/*m_preferred=*/false, /*m_relay_permissions=*/false, /*m_wtxid_relay=*/true};

39+    NOTFOUND,
40+};
41+
42+// What a peer may send in response to a getdata.
43+enum class PossibleResponse {
44+    /** Send the target transaction, in response to getdata */

5146+                    tx_invs.emplace_back(inv.hash);
5147                 }
5148             }
5149         }
5150+        LOCK(m_tx_download_mutex);
5151+        m_txdownloadman.ReceivedNotFound(pfrom.GetId(), tx_invs);

113+    std::unique_ptr<CRollingBloomFilter> m_lazy_recent_confirmed_transactions{nullptr};
114+
115+    CRollingBloomFilter& RecentConfirmedTransactionsFilter()
116+    {
117+        if (!m_lazy_recent_confirmed_transactions) {
118+            m_lazy_recent_confirmed_transactions = std::make_unique<CRollingBloomFilter>(120'000, 0.000'001);

206@@ -203,4 +207,13 @@ std::vector<GenTxid> TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std
207     }
208     return requests;
209 }
210+
211+void TxDownloadManagerImpl::ReceivedNotFound(NodeId nodeid, const std::vector<uint256>& txhashes)
212+{
213+    for (const auto& txhash: txhashes) {

450+    const uint256& txid = ptx->GetHash();
451+    const uint256& wtxid = ptx->GetWitnessHash();
452+
453+    // Mark that we have received a response
454+    m_txrequest.ReceivedResponse(nodeid, ptx->GetHash());
455+    if (ptx->HasWitness()) m_txrequest.ReceivedResponse(nodeid, ptx->GetWitnessHash());

447+}
448+
449+std::pair<bool, std::optional<PackageToValidate>> TxDownloadManagerImpl::ReceivedTx(NodeId nodeid, const CTransactionRef& ptx)
450+{
451+    const uint256& txid = ptx->GetHash();
452+    const uint256& wtxid = ptx->GetWitnessHash();

378+
379+                    const auto& package = maybe_package->m_txns;
380+                    // Parent is in m_lazy_recent_rejects_reconsiderable and child is in m_orphanage
381+                    Assert(txdownload_impl.RecentRejectsReconsiderableFilter().contains(rand_tx->GetWitnessHash().ToUint256()));
382+                    Assert(txdownload_impl.m_orphanage.HaveTx(maybe_package->m_txns.back()->GetWitnessHash()));
383+                    // Package has not been rejected

331+                    if (rejected_parent_reconsiderable.has_value()) {
332+                        fRejectedParents = true;
333+                        break;
334+                    }
335+                }
336+                rejected_parent_reconsiderable = parent_txid;

567@@ -583,53 +568,29 @@ class PeerManagerImpl final : public PeerManager
568     bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);
569 
570     /** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID.
571-     * @param[in]   maybe_add_extra_compact_tx    Whether this tx should be added to vExtraTxnForCompact.
572+     * @param[in]   first_time_failure            Whether we should consider inserting into vExtraTxnForCompact, adding

4281-            // regardless of false positives.
4282             return;
4283         }
4284 
4285+        // ReceivedTx should not be telling us to validate the tx and a package.
4286+        Assume(!package_to_validate.has_value());

4868@@ -5321,16 +4869,18 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4869     if (msg_type == NetMsgType::NOTFOUND) {
4870         std::vector<CInv> vInv;
4871         vRecv >> vInv;
4872-        if (vInv.size() <= MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
4873-            LOCK(m_tx_download_mutex);
4874+        std::vector<uint256> tx_invs;
4875+        if (vInv.size() <= node::MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
4876             for (CInv &inv : vInv) {

5915-                    m_txrequest.RequestedTx(pto->GetId(), gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL);
5916-                } else {
5917-                    // We have already seen this transaction, no need to download. This is just a belt-and-suspenders, as
5918-                    // this should already be called whenever a transaction becomes AlreadyHaveTx().
5919-                    m_txrequest.ForgetTxHash(gtxid.GetHash());
5920+            for (const GenTxid& gtxid : m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time)) {

76+    // Move assignment
77+    PackageToValidate& operator=(PackageToValidate&& other) {
78+        this->m_txns = std::move(other.m_txns);
79+        this->m_senders = std::move(other.m_senders);
80+        return *this;
81+    }

116+
117+    /** Deletes all txrequest announcements and orphans for a given peer. */
118+    void DisconnectedPeer(NodeId nodeid);
119+
120+    /** New inv has been received. May be added as a candidate to txrequest.
121+     * @param[in] p2p_inv     When true, only add this announcement if we don't already have the tx.

148+}
149+
150+void TxDownloadManagerImpl::ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info)
151+{
152+    // If already connected (shouldn't happen in practice), exit early.
153+    if (m_peer_info.contains(nodeid)) return;

162+    m_txrequest.DisconnectedPeer(nodeid);
163+
164+    if (m_peer_info.contains(nodeid)) {
165+        if (m_peer_info.at(nodeid).m_connection_info.m_wtxid_relay) m_num_wtxid_peers -= 1;
166+        m_peer_info.erase(nodeid);
167+    }

173+    // If this is a request for the parent of an orphan, we don't drop transactions that we already have. In particular,
174+    // we *do* want to request parents that are in m_lazy_recent_rejects_reconsiderable, since they can be CPFP'd.
175+    if (p2p_inv && AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) return true;
176+
177+    if (m_peer_info.count(peer) == 0) return false;
178+    const auto& info = m_peer_info.at(peer).m_connection_info;

270+    std::shuffle(tx_indices.begin(), tx_indices.end(), m_opts.m_rng);
271+
272+    for (const auto index : tx_indices) {
273+        // If we already tried a package and failed for any reason, the combined hash was
274+        // cached in m_lazy_recent_rejects_reconsiderable.
275+        const auto [child_tx, child_sender] = cpfp_candidates_different_peer.at(index);

316+            for (const CTxIn& txin : tx.vin) {
317+                // We start with all parents, and then remove duplicates below.
318+                unique_parents.push_back(txin.prevout.hash);
319+            }
320+            std::sort(unique_parents.begin(), unique_parents.end());
321+            unique_parents.erase(std::unique(unique_parents.begin(), unique_parents.end()), unique_parents.end());