p2p: Correct unrealistic headerssync unit test behavior #32579

I know this is just a move, but instead of just bare magic number + magic comment (which also has to be manually updated every time), could we have the something like : Generated by headerssync-params.py (on 2025-03-04)

I like the idea of including the generation date, and was including changes to the Python generator file in an unpublished version of this PR, but decided to keep that separate. Added to that separate WIP branch.

The magic numbers are not updated manually, they are included in the calculated output: https://github.com/bitcoin/bitcoin/blob/11a2d3a63e90cdc1920ede3c67d52a9c72860e6b/contrib/devtools/headerssync-params.py#L347-L348

Added generation date in latest push as it was making modifications to the Python script anyway.

34e1513efeb193a46d37f184b9ab41bcda974afa: I have inlined the methods and verified that the remaining changes are only comments, reused-and-cleared pointers and vectors 👍

<details> <summary>A few more refactoring nits that could be considered to make this test even more readable and consistent</summary>

diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp
index 054853bbbf..89bb6682d1 100644
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -13,29 +13,32 @@
 
 #include <boost/test/unit_test.hpp>
 
-struct HeadersGeneratorSetup : public RegTestingSetup {
+struct HeadersGeneratorSetup : RegTestingSetup
+{
     /** Search for a nonce to meet (regtest) proof of work */
-    void FindProofOfWork(CBlockHeader& starting_header);
+    static void FindProofOfWork(CBlockHeader& starting_header);
     /**
      * Generate headers in a chain that build off a given starting hash, using
      * the given nVersion, advancing time by 1 second from the starting
      * prev_time, and with a fixed merkle root hash.
      */
-    void GenerateHeaders(std::vector<CBlockHeader>& headers, size_t count,
-            const uint256& starting_hash, const int nVersion, int prev_time,
-            const uint256& merkle_root, const uint32_t nBits);
+    static void GenerateHeaders(
+        std::vector<CBlockHeader>& headers, size_t count,
+        const uint256& starting_hash, int nVersion, int prev_time,
+        const uint256& merkle_root, uint32_t nBits);
 };
 
 void HeadersGeneratorSetup::FindProofOfWork(CBlockHeader& starting_header)
 {
     while (!CheckProofOfWork(starting_header.GetHash(), starting_header.nBits, Params().GetConsensus())) {
-        ++(starting_header.nNonce);
+        ++starting_header.nNonce;
     }
 }
 
-void HeadersGeneratorSetup::GenerateHeaders(std::vector<CBlockHeader>& headers,
-        size_t count, const uint256& starting_hash, const int nVersion, int prev_time,
-        const uint256& merkle_root, const uint32_t nBits)
+void HeadersGeneratorSetup::GenerateHeaders(
+    std::vector<CBlockHeader>& headers,
+    const size_t count, const uint256& starting_hash, const int nVersion, int prev_time,
+    const uint256& merkle_root, const uint32_t nBits)
 {
     uint256 prev_hash = starting_hash;
 
@@ -45,14 +48,13 @@ void HeadersGeneratorSetup::GenerateHeaders(std::vector<CBlockHeader>& headers,
         next_header.nVersion = nVersion;
         next_header.hashPrevBlock = prev_hash;
         next_header.hashMerkleRoot = merkle_root;
-        next_header.nTime = prev_time+1;
+        next_header.nTime = prev_time + 1;
         next_header.nBits = nBits;
 
         FindProofOfWork(next_header);
         prev_hash = next_header.GetHash();
         prev_time = next_header.nTime;
     }
-    return;
 }
 
 BOOST_FIXTURE_TEST_SUITE(headers_sync_chainwork_tests, HeadersGeneratorSetup)
@@ -63,31 +65,41 @@ BOOST_FIXTURE_TEST_SUITE(headers_sync_chainwork_tests, HeadersGeneratorSetup)
 //    updates to the REDOWNLOAD phase successfully.
 // 2. Then we deliver the second set of headers and verify that they fail
 //    processing (presumably due to commitments not matching).
-static void SneakyRedownload(const std::vector<CBlockHeader>& first_chain, const std::vector<CBlockHeader>& second_chain, const CBlockIndex* chain_start, const arith_uint256& chain_work);
+static void SneakyRedownload(
+    const std::vector<CBlockHeader>& first_chain,
+    const std::vector<CBlockHeader>& second_chain,
+    const CBlockIndex* chain_start,
+    const arith_uint256& chain_work);
 // 3. Verify that repeating with the first set of headers in both phases is
 //    successful.
-static void HappyPath(const std::vector<CBlockHeader>& first_chain, const CBlockIndex* chain_start, const arith_uint256& chain_work);
+static void HappyPath(
+    const std::vector<CBlockHeader>& first_chain,
+    const CBlockIndex* chain_start,
+    const arith_uint256& chain_work);
 // 4. Finally, repeat the second set of headers in both phases to demonstrate
 //    behavior when the chain a peer provides has too little work.
-static void TooLittleWork(const std::vector<CBlockHeader>& second_chain, const CBlockIndex* chain_start, const arith_uint256& chain_work);
+static void TooLittleWork(
+    const std::vector<CBlockHeader>& second_chain,
+    const CBlockIndex* chain_start,
+    const arith_uint256& chain_work);
 
 BOOST_AUTO_TEST_CASE(headers_sync_state)
 {
     std::vector<CBlockHeader> first_chain;
     std::vector<CBlockHeader> second_chain;
 
-    const int target_blocks = 15000;
-    arith_uint256 chain_work = target_blocks*2;
+    constexpr int target_blocks = 15000;
+    const arith_uint256 chain_work = target_blocks * 2;
 
     // Generate headers for two different chains (using differing merkle roots
     // to ensure the headers are different).
-    GenerateHeaders(first_chain, target_blocks-1, Params().GenesisBlock().GetHash(),
-            Params().GenesisBlock().nVersion, Params().GenesisBlock().nTime,
-            ArithToUint256(0), Params().GenesisBlock().nBits);
+    GenerateHeaders(first_chain, target_blocks - 1, Params().GenesisBlock().GetHash(),
+                    Params().GenesisBlock().nVersion, Params().GenesisBlock().nTime,
+                    ArithToUint256(0), Params().GenesisBlock().nBits);
 
-    GenerateHeaders(second_chain, target_blocks-2, Params().GenesisBlock().GetHash(),
-            Params().GenesisBlock().nVersion, Params().GenesisBlock().nTime,
-            ArithToUint256(1), Params().GenesisBlock().nBits);
+    GenerateHeaders(second_chain, target_blocks - 2, Params().GenesisBlock().GetHash(),
+                    Params().GenesisBlock().nVersion, Params().GenesisBlock().nTime,
+                    ArithToUint256(1), Params().GenesisBlock().nBits);
 
     const CBlockIndex* chain_start = WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(Params().GenesisBlock().GetHash()));
 
@@ -96,7 +108,10 @@ BOOST_AUTO_TEST_CASE(headers_sync_state)
     TooLittleWork(second_chain, chain_start, chain_work);
 }
 
