validation: Remove min_pow_checked arg in ProcessNewBlockHeaders

sedited commented at 12:20 pm on December 6, 2025: contributor

It is always set to true, since its introduction in ed6cddd98e32263fc116a4380af6d66da20da990. It is supposed to “…ensure that all call-sites which might cause a new header to be accepted into memory have to grapple with the question of whether the header is safe to accept, or needs further validation”. I think it has surpassed its usefulness at this point for this function. Headers presync is a well-established concept, and developers imitate existing call sites, without necessarily considering its nested implication, calling its usefulness into question. The path also lacks unit and fuzz tests.

DrahtBot added the label Validation on Dec 6, 2025

DrahtBot commented at 12:20 pm on December 6, 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/34022.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, danielabrozzoni, yuvicc

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> 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)
#32740 (refactor: Header sync optimisations & simplifications by danielabrozzoni)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

validationinterface -> validation interface (or ValidationInterface) [compound word lacks spacing/incorrect capitalization and may be unclear; write as two words “validation interface” or use the actual class name “ValidationInterface”]

2025-12-08

yuvicc commented at 1:57 am on December 7, 2025: contributor

Concept ACK

in src/test/validation_block_tests.cpp:107 in beff6ff964 outdated

103@@ -104,7 +104,7 @@ std::shared_ptr<CBlock> MinerTestingSetup::FinalizeBlock(std::shared_ptr<CBlock>
104     // submit block header, so that miner can get the block height from the
105     // global state and the node has the topology of the chain
106     BlockValidationState ignored;
107-    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({{pblock->GetBlockHeader()}}, true, ignored));
108+    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({{pblock->GetBlockHeader()}}, ignored));

Ataraxia009 commented at 6:15 am on December 7, 2025:

If you are going to do this, then add a comment here saying ProcessNewBlockHeaders assumes that the caller has checked for min pow or something along those lines, for clarity

sedited commented at 10:28 am on December 7, 2025:

Not sure if that is really useful. We have a lot of functions that assume some check has been done beforehand to protect against dos. I don’t see this function being misused again in the future by mistake, headers-presync is just too well established in the code base at this point. Maybe a more complete description of what the function does would be useful for people reading this the first time, that could then in turn describe why a dos check should be done before calling. I you want, I could add one.

in src/validation.cpp:4294 in beff6ff964 outdated

