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

#36171 (blockstorage: Defer error notifications to callers by w0xlt)
#36066 (validation: Separate check-only version of ConnectBlock by optout21)
#35906 (First steps towards a stateless, side-effect free validation library by purpleKarrot)
#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)
#33854 (fix assumevalid is ignored during reindex by Eunovo)
#30342 (kernel, logging: Pass Logger instances to kernel objects 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.

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(pblockone, &pindex, true, nullptr, &newblock, true) in src/test/validation_chainstate_tests.cpp
AcceptBlock(pblock, nullptr, true, dbp, nullptr, true) in src/validation.cpp

2026-09-08 09:53:26

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

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.

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

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.

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.

712dc38c12

Refactor CheckBlockHeader signature

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

0f6e59cd81

Refactor ContextualCheckBlockHeader signature

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

4757f129a1

Refactor AcceptBlockHeader signature

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

86104e9d74

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>

4ed1273687

Refactor CheckMerkleRoot signature

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

e16a5ccdfd

Refactor CheckBlock signature

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

02d38874f7

Refactor CheckWitnessMalleation signature

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

7a4467d3ee

Refactor ContextualCheckBlock signature

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

fca3032372

Refactor FatalError signature

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

5aeac89cb6

Refactor FlushStateToDisk signature

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

cf9b7d80e8

Refactor AcceptBlock signature

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

73eb0af050

Internal simplification in TestBlockValidity 9bde06f820

Add Error factory method to BlockValidationState

Add an `ErrorState` factory method to the `BlockValidationState` error class, similar to `InvalidState`.
Use it throughout validation.cpp, where Error state is created (two instances).

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

58d6d23275

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