refactor: Change some validation.cpp methods to return BlockValidationState #35570

optout21 commented at 6:15 AM on June 20, 2026: contributor

Summary. Refactor validation result to be a return value instead of an output parameter in several validation.cpp methods.

Motivation. The benefits of the change are:

Exclude the potentially inconsistent case when the bool return value and the returned state are inconsistent
Exclude the ambiguity whether the passed in value of state is used or not (not obvious in chained calls)
Remove the possibility of unintuitive interaction between subsequent calls with the same state. In case of a validation error, a failure reason is set if the state was valid, but not if it was already invalid.
Slightly simpler: It's more evident which is the result; one less parameters.

This has grown out from #33856, mentioned in comment here and here.

Details. Many methods follow the scheme where the validation state is returned in an output parameter (BlockValidationState& state), and and additional bool return value indicating success. In success case the convention is that state.IsValid() and the return value are both true. After the change there is only a BlockValidationState return value, which is either success (state.IsValid() == true), or an invalid/error case.

This change is a highly localized refactor, but touching a sensitive file.

Relevant methods called by ProcessNewBlockHeaders and AcceptBlock (directly and indirectly) are touched.

Changes are separated into commits by touched methods, ordered by bottom-to-top in the call hierarchy.

DrahtBot added the label Refactoring on Jun 20, 2026

DrahtBot commented at 6:15 AM on June 20, 2026: 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/35570.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	w0xlt, purpleKarrot, stringintech, yuvicc
Stale ACK	arejula27

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35646 (RFC: Separate out runtime errors from BlockValidationState using util::Expected by yuvicc)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#35557 (kernel, validation: Add btck_chainstate_manager_set_clock_time by ryanofsky)
#34895 (fuzz: Fuzzing harnesses for ActivateBestChainStep and ActivateBestChain by RobinDavid)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)

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 places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

CheckBlock(block, chainparams.GetConsensus(), false, false) in src/bench/duplicate_inputs.cpp
CheckBlock(block, consensus_params, true, true) in src/test/fuzz/block.cpp
CheckBlock(block, consensus_params, true, false) in src/test/fuzz/block.cpp
CheckBlock(block, consensus_params, false, true) in src/test/fuzz/block.cpp
CheckBlock(block, consensus_params, false, false) in src/test/fuzz/block.cpp
AcceptBlock(new_block, &new_block_index, true, nullptr, nullptr, true) in src/test/coinstatsindex_tests.cpp
AcceptBlock(pblockone, &pindex, true, nullptr, &newblock, true) in src/test/validation_chainstate_tests.cpp
AcceptBlock(pblock, nullptr, true, dbp, nullptr, true) in src/validation.cpp
AcceptBlock(pblockrecursive, nullptr, true, &it->second, nullptr, true) in src/validation.cpp

2026-07-08 04:32:39

optout21 force-pushed on Jun 20, 2026

DrahtBot added the label CI failed on Jun 20, 2026

DrahtBot commented at 7:12 AM on June 20, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/27862735029/job/82461396003 LLM reason (✨ experimental): CI failed because clang-tidy reported readability-const-return-type errors in src/validation.cpp (e.g., static const BlockValidationState ... return types), causing the clang-tidy step to exit with failure.

<details><summary>Hints</summary>

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.

</details>

optout21 force-pushed on Jun 20, 2026

DrahtBot removed the label CI failed on Jun 20, 2026

optout21 commented at 1:30 PM on June 20, 2026: contributor

While working on this PR, one instance was identified where the invariant return_value == state.IsValid() was not guaranteed: At the end of ChainstateManager::AcceptBlock(), FlushStateToDisk() was called, and its return value discarded, but it could have a side effect in state. In turn, this could make a difference in ChainstateManager::LoadExternalBlockFile().

Options to resolve this:

A. Since the return value is ignored, ignore the state returned as well. B. Handle the error from FlushStateToDisk(), and pass the error.

The ignoring of flush result (A.) is also proposed in #29700 (cc: @ryanofsky ).

The minor behavior-change could be also omitted from this PR, limiting strictly to a no-behavior-change refactor:

Do not change AcceptBlock, keep the dual value & state return values
Restrict the scope of the PR and drop all commits 5-13
Defer this PR until this issue is solved separately first

optout21 force-pushed on Jun 20, 2026

w0xlt commented at 10:36 PM on June 20, 2026: contributor

Concept ACK

optout21 marked this as ready for review on Jun 21, 2026

