RFC: Separate out runtime errors from BlockValidationState using `util::Expected` #35646

BlockValidationState currently carries two unrelated kinds of failure: consensus/policy invalidity which is the actual purpose and runtime errors via a M_ERROR mode in ModeState enum inside ValidationState class. This PR removes M_ERROR and routes runtime errors through util::Expected<T, std::string> instead, making the two failure modes distinct.

Motivation

BlockValidationState exists to describe why a block or transaction is invalid. It holds a consensus/policy reject reason. A disk write or system runtime error is not that, yet it was folded into the same object as a third state i.e. M_ERROR.

This conflation has two costs:

• Wrong abstraction for non-validating functions. FlushStateToDisk, DisconnectTip, ActivateBestChain, PreciousBlock, and InvalidateBlock do no block validation, but each takes a BlockValidationState out-param to report any runtime error.

• Three outcomes in one object. Functions that can both validate and hit a runtime error (ConnectBlock, AcceptBlock, ProcessNewBlock) holds valid, invalid, and fatal into a single BlockValidationState, forcing every caller to unwrap.

As noted in the original discussion here:

Functions like FlushStateToDisk, ActivateBestChain aren't exactly about validating a certain block, yet they take a BlockValidationState& out-param just to absorb runtime errors, which feels like the wrong abstraction.

And also discussion here to remove M_ERROR value.

• This would also pave a long term fix for #35570, which returns BlockValidationState from validation methods instead of boolean value.

util::Expected is a good option for this reason, it keeps the error handling in the same return value style (system/runtime errors and ValidationState for consensus correctness) and separates-out runtime errors from ValidationState.

Runtime errors could be signaled either by a return value or by throwing. This PR keeps them as return values as it's a minimal change. M_ERROR was already a return-value mechanism included in ValidationState with consensus verdict. This PR doesn't open the exceptions vs returns question, it keeps the existing return-value style and just moves the fatal error into a proper Expected channel.

<!--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/35646.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy 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:

• #35675 (mining: add block template manager by ismaelsadeeq)
• #35581 (node: add block template manager and track waitNext fee inflow by ismaelsadeeq)
• #35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
• #35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
• #35307 (blockstorage: keep snapshot base in normal blockfile range by shuv-amp)
• #35295 (validation: fetch block input prevouts in parallel during ConnectBlock by andrewtoth)
• #34729 (Reduce log noise by ajtowns)
• #34672 (mining: add reason/debug to submitSolution and unify with submitBlock by w0xlt)
• #34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
• #33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
• #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):

• 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
• ProcessNewBlock(shared_pblock, true, true, nullptr) in src/test/util/setup_common.cpp
• ProcessNewBlock(block, true, true, &new_block) in src/test/util/mining.cpp
• ProcessNewBlock(std::make_shared<CBlock>(Params().GenesisBlock()), true, true, &ignored) in src/test/validation_block_tests.cpp
• ProcessNewBlock(block, true, true, &ignored) in src/test/validation_block_tests.cpp

^{2026-07-06 16:26:32}

Rebased on master to resolve conflicts with #35621. Adopted its "ignore the flush error" behavior in the new util::Expected API by dropping the flush-error propagation from AcceptBlock.

Hmm, it could make sense to be more type-safe here, but looking at the code, there are places that flatten this back down to a boolean, so I wonder what the overall benefit is?

I think it could make sense to think whether any places that flatten this down again to a boolean need a different handling? If yes, fixing that handling should probably be done early in a pull request changing the behavior.

Moreover, I presume all of the runtime-errors are fatal, so I presume they must all call the fatal error function. Maybe this can be enforced at compile-time, so that all those fatal errors ensure that the fatal error function is called exactly once?

Concept ACK

The advantages of this change are:

• enforcement of returned errors -- some methods can return only runtime errors;
• reduced risk of accidental mix-up of validation and runtime errors.

At first it looked a bit strange that some errors (runtime errors) are treated as exceptional cases, while validation errors are treated as return values, as previously both were treated the same. However, this makes sense; validation status can be regarded as the non-exceptional output of the checker algotihms, while runtime errors are proper exceptional errors.

Some minor observations:

• Maybe the big change could be broken up (e.g. in two, first the void return value changes, then the BlockValidationState changes)
• CheckBlock, CheckBlockHeader could be also be changed to the return-state pattern
• [[nodiscard]] could be added touched signatures, to reduce risk of ignored errors.
• In some places where a local state variable is used, its scope could be reduced, or omitted entirely. Preferably error from a call should be handled right away, there is no need for method-wide state/error variable (left comments in a few places).

Hmm, it could make sense to be more type-safe here, but looking at the code, there are places that flatten this back down to a boolean, so I wonder what the overall benefit is?

You're right that the top-level callers collapses the result back to yes/no, wouldn't that be the right place for it to collapse? e.g. ConnectBlock -> ConnectTip -> ActivateBestChainStep -> ActivateBestChain, where M_ERROR used to be inside ValidationState next to consensus verdict and every caller had to unwrap to distinguish b/w consensus failure v/s fatal error.

I think it could make sense to think whether any places that flatten this down again to a boolean need a different handling?

I've checked, every fatal error already fires at the origin(FatalError()), so by the time it reaches the top-level caller, the node might be shutting down regardless of what the caller does with the string, none of the callers branch on fatal-vs-non-fatal; they just surface the message. If other reviewers also feels the same that the caller needs to programmatically tell the two apart, we could have a distinct type for fatal and non-fatal type.

Moreover, I presume all of the runtime-errors are fatal, so I presume they must all call the fatal error function. Maybe this can be enforced at compile-time, so that all those fatal errors ensure that the fatal error function is called exactly once?

This is mostly true, except for invalidateblock/reconsiderblock rpc whose error returns are non-fatal and fire nothing. I think we can get a compile-time guarantee for the functions where every error is fatal, by constructing FatalError like below?

<details>

class FatalError
{
public:
    //! Fire the fatalError notification.
    [[nodiscard]] static util::Unexpected<FatalError> Raise(
        kernel::Notifications& notifications, const bilingual_str& message);

    //! Untranslated message, for logging or surfacing through an RPC error.
    const std::string& message() const LIFETIMEBOUND { return m_message; }

private:
    explicit FatalError(std::string message) : m_message{std::move(message)} {}
    std::string m_message;
};

util::Unexpected<FatalError> FatalError::Raise(Notifications& notifications, const bilingual_str& message)
{
    notifications.fatalError(message);                 // fire (→ AbortNode → shutdown)
    return util::Unexpected{FatalError{message.original}};  // then mint the value
}

</details>

Hmm, it could make sense to be more type-safe here, but looking at the code, there are places that flatten this back down to a boolean, so I wonder what the overall benefit is?

You're right that the top-level callers collapses the result back to yes/no, wouldn't that be the right place for it to collapse?

Yeah, I am mostly wondering aloud. Because currently, your patch will map both to RPC_INTERNAL_ERROR in GenerateBlock. However, in generateblock, they are mapped to RPC_INTERNAL_ERROR or RPC_VERIFY_ERROR. And in submitblock, they are mapped to a new imaginary/undocumented? "validation-error" string?

That doesn't seem consistent or worthwhile. I'd say:

• Either we can distinguish the two cases clearly, in which case a separate type and handling makes sense (and they shouldn't be flattened down).
• Or, we can not distinguish them, in which case splitting them up and then flattening them down again seems pointless?

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.