kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState

yuvicc commented at 5:23 pm on November 11, 2025: contributor

Motivation

This PR refactors ProcessNewBlockHeaders() to return BlockValidationState by value instead of using out-parameters or boolean returns. This follows the pattern established in #31981 (commit 74690f4) which refactored TestBlockValidity() in a similar manner.

Current Issues

ProcessNewBlockHeaders(): Uses an out-parameter BlockValidationState& state, making the API less intuitive.

As noted by @theCharlatan in #33822 comment and can be a fix for that too:

One thing that could be considered here is returning the BlockValidationState directly instead of having an in/out param. To safely do that I think we’d need to refactor ProcessNewBlockHeaders though, similarly to what was done in 74690f4.

The changes are split into two commits, see individual commit message for more info.

DrahtBot commented at 5:24 pm on November 11, 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/33856.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt
Concept ACK	hodlinator, exp3rimenter
Stale ACK	danielabrozzoni

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:

#33908 (kernel: add context‑free block validation API (btck_check_block_context_free) with POW/Merkle flags by w0xlt)
#33822 (kernel: Add block header support and validation by yuvicc)
#33796 (kernel: Expose CheckTransaction consensus validation function by w0xlt)
#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.

in src/validation.cpp:4489 in 4c6301c01c outdated

4547+        return state;
4548     }
4549 
4550     Chainstate* bg_chain{WITH_LOCK(cs_main, return BackgroundSyncInProgress() ? m_ibd_chainstate.get() : nullptr)};
4551     BlockValidationState bg_state;
4552     if (bg_chain && !bg_chain->ActivateBestChain(bg_state, block)) {

hodlinator commented at 8:13 am on November 12, 2025:

Why not reuse state here?

yuvicc commented at 6:44 am on November 16, 2025:

The problem is that ActivateBestChain would overwrite the state result, when the background chain’s ActivateBestChain succeeds, it would overwrite state with its own state. Then at line 4554, we’d be returning the background chain’s state instead of the active chainstate’s state. So by separating out, we preserve the active chainstate result.

hodlinator commented at 1:21 pm on November 17, 2025:

But if state contained anything interesting, wouldn’t we already have returned it before reaching this block? Callers of the function only see either bg_state or state as this PR stands anyway.

Thanks for taking my other suggestions!

yuvicc commented at 10:53 am on November 19, 2025:

Correct. Good observation. So at this stage the state already represents success. If the background chain fails, overwriting it with the failure state is fine - that’s what you want to return anyway. If the background chain succeeds, state gets overwritten with success - also fine. The only argument for keeping bg_state separate would be if you wanted clearer variable naming to show intent, but functionally reusing state works fine here.

w0xlt commented at 6:47 pm on November 20, 2025:

nit: Maybe the variables could be renamed in this PR for better clarity. The “description” rows below can be converted into comments to document the variables.

Current name	Suggestion	Description
`state`	`accept_state`	determines if the block data is valid enough to be written to the disk and entered into the block index.
`bg_state`	`activation_state`	attempts to connect the block to the tip of the active chain (executing scripts and updating the UTXO set).

yuvicc commented at 4:53 am on November 23, 2025:

Makes sense. Thanks.

in src/validation.cpp:4537 in 4c6301c01c outdated

4532@@ -4533,26 +4533,25 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& blo
4533                 m_options.signals->BlockChecked(block, state);
4534             }
4535             LogError("%s: AcceptBlock FAILED (%s)\n", __func__, state.ToString());
4536-            return false;
4537+            return state;

hodlinator commented at 8:14 am on November 12, 2025:

Should remove the shadowing state on line 4515 IMO.

in src/validation.cpp:4289 in b9d4ea94a0 outdated

4348@@ -4349,9 +4349,10 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
4349 }
4350 
4351 // Exposed wrapper for AcceptBlockHeader
4352-bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex)
4353+BlockValidationState ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex)
4354 {
4355     AssertLockNotHeld(cs_main);
4356+    BlockValidationState state;

hodlinator commented at 8:22 am on November 12, 2025:

Worth adding an Assume(!headers.empty()) before this, as state will be unset if we don’t process any, whereas before we would return true regardless?

yuvicc commented at 6:59 am on November 16, 2025:

Correct, we will definitely need that.

danielabrozzoni commented at 4:55 pm on December 1, 2025:

I’m ok with adding the Assume, but I don’t think it would have been a problem, as BlockValidationState is initialized to valid: https://github.com/bitcoin/bitcoin/blob/6356041e58d1ba86695e2e7c219c68ee5abe583f/src/consensus/validation.h#L82

I tested locally with:

 0diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
 1index 9973b33b57..f9080ef40f 100644
 2--- a/src/test/validation_block_tests.cpp
 3+++ b/src/test/validation_block_tests.cpp
 4@@ -363,4 +363,9 @@ BOOST_AUTO_TEST_CASE(witness_commitment_index)
 5 
 6     BOOST_CHECK_EQUAL(GetWitnessCommitmentIndex(pblock), 2);
 7 }
 8+
 9+BOOST_AUTO_TEST_CASE(test_empty_process_new_block_headers) {
10+    auto res = m_node.chainman->ProcessNewBlockHeaders({}, true);
11+    BOOST_CHECK(res.IsValid());
12+}
13 BOOST_AUTO_TEST_SUITE_END()

