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

3153e7d [fuzz] Add HeadersSyncState target (dergoegge)
53552af [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 3153e7d

Tree-SHA512: 8a4630ceeeb30e4eeabaa8eb5491d98f0bf900efe7cda07384eaac9f2afaccfbcaa979cc1cc7f0b6ca297a8f5c17a7759f94809dd87eb87d35348d847c83e8ab

Author: fanquake

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 \

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)
 {

src/headerssync.h

@@ -175,6 +175,13 @@ class HeadersSyncState {
      */
     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 @@ class HeadersSyncState {
     /** 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.
      *

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();
+        }
+    }
+}