refactor: Header sync optimisations & simplifications #32740

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

    This is a partial* revival of #25968

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

    • Make ProcessNextHeaders use the headers argument as in/out: This allows reusing the same vector storage for input and output headers to HeadersSync::ProcessNextHeaders. It’s also natural in the sense that this argument just represents the headers-to-be-processed, both in the caller and the callee, and both before and after the calls.
    • 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 two commits that were in #25968 (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 two 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. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32579 (headerssync: Preempt unrealistic unit test behavior by hodlinator)

    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:143 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

  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


    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;
    
  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: contributor

    @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. refactor: Make ProcessNextHeaders use the headers argument as in/out
    This allows reusing the same vector storage for input and output headers to
    HeadersSync::ProcessNextHeaders. It's also natural in the sense that this argument
    just represents the headers-to-be-processed, both in the caller and the callee, and
    both before and after the calls.
    
    Co-Authored-By: Daniela Brozzoni <danielabrozzoni@protonmail.com>
    7d09855eae
  18. p2p: Avoid an IsAncestorOfBestHeaderOrTip call
    Just don't call this function when it won't have any effect.
    7724da4031
  19. 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. Also use some Spans where possible.
    b8462ac73d
  20. 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.
    254fe7823b
  21. 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>
    c856a0e375
  22. refactor: Remove unused parameter in ReportHeadersPresync
    Co-Authored-By: Aurèle Oulès <aurele@oules.com>
    09932adb80
  23. refactor: Use initializer list in CompressedHeader
    Also mark GetFullHeader as const
    
    Co-Authored-By: Aurèle Oulès <aurele@oules.com>
    b30738e582
  24. 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
    
    Co-Authored-By: maflcko <6399679+maflcko@users.noreply.github.com>
    f4b33a14f2
  25. danielabrozzoni force-pushed on Jul 7, 2025
  26. danielabrozzoni commented at 4:49 pm on July 7, 2025: contributor

    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!


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-07-08 12:13 UTC

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