purpleKarrot commented at 5:59 AM on June 22, 2026: contributor

Concept ACK. Preferring the return value over output parameters is a useful and I would say necessary improment.

But could we avoid using BlockValidationState as a state machine, or mutable local variable? That means, instead of code like:

{
  BlockValidationState state;
  if (...) {
    state.Invalid(...);
  }
  return state;
}

I would prefer something like:

{
  if (...) {
    return BlockValidationState::Invalid(...);
  }
  return BlockValidationState::Valid();
}

optout21 commented at 6:16 AM on June 22, 2026: contributor

could we avoid using BlockValidationState as a state machine, or mutable local variable

I fully agree, shorter-scoped variables mean less chance of potential interplay between calls, less complexity. I did change this in several places, but not everywhere. In some places there was a real chance for interaction between subsequent calls (e.g. error reason is set conditional of already set value), and I kept it to be safe (w.r.t. behavior changes). But I should review and change it more aggressively.

optout21 force-pushed on Jun 22, 2026

optout21 commented at 4:40 PM on June 22, 2026: contributor

Applied some improvements:

Got rid of method-wide BlockValidationState state variables, use only restricted scope state variables, optimally in "if (const auto state = Xxx; !state.IsValid())" construct. Thanks @purpleKarrot for the emphasis!
Added InvalidState static helper to create invalid BlockValidationState instance. In error branches, instead of 3 statements (declaration, setting, and return) now one is enough (return&construction). Added as new first commit.

DrahtBot added the label CI failed on Jun 22, 2026

DrahtBot removed the label CI failed on Jun 22, 2026

in src/validation.h:1272 in 8207ac1bb7 outdated

