refactor: Header sync optimisations & simplifications #32740

danielabrozzoni · 2025-06-12T20:20:06Z

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 (4e7ac7b, ab52fb4, 7f1cf44), 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 :)

DrahtBot · 2025-06-12T20:20:17Z

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, hodlinator, polespinasa, sipa, achow101

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33856 (kernel, validation: Refactor ProcessNewBlockHeaders 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.

ajtowns · 2025-06-13T02:43:25Z

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" ?)

hodlinator

Reviewed 13e123e

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

hodlinator · 2025-06-16T08:42:30Z

src/headerssync.h


    /** Result data structure for ProcessNextHeaders. */
    struct ProcessingResult {
-        std::vector<CBlockHeader> pow_validated_headers;


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

Thanks for dropping the commit introducing the in/out parameter!

src/validation.cpp

src/headerssync.cpp

src/headerssync.h

danielabrozzoni · 2025-06-24T12:40:49Z

@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

l0rinc

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.

src/headerssync.h

src/test/headers_sync_chainwork_tests.cpp

danielabrozzoni · 2025-07-07T16:49:48Z

Last push:

Reformat to address refactor: Header sync optimisations & simplifications #32740 (comment)
Rebase on master

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!

l0rinc

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.

src/headerssync.h

src/headerssync.cpp

src/test/headers_sync_chainwork_tests.cpp

src/headerssync.cpp

src/net_processing.cpp

l0rinc · 2025-07-23T23:43:02Z

src/net_processing.cpp

@@ -4365,7 +4365,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                MaybeSendGetHeaders(pfrom, GetLocator(m_chainman.m_best_header), *peer);
            }
            return;
-        } else if (prev_block->nChainWork + CalculateClaimedHeadersWork({{cmpctblock.header}}) < GetAntiDoSWorkThreshold()) {
+        } else if (prev_block->nChainWork + GetBlockProof(cmpctblock.header) < GetAntiDoSWorkThreshold()) {


src/merkleblock.cpp

src/test/blockfilter_index_tests.cpp

src/headerssync.h

src/test/headers_sync_chainwork_tests.cpp

danielabrozzoni · 2025-08-01T15:41:52Z

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: 852aca5 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 :)

danielabrozzoni

In my last push (f33bd2f):

Removed first commit as per my last comment (#32740 (comment))
Addressed review comments

src/test/headers_sync_chainwork_tests.cpp

danielabrozzoni · 2025-08-08T15:00:16Z

src/validation.h

@@ -394,7 +394,7 @@ bool TestBlockValidity(BlockValidationState& state,
                       bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

 /** Check with the proof of work on each blockheader matches the value in nBits */
-bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams);
+bool HasValidProofOfWork(const std::span<const CBlockHeader> headers, const Consensus::Params& consensusParams);


That makes a lot of sense, thank you!
I only removed the const, I prefer to keep the argument names

danielabrozzoni · 2025-08-08T15:02:01Z

src/validation.cpp

@@ -4159,9 +4159,9 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&
    return commitment;
 }

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


Thanks, I removed the const and put this change in a separate commit

l0rinc

code review ACK f33bd2f1425050ff89caa018edfb2265a754fb5d

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

l0rinc · 2025-08-08T23:37:24Z

src/validation.cpp

@@ -4149,8 +4149,8 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&

 bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams)
 {
-    return std::all_of(headers.cbegin(), headers.cend(),
-            [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);});
+    return std::ranges::all_of(headers,


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

~~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 (comment) below: Now realize I was mixing up CheckProofOfWork with GetBlockProof - we are still applying the same thing.

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

l0rinc · 2025-08-08T23:38:39Z

src/primitives/block.h

@@ -101,18 +101,6 @@ class CBlock : public CBlockHeader
        m_checked_merkle_root = false;
    }

-    CBlockHeader GetBlockHeader() const


l0rinc · 2025-08-08T23:41:19Z

src/headerssync.cpp

-    m_current_chain_work(chain_start->nChainWork),
-    m_last_header_received(m_chain_start->GetBlockHeader()),
-    m_current_height(chain_start->nHeight)
+                                   const CBlockIndex& chain_start, const arith_uint256& minimum_required_work) : m_commit_offset(FastRandomContext().randrange<unsigned>(HEADER_COMMITMENT_PERIOD)),


nit: if you touch again, please format this differently

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.

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!

(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).

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...

Oh, so clang-format was pushing for it... seems it could do with some reconfiguration in the initializer-list aspect.

