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

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.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35570.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35569 (Making CTransaction a Regular Type by purpleKarrot)
• #35557 (kernel, validation: Add btck_chainstate_manager_set_clock_time by ryanofsky)
• #34897 (indexes: Don't commit ahead of the flushed chainstate by mzumsande)
• #34895 (fuzz: Fuzzing harnesses for ActivateBestChainStep and ActivateBestChain by RobinDavid)
• #33854 (fix assumevalid is ignored during reindex by Eunovo)
• #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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

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
• 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-06-20 13:31:13}

<!--85328a0da195eb286784d51f73fa0af9-->

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

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

Concept ACK