hodlinator commented at 8:32 am on November 12, 2025: contributor

Concept ACK moving out-reference-parameters to return value

Not familiar with the kernel API but had a brief look at net_processing and validation.

w0xlt commented at 11:18 pm on November 13, 2025: contributor

Concept ACK

yuvicc force-pushed on Nov 16, 2025

DrahtBot added the label CI failed on Nov 16, 2025

DrahtBot commented at 7:24 am on November 16, 2025: contributor

🚧 At least one of the CI tasks failed. Task Linux->Windows cross, no tests: https://github.com/bitcoin/bitcoin/actions/runs/19402124289/job/55511133875 LLM reason (✨ experimental): Compilation failed due to a syntax error in validation.cpp (missing semicolon before BlockValidationState in ProcessNewBlockHeaders).

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

yuvicc commented at 7:32 am on November 16, 2025: contributor

Thanks for the review @hodlinator

Added Assume(!headers.empty()) before instantiating validation state as suggested in comment
Removed shadow state comment

DrahtBot removed the label CI failed on Nov 17, 2025

in src/test/blockfilter_index_tests.cpp:107 in 1f589e1af1 outdated

103@@ -104,8 +104,7 @@ bool BuildChainTestingSetup::BuildChain(const CBlockIndex* pindex,
104         block = std::make_shared<CBlock>(CreateBlock(pindex, no_txns, coinbase_script_pub_key));
105         CBlockHeader header = block->GetBlockHeader();
106 
107-        BlockValidationState state;
108-        if (!Assert(m_node.chainman)->ProcessNewBlockHeaders({{header}}, true, state, &pindex)) {
109+        if (BlockValidationState state{Assert(m_node.chainman)->ProcessNewBlockHeaders({{header}}, true, &pindex)}; !state.IsValid()) {

yancyribbens commented at 0:03 am on November 18, 2025:

in 1f589e1af162c2c2705f0404496c549785f2f545 could !state.IsValud() be state.IsInvalid()?

yuvicc commented at 11:00 am on November 19, 2025:

No we cannot as:

0    enum class ModeState {
1        M_VALID,   //!< everything ok
2        M_INVALID, //!< network rule violation (DoS value may be set)
3        M_ERROR,   //!< run-time error
4    }

!state.IsValid() returns true when the state is either M_INVALID or M_ERROR state.IsInvalid() returns true only when the state is M_INVALID, misses the M_ERROR or run-time error case.

exp3rimenter commented at 2:50 pm on November 19, 2025: none

Concept ACK on refactoring bool to return state.

w0xlt commented at 7:23 pm on November 20, 2025: contributor

This PR eliminates the ambiguity of the boolean return value and improves the clarity of both the internal and kernel APIs.

ACK https://github.com/bitcoin/bitcoin/pull/33856/commits/1f589e1af162c2c2705f0404496c549785f2f545 (with the above-mentioned nit)

DrahtBot requested review from hodlinator on Nov 20, 2025

yuvicc force-pushed on Nov 23, 2025

yuvicc commented at 6:03 am on November 23, 2025: contributor

Thanks for the review @w0xlt

Addressed the suggestion and added as co-author

in src/validation.cpp:4296 in 8d971934f9 outdated

4361@@ -4360,7 +4362,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> hea
4362             CheckBlockIndex();
4363 
4364             if (!accepted) {
4365-                return false;

sedited commented at 1:35 pm on November 23, 2025:

We should include post-condition checks here. For the TestBlockValidity refactor, we added

0if (state.IsValid()) NONFATAL_UNREACHABLE();

I think such a check should be added wherever a boolean was returned previously.

in src/validation.cpp:4514 in 8d971934f9 outdated

4506@@ -4505,14 +4507,15 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,
4507     return true;
4508 }
4509 
4510-bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked, bool* new_block)
4511+BlockValidationState ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked, bool* new_block)

sedited commented at 1:47 pm on November 23, 2025:

I don’t think this change is correct. Previously false was returned when the block failed to be accepted or on a fatal error condition, not when it failed to validate. If you look into ActivateBestChainStep, you’ll see that we break on a validation failure, reset the state again and don’t return false. Can you drop this change again?

yuvicc force-pushed on Nov 25, 2025

yuvicc commented at 2:28 pm on November 25, 2025: contributor

Thanks for the review @TheCharlatan.

Addressed comment on using post-condition checks.
Dropped the change to use separate state for reporting errors and not invalidity as suggested by @TheCharlatan comment

in src/validation.cpp:4560 in 857ebdd20c

4562-        return false;
4563-     }
4564 
4565-    return true;
4566+    // Attempts to connect the block to the tip of the active chain.
4567+    BlockValidationState activation_state;

