Merge bitcoin/bitcoin#26662: fuzz: Add HeadersSyncState target

3153e7d779ac284f86e433af033d63f13f361b6f [fuzz] Add HeadersSyncState target (dergoegge)
53552affca381cdb5103ecdbcc7f3fb562e66ac4 [headerssync] Make m_commit_offset protected (dergoegge)

Pull request description:

  This adds a fuzz target for the `HeadersSyncState` class.

  I am unsure how well this is able to cover the logic since it is just processing unserialized CBlockHeaders straight from the fuzz input (headers are sometimes made continuous). However, it does manage to get to the redownload phase so i thought it is better then not having fuzzing at all.

  It would also be nice to fuzz the p2p logic that is using `HeadersSyncState` (e.g. `TryLowWorkHeadersSync`, `IsContinuationOfLowWorkHeadersSync`) but that likely requires some more work (refactoring👻).

ACKs for top commit:
  mzumsande:
    ACK 3153e7d779ac284f86e433af033d63f13f361b6f

Tree-SHA512: 8a4630ceeeb30e4eeabaa8eb5491d98f0bf900efe7cda07384eaac9f2afaccfbcaa979cc1cc7f0b6ca297a8f5c17a7759f94809dd87eb87d35348d847c83e8ab

4 files changed, 126 insertions, 7 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index a39b0abd9d..15d5a17cec 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -272,6 +272,7 @@ test_fuzz_fuzz_SOURCES = \
  test/fuzz/flatfile.cpp \
  test/fuzz/float.cpp \
  test/fuzz/golomb_rice.cpp \
+ test/fuzz/headerssync.cpp \
  test/fuzz/hex.cpp \
  test/fuzz/http_request.cpp \
  test/fuzz/i2p.cpp \
diff --git a/src/headerssync.cpp b/src/headerssync.cpp
index 757b942cd9..a3adfb4f70 100644
--- a/src/headerssync.cpp
+++ b/src/headerssync.cpp
@@ -24,11 +24,11 @@ static_assert(sizeof(CompressedHeader) == 48);
 
 HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
         const CBlockIndex* chain_start, const arith_uint256& minimum_required_work) :
+    m_commit_offset(GetRand<unsigned>(HEADER_COMMITMENT_PERIOD)),
     m_id(id), m_consensus_params(consensus_params),
     m_chain_start(chain_start),
     m_minimum_required_work(minimum_required_work),
     m_current_chain_work(chain_start->nChainWork),