-static void SneakyRedownload(const std::vector<CBlockHeader>& first_chain, const std::vector<CBlockHeader>& second_chain, const CBlockIndex* chain_start, const arith_uint256& chain_work)
+static void SneakyRedownload(
+    const std::vector<CBlockHeader>& first_chain,
+    const std::vector<CBlockHeader>& second_chain,
+    const CBlockIndex* chain_start, const arith_uint256& chain_work)
 {
     std::vector<CBlockHeader> headers_batch;
 
@@ -109,56 +124,59 @@ static void SneakyRedownload(const std::vector<CBlockHeader>& first_chain, const
     // Pretend the first header is still "full", so we don't abort.
     auto result = hss.ProcessNextHeaders(headers_batch, true);
 
-    // This chain should look valid, and we should have met the proof-of-work
-    // requirement.
+    // This chain should look valid, and we should have met the proof-of-work requirement.
     BOOST_CHECK(result.success);
     BOOST_CHECK(result.request_more);
-    BOOST_CHECK(hss.GetState() == HeadersSyncState::State::REDOWNLOAD);
+    BOOST_CHECK_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
 
     // Try to sneakily feed back the second chain.
     result = hss.ProcessNextHeaders(second_chain, true);
     BOOST_CHECK(!result.success); // foiled!
-    BOOST_CHECK(hss.GetState() == HeadersSyncState::State::FINAL);
+    BOOST_CHECK_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
 }
 
-static void HappyPath(const std::vector<CBlockHeader>& first_chain, const CBlockIndex* chain_start, const arith_uint256& chain_work)
+static void HappyPath(
+    const std::vector<CBlockHeader>& first_chain,
+    const CBlockIndex* chain_start,
+    const arith_uint256& chain_work)
 {
     // This time we feed the first chain twice.
     HeadersSyncState hss{0, Params().GetConsensus(), chain_start, chain_work};
     (void)hss.ProcessNextHeaders(first_chain, true);
-    BOOST_CHECK(hss.GetState() == HeadersSyncState::State::REDOWNLOAD);
+    BOOST_CHECK_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
 
-    auto result = hss.ProcessNextHeaders(first_chain, true);
-    BOOST_CHECK(result.success);
-    BOOST_CHECK(!result.request_more);
+    const auto [pow_validated_headers, success, request_more] = hss.ProcessNextHeaders(first_chain, true);
+    BOOST_CHECK(success);
+    BOOST_CHECK(!request_more);
     // All headers should be ready for acceptance:
-    BOOST_CHECK(result.pow_validated_headers.size() == first_chain.size());
+    BOOST_CHECK_EQUAL(pow_validated_headers.size(), first_chain.size());
     // Nothing left for the sync logic to do:
-    BOOST_CHECK(hss.GetState() == HeadersSyncState::State::FINAL);
+    BOOST_CHECK_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
 }
 
-static void TooLittleWork(const std::vector<CBlockHeader>& second_chain, const CBlockIndex* chain_start, const arith_uint256& chain_work)
+static void TooLittleWork(
+    const std::vector<CBlockHeader>& second_chain,
+    const CBlockIndex* chain_start,
+    const arith_uint256& chain_work)
 {
-    // Verify that just trying to process the second chain would not succeed
-    // (too little work).
+    // Verify that just trying to process the second chain would not succeed (too little work).
     HeadersSyncState hss{0, Params().GetConsensus(), chain_start, chain_work};
     BOOST_CHECK(hss.GetState() == HeadersSyncState::State::PRESYNC);
-     // Pretend just the first message is "full", so we don't abort.
+    // Pretend just the first message is "full", so we don't abort.
     (void)hss.ProcessNextHeaders({second_chain.front()}, true);
     BOOST_CHECK(hss.GetState() == HeadersSyncState::State::PRESYNC);
 
     std::vector<CBlockHeader> headers_batch;
     headers_batch.insert(headers_batch.end(), std::next(second_chain.begin(), 1), second_chain.end());
     // Tell the sync logic that the headers message was not full, implying no
-    // more headers can be requested. For a low-work-chain, this should causes
+    // more headers can be requested. For a low-work-chain, this should cause
     // the sync to end with no headers for acceptance.
-    auto result = hss.ProcessNextHeaders(headers_batch, false);
+    const auto [pow_validated_headers, success, request_more] = hss.ProcessNextHeaders(headers_batch, false);
     BOOST_CHECK(hss.GetState() == HeadersSyncState::State::FINAL);
-    BOOST_CHECK(result.pow_validated_headers.empty());
-    BOOST_CHECK(!result.request_more);
-    // Nevertheless, no validation errors should have been detected with the
-    // chain:
-    BOOST_CHECK(result.success);
+    BOOST_CHECK(pow_validated_headers.empty());
+    BOOST_CHECK(!request_more);
+    // Nevertheless, no validation errors should have been detected with the chain:
+    BOOST_CHECK(success);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

</details>

I see that some of these were done in next commits, but I'd prefer doing either moves or refactors or adding new checks in the commits to simplify review

(Incorporated most of this, breaking out commits, expanding function prototypes into multiple lines, even removing the HeadersGeneratorSetup type in favor of free functions).

we're reusing the previous result (which was brace initialized), what if we used braces to delimit the two independent code parts (+ a few other nits you might want to consider):

// Verify that just trying to process the second chain would not succeed (too little work).
HeadersSyncState hss{0, Params().GetConsensus(), chain_start, CHAIN_WORK};
BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
// Pretend just the first message is "full", so we don't abort.
{
    const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders({second_chain.begin(), 1}, true)};
    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
    BOOST_CHECK(success);
    BOOST_CHECK(request_more);
    BOOST_CHECK_EQUAL(pow_validated_headers.size(), 0);
}

// Tell the sync logic that the headers message was not full, implying no
// more headers can be requested. For a low-work-chain, this should cause
// the sync to end with no headers for acceptance.
{
    const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders({second_chain.begin() + 1, second_chain.end()}, false)};
    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
    BOOST_CHECK(!request_more);
    BOOST_CHECK_EQUAL(pow_validated_headers.size(), 0);
    // Nevertheless, no validation errors should have been detected with the chain:
    BOOST_CHECK(success);
}

Took the scoping idea, still not sure about the structured bindings.

not sure about the structured bindings

That's what helped me in noticing that we weren't checking request_more

Is there anything to catch wrong order?

const auto [pow_validated_headers, success, request_more] = ...

vs

const auto [pow_validated_headers, request_more, success] = ...