After #32813 we could customize the list formatting as well

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.

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

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.

hodlinator

Reviewed f33bd2f1425050ff89caa018edfb2265a754fb5d

hodlinator · 2025-08-12T13:28:17Z

src/net_processing.cpp

(Inline comment in random place).

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

Ooops, not sure what happened there, thanks for noticing!

hodlinator · 2025-08-12T13:46:21Z

src/headerssync.h


    /** Result data structure for ProcessNextHeaders. */
    struct ProcessingResult {
-        std::vector<CBlockHeader> pow_validated_headers;


Thanks for dropping the commit introducing the in/out parameter!

src/merkleblock.cpp

hodlinator · 2025-08-12T13:55:55Z

src/validation.cpp

@@ -4149,8 +4149,8 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&

 bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams)
 {
-    return std::all_of(headers.cbegin(), headers.cend(),
-            [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);});
+    return std::ranges::all_of(headers,


~~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 (comment) below: Now realize I was mixing up CheckProofOfWork with GetBlockProof - we are still applying the same thing.

hodlinator · 2025-08-12T13:59:00Z

src/headerssync.cpp

-    m_current_chain_work(chain_start->nChainWork),
-    m_last_header_received(m_chain_start->GetBlockHeader()),
-    m_current_height(chain_start->nHeight)
+                                   const CBlockIndex& chain_start, const arith_uint256& minimum_required_work) : m_commit_offset(FastRandomContext().randrange<unsigned>(HEADER_COMMITMENT_PERIOD)),


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.

hodlinator · 2025-08-12T14:14:10Z

src/validation.h

@@ -393,7 +393,7 @@ bool TestBlockValidity(BlockValidationState& state,
                       bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

 /** Check with the proof of work on each blockheader matches the value in nBits */


nit: Could fix typo as suggested by LLM (#32740 (comment)).

Suggested change

/** Check with the proof of work on each blockheader matches the value in nBits */

/** Check that the proof of work on each blockheader matches the value in nBits */

danielabrozzoni

Thanks for the reviews!

In my last push:

Rebased onto the right commit 😅 (see #32740 (comment))
Addressed review comments

danielabrozzoni · 2025-08-15T16:35:12Z

src/validation.cpp

@@ -4149,8 +4149,8 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&

 bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams)
 {
-    return std::all_of(headers.cbegin(), headers.cend(),
-            [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);});
+    return std::ranges::all_of(headers,


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

danielabrozzoni · 2025-08-18T09:12:19Z

src/net_processing.cpp

Ooops, not sure what happened there, thanks for noticing!

danielabrozzoni · 2025-08-18T10:01:36Z

src/headerssync.cpp

-    m_current_chain_work(chain_start->nChainWork),
-    m_last_header_received(m_chain_start->GetBlockHeader()),
-    m_current_height(chain_start->nHeight)
+                                   const CBlockIndex& chain_start, const arith_uint256& minimum_required_work) : m_commit_offset(FastRandomContext().randrange<unsigned>(HEADER_COMMITMENT_PERIOD)),


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

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

hodlinator · 2025-08-18T15:10:46Z

src/headerssync.cpp

-    m_current_chain_work(chain_start->nChainWork),
-    m_last_header_received(m_chain_start->GetBlockHeader()),
-    m_current_height(chain_start->nHeight)
+                                   const CBlockIndex& chain_start, const arith_uint256& minimum_required_work) : m_commit_offset(FastRandomContext().randrange<unsigned>(HEADER_COMMITMENT_PERIOD)),


(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).

l0rinc · 2025-08-21T03:01:19Z

recrACK 50e61f448eedfe0ccfa07f3124aeef9c7762a69b

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

src/net_processing.cpp

src/headerssync.h

src/headerssync.cpp

src/net_processing.cpp

l0rinc · 2025-11-03T10:29:59Z

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 (comment) was on purpose or not, but either is fine with me.

danielabrozzoni · 2025-11-03T12:42:45Z

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

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

l0rinc · 2025-11-03T12:51:16Z

Code review reACK 9a387e9175d34d1eaf83b9008f5e857d1eb04b8e

Finalized the pointer to reference migration since last ack.

hodlinator

re-ACK 9a387e9175d34d1eaf83b9008f5e857d1eb04b8e

Changes since previous review (#32740 (review)):

Re-added Assert in response to #32740 (comment)
I was slightly worried the lock lambda would become more likely to return by-value, instead of by-reference, but CBlockIndex has protected/deleted copying:
https://github.com/bitcoin/bitcoin/blob/9a387e9175d34d1eaf83b9008f5e857d1eb04b8e/src/chain.h#L335-L349

hodlinator · 2025-11-03T19:30:58Z

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

--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -52,7 +52,7 @@ 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 *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
+    const CBlockIndex& chain_start{ChainStart()};
 
     // Generate headers for two different chains (using differing merkle roots
     // to ensure the headers are different).
@@ -101,6 +101,15 @@ private:
     std::vector<CBlockHeader> GenerateHeaders(size_t count,
             uint256 prev_hash, int32_t nVersion, uint32_t prev_time,
             const uint256& merkle_root, uint32_t nBits);
+
+    // Broken out into its own function to please 32-bit ARM (GCC 13.3) and
+    // Linux->Windows cross builds (GCC 13.0) which otherwise complain about
+    // "possibly dangling reference to a temporary [-Werror=dangling-reference]"
+    const CBlockIndex& ChainStart()
+    {
+        LOCK(::cs_main);
+        return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash()));
+    }
 };
 
 void HeadersGeneratorSetup::FindProofOfWork(CBlockHeader& starting_header)