4291     {
4292         LOCK(cs_main);
4293         for (const CBlockHeader& header : headers) {
4294             CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast
4295-            bool accepted{AcceptBlockHeader(header, state, &pindex, min_pow_checked)};
4296+            bool accepted{AcceptBlockHeader(header, state, &pindex, /*min_pow_checked=*/true)};

l0rinc commented at 10:25 am on December 7, 2025:

I redid the change locally and I think this line is slightly different from the rest: could we do it in a separate commit before the refactor, so that after that the arg removal is a clean refactor?

We could also add assert(min_pow_checked); to the first commit, so that reviewers and CI can run all tests with the assert as an extra safety measure before the removal in the next one.

0assert(min_pow_checked); // TODO removed in next commit
1bool accepted{AcceptBlockHeader(header, state, &pindex, /*min_pow_checked=*/true)};

As a bonus, we could add type name hints to the remaining naked true values for hard-coded /*min_pow_checked=*/ assignments for consistency.

This also begs the question: is AcceptBlockHeader in the same situation, would the tests pass if we added an assert(min_pow_checked); to that method? The only variable assignment is in https://github.com/bitcoin/bitcoin/blob/89dc82295ebd8a959fb5f6e7a116b6ce9e44e340/src/validation.cpp#L4353 which is only called with a non-const true value from https://github.com/bitcoin/bitcoin/blob/89dc82295ebd8a959fb5f6e7a116b6ce9e44e340/src/validation.cpp#L4464 which is only called with non-true from https://github.com/bitcoin/bitcoin/blob/89dc82295ebd8a959fb5f6e7a116b6ce9e44e340/src/net_processing.cpp#L3293 which can be false if https://github.com/bitcoin/bitcoin/blob/fa05181d904d620cd8944123e64e3931b21298f9/src/net_processing.cpp#L4670 isn’t fulfilled - so keeping the min_pow_checked in AcceptBlockHeader is correct - several tests fail correctly if we do that.

sedited commented at 6:51 pm on December 7, 2025:

See https://github.com/sedited/bitcoin/tree/rmHeaderLowWork :)

The reason we are using it in net_processing is the following: We might receive an unrequested block whose block header we previously have not processed and which forks off at a low-work point. A peer sending as such a block is not punished, but the message is essentially just ignored. We could just check for that condition in net_processing and return early, but then we are also skipping a bunch of validation work, that might catch validation failures that would lead to the peer being disconnected, for example passing a block having invalid proof of work. It’s not clear to me what additional assumption that introduces.

l0rinc approved

l0rinc commented at 11:21 am on December 7, 2025: contributor

Redid the change locally, ended up with basically the same state. I suggest separating it into two commits to make make the change even safer to review, but I’m also fine with merging as-is (since I already did that locally). Happy to re-ack if needed.

ACK beff6ff964c5c1b4a96af89daf6b2ddaede3464a

in src/validation.h:1233 in beff6ff964 outdated

1229@@ -1230,12 +1230,11 @@ class ChainstateManager
1230      * validationinterface callback.
1231      *
1232      * @param[in]  headers The block headers themselves
1233-     * @param[in]  min_pow_checked  True if proof-of-work anti-DoS checks have been done by caller for headers chain

darosior commented at 3:30 pm on December 8, 2025:

Should we explicitly document “this function will not check pow!” then?

sedited commented at 3:44 pm on December 8, 2025:

Do you mean “this function will not check for a minimum proof of work threshold”? But as I said here, I think the potential for misuse at this point is low. I’d gladly add a text if you want to suggest one though.

darosior commented at 4:12 pm on December 8, 2025:

I hadn’t seen your other comment. Makes sense.

Sjors commented at 6:52 pm on December 8, 2025:

Maybe something loosely based on the original commit message:

all call-sites which might cause a new header to be accepted into memory have to grapple with the question of whether the header is safe to accept, or needs further validation

sedited commented at 1:05 pm on December 14, 2025:

Maybe something loosely based on the original commit message:

Thanks, added something along the lines of it.

Sjors commented at 6:44 pm on December 8, 2025: member

It is always set to true

🌍👨‍🚀🔫👨‍🚀

This was intentional, as explained in the commit message of ed6cddd98e32263fc116a4380af6d66da20da990 (part of #25717):

This commit adds a new argument to AcceptBlockHeader() so that we can ensure that all call-sites which might cause a new header to be accepted into memory have to grapple with the question of whether the header is safe to accept, or needs further validation.

Not sure if we need to keep it forever, as you say in your comment: #34022 (review).

cc @sdaftuar

validation: Remove min_pow_checked arg in ProcessNewBlockHeaders

It is always set to true, so there is no point in keeping it.

8ca9997e48

sedited force-pushed on Dec 8, 2025

sedited commented at 10:11 pm on December 8, 2025: contributor

Updated beff6ff964c5c1b4a96af89daf6b2ddaede3464a -> 8ca9997e48bb2067ea83fabb1c640af72178f97c (rmMinPowChecked_0 -> rmMinPowChecked_1, compare)

Addressed comment, and comment, documenting the proof of work pre-check as an anti-dos measure in the function docstring.

l0rinc commented at 10:14 pm on December 8, 2025: contributor

reACK 8ca9997e48bb2067ea83fabb1c640af72178f97c

only doc updates since last ACK

danielabrozzoni commented at 2:27 pm on December 10, 2025: member

ACK 8ca9997e48bb2067ea83fabb1c640af72178f97c - it makes sense to remove the parameter since it’s always set to true.

I initially thought that min_pow_checked = true meant “we’ve already verified this block’s proof of work, i.e., that the block’s hash is lower than the current target”, but after some digging I realized that wasn’t the case… This is my updated understanding, can someone double check if it’s correct? There are two types of PoW check:

Check if the block proof of work satisfies the requirement specified by nbits. It happens in CheckProofOfWork, which here gets called in CheckBlockHeader, called from AcceptBlockHeader. This check is run regardless of the min_pow_checked value.
Proof of work threshold check: this is what min_pow_checked refers to. We check that the received block, along with its previous chain, has enough proof of work (higher than GetAntiDoSWorkThreshold). This is to protect us from writing to disk a block that is forking off a low-work point in the chain.

yuvicc commented at 12:41 pm on December 13, 2025: contributor

ACK 8ca9997e48bb2067ea83fabb1c640af72178f97c

mzumsande commented at 8:47 pm on December 14, 2025: contributor

I’m not convinced by this. Since headers are append-only and never deleted they are one of the most important memory DoS vectors (in contrast to transactions and addrs, which are stored in containers that are limited), and there have been bugs about this in the past.

So I think it’s justified from a defensive programming standpoint to have a bit of extra caution by making the PoW check explicit so that any callers in future changes are forced to deal with this, even if the documentation is missed.

I would prefer to keep the arg (which has a minimal cost), and maybe in addition to the doc added here also add a line to the documentation of the function that the arg is there on purpose, even if it’s currently true for all current callers.

l0rinc commented at 8:54 pm on December 14, 2025: contributor

@mzumsande a constant value with comment won’t bite back if misused: can we rather add a test that makes sure this remains correct (while removing the misleading arg)?

sedited commented at 9:14 pm on December 14, 2025: contributor

Re #34022#pullrequestreview-3575826688

So I think it’s justified from a defensive programming standpoint to have a bit of extra caution by making the PoW check explicit so that any callers in future changes are forced to deal with this, even if the documentation is missed.

I just don’t think this does much from a defensive programming perspective. Reading through the comments here, people seem confused on what it even is supposed to signal, and my guess is that it just being true everywhere leads people to just blindly copy that anyway.

That said, if people believe a bit of defensive programming is necessary here, an alternative to this could be (and this is something I could similarly see working for Blocks with regards to fChecked) introducing a separate min pow checked header type that can only be retrieved from sources where we indeed do the check, or have clearly ascertained that it is harmless. That should leave little in the way of applying it inconsistently, or being confused about its purpose. What do you think?

darosior commented at 9:02 am on December 15, 2025: member

What do you think?

If doable, introducing some type safety here does sound compelling.

mzumsande commented at 6:06 pm on December 15, 2025: contributor

I just don’t think this does much from a defensive programming perspective. Reading through the comments here, people seem confused on what it even is supposed to signal, and my guess is that it just being true everywhere leads people to just blindly copy that anyway.

To address that type of confusion maybe just the name would need be clearer, for example has_minimum_acceptable_pow or similar.

can we rather add a test that makes sure this remains correct (while removing the misleading arg)?

What that acceptable minimum work is depends on the context and how trusted the caller is, could be minchainwork or nothing - what would a unit or functional test for this look like?

That said, if people believe a bit of defensive programming is necessary here, an alternative to this could be (and this is something I could similarly see working for Blocks with regards to fChecked) introducing a separate min pow checked header type that can only be retrieved from sources where we indeed do the check, or have clearly ascertained that it is harmless.

that sounds like an interesting idea, even though I’m not sure I understand it completely yet.

One point for removing the arg that hasn’t been brought up yet is that we now have a fuzz test with a rather general design (#30661) that should catch many types of possible future bugs / regressions.

Also worth noting that there is ongoing and planned work in this area (#33547), so even if headers-sync is well-established by now, the underlying code might undergo some changes in the near future.

sedited commented at 8:28 pm on December 15, 2025: contributor

Thanks for the reviews. I think I’ll close this again based on the number of concerns raised. A more comprehensive solution that retains some defensive techniques to prohibit acceptance of low-work headers was raised repeatedly as desirable. I’d prefer to do that at some point on a clean slate.

sedited closed this on Dec 15, 2025

validation: Remove min_pow_checked arg in ProcessNewBlockHeaders #34022

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)