Merge bitcoin/bitcoin#30110: refactor: TxDownloadManager + fuzzing

0f4bc63 [fuzz] txdownloadman and txdownload_impl (glozow)
699643f [unit test] MempoolRejectedTx (glozow)
fa584cb [p2p] add TxDownloadOptions bool to make TxRequestTracker deterministic (glozow)
f803c8c [p2p] filter 1p1c for child txid in recent rejects (glozow)
5269d57 [p2p] don't process orphan if in recent rejects (glozow)
2266eba [p2p] don't find 1p1cs for reconsiderable txns that are AlreadyHaveTx (glozow)
fa7027d [refactor] add CheckIsEmpty and GetOrphanTransactions, remove access to TxDownloadMan internals (glozow)
969b072 [refactor] wrap {Have,Get}TxToReconsider in txdownload (glozow)
f150fb9 [refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownloadImpl (glozow)
1e08195 [refactor] move new tx logic to txdownload (glozow)
257568e [refactor] move invalid package processing to TxDownload (glozow)
c4ce0c1 [refactor] move invalid tx processing to TxDownload (glozow)
c6b2174 [refactor] move valid tx processing to TxDownload (glozow)
a8cf3b6 [refactor] move Find1P1CPackage to txdownload (glozow)
f497414 [refactor] put peerman tasks at the end of ProcessInvalidTx (glozow)
6797bc4 [p2p] restrict RecursiveDynamicUsage of orphans added to vExtraTxnForCompact (glozow)
798cc8f [refactor] move Find1P1CPackage into ProcessInvalidTx (glozow)
416fbc9 [refactor] move new orphan handling to ProcessInvalidTx (glozow)
c8e67b9 [refactor] move ProcessInvalidTx and ProcessValidTx definitions down (glozow)
3a41926 [refactor] move notfound processing to txdownload (glozow)
042a97c [refactor] move tx inv/getdata handling to txdownload (glozow)
58e09f2 [p2p] don't log tx invs when in IBD (glozow)
2888653 [refactor] rename maybe_add_extra_compact_tx to first_time_failure (glozow)
f48d36c [refactor] move peer (dis)connection logic to TxDownload (glozow)
f61d9e4 [refactor] move AlreadyHaveTx to TxDownload (glozow)
84e4ef8 [txdownload] add read-only reference to mempool (glozow)
af91834 [refactor] move ValidationInterface functions to TxDownloadManager (glozow)
f6c860e [doc] fix typo in m_lazy_recent_confirmed_transactions doc (glozow)
5f9004e [refactor] add TxDownloadManager wrapping TxOrphanage, TxRequestTracker, and bloom filters (glozow)

Pull request description:

  Part of #27463.

  This PR does 3 things:

  (1) It modularizes transaction download logic into a `TxDownloadManager`. Transaction download logic refers to the process of deciding what transactions to request, download, and validate.[1] There should be no behavior changes. Using `--color_moved=dimmed_zebra -w` may help.
  (2) It adds unit and fuzz (:magic_wand:) testing for transaction download.
  (3) It makes a few small behavioral changes:
  - Stop (debug-only) logging tx invs during IBD
  - Just like all other transactions, require orphans have RecursiveDynamicUsage < 100k before adding to vExtraTxnForCompact
  - Don't return a 1p1c that contains a parent or child in recent rejects. Don't process any orphan already in recent rejects. These cases should not happen in actual node operation; it's just to allow tighter sanity checks during fuzzing.

  There are several benefits to this interface, such as:
  - Unit test coverage and fuzzing for logic that currently isn't feasible to test as thoroughly (without lots of overhead) and/or currently only lightly tested through `assert_debug_log` (not good) in functional tests.
  - When we add more functionality (e.g. package relay messages, more robust orphan handling), the vast majority of it will be within `TxDownloadManager` instead of `PeerManager`, making it easier to review and test. See #28031 for what this looks like.
  - `PeerManager` will no longer know anything about / have access to `TxOrphanage`, `TxRequestTracker` or the rejection caches. Its primary interface with `TxDownloadManager` would be much simpler:
      - Passing on  `ValidationInterface` callbacks
      - Telling `txdownloadman` when a peer {connects, disconnects}
      - Telling `txdownloadman`when a {transaction, package} is {accepted, rejected} from mempool
      - Telling `txdownloadman` when invs, notfounds, and txs are received.
      - Getting instructions on what to download.
      - Getting instructions on what {transactions, packages, orphans} to validate.
      - Get whether a peer `HaveMoreWork` for the `ProessMessages` loop
  - (todo) Thread-safety can be handled internally.

  [1]: This module is concerned with tx *download*, not upload. It excludes transaction announcements/gossip which happens after we download/accept a transaction. Txreconciliation (erlay) is excluded from this module, as it only relates to deciding which `inv`s to send or helping the other peer decide which `inv`s to send. It is independent from this logic.

ACKs for top commit:
  achow101:
    light ACK 0f4bc63
  theStack:
    ACK 0f4bc63
  instagibbs:
    reACK 0f4bc63
  naumenkogs:
    ACK 0f4bc63

Tree-SHA512: 84ab8ef8a0fc705eb829d7f7d6885f28944aaa42b03172f256a42605677b3e783919bb900d4e3b8589f85a0c387dfbd972bcd61d252d44a88c6aaa90e4bf920f

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

src/node/txdownloadman.h

@@ -0,0 +1,178 @@
+// 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 <net.h>
+#include <policy/packages.h>
+#include <txorphanage.h>
+
+#include <cstdint>
+#include <memory>
+
+class CBlock;
+class CRollingBloomFilter;
+class CTxMemPool;
+class GenTxid;
+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;
+    /** RNG provided by caller. */
+    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. */
+    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;
+};
+struct PackageToValidate {
+    Package m_txns;
+    std::vector<NodeId> 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)} {}
+    // Copy ctor
+    PackageToValidate(const PackageToValidate& other) = default;
+
+    // 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());
+    }
+};
+struct RejectedTxTodo
+{
+    bool m_should_add_extra_compact_tx;
+    std::vector<uint256> m_unique_parents;
+    std::optional<PackageToValidate> m_package_to_validate;
+};
+
+
+/**
+ * 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<TxDownloadManagerImpl> m_impl;
+
+public:
+    explicit TxDownloadManager(const TxDownloadOptions& options);
+    ~TxDownloadManager();
+
+    // 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();
+
+    /** 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);
+
+    /** 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<GenTxid> 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<uint256>& txhashes);
+
+    /** 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);
+
+    /** 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<bool, std::optional<PackageToValidate>> 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);
+
+    /** 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<TxOrphanage::OrphanTxBase> GetOrphanTransactions() const;
+};
+} // namespace node
+#endif // BITCOIN_NODE_TXDOWNLOADMAN_H