From 87c706713e5d1c78bad943a42bf7c69047d28ea5 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 2 Oct 2023 14:11:55 +0100 Subject: [PATCH 1/6] [net processing] PeerManager holds a FastRandomContext --- src/net_processing.cpp | 5 ++++- src/net_processing.h | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b046b3ac168..8963424c9be 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -695,6 +695,8 @@ class PeerManagerImpl final : public PeerManager /** Send `feefilter` message. */ void MaybeSendFeefilter(CNode& node, Peer& peer, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); + FastRandomContext m_rng GUARDED_BY(NetEventsInterface::g_msgproc_mutex); + const CChainParams& m_chainparams; CConnman& m_connman; AddrMan& m_addrman; @@ -1808,7 +1810,8 @@ std::unique_ptr PeerManager::make(CConnman& connman, AddrMan& addrm PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, CTxMemPool& pool, Options opts) - : m_chainparams(chainman.GetParams()), + : m_rng{opts.deterministic_rng}, + m_chainparams(chainman.GetParams()), m_connman(connman), m_addrman(addrman), m_banman(banman), diff --git a/src/net_processing.h b/src/net_processing.h index 837e3086173..80d07648a48 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -58,6 +58,9 @@ class PeerManager : public CValidationInterface, public NetEventsInterface uint32_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN}; //! Whether all P2P messages are captured to disk bool capture_messages{false}; + //! Whether or not the internal RNG behaves deterministically (this is + //! a test-only option). + bool deterministic_rng{false}; }; static std::unique_ptr make(CConnman& connman, AddrMan& addrman, From a648dd79e5ebfdb627d0221b1207862efb664dfc Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 2 Oct 2023 14:13:05 +0100 Subject: [PATCH 2/6] [net processing] PushAddress uses PeerManager's rng --- src/net_processing.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8963424c9be..77d7f168bf9 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1022,7 +1022,7 @@ class PeerManagerImpl final : public PeerManager bool SetupAddressRelay(const CNode& node, Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); void AddAddressKnown(Peer& peer, const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); - void PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& insecure_rand) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); + void PushAddress(Peer& peer, const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); }; const CNodeState* PeerManagerImpl::State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main) @@ -1054,7 +1054,7 @@ void PeerManagerImpl::AddAddressKnown(Peer& peer, const CAddress& addr) peer.m_addr_known->insert(addr.GetKey()); } -void PeerManagerImpl::PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& insecure_rand) +void PeerManagerImpl::PushAddress(Peer& peer, const CAddress& addr) { // Known checking here is only to save space from duplicates. // Before sending, we'll filter it again for known addresses that were @@ -1062,7 +1062,7 @@ void PeerManagerImpl::PushAddress(Peer& peer, const CAddress& addr, FastRandomCo assert(peer.m_addr_known); if (addr.IsValid() && !peer.m_addr_known->contains(addr.GetKey()) && IsAddrCompatible(peer, addr)) { if (peer.m_addrs_to_send.size() >= MAX_ADDR_TO_SEND) { - peer.m_addrs_to_send[insecure_rand.randrange(peer.m_addrs_to_send.size())] = addr; + peer.m_addrs_to_send[m_rng.randrange(peer.m_addrs_to_send.size())] = addr; } else { peer.m_addrs_to_send.push_back(addr); } @@ -2108,7 +2108,6 @@ void PeerManagerImpl::RelayAddress(NodeId originator, const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY) .Write(hash_addr) .Write(time_addr)}; - FastRandomContext insecure_rand; // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers. unsigned int nRelayNodes = (fReachable || (hasher.Finalize() & 1)) ? 2 : 1; @@ -2132,7 +2131,7 @@ void PeerManagerImpl::RelayAddress(NodeId originator, }; for (unsigned int i = 0; i < nRelayNodes && best[i].first != 0; i++) { - PushAddress(*best[i].second, addr, insecure_rand); + PushAddress(*best[i].second, addr); } } @@ -4657,9 +4656,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } else { vAddr = m_connman.GetAddresses(pfrom, MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND); } - FastRandomContext insecure_rand; for (const CAddress &addr : vAddr) { - PushAddress(*peer, addr, insecure_rand); + PushAddress(*peer, addr); } return; } @@ -5261,8 +5259,7 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros } if (std::optional local_service = GetLocalAddrForPeer(node)) { CAddress local_addr{*local_service, peer.m_our_services, Now()}; - FastRandomContext insecure_rand; - PushAddress(peer, local_addr, insecure_rand); + PushAddress(peer, local_addr); } peer.m_next_local_addr_send = GetExponentialRand(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); } From 77506f4ac6b3a3d7396a3a6101345019e05b3b10 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 2 Oct 2023 14:14:37 +0100 Subject: [PATCH 3/6] [net processing] Addr shuffle uses PeerManager's rng --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 77d7f168bf9..ca8bd482252 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3714,7 +3714,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const bool rate_limited = !pfrom.HasPermission(NetPermissionFlags::Addr); uint64_t num_proc = 0; uint64_t num_rate_limit = 0; - Shuffle(vAddr.begin(), vAddr.end(), FastRandomContext()); + Shuffle(vAddr.begin(), vAddr.end(), m_rng); for (CAddress& addr : vAddr) { if (interruptMsgProc) From 47520ed209d9341702a0fb6006bee6f63f7da42e Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 2 Oct 2023 14:28:21 +0100 Subject: [PATCH 4/6] [net processing] Make fee filter rounder non-global --- src/net_processing.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ca8bd482252..b38965c0b84 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -697,6 +697,8 @@ class PeerManagerImpl final : public PeerManager FastRandomContext m_rng GUARDED_BY(NetEventsInterface::g_msgproc_mutex); + FeeFilterRounder m_fee_filter_rounder GUARDED_BY(NetEventsInterface::g_msgproc_mutex); + const CChainParams& m_chainparams; CConnman& m_connman; AddrMan& m_addrman; @@ -1811,6 +1813,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, CTxMemPool& pool, Options opts) : m_rng{opts.deterministic_rng}, + m_fee_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}}, m_chainparams(chainman.GetParams()), m_connman(connman), m_addrman(addrman), @@ -5338,14 +5341,13 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi if (pto.IsBlockOnlyConn()) return; CAmount currentFilter = m_mempool.GetMinFee().GetFeePerK(); - static FeeFilterRounder g_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}}; if (m_chainman.IsInitialBlockDownload()) { // Received tx-inv messages are discarded when the active // chainstate is in IBD, so tell the peer to not send them. currentFilter = MAX_MONEY; } else { - static const CAmount MAX_FILTER{g_filter_rounder.round(MAX_MONEY)}; + static const CAmount MAX_FILTER{m_fee_filter_rounder.round(MAX_MONEY)}; if (peer.m_fee_filter_sent == MAX_FILTER) { // Send the current filter if we sent MAX_FILTER previously // and made it out of IBD. @@ -5353,7 +5355,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi } } if (current_time > peer.m_next_send_feefilter) { - CAmount filterToSend = g_filter_rounder.round(currentFilter); + CAmount filterToSend = m_fee_filter_rounder.round(currentFilter); // We always have a fee filter of at least the min relay fee filterToSend = std::max(filterToSend, m_mempool.m_min_relay_feerate.GetFeePerK()); if (filterToSend != peer.m_fee_filter_sent) { From fecec3e1c661ba273470ecc5ef12d4c070b53050 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 2 Oct 2023 14:21:35 +0100 Subject: [PATCH 5/6] [net processing] FeeFilterRounder doesn't own a FastRandomContext --- src/net_processing.cpp | 2 +- src/policy/fees.cpp | 5 +++-- src/policy/fees.h | 4 ++-- src/test/fuzz/fees.cpp | 3 ++- src/test/policy_fee_tests.cpp | 3 ++- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b38965c0b84..d58db134249 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1813,7 +1813,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, CTxMemPool& pool, Options opts) : m_rng{opts.deterministic_rng}, - m_fee_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}}, + m_fee_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}, m_rng}, m_chainparams(chainman.GetParams()), m_connman(connman), m_addrman(addrman), diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 553c88fddc3..87bfa4cfc30 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -1054,8 +1054,9 @@ static std::set MakeFeeSet(const CFeeRate& min_incremental_fee, return fee_set; } -FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee) - : m_fee_set{MakeFeeSet(minIncrementalFee, MAX_FILTER_FEERATE, FEE_FILTER_SPACING)} +FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee, FastRandomContext& rng) + : m_fee_set{MakeFeeSet(minIncrementalFee, MAX_FILTER_FEERATE, FEE_FILTER_SPACING)}, + insecure_rand{rng} { } diff --git a/src/policy/fees.h b/src/policy/fees.h index 8ed13482e92..69bda195be4 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -320,7 +320,7 @@ class FeeFilterRounder public: /** Create new FeeFilterRounder */ - explicit FeeFilterRounder(const CFeeRate& min_incremental_fee); + explicit FeeFilterRounder(const CFeeRate& min_incremental_fee, FastRandomContext& rng); /** Quantize a minimum fee for privacy purpose before broadcast. */ CAmount round(CAmount currentMinFee) EXCLUSIVE_LOCKS_REQUIRED(!m_insecure_rand_mutex); @@ -328,7 +328,7 @@ class FeeFilterRounder private: const std::set m_fee_set; Mutex m_insecure_rand_mutex; - FastRandomContext insecure_rand GUARDED_BY(m_insecure_rand_mutex); + FastRandomContext& insecure_rand GUARDED_BY(m_insecure_rand_mutex); }; #endif // BITCOIN_POLICY_FEES_H diff --git a/src/test/fuzz/fees.cpp b/src/test/fuzz/fees.cpp index deb0ed65cae..38a8c6798ec 100644 --- a/src/test/fuzz/fees.cpp +++ b/src/test/fuzz/fees.cpp @@ -17,7 +17,8 @@ FUZZ_TARGET(fees) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); const CFeeRate minimal_incremental_fee{ConsumeMoney(fuzzed_data_provider)}; - FeeFilterRounder fee_filter_rounder{minimal_incremental_fee}; + FastRandomContext rng{/*fDeterministic=*/true}; + FeeFilterRounder fee_filter_rounder{minimal_incremental_fee, rng}; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { const CAmount current_minimum_fee = ConsumeMoney(fuzzed_data_provider); const CAmount rounded_fee = fee_filter_rounder.round(current_minimum_fee); diff --git a/src/test/policy_fee_tests.cpp b/src/test/policy_fee_tests.cpp index 25fb5343e32..29d70cb5f85 100644 --- a/src/test/policy_fee_tests.cpp +++ b/src/test/policy_fee_tests.cpp @@ -13,7 +13,8 @@ BOOST_AUTO_TEST_SUITE(policy_fee_tests) BOOST_AUTO_TEST_CASE(FeeRounder) { - FeeFilterRounder fee_rounder{CFeeRate{1000}}; + FastRandomContext rng{/*fDeterministic=*/true}; + FeeFilterRounder fee_rounder{CFeeRate{1000}, rng}; // check that 1000 rounds to 974 or 1071 std::set results; From 4cafe9f176e93ebb6c38abb12140e8d8be005cbf Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 2 Oct 2023 14:41:08 +0100 Subject: [PATCH 6/6] [test] Make PeerManager's rng deterministic in tests --- src/test/util/setup_common.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2947bc3fcb2..e27d5a27ad0 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -256,6 +256,7 @@ TestingSetup::TestingSetup( m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); // Deterministic randomness for tests. PeerManager::Options peerman_opts; ApplyArgsManOptions(*m_node.args, peerman_opts); + peerman_opts.deterministic_rng = true; m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman, m_node.banman.get(), *m_node.chainman, *m_node.mempool, peerman_opts);