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:

• #36326 (kernel: Use typed errors for fatal and flush error notifications by arejula27)
• #36318 (validation: Ensure Invalid ValidationState has result by optout21)
• #36066 (validation: Separate check-only version of ConnectBlock by optout21)
• #36000 (validation: prefetch blocks while connecting by l0rinc)
• #35751 (validation: use parallel input prevout fetching in TestBlockValidity by andrewtoth)
• #35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
• #35524 (validation: Avoid rewriting the genesis block during index recovery by winterrdog)
• #35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
• #35307 (blockstorage: keep snapshot base in normal blockfile range by shuv-amp)
• #34729 (Reduce log noise by ajtowns)
• #34254 (validation: Prevent duplicate logging and looping in invalid block handling by mzumsande)
• #33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
• #32554 (bench: replace embedded raw block with configurable block generator by l0rinc)

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

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?

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?

Thanks, I agree. I went with the first option. Fatal/system failures are now represented separately as kernel::FatalError and propagated via util::Expected, while BlockValidationState now only represents valid vs. invalid consensus validation.

Also, the existing RPC behavior is preserved.

Rebased to master.

Rebased on master

Ready for review.

Concept ACK 4b183f4e077eb1e7d48cf5d53036c8591abf9478

EDIT (Sept 17): Edited the above line, inserted a space between concept and a, to show as intended.

Reviewed again, generally looks good. I've left some comments; most are rather minor, but there are some that should be checked. Therefore not upgrading my Concept A to a proper acknowledgement as of now. This PR is related to #35570 (of mine), and my assessment is that both PR's stand on their own, but, despite an overlap, they also complement each other.

I uphold my comment from last review:

• I would be happy to see the second large commit broken up (possible ways: first the void return value changes, then the BlockValidationState changes; first internal validation method changes, then external ones; etc.)

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task macOS-cross to arm64: https://github.com/bitcoin/bitcoin/actions/runs/35190448674/job/105101625071} _{LLM reason (✨ experimental): CI failed due to a C++ build error: Chainstate().InvalidateBlock is declared to take a single CBlockIndex* argument, but wallet_tests.cpp calls it with two arguments.}

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

Concept ACK the earlier push 4b183f4e077eb1e7d48cf5d53036c8591abf9478

Extracting the error-part of BlockValidationState by using util::Expected is a promising direction.

As the PRs stand today, something like this should be merged before #35570 is considered.

This PR would make review easier if broken up into more commits so that the transforms are applied to one function at a time.

See my suggestions branch: https://github.com/hodlinator/bitcoin/tree/pr/35646_piecemeal

not upgrading my Concept A to a proper acknowledgement @DrahtBot note: As I've noticed during the discussion with @hodlinator , my previous concept A was interpreted as a plain ack. I think this is because there was no space between the "Concept" and "A", and there was a following commit ID. This was against my intention. I edit it to reflect this (insert a space). I should have noticed this, my mistake. Screenshots follow.

<img width="1358" height="254" alt="image" src="https://github.com/user-attachments/assets/b2d5ee14-58c8-4b31-8942-28bc4de0ec67" /> <img width="1302" height="240" alt="image" src="https://github.com/user-attachments/assets/2ef85424-994f-46d8-a03c-30489fc5b821" />

Sure, fixed in https://github.com/maflcko/DrahtBot/commit/7554887cadb7c8fb614a278feef14ae399d125ce. You can edit it back, if you want.

Sure, fixed in maflcko/DrahtBot@7554887.

That was fast, thanks! 🙏

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/35315445149/job/105651757101} _{LLM reason (✨ experimental): CTest failed because the miner_tests test suite failed (ctest returned non-zero exit status 8).}

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

Thanks for the review @hodlinator and @optout21, I have split the commits more for easier review and addressed other comments:

• Added the move-only kernel::FatalError type and co-located it with Notifications.

