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, nervana21, enirox001
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:

#36353 (kernel: don't reuse cached CheckBlock results across params by FlashWayne)
#36326 (kernel: Use typed errors for fatal and flush error notifications by arejula27)
#36318 (validation: Ensure Invalid ValidationState has result by optout21)
#36109 (test: harden arbitrary-parent block creation by l0rinc)
#36066 (validation: Separate check-only version of ConnectBlock by optout21)
#35906 (First steps towards a stateless, side-effect free validation library by purpleKarrot)
#35820 (refactor: keep duration calculations typed by l0rinc)
#35793 (Implement BIP 54 (Consensus Cleanup) without mainnet activation by darosior)
#35751 (validation: use parallel input prevout fetching in TestBlockValidity by andrewtoth)
#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)
#35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)

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
AcceptBlock(new_block, &new_block_index, true, nullptr, nullptr, true) in src/test/baseindex_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-09-19 05:15:48

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"?

optout21 commented at 7:11 AM on September 8, 2026:

Good catch. On failure IsValid() is false, moreover IsInvalid() is true. I state: "On failure, IsValid() is false and the state contains the specific validation failure reason. Never returns Error state.". From the last bit it follows that IsInvalid() is true, but I don't mention it here, only IsValid(), as that should be used to check for normal vs. exceptional case.

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

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: contributor

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: contributor

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: contributor

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

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

nervana21 commented at 4:49 PM on July 9, 2026: contributor

Concept ACK

DrahtBot added the label Needs rebase on Jul 10, 2026

Kino1994 referenced this in commit cd0cbdb870 on Jul 10, 2026

optout21 commented at 7:07 AM on July 12, 2026: contributor

Rebased, post #34897 (straightforward resolution).

optout21 force-pushed on Jul 12, 2026

DrahtBot removed the label Needs rebase on Jul 12, 2026

DrahtBot added the label Needs rebase on Aug 14, 2026

optout21 force-pushed on Aug 18, 2026

optout21 commented at 11:03 AM on August 18, 2026: contributor

Rebased to current master. Only tests affected: some removed tests, some straightforward resolutions. git range-diff e600c4ce022bf1e856cb81ca941310fe177d7eb6...f65361acfae8d5d0d9f525fe7887334f03cedcc3

DrahtBot removed the label Needs rebase on Aug 18, 2026

Kino1994 referenced this in commit f91e18f193 on Aug 19, 2026

in src/validation.cpp:3848 in 6c438cde68 outdated

3848 | -
3849 | -    return true;
3850 | +    if (fCheckPOW && !CheckProofOfWork(block.GetHash(), block.nBits, consensusParams)) {
3851 | +        return BlockValidationState::InvalidState(BlockValidationResult::BLOCK_INVALID_HEADER, "high-hash", "proof of work failed");
3852 | +    }
3853 | +    return BlockValidationState{};

arejula27 commented at 1:40 PM on September 5, 2026:

I see the default constructor gets a valid state from

enum class ModeState {
    M_VALID,   //!< everything ok
    M_INVALID, //!< network rule violation (DoS value may be set)
    M_ERROR,   //!< run-time error
} m_mode{ModeState::M_VALID};   // <-- in-class initializer

however it would be better for the reader if you also added a factory for the valid state (alongside InvalidState(...)), so the returned state is explicit rather than implicit and independent of the default value of ModeState

optout21 commented at 9:47 AM on September 8, 2026:

I think it's intuitive for the state class to initialize to Valid by default. Adding a factory method for the Valid would be an overkill in my view. It's true that BlockValidationState and ValidationState don't have an explicit constructor, but it's rather clear that the default constructor initializes to M_VALID. I have added a note to ValidationState to clarify, but not changing anything else.

arejula27 commented at 4:03 PM on September 10, 2026:

agree

in src/validation.cpp:2151 in b04810f793