-    m_commit_offset(GetRand<unsigned>(HEADER_COMMITMENT_PERIOD)),
     m_last_header_received(m_chain_start->GetBlockHeader()),
     m_current_height(chain_start->nHeight)
 {
diff --git a/src/headerssync.h b/src/headerssync.h
index 16da964246..e93f67e6da 100644
--- a/src/headerssync.h
+++ b/src/headerssync.h
@@ -175,6 +175,13 @@ public:
      */
     CBlockLocator NextHeadersRequestLocator() const;
 
+protected:
+    /** The (secret) offset on the heights for which to create commitments.
+     *
+     * m_header_commitments entries are created at any height h for which
+     * (h % HEADER_COMMITMENT_PERIOD) == m_commit_offset. */
+    const unsigned m_commit_offset;
+
 private:
     /** Clear out all download state that might be in progress (freeing any used
      * memory), and mark this object as no longer usable.
@@ -222,12 +229,6 @@ private:
     /** A queue of commitment bits, created during the 1st phase, and verified during the 2nd. */
     bitdeque<> m_header_commitments;
 
-    /** The (secret) offset on the heights for which to create commitments.
-     *
-     * m_header_commitments entries are created at any height h for which
-     * (h % HEADER_COMMITMENT_PERIOD) == m_commit_offset. */
-    const unsigned m_commit_offset;
-
     /** m_max_commitments is a bound we calculate on how long an honest peer's chain could be,
      * given the MTP rule.
      *
diff --git a/src/test/fuzz/headerssync.cpp b/src/test/fuzz/headerssync.cpp
new file mode 100644
index 0000000000..521afadb51
--- /dev/null
+++ b/src/test/fuzz/headerssync.cpp
@@ -0,0 +1,117 @@
+#include <arith_uint256.h>
+#include <chain.h>
+#include <chainparams.h>
+#include <headerssync.h>
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+#include <test/util/setup_common.h>
+#include <uint256.h>
+#include <util/time.h>
+#include <validation.h>
+
+#include <iterator>
+#include <vector>
+
+static void initialize_headers_sync_state_fuzz()
+{
+    static const auto testing_setup = MakeNoLogFileContext<>(
+        /*chain_name=*/CBaseChainParams::MAIN);
+}
+
+void MakeHeadersContinuous(
+    const CBlockHeader& genesis_header,
+    const std::vector<CBlockHeader>& all_headers,
+    std::vector<CBlockHeader>& new_headers)
+{
+    Assume(!new_headers.empty());
+
+    const CBlockHeader* prev_header{
+        all_headers.empty() ? &genesis_header : &all_headers.back()};
+
+    for (auto& header : new_headers) {
+        header.hashPrevBlock = prev_header->GetHash();
+
+        prev_header = &header;
+    }
+}
+
+class FuzzedHeadersSyncState : public HeadersSyncState
+{
+public:
+    FuzzedHeadersSyncState(const unsigned commit_offset, const CBlockIndex* chain_start, const arith_uint256& minimum_required_work)
+        : HeadersSyncState(/*id=*/0, Params().GetConsensus(), chain_start, minimum_required_work)
+    {
+        const_cast<unsigned&>(m_commit_offset) = commit_offset;
+    }
+};
+
+FUZZ_TARGET_INIT(headers_sync_state, initialize_headers_sync_state_fuzz)
+{
+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    auto mock_time{ConsumeTime(fuzzed_data_provider)};
+
+    CBlockHeader genesis_header{Params().GenesisBlock()};
+    CBlockIndex start_index(genesis_header);
+
+    if (mock_time < start_index.GetMedianTimePast()) return;
+    SetMockTime(mock_time);
+
+    const uint256 genesis_hash = genesis_header.GetHash();
+    start_index.phashBlock = &genesis_hash;
+
+    arith_uint256 min_work{UintToArith256(ConsumeUInt256(fuzzed_data_provider))};
+    FuzzedHeadersSyncState headers_sync(
+        /*commit_offset=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(1, 1024),
+        /*chain_start=*/&start_index,
+        /*minimum_required_work=*/min_work);
+
+    // Store headers for potential redownload phase.
+    std::vector<CBlockHeader> all_headers;
+    std::vector<CBlockHeader>::const_iterator redownloaded_it;
+    bool presync{true};
+    bool requested_more{true};
+
+    while (requested_more) {
+        std::vector<CBlockHeader> headers;
+
+        // Consume headers from fuzzer or maybe replay headers if we got to the
+        // redownload phase.
+        if (presync || fuzzed_data_provider.ConsumeBool()) {
+            auto deser_headers = ConsumeDeserializable<std::vector<CBlockHeader>>(fuzzed_data_provider);
+            if (!deser_headers || deser_headers->empty()) return;
+
+            if (fuzzed_data_provider.ConsumeBool()) {
+                MakeHeadersContinuous(genesis_header, all_headers, *deser_headers);
+            }
+
+            headers.swap(*deser_headers);
+        } else if (auto num_headers_left{std::distance(redownloaded_it, all_headers.cend())}; num_headers_left > 0) {
+            // Consume some headers from the redownload buffer (At least one
+            // header is consumed).
+            auto begin_it{redownloaded_it};
+            std::advance(redownloaded_it, fuzzed_data_provider.ConsumeIntegralInRange<int>(1, num_headers_left));
+            headers.insert(headers.cend(), begin_it, redownloaded_it);
+        }
+
+        if (headers.empty()) return;
+        auto result = headers_sync.ProcessNextHeaders(headers, fuzzed_data_provider.ConsumeBool());
+        requested_more = result.request_more;
+
+        if (result.request_more) {
+            if (presync) {
+                all_headers.insert(all_headers.cend(), headers.cbegin(), headers.cend());
+
+                if (headers_sync.GetState() == HeadersSyncState::State::REDOWNLOAD) {
+                    presync = false;
+                    redownloaded_it = all_headers.cbegin();
+
+                    // If we get to redownloading, the presynced headers need
+                    // to have the min amount of work on them.
+                    assert(CalculateHeadersWork(all_headers) >= min_work);
+                }
+            }
+
+            (void)headers_sync.NextHeadersRequestLocator();
+        }
+    }
+}