• Changed FatalError::Raise() to take ownership of the translated message and move the original string into the error.

• Renamed WithMessage() to ReplaceMessage().

• Converted block storage, flushing, block connection, block acceptance, chain activation, invalidation, precious-block handling, and block submission to propagate fatal failures through util::Expected.

• Changed FindUndoPos() to return FlatFilePos directly instead of using an output parameter.

• Simplified util::Expected checks with value_or(false) where appropriate.

• Preserved explicit validation-state checks where the returned BlockValidationState remains meaningful, including submitheader.

• Removed the obsolete validation-state error mode and the kernel INTERNAL_ERROR mode.

• Kept BlockChecked for ordinary validation results and handled fatal errors through the direct return value and fatalError notification.

• Addressed the behavior change where fatal AcceptBlock failures no longer emit BlockChecked, peer block-source cleanup now happens directly when ProcessNewBlock() returns a fatal error.

• Other nits and typos.

Reviewed 22af5819cd2d29df7a937909d6dd11f1f8abe2b2, looks good! Left some comments.

Might take a while until I'm able to do a full review but I see you've incorporated my feedback from #35646#pullrequestreview-5210572199, thanks!

Thanks for review @optout21. Addressed comments:

• Included the FatalError message in the FindUndoPos and FindNextBlockPos failure logs. Restored the assert(!res->IsNull()) that was unintentionally dropped in the AcceptBlock commit.
• Return {} instead of state on paths where the state is always valid.
• the MaybeUpdateMempoolForReorg(...) line is now untouched and keeps ret.value_or(false).
• dropped the now-redundant IsInvalid() check before InvalidBlockFound() in ConnectTip()

I agree with the full motivation but disagree (NACK) conceptually with using util::Expected/std::expected.

std::expected-style error handling trades runtime efficiency for runtime deterministic performance, which is a valid trade-off for systems with hard-realtime-requirements. I explained it on multiple occasions that for Bitcoin, runtime efficiency is more important than runtime deterministic performance, because we need IBD to be as fast as possible and new blocks are added on an extremely irregular cadence (approximately ten minutes on average is about as far away from a hard realtime system as you can possibly imagine). Hence, for Bitcoin, throwing exceptions is the more appropriate mechanism for error handling.

I explained it on multiple occasions that for Bitcoin, runtime efficiency is more important than runtime deterministic performance

