Merge bitcoin/bitcoin#36077: bugfix: give TxDownloadManager its own RNG
What the developer wrote
Authored by merge-script
Merge bitcoin/bitcoin#36077: bugfix: give TxDownloadManager its own RNG
80eaa6cabf28b7d68c61fc5aced189b36970f17e bugfix: give TxDownloadManager its own RNG (Greg Sanders)
Pull request description:
TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng,
which is non-thread-safe and guarded by g_msgproc_mutex.
BlockConnected runs on the validation background thread while holding
only m_tx_download_mutex. Reconsidering an orphan with multiple
announcers could therefore use m_rng concurrently with message
processing.
Regression introduced in #35986
Added a regression test on second commit, can remove it from the PR if deemed superfluous.
This is a Project Loupe find.
ACKs for top commit:
maflcko:
review ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e 🐓
hodlinator:
ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e
sedited:
ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e
Tree-SHA512: 2dbc4a9298bfa1375dc364ead4b1ec74c2ebe54fb7c311180fa06fc32240406be2979a2dd6ae0e7a23b099ddcd63f5c76c126d84a1405fc7eacd337eb009dd88
What changed, and why it matters
This fix gives a network transaction-download component its own random-number generator instead of borrowing one from another part of the program. The borrowed generator was only meant to be used while a specific message-processing lock was held, but a background validation thread could use it at the same time without that lock. That is a race condition: two threads updating the same internal state unpredictably. In practice this could corrupt the random generator's state, cause crashes, or make transaction-download behavior unreliable, but it is not a direct theft-of-coins bug and would be hard to trigger on demand.
Apply the patch. It is a targeted, low-risk refactor that removes a real data race. Operators running nodes built from commits between #35986 and this fix should upgrade, though no emergency response is warranted because exploitation is not straightforward and the bug is more reliability/integrity than direct fund theft.
Security signals we found
Concurrent use of non-thread-safe FastRandomContext across threads
Missing lock synchronization between message-processing and validation background threads
Regression introduced by prior refactor (#35986)
Fix includes regression test
Disclosed as a Project Loupe find
Evidence from the diff
TxDownloadManagerImpl previously held a reference to PeerManagerImpl::m_rng, a FastRandomContext guarded by g_msgproc_mutex. BlockConnected runs on the validation background thread while holding only m_tx_download_mutex, and calls AddChildrenToWorkSet which uses that shared RNG. This creates a data race on the RNG state between message processing and validation threads. The patch removes the borrowed RNG from TxDownloadOptions and gives TxDownloadManagerImpl its own FastRandomContext member, initialized deterministically for tests. The change also replaces m_opts.m_mempool with a direct m_mempool reference, but the security-relevant fix is the RNG isolation.
Changed components
src/net_processing.cppsrc/node/txdownloadman.hsrc/node/txdownloadman_impl.cppsrc/node/txdownloadman_impl.hFastRandomContext / random number generationTxDownloadManager / orphan transaction processingBlockConnected validation background threadInspect captured patch +18 / −17
### src/net_processing.cpp
@@ -2137,7 +2137,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
m_banman(banman),
m_chainman(chainman),
m_mempool(pool),
- m_txdownloadman(node::TxDownloadOptions{pool, m_rng, opts.deterministic_rng}),
+ m_txdownloadman{node::TxDownloadOptions{pool, opts.deterministic_rng}},
m_warnings{warnings},
m_opts{opts},
m_inbound_inv_bucket(/*rate=*/m_opts.tx_send_rate, /*mult=*/1.0),
### src/node/txdownloadman.h
@@ -39,8 +39,6 @@ inline constexpr auto GETDATA_TX_INTERVAL{60s};
struct TxDownloadOptions {
/** Read-only reference to mempool. */
const CTxMemPool& m_mempool;
- /** RNG provided by caller. */
- FastRandomContext& m_rng;
/** Instantiate TxRequestTracker as deterministic (used for tests). */
bool m_deterministic_txrequest{false};
};
### src/node/txdownloadman_impl.cpp
@@ -101,7 +101,7 @@ void TxDownloadManagerImpl::BlockConnected(const std::shared_ptr<const CBlock>&
for (const auto& ptx : pblock->vtx) {
// Reconsider potential child transactions.
- m_orphanage->AddChildrenToWorkSet(*ptx, m_opts.m_rng);
+ m_orphanage->AddChildrenToWorkSet(*ptx, m_rng);
RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256());
if (ptx->HasWitness()) {
@@ -146,7 +146,7 @@ bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_rec
if (RecentConfirmedTransactionsFilter().contains(hash)) return true;
- return RecentRejectsFilter().contains(hash) || std::visit([&](const auto& id) { return m_opts.m_mempool.exists(id); }, gtxid);
+ return RecentRejectsFilter().contains(hash) || std::visit([&](const auto& id) { return m_mempool.exists(id); }, gtxid);
}
void TxDownloadManagerImpl::ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info)
@@ -330,7 +330,7 @@ void TxDownloadManagerImpl::MempoolAcceptedTx(const CTransactionRef& tx)
m_txrequest.ForgetTxHash(tx->GetHash().ToUint256());
m_txrequest.ForgetTxHash(tx->GetWitnessHash().ToUint256());
- m_orphanage->AddChildrenToWorkSet(*tx, m_opts.m_rng);
+ m_orphanage->AddChildrenToWorkSet(*tx, m_rng);
// If it came from the orphanage, remove it. No-op if the tx is not in txorphanage.
m_orphanage->EraseTx(tx->GetWitnessHash());
}
@@ -380,7 +380,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
fRejectedParents = true;
break;
} else if (RecentRejectsReconsiderableFilter().contains(parent_txid.ToUint256()) &&
- !m_opts.m_mempool.exists(parent_txid)) {
+ !m_mempool.exists(parent_txid)) {
// More than 1 parent in m_lazy_recent_rejects_reconsiderable: 1p1c will not be
// sufficient to accept this package, so just give up here.
if (rejected_parent_reconsiderable.has_value()) {
### src/node/txdownloadman_impl.h
@@ -13,13 +13,15 @@
#include <node/txorphanage.h>
#include <primitives/transaction.h>
#include <policy/packages.h>
+#include <random.h>
#include <txrequest.h>
class CTxMemPool;
namespace node {
class TxDownloadManagerImpl {
public:
- TxDownloadOptions m_opts;
+ const CTxMemPool& m_mempool;
+ FastRandomContext m_rng;
/** Manages unvalidated tx data (orphan transactions for which we are downloading ancestors). */
std::unique_ptr<TxOrphanage> m_orphanage;
@@ -128,7 +130,12 @@ class TxDownloadManagerImpl {
return *m_lazy_recent_confirmed_transactions;
}
- TxDownloadManagerImpl(const TxDownloadOptions& options) : m_opts{options}, m_orphanage{MakeTxOrphanage()}, m_txrequest{options.m_deterministic_txrequest} {}
+ TxDownloadManagerImpl(const TxDownloadOptions& options)
+ : m_mempool{options.m_mempool},
+ m_rng{options.m_deterministic_txrequest},
+ m_orphanage{MakeTxOrphanage()},
+ m_txrequest{options.m_deterministic_txrequest}
+ {}
struct PeerInfo {
/** Information relevant to scheduling tx requests. */
### src/test/fuzz/txdownloadman.cpp
@@ -174,8 +174,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize)
// Initialize txdownloadman
bilingual_str error;
CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
- FastRandomContext det_rand{true};
- node::TxDownloadManager txdownloadman{node::TxDownloadOptions{pool, det_rand, true}};
+ node::TxDownloadManager txdownloadman{node::TxDownloadOptions{.m_mempool = pool, .m_deterministic_txrequest = true}};
std::chrono::microseconds time{244466666};
@@ -298,8 +297,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize)
// Initialize a TxDownloadManagerImpl
bilingual_str error;
CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
- FastRandomContext det_rand{true};
- node::TxDownloadManagerImpl txdownload_impl{node::TxDownloadOptions{pool, det_rand, true}};
+ node::TxDownloadManagerImpl txdownload_impl{node::TxDownloadOptions{.m_mempool = pool, .m_deterministic_txrequest = true}};
std::chrono::microseconds time{244466666};
### src/test/txdownload_tests.cpp
@@ -114,8 +114,7 @@ static CTransactionRef CreatePlaceholderTx(bool segwit)
BOOST_FIXTURE_TEST_CASE(tx_rejection_types, TestChain100Setup)
{
CTxMemPool& pool = *Assert(m_node.mempool);
- FastRandomContext det_rand{true};
- node::TxDownloadOptions DEFAULT_OPTS{pool, det_rand, true};
+ node::TxDownloadOptions DEFAULT_OPTS{.m_mempool = pool, .m_deterministic_txrequest = true};
// A new TxDownloadManagerImpl is created for each tx so we can just reuse the same one.
TxValidationState state;
@@ -172,8 +171,7 @@ BOOST_FIXTURE_TEST_CASE(tx_rejection_types, TestChain100Setup)
BOOST_FIXTURE_TEST_CASE(handle_missing_inputs, TestChain100Setup)
{
CTxMemPool& pool = *Assert(m_node.mempool);
- FastRandomContext det_rand{true};
- node::TxDownloadOptions DEFAULT_OPTS{pool, det_rand, true};
+ node::TxDownloadOptions DEFAULT_OPTS{.m_mempool = pool, .m_deterministic_txrequest = true};
NodeId nodeid{1};
node::TxDownloadConnectionInfo DEFAULT_CONN{/*m_preferred=*/false, /*m_relay_permissions=*/false, /*m_wtxid_relay=*/true};
Why this scored 49/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.