Merge bitcoin#32198: fuzz: Make p2p_headers_presync more deterministic

faa3ce3 fuzz: Avoid influence on the global RNG from peerman m_rng (MarcoFalke)
faf4c1b fuzz: Disable unused validation interface and scheduler in p2p_headers_presync (MarcoFalke)
fafaca6 fuzz: Avoid setting the mock-time twice (MarcoFalke)
fad2214 refactor: Use MockableSteadyClock in ReportHeadersPresync (MarcoFalke)
fa9c387 test: Introduce MockableSteadyClock::mock_time_point and ElapseSteady helper (MarcoFalke)
faf2d51 fuzz: Move global node id counter along with other global state (MarcoFalke)
fa98455 fuzz: Set ignore_incoming_txs in p2p_headers_presync (MarcoFalke)
faf2e23 fuzz: Shuffle files before testing them (MarcoFalke)

Pull request description:

  This should make the `p2p_headers_presync` fuzz target more deterministic.

  Tracking issue: bitcoin#29018.

  The first commits adds an `ElapseSteady` helper and type aliases. The second commit uses those helpers in `ReportHeadersPresync` and in the fuzz target to increase determinism.

  ### Testing

  It can be tested via (setting 32 parallel threads):

  ```
  cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ p2p_headers_presync 32
  ```

  The failing diff is contained in the commit messages, if applicable.

ACKs for top commit:
  Crypt-iQ:
    tACK faa3ce3
  janb84:
    Re-ACK [faa3ce3](bitcoin@faa3ce3)
  marcofleon:
    ACK faa3ce3

Tree-SHA512: 7e2e0ddf3b4e818300373d6906384df57a87f1eeb507fa43de1ba88cf03c8e6752a26b6e91bfb3ee26a21efcaf1d0d9eaf70d311d1637b671965ef4cb96e6b59

Author: glozow

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
             }

src/test/fuzz/fuzz.cpp

@@ -15,6 +15,7 @@
 #include <util/sock.h>
 #include <util/time.h>
 
+#include <algorithm>
 #include <csignal>
 #include <cstdint>
 #include <cstdio>
@@ -26,6 +27,7 @@
 #include <iostream>
 #include <map>
 #include <memory>
+#include <random>
 #include <string>
 #include <tuple>
 #include <utility>
@@ -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<fs::path> 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();

src/test/fuzz/p2p_headers_presync.cpp

@@ -15,6 +15,7 @@
 #include <test/util/net.h>
 #include <test/util/script.h>
 #include <test/util/setup_common.h>
+#include <test/util/time.h>
 #include <uint256.h>
 #include <validation.h>
 
@@ -31,6 +32,15 @@ class HeadersSyncSetup : public TestingSetup
         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;
         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);
@@ -51,7 +61,7 @@ void HeadersSyncSetup::ResetAndInitialize()
     auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
     connman.StopNodes();
 
-    NodeId id{0};
+    static NodeId id{0};
     std::vector<ConnectionType> conn_types = {
         ConnectionType::OUTBOUND_FULL_RELAY,
         ConnectionType::BLOCK_RELAY,
@@ -146,7 +156,12 @@ HeadersSyncSetup* g_testing_setup;
 
 void initialize()
 {
-    static auto setup = MakeNoLogFileContext<HeadersSyncSetup>(ChainType::MAIN);
+    static auto setup{
+        MakeNoLogFileContext<HeadersSyncSetup>(ChainType::MAIN,
+                                               {
+                                                   .setup_validation_interface = false,
+                                               }),
+    };
     g_testing_setup = setup.get();
 }
 } // namespace
@@ -155,17 +170,18 @@ FUZZ_TARGET(p2p_headers_presync, .init = initialize)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
-    SetMockTime(ConsumeTime(fuzzed_data_provider));
+    // The steady clock is currently only used for logging, so a constant
+    // time-point seems acceptable for now.
+    ElapseSteady elapse_steady{};
 
     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)};
@@ -231,6 +247,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();
 }

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

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 <test/util/time.h>

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 <util/time.h>
+
+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

src/util/time.cpp

@@ -21,7 +21,7 @@ void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread
 
 static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing
 std::atomic<bool> g_used_system_time{false};
-static std::atomic<std::chrono::milliseconds> g_mock_steady_time{}; //!< For testing
+static std::atomic<MockableSteadyClock::mock_time_point::duration> 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);

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<MockableSteadyClock>;
 
-    static constexpr std::chrono::milliseconds INITIAL_MOCK_TIME{1};
+    using mock_time_point = std::chrono::time_point<MockableSteadyClock, std::chrono::milliseconds>;
+    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();

src/validation.cpp

@@ -4456,16 +4456,16 @@ bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> 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;
     }

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 @@ class ChainstateManager
     friend Chainstate;
 
     /** Most recent headers presync progress update, for rate-limiting. */
-    std::chrono::time_point<std::chrono::steady_clock> m_last_presync_update GUARDED_BY(::cs_main) {};
+    MockableSteadyClock::time_point m_last_presync_update GUARDED_BY(GetMutex()){};
 
     std::array<ThresholdConditionCache, VERSIONBITS_NUM_BITS> m_warningcache GUARDED_BY(::cs_main);