refactor: Header sync optimisations & simplifications #32740

pull danielabrozzoni wants to merge 8 commits into bitcoin:master from danielabrozzoni:upforgrabs/25968 changing 15 files +78 −82
  1. danielabrozzoni commented at 8:20 pm on June 12, 2025: member

    This is a partial* revival of #25968

    It contains a list of most-unrelated simplifications and optimizations to the code merged in #25717:

    • Avoid an IsAncestorOfBestHeaderOrTip call: Just don’t call this function when it won’t have any effect.
    • Compute work from headers without CBlockIndex: Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Also use some Spans where possible.
    • Remove useless CBlock::GetBlockHeader: There is no need for a function to convert a CBlock to a CBlockHeader, as it’s a child class of it.

    It also contains the following code cleanups, which were suggested by reviewers in #25968:

    • Remove redundant parameter from CheckHeadersPoW: No need to pass consensusParams, as CheckHeadersPow already has access to m_chainparams.GetConsensus()
    • Remove unused parameter in ReportHeadersPresync
    • Use initializer list in CompressedHeader, also make GetFullHeader const
    • Use reference for chain_start in HeadersSyncState: chain_start can never be null, so it’s better to pass it as a reference rather than a raw pointer

    *I decided to leave out three commits that were in #25968 (4e7ac7b94d04e056e9994ed1c8273c52b7b23931, ab52fb4e95aa2732d1a1391331ea01362e035984, 7f1cf440ca1a9c86085716745ca64d3ac26957c0), since they’re a bit more involved, and I’m a new contributor. If this PR gets merged, I’ll comment under #25968 to note that these three commits are still up for grabs :)

  2. DrahtBot commented at 8:20 pm on June 12, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32740.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc
    Stale ACK polespinasa
    User requested bot ignore hodlinator

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33856 (kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState by yuvicc)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Refactoring on Jun 12, 2025
  4. ajtowns commented at 2:43 am on June 13, 2025: contributor
    Maybe change the title to describe what’s actually going on for those of us who don’t memorise every pr number? :) (“Header sync optimisations/simplifications” ?)
  5. danielabrozzoni renamed this:
    refactor: Optimizations & simplifications following #25717
    refactor: Header sync optimisations & simplifications #25717
    on Jun 13, 2025
  6. danielabrozzoni renamed this:
    refactor: Header sync optimisations & simplifications #25717
    refactor: Header sync optimisations & simplifications
    on Jun 13, 2025
  7. in src/headerssync.h:144 in 16d16f3e08 outdated
    139@@ -140,33 +140,30 @@ class HeadersSyncState {
    140 
    141     /** Result data structure for ProcessNextHeaders. */
    142     struct ProcessingResult {
    143-        std::vector<CBlockHeader> pow_validated_headers;
    


    hodlinator commented at 8:42 am on June 16, 2025:

    Not thrilled by increasing the number of in/out parameters - it pushes readers towards having to lean on comments more, rather than having the code somewhat self-documenting. It also makes the unit tests more cumbersome. What makes the change somewhat acceptable is that it conforms to the style of net_processing.cpp.

    If we keep this, I would also suggest further simplification of ProcessingResult through converting it to an enum: f2a617955e65682b0765c7909c06aea19ca43eae


    hodlinator commented at 1:46 pm on August 12, 2025:
    Thanks for dropping the commit introducing the in/out parameter!
  8. in src/validation.cpp:4202 in 13e123e7cc outdated
    4198@@ -4199,10 +4199,7 @@ bool IsBlockMutated(const CBlock& block, bool check_witness_root)
    4199 arith_uint256 CalculateClaimedHeadersWork(std::span<const CBlockHeader> headers)
    4200 {
    4201     arith_uint256 total_work{0};
    4202-    for (const CBlockHeader& header : headers) {
    4203-        CBlockIndex dummy(header);
    4204-        total_work += GetBlockProof(dummy);
    4205-    }
    4206+    for (const CBlockHeader& header : headers) total_work += GetBlockProof(header);
    


    hodlinator commented at 8:52 am on June 16, 2025:

    nit in 2dcf2e4a854c21b71dc133e2fd891b0757204f43: IMO should keep braces as they already existed on master and only if are allowed to omit braces according to developer-notes.md.

    0    for (const CBlockHeader& header : headers) {
    1        total_work += GetBlockProof(header);
    2    }
    
  9. in src/headerssync.cpp:1 in 13e123e7cc outdated


    hodlinator commented at 8:58 am on June 16, 2025:

    Could simplify GetBlockProof calls in line with the other changes:

     0@@ -207,7 +207,7 @@ bool HeadersSyncState::ValidateAndProcessSingleHeader(const CBlockHeader& curren
     1         }
     2     }
     3 
     4-    m_current_chain_work += GetBlockProof(CBlockIndex(current));
     5+    m_current_chain_work += GetBlockProof(current);
     6     m_last_header_received = current;
     7     m_current_height = next_height;
     8 
     9@@ -243,7 +243,7 @@ bool HeadersSyncState::ValidateAndStoreRedownloadedHeader(const CBlockHeader& he
    10     }
    11 
    12     // Track work on the redownloaded chain
    13-    m_redownload_chain_work += GetBlockProof(CBlockIndex(header));
    14+    m_redownload_chain_work += GetBlockProof(header);
    15 
    16     if (m_redownload_chain_work >= m_minimum_required_work) {
    17         m_process_all_remaining_headers = true;
    

    hodlinator commented at 1:28 pm on August 12, 2025:

    (Inline comment in random place).

    It seems like the PR has an odd parent commit? Being based upon 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b which is within another PR’s commits, rather than the merge into master which was 9f3dcacef733ff7894aedd1bca673dfaad7dfd10.


    danielabrozzoni commented at 9:12 am on August 18, 2025:
    Ooops, not sure what happened there, thanks for noticing!

    l0rinc commented at 2:59 am on August 21, 2025:
    another super-nit: CheckHeadersPow -> CheckHeadersPoW in commit message. Only change if you modify the PR for any other reason.
  10. in src/headerssync.h:41 in 13e123e7cc outdated
    46+        hashMerkleRoot(header.hashMerkleRoot),
    47+        nTime(header.nTime),
    48+        nBits(header.nBits),
    49+        nNonce(header.nNonce) {}
    50+
    51+    CBlockHeader GetFullHeader(const uint256& hash_prev_block) const {
    


    hodlinator commented at 9:00 am on June 16, 2025:

    nit in 3ff26e5c060a1076c2ae61ec521b7595e413dd08: Should have brace on new line as per developer-notes.md.

    0    CBlockHeader GetFullHeader(const uint256& hash_prev_block) const
    1    {
    
  11. hodlinator commented at 9:19 am on June 16, 2025: contributor

    Reviewed 13e123e7cc185760bb9dafc1a6892c92112e5450

    Thanks for picking this up!

    Conflicts somewhat with #32579 where I was aiming to change the headers-parameter to a span. But if we follow through and also take my enum suggestion I would still see it as an improvement on balance (see inline comment).

  12. danielabrozzoni force-pushed on Jun 24, 2025
  13. danielabrozzoni commented at 12:40 pm on June 24, 2025: member

    @hodlinator thank you for your review!

    I like the idea of converting ProcessingResult to an enum, I think it makes the code easier to read.

    As for pow_validated_headers, I don’t have a strong preference between using a span or an argument for I/O just yet. I’ll take a closer look at #32579 over the next few days and should have a more informed opinion afterward :) I’d also be interested to hear what other reviewers think!

    Latest push:

    • style improvements based on review comments
  14. in src/headerssync.h:147 in 7634d81da5 outdated
    145     };
    146 
    147     /** Process a batch of headers, once a sync via this mechanism has started
    148      *
    149-     * received_headers: headers that were received over the network for processing.
    150+     * headers: headers that were received over the network for processing.
    


    l0rinc commented at 12:19 pm on July 4, 2025:
    nit: please reformat the whole block
  15. in src/test/headers_sync_chainwork_tests.cpp:96 in 7634d81da5 outdated
    93     // initially and then the rest.
    94     headers_batch.insert(headers_batch.end(), std::next(first_chain.begin()), first_chain.end());
    95 
    96     hss.reset(new HeadersSyncState(0, Params().GetConsensus(), chain_start, chain_work));
    97-    (void)hss->ProcessNextHeaders({first_chain.front()}, true);
    98+    std::vector<CBlockHeader> to_proc{first_chain.front()};
    


    l0rinc commented at 12:24 pm on July 4, 2025:
    There’s a significant overlap with https://github.com/bitcoin/bitcoin/pull/32579/files#diff-46faf106e779941e78de61d061d7817f9dd7b1f12a4157e6991417c7f0a7e9d3L95 - would be great if we could avoid a complete rewrite after one of them is merged

    danielabrozzoni commented at 3:21 pm on July 7, 2025:

    Yes absolutely! I think it all comes down to #32740 (review):

    • This PR changes MakeProcessNextHeaders/PopHeadersReadyForAcceptance to use headers as a I/O parameter (09c7d0f703067d455aae4ee458ba7953c29d72fb)
    • #32579 changes MakeProcessNextHeaders to use span instead of vector for headers (4c45b5d349c59ed6a795819cdfbaec90bc189a24)

    The reasons for switching to headers as I/O parameter are explained in sipa’s commit (09c7d0f703067d455aae4ee458ba7953c29d72fb):

    That said, there are downsides:

    I’m really not sure which solution is better - my C++ is still at a beginner level, so I’d really appreciate any input on this! I’m also happy to drop the first commit if reviewers feel the span approach is the better direction.


    hodlinator commented at 9:07 pm on July 7, 2025:
    meta: There’s an issue with the https://github.com/bitcoin/bitcoin/commit/09c7d0f703067d455aae4ee458ba7953c29d72fb#r2149379657 style links (taken from outdated “Files changed”-tab?). I’ve switched to mostly grabbing links from the “Conversation”-tab https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2149379657.
  16. l0rinc commented at 12:50 pm on July 4, 2025: contributor
    I’d like to review this - can you tell me how to measure the effect of the “optimizations” using either the microbenchmarks or a reindex or anything that shows the magnitude of change? Would be helpful if you could publish your measurements if you have any.
  17. danielabrozzoni force-pushed on Jul 7, 2025
  18. danielabrozzoni commented at 4:49 pm on July 7, 2025: member

    Last push:

    I’d like to review this - can you tell me how to measure the effect of the “optimizations” using either the microbenchmarks or a reindex or anything that shows the magnitude of change? Would be helpful if you could publish your measurements if you have any.

    I don’t have any, working on it!

  19. in src/headerssync.h:204 in 7d09855eae outdated
    200@@ -204,8 +201,8 @@ class HeadersSyncState {
    201      * buffer for later processing */
    202     bool ValidateAndStoreRedownloadedHeader(const CBlockHeader& header);
    203 
    204-    /** Return a set of headers that satisfy our proof-of-work threshold */
    205-    std::vector<CBlockHeader> PopHeadersReadyForAcceptance();
    206+    /** Populate the given vector with headers that satisfy our proof-of-work threshold */
    


    l0rinc commented at 10:46 pm on July 22, 2025:
    “populate” doesn’t hint at the precondition state: should it be empty or do we just erase it instantly?
  20. in src/headerssync.cpp:70 in 7d09855eae outdated
    65@@ -66,13 +66,13 @@ void HeadersSyncState::Finalize()
    66 /** Process the next batch of headers received from our peer.
    67  *  Validate and store commitments, and compare total chainwork to our target to
    68  *  see if we can switch to REDOWNLOAD mode.  */
    69-HeadersSyncState::ProcessingResult HeadersSyncState::ProcessNextHeaders(const
    70-        std::vector<CBlockHeader>& received_headers, const bool full_headers_message)
    71+HeadersSyncState::ProcessingResult HeadersSyncState::ProcessNextHeaders(
    72+        std::vector<CBlockHeader>& headers, const bool full_headers_message)
    


    l0rinc commented at 10:03 pm on July 23, 2025:

    Hmm, so now we have a dedicated return value called “ProcessingResult” and it won’t contain the processing results, but the input parameter will be repurposed for that?

    I understand that it’s a lot of data to move or copy, but I don’t find the result to be natural at all - see how awkward and stateful the tests have became, it’s often a good sign that the interface isn’t intuitive.

    Given that MAX_HEADERS_RESULTS = 2000 we’re optimizing for 160kb of memory here? I agree that that’s not nothing, but I find the code a lot more stateful and cumbersome and not sure the savings are even measurable.

  21. in src/test/headers_sync_chainwork_tests.cpp:132 in 7d09855eae outdated
    130@@ -126,17 +131,19 @@ BOOST_AUTO_TEST_CASE(headers_sync_state)
    131     hss.reset(new HeadersSyncState(0, Params().GetConsensus(), chain_start, chain_work));
    132     BOOST_CHECK(hss->GetState() == HeadersSyncState::State::PRESYNC);
    133      // Pretend just the first message is "full", so we don't abort.
    134-    (void)hss->ProcessNextHeaders({second_chain.front()}, true);
    135+    to_proc = {second_chain.front()};
    136+    (void)hss->ProcessNextHeaders(to_proc, true);
    137     BOOST_CHECK(hss->GetState() == HeadersSyncState::State::PRESYNC);
    138 
    139     headers_batch.clear();
    


    l0rinc commented at 10:51 pm on July 23, 2025:
    It was already hard to follow this stateful test, now it’s even more so - could we maybe cherry-pick @hodlinator’s cleanup commit before of this change?
  22. in src/headerssync.cpp:86 in 7d09855eae outdated
    80@@ -81,8 +81,9 @@ HeadersSyncState::ProcessingResult HeadersSyncState::ProcessNextHeaders(const
    81         // During PRESYNC, we minimally validate block headers and
    82         // occasionally add commitments to them, until we reach our work
    83         // threshold (at which point m_download_state is updated to REDOWNLOAD).
    84-        ret.success = ValidateAndStoreHeadersCommitments(received_headers);
    85+        ret.success = ValidateAndStoreHeadersCommitments(headers);
    86         if (ret.success) {
    87+            headers = {};
    


    l0rinc commented at 11:01 pm on July 23, 2025:
    hmmm, shouldn’t ValidateAndStoreHeadersCommitments be responsible for this? The state is spread out all over now, I have a hard time following
  23. in src/net_processing.cpp:2942 in 7724da4031 outdated
    2936@@ -2937,7 +2937,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
    2937     {
    2938         LOCK(cs_main);
    2939         last_received_header = m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash());
    2940-        if (IsAncestorOfBestHeaderOrTip(last_received_header)) {
    2941+        if (!already_validated_work && IsAncestorOfBestHeaderOrTip(last_received_header)) {
    2942             already_validated_work = true;
    2943         }
    


    l0rinc commented at 11:23 pm on July 23, 2025:

    so if already_validated_work is false and IsAncestorOfBestHeaderOrTip returns true, we update already_validated_work to true - we could simplify it to:

    0        already_validated_work = already_validated_work || IsAncestorOfBestHeaderOrTip(last_received_header);
    

    hodlinator commented at 2:23 pm on August 12, 2025:

    (re #32740#pullrequestreview-3102319617:

    nit: the PR isn’t really an optimization anymore, consider updating the title

    I guess this is technically still an optimization).

  24. in src/validation.h:397 in b8462ac73d outdated
    393@@ -394,7 +394,7 @@ bool TestBlockValidity(BlockValidationState& state,
    394                        bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    395 
    396 /** Check with the proof of work on each blockheader matches the value in nBits */
    397-bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams);
    398+bool HasValidProofOfWork(const std::span<const CBlockHeader> headers, const Consensus::Params& consensusParams);
    


    l0rinc commented at 11:28 pm on July 23, 2025:

    const std::span in a header definition doesn’t make a lot of sense:

    0bool HasValidProofOfWork(std::span<const CBlockHeader> headers, const Consensus::Params& consensusParams);
    

    or even

    0bool HasValidProofOfWork(std::span<const CBlockHeader>, const Consensus::Params&);
    

    danielabrozzoni commented at 3:00 pm on August 8, 2025:
    That makes a lot of sense, thank you! I only removed the const, I prefer to keep the argument names
  25. in src/chain.h:346 in b8462ac73d outdated
    342@@ -343,7 +343,15 @@ class CBlockIndex
    343     CBlockIndex& operator=(CBlockIndex&&) = delete;
    344 };
    345 
    346-arith_uint256 GetBlockProof(const CBlockIndex& block);
    347+/** Compute how much work an nBits value corresponds to, based on its nBits value. */
    


    l0rinc commented at 11:31 pm on July 23, 2025:
    0/** Compute how much work an nBits value corresponds to. */
    
  26. in src/validation.cpp:4164 in b8462ac73d outdated
    4158@@ -4159,9 +4159,9 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&
    4159     return commitment;
    4160 }
    4161 
    4162-bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams)
    4163+bool HasValidProofOfWork(const std::span<const CBlockHeader> headers, const Consensus::Params& consensusParams)
    4164 {
    4165-    return std::all_of(headers.cbegin(), headers.cend(),
    4166+    return std::all_of(headers.begin(), headers.end(),
    


    l0rinc commented at 11:38 pm on July 23, 2025:

    If we’re changing it, we might as well modernize it to:

    0    return std::ranges::all_of(headers,
    
  27. in src/validation.cpp:4162 in b8462ac73d outdated
    4158@@ -4159,9 +4159,9 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&
    4159     return commitment;
    4160 }
    4161 
    4162-bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams)
    4163+bool HasValidProofOfWork(const std::span<const CBlockHeader> headers, const Consensus::Params& consensusParams)
    


    l0rinc commented at 11:38 pm on July 23, 2025:

    Not sure why we’re modifying this method in this commit, it’s not strictly needed for the GetBitsProof change.

    nit:

    0bool HasValidProofOfWork(std::span<const CBlockHeader> headers, const Consensus::Params& consensusParams)
    

    danielabrozzoni commented at 3:02 pm on August 8, 2025:
    Thanks, I removed the const and put this change in a separate commit
  28. in src/net_processing.cpp:4351 in b8462ac73d outdated
    4364@@ -4365,7 +4365,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4365                 MaybeSendGetHeaders(pfrom, GetLocator(m_chainman.m_best_header), *peer);
    4366             }
    4367             return;
    4368-        } else if (prev_block->nChainWork + CalculateClaimedHeadersWork({{cmpctblock.header}}) < GetAntiDoSWorkThreshold()) {
    4369+        } else if (prev_block->nChainWork + GetBlockProof(cmpctblock.header) < GetAntiDoSWorkThreshold()) {
    


    l0rinc commented at 11:43 pm on July 23, 2025:
    nice!
  29. in src/merkleblock.cpp:32 in 254fe7823b outdated
    28@@ -29,7 +29,7 @@ std::vector<bool> BytesToBits(const std::vector<unsigned char>& bytes)
    29 
    30 CMerkleBlock::CMerkleBlock(const CBlock& block, CBloomFilter* filter, const std::set<Txid>* txids)
    31 {
    32-    header = block.GetBlockHeader();
    33+    header = block;
    


    l0rinc commented at 11:47 pm on July 23, 2025:

    maybe casting would avoid the slicing warning:

    0    header = static_cast<CBlockHeader>(block);
    

    hodlinator commented at 1:53 pm on August 12, 2025:

    nit: Would have a slight preference for:

    0    header = static_cast<const CBlockHeader&>(block);
    

    to remove any doubt that we’re creating an extra temporary CBlockHeader instance.

  30. in src/test/blockfilter_index_tests.cpp:104 in 254fe7823b outdated
    100@@ -101,7 +101,7 @@ bool BuildChainTestingSetup::BuildChain(const CBlockIndex* pindex,
    101     chain.resize(length);
    102     for (auto& block : chain) {
    103         block = std::make_shared<CBlock>(CreateBlock(pindex, no_txns, coinbase_script_pub_key));
    104-        CBlockHeader header = block->GetBlockHeader();
    105+        CBlockHeader header = *block;
    


    l0rinc commented at 11:48 pm on July 23, 2025:
    we can likely inline this now to be consistent with other similar usages
  31. in src/headerssync.h:38 in b30738e582 outdated
    41-    }
    42+    CompressedHeader(const CBlockHeader& header) : nVersion(header.nVersion),
    43+                                                   hashMerkleRoot(header.hashMerkleRoot),
    44+                                                   nTime(header.nTime),
    45+                                                   nBits(header.nBits),
    46+                                                   nNonce(header.nNonce) {}
    


    l0rinc commented at 11:58 pm on July 23, 2025:

    nit: consider making it explicit and using brace init instead to avoid narrowing:

    0    explicit CompressedHeader(const CBlockHeader& header)
    1        : nVersion{header.nVersion},
    2          hashMerkleRoot{header.hashMerkleRoot},
    3          nTime{header.nTime},
    4          nBits{header.nBits},
    5          nNonce{header.nNonce} {}
    
  32. in src/test/headers_sync_chainwork_tests.cpp:88 in f4b33a14f2 outdated
    84@@ -85,7 +85,7 @@ BOOST_AUTO_TEST_CASE(headers_sync_state)
    85             Params().GenesisBlock().nVersion, Params().GenesisBlock().nTime,
    86             ArithToUint256(1), Params().GenesisBlock().nBits);
    87 
    88-    const CBlockIndex* chain_start = WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(Params().GenesisBlock().GetHash()));
    89+    const CBlockIndex& chain_start = {*Assert(WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(Params().GenesisBlock().GetHash())))};
    


    l0rinc commented at 0:05 am on July 24, 2025:

    It’s a bit weird to have Assert outside, how about:

    0    const auto& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(Params().GenesisBlock().GetHash())))};
    

    ajtowns commented at 0:58 am on July 24, 2025:

    Returning a pointer from an anonymous function with no explicit return type seems safer and easier to follow than returning a reference? Not sure what’s weird about having the Assert outside the lock. We have a similar instance already:

    https://github.com/bitcoin/bitcoin/blob/eb137184482cea8a2a7cdfcd3c73ec6ae5fdd0d6/src/node/blockstorage.cpp#L935


    l0rinc commented at 1:28 am on July 24, 2025:
    I prefer validating closely to the source, but I don’t mind either way, thanks for the comment

    danielabrozzoni commented at 2:58 pm on August 8, 2025:
    I don’t have a strong preference, leaving as-is for consistency

    l0rinc commented at 10:17 am on November 3, 2025:
    I see you’ve reverted this after the rebase - was it on purpose?

    hodlinator commented at 11:07 am on November 3, 2025:

    Agree latest push drops part of the change from bottom of #25968 (review). Would prefer this squashed into the last commit:

     0--- a/src/test/headers_sync_chainwork_tests.cpp
     1+++ b/src/test/headers_sync_chainwork_tests.cpp
     2@@ -52,7 +52,7 @@ constexpr size_t COMMITMENT_PERIOD{600}; // Somewhat close to mainnet.
     3 
     4 struct HeadersGeneratorSetup : public RegTestingSetup {
     5     const CBlock& genesis{Params().GenesisBlock()};
     6-    const CBlockIndex* chain_start{WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash()))};
     7+    const CBlockIndex& chain_start{*Assert(WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
     8 
     9     // Generate headers for two different chains (using differing merkle roots
    10     // to ensure the headers are different).
    11@@ -86,7 +86,7 @@ struct HeadersGeneratorSetup : public RegTestingSetup {
    12                     .commitment_period = COMMITMENT_PERIOD,
    13                     .redownload_buffer_size = REDOWNLOAD_BUFFER_SIZE,
    14                 },
    15-                *chain_start,
    16+                chain_start,
    17                 /*minimum_required_work=*/CHAIN_WORK};
    18     }
    19 
    

    (Edit: Can see pros/cons to both ways of placing the Assert vs the lock, no strong preference).


    danielabrozzoni commented at 12:08 pm on November 3, 2025:

    Thanks for catching it and sorry for not specifying in my push comment!

    I dropped it on purpose because it caused a compilation warning:

    0[ 73%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/headers_sync_chainwork_tests.cpp.o
    1/home/daniela/Developer/BitcoinCore/bitcoin/src/test/headers_sync_chainwork_tests.cpp:55:37: warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function]
    2   55 |     const CBlockIndex& chain_start{*Assert(WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
    3      |                                     ^
    4/home/daniela/Developer/BitcoinCore/bitcoin/src/util/check.h:115:75: note: expanded from macro 'Assert'
    5  115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
    6      |                                                                           ^
    

    This didn’t happen before my rebase onto #32579, as chain_start was initialized inside a function.

    I also had a preference for using the Assert, but I’m not sure if it can still be done somehow… I’m open to suggestions :)


    l0rinc commented at 12:25 pm on November 3, 2025:

    Can you try my originally suggestion to put the Assert inside?

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

    it works for me locally. Or ignore, since we’ve already acked.


    danielabrozzoni commented at 12:40 pm on November 3, 2025:

    Ah! Of course, this way the Assert is inside a function

    Sorry, I pushed again and I’ll have you to re-ACK, I really prefer having an assertion there


    maflcko commented at 9:17 pm on November 4, 2025:
    Fixed in #33785. You should be able to push your previous *Assert(WITH_LOCK(...)) after it.
  33. l0rinc changes_requested
  34. l0rinc commented at 0:08 am on July 24, 2025: contributor

    Concept ACK, I’m not sure about the first change but the rest make sense.

    reviewed f4b33a14f27bb3ee734e8d4eb9c2b2ccaa5df52a, left some comments for most commits, my main concern is the commit where we’re reusing the headers vector, making the code significantly more contrived. It would also be great if we could extract the common parts of this and @hodlinator’s change and merge that (or his change) before we do this.

  35. danielabrozzoni commented at 3:41 pm on August 1, 2025: member

    After some thinking, I decided to drop the first commit, and let ProcessNextHeaders take the headers argument and return it, instead of using it as a I/O argument. I tried to write a benchmark to measure how much using the vector as I/O argument would optimize the code, but it convinced me that the new interface would be too hard to work with, as reviewers pointed out. While it might still be a performance improvement, I’m not sure if it’s really worth it.

    For anyone interested, here’s my attempt at writing a benchmark: 852aca5e34320449c47488d181e98ce72b74adfd and my branch (Unfortunately, since ProcessNextHeaders changes the content of the headers vector passed in, I had to copy the vector every single time, which I suppose could make the benchmark a bit unreliable? There are also a few assertions in the bench.run() function, which I used to make sure that my code wasn’t bugged, but I’m not sure if they should be there in any serious attempt at benchmarking)

    Will update the PR in the next few days :)

  36. danielabrozzoni force-pushed on Aug 8, 2025
  37. danielabrozzoni force-pushed on Aug 8, 2025
  38. danielabrozzoni commented at 3:05 pm on August 8, 2025: member

    In my last push (f33bd2f):

  39. in src/validation.cpp:4135 in 80c63c9cec outdated
    4148@@ -4149,8 +4149,8 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&
    4149 
    4150 bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams)
    4151 {
    4152-    return std::all_of(headers.cbegin(), headers.cend(),
    4153-            [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);});
    4154+    return std::ranges::all_of(headers,
    


    l0rinc commented at 11:37 pm on August 8, 2025:
    nit: this change could either be merged with the vector-to-std::span or moved over to that commit, it’s not needed in 80c63c9ceccc89295f004099c8cd7e7dfd99b556

    hodlinator commented at 1:55 pm on August 12, 2025:

    I can see it belonging in either commit, as one modifies the container and the other modifies what operators we apply to it.

    Edit after #32740 (review) below: Now realize I was mixing up CheckProofOfWork with GetBlockProof - we are still applying the same thing.


    danielabrozzoni commented at 4:35 pm on August 15, 2025:
    I think it fits slightly better with the span change… since in c++20 span doesn’t have a cbegin method, we needed to change the initial code, and update to ranges::all_of
  40. in src/primitives/block.h:104 in adfedb2df8 outdated
    100@@ -101,18 +101,6 @@ class CBlock : public CBlockHeader
    101         m_checked_merkle_root = false;
    102     }
    103 
    104-    CBlockHeader GetBlockHeader() const
    


    l0rinc commented at 11:38 pm on August 8, 2025:
    nice!
  41. in src/headerssync.cpp:27 in f33bd2f142 outdated
    30-    m_chain_start(chain_start),
    31-    m_minimum_required_work(minimum_required_work),
    32-    m_current_chain_work(chain_start->nChainWork),
    33-    m_last_header_received(m_chain_start->GetBlockHeader()),
    34-    m_current_height(chain_start->nHeight)
    35+                                   const CBlockIndex& chain_start, const arith_uint256& minimum_required_work) : m_commit_offset(FastRandomContext().randrange<unsigned>(HEADER_COMMITMENT_PERIOD)),
    


    l0rinc commented at 11:41 pm on August 8, 2025:
    nit: if you touch again, please format this differently

    hodlinator commented at 1:59 pm on August 12, 2025:

    Agree using style much closer to the original would be very welcome. Listing the member initializers at the indentation level where the function arguments end makes potential comments to the right of those arguments disappear out of sight etc.

    Would also be nice to use more common style for CompressedHeader but at least there the initializer list didn’t exist before.


    danielabrozzoni commented at 10:01 am on August 18, 2025:

    Thanks, I didn’t like the initial formatting either, but I thought it was common (looking at other constructors in the repository, no, it’s not…)

    I updated the formatting both in HeadersSyncState and CompressedHeaders!


    hodlinator commented at 3:10 pm on August 18, 2025:
    (Still have a slight preference for using the original way of formatting the HeadersSyncState initializer-list for optimal git blame integrity, but this is tolerable compared to the previous push, I see it matches PeerManagerImpl).

    danielabrozzoni commented at 11:05 am on August 22, 2025:

    I would still keep the reformatting as clang-format doesn’t like the one on master, and reformats it as I previously did (with the member initializers pushed to the right). At least this reformat is nice and clang-format doesn’t try to modify it.

    I hadn’t considered git blame though, I don’t love the idea of polluting it…


    hodlinator commented at 8:59 pm on August 22, 2025:
    Oh, so clang-format was pushing for it… seems it could do with some reconfiguration in the initializer-list aspect.

    l0rinc commented at 9:03 pm on August 22, 2025:
    After #32813 we could customize the list formatting as well

    hodlinator commented at 10:50 am on November 3, 2025:

    Given the current stats on initializer list formatting, I don’t think there’s enough reason to open a PR to turn the tide by changing src/.clang-format.

     0# BeforeColon (current default in src/.clang-format, style partially objected to in this thread):
     1$ grep -zoP "\n( {4})+( {2})m_\w+[({]" -r src/ |wc -l
     2135
     3
     4# AfterColon (what I expected to be most common):
     5$ grep -zoP "\n( {4})+m_\w+[({]"  -r src/ |wc -l
     6117
     7
     8# BeforeComma (BetaMax of initializer lists, leads to smallest diffs):
     9$ grep -zoP "\n( {4})+, m_\w+[({]"  -r src/ |wc -l
    1017
    

    https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#breakconstructorinitializers

    So this thread can be resolved IMO. Cheers to @l0rinc for the reminder.

  42. l0rinc approved
  43. l0rinc commented at 11:45 pm on August 8, 2025: contributor

    code review ACK f33bd2f1425050ff89caa018edfb2265a754fb5d

    nit: the PR isn’t really an optimization anymore, consider updating the title

  44. in src/validation.h:395 in f33bd2f142 outdated
    392@@ -393,7 +393,7 @@ bool TestBlockValidity(BlockValidationState& state,
    393                        bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    394 
    395 /** Check with the proof of work on each blockheader matches the value in nBits */
    


    hodlinator commented at 2:14 pm on August 12, 2025:

    nit: Could fix typo as suggested by LLM (https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-2968054282).

    0/** Check that the proof of work on each blockheader matches the value in nBits */
    
  45. hodlinator commented at 2:17 pm on August 12, 2025: contributor
    Reviewed f33bd2f1425050ff89caa018edfb2265a754fb5d
  46. danielabrozzoni force-pushed on Aug 18, 2025
  47. danielabrozzoni commented at 2:46 pm on August 18, 2025: member

    Thanks for the reviews!

    In my last push:

    • Rebased onto the right commit :sweat_smile: (see #32740 (review))
    • Addressed review comments
  48. hodlinator approved
  49. hodlinator commented at 3:41 pm on August 18, 2025: contributor

    ACK 50e61f448eedfe0ccfa07f3124aeef9c7762a69b

    PR improves the code in several minor ways, such as:

    • Computing PoW for a CBlockHeader (or CBlock) without having to jump through temporarily constructed CBlockIndex (552b9c3a565c1817074468bf71fb4526f3a47f42).
    • Removing CBlock::GetBlockHeader() leaves less room for forgetting to properly set fields in the new CBlockHeader instance (67aa62c80e7d2faa485ba020cefe262f479190f4).
    • Changes const CBlockIndex* HeadersSyncState::m_chain_start to a reference as it is never null and never changed (50e61f448eedfe0ccfa07f3124aeef9c7762a69b).
  50. DrahtBot requested review from l0rinc on Aug 18, 2025
  51. in src/net_processing.cpp:2922 in 6f3c9ce200 outdated
    2905@@ -2906,9 +2906,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
    2906     {
    2907         LOCK(cs_main);
    2908         last_received_header = m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash());
    2909-        if (IsAncestorOfBestHeaderOrTip(last_received_header)) {
    2910-            already_validated_work = true;
    2911-        }
    2912+        already_validated_work = already_validated_work || IsAncestorOfBestHeaderOrTip(last_received_header);
    


    l0rinc commented at 2:44 am on August 21, 2025:
    nit: if you touch again, you could explain in the commit message why the LookupBlockIndex cannot be eliminated as well
  52. in src/headerssync.h:34 in b58b255673 outdated
    30@@ -31,16 +31,17 @@ struct CompressedHeader {
    31         hashMerkleRoot.SetNull();
    32     }
    33 
    34-    CompressedHeader(const CBlockHeader& header)
    35+    explicit CompressedHeader(const CBlockHeader& header)
    


    l0rinc commented at 2:49 am on August 21, 2025:
    nit: if touching again, the commit message should probably mention the explicit as well. No that important, just noticed it.

    polespinasa commented at 2:39 am on October 29, 2025:
    What’s the use-case for explicit here? and why is better to use the list instead of a “normal” constructor?

    danielabrozzoni commented at 7:20 pm on October 29, 2025:

    The explicit was suggested in #32740 (review). As far as I know it’s good practice, so that the compiler can’t make implicit conversions to resolve the parameters of a function. Also see: https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/doc/developer-notes.md?plain=1#L835

    The initializer list was suggested in #25968#pullrequestreview-1096400728, I think it’s consistent with the rest of the code, but I’m not sure if there’s any benefit to it

    Thanks for the review btw! I’m currently working on rebasing, it might take a couple of days though (I have a test failing…)

  53. in src/headerssync.cpp:47 in 50e61f448e
    43@@ -41,7 +44,7 @@ HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus
    44     // exceeds this bound, because it's not possible for a consensus-valid
    45     // chain to be longer than this (at the current time -- in the future we
    46     // could try again, if necessary, to sync a longer chain).
    47-    m_max_commitments = 6*(Ticks<std::chrono::seconds>(NodeClock::now() - NodeSeconds{std::chrono::seconds{chain_start->GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME) / HEADER_COMMITMENT_PERIOD;
    48+    m_max_commitments = 6 * (Ticks<std::chrono::seconds>(NodeClock::now() - NodeSeconds{std::chrono::seconds{chain_start.GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME) / HEADER_COMMITMENT_PERIOD;
    


    l0rinc commented at 2:50 am on August 21, 2025:
    nit: as mentioned this should be synchronized with the other PR to simplify the merge conflicts

    danielabrozzoni commented at 10:58 am on August 22, 2025:
    I’m happy to put this in draft and wait for #32579 to be merged first… This PR is smaller, and probably easier to rebase. What do you think?

    l0rinc commented at 2:39 pm on August 22, 2025:
    I personally would let the maintainers decide the order, ank keep both open

    hodlinator commented at 8:56 pm on August 22, 2025:
    Yeah, I don’t mind the order much, especially now that it seems like neither will be in v30. Thanks for asking!
  54. l0rinc commented at 3:01 am on August 21, 2025: contributor

    recrACK 50e61f448eedfe0ccfa07f3124aeef9c7762a69b

    I only left nits, I’m fine with merging it as is.

  55. l0rinc approved
  56. DrahtBot added the label Needs rebase on Oct 23, 2025
  57. polespinasa approved
  58. polespinasa commented at 2:49 am on October 29, 2025: member

    Code Review ACK 50e61f448eedfe0ccfa07f3124aeef9c7762a69b (will re-review after rebasing)

    This PR cleans and simplifies the code in many ways like avoiding calling unneeded functions, avoiding constructing temporary objects, removing unused params, among others…

    Just left a single comment with a question :)

    btw 552b9c3a565c1817074468bf71fb4526f3a47f42 and 67aa62c80e7d2faa485ba020cefe262f479190f4 are pretty :)

  59. p2p: Avoid an IsAncestorOfBestHeaderOrTip call
    Just don't call this function when it won't have any effect.
    
    Note that we can't remove the LookupBlockIndex call, since `last_received_header`
    is needed to check if new headers were received (`received_new_header`).
    0bf6139e19
  60. refactor: Compute work from headers without CBlockIndex
    Avoid the need to construct a CBlockIndex object just to compute work for a header,
    when its nBits value suffices for that.
    
    Co-Authored-By: Pieter Wuille <pieter@wuille.net>
    4066bfe561
  61. refactor: Use std::span in HasValidProofOfWork
    Co-Authored-By: Pieter Wuille <pieter@wuille.net>
    4568652222
  62. refactor: Remove useless CBlock::GetBlockHeader
    There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child
    class of it.
    ca0243e3a6
  63. refactor: Remove redundant parameter from CheckHeadersPoW
    No need to pass consensusParams, as CheckHeadersPoW already has access
    to m_chainparams.GetConsensus()
    
    Co-Authored-By: maflcko <6399679+maflcko@users.noreply.github.com>
    256246a9fa
  64. refactor: Remove unused parameter in ReportHeadersPresync
    Co-Authored-By: Aurèle Oulès <aurele@oules.com>
    0488bdfefe
  65. refactor: Use initializer list in CompressedHeader
    Also mark CompressedHeader as explicit, and GetFullHeader as const
    
    Co-Authored-By: Aurèle Oulès <aurele@oules.com>
    e37555e540
  66. danielabrozzoni force-pushed on Nov 3, 2025
  67. danielabrozzoni force-pushed on Nov 3, 2025
  68. DrahtBot removed the label Needs rebase on Nov 3, 2025
  69. hodlinator approved
  70. hodlinator commented at 8:44 am on November 3, 2025: contributor

    re-ACK 8a4ab45c13dd7ca4474bc4621110da82e0a634f8

    Changes since previous review (https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3128785887):

    • More helpful commit messages.
    • Rebased, especially to handle #32579.
  71. DrahtBot requested review from l0rinc on Nov 3, 2025
  72. DrahtBot requested review from polespinasa on Nov 3, 2025
  73. danielabrozzoni commented at 8:57 am on November 3, 2025: member

    @hodlinator thanks! Was about to comment with the update haha :)

    In my last push:

  74. l0rinc commented at 10:29 am on November 3, 2025: contributor

    Code review reACK 8a4ab45c13dd7ca4474bc4621110da82e0a634f8

    I have re-rebased my previous ack, solved the conflicts and did a soft reset compared to this, the only differences were whitespace changes, comment fixes, static cast update. Not sure if #32740 (review) was on purpose or not, but either is fine with me.

  75. danielabrozzoni force-pushed on Nov 3, 2025
  76. danielabrozzoni commented at 12:42 pm on November 3, 2025: member

    In my last push: re-introduce Assert in header sync tests, see #32740 (review)

    Sorry for another push after the acks, and thanks for the reviews

  77. l0rinc commented at 12:51 pm on November 3, 2025: contributor

    Code review reACK 9a387e9175d34d1eaf83b9008f5e857d1eb04b8e

    Finalized the pointer to reference migration since last ack.

  78. DrahtBot requested review from hodlinator on Nov 3, 2025
  79. hodlinator approved
  80. hodlinator commented at 12:53 pm on November 3, 2025: contributor

    re-ACK 9a387e9175d34d1eaf83b9008f5e857d1eb04b8e

    Changes since previous review (https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3409949311):

  81. hodlinator commented at 7:30 pm on November 3, 2025: contributor

    Tested running the Linux->Windows CI locally and this patch seems to fix the failure:

     0--- a/src/test/headers_sync_chainwork_tests.cpp
     1+++ b/src/test/headers_sync_chainwork_tests.cpp
     2@@ -52,7 +52,7 @@ constexpr size_t COMMITMENT_PERIOD{600}; // Somewhat close to mainnet.
     3 
     4 struct HeadersGeneratorSetup : public RegTestingSetup {
     5     const CBlock& genesis{Params().GenesisBlock()};
     6-    const CBlockIndex& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
     7+    const CBlockIndex& chain_start{ChainStart()};
     8 
     9     // Generate headers for two different chains (using differing merkle roots
    10     // to ensure the headers are different).
    11@@ -101,6 +101,15 @@ private:
    12     std::vector<CBlockHeader> GenerateHeaders(size_t count,
    13             uint256 prev_hash, int32_t nVersion, uint32_t prev_time,
    14             const uint256& merkle_root, uint32_t nBits);
    15+
    16+    // Broken out into its own function to please 32-bit ARM (GCC 13.3) and
    17+    // Linux->Windows cross builds (GCC 13.0) which otherwise complain about
    18+    // "possibly dangling reference to a temporary [-Werror=dangling-reference]"
    19+    const CBlockIndex& ChainStart()
    20+    {
    21+        LOCK(::cs_main);
    22+        return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash()));
    23+    }
    24 };
    25 
    26 void HeadersGeneratorSetup::FindProofOfWork(CBlockHeader& starting_header)
    
  82. maflcko referenced this in commit faf6c1ac5d on Nov 4, 2025
  83. maflcko referenced this in commit fa60409af1 on Nov 4, 2025
  84. maflcko referenced this in commit faf6c7ee6b on Nov 4, 2025
  85. maflcko referenced this in commit fa5fbcd615 on Nov 5, 2025
  86. danielabrozzoni commented at 4:54 am on November 10, 2025: member

    Thank you @hodlinator!

    In my last push (184c54c):

    • Push hodlinator’s fix as a co-authored commit on top (I slightly changed the diff to align with the style of HeadersGeneratorSetup). I can squash the last two commits if that’s preferred.
  87. hodlinator commented at 7:39 am on November 10, 2025: contributor
    * Push hodlinator's fix as a co-authored commit on top (I slightly changed the diff to align with the style of `HeadersGeneratorSetup`). I can squash the last two commits if that's preferred.
    

    Thanks for taking my change! Technically I think the repo policy is that every commit should build on all CI, see for example https://github.com/bitcoin/bitcoin/pull/33785/commits/fae1d99651e29341e486a10e6340335c71a2144e which adds const& in the same change as disabling the errors, instead of doing const& in the parent commit. So 184c54c0843cdaf90a0282532d627213fa5d018d should be squashed into the parent commit.

    Alternatively, if rebasing on top of #33785 makes it work, and since it has 3 ACKs, we could wait for it to be merged before this PR and drop 184c54c0843cdaf90a0282532d627213fa5d018d.

  88. in src/test/headers_sync_chainwork_tests.cpp:55 in 184c54c084
    51@@ -52,7 +52,7 @@ constexpr size_t COMMITMENT_PERIOD{600}; // Somewhat close to mainnet.
    52 
    53 struct HeadersGeneratorSetup : public RegTestingSetup {
    54     const CBlock& genesis{Params().GenesisBlock()};
    55-    const CBlockIndex& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
    56+    const CBlockIndex& chain_start{ChainStart()};
    


    l0rinc commented at 10:14 am on November 10, 2025:

    I don’t like this, why not just remove the const?

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

    hodlinator commented at 1:31 pm on November 10, 2025:
    Now that #33785 has been merged we hopefully can use const& ... Assert( in this PR as long as it’s rebased.

    maflcko commented at 7:27 pm on November 20, 2025:

    Now that #33785 has been merged we hopefully can use const& ... Assert( in this PR as long as it’s rebased.

    The -Wno-error=dangling-reference was removed again in https://github.com/bitcoin/bitcoin/pull/33840/files


    hodlinator commented at 7:36 pm on November 20, 2025:
    Aha, that explains why I was getting other results when not rebasing onto latest master (https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3559658957).
  89. DrahtBot requested review from l0rinc on Nov 10, 2025
  90. fanquake referenced this in commit 490cb056f6 on Nov 10, 2025
  91. refactor: Use reference for chain_start in HeadersSyncState
    chain_start can never be null, so it's better to pass it as a reference
    rather than a raw pointer
    
    Also slightly reformat HeaderSyncState constructor to make clang-format
    happy
    
    Lastly, remove `const` from `chain_start` declaration in
    headers_sync_chainwork_tests, to work aroud a false-positive
    dangling-reference warning in gcc 13.0
    
    Co-Authored-By: maflcko <6399679+maflcko@users.noreply.github.com>
    de4242f474
  92. danielabrozzoni force-pushed on Nov 20, 2025
  93. danielabrozzoni commented at 3:23 am on November 20, 2025: member

    Hey, sorry this took a while. I tried rebasing onto #33785 but it wouldn’t fix the warning.

    In my last push (de4242f):

    • I didn’t rebase, to hopefully make re-reviewing easier
    • I removed const as suggested in #32740 (review), I like it a bit better than the previous solution
    • I squashed the fix inside the last commit instead pushing as a separate one

    Technically I think the repo policy is that every commit should build on all CI,

    Understood, thanks for explaining! :)

  94. l0rinc commented at 10:32 am on November 20, 2025: contributor

    ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4

    Only a const removal since my last ACK

  95. hodlinator commented at 7:18 pm on November 20, 2025: contributor

    Edit: invalidated test by not testing on latest master. See #32740 (review).

    I tried rebasing onto #33785 but it wouldn’t fix the warning.

    Checked this by:

    0₿ git pr 32740
    1Switched to branch 'pr/32740'
    

    Rebasing onto the merge commit of #33785:

    0₿ git rebase HEAD~8 --onto 490cb056f651f4247731e39efab35ef8e8a59833
    

    Adding back const in commit 9bfcde056df7698ddf7ce9d958d5a9d3f3d0b43a:

    ARM 32 build and Linux->Windows cross builds succeed on CI with GNU (GCC) 13.3: https://github.com/hodlinator/bitcoin/actions/runs/19529971162/job/55910436956#step:8:3389 https://github.com/hodlinator/bitcoin/actions/runs/19529971162/job/55910436924#step:6:1935

    It’s true that there is still a warning (https://github.com/hodlinator/bitcoin/actions/runs/19529971162/job/55910436956#step:8:4290), but it’s excluded from triggering an error thanks to #33785’s -Wno-error=dangling-reference: https://github.com/bitcoin/bitcoin/pull/33785/commits/fae1d99651e29341e486a10e6340335c71a2144e#diff-01c24871dbdf85d7ddae09d013d9280074676ba15cbabb07258e8e9a8f5a1640R16

    So I think rebasing would still be preferable (instead of dropping const).

  96. hodlinator approved
  97. hodlinator commented at 7:39 pm on November 20, 2025: contributor

    re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4

    Only removed const from HeadersGeneratorSetup::chain_start to work around GCC 13 issue since my previous ACK (https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3410905600).


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-20 21:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me