kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState

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

Motivation

This PR refactors ProcessNewBlock() and 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.
ProcessNewBlock(): Returns a simple boolean with no information about why validation failed.
bitcoinkernel API: btck_chainstate_manager_process_block() returns int (0/-1) with no way for library users to determine validation failure reasons without registering callbacks.

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 3 commits, see individual commit message for more info.

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.

e78d9b2bcf

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
Concept ACK	hodlinator, exp3rimenter
Stale ACK	w0xlt

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)
#33553 (validation: Improve warnings in case of chain corruption by mzumsande)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)
#32740 (refactor: Header sync optimisations & simplifications by danielabrozzoni)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

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:4550 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:4356 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.

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

validation, kernel: Return BlockValidationState from ProcessNewBlock

Return `BlockValidationState` by value instead of passing a reference, following the pattern established in 74690f4ed82b1584abb07c0387db0d924c4c0cab for `TestBlockValidity`.

This provides verbose information to callers about why block processing succeeded or failed. Previously, callers needed to register validation interface callbacks to determine the specific validation failure reason.

Co-authored-by: w0xlt <woltx@protonmail.com>

20960d4868

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.

857ebdd20c

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

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

Motivation

Current Issues

Code Coverage & Benchmarks

Reviews

Conflicts