2148 |  {
2149 |      notifications.fatalError(message);
2150 | -    return state.Error(message.original);
2151 | +    BlockValidationState state;
2152 | +    (void)state.Error(message.original);
2153 | +    return state;

arejula27 commented at 2:23 PM on September 5, 2026:

I only found one other manualError(...)construction, but might still be cleaner to have a factory here too, e.g. BlockValidationState::ErrorState(reject_reason). Then this becomes:

    notifications.fatalError(message);
    return BlockValidationState::ErrorState(message.original);

optout21 commented at 9:49 AM on September 8, 2026:

Suggestion taken, factory method for Error added. I've also found a second construction instance (in AcceptBlock), both use the factory method now. Added you as co-author.

in src/validation.cpp:4336 in 73e20f87de

4336 | -    CheckBlockIndex();
4337 | -
4338 | -    if (!state.IsValid())
4339 | -        return false;
4340 | +    {
4341 | +        BlockValidationState accept_state{AcceptBlockHeader(block, &pindex, min_pow_checked)};

arejula27 commented at 2:44 PM on September 5, 2026:

nit: I would prefer header_state, not a fan of using verbs in variable names

        BlockValidationState header_state{AcceptBlockHeader(block, &pindex, min_pow_checked)};

optout21 commented at 9:49 AM on September 8, 2026:

Taken.

in src/consensus/validation.h:134 in 4a3a1a4eb0

 130 | @@ -131,7 +131,17 @@ class ValidationState
 131 |  };
 132 |  
 133 |  class TxValidationState : public ValidationState<TxValidationResult> {};
 134 | -class BlockValidationState : public ValidationState<BlockValidationResult> {};
 135 | +class BlockValidationState : public ValidationState<BlockValidationResult> {

arejula27 commented at 2:51 PM on September 5, 2026:

nit: could you put this brace a line below, to keep a consistent format with the file (see class ValidationState right above)?

optout21 commented at 9:49 AM on September 8, 2026:

Done.

arejula27 commented at 3:14 PM on September 5, 2026: contributor

Ack f65361acfae8d5d0d9f525fe7887334f03cedcc3

This PR only changes signatures as the PR description says, no behavioral changes.

Wrote some small comments, none of them are blockers, just suggestions, but I would highly encourage at least fixing the comment/doc error I mentioned in my last review (validation.h:1280, "On failure, IsInvalid() is false" should read "true").

As I mentioned in the inline comments, I think adding both factories would help readers understand the code and make it cleaner.

Also wrote two small tests (arejula27/bitcoin@67a3be1) that guard how each case is reported (valid/invalid/error). Feel free to cherry-pick, copy, edit or omit.

optout21 commented at 9:52 AM on September 8, 2026: contributor

Thanks for the reviews, @arejula27! Changes applied -- all minor:

Fix error in documentation of ProcessNewBlockHeaders.
ProcessNewBlockHeaders: add assert to check that the non-valid case is invalid (and not error).
AcceptBlockHeader: add asserts to check that non-valid cases are all invalid (and not error).
Minor local variable rename in AcceptBlock
Minor formatting change (src/consensus/validation.h)
Add ErrorState factory method to the BlockValidationState error class. Added as a separate commit.

git range-diff f65361acfae8d5d0d9f525fe7887334f03cedcc3..58d6d23275067dc284ca13f3d7906ac9aaf7843a

optout21 force-pushed on Sep 8, 2026

optout21 requested review from arejula27 on Sep 8, 2026

arejula27 commented at 4:03 PM on September 10, 2026: contributor

reACK 58d6d23275067dc284ca13f3d7906ac9aaf7843a

optout21 commented at 10:12 PM on September 10, 2026: contributor

re-ack 58d6d23275067dc284ca13f3d7906ac9aaf7843a

Thanks, but I think you need the form "reACK" exactly, to be picked up by the review bot. Could you edit it (or re-comment)?

arejula27 commented at 10:19 PM on September 10, 2026: contributor

Sorry, first time doing it😅

in src/test/fuzz/block.cpp:37 in 58d6d23275 outdated

  36 | +    BlockValidationState validation_state_pow_and_merkle{CheckBlock(block, consensus_params, /* fCheckPOW= */ true, /* fCheckMerkleRoot= */ true)};
  37 |      assert(validation_state_pow_and_merkle.IsValid() || validation_state_pow_and_merkle.IsInvalid() || validation_state_pow_and_merkle.IsError());
  38 |      (void)validation_state_pow_and_merkle.Error("");
  39 | -    BlockValidationState validation_state_pow;
  40 | -    const bool valid_incl_pow = CheckBlock(block, validation_state_pow, consensus_params, /* fCheckPOW= */ true, /* fCheckMerkleRoot= */ false);
  41 | +    const BlockValidationState validation_state_pow{CheckBlock(block, consensus_params, /* fCheckPOW= */ true, /* fCheckMerkleRoot= */ false)};

w0xlt commented at 1:07 AM on September 11, 2026:

I am trying to understand this. Calling validation_state_pow_and_merkle.Error("") changes the state to Error, making the later if (validation_state_pow_and_merkle.IsValid()) always false so it never runs.

Separately, the IsValid() || IsInvalid() || IsError() assertions appear redundant, even before the refactor: a default-constructed state was Valid, so these assertions would pass even if CheckBlock() never updated it.

optout21 commented at 7:20 AM on September 11, 2026:

Pre-change, CheckBlock returned a bool and status in the parameter. The fuzz test saved the bools, and used them for checks at the end. Since there is no bool return any more, those became unavailable, and status.IsValid() is used instead.

You are right, that the setting of validation_state_pow_and_merkle to error interferes with this! I'm not sure of the intention of that line, but it seems it didn't do anything meaningful, as that variable was not accessed afterwards. It may contribute to test coverage though. So my resulotion is to move the .Error() call to the end.

Applying now, as this is a (minor) bug to the fuzz test. Thanks for the find!

DrahtBot requested review from w0xlt on Sep 11, 2026

optout21 commented at 7:22 AM on September 11, 2026: contributor

Applied fix to a minor fuzz test bug, found in review (thanks @w0xlt).

git range-diff 58d6d23275067dc284ca13f3d7906ac9aaf7843a...981912489d89d40a2f54b58b9e035906e5cb4737

optout21 force-pushed on Sep 11, 2026

in src/consensus/validation.h:141 in 981912489d

 137 | +class BlockValidationState : public ValidationState<BlockValidationResult>
 138 | +{
 139 | +public:
 140 | +    //! Factory helper method to create an Invalid BlockValidationState
 141 | +    static BlockValidationState InvalidState(BlockValidationResult result,
 142 | +        const std::string& reject_reason = "", const std::string& debug_message = "")

hodlinator commented at 9:48 AM on September 15, 2026:

nit: Could use default ctor initialization to avoid any risk of having to seek through the string literals for NUL term.

        const std::string& reject_reason = {}, const std::string& debug_message = {})

optout21 commented at 12:44 PM on September 16, 2026:

Taken.

hodlinator commented at 6:25 PM on September 16, 2026:

thread #35570 (review):

Please also fix MakeError(const std::string& reject_reason = "").

optout21 commented at 12:57 PM on September 18, 2026:

Sorry, the fell through the cracks; done.

in src/consensus/validation.h:143 in 981912489d outdated

 139 | +public:
 140 | +    //! Factory helper method to create an Invalid BlockValidationState
 141 | +    static BlockValidationState InvalidState(BlockValidationResult result,
 142 | +        const std::string& reject_reason = "", const std::string& debug_message = "")
 143 | +    {
 144 | +        BlockValidationState state;

hodlinator commented at 9:53 AM on September 15, 2026:

nit: Could add precondition:

        Assume(result != BlockValidationResult::BLOCK_RESULT_UNSET);

optout21 commented at 12:44 PM on September 16, 2026:

Accepted.

arejula27 commented at 2:21 PM on September 18, 2026:

One question, how do you decide when to use assume and when assert?

optout21 commented at 4:51 AM on September 19, 2026:

@hodlinator: This is a rather bold Assume. It could be made even bolder, if placed inside ValidationState::Invalid()! :D The CI failure caught one violation of this: #31981 introduced a usage where BlockValidationState is used, but the result is discarded, and Unset is used. My fix is to leave out MakeInvalid() in that case, leave the direct ValidationState::Invalid() call there. (Note: I'm not sure why this failure occurs in only some CI runs, but that's not too important.)

hodlinator commented at 11:40 AM on September 22, 2026:

Yeah, see my older comment #35570 (review), I thought this Assume was obviously correct but for some reason we have a call-site which didn't want to specify its own enum value.

optout21 commented at 2:49 PM on September 22, 2026:

I overlooked that you also found this usage. This is a rather localized usage. Before #31981, the inconclusive-not-best-prevblk check was outside TestBlockValidity, and only the string was used. It was moved into TestBlockValidity, so now it is returned in the reject_reason of the state, but since only reject_reason is used, the reason is left empty. It could use a new BLOCK_INCONCLUSIVE value. I may tackle this separately.

optout21 commented at 3:04 PM on September 22, 2026:

One question, how do you decide when to use assume and when assert? @arejula27 , there are no clear-cut rules. assert stops the program immediately, so it should be used only in cases which should never happen. My guideline: use if its truthfulness can be established by static code analysis of the near-scope code, and excludes further potential error cases. E.g. if previous code makes it impossible that a pointer is null, and the pointer is dereferenced later, it makes sense to assert that it cannot be null. Assert is similar to assert, but it produces logs. Assume is the least dangerous, it produces logs, but it creates as exception only, not abort.

hodlinator commented at 7:05 PM on September 22, 2026:

It could use a new BLOCK_INCONCLUSIVE value. I may tackle this separately.

Sounds worth exploring!

optout21 commented at 11:55 AM on September 23, 2026:

--> break-out PR created: #36318

in src/consensus/validation.h:140 in 981912489d

 132 | @@ -130,7 +133,25 @@ class ValidationState
 133 |  };
 134 |  
 135 |  class TxValidationState : public ValidationState<TxValidationResult> {};
 136 | -class BlockValidationState : public ValidationState<BlockValidationResult> {};
 137 | +class BlockValidationState : public ValidationState<BlockValidationResult>
 138 | +{
 139 | +public:
 140 | +    //! Factory helper method to create an Invalid BlockValidationState
 141 | +    static BlockValidationState InvalidState(BlockValidationResult result,

hodlinator commented at 9:54 AM on September 15, 2026:

nits: So much "state" in BlockValidationState::InvalidState(... how about:

    static BlockValidationState MakeInvalid(BlockValidationResult result,

or alternatively make it a free function outside of BlockValidationState as we don't need access to private parts:

[[nodiscard]] inline BlockValidationState InvalidBlockValidationState(BlockValidationResult result,
    const std::string& reject_reason = {}, const std::string& debug_message = {})
{

(+ make it nodiscard).

optout21 commented at 12:43 PM on September 16, 2026:

Accepted.

sed -i "s/InvalidState/MakeInvalid/g" src/validation.cpp src/validation.h
sed -i "s/ErrorState/MakeError/g" src/validation.cpp src/validation.h

in src/validation.cpp:3941 in 0f6e59cd81

3937 | @@ -3938,7 +3938,8 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
3938 |  
3939 |      // Check that the header is valid (particularly PoW).  This is mostly
3940 |      // redundant with the call in AcceptBlockHeader.
3941 | -    if (!CheckBlockHeader(block, state, consensusParams, fCheckPOW))
3942 | +    state = CheckBlockHeader(block, consensusParams, fCheckPOW);

hodlinator commented at 10:00 AM on September 15, 2026:

0f6e59c Refactor CheckBlockHeader signature:

We want to make sure that we are not overriding an invalid/error state with a valid state with this refactor. Could add this before overwriting the incoming state parameter:

    Assume(state.IsValid());

The CI tests and reviewers running the ancestor commits can then help verify that the behavior doesn't change.

Most of them could then hopefully be removed in later commits where we further reduce the scopes of state-variables.

Same in AcceptBlockHeader() in this commit, and same pattern in later commits.

optout21 commented at 12:40 PM on September 16, 2026:

This point becomes irrelevant in a later commit, when CheckBlock itself doesn't get an input state, and the state returned from CheckBlockHeader call is placed in a new local variable, so no overwriting can happen.

    // Check that the header is valid (particularly PoW).  This is mostly
    // redundant with the call in AcceptBlockHeader.
    if (const auto state = CheckBlockHeader(block, consensusParams, fCheckPOW); !state.IsValid())
        return state;

hodlinator commented at 7:37 PM on September 16, 2026:

thread #35570 (review):

I know many checks away in later commits but still think it's good practice to add them in intermediate commits when you are changing behavior of such critical code.

At the very least the "test ancestor commits" CI job can churn through them.

You are changing the local code behavior from maybe changing state when passed as an argument, to definitely stomping it with a return value.

I went through and added Assumes in my suggestions branch and many of them survive until the end, although I admit some are overly paranoid.

optout21 commented at 2:01 PM on September 18, 2026:

You are right, in the previous form with input-output parameters method had the chance to overwrite the status, but did so only in error case. I've added the proposed Assume's in every place where we overwrite the status always. Some of these Assume's disappear in later commits, but not all.

in src/validation.cpp:4232 in 981912489d outdated

4231 | +            return BlockValidationState{};
4232 |          }
4233 |  
4234 | -        if (!CheckBlockHeader(block, state, GetConsensus())) {
4235 | +        if (const auto state = CheckBlockHeader(block, GetConsensus()); !state.IsValid()) {
4236 | +            assert(state.IsInvalid());

hodlinator commented at 11:01 AM on September 15, 2026:

remark in 86104e9d74d92cd0c4b72aa0983177c9fcf665e0 "Refactor AcceptBlockHeader signature":

I understand that these asserts can be added since no paths currently return error. Hope these wires won't be tripped outside of tests in the future. Not sure whether these types of documentation asserts should be used in such critical code when the conditions they catch don't obviously cause problems later on during execution (in contrast to checking a pointer for non-nullness before de-referencing).

It would feel safer if we made the paths which do not return error states instead return a narrower state type which only allowed valid/invalid, instead of having these more imperative checks.

Maybe their additions could be broken out into a commit at the end, or be deferred to a later PR, keeping this more strictly a refactoring PR. Similar changes are done in 4ed127368759983972278265320c278ef3014230 "Change ProcessNewBlockHeaders", which in its title admits it's not a pure refactoring commit.

optout21 commented at 1:47 PM on September 16, 2026:

I understand the doubts about using potentially catastrophic assert's. However, if an invariant can be verified by static code analysis of a small amount of code, assert are useful for not only documenting, but enforcing invariants. I agree that narrower return values are superior solution in this case, but I did not see an easy way to ensure that. The concern about code being changed later is always a valid worry, I agree. I would prefer these methods to have a single return at the end, and assert just before the single return. However, with early-exit returns style that's not practical.

In this PR, the following 4 methods can return Valid or Invalid, but never Error:

CheckBlockHeader: Internal method, and trivial to verify the code: it has 3 lines with 2 returns, and none of them is Error.
ContextualCheckBlockHeader: Internal method, more complicated, around 8 returns, one {} and the rest all MakeInvalid
AcceptBlockHeader: This is a ~~public~~ private method, and its return are Invalid or Valid, except 2 cases, where it calls into the above two methods. If those return non-valid, that must be invalid, and this is asserted and returned. This way it can be verified by looking only at this method that it cannot return Error.
ProcessNewBlockHeaders: Public method. Returns valid, or non-valid from AcceptBlockHeader, which, by the above, must be Invalid. This is asserted, so that the invariant can be verified by looking only at this method. It is only this last method which is called from outside, from net_processing.cpp, and there is is Assume'd that it doesn't return Error.

Moving these asserts to a separate commit is tempting, but in fact it's only cosmetics. I've also considered using less dangerous Assume. Or leaving them out altogether.

Based on the above explanation I keep as it is for now. Those asserts are needed to ensure the invariant added to the documentation ("Never returns Error state").

hodlinator commented at 8:04 PM on September 16, 2026:

thread #35570 (review):

Following the call tree on the current PR base is good to do, but not the best we can do. Adding asserts and now comments about current behavior feels like duct tape to me.

As the PRs currently stand, I much prefer #35646 be merged before this PR so invariants are enforced at compile time through types rather than by the duct tape that is runtime failures and comments. That makes it much harder for silent merge conflicts and the like to cause issues.

in src/consensus/validation.h:148 in 981912489d

 144 | +        BlockValidationState state;
 145 | +        (void)state.Invalid(result, reject_reason, debug_message);
 146 | +        return state;
 147 | +    }
 148 | +    //! Factory helper method to create an Error BlockValidationState
 149 | +    static BlockValidationState ErrorState(const std::string& reject_reason = "")

hodlinator commented at 12:10 PM on September 15, 2026:

nit: Would prefer this were introduced together with InvalidState() and then used in the later refactoring commits for FatalError() and AcceptBlock().

optout21 commented at 12:29 PM on September 16, 2026:

It is done (in later commit?).

hodlinator commented at 7:31 PM on September 16, 2026:

thread #35570 (review):

What I meant is to add both MakeInvalid() and MakeError() helpers in the first commit. Then in the "Refactor FatalError signature" and "Refactor AcceptBlock signature" commits you can switch directly to MakeError() instead of the intermediate hop through state.Error() ... return state which those commits currently do.

optout21 commented at 1:04 PM on September 18, 2026:

Taken. Through reordering, the double touching of FatalError and AcceptBlock can be avoided.

in src/bench/checkblock.cpp:48 in 981912489d

  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 | +            const BlockValidationState validationState{CheckBlock(block, chain_params->GetConsensus())};
  51 | +            assert(validationState.IsValid());

hodlinator commented at 12:27 PM on September 15, 2026:

nanonit: Could reduce camelCase in touched lines as developer-notes.md enforces snake_case.

            const BlockValidationState state{CheckBlock(block, chain_params->GetConsensus())};
            assert(state.IsValid());

optout21 commented at 12:28 PM on September 16, 2026:

Taken.

in src/bench/duplicate_inputs.cpp:74 in 981912489d

  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 | +        const BlockValidationState cvstate{CheckBlock(block, chainparams.GetConsensus(), false, false)};
  76 | +        assert(!cvstate.IsValid());

hodlinator commented at 12:30 PM on September 15, 2026:

nanonit: Even stronger assertion:

        assert(cvstate.IsInvalid());

optout21 commented at 12:25 PM on September 16, 2026:

Technically the check only checked for false return value, that corresponds to !IsValid(). This is a tightening of the checks, but the duplicate inputs case should result in invalid (not error), so it is safe to change. Accepted.

hodlinator commented at 1:18 PM on September 15, 2026: contributor

Reviewed 981912489d89d40a2f54b58b9e035906e5cb4737

I get the impression the alternative #35646 returns narrower types, which makes the code more correct by construction rather than adding assert(state.IsInvalid). Only just discovered that PR though, so will try to review it next, before coming back to this one.

optout21 commented at 12:22 PM on September 16, 2026: contributor

I get the impression the alternative #35646 returns narrower types, which makes the code more correct by construction rather than adding assert(state.IsInvalid). Only just discovered that PR though, so will try to review it next, before coming back to this one.

Thanks for the review! #35646 and this PR overlap, but not completely, both have their own merits. PR 35646 was raised later, and I like it, I ack'ed it. I haven't closed this PR, as it has some improvements not present there. In case 35646 is merged first, I will update this PR. Please have a look at #35646 too.

Clarification (Sept 17): I concept-acked #35646, not proper-ack, but I made a mistake, and it showed up as proper-ack (https://github.com/bitcoin/bitcoin/pull/35646#issuecomment-5712874848).

optout21 commented at 1:59 PM on September 16, 2026: contributor

Made some (minor) changes, following latest review (@hodlinator).

git range-diff 981912489d89d40a2f54b58b9e035906e5cb4737...312c4e78faa5dc775bb33351ea4dda8a9e18b50a

optout21 force-pushed on Sep 16, 2026

DrahtBot added the label CI failed on Sep 16, 2026

DrahtBot commented at 3:24 PM on September 16, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/35105544002/job/104825688520 LLM reason (✨ experimental): CI failed because IWYU reported include fixes were needed and intentionally exited non-zero (“Failure generated from IWYU”).

<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 commented at 4:50 PM on September 16, 2026: contributor

Fixed IWYU CI failure (missing include for new Assume). Additionally added comments to CheckBlockHeader and ContextualCheckBlockHeader (that they cannot return Error), and also fixed an outdated comment in src/net_processing.cpp (AcceptBlockHeader/ProcessNewBlockHeaders).

optout21 force-pushed on Sep 16, 2026

DrahtBot removed the label CI failed on Sep 16, 2026

in src/kernel/bitcoinkernel.cpp:1203 in a92cd68a34

hodlinator commented at 7:15 PM on September 16, 2026:

This can be dropped in the "Refactor CheckBlock signature" commit so we only assign state once.

hodlinator commented at 7:35 AM on September 17, 2026:

470b305 Internal simplification in TestBlockValidity:

This state should be removed if we keep this commit where we call CheckBlock() etc which return it's own instances instead of overwriting state. That means we also refactor the next if-block:

    if (block.hashPrevBlock != *Assert(tip->phashBlock)) {
        return BlockValidationState::MakeInvalid({}, "inconclusive-not-best-prevblk");
    }

That also necessitates re-introducing state before calling ConnectBlock(), unless that method is also refactored to return BlockValidationState:

    {
        BlockValidationState state;
        // Set fJustCheck to true in order to update, and not clear, validation caches.
        if (!chainstate.ConnectBlock(block, state, &index_dummy, view_dummy, /*fJustCheck=*/true)) {
            if (state.IsValid()) NONFATAL_UNREACHABLE();
            return state;
        } else {
            if (!state.IsValid()) NONFATAL_UNREACHABLE();
        }
    }

hodlinator commented at 2:19 PM on September 17, 2026:

Realized BlockValidationState::MakeInvalid({}, ... triggers the Assume(result != BlockValidationResult::BLOCK_RESULT_UNSET), so might be best to skip adding the latter for now.

optout21 commented at 12:59 PM on September 18, 2026:

Indeed, MakeInvalid can be used, and then there is no need for the state variable, only further down. Done.

hodlinator commented at 7:55 AM on September 17, 2026: contributor

Reviewed a92cd68a3433b2cdae9c944345d8d20417c5a46f

The way this PR transforms pre-existing functions one at a time is something I wish #35646 did as well.

Agree that it complements #35646, but think it should go in after that one (see inline comment), so I'll defer my Concept A-C-K until something like that lands. Please don't see my reservations as disinterest, I spend time on it because I agree the general direction is worthwhile.

Branch incorporating my suggestions into existing commits: https://github.com/hodlinator/bitcoin/tree/pr/35570_suggestions

optout21 force-pushed on Sep 18, 2026

optout21 commented at 2:05 PM on September 18, 2026: contributor

New round of changes following new review comments.

Add factory method to BlockValidationState for easier construction

Add `MakeInvalid` 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.

0ef8aa8968

Refactor CheckBlockHeader signature

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

a956c44905

Refactor ContextualCheckBlockHeader signature

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

c74b01f33c

Refactor AcceptBlockHeader signature

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

dc592c8205

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>

4f95dced43

Refactor CheckMerkleRoot signature

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

d5733120c4

optout21 commented at 2:59 PM on September 18, 2026: contributor

Rebased ot master, hoping to fix the mining_template_verification.py failure in CI (could not reproduce locally).

optout21 force-pushed on Sep 18, 2026

DrahtBot added the label CI failed on Sep 18, 2026

optout21 commented at 3:48 PM on September 18, 2026: contributor

Test failure is due to this if in TestBlockValidity:

    if (block.hashPrevBlock != *Assert(tip->phashBlock)) {
        return BlockValidationState::MakeInvalid({}, "inconclusive-not-best-prevblk");
    }

which creates an Invalid state with result == BlockValidationResult::BLOCK_RESULT_UNSET, which violates a newly added Assume.

optout21 commented at 4:54 AM on September 19, 2026: contributor

Small change to fix CI failure. CI caught a violation of the newly introduced Assume: #31981 introduced a usage of BlockValidationState where the result is discarded, and Unset result is used.

git range-diff 6294fb57f8f8db077d22ec294633e7e78762d5b1...0e8dc6b9dd7f07e756dcfe0b8146cc2daa17008e

optout21 force-pushed on Sep 19, 2026

Refactor CheckBlock signature

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

04271ff1b8

Refactor CheckWitnessMalleation signature

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

7e2a87a345

Refactor ContextualCheckBlock signature

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

a244c30780

Refactor FatalError signature

Change the method `FatalError` to return the constructed `BlockValidationState`
object in a return value instead of an output parameter.
Add a helper `MakeError` factory method to the `BlockValidationState`, similar to `MakeInvalid`.

Co-authored-by: Íñigo Aréjula Aísa <arejula27@gmail.com>

ab51a328fe

Refactor FlushStateToDisk signature

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

5e2d798b28

Refactor AcceptBlock signature

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

Co-authored-by: Íñigo Aréjula Aísa <arejula27@gmail.com>

9e3c5ba481

optout21 force-pushed on Sep 19, 2026

DrahtBot removed the label CI failed on Sep 19, 2026

enirox001 commented at 10:54 AM on September 22, 2026: contributor

Concept ACK https://github.com/bitcoin/bitcoin/pull/35570/changes/5e2d798b28d55fe3786ea23efc33f5fbf4f4fba0

Thanks for working on this, Initial looks at the validation.cpp methods use of the BlockValidationState was a bit ambiguous to understand, this makes things much clearer since the validation states can be used to replace the boolean values being returned.

I have a question, I can see in validation.cpp that there are some methods that have been skipped such as ConnectBlock and ConnectTip these seem to be good prospective targets for this specific refactor?

optout21 commented at 2:54 PM on September 22, 2026: contributor

I have a question, I can see in validation.cpp that there are some methods that have been skipped such as ConnectBlock and ConnectTip these seem to be good prospective targets for this specific refactor?

Yes, the idea can be applied on those equally, I have left them out (or for later?) to limit the scope. Note that #35646 (somewhat related) also tackles those.

in src/consensus/validation.h:140 in 0ef8aa8968

 132 | @@ -130,7 +133,19 @@ class ValidationState
 133 |  };
 134 |  
 135 |  class TxValidationState : public ValidationState<TxValidationResult> {};
 136 | -class BlockValidationState : public ValidationState<BlockValidationResult> {};
 137 | +class BlockValidationState : public ValidationState<BlockValidationResult>
 138 | +{
 139 | +public:
 140 | +    //! Factory helper method to create an Invalid BlockValidationState
 141 | +    [[nodiscard]] static inline BlockValidationState MakeInvalid(BlockValidationResult result,

optout21 commented at 11:07 AM on September 23, 2026:

0ef8aa8 Add factory method to BlockValidationState for easier construction:

(Documenting as self-comment). I've realized that the factory method could be added to the template base class ValidationState instead of the specialized/derived class. BlockValidationState would be simpler, the factory would be available for TxValidationState as well, at the price that BlockValidationState would need a constructor from ValidationState<BlockValidationResult>. However, as the factory would not be useful for TX validation as of now, this wouldn't make much sense.