(There's clang-tidy logic to catch /*paramname=*/true mismatches for function calls).

I don't think there could/should be, you could name them as const auto [x, y, z]{hss.ProcessNextHeaders(std::span{second_chain.begin() + 1, second_chain.end()}, false)}; as well

Yes, but when breaking apart a struct and two of the new variable names match struct field names with identical types, it would be nice to have some kind of guard rail.

This seems to be the only place where request_more isn't validated.

nit: what's the meaning of "foiled" here?

If we didn't succeed, then request_more shouldn't matter, but agree it's best to have it be false (just like pow_validated_headers should be empty). It would be better if ProcessNextHeaders returned something like std::optional<ProcessingResult> instead of having a bool field to indicate success, but this PR already feels large enough.

"foiled" means the attack attempt was detected and therefore failed. Elaborated the comment a bit in next push.

Hm.. my std::optional<ProcessingResult> suggestion would ~not work~ not clarify things here:

https://github.com/bitcoin/bitcoin/blob/a7b9cc4a0af0dbe4031ec5848d403f61dc9c3356/src/test/headers_sync_chainwork_tests.cpp#L127-L136

Success seems to mean that PRESYNC- and REDOWNLOAD-headers matched, even if the chain is too short.

should we make any of these explicit?

One could argue at least the uint64_t-taking ctor here and in arith_uint256 should be explicit, but I'd rather not have to change src/pow.cpp in this PR.

size_t might me more fitting here since GenerateHeaders accepts that instead of an int:

constexpr size_t TARGET_BLOCKS{15'000};

Could we use const std::span<const CBlockHeader> in these helpers as well? And maybe split these long lines.

Agree on the split but what would be the point of changing the type?

• they're simpler/less specific
• they're shorter (matters for these long lines)
• you're already changing them in other places

i.e. what's the point of keeping them as vectors when we don't actually need so much power?

Using const vector& communicates that we are sending in the entire chain, not some subset. I like the specificity. const vectors feel more neutered than spans IMO.

When we're only iterating inside, it makes sense to have a fine-grained, segregated interface and only define the narrowest functionality which supports the operations we actually need inside. It announces to the reader that we shouldn't expect any extra complexity, only the ones that the interface implements. But if you think vector does it better, I'm also fine with that. For me span is not about a subset of teh data but a subset of the functionality - just a narrowed view to rule out certain operations that we definitely won't need.

👍 for comparing size instead of empty - the failure message would likely be more meaningful

I think we could do these in the previous refactor commit instead - to separate low risk changes from ones where we have to may more attention, while keeping the first one about move-only changes + minor ones. Or do the first commit as strictly move-only and add a second one with these tiny refactors (I'd like the latter more).

full patch for 13aa0b860e9692715d96b13acacb3f83a537b905

<details> <summary>Details</summary>

diff --git a/src/arith_uint256.h b/src/arith_uint256.h
index e021947381..b07e8e9f41 100644
--- a/src/arith_uint256.h
+++ b/src/arith_uint256.h
@@ -31,7 +31,7 @@ protected:
     uint32_t pn[WIDTH];
 
 public:
-    constexpr base_uint()
+    explicit constexpr base_uint()
     {
         for (int i = 0; i < WIDTH; i++)
             pn[i] = 0;
@@ -243,8 +243,8 @@ class arith_uint256 : public base_uint<256>
 {
 public:
     constexpr arith_uint256() = default;
-    constexpr arith_uint256(const base_uint<256>& b) : base_uint<256>(b) {}
-    constexpr arith_uint256(uint64_t b) : base_uint<256>(b) {}
+    constexpr arith_uint256(const base_uint& b) : base_uint(b) {}
+    constexpr arith_uint256(const uint64_t b) : base_uint(b) {}
 
     /**
      * The "compact" format is a representation of a whole
diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp
index b696285d0e..264959f8c3 100644
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -13,32 +13,35 @@
 
 #include <boost/test/unit_test.hpp>
 
-constexpr int TARGET_BLOCKS{15000};
+constexpr size_t TARGET_BLOCKS{15'000};
 constexpr arith_uint256 CHAIN_WORK{TARGET_BLOCKS * 2};
 
-struct HeadersGeneratorSetup : public RegTestingSetup {
+struct HeadersGeneratorSetup : RegTestingSetup
+{
     /** Search for a nonce to meet (regtest) proof of work */
-    void FindProofOfWork(CBlockHeader& starting_header);
+    static void FindProofOfWork(CBlockHeader& starting_header);
     /**
      * Generate headers in a chain that build off a given starting hash, using
      * the given nVersion, advancing time by 1 second from the starting
      * prev_time, and with a fixed merkle root hash.
      */
-    void GenerateHeaders(std::vector<CBlockHeader>& headers, size_t count,
-            const uint256& starting_hash, const int nVersion, int prev_time,
-            const uint256& merkle_root, const uint32_t nBits);
+    static void GenerateHeaders(
+        std::vector<CBlockHeader>& headers, size_t count,
+        const uint256& starting_hash, int nVersion, int prev_time,
+        const uint256& merkle_root, uint32_t nBits);
 };
 
 void HeadersGeneratorSetup::FindProofOfWork(CBlockHeader& starting_header)
 {
     while (!CheckProofOfWork(starting_header.GetHash(), starting_header.nBits, Params().GetConsensus())) {
-        ++(starting_header.nNonce);
+        ++starting_header.nNonce;
     }
 }
 
-void HeadersGeneratorSetup::GenerateHeaders(std::vector<CBlockHeader>& headers,
-        size_t count, const uint256& starting_hash, const int nVersion, int prev_time,
-        const uint256& merkle_root, const uint32_t nBits)
+void HeadersGeneratorSetup::GenerateHeaders(
+    std::vector<CBlockHeader>& headers,
+    const size_t count, const uint256& starting_hash, const int nVersion, int prev_time,
+    const uint256& merkle_root, const uint32_t nBits)
 {
     uint256 prev_hash = starting_hash;
 
@@ -48,7 +51,7 @@ void HeadersGeneratorSetup::GenerateHeaders(std::vector<CBlockHeader>& headers,
         next_header.nVersion = nVersion;
         next_header.hashPrevBlock = prev_hash;
         next_header.hashMerkleRoot = merkle_root;
-        next_header.nTime = prev_time+1;
+        next_header.nTime = prev_time + 1;
         next_header.nBits = nBits;
 
         FindProofOfWork(next_header);
@@ -65,13 +68,20 @@ BOOST_FIXTURE_TEST_SUITE(headers_sync_chainwork_tests, HeadersGeneratorSetup)
 //    updates to the REDOWNLOAD phase successfully.
 // 2. Then we deliver the second set of headers and verify that they fail
 //    processing (presumably due to commitments not matching).
-static void SneakyRedownload(const std::vector<CBlockHeader>& first_chain, const std::vector<CBlockHeader>& second_chain, const CBlockIndex* chain_start);
+static void SneakyRedownload(
+    const std::span<const CBlockHeader> first_chain,
+    const std::span<const CBlockHeader> second_chain,
+    const CBlockIndex* chain_start);
 // 3. Verify that repeating with the first set of headers in both phases is
 //    successful.
-static void HappyPath(const std::vector<CBlockHeader>& first_chain, const CBlockIndex* chain_start);
+static void HappyPath(
+    const std::span<const CBlockHeader> first_chain,
+    const CBlockIndex* chain_start);
 // 4. Finally, repeat the second set of headers in both phases to demonstrate
 //    behavior when the chain a peer provides has too little work.
-static void TooLittleWork(const std::vector<CBlockHeader>& second_chain, const CBlockIndex* chain_start);
+static void TooLittleWork(
+    const std::span<const CBlockHeader> second_chain,
+    const CBlockIndex* chain_start);
 
 BOOST_AUTO_TEST_CASE(headers_sync_state)
 {
@@ -94,81 +104,101 @@ BOOST_AUTO_TEST_CASE(headers_sync_state)
     TooLittleWork(second_chain, chain_start);
 }
 
-static void SneakyRedownload(const std::vector<CBlockHeader>& first_chain, const std::vector<CBlockHeader>& second_chain, const CBlockIndex* chain_start)
+static void SneakyRedownload(
+    const std::span<const CBlockHeader> first_chain,
+    const std::span<const CBlockHeader> second_chain,
+    const CBlockIndex* chain_start)
 {
     // Feed the first chain to HeadersSyncState, by delivering 1 header
     // initially and then the rest.
     HeadersSyncState hss{0, Params().GetConsensus(), chain_start, CHAIN_WORK};
-    auto result{hss.ProcessNextHeaders(std::span{first_chain.begin(), 1}, true)};
-    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
-    BOOST_CHECK(result.success);
-    BOOST_CHECK(result.request_more);
-    BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
-    BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), first_chain.front().GetHash());
+    {
+        const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders({first_chain.begin(), 1}, true)};
+        BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
+        BOOST_CHECK(success);
+        BOOST_CHECK(request_more);
+        BOOST_CHECK_EQUAL(pow_validated_headers.size(), 0);
+        BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), first_chain.front().GetHash());
+    }
 
     // Pretend the first header is still "full", so we don't abort.
-    result = hss.ProcessNextHeaders(std::span{first_chain.begin() + 1, first_chain.end()}, true);
-    // This chain should look valid, and we should have met the proof-of-work
-    // requirement during PRESYNC and transitioned to REDOWNLOAD.
-    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
-    BOOST_CHECK(result.success);
-    BOOST_CHECK(result.request_more);
-    BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
-    // The locator should reset to genesis.
-    BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), Params().GenesisBlock().GetHash());
+    {
+        const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders({first_chain.begin() + 1, first_chain.end()}, true)};
+        // This chain should look valid, and we should have met the proof-of-work
+        // requirement during PRESYNC and transitioned to REDOWNLOAD.
+        BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
+        BOOST_CHECK(success);
+        BOOST_CHECK(request_more);
+        BOOST_CHECK_EQUAL(pow_validated_headers.size(), 0);
+        // The locator should reset to genesis.
+        BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), Params().GenesisBlock().GetHash());
+    }
 
     // Try to sneakily feed back the second chain during REDOWNLOAD.
-    result = hss.ProcessNextHeaders(second_chain, true);
-    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
-    BOOST_CHECK(!result.success); // foiled!
-    BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
+    {
+        const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders(second_chain, true)};
+        BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
+        BOOST_CHECK(!success); // foiled!
+        BOOST_CHECK(!request_more);
+        BOOST_CHECK_EQUAL(pow_validated_headers.size(), 0);
+    }
 }
 
-static void HappyPath(const std::vector<CBlockHeader>& first_chain, const CBlockIndex* chain_start)
+static void HappyPath(
+    const std::span<const CBlockHeader> first_chain,
+    const CBlockIndex* chain_start)
 {
     // This time we feed the first chain twice.
     HeadersSyncState hss{0, Params().GetConsensus(), chain_start, CHAIN_WORK};
-    auto result{hss.ProcessNextHeaders(first_chain, true)};
-    // Switched from PRESYNC to REDOWNLOAD after reaching sufficient work:
-    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
-    BOOST_CHECK(result.success);
-    BOOST_CHECK(result.request_more);
-    BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
-    // The locator should reset to genesis.
-    BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), Params().GenesisBlock().GetHash());
-
-    result = hss.ProcessNextHeaders(first_chain, true);
-    // Nothing left for the sync logic to do:
-    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
-    BOOST_CHECK(result.success);
-    BOOST_CHECK(!result.request_more);
-    // All headers should be ready for acceptance:
-    BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), first_chain.size());
+    {
+        const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders(first_chain, true)};
+        // Switched from PRESYNC to REDOWNLOAD after reaching sufficient work:
+        BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
+        BOOST_CHECK(success);
+        BOOST_CHECK(request_more);
+        BOOST_CHECK_EQUAL(pow_validated_headers.size(), 0);
+        // The locator should reset to genesis.
+        BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), Params().GenesisBlock().GetHash());
+    }
+
+    {
+        const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders(first_chain, true)};
+        // Nothing left for the sync logic to do:
+        BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
+        BOOST_CHECK(success);
+        BOOST_CHECK(!request_more);
+        // All headers should be ready for acceptance:
+        BOOST_CHECK_EQUAL(pow_validated_headers.size(), first_chain.size());
+    }
 }
 
