Skip to content

Commit 7b66815

Browse files
committed
Merge 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 bitcoin#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 bitcoin#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
2 parents dc97e7f + 0f4bc63 commit 7b66815

10 files changed

+1771
-549
lines changed

src/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,7 @@ add_library(bitcoin_node STATIC EXCLUDE_FROM_ALL
246246
node/psbt.cpp
247247
node/timeoffsets.cpp
248248
node/transaction.cpp
249+
node/txdownloadman_impl.cpp
249250
node/txreconciliation.cpp
250251
node/utxo_snapshot.cpp
251252
node/warnings.cpp

src/net_processing.cpp

Lines changed: 79 additions & 548 deletions
Large diffs are not rendered by default.

src/node/txdownloadman.h

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
// Copyright (c) 2024 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#ifndef BITCOIN_NODE_TXDOWNLOADMAN_H
6+
#define BITCOIN_NODE_TXDOWNLOADMAN_H
7+
8+
#include <net.h>
9+
#include <policy/packages.h>
10+
#include <txorphanage.h>
11+
12+
#include <cstdint>
13+
#include <memory>
14+
15+
class CBlock;
16+
class CRollingBloomFilter;
17+
class CTxMemPool;
18+
class GenTxid;
19+
class TxRequestTracker;
20+
namespace node {
21+
class TxDownloadManagerImpl;
22+
23+
/** Maximum number of in-flight transaction requests from a peer. It is not a hard limit, but the threshold at which
24+
* point the OVERLOADED_PEER_TX_DELAY kicks in. */
25+
static constexpr int32_t MAX_PEER_TX_REQUEST_IN_FLIGHT = 100;
26+
/** Maximum number of transactions to consider for requesting, per peer. It provides a reasonable DoS limit to
27+
* per-peer memory usage spent on announcements, while covering peers continuously sending INVs at the maximum
28+
* rate (by our own policy, see INVENTORY_BROADCAST_PER_SECOND) for several minutes, while not receiving
29+
* the actual transaction (from any peer) in response to requests for them. */
30+
static constexpr int32_t MAX_PEER_TX_ANNOUNCEMENTS = 5000;
31+
/** How long to delay requesting transactions via txids, if we have wtxid-relaying peers */
32+
static constexpr auto TXID_RELAY_DELAY{2s};
33+
/** How long to delay requesting transactions from non-preferred peers */
34+
static constexpr auto NONPREF_PEER_TX_DELAY{2s};
35+
/** How long to delay requesting transactions from overloaded peers (see MAX_PEER_TX_REQUEST_IN_FLIGHT). */
36+
static constexpr auto OVERLOADED_PEER_TX_DELAY{2s};
37+
/** How long to wait before downloading a transaction from an additional peer */
38+
static constexpr auto GETDATA_TX_INTERVAL{60s};
39+
struct TxDownloadOptions {
40+
/** Read-only reference to mempool. */
41+
const CTxMemPool& m_mempool;
42+
/** RNG provided by caller. */
43+
FastRandomContext& m_rng;
44+
/** Maximum number of transactions allowed in orphanage. */
45+
const uint32_t m_max_orphan_txs;
46+
/** Instantiate TxRequestTracker as deterministic (used for tests). */
47+
bool m_deterministic_txrequest{false};
48+
};
49+
struct TxDownloadConnectionInfo {
50+
/** Whether this peer is preferred for transaction download. */
51+
const bool m_preferred;
52+
/** Whether this peer has Relay permissions. */
53+
const bool m_relay_permissions;
54+
/** Whether this peer supports wtxid relay. */
55+
const bool m_wtxid_relay;
56+
};
57+
struct PackageToValidate {
58+
Package m_txns;
59+
std::vector<NodeId> m_senders;
60+
/** Construct a 1-parent-1-child package. */
61+
explicit PackageToValidate(const CTransactionRef& parent,
62+
const CTransactionRef& child,
63+
NodeId parent_sender,
64+
NodeId child_sender) :
65+
m_txns{parent, child},
66+
m_senders{parent_sender, child_sender}
67+
{}
68+
69+
// Move ctor
70+
PackageToValidate(PackageToValidate&& other) : m_txns{std::move(other.m_txns)}, m_senders{std::move(other.m_senders)} {}
71+
// Copy ctor
72+
PackageToValidate(const PackageToValidate& other) = default;
73+
74+
// Move assignment
75+
PackageToValidate& operator=(PackageToValidate&& other) {
76+
this->m_txns = std::move(other.m_txns);
77+
this->m_senders = std::move(other.m_senders);
78+
return *this;
79+
}
80+
81+
std::string ToString() const {
82+
Assume(m_txns.size() == 2);
83+
return strprintf("parent %s (wtxid=%s, sender=%d) + child %s (wtxid=%s, sender=%d)",
84+
m_txns.front()->GetHash().ToString(),
85+
m_txns.front()->GetWitnessHash().ToString(),
86+
m_senders.front(),
87+
m_txns.back()->GetHash().ToString(),
88+
m_txns.back()->GetWitnessHash().ToString(),
89+
m_senders.back());
90+
}
91+
};
92+
struct RejectedTxTodo
93+
{
94+
bool m_should_add_extra_compact_tx;
95+
std::vector<uint256> m_unique_parents;
96+
std::optional<PackageToValidate> m_package_to_validate;
97+
};
98+
99+
100+
/**
101+
* Class responsible for deciding what transactions to request and, once
102+
* downloaded, whether and how to validate them. It is also responsible for
103+
* deciding what transaction packages to validate and how to resolve orphan
104+
* transactions. Its data structures include TxRequestTracker for scheduling
105+
* requests, rolling bloom filters for remembering transactions that have
106+
* already been {accepted, rejected, confirmed}, an orphanage, and a registry of
107+
* each peer's transaction relay-related information.
108+
*
109+
* Caller needs to interact with TxDownloadManager:
110+
* - ValidationInterface callbacks.
111+
* - When a potential transaction relay peer connects or disconnects.
112+
* - When a transaction or package is accepted or rejected from mempool
113+
* - When a inv, notfound, or tx message is received
114+
* - To get instructions for which getdata messages to send
115+
*
116+
* This class is not thread-safe. Access must be synchronized using an
117+
* external mutex.
118+
*/
119+
class TxDownloadManager {
120+
const std::unique_ptr<TxDownloadManagerImpl> m_impl;
121+
122+
public:
123+
explicit TxDownloadManager(const TxDownloadOptions& options);
124+
~TxDownloadManager();
125+
126+
// Responses to chain events. TxDownloadManager is not an actual client of ValidationInterface, these are called through PeerManager.
127+
void ActiveTipChange();
128+
void BlockConnected(const std::shared_ptr<const CBlock>& pblock);
129+
void BlockDisconnected();
130+
131+
/** Creates a new PeerInfo. Saves the connection info to calculate tx announcement delays later. */
132+
void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info);
133+
134+
/** Deletes all txrequest announcements and orphans for a given peer. */
135+
void DisconnectedPeer(NodeId nodeid);
136+
137+
/** New inv has been received. May be added as a candidate to txrequest.
138+
* @param[in] p2p_inv When true, only add this announcement if we don't already have the tx.
139+
* Returns true if this was a dropped inv (p2p_inv=true and we already have the tx), false otherwise. */
140+
bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv);
141+
142+
/** Get getdata requests to send. */
143+
std::vector<GenTxid> GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time);
144+
145+
/** Should be called when a notfound for a tx has been received. */
146+
void ReceivedNotFound(NodeId nodeid, const std::vector<uint256>& txhashes);
147+
148+
/** Respond to successful transaction submission to mempool */
149+
void MempoolAcceptedTx(const CTransactionRef& tx);
150+
151+
/** Respond to transaction rejected from mempool */
152+
RejectedTxTodo MempoolRejectedTx(const CTransactionRef& ptx, const TxValidationState& state, NodeId nodeid, bool first_time_failure);
153+
154+
/** Respond to package rejected from mempool */
155+
void MempoolRejectedPackage(const Package& package);
156+
157+
/** Marks a tx as ReceivedResponse in txrequest and checks whether AlreadyHaveTx.
158+
* Return a bool indicating whether this tx should be validated. If false, optionally, a
159+
* PackageToValidate. */
160+
std::pair<bool, std::optional<PackageToValidate>> ReceivedTx(NodeId nodeid, const CTransactionRef& ptx);
161+
162+
/** Whether there are any orphans to reconsider for this peer. */
163+
bool HaveMoreWork(NodeId nodeid) const;
164+
165+
/** Returns next orphan tx to consider, or nullptr if none exist. */
166+
CTransactionRef GetTxToReconsider(NodeId nodeid);
167+
168+
/** Check that all data structures are empty. */
169+
void CheckIsEmpty() const;
170+
171+
/** Check that all data structures that track per-peer information have nothing for this peer. */
172+
void CheckIsEmpty(NodeId nodeid) const;
173+
174+
/** Wrapper for TxOrphanage::GetOrphanTransactions */
175+
std::vector<TxOrphanage::OrphanTxBase> GetOrphanTransactions() const;
176+
};
177+
} // namespace node
178+
#endif // BITCOIN_NODE_TXDOWNLOADMAN_H

0 commit comments

Comments
 (0)