sedited commented at 9:13 pm on November 26, 2025:

I’m confused by this change. Why are you introducing this variable? The comment is also wrong, since this processes the block on the background chain. I would still prefer if the behaviour of ProcessNewBlock would not be changed as part of this patch set.

yuvicc commented at 3:09 am on November 27, 2025:

I don’t think the behavior of ProcessNewBlock is changed here. It’s just a change in variable name (bg_state -> activation_state) see comment, we can keep the change as is if you prefer that way?

sedited commented at 10:23 am on November 27, 2025:

The comment you linked seems misleading. bg_state is the correct name in my view. We are not operating on the active chain here.

I think the belt-and-suspenders check in ActivateBestChain is broken by this change. When we return false in the case of the chain being disabled, we now run into NONFATAL_UNREACHABLE. It might be true that there are no similar cases in the call graph of ActivateBestChain, but can we guarantee that? For this reason, I think the commit should be dropped.

yuvicc force-pushed on Nov 27, 2025

yuvicc commented at 11:27 am on November 27, 2025: contributor

I agree with @sedited comment, the belt-and-suspenders check in ActivateBestChain in validation.cpp is broken by this change. When m_disabled is true, ActivateBestChain returns false without setting the BlockValidationState to invalid. This causes the new NONFATAL_UNREACHABLE() assert in ProcessNewBlock to trigger incorrectly. The belt-and-suspenders check exists precisely because we can’t guarantee ActivateBestChain won’t be called on a disabled chainstate.

So dropping the change in ProcessNewBlock and only keeping ProcessNewBlockHeaders now.

yuvicc renamed this:
~~kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState~~
kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState
on Nov 27, 2025

danielabrozzoni commented at 10:31 am on December 8, 2025: member

light ACK f31f7c21ff7093a4c90199cd0bb2128d19bf2d33

Code looks good to me, and the interface is cleaner now. I’m only light-acking sicne I’m not familiar with the kernel :)

As I pointed out in a comment, I don’t think we needed the Assume(!headers.empty()) at the start of ProcessNewBlockHeaders, but I’m ok with keeping it.

DrahtBot requested review from w0xlt on Dec 8, 2025

DrahtBot added the label Needs rebase on Dec 9, 2025

kernel: Add Handle/View pattern for BlockValidationState

Add C API functions for managing BlockValidationState lifecycle:
  - btck_block_validation_state_create()
  - btck_block_validation_state_copy()
  - btck_block_validation_state_destroy()

Introduce BlockValidationStateApi<> template to share common getter methods between BlockValidationState (Handle) and BlockValidationStateView (View) classes in the C++ wrapper.

Update ValidationInterface::BlockChecked to use BlockValidationStateView since it doesn't need ownership.

This changes prepares the kernel API to return BlockValidationState by value in subsequent commits.

88af3a29d9

validation: Return BlockValidationState from ProcessNewBlockHeaders

Return BlockValidationState by value instead of using an out-parameter, similar to the TestBlockValidity refactoring in 74690f4ed82b1584abb07c0387db0d924c4c0cab.

This provides a cleaner API and enables callers to inspect detailed validation state without relying on side effects through reference parameters.

be379fd52b

yuvicc force-pushed on Dec 10, 2025

yuvicc commented at 4:58 am on December 10, 2025: contributor

Rebased f31f7c2 -> be379fd.

DrahtBot removed the label Needs rebase on Dec 10, 2025

w0xlt commented at 0:33 am on December 11, 2025: contributor

reACK https://github.com/bitcoin/bitcoin/pull/33856/commits/be379fd52b82805ab99400457ea44f93430f16e2

DrahtBot requested review from danielabrozzoni on Dec 11, 2025

kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState #33856

Motivation

Current Issues

Code Coverage & Benchmarks

Reviews

Conflicts