Without this, compile warnings could be hit about __func__ being only valid inside functions. warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert 115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val) | ^ Ref bitcoin#32740 (comment)

danielabrozzoni · 2025-11-10T04:54:09Z

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.

hodlinator · 2025-11-10T07:39:23Z

* 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 fae1d99 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.

l0rinc · 2025-11-10T10:14:40Z

src/test/headers_sync_chainwork_tests.cpp

@@ -52,7 +52,7 @@ 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 *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
+    const CBlockIndex& chain_start{ChainStart()};


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

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

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

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

Aha, that explains why I was getting other results when not rebasing onto latest master (#32740 (comment)).

fad6efd refactor: Use STR_INTERNAL_BUG macro where possible (MarcoFalke) fada379 doc: Remove unused bugprone-lambda-function-name suppression (MarcoFalke) fae1d99 refactor: Use const reference to std::source_location (MarcoFalke) fa5fbcd util: Allow Assert() in contexts without __func__ (MarcoFalke) Pull request description: Without this, compile warnings could be hit about `__func__` being only valid inside functions. ``` warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert 115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val) | ^ ``` Ref #32740 (comment) This also introduces a slight behaviour change, because `std::source_location::function_name` usually includes the entire function signature instead of just the name. ACKs for top commit: l0rinc: Code review ACK fad6efd stickies-v: ACK fad6efd hodlinator: re-ACK fad6efd Tree-SHA512: e78a2d812d5ae22e45c93db1661dafbcd22ef209b3d8d8d5f2ac514e92fd19a17c3f0a5db2ef5e7748aa2083b10c0465326eb36812e6a80e238972facd2c7e98

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 <[email protected]>

danielabrozzoni · 2025-11-20T03:23:00Z

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 refactor: Header sync optimisations & simplifications #32740 (comment), 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! :)

l0rinc · 2025-11-20T10:32:22Z

ACK de4242f

Only a const removal since my last ACK

hodlinator · 2025-11-20T19:18:33Z

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

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

Checked this by:

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

Rebasing onto the merge commit of #33785:

₿ 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:
fae1d99#diff-01c24871dbdf85d7ddae09d013d9280074676ba15cbabb07258e8e9a8f5a1640R16

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

hodlinator

re-ACK de4242f

Only removed const from HeadersGeneratorSetup::chain_start to work around GCC 13 issue since my previous ACK (#32740 (review)).

hodlinator · 2025-11-20T19:36:57Z

src/test/headers_sync_chainwork_tests.cpp

@@ -52,7 +52,7 @@ 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 *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
+    const CBlockIndex& chain_start{ChainStart()};


Aha, that explains why I was getting other results when not rebasing onto latest master (#32740 (comment)).

polespinasa · 2026-01-07T09:42:29Z

re-ACK de4242f

lgtm nothing to add to what others already said :)

danielabrozzoni · 2026-01-14T14:15:22Z

Ready for merge?

sipa · 2026-01-14T15:12:41Z

ACK de4242f

achow101 · 2026-01-14T19:25:03Z

ACK de4242f

DrahtBot added the Refactoring label Jun 12, 2025

DrahtBot mentioned this pull request Jun 13, 2025

p2p: Correct unrealistic headerssync unit test behavior #32579

Merged

danielabrozzoni changed the title ~~refactor: Optimizations & simplifications following #25717~~ refactor: Header sync optimisations & simplifications #25717 Jun 13, 2025

danielabrozzoni changed the title ~~refactor: Header sync optimisations & simplifications #25717~~ refactor: Header sync optimisations & simplifications Jun 13, 2025

hodlinator reviewed Jun 16, 2025

View reviewed changes

danielabrozzoni force-pushed the upforgrabs/25968 branch from 13e123e to 7634d81 Compare June 24, 2025 12:31

l0rinc reviewed Jul 4, 2025

View reviewed changes

src/headerssync.h Outdated Show resolved Hide resolved

src/test/headers_sync_chainwork_tests.cpp Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the upforgrabs/25968 branch from 7634d81 to f4b33a1 Compare July 7, 2025 16:49

l0rinc suggested changes Jul 24, 2025

View reviewed changes

danielabrozzoni force-pushed the upforgrabs/25968 branch 2 times, most recently from 4d95119 to f33bd2f Compare August 8, 2025 12:46

danielabrozzoni commented Aug 8, 2025

View reviewed changes

l0rinc approved these changes Aug 8, 2025

View reviewed changes

hodlinator reviewed Aug 12, 2025

View reviewed changes

danielabrozzoni force-pushed the upforgrabs/25968 branch from f33bd2f to 50e61f4 Compare August 18, 2025 14:21

danielabrozzoni commented Aug 18, 2025

View reviewed changes

hodlinator approved these changes Aug 18, 2025

View reviewed changes

DrahtBot requested a review from l0rinc August 18, 2025 15:41

l0rinc approved these changes Aug 21, 2025

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

src/headerssync.h Show resolved Hide resolved

src/headerssync.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

l0rinc mentioned this pull request Aug 22, 2025

rpc: followups for min fee changes #33189

Merged

hodlinator mentioned this pull request Sep 2, 2025

clang-format: make formatting deterministic for different formatter versions #32813

Merged

hodlinator mentioned this pull request Oct 6, 2025

HeadersSync tracking issue #33547

Open

DrahtBot added the Needs rebase label Oct 23, 2025

hodlinator mentioned this pull request Oct 27, 2025

headerssync: Add headers cache l0rinc/bitcoin#3

Closed

danielabrozzoni force-pushed the upforgrabs/25968 branch from 8a4ab45 to 9a387e9 Compare November 3, 2025 12:38

DrahtBot requested a review from hodlinator November 3, 2025 12:51

hodlinator approved these changes Nov 3, 2025

View reviewed changes

maflcko mentioned this pull request Nov 4, 2025

util: Allow Assert (et al.) in contexts without __func__ #33785

Merged

l0rinc reviewed Nov 10, 2025

View reviewed changes

DrahtBot requested a review from l0rinc November 10, 2025 10:14

DrahtBot mentioned this pull request Nov 12, 2025

kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState #33856

Open

danielabrozzoni force-pushed the upforgrabs/25968 branch from 184c54c to de4242f Compare November 20, 2025 02:37

hodlinator approved these changes Nov 20, 2025

View reviewed changes

DrahtBot mentioned this pull request Dec 7, 2025

validation: Remove min_pow_checked arg in ProcessNewBlockHeaders #34022

Closed

achow101 merged commit b0b6533 into bitcoin:master Jan 14, 2026
23 checks passed

		@@ -393,7 +393,7 @@ bool TestBlockValidity(BlockValidationState& state,
		bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

		/** Check with the proof of work on each blockheader matches the value in nBits */

	/** Check with the proof of work on each blockheader matches the value in nBits */
	/** Check that the proof of work on each blockheader matches the value in nBits */

refactor: Header sync optimisations & simplifications #32740

refactor: Header sync optimisations & simplifications #32740

Conversation

danielabrozzoni commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

ajtowns commented Jun 13, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielabrozzoni commented Jun 24, 2025

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danielabrozzoni commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielabrozzoni commented Aug 1, 2025

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

danielabrozzoni commented Jun 12, 2025 •

edited

Loading

DrahtBot commented Jun 12, 2025 •

edited

Loading

danielabrozzoni commented Jul 7, 2025 •

edited

Loading

hodlinator Aug 12, 2025 •

edited

Loading

hodlinator Aug 12, 2025 •

edited

Loading

hodlinator left a comment •

edited

Loading