-static void TooLittleWork(const std::vector<CBlockHeader>& second_chain, const CBlockIndex* chain_start)
+static void TooLittleWork(
+    const std::span<const CBlockHeader> second_chain,
+    const CBlockIndex* chain_start)
 {
-    // Verify that just trying to process the second chain would not succeed
-    // (too little work).
+    // Verify that just trying to process the second chain would not succeed (too little work).
     HeadersSyncState hss{0, Params().GetConsensus(), chain_start, CHAIN_WORK};
     BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
     // Pretend just the first message is "full", so we don't abort.
-    auto result{hss.ProcessNextHeaders(std::span{second_chain.begin(), 1}, true)};
-    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
-    BOOST_CHECK(result.success);
-    BOOST_CHECK(result.request_more);
-    BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
+    {
+        const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders({second_chain.begin(), 1}, true)};
+        BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
+        BOOST_CHECK(success);
+        BOOST_CHECK(request_more);
+        BOOST_CHECK_EQUAL(pow_validated_headers.size(), 0);
+    }
 
     // Tell the sync logic that the headers message was not full, implying no
     // more headers can be requested. For a low-work-chain, this should cause
     // the sync to end with no headers for acceptance.
-    result = hss.ProcessNextHeaders(std::span{second_chain.begin() + 1, second_chain.end()}, false);
-    BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
-    BOOST_CHECK(!result.request_more);
-    BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
-    // Nevertheless, no validation errors should have been detected with the
-    // chain:
-    BOOST_CHECK(result.success);
+    {
+        const auto [pow_validated_headers, success, request_more]{hss.ProcessNextHeaders({second_chain.begin() + 1, second_chain.end()}, false)};
+        BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
+        BOOST_CHECK(!request_more);
+        BOOST_CHECK_EQUAL(pow_validated_headers.size(), 0);
+        // Nevertheless, no validation errors should have been detected with the chain:
+        BOOST_CHECK(success);
+    }
 }
 
 BOOST_AUTO_TEST_SUITE_END()

</details>

For other headers we've used an underscore instead of a hyphen in the file name: src/headerssync_params.h

What's the point of making a constructor that takes no arguments explicit?

Started renaming but then realized that we already have headerssync-params.py. It's not obvious to me that underscores would be more correct for C++ files. We do have other C++ files with hyphens.

Based on the Sonarcloud recommendations I eagerly checked which other constructors can be explicit, but exploring your question in more detail it seems there's barely any advantage in explicit default constructors.

It seems to be a mix indeed:

% find . -name "*.h" | grep '_' | wc -l
185

vs

% find . -name "*.h" | grep '-' | wc -l
39

and most of those are from the recent ipc changes:

% find . -name "*.h" | grep '-' | grep -v ipc | wc -l            
11

it's just a nit from my part, but standard lib also uses underscores mostly (e.g. string_view) and in many regexes the \w, i.e. a word character includes _ but usually not - (if I remember it correctly some bash implementations can also trip on -, but that's likely not a problem here).

Ah, the Sonarcloud recommendation was actually for the uint64_t-ctor, similar to my response here: #32579 (review).

Tested out making it explicit, modifying 13 additional files. It did uncover some implicit casts that went from int64_t Params::nPowTargetTimespan to arith_uint256... But the rest felt a bit forced. I think it is intentional and makes sense for u64s to implicitly cast to u256.

Yeah, seems src/ipc/ really went all in on -. Holding off on changing for now to see what others think.

Thanks for checking!

typo in commit message of 42e746096b93b8026569817d8925750089d6daf4

Fixed.

should we update https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md?plain=1#L140 as well?

Thanks, missed that!

I find the new comment slightly more confusing - the point is that the cpp and py assumptions should align, right?

static_assert(sizeof(CompressedHeader) == 48); // align with headerssync-params.py

The new comment is replacing the first (mention of python file) and last comments here: https://github.com/bitcoin/bitcoin/blob/11a2d3a63e90cdc1920ede3c67d52a9c72860e6b/src/headerssync.cpp#L12-L25

Your comment would probably be enough but I'd rather keep some more context.

nit: I find it a bit circular that the comment explaining a constant references the upcoming constant itself

//! Commitment period, in blocks

Added to separate WIP branch modifying generator script.

Latest push has:

struct HeadersSyncParams {
    //! Distance in blocks between header commitments.
    size_t commitment_period;
...

in src/kernel/chainparams.h.

Just realized a different possible approach: passing HEADER_COMMITMENT_PERIOD and REDOWNLOAD_BUFFER_SIZE values into the HeadersSyncState-ctor. Right now that aspect of the class is hardcoded for mainnet, despite the ctor accepting Consensus::Params. Having these values be sent in would enable tests to send in different (smaller) variables for regtest, while still testing behavior around the threshold.

In fact, it would fit nicely with other values updated during the release process to have them written into the corresponding section (mainnet/regtest/etc) of src/kernel/chainparams.cpp.

Started implementing that approach to see how it pans out.

Implemented in latest push.

I don't think it's regtest specific - is it?

The test is running on RegTest:

https://github.com/bitcoin/bitcoin/blob/42e746096b93b8026569817d8925750089d6daf4/src/test/headers_sync_chainwork_tests.cpp#L72

I'm guessing including the comment is meant to assuage fears that this would be an intensive operation.

ah, so it's jut a reassurance that it doesn't need a fancy asic, it can mine a block in no time - makes sense

Doc states: by delivering 1 header initially and then the rest. Could we clarify this by code instead? We don't really have head/tail like in many functional programming languages, but maybe we could do:

    const std::span headers_batch{second_chain};
    (void)hss->ProcessNextHeaders(headers_batch.first(1), true);
...
    result = hss->ProcessNextHeaders(headers_batch.last(headers_batch.size() - 1), false);

Would rather not go back to having an intermediate headers_batch variable. Changed to {{second_chain.front()}} + std::span{second_chain}.last(second_chain.size() - 1).

Kept the almost the existing style in HappyPath since I couldn't see an improvement to {first_chain.begin() + REDOWNLOAD_BUFFER_SIZE, 1}.

👍 for getting rid of this

👍 for splitting these up. Is it important to keep 1) and 2) as a single method, given that we thought it's best described in two separate list items? Since you grouped them into a "redownload", it's probably the comments that should be adjusted.

Agreed that 1 & 2 should ideally be combined if this were written from scratch, but I prefer keeping the current approach to make the diff less jarring compared to master. Open to re-evaluate if other reviewers weigh in.

Can we add 2 helpers in that case? I really dislike that the comments don't follow the code's structure.

Renumbered in the "Improve comments"-commit.

nit: Maybe mention in the previous commit message that "don't worry, I'm not gonna' leave the inits like that" :D

nit: I don't particularly like the abbreviated hss name - would const auto sync_state{CreateState(chain_start, Params())}; maybe be more descriptive?

net_processing.cpp uses m_headers_sync, but I'm totally fine with hss-abbreviation, especially if we keep spelling out HeadersSyncState. Makes for a slightly less stark diff with master.

nit:

        const auto result{hss.ProcessNextHeaders({first_chain.begin(), 1}, /*full_headers_message=*/true)};

Replaced with CHECK_RESULT() macro from your suggestion in #32579 (review). Sprinkled some less obvious /*full_headers_message=*/ around.

HeadersSyncState::State::PRESYNC is very verbose in a file called headers_sync_chainwork_tests - can we using State = HeadersSyncState::State; here instead to reduce the noise somewhat?

Done in the same commit as we switch to BOOST_REQUIRE_EQUAL.

do we really need these forward declarations here? Wouldn't it be simpler to just put the test at the end of the file like we're doing in other places e.g. https://github.com/bitcoin/bitcoin/blob/master/src/test/addrman_tests.cpp#L23-L61 ? If we do need the prototypes can we maybe remove the arg names to reduce the noise?

do we really need these forward declarations here? Wouldn't it be simpler to just put the test at the end of the file like we're doing in other places e.g. https://github.com/bitcoin/bitcoin/blob/master/src/test/addrman_tests.cpp#L23-L61 ?

Did it in order to weave the breakup into the existing comments, while avoiding to re-order chunks of code to minimize diffs.

If we do need the prototypes can we maybe remove the arg names to reduce the noise?

