Repository: bitcoin/bitcoin
File: src/test/headers_sync_chainwork_tests.cpp (C++)

Code before:

    while (headers.size() < count) {
        headers.emplace_back();
        CBlockHeader& next_header = headers.back();;
        next_header.nVersion = nVersion;
        next_header.hashPrevBlock = prev_hash;
        next_header.hashMerkleRoot = merkle_root;
        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)

// In this test, we construct two sets of headers from genesis, one with
// sufficient proof of work and one without.
// 1. We deliver the first set of headers and verify that the headers sync state
//    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).
// 3. Finally, we verify that repeating with the first set of headers in both
//    phases is successful.
BOOST_AUTO_TEST_CASE(headers_sync_state)
{
    std::vector<CBlockHeader> first_chain;
    std::vector<CBlockHeader> second_chain;

    std::unique_ptr<HeadersSyncState> hss;

    const int target_blocks = 15000;
    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(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()));
    std::vector<CBlockHeader> headers_batch;

    // Feed the first chain to HeadersSyncState, by delivering 1 header

Review comment (bug):
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
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

Code after:
        }                                                                                                \
    } while (false)

constexpr size_t TARGET_BLOCKS{15'000};
constexpr arith_uint256 CHAIN_WORK{TARGET_BLOCKS * 2};

// Subtract MAX_HEADERS_RESULTS (2000 headers/message) + an arbitrary smaller
// value (123) so our redownload buffer is well below the number of blocks
// required to reach the CHAIN_WORK threshold, to behave similarly to mainnet.
constexpr size_t REDOWNLOAD_BUFFER_SIZE{TARGET_BLOCKS - (MAX_HEADERS_RESULTS + 123)};
constexpr size_t COMMITMENT_PERIOD{600}; // Somewhat close to mainnet.

struct HeadersGeneratorSetup : public RegTestingSetup {
    const CBlock& genesis{Params().GenesisBlock()};
    const CBlockIndex* chain_start{WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash()))};

    // Generate headers for two different chains (using differing merkle roots
    // to ensure the headers are different).
    const std::vector<CBlockHeader>& FirstChain()
    {
        // Block header hash target is half of max uint256 (2**256 / 2), expressible
        // roughly as the coefficient 0x7fffff with the exponent 0x20 (32 bytes).
        // This implies around every 2nd hash attempt should succeed, which
        // is why CHAIN_WORK == TARGET_BLOCKS * 2.
        assert(genesis.nBits == 0x207fffff);

        // Subtract 1 since the genesis block also contributes work so we reach
        // the CHAIN_WORK target.
        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;
    }
    const std::vector<CBlockHeader>& SecondChain()
    {
        // Subtract 2 to keep total work below the target.
        static const auto second_chain{GenerateHeaders(/*count=*/TARGET_BLOCKS - 2, genesis.GetHash(),
                genesis.nVersion, genesis.nTime, /*merkle_root=*/uint256::ONE, genesis.nBits)};
        return second_chain;
    }

    HeadersSyncState CreateState()
    {
        return {/*id=*/0,
                Params().GetConsensus(),
                HeadersSyncParams{
                    .commitment_period = COMMITMENT_PERIOD,
                    .redownload_buffer_size = REDOWNLOAD_BUFFER_SIZE,
                },
                chain_start,
                /*minimum_required_work=*/CHAIN_WORK};
    }