1266 | @@ -1268,11 +1267,12 @@ class ChainstateManager
1267 |       *
1268 |       * @param[in]  headers The block headers themselves
1269 |       * @param[in]  min_pow_checked  True if proof-of-work anti-DoS checks have been done by caller for headers chain
1270 | -     * @param[out] state This may be set to an Error state if any error occurred processing them
1271 |       * @param[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers
1272 | -     * @returns false if AcceptBlockHeader fails on any of the headers, true otherwise (including if headers were already known)
1273 | +     * @returns BlockValidationState indicating the result. IsValid() returns true if all headers
1274 | +     *          were accepted. On failure, IsInvalid() is false and the state contains the specific

arejula27 commented at 4:36 PM on June 23, 2026:

On failure, IsInvalid() is false Would not be "true"?

arejula27 commented at 6:11 PM on June 23, 2026: none

concept ACK 8207ac1bb79f76c9005e8e2d695b725539b3c5c0

Looks good overall. I'd push this one step further and use util::Expected here, it's a natural fit and follows the PR's own motivation. The PR already cleans up the bool + state out-param inconsistency by making state the return value; util::Expected<T, BlockValidationState> does the same thing but at the type level instead of by convention. Success and failure become distinct alternatives of the type.

Concretely, a bare BlockValidationState still carries all three modes (M_VALID/M_INVALID/M_ERROR) in the return value even though M_VALID is now redundant with "the call succeeded". With util::Expected that mode collapses into has_value() and is no longer stored, the failure modes move to the error channel, and out-params like ppindex fold into the success channel. util/result.h also explicitly steers low-level functions toward util::Expected.

// pindex folded into the success channel
[[nodiscard]] util::Expected<CBlockIndex*, BlockValidationError> AcceptBlockHeader(
    const CBlockHeader& block, bool min_pow_checked);

auto res = AcceptBlockHeader(header, /*min_pow_checked=*/true);
if (!res) {
    if (res.error().IsInvalid()) MaybePunishNodeForBlock(...); // peer's fault
    return util::Unexpected{std::move(res).error()};
}
CBlockIndex* pindex = res.value(); // only reachable on success

One open question (happy to leave it as discussion): where does M_INVALID fit best? Above I put it in the error channel next to M_ERROR, but it's arguably a successful, expected outcome of validation, the function did run and produced a verdict ("this block is invalid"), whereas M_ERROR is a genuine runtime failure that prevented producing a result. So an alternative split would keep Valid/Invalid in the value channel and reserve the error channel for runtime errors only. I don't have a strong preference.

stringintech commented at 8:59 PM on June 23, 2026: contributor

Concept ACK

One open question (happy to leave it as discussion): where does M_INVALID fit best? Above I put it in the error channel next to M_ERROR, but it's arguably a successful, expected outcome of validation, the function did run and produced a verdict ("this block is invalid"), whereas M_ERROR is a genuine runtime failure that prevented producing a result. So an alternative split would keep Valid/Invalid in the value channel and reserve the error channel for runtime errors only. I don't have a strong preference.

There is a thread in #33856 where this was also brought up. I included a POC in the last comment of that thread which keeps the value channel for M_VALID/M_INVALID and the error channel for M_ERROR, which I think makes more sense as you're suggesting: M_INVALID is not an operation failure.

In general, I think stripping the runtime failure from BlockValidationState could also leave the door open for preferring exceptions over error values in the future. That said, this should be orthogonal to this PR and could be addressed in a follow-up if desired.

arejula27 commented at 10:36 AM on June 24, 2026: none

which I think makes more sense as you're suggesting: M_INVALID is not an operation failure.

I think both approaches are ok, though I slightly prefer util::Expected<CBlockIndex*, BlockValidationError>, an invalid block is an error in the same way a hardware failure causes validation to fail. That said, I understand the distinction behind util::Expected<BlockValidationState, std::string>, where the value represents logical state (valid or not) and the error is something unrelated to the function's logic. I'd accept either.

That said, this should be orthogonal to this PR and could be addressed in a follow-up if desired.

I completely disagree that this is orthogonal to the PR. This is a fundamentally different refactor, it differs not just internals but the API and its callers in a critical section of the code. I don't think it's correct to keep returning state through an value of the BlockValidationState when we already have util::Expected in the codebase for exactly this purpose.

On top of that, I don't think it's a good idea to keep touching this section of the code without a clear long-term direction changes that are purely stylistic or for maintainability, only to refactor everything again later when util::Expected gets introduced. This PR is already proposing a structural change to error handling here, it should go all the way rather than leave a half-finished migration that will need to be revisited.

stringintech commented at 11:03 AM on June 24, 2026: contributor

I completely disagree that this is orthogonal to the PR. This is a fundamentally different refactor, it differs not just internals but the API and its callers in a critical section of the code. I don't think it's correct to keep returning state through an value of the BlockValidationState when we already have util::Expected in the codebase for exactly this purpose.

The way I see it, the main thing we achieve with the current refactor is the ability to reason about things more locally, by not having to pass state around and absorb it across a chain of operations. This seems like a separate objective from making BlockValidationState no longer absorb runtime errors or changing it to BlockValidationError. Even if reviewers would prefer seeing both goals addressed in a single batch/PR, I think it would be valuable to have a kind of separation between them through distinct commits.

arejula27 commented at 2:15 PM on June 24, 2026: none

The way I see it, the main thing we achieve with the current refactor is the ability to reason about things more locally, by not having to pass state around and absorb it across a chain of operations

Agree, this is a relevant topic alone to be merged, and my concerns should not be a blocker, however i feel that would be the correct way to implement this and fits inside the PR. I will continue the discussion when more changes or reviews are submitted to not flood the thread

optout21 commented at 2:38 PM on June 25, 2026: contributor

@arejula27: very valuable suggestion using util::Expected, I will explore this idea. The current PR doesn't change the content of the BlockValidationState returned, only how it is returned, there is no behavior change (except the special case 9965489e243143fdbba6e8f379e598c92368d45c), the changes in the commits are relatively localized scope. Nonetheless, I will explore the util::Expected suggestion, and apply it, or not (in that case I will mention it as an alternative considered).

Another idea related to this: some methods return only Valid or Invalid, but not Error -- I've documented this in the header comments in a few places for more visibility. This is by convention, and not enforced. Maybe this could be enforced (maybe by the return value used).

ryanofsky commented at 9:24 PM on June 28, 2026: contributor

re: #35570 (comment)

While working on this PR, one instance was identified where the invariant return_value == state.IsValid() was not guaranteed

Really nice catch. I think it would make sense to open a separate PR to fix this, so this PR can just be a refactoring and not change behavior. One approach could be: 78100ea27d076e9c76f444364dfd8fbaf719fefc

optout21 commented at 1:11 PM on June 29, 2026: contributor

Really nice catch. I think it would make sense to open a separate PR to fix this

Thanks, I'll do this. Thanks for the prepared commit as well.

optout21 commented at 9:48 PM on June 29, 2026: contributor

Update:

The single minor behavior-relevant change has been split off into #35621.
The commit from there is included in this PR as first commit (rebased on top of #35621); this PR should come only after #35621.

optout21 force-pushed on Jun 29, 2026

yuvicc commented at 4:49 PM on June 30, 2026: contributor

Concept ACK

yuvicc commented at 5:13 AM on July 3, 2026: contributor

Following up on the discussion above about util::Expected being the long term refactor, I've opened an RFC #35646 which might be the direction where this could end.

in src/validation.cpp:3883 in 014f2774b9

3879 | @@ -3877,18 +3880,18 @@ static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state)
3880 |   * Note: If the witness commitment is expected (i.e. `expect_witness_commitment
3881 |   * = true`), then the block is required to have at least one transaction and the
3882 |   * first transaction needs to have at least one input. */
3883 | -static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment, BlockValidationState& state)
3884 | +static BlockValidationState CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment)