Done - not used to this style, but warming up to it. :)

chunks of code to minimize diffs

We've rewritten the tests at this stage, diffs and the function prototypes don't help in my opinion. But if you insist, I don't mind, please resolve the comment

first_chain/second_chain is a bit verbose given how often we're using them

Would rather keep the names to minimize diffs. sufficient_chain & insufficient_chain would have been more descriptive but longer.

this scenario could also use a comment like the rest

Done.

This is a bit hacky - can we add a negated helper to logging.h?

// Test fails if the pattern *DOES NOT* show up.
#define ASSERT_DEBUG_LOG(message) DebugLogHelper UNIQUE_NAME(debugloghelper)(message)
// Test fails if the pattern *DOES* show up.
#define ASSERT_NO_DEBUG_LOG(message)                                              \
    DebugLogHelper UNIQUE_NAME(nologhelper){                                      \
        message,                                                                  \
        [](const std::string* line) {                                             \
            if (line) throw std::runtime_error("unexpected log line: " + *line);  \
            return false; /* suppress default 'not found' failure */              \
        }                                                                         \
    }

And use it here as:

    // During normal operation we shouldn't get the redownload buffer size warning.
    ASSERT_NO_DEBUG_LOG(BUFFER_SIZE_WARNING);

Originally resisted the impulse since there is only one case, but done now. It mirrors what we do in functional Python tests after all.

we should be able to reuse CreateState here - if we make HeadersSyncParams configurable:

static HeadersSyncState CreateState(const CChainParams& chain_params, const HeadersSyncParams& sync_params, const CBlockIndex* chain_start)
{
    return {/*id=*/0,
            chain_params.GetConsensus(),
            sync_params,
            chain_start,
            /*minimum_required_work=*/CHAIN_WORK};
}

and

// Scenario that intentionally uses an oversized buffer to trigger the warning.
static void TooBigBuffer(const CBlockIndex* chain_start, const std::vector<CBlockHeader>& chain1)
{
    // Intentionally too big redownload buffer in order to trigger warning.
    HeadersSyncState state{CreateState(Params(), HeadersSyncParams{COMMITMENT_PERIOD, chain1.size()}, chain_start)};
    ...

Taken in broad strokes.

can we validate this as well instead of just voiding it?

CHECK_RESULT(state.ProcessNextHeaders(chain1, /*full_headers_message=*/true),
             state, State::REDOWNLOAD,
             /*exp_success=*/true,
             /*exp_request_more=*/true,
             /*exp_headers_size=*/0);

We could validate request_more and pow_validated_headers here as well:

CHECK_RESULT(state.ProcessNextHeaders(chain1, /*full_headers_message=*/true),
             state, State::FINAL,
             /*exp_success=*/true,
             /*exp_request_more=*/false,
             /*exp_headers_size=*/chain1.size());

👍 for reducing the scope of each separate check.

Was hoping we could compare the result object directly as:

BOOST_CHECK_EQUAL(result, (HeadersSyncState::ProcessingResult{
    .pow_validated_headers = {},
    .success = true,
    .request_more = true,
}));

But unfortunately we'd need to add equality operators to ProcessingResult and CBlockHeader which would defeat the purpose (+ the operator<<(std::ostream& for printing)...

But if we extract a validation helper function instead, we'd have keep the scope reduction and make the differences between the checks less noisy:

// Standard set of checks common to all scenarios; macro keeps failure lines at the call-site.
#define CHECK_RESULT(res, state, exp_state, exp_success, exp_request_more, exp_headers_size) \
    do {                                                                                     \
        const auto& [res_headers, res_success, res_request_more]{res};                       \
        BOOST_REQUIRE_EQUAL(state.GetState(), exp_state);                                    \
        BOOST_CHECK_EQUAL(res_headers.size(), exp_headers_size);                             \
        BOOST_CHECK_EQUAL(res_success, exp_success);                                         \
        BOOST_CHECK_EQUAL(res_request_more, exp_request_more);                               \
    } while (false)

and using it as e.g.

// Repeat the second set of headers in both phases to demonstrate
// behavior when the chain a peer provides has too little work.
static void TooLittleWork(const CBlockIndex* chain_start, const std::vector<CBlockHeader>& chain2)
{
    // Verify that just trying to process the second chain would not succeed (too little work).
    HeadersSyncState state{CreateState(Params(), HeadersSyncParams{COMMITMENT_PERIOD, REDOWNLOAD_BUFFER_SIZE}, chain_start)};

    // Pretend just the first message is "full", so we don't abort.
    CHECK_RESULT(state.ProcessNextHeaders({chain2.begin(), 1}, /*full_headers_message=*/true),
                 state, State::PRESYNC,
                 /*exp_success=*/true,
                 /*exp_request_more=*/true,
                 /*exp_headers_size=*/0);

    // Tell the sync logic that the headers message was not full, implying no
    // more headers can be requested. For a low-work-chain, this should cause
    // the sync to end with no headers for acceptance.
    CHECK_RESULT(state.ProcessNextHeaders({chain2.begin() + 1, chain2.end()}, /*full_headers_message=*/false),
                 state, State::FINAL,
                 /*exp_success=*/true,
                 /*exp_request_more=*/false,
                 /*exp_headers_size=*/0);
}

Taken, added exp_pow_validated_prev and exp_locator_hash-parameters. Feels good to verify all parameters, also always in the same order.

We'll update this during the v30 release?

As release-process.md documents, it's not critical to update on every release. It's mostly about fine-tuning memory usage under adversarial conditions, and the values don't tend to change that much. But I would not be surprised if it were updated since it was updated during the previous 2 releases:

• v29: 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b
• v28: 221809b81cfcecb04050915eebacffda2599da42

Also, we're slated to remove checkpoints (#31649) in v30 which might further encourage the release manager to do this.

👍

nit: I'd join this with the previous line, it's just confusing this way. And can we round this (given we already have a leading ~) instead of adding a weird decimal (what is 0.8 commitment mean?)

And why are we repeating the division in the comment, why not just:

print(f"            .redownload_buffer_size = {bufsize}}; // ~{round(bufsize / period)} commitments")

resulting in something like:

        // Generated by headerssync-params.py on 2025-03-04.
        m_headers_sync_params = HeadersSyncParams{
            .commitment_period = 624,
            .redownload_buffer_size = 14827}; // ~24 commitments

I'd join this with the previous line, it's just confusing this way.

Prefer keeping this line unchanged compared to master (would rather have lines closer to 80 than 120 chars here).

can we round this (given we already have a leading ~) instead of adding a weird decimal (what is 0.8 commitment mean?)

Including one decimal makes it more likely that the number changes between releases, indicating some kind of usefulness + precision of simulation. Not sure if that was what the original authors intended.

And why are we repeating the division in the comment

I think keeping the division makes it slightly clearer what the number is to readers of the C++ source.

makes it more likely that the number changes between releases

That sounds like a counter-argument to me, but don't have strong feelings about it, if you disagree, I don't mind, please resolve it.

• 48 is already stated in the code, no need to repeat it in the comment
• we should re-calculate parameters if we compress further - doesn't the static assert already obviate that?

48 is already stated in the code, no need to repeat it in the comment

Replaced with "this many".

we should re-calculate parameters if we compress further - doesn't the static assert already obviate that?

Don't think so? This is about prompting whoever changes the struct to also alter and re-run the Python script.

nice, we don't need the comment anymore after this change

Still think it is good to document why something is expected.

Not sure I understand, especially in the new code which states something like locator == genesis - which is basically the same as the comment

Q: I can't immediately tell if this should be triggered for equality as well - please just resolve if it's a stupid question.

Appreciate feedback on this!

Are you asking whether m_redownloaded_headers.size() < m_params.redownload_buffer_size might be more correct?

The while-condition below includes m_redownloaded_headers.size() > m_params.redownload_buffer_size as one of the conditions to start feeding back PoW-validated headers. This means using m_redownloaded_headers.size() < m_params.redownload_buffer_size for the condition above would give us no headers and no warning when the buffer size equals the parameter.

Hmmm, if it has to be the opposite of the while below, would it make sense to extract the common parts to explain it with code (your explanation makes sense, would prefer if the code already made it simpler so that I don't have to check the code that's after the current one to understand it).

Broke out bool buffer_exceeded, let me know if you think it's an improvement.

I don't like the new repetition - will have to debug it locally to understand why we run the condition outside of the loop in the first place 🤔

Latest push improves this part of the code, please check it out!

would't an .at(0) suffice?

Slightly more confusing error, but like that it's more terse. Taken.

I think it applies to BOOST_REQUIRE_GT(result.pow_validated_headers.size(), 0); as well

True, sole change in latest push.

this comment is obvious from the code now: /*exp_locator_hash=*/Params().GenesisBlock().GetHash() or /*exp_locator_hash=*/genesis_hash later in 65a96b507c11b3f34efa996919892fa1a2fcf49c

I'm not sure what "this many headers on top have been received"

Agree. New attempt:

//! Only start outputting headers once this many headers have been received
//! and validated against commitments.

is this comment accurate?

No, rebase mishap. Thanks for catching!

// 5. Sets the redownload buffer size to be large enough that we

nice, this is exactly how comments should be used to augment what the code cannot easily tell <3

might make sense to extract genesis hash here as well like we did in HappyPath

Just to confirm that I understand this correctly, are you removing the <256> because it's implicit?

Yes, they're redundant, can be deduced by the compiler

nit in 79bdc6a785bca96dababc698336c04b8625b7d1c: in some CHECK_RESULT you are putting /*full_headers_message=*/, in some others you're not, like this one, the one below, and the ones in TooLittleWork

In this case we have the comment right above describing the same thing: https://github.com/bitcoin/bitcoin/blob/79bdc6a785bca96dababc698336c04b8625b7d1c/src/test/headers_sync_chainwork_tests.cpp#L140

Went through the code and realized HappyPath was repeating /*full_headers_message=*/ in one piece of newer code, so removed that.

This boolean parameter to ProcessNextHeaders() is somewhat of a code smell. Maybe could be split into 2 public functions (ProcessNextHeaders() + ProcessLastHeaders()... but that would make the code as a whole more convoluted I think).

nit in ced2ddb37c957e53d4a8aa40d3512ea3786511b4: the previous comment (in headerssync.cpp) said: "Only feed headers to validation once this many headers.."

I think that phrasing was a bit clearer than “outputting”

Changed to the following, let me know what you have further suggestions:

Start feeding back headers into the permanent block index once more than this number of them have been received and validated against commitments.

micro nit in 79bdc6a785bca96dababc698336c04b8625b7d1c: you could check exp_pow_validated_prev first and exp_locator_hash second, to maintain the same order as the macro parameters

Agree that's cleaner, fixed! Also changed from == std::nullopt to .has_value().

nit: we already have opt in the name, consider:

if (pow_validated_prev_opt) {                                                                    \

nit2: result is const, consider unifying:

        const std::optional<uint256> pow_validated_prev_opt{exp_pow_validated_prev};                     \

It took me a while to figure this out - I was initially wondering why the chains have TARGET_BLOCKS - 1 and TARGET_BLOCKS - 2 blocks, until I realized that I wasn't counting the genesis block 😅

I'm not sure if it might be worth it to add a comment like // The first chain will contain TARGET_BLOCKS, the second one TARGET_BLOCKS - 1

It might be obvious already, feel free to drop the suggestion

It's not obvious to me, thanks for sending me down a rabbit hole. :) Please correct me if I'm wrong but this is my refreshed understanding:

• CBlockHeader::nBits is the PoW target for the current difficulty period (in compact int format). https://learnmeabitcoin.com/technical/block/bits/
• GetBlockProof() is somewhat terse in comments (since inception in df9eb5e14fa8072bc8a82b59e712c2ba36f13f4c), but as I understand it: Given a target nBits, it returns an estimate for how many SHA256 hash-operations on average are required to mine a given block.
• Total PoW of a chain, CBlockIndex::nChainWork, is actually the sum of the targets, not the actual block hashes. This somewhat narrows my perception of claims I've heard previously that Bitcoin follows the highest PoW chain rather than the longest chain (so it's true as far as difficulty periods go, but not on a per-block level). This has the gambler side of me a bit disappointed, but okay. https://github.com/bitcoin/bitcoin/blob/78503a41546f6432045d3f9fd39860ead11d5c95/src/node/blockstorage.cpp#L227

Based on the above, I've added comments to the "doc(test): Improve comments"-commit.

Thanks a lot for the explanation, and the updated comments! It's much more understandable now.

It also confused me for a second but for a different reason, I though they're heights, maybe adding a count hint

GenerateHeaders(first_chain, /*count=*/TARGET_BLOCKS - 1, genesis.GetHash(), genesis.nVersion,

would help.

A couple of questions regarding these two:

• REDOWNLOAD_BUFFER_SIZE: can we add a comment explaining why subtracting MAX_HEADERS_RESULT + 123? Copy-pasting what's in the commit description is ok
• COMMITMENT_PERIOD = 600: I suppose this is just mimicking the mainnet value?

Good call - added comments and removed the redundant part from the commit message.

57a351912b057d1a2234d4a3b3dd0c745690e3d0: this new addition temporarily invalidates the introductory comment describing the workings of headers_sync_state

Are you referring to this? https://github.com/bitcoin/bitcoin/blob/57a351912b057d1a2234d4a3b3dd0c745690e3d0/src/test/headers_sync_chainwork_tests.cpp#L66-L67

We are feeding in the same chain twice, it's just that we cut it up in smaller pieces.

you're updating the comments in a later commit, adding the "happy path", but the code that corresponds to it was added here

Hoping the change to the happy path is clearer now with the extracted final commit.

nit: joining the lines would reduce the noise, we have longer lines than that - which would still be below 100 chars anyway nit2: in other cases we ended with a :, not a .

👍, test passes with REDOWNLOAD_BUFFER_SIZE{15827} without this commit, fails after the addition, as expected

(This is the check that fails for me specifically): https://github.com/bitcoin/bitcoin/blob/57a351912b057d1a2234d4a3b3dd0c745690e3d0/src/test/headers_sync_chainwork_tests.cpp#L127

../src/test/headers_sync_chainwork_tests.cpp(127): error: in "headers_sync_chainwork_tests/headers_sync_state": check result.pow_validated_headers.size() > 0 has failed [0 <= 0]

Same check fails for me too 🤝

nit: because of these changes the commit message should likely be adjusted: the refactor is not test-only anymore

d9cf9c7c2a1cbe9e99b3b5e641b3a94a149c8ee8 nit: since we're editing this already, consider removing the primitive consts:

        size_t count, const uint256& starting_hash, int32_t nVersion, uint32_t prev_time,
        const uint256& merkle_root, uint32_t nBits)

slightly unrelated: this is a fixed iteration, it implicitly assumes headers is empty when we get here - maybe we can make that clearer here or in another follow-up by explicitly iterating from 0 or headers.size() to count instead:

void HeadersGeneratorSetup::GenerateHeaders(
    std::vector<CBlockHeader>& headers,
    size_t count, const uint256& starting_hash, int32_t nVersion, uint32_t prev_time,
    const uint256& merkle_root, uint32_t nBits)
{
    for (size_t i{0}; i < count; ++i) {
        auto prev_hash{i ? headers.back().GetHash() : starting_hash};
        auto& next_header{headers.emplace_back()};
        next_header.nVersion = nVersion;
        next_header.hashPrevBlock = prev_hash;
        next_header.hashMerkleRoot = merkle_root;
        next_header.nTime = ++prev_time;
        next_header.nBits = nBits;

        FindProofOfWork(next_header);
    }
}

Or since we're already reassigning arguments:

void HeadersGeneratorSetup::GenerateHeaders(
    std::vector<CBlockHeader>& headers,
    size_t count, uint256 prev_hash, int32_t nVersion, uint32_t prev_time,
    const uint256& merkle_root, uint32_t nBits)
{
    for (size_t i{0}; i < count; ++i) {
        auto& next_header{headers.emplace_back()};
        next_header.nVersion = nVersion;
        next_header.hashPrevBlock = prev_hash;
        next_header.hashMerkleRoot = merkle_root;
        next_header.nTime = ++prev_time;
        next_header.nBits = nBits;

        FindProofOfWork(next_header);
        prev_hash = next_header.GetHash();
    }
}

What's changed:

• fixed for loop, since we know exactly the number of iterations;
• prev_hash can be localized, we don't need to update it based on the iteration's state;
• or prev_hash arg can be copied and reassigned, like we did with prev_time already;
• prev_time is basically just incremented, no need to assign it back and forth;
• no point in emplace_back + back(), let's just store the result of the emplace;
• edit: nVersion and nTime should be a int32_t instead.

Maybe this reduces the need for such a detailed documentation as well (e.g. the advance of 1 second is obvious now - it wasn't obvious before)

Incorporated many of your suggestions, latest version:

https://github.com/bitcoin/bitcoin/blob/a4191b57b21a0fea871206f368972c7741ad6437/src/test/headers_sync_chainwork_tests.cpp#L60-L80

• Return value enables making first_chain/second_chain const from the beginning.
• Looping over headers means we don't need the explicit i.
• Still keeping const on primitive arguments which we do not modify within function body, such as nVersion. (Agree that they don't belong if we had a separate function declaration without the body).
• Shortened the comment about advancing 1 second.

hah, that's even better! I still recommend changing prev_time and nVersion to uint32_t

Adjusted - CBlockHeader::nVersion is int32_t however, so matched that.

nit:

    const auto& genesis{Params().GenesisBlock()};

nit2: we may want to extract its hash as well

Added ampersand but keeping the 3 GetHash() calls for now.

not sure what "starting" refers to here, since we're mutating it, so it will also store all intermediary and final header states as well

Name could be better, but keeping it for now as it helps make the diff somewhat less jarring.

058ebc1b02ccfc75bbe5178a4191aebdcf9b4d3a: might make sense to phrase the comment like this in the first place to minimize diffs

Postponed updating this and one other comment until the "Improve comments" commit.

Didn't we just add this comment a few commits before? I meant it may make sense to phrase it such that we don't need changing later

~Hm...~ (too trigger happy with publishing)

Didn't we just add this comment a few commits before?

No

Based on your initial comment in this thread, I undid modifying the comment from master until the Improve comments commit (0863bde64e62b040da749161f7bd9c5628759a18, this aspect is unchanged from previous push).

can it ever be null or can we do static HeadersSyncState CreateState(const CBlockIndex& chain_start) by asserting the lookup:

    const CBlockIndex& chain_start{*Assert(WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};

Would rather postpone this until #32740 (which also changes it to a reference type in the HeadersSyncState-ctor) has been merged.

Arguably this is where the tests start and the above methods are helpers, consider moving BOOST_FIXTURE_TEST_SUITE(headers_sync_chainwork_tests, RegTestingSetup) here

It's true that many tests have BOOST_FIXTURE_TEST_SUITE directly followed by BOOST_AUTO_TEST_CASE, but these methods (TooLittleWork etc) are tightly coupled with the headers_sync_state test case. They could almost be their own test cases entirely but I'd rather avoid repeated cycles to generate the chains. (Actually (re?)tested implementing separate test cases but fixtures are re-instantiated for each test case, so runtime went from 0.39 to 0.65, added note in commit message breaking up into functions).

nit: as mentioned, we should only move things around here, this comment should be added to where we've introduced the happy path

~I find it hard to follow, are you referring to a specific commit?~

Hopefully this is the same as #32579 (review)

why do you think we need this whole new setup?

I'm not sure I understand why we're checking a constant, we've just assigned it to TARGET_BLOCKS * 2 a few lines before

It's my way of documenting why we are multiplying by 2. :) Comes from the research I did in this thread: #32579 (review)

I don't think it's useful, it's literally just stating:

constexpr arith_uint256 CHAIN_WORK{TARGET_BLOCKS * 2};
...
assert(CHAIN_WORK == TARGET_BLOCKS * 2);

Ideally the genesis-block and the other expressions would be constexpr, so these could be static_asserts right after CHAIN_WORK and TARGET_BLOCKS. But given they are 90+ lines apart, I'd rather keep it like this. Hm.. I could experiment with ways of decreasing that line distance if I re-push (extracting function bodies and putting them later, moving these constants after the initial functions).

Moved up the macro above the constants in the latest push, and brought the assert block up closer to the constants - in the process removing the final assert.

this looks like a list item, starting with -, not sure it helps with understanding the code better. Also, explaining TARGET_BLOCKS - 2 at the TARGET_BLOCKS - 1 line is also a bit confusing.

Agreed, the - 1 and -2 part of comment was confusing to me at first.

Adjusted comment to avoid list item interpretation.

nit: to be consistent with the following lines

        const auto result{result_expression};                                                            \

👍, it seems to generate to correct values currently:

pypy3 contrib/devtools/headerssync-params.py

Searching configurations:
- Initial: period=618, buffer=14672, mem=701.418 KiB
- New best: period=629, buffer=14955, mem=701.188 KiB
- New best: period=622, buffer=14775, mem=696.908 KiB
- New best: period=625, buffer=14852, mem=696.360 KiB
- New best: period=623, buffer=14801, mem=695.789 KiB
- New best: period=624, buffer=14827, mem=695.189 KiB

Given current min chainwork headers of 886157, the optimal parameters for low
memory usage on mainchain for release until 2027-10-06 is:

        // Generated by headerssync-params.py on 2025-08-12.
        m_headers_sync_params = HeadersSyncParams{
            .commitment_period = 624,
            .redownload_buffer_size = 14827, // 14827/624 = ~23.8 commitments
        };

Properties:
- Per-peer memory for mainchain sync: 695.189 KiB
- Per-peer memory for timewarp attack: 694.674 KiB
- Attack rate: 31382.7 attacks for 1 header of memory growth
  (where each attack costs 68.454 MiB bandwidth)

The new value after the rebase is:

- Initial: period=626, buffer=14854, mem=710.549 KiB
- New best: period=634, buffer=15061, mem=706.160 KiB
- New best: period=632, buffer=15009, mem=703.804 KiB

Given current min chainwork headers of 912683, the optimal parameters for low
memory usage on mainchain for release until 2028-04-02 is:

        // Generated by headerssync-params.py on 2025-09-04.
        m_headers_sync_params = HeadersSyncParams{
            .commitment_period = 632,
            .redownload_buffer_size = 15009, // 15009/632 = ~23.7 commitments
        };

Properties:
- Per-peer memory for mainchain sync: 703.723 KiB
- Per-peer memory for timewarp attack: 703.804 KiB
- Attack rate: 30460.2 attacks for 1 header of memory growth
  (where each attack costs 70.503 MiB bandwidth)

which matches https://github.com/bitcoin/bitcoin/blob/eb0f41ee1ed9ea54019fcaa4e3ce33481da4459d/src/kernel/chainparams.cpp#L200-L203 (I haven't checked the other chains)

nit: given size_t commitment_period we might as well:

    m_commit_offset(FastRandomContext().randrange<size_t>(params.commitment_period)),

Changed HeadersSyncState::m_commit_offset from unsigned to size_t as well to remove the explicit template parameter.

nit: might as well format this monstrosity to reduce conflict with https://github.com/bitcoin/bitcoin/pull/32740/files#diff-d27c4ae51fdaf60573118eb2f5c44276de12c45bd1948da5613a07a136e4d5faR43

Extracted max_seconds_since_start.

The comment on redownload_buffer_size is quite cumbersome, starts with an instruction and keeps going in circles, please consider:

//! Configuration for Headers-Sync memory usage.
struct HeadersSyncParams
{
    //! Distance, in blocks, between header commitments.
    size_t commitment_period;
    //! Minimum number of validated headers to accumulate in the redownload
    //! buffer before feeding them into the permanent block index.
    size_t redownload_buffer_size;
};

a4191b57b21a0fea871206f368972c7741ad6437 is a bit complicated, can you split it up to smaller refactors?

I broke out many of the chainwork test changes into a later commit. Beyond that, I have a hard time untangling things elegantly.

Could change these to zero to better show they aren't needed.

The commitment_period needs to be non-zero for functional tests. Example:

₿ ./build/test/functional/p2p_headers_sync_with_minchainwork.py
[1/13] Generating bitcoin-build-info.h
2025-08-13T19:03:34.693000Z TestFramework (INFO): PRNG seed is: 3053858877177300552
2025-08-13T19:03:34.694000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_cm19o0ak
2025-08-13T19:03:35.502000Z TestFramework (INFO): Generate blocks on the node with no required chainwork, and verify nodes 1 and 2 have no new headers in their headers tree
2025-08-13T19:03:35.580000Z TestFramework (INFO): Check that node3 will sync headers (due to noban permissions)
2025-08-13T19:03:35.588000Z TestFramework (INFO): Generate more blocks to satisfy node1's minchainwork requirement, and verify node2 still has no new headers in headers tree
2025-08-13T19:03:35.647000Z TestFramework (INFO): Check that node3 accepted these headers as well
2025-08-13T19:03:35.649000Z TestFramework (INFO): Generate long chain for node0/node1/node3
2025-08-13T19:03:36.337000Z TestFramework (INFO): Verify that node2 and node3 will sync the chain when it gets long enough
2025-08-13T19:03:36.890000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 184, in main
    self.run_test()
  File "/home/hodlinator/bitcoin/./build/test/functional/p2p_headers_sync_with_minchainwork.py", line 164, in run_test
    self.test_chains_sync_when_long_enough()
  File "/home/hodlinator/bitcoin/./build/test/functional/p2p_headers_sync_with_minchainwork.py", line 108, in test_chains_sync_when_long_enough
    self.sync_blocks()
  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 777, in sync_blocks
    best_hash = [x.getbestblockhash() for x in rpc_connections]
                 ^^^^^^^^^^^^^^^^^^^^
  File "/home/hodlinator/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hodlinator/bitcoin/test/functional/test_framework/authproxy.py", line 132, in __call__
    response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hodlinator/bitcoin/test/functional/test_framework/authproxy.py", line 106, in _request
    return self._get_response()
           ^^^^^^^^^^^^^^^^^^^^
  File "/home/hodlinator/bitcoin/test/functional/test_framework/authproxy.py", line 169, in _get_response
    http_response = self.__conn.getresponse()
                    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/fqm9bqqlmaqqr02qbalm1bazp810qfiw-python3-3.12.9/lib/python3.12/http/client.py", line 1430, in getresponse
    response.begin()
  File "/nix/store/fqm9bqqlmaqqr02qbalm1bazp810qfiw-python3-3.12.9/lib/python3.12/http/client.py", line 331, in begin
    version, status, reason = self._read_status()
                              ^^^^^^^^^^^^^^^^^^^
  File "/nix/store/fqm9bqqlmaqqr02qbalm1bazp810qfiw-python3-3.12.9/lib/python3.12/http/client.py", line 292, in _read_status
    line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1")
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/fqm9bqqlmaqqr02qbalm1bazp810qfiw-python3-3.12.9/lib/python3.12/socket.py", line 720, in readinto
    return self._sock.recv_into(b)
           ^^^^^^^^^^^^^^^^^^^^^^^
ConnectionResetError: [Errno 104] Connection reset by peer

...and it's currently a bit tricky to get functional tests to be able to specify values.

But this prompted me to make the fields default to zero in the struct definition and add an assert in the HeadersSyncState-ctor.

👍 nice

we already have an Assume(range > 0) inside, isn't that enough?

True, but Assume()s are not always active, and checking it here means we can describe the constraint with a comment as is done in this project.

nit: the AI gods think this should be:

// required to reach the CHAIN_WORK threshold, to behave like mainnet.

or

// required to reach the CHAIN_WORK threshold, to behave similarly to mainnet.

Cheers, fixed in latest push!

Because commit_offset will never be greater than commitment_period:

         const HeadersSyncParams params{
        .commitment_period = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, Params().HeadersSync().commitment_period * 2),
        .redownload_buffer_size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, Params().HeadersSync().redownload_buffer_size * 2),
    };
    const size_t commit_offset = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, params.commitment_period - 1);
    FuzzedHeadersSyncState headers_sync(
        params,
        commit_offset,
        /*chain_start=*/&start_index,
        /*minimum_required_work=*/min_work);

Although the fuzzer will come up with correct values at some point, so this isn't too critical.

Thanks! Pushed almost the same.

In 232485d2486f08221203224212d5451a47eb4fc8 "refactor(test): Break up headers_sync_chainwork_tests"

Our preferred pattern for test cases within unit tests is to define them with BOOST_AUTO_TEST_CASE rather than functions like these.

Was concerned about increasing the runtime of the tests due to repeatedly re-generating first_chain/second_chain as documented in the commit message:

The approach to use one BOOST_AUTO_TEST_CASE for each scenario was attempted but the common initialization is repeated for every test case using the same fixture. This made the local runtime go from 0.39s to 0.65s. Would rather avoid global fixtures, so continued with one boost test case for now.

However, I figured out an okay way to switch back to using a fixture while avoiding to re-generate the chains:

    const std::vector<CBlockHeader>& FirstChain()
    {
        static const auto first_chain{GenerateHeaders(/*count=*/TARGET_BLOCKS - 1, genesis.GetHash(),
                genesis.nVersion, genesis.nTime, /*merkle_root=*/uint256::ZERO, genesis.nBits)};
        return first_chain;
    }

I'm running some final tests on this but thought I'd give a heads up to other reviewers.

Pushed version with multiple test cases.

nit: The sneaky redownload and insufficient work cases should be differentiated by making the second chain have enough work, e.g.: https://github.com/davidgumberg/bitcoin/commit/1b1b7f2d06e3b11eac0f844002ff4c1bcf497b0d

That might be a cool higher level change, although hopefully a commitment will mismatch before we reach the final header.

Hm... thinking through the current code - given 15'000 headers and a period of 600, we have 38 commitments. So there's a 1 in 2^38 chance that all commitments will match. There's a 1/600 chance that HeadersSyncState::commit_offset will be 599, in which case second_chain would be too short to check the last commitment, making it a 1 in 2^37 chance of test failure for that offset. (In that case, we would not reach the PoW limit from processing the second chain and the CHECK_RESULT() below will explode).

https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/test/headers_sync_chainwork_tests.cpp#L168-L172

Although this aspect isn't materially changed by this PR, I agree it would be good to at least document these probabilities in the test. Will do if I re-push.

(Also note that the chain generation functions use static internally in order to avoid increasing the runtime of the test).

I don't think we should be that worried about a $(\frac{599}{600} * \frac{1}{2^{38}}) + (\frac{1}{600} * \frac{1}{2^{37}})$ probability of spurious test failure, but we could just double the number of headers if that's a concern.

Had my calculator set to hexadecimal when getting it to 38 commitments. :face_in_clouds: The risk of spurious test failure is closer to $\frac{1}{2 ^ {25}}$ = one in 33,554,432. Added comment + static_assert in sneaky_redownload before the check that would fail.

It really feels like the default constructor for this should be deleted, but that's not possible because CChainParams() default constructs HeadersSyncParams: https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/kernel/chainparams.h#L165

Ah, I missed that one of the params gets checked for not being default-constructed here:

https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/headerssync.cpp#L20

Agreed, would be nice to have each chain's params be a constexpr expression tree in an ideal world.

why limit these to * 2?

Giving us 2x mainnet should give the fuzzers enough time to break things before we reach that level. See 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b for an example of how gradually these values change for 1 release.

nit, I think this was wrong in the original test as well, should be:

    CHECK_RESULT(hss.ProcessNextHeaders(std::span{first_chain}.last(first_chain.size() - 1), false),

I think the idea is to pretend that our peer has longer chain, meaning they are able to send us more headers, it's just that HeadersSyncState should be detecting that we've met the threshold and switch to REDOWNLOAD even though we tempt it that there are more headers in case it erroneously wants to continue PRESYNC.

nit: I think this comment was incorrectly placed in the original, it is the ProcessNextHeaders above that needs /*full_headers_message=*/true

Seems related to your previous GH-comment. Current PR version has the comment: https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/test/headers_sync_chainwork_tests.cpp#L153

My bad, what I mean is that this comment:

// Pretend the message is still "full", so we don't abort.

Belongs to the ProcessNextHeaders above.

Base version for this PR: https://github.com/bitcoin/bitcoin/blob/fa05a726c225dc65dee79367bb67f099ae4f99e6/src/test/headers_sync_chainwork_tests.cpp#L95-L98

That comment would indeed be more useful before the first ProcessNextHeaders-call. Will correct if I push again.

Done.

nit:

    CHECK_RESULT(hss.ProcessNextHeaders({first_chain.begin() + REDOWNLOAD_BUFFER_SIZE + 1, first_chain.end()}, false),

Agree here, it seems to be a more taxing test to not pretend like there are any more headers, that HeadersSyncState will have to make do. Will consider changing this if I push, although it is the same on master. Maybe making it random or doing a for x in {true, false} loop over it would be even better.

Done.

Does https://github.com/bitcoin/bitcoin/blob/eb0f41ee1ed9ea54019fcaa4e3ce33481da4459d/src/headerssync.h#L183 need updating?

Thanks for catching this! Pushed fix.

I'm still not comfortable with this assert here, can we assign it in the body or do an immediately-invoked lambda instead?

    m_commit_offset{[&]() {
          assert(params.commitment_period > 0);
          return FastRandomContext().randrange(params.commitment_period);
      }()},

Note that in this case the comment can also be eliminated

What makes a 2-statement lambda better than https://en.cppreference.com/w/cpp/language/operator_other.html#Built-in_comma_operator in this case?

Also not sure why it changes the need for the comment.