From faf2e238fbbb23fd0e20970dd95dcf01846df9aa Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 7 Apr 2025 12:01:25 +0200 Subject: [PATCH 1/8] fuzz: Shuffle files before testing them When iterating over all fuzz input files in a folder, the order should not matter. However, shuffling may be useful to detect non-determinism. Thus, shuffle in fuzz.cpp, when using neither libFuzzer, nor AFL. Also, shuffle in the deterministic-fuzz-coverage tool, when using libFuzzer. --- .../devtools/deterministic-fuzz-coverage/src/main.rs | 2 +- src/test/fuzz/fuzz.cpp | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs index 9c1738396b5..3eeb121db02 100644 --- a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs +++ b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs @@ -133,7 +133,7 @@ fn deterministic_coverage( let output = { let mut cmd = Command::new(fuzz_exe); if using_libfuzzer { - cmd.arg("-runs=1"); + cmd.args(["-runs=1", "-shuffle=1", "-prefer_small=0"]); } cmd } diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp index 30766700b9d..2c0f53cc347 100644 --- a/src/test/fuzz/fuzz.cpp +++ b/src/test/fuzz/fuzz.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -26,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -241,10 +243,15 @@ int main(int argc, char** argv) for (int i = 1; i < argc; ++i) { fs::path input_path(*(argv + i)); if (fs::is_directory(input_path)) { + std::vector files; for (fs::directory_iterator it(input_path); it != fs::directory_iterator(); ++it) { if (!fs::is_regular_file(it->path())) continue; - g_input_path = it->path(); - Assert(read_file(it->path(), buffer)); + files.emplace_back(it->path()); + } + std::ranges::shuffle(files, std::mt19937{std::random_device{}()}); + for (const auto& input_path : files) { + g_input_path = input_path; + Assert(read_file(input_path, buffer)); test_one_input(buffer); ++tested; buffer.clear(); From fa98455e4b162876d6a13057a0d6c03efae32505 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 2 Apr 2025 18:17:35 +0200 Subject: [PATCH 2/8] fuzz: Set ignore_incoming_txs in p2p_headers_presync This avoids non-determistic code paths. Without this patch, the tool would report a diff: cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32 ... - 5371| 393| peer.m_next_send_feefilter = current_time + m_rng.randrange(MAX_FEEFILTER_CHANGE_DELAY); - 5372| 393| } + 5371| 396| peer.m_next_send_feefilter = current_time + m_rng.randrange(MAX_FEEFILTER_CHANGE_DELAY); + 5372| 396| } 5373| 16.2k|} ... --- src/test/fuzz/p2p_headers_presync.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 6e568ad1cfd..40e57254b1c 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -31,6 +31,9 @@ public: PeerManager::Options peerman_opts; node::ApplyArgsManOptions(*m_node.args, peerman_opts); peerman_opts.max_headers_result = FUZZ_MAX_HEADERS_RESULTS; + // No txs are relayed. Disable irrelevant and possibly + // non-deterministic code paths. + peerman_opts.ignore_incoming_txs = true; m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman, m_node.banman.get(), *m_node.chainman, *m_node.mempool, *m_node.warnings, peerman_opts); From faf2d512c529db9ec7edd14bb2cd3e75f037489c Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 7 Apr 2025 14:02:01 +0200 Subject: [PATCH 3/8] fuzz: Move global node id counter along with other global state The global m_headers_presync_stats is not reset in ResetAndInitialize. This may lead to non-determinism. Fix it by incrementing the global node id counter instead. Without this patch, the tool would report a diff: cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32 ... 2587| 3.73k| if (best_it == m_headers_presync_stats.end()) { ------------------ - | Branch (2587:17): [True: 80, False: 3.65k] + | Branch (2587:17): [True: 73, False: 3.66k] ------------------ ... --- src/test/fuzz/p2p_headers_presync.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 40e57254b1c..43e6493ae35 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -54,7 +54,7 @@ void HeadersSyncSetup::ResetAndInitialize() auto& connman = static_cast(*m_node.connman); connman.StopNodes(); - NodeId id{0}; + static NodeId id{0}; std::vector conn_types = { ConnectionType::OUTBOUND_FULL_RELAY, ConnectionType::BLOCK_RELAY, From fa9c38794efb476d2c6e9dcbc9b137b8a1bac64a Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 28 Mar 2025 10:06:25 +0100 Subject: [PATCH 4/8] test: Introduce MockableSteadyClock::mock_time_point and ElapseSteady helper This refactor clarifies that the MockableSteadyClock::mock_time_point has millisecond precision by defining a type an using it. Moreover, a ElapseSteady helper is added which can be re-used easily. --- src/test/util/CMakeLists.txt | 1 + src/test/util/time.cpp | 5 +++++ src/test/util/time.h | 23 +++++++++++++++++++++++ src/util/time.cpp | 4 ++-- src/util/time.h | 5 +++-- 5 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 src/test/util/time.cpp create mode 100644 src/test/util/time.h diff --git a/src/test/util/CMakeLists.txt b/src/test/util/CMakeLists.txt index cd7c4c01183..3a6e31c720b 100644 --- a/src/test/util/CMakeLists.txt +++ b/src/test/util/CMakeLists.txt @@ -15,6 +15,7 @@ add_library(test_util STATIC EXCLUDE_FROM_ALL script.cpp setup_common.cpp str.cpp + time.cpp transaction_utils.cpp txmempool.cpp validation.cpp diff --git a/src/test/util/time.cpp b/src/test/util/time.cpp new file mode 100644 index 00000000000..8c8d98c0add --- /dev/null +++ b/src/test/util/time.cpp @@ -0,0 +1,5 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include diff --git a/src/test/util/time.h b/src/test/util/time.h new file mode 100644 index 00000000000..4e5d760bf3c --- /dev/null +++ b/src/test/util/time.h @@ -0,0 +1,23 @@ +// Copyright (c) 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_TEST_UTIL_TIME_H +#define BITCOIN_TEST_UTIL_TIME_H + +#include + +struct ElapseSteady { + MockableSteadyClock::mock_time_point::duration t{MockableSteadyClock::INITIAL_MOCK_TIME}; + ElapseSteady() + { + (*this)(0s); // init + } + void operator()(std::chrono::milliseconds d) + { + t += d; + MockableSteadyClock::SetMockTime(t); + } +}; + +#endif // BITCOIN_TEST_UTIL_TIME_H diff --git a/src/util/time.cpp b/src/util/time.cpp index cafc27e0d05..62d2c98debd 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -21,7 +21,7 @@ void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread static std::atomic g_mock_time{}; //!< For testing std::atomic g_used_system_time{false}; -static std::atomic g_mock_steady_time{}; //!< For testing +static std::atomic g_mock_steady_time{}; //!< For testing NodeClock::time_point NodeClock::now() noexcept { @@ -62,7 +62,7 @@ MockableSteadyClock::time_point MockableSteadyClock::now() noexcept return time_point{ret}; }; -void MockableSteadyClock::SetMockTime(std::chrono::milliseconds mock_time_in) +void MockableSteadyClock::SetMockTime(mock_time_point::duration mock_time_in) { Assert(mock_time_in >= 0s); g_mock_steady_time.store(mock_time_in, std::memory_order_relaxed); diff --git a/src/util/time.h b/src/util/time.h index c43b306ff24..6bfa469a52a 100644 --- a/src/util/time.h +++ b/src/util/time.h @@ -38,7 +38,8 @@ using SystemClock = std::chrono::system_clock; struct MockableSteadyClock : public std::chrono::steady_clock { using time_point = std::chrono::time_point; - static constexpr std::chrono::milliseconds INITIAL_MOCK_TIME{1}; + using mock_time_point = std::chrono::time_point; + static constexpr mock_time_point::duration INITIAL_MOCK_TIME{1}; /** Return current system time or mocked time, if set */ static time_point now() noexcept; @@ -50,7 +51,7 @@ struct MockableSteadyClock : public std::chrono::steady_clock { * for testing. * To stop mocking, call ClearMockTime(). */ - static void SetMockTime(std::chrono::milliseconds mock_time_in); + static void SetMockTime(mock_time_point::duration mock_time_in); /** Clear mock time, go back to system steady clock. */ static void ClearMockTime(); From fad22149f4677a650939be5aa0865f5f6e709aec Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 28 Mar 2025 13:51:40 +0100 Subject: [PATCH 5/8] refactor: Use MockableSteadyClock in ReportHeadersPresync This allows the clock to be mockable in tests. Also, replace cs_main with GetMutex() while touching this function. Also, use the ElapseSteady test helper in the p2p_headers_presync fuzz target to make it more deterministic. The m_last_presync_update variable is a global that is not reset in ResetAndInitialize. However, it is only used for logging, so completely disable it for now. Without this patch, the tool would report a diff: cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32 ... 4468| 81| auto now = std::chrono::steady_clock::now(); 4469| 81| if (now < m_last_presync_update + std::chrono::milliseconds{250}) return; - ^80 + ^79 ... --- src/test/fuzz/p2p_headers_presync.cpp | 4 ++++ src/validation.cpp | 6 +++--- src/validation.h | 4 ++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 43e6493ae35..5358ebee330 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -158,6 +159,9 @@ FUZZ_TARGET(p2p_headers_presync, .init = initialize) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + // The steady clock is currently only used for logging, so a constant + // time-point seems acceptable for now. + ElapseSteady elapse_steady{}; SetMockTime(ConsumeTime(fuzzed_data_provider)); ChainstateManager& chainman = *g_testing_setup->m_node.chainman; diff --git a/src/validation.cpp b/src/validation.cpp index fedcb9ca57f..aa1effb736f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4456,16 +4456,16 @@ bool ChainstateManager::ProcessNewBlockHeaders(std::span hea void ChainstateManager::ReportHeadersPresync(const arith_uint256& work, int64_t height, int64_t timestamp) { - AssertLockNotHeld(cs_main); + AssertLockNotHeld(GetMutex()); { - LOCK(cs_main); + LOCK(GetMutex()); // Don't report headers presync progress if we already have a post-minchainwork header chain. // This means we lose reporting for potentially legitimate, but unlikely, deep reorgs, but // prevent attackers that spam low-work headers from filling our logs. if (m_best_header->nChainWork >= UintToArith256(GetConsensus().nMinimumChainWork)) return; // Rate limit headers presync updates to 4 per second, as these are not subject to DoS // protection. - auto now = std::chrono::steady_clock::now(); + auto now = MockableSteadyClock::now(); if (now < m_last_presync_update + std::chrono::milliseconds{250}) return; m_last_presync_update = now; } diff --git a/src/validation.h b/src/validation.h index f6cbee28fc5..e361c7af101 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -933,7 +933,7 @@ private: friend Chainstate; /** Most recent headers presync progress update, for rate-limiting. */ - std::chrono::time_point m_last_presync_update GUARDED_BY(::cs_main) {}; + MockableSteadyClock::time_point m_last_presync_update GUARDED_BY(GetMutex()){}; std::array m_warningcache GUARDED_BY(::cs_main); From fafaca6cbc2fdabf95c286c9d8346e331a2de216 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 2 Apr 2025 18:11:18 +0200 Subject: [PATCH 6/8] fuzz: Avoid setting the mock-time twice It should be sufficient to set it once. Especially, if the dynamic value is only used by ResetAndInitialize. This also avoids non-determistic code paths, when ResetAndInitialize may re-initialize m_next_inv_to_inbounds. Without this patch, the tool would report a diff: cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32 ... - 1126| 3| m_next_inv_to_inbounds = now + m_rng.rand_exp_duration(average_interval); - 1127| 3| } + 1126| 10| m_next_inv_to_inbounds = now + m_rng.rand_exp_duration(average_interval); + 1127| 10| } 1128| 491| return m_next_inv_to_inbounds; ... --- src/test/fuzz/p2p_headers_presync.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 5358ebee330..93575877407 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -162,17 +162,15 @@ FUZZ_TARGET(p2p_headers_presync, .init = initialize) // The steady clock is currently only used for logging, so a constant // time-point seems acceptable for now. ElapseSteady elapse_steady{}; - SetMockTime(ConsumeTime(fuzzed_data_provider)); ChainstateManager& chainman = *g_testing_setup->m_node.chainman; + CBlockHeader base{chainman.GetParams().GenesisBlock()}; + SetMockTime(base.nTime); LOCK(NetEventsInterface::g_msgproc_mutex); g_testing_setup->ResetAndInitialize(); - CBlockHeader base{chainman.GetParams().GenesisBlock()}; - SetMockTime(base.nTime); - // The chain is just a single block, so this is equal to 1 size_t original_index_size{WITH_LOCK(cs_main, return chainman.m_blockman.m_block_index.size())}; arith_uint256 total_work{WITH_LOCK(cs_main, return chainman.m_best_header->nChainWork)}; From faf4c1b6fc330885ddf84b643838d1e301aaeab2 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 9 Apr 2025 11:15:09 +0200 Subject: [PATCH 7/8] fuzz: Disable unused validation interface and scheduler in p2p_headers_presync This may also avoid non-determinism in the scheduler thread. --- src/test/fuzz/p2p_headers_presync.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 93575877407..3a7146df8fe 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -150,7 +150,12 @@ HeadersSyncSetup* g_testing_setup; void initialize() { - static auto setup = MakeNoLogFileContext(ChainType::MAIN); + static auto setup{ + MakeNoLogFileContext(ChainType::MAIN, + { + .setup_validation_interface = false, + }), + }; g_testing_setup = setup.get(); } } // namespace @@ -236,6 +241,4 @@ FUZZ_TARGET(p2p_headers_presync, .init = initialize) // to meet the anti-DoS work threshold. So, if at any point the block index grew in size, then there's a bug // in the headers pre-sync logic. assert(WITH_LOCK(cs_main, return chainman.m_blockman.m_block_index.size()) == original_index_size); - - g_testing_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue(); } From faa3ce31998cdbdcb888132b0d38ee7a1e743682 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 9 Apr 2025 17:58:20 +0200 Subject: [PATCH 8/8] fuzz: Avoid influence on the global RNG from peerman m_rng This should avoid the remaining non-determistic code coverage paths. Without this patch, the tool would report a diff (only when running without libFuzzer): cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32 --- src/test/fuzz/p2p_headers_presync.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 3a7146df8fe..b31b74ee4fe 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -32,6 +32,12 @@ public: PeerManager::Options peerman_opts; node::ApplyArgsManOptions(*m_node.args, peerman_opts); peerman_opts.max_headers_result = FUZZ_MAX_HEADERS_RESULTS; + // The peerman's rng is a global that is re-used, so it will be re-used + // and may cause non-determinism between runs. This may even influence + // the global RNG, because seeding may be done from the gloabl one. For + // now, avoid it influencing the global RNG, and initialize it with a + // constant instead. + peerman_opts.deterministic_rng = true; // No txs are relayed. Disable irrelevant and possibly // non-deterministic code paths. peerman_opts.ignore_incoming_txs = true;