arejula27 commented at 11:33 PM on July 4, 2026:

Missing [[nodiscard]], unlike the other converted functions (CheckBlockHeader, CheckMerkleRoot, ContextualCheckBlockHeader)

optout21 commented at 4:54 AM on July 6, 2026:

I haven't deemed nodiscard important on all compile-unit-internal methods, but I've added now for consistency.

in src/validation.cpp:4150 in 014f2774b9

4146 | @@ -4136,7 +4147,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
4147 |   *  in ConnectBlock().
4148 |   *  Note that -reindex-chainstate skips the validation that happens here!
4149 |   */
4150 | -static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const ChainstateManager& chainman, const CBlockIndex* pindexPrev)
4151 | +static BlockValidationState ContextualCheckBlock(const CBlock& block, const ChainstateManager& chainman, const CBlockIndex* pindexPrev)

arejula27 commented at 11:34 PM on July 4, 2026:

Same here: missing [[nodiscard]] for consistency with the other converted functions

optout21 commented at 4:55 AM on July 6, 2026:

Added (as above)

in src/kernel/bitcoinkernel.cpp:1348 in 014f2774b9 outdated

1347 |  
1348 | -        auto state = btck_BlockValidationState::create();
1349 | -        bool result{chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true, btck_BlockValidationState::get(state))};
1350 | -        assert(result == btck_BlockValidationState::get(state).IsValid());
1351 | -        return state;
1352 | +        return btck_BlockValidationState::create(state);

arejula27 commented at 11:35 PM on July 4, 2026:

nit: state is dead after this line, so create(std::move(state)) avoids copy-constructing the state.

optout21 commented at 5:04 AM on July 6, 2026:

Done