And you received pushback every single time (e.g. #36101): exceptions are hard-to-predict invisible control flow, which is why many newer languages are moving away from them and towards explicit, value-based error handling to minimize surprises. You're presenting your opinion as facts here, ignoring the project's specific needs and history.

because we need IBD to be as fast as possible

I'm glad you're worried about this, so are we, it's why we've been working on this for years, we still have many open PRs optimizing this, you can help by reviewing them. That's also a good way to familiarize yourself with the project.

Hence, for Bitcoin, throwing exceptions is the more appropriate mechanism for error handling.

No, in mission-critical applications exceptions are often forbidden for good reason: they're hard to predict in exceptional (pun intended) circumstances.

@purpleKarrot I watched the talk you recommended by Khalil Estell. My impression was that [Ee]xpected performs poorly primarily when the involved types are large/complex. Types being able to be moved like the proposed FatalError probably helps.

I think sacrificing a tiny bit of efficiency in exchange for making error handling explicit at each level is the prudent approach in this part of the code.

std::expected-style error handling trades runtime efficiency for runtime deterministic performance, which is a valid trade-off for systems with hard-realtime-requirements.

Thanks @purpleKarrot. I agree that IBD throughput matters and that exceptions might provide a cheaper success path. However, deterministic execution isn’t the reason for choosing util::Expected here. Its other benefit is making the distinction between a validation result and an operational failure explicit in the function’s interface. That is also part of the motivation for std::expected.

Apart from that there is also existing control flow to preserve. For example, ActivateBestChainStep() updates the mempool after a failed connection or disconnection before propagating the failure. An exception based implementation could preserve this too, but would require deliberately arranging cleanup and catch boundaries, including at the kernel API. Keeping explicit propagation makes that separation a more easier to review change. Also opened a PR in benchcoin to check the IBD performance.

Rebased onto master (e8e7e91a11) and resolved conflicts with #35675.

making the distinction between a validation result and an operational failure explicit

My point is that this distinction is not explicit enough when both are communicated over the same channel.

When block validation returns a success value and throws on exception, client code will look like this:

auto res = validate(block);
if (res) {
  // block is valid
} else {
  // block is invalid
}

Operational failure will take neither of the two branches.

But when validate returns an expected object, operator bool will just tell whether a fatal error occurred. If user code uses conditions like res && res->IsValid(), it is very dangerous to have an else branch, because it may lead to the same situation that caused the 2013 chain split.

And furthermore: If functions returning expected are not marked noexcept, all clients now have to deal with two error handling mechanisms. But if the function is marked noexcept, it will terminate when an actual exception is thrown.

I agree with the full motivation but disagree (NACK) conceptually with using util::Expected/std::expected.

Contrarian views have the potential to advance things. I've considered your point of view from different sides (including discussions in #36101). My personal conclusion is that in this particular instance (and probably in many others as well) returns are preferable over exceptions, because the latter is more brittle and error prone in face of future code changes. I say this considering the criticality of the validation code, and also the losly organized nature of the project. Your objection did raise my awareness of the cost aspects of passing objects as return values (structs, Expected, string fields, movability, etc.), but I dismiss your performance degradation point as minor in comparison with the risks of future mishandled errors.

Triggered by the return-vs-throw discussion (@purpleKarrot), I've done some microbenchmarking of the different error styles. Styles compared:

• 0 baseline: always return void, no state
• 1 return bool, with BlockValidationState as parameter
• 1.a out-only: set the state on success too (state = {})
• 1.b in-out: do not touch the state on success (as in the codebase)
• 2 return BlockValidationState
• 3 return util::Expected<BlockValidationState, kernel::FatalError>
• 4 return BlockValidationState (valid or invalid), throw on fatal error
• 5 return util::Expected<void, std::variant<BlockValidationState, kernel::FatalError>>

For 1&2 it is assumed that state can represent Error as well (not the case in this PR any more, but was before)

The benchmark does 50M iterations of a method call, which itself calls into other methods, with branch-out factor of 2 for 2 levels (1+2+4 = 7 total calls).

Every 1000th call returns invalid, every 1000th (not the same ones) return Error, the rest of the calls (998) return success.

The benchmarks is available at: https://github.com/optout21/bitcoin/blob/error-style-bench/src/bench/validation_state_error_handling.cpp

Sample results below.

Some conclusions:

• 1.b is the fastest, when the state parameter is not touched at all in the success branch. However, this is not very clean, as state from different methods can get mixed or overwritten, and we aim to make this cleaner (this PR, #35570)
• util::Expected is generally not slower than out-param style. In fact in some earlier iteration it was even faster than the plain return (analysis found that the differences are due to the specifics of compiler branch optimizations).
• return+throw is the slowest, due to the overhead of exceptions. With no exceptions, it's the same as the plain return.
• Generally differences are very small, especially compared to time for the real work of the methods. Differences are on the micro-optimization level. A lot depends on how the state variable is set, in what order it is checked, etc.
• An interesting idea out of this is that we could to optimize for the frequent success case, and do not use a BlockValidationState object in success case at all. This can be achieved e.g. by instead of:`

util::Expected<BlockValidationState, kernel::FatalError>

using:

util::Expected<void, util::Expected<BlockValidationState, kernel::FatalError>> or util::Expected<void, std::variant<BlockValidationState, kernel::FatalError>>

This way the success case is optimized, and this is confirmed by the benchmark. @yuvicc, give it a thought. However, I think it's a micro-optimization not worth doing.

<details> <summary> Sample benchmark results </summary>

|             ns/call |              call/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|                8.23 |      121,479,880.24 |    0.1% |      4.53 | `ErrorHandlingBaselineVoid`
|                8.37 |      119,486,068.62 |    0.0% |      4.60 | `ErrorHandlingBoolInOutState`
|               16.11 |       62,068,002.98 |    0.2% |      8.87 | `ErrorHandlingBoolOutState`
|               14.93 |       66,989,455.90 |    0.1% |      8.27 | `ErrorHandlingReturnExpectedState`
|               14.43 |       69,293,814.61 |    0.0% |      7.94 | `ErrorHandlingReturnState`
|               21.06 |       47,483,573.19 |    0.1% |     11.59 | `ErrorHandlingReturnStateThrow`
|               10.52 |       95,014,644.68 |    0.0% |      5.80 | `ErrorHandlingReturnVoidExpected`

</details>

Concept ACK

I like the util::Expected direction, as I also asked for it in [my review on #35570](/bitcoin-bitcoin/35570/#pullrequestreview-4555262677), so consider this a supporting vote for it. I do not like exceptions, and this error handling is much easier to understand and follow.

Since this PR and my own #36326 ("kernel: Use typed errors for fatal and flush error notifications") overlap, there are a couple of things I would like to mention.

Adding a new type to the kernel here feels from my pov out of scope for this PR, separate from what it sets out to fix (the tri-state in BlockValidationState), since that is already achievable with plain strings. Adding a type is an improvement on top of that, not a requirement for it, and has more implications than what has been already discussed in this conversation: it pulls more core-owned dependencies into the kernel, which I think goes against the general direction of the libbitcoinkernel project, fixes the shape of the errors inside the kernel and also broadens kernel maintenance surface. The kernel is consensus-critical code, so the less responsibility it carries, the easier it is to maintain. Those feel like implications that deserve their own discussion. I have been exploring how to improve the error type and remove dependencies from the kernel for a few months now, until finally proposing #36326. My focus was extracting bilingual_str and improving the kernel API, unrelated to this PR's behavior, but our work ended up colliding.

I think the two ideas are not in conflict, and can actually be combined: the typed kernel::FatalError variant from #36326 could be the payload util::Expected carries here instead of a wrapped-string. Notifying and building the message are already two separate steps in #36326: fatalError() fires right at the origin with the typed data, no string needed at that point, and the translated message gets built afterwards, downstream, on the node side. So swapping the payload would not delay or otherwise change when the fatal notification fires, only what it carries. Worth discussing which PR keeps which part before either one merges, since right now both define kernel::FatalError with the same name, different responsibilities, and incompatible shapes.

Thanks @optout21 for putting this together! I reproduced it locally ( with standalone at -O2, pinned core, GCC 14.2 / Clang 18.1). Results in ns per top-level call (7 nested calls):

<details> <summary> benchmark results </summary> | Style | GCC | Clang | |-----------------------------|------|-------| | 0 baseline | 10.4 | 10.6 | | 1a out-param | 24.9 | 29.2 | | 1b in-out | 11.9 | 11.5 | | 2 return state | 24.0 | 23.6 | | 3 Expected<BVS, FatalError> | 26.4 | 32.4 | | 4 return + throw | 33.8 | 39.7 | | 5 Expected<void, variant> | 16.7 | 20.4 |

</details>

The ordering matches yours one. The main cost is building a BlockValidationState (two std::strings) on the success path, not the return mechanism. Agree that style no. 5 isn't worth it now. Moving Invalid into the error channel alongside FatalError would also blur the separation this PR aims for.

Agree that style no. 5 isn't worth it now. Moving Invalid into the error channel alongside FatalError would also blur the separation this PR aims for.

Thanks for looking into it! I think we are in agreement. The result return is not the hot-path of the chain validation, over-optimization doesn't worth it.