in src/bench/checkblock.cpp:47 in 014f2774b9 outdated

  43 | @@ -44,9 +44,8 @@ static void CheckBlockTest(benchmark::Bench& bench)
  44 |              assert(block.vtx.size() == 1557);
  45 |          })
  46 |          .run([&] {
  47 | -            BlockValidationState validationState;
  48 | -            const bool checked{CheckBlock(block, validationState, chain_params->GetConsensus())};
  49 | -            assert(checked);
  50 | +            BlockValidationState validationState{CheckBlock(block, chain_params->GetConsensus())};

arejula27 commented at 11:36 PM on July 4, 2026:

nit: could be const (only IsValid() is called), like the fuzz/block.cpp temporaries this PR already made const.

optout21 commented at 5:04 AM on July 6, 2026:

Done

in src/bench/duplicate_inputs.cpp:73 in 014f2774b9

  69 | @@ -70,8 +70,8 @@ static void DuplicateInputs(benchmark::Bench& bench)
  70 |      block.hashMerkleRoot = BlockMerkleRoot(block);
  71 |  
  72 |      bench.run([&] {
  73 | -        BlockValidationState cvstate{};
  74 | -        assert(!CheckBlock(block, cvstate, chainparams.GetConsensus(), false, false));
  75 | +        BlockValidationState cvstate{CheckBlock(block, chainparams.GetConsensus(), false, false)};

arejula27 commented at 11:36 PM on July 4, 2026:

nit: could be const (only const methods are called on it).

optout21 commented at 5:04 AM on July 6, 2026:

Done

in src/test/validation_block_tests.cpp:388 in 014f2774b9 outdated

 381 | @@ -383,4 +382,10 @@ BOOST_AUTO_TEST_CASE(witness_commitment_index)
 382 |  
 383 |      BOOST_CHECK_EQUAL(GetWitnessCommitmentIndex(pblock), 2);
 384 |  }
 385 | +
 386 | +BOOST_AUTO_TEST_CASE(test_empty_process_new_block_headers)
 387 | +{
 388 | +    auto res = m_node.chainman->ProcessNewBlockHeaders({}, true);

arejula27 commented at 11:37 PM on July 4, 2026:

nit: could be const auto res{...}

optout21 commented at 5:05 AM on July 6, 2026:

Done

in src/validation.cpp:4563 in 014f2774b9 outdated

4563 |  
4564 | -    // Ensure no check returned successfully while also setting an invalid state.
4565 | -    if (!state.IsValid()) NONFATAL_UNREACHABLE();
4566 | -
4567 | -    return state;
4568 | +    return BlockValidationState{};

arejula27 commented at 11:39 PM on July 4, 2026:

I miss a comment here about the case.

I would like to know the reason to drop the previous (if (!state.IsValid()) NONFATAL_UNREACHABLE(); return state;), so a hypothetical ConnectBlock that returns true while setting an invalid state would now be reported as Valid.

Intentional as part of the simplification? Is this case not possible anymore?

optout21 commented at 4:44 AM on July 6, 2026:

Indeed. Such checks ensured that the bool and state return values are in-sync, and they became pointless in most places. However, here ConnectBlock still return both bool and state, so the check makes sense. I've put it back in the else branch.

arejula27 commented at 11:42 PM on July 4, 2026: none

re-concept-ACK 014f2774b9 Second pass focused on correctness and the new signatures, nothing significant to flag

DrahtBot requested review from purpleKarrot on Jul 4, 2026

optout21 force-pushed on Jul 6, 2026

optout21 commented at 5:07 AM on July 6, 2026: contributor

Applied some (minor) changes following review from @arejula27 -- thanks!

optout21 force-pushed on Jul 6, 2026

DrahtBot added the label CI failed on Jul 6, 2026

DrahtBot removed the label CI failed on Jul 6, 2026

fanquake referenced this in commit bab0120053 on Jul 6, 2026

Add factory method to BlockValidationState for easier construction

Add `InvalidState` static helper to create invalid `BlockValidationState` instance,
for easier construction in error branches. Instead of the 3 statements of
declaration, setting, and return with the exisiting non-static method, now
return&construction is possible in one statement.

a2b8fa38d4

Refactor CheckBlockHeader signature

Change the (internal) method `CheckBlockHeader` to return the validation
result in the return value instead of an output parameter.

530093dbd5

Refactor ContextualCheckBlockHeader signature

Change the (internal) method `ContextualCheckBlockHeader` to return the validation
result in the return value instead of an output parameter.

ac88aadd0c

Refactor AcceptBlockHeader signature

Change the method `AcceptBlockHeader` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.

8a1da9cec1

Change ProcessNewBlockHeaders

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

Remove redundant int return from btck_chainstate_manager_process_block_header.
Previously returned both an int result and an output validation state parameter, creating ambiguity
where non-zero could mean either invalid header or processing failure. Since ProcessNewBlockHeaders already provides complete validation info, the int return was redundant.

Co-authored-by: stringintech <stringintech@gmail.com>
Co-authored-by: stickies-v <stickies-v@protonmail.com>

edc8d6abd4

Refactor CheckMerkleRoot signature

Change the (internal) method `CheckMerkleRoot` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.

3f05c6f85a

Refactor CheckBlock signature

Change the method `CheckBlock` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.

67212e80c2

Refactor CheckWitnessMalleation signature

Change the (internal) method `CheckWitnessMalleation` to return the
`BlockValidationState` validation result in the return value
instead of an output parameter.

bd9c03062d

Refactor ContextualCheckBlock signature

Change the (internal) method `ContextualCheckBlock` to return the
`BlockValidationState` validation result in the return value
instead of an output parameter.

a4e4a7c147

Refactor FatalError signature

Change the method `FatalError` to return the constructed
`BlockValidationState` object in a return value instead of
an output parameter.

9d692ad86e

Refactor FlushStateToDisk signature

Change the method `FlushStateToDisk` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.

442c5d0247

Refactor AcceptBlock signature

Change the method `AcceptBlock` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.

bcfb696dbd

Internal simplification in TestBlockValidity 6e5a0392d2

optout21 commented at 4:31 AM on July 8, 2026: contributor

Rebased, following the inclusion of #35621 (first commit dropped, as already in master).

optout21 force-pushed on Jul 8, 2026