#35646 RFC: Separate out runtime errors from BlockValidationState using `util::Expected`
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #35646 RFC: Separate out runtime errors from BlockValidationState using `util::Expected` | Waiting on author | 0 -1 | Blocked | 342 + 233 tests | ||||||
|
|
|
|
| ||||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | ||||||||||
Goal
Removes the `M_ERROR` runtime error state from `ValidationState` and decouples system runtime failures from consensus or policy validity. Functions across validation and block storage (including `ConnectBlock`, `AcceptBlock`, `FlushStateToDisk`, `ActivateBestChain`, and `ProcessNewBlock`) are refactored to return `util::Expected<T, kernel::FatalError>`. Corresponding internal error enumerations are also removed from the libbitcoinkernel C API and C++ wrapper.
Problem: `BlockValidationState` conflates block validity with unexpected system failures like disk write or memory errors. This forces non-validating functions such as `FlushStateToDisk`, `DisconnectTip`, and `InvalidateBlock` to take a `BlockValidationState` out-parameter solely for error signaling, while validating routines force callers to disentangle three separate outcomes (valid, invalid, or fatal error) from one object.
Evidence of importance:
- PR description notes discussions in #33856 and #35570 calling to separate `M_ERROR` from `ValidationState`.
- PR description notes this refactoring paves the way for #35570 to return `BlockValidationState` cleanly from validation methods instead of boolean flags.
Scope: A wide-reaching validation and kernel refactor touching 40 files, replacing `BlockValidationState` out-parameters across chainstate functions with `util::Expected<T, kernel::FatalError>` returns and modifying the libbitcoinkernel C API.
Risk of a mistake: medium (validation, consensus). A mistake in propagating fatal error returns across the block connection and activation pipeline could improperly classify hardware or I/O failures as consensus invalidity, or vice versa, causing unexpected node behavior or crashes.
Changed since the previous assessment: Rebased without fundamental architectural changes; maintains the introduction of `kernel::FatalError` and `util::Expected` across validation and block manager interfaces.
Reviewability
Paused · Waiting on author
Open objections from alexanderwiederin, arejula27, optout21 with no author reply for 11 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: address bugs and behavior changes raised in optout21's review 2026-10-21
- decision: settle long-term fatal error mechanism between Expected and exceptions 2026-10-12
Agreement
Blocked
objection that needs a change, nobody has replied (optout21)
Thread notes: Added review feedback from optout21 pointing out potential fallthrough bugs, missing error states, notification behavior changes, and cleanup suggestions.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| maflcko (member) | objection, interface | Separating errors causes inconsistent RPC error mapping and redundant complexity if errors are immediately flattened back down to booleans. | fixed · fix pushed · settled by introduction of kernel::FatalError type | answered | yes | 2026-07-08: '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' Status from: 2026-07-16: yuvicc: 'Fatal/system failures are now represented separately as kernel::FatalError and propagated via util::Expected, while Bloc' |
| optout21 (contributor) | objection, correctness | May alter miner error handling where error cases previously mapped to inconclusive. | contested · optout21 pushed back that error strings still replace inconclusive reasons | answered | yes | 2026-09-08: 'It's not clear to me that this does not introduce new error reasons. I have the impression that the Error case previously resulted in "inconclusive" (but I'm not sure). If that's the case, this needs ' Status from: 2026-09-22: optout21: 'error strings still replace inconclusive reasons' |
| optout21 (contributor) | objection, correctness | Omitting the BlockChecked callback in error paths changes validation notification behavior for interface consumers. | contested · optout21 pushed back asking to unresolve as the behavior change still applies | answered | yes | 2026-09-08: 'I think there is a behavior change here in case of Error: previously `BlockChecked` was called, and now it isn't.' Status from: 2026-09-22: optout21: 'asking to unresolve as the behavior change still applies' |
| purpleKarrot (member) | objection, approach | Using util::Expected trades runtime efficiency for deterministic performance, slowing down IBD compared to throwing exceptions. | contested · purpleKarrot pushed back that returning expected does not make the distinction explicit enough | changed | yes | 2026-09-23: 'I agree with the full motivation but disagree (NACK) conceptually with using `util::Expected`/`std::expected`.' Status from: 2026-09-25: purpleKarrot: 'My point is a bit stronger though: that property currently holds because reviewers checked each site, not because the in' |
| purpleKarrot (member) | objection, safety | Callers testing util::Expected with else branches may conflate fatal operational failure with block invalidity, risking a chain split. | contested · purpleKarrot pushed back reiterating risks of result-style error handling and 2013 chain split | changed | by others | 2026-09-25: '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.' Status from: 2026-10-09: purpleKarrot: 'I have documented the risks of result-style error handling ... and made a direct connection to the 2013 chain split.' |
| purpleKarrot (member) | objection, interface | Expected-returning functions force callers to handle both expected errors and exceptions unless marked noexcept, which would terminate on unexpected exceptions. | answered · optout21 replied defending return-value error handling over exceptions | changed | by others | 2026-09-25: 'If functions returning expected are not marked noexcept, all clients now have to deal with two error handling mechanisms.' Status from: 2026-09-25: optout21: 'returns are preferable over exceptions, because the latter is more brittle and error prone in face of future code change' |
| optout21 (contributor) | suggestion, interface | Consider simplifying ModeState to a bool now that it is only binary. | open | answered | no | 2026-09-25: 'Now that the mode is only binary, maybe consider simplifying to a `bool`?' |
| arejula27 (contributor) | objection, scope | Adding a new FatalError type to the kernel is out of scope and broadens kernel maintenance surface by pulling core-owned dependencies into consensus-critical code. | open | answered | no | 2026-09-26: '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 ' |
| arejula27 (contributor) | suggestion, approach | Coordinate with #36326 on which PR introduces kernel::FatalError and consider using its typed variant as the util::Expected payload. | open | answered | no | 2026-09-26: '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.' |
| alexanderwiederin (member) | objection, interface | Flattening results into booleans allows fatal runtime errors to silently become invalid blocks, leaving safety reliant on reviewer checks rather than interface enforcement. | contested · alexanderwiederin pushed back that safety relies on manual reviewer vigilance rather than interface enforcement | answered | by others | 2026-10-09: 'the current design still leaves room for this when results are flattened into a boolean, e.g. `bool ok{res && res->IsValid()}`, where a fatal error silently becomes "invalid"' Status from: 2026-10-10: alexanderwiederin: 'that property currently holds because reviewers checked each site, not because the interface enforces it' |
| alexanderwiederin (member) | suggestion, approach | Settle the mechanism discussion for fatal errors in #36101 to give the project a consistent long-term direction. | open | answered | no | 2026-10-12: 'I would suggest settling the mechanism discussion in #36101, which gives the project a consistent direction long term.' |
| optout21 (contributor) | suggestion, maintenance | Move the implementation to the .cpp file to avoid including util/translation.h in the header. | open | answered | no | 2026-10-21: 'Moving the implementation to the .cpp file could save an include here (`util/translation.h`).' |
| optout21 (contributor) | suggestion, maintenance | Delete the unused state variable or move it inside the for loop to reduce its scope. | open | answered | no | 2026-10-21: 'state is not used from here on, so this could be deleted. In fact, the state local variable could be moved inside the for loop to reduce its scope.' |
| optout21 (contributor) | suggestion, safety | Return state instead of {} to avoid swallowing unchecked non-valid states. | open | answered | no | 2026-10-21: 'For robustness (against future changes) `return state` would be more appropriate here. If it's still Valid, it is the same as `{}`. If it's not Valid, it should have been checked already.' |
| optout21 (contributor) | suggestion, maintenance | Update comment to reflect that returning FatalError cannot currently be triggered when fJustCheck=true. | open | answered | no | 2026-10-21: 'In the current version there is not way to trigger returning a `FatalError`... I suggest changing the comment accordingly' |
| optout21 (contributor) | objection, correctness | Validation state is not set to Error before returning false. | open | changed | no | 2026-10-21: 'Important! `state` has to be set to `Error` here before returning `false`.' |
| optout21 (contributor) | objection, correctness | BlockChecked is no longer signaled upon Error. | open | answered | no | 2026-10-21: 'Behavior change: previously `BlockChecked` was signaled upon `Error`, now it isn't. Should be checked.' |
| optout21 (contributor) | objection, correctness | Error branch fails to return false, causing fallthrough to Invalid case and triggering FatalError. | open | changed | no | 2026-10-21: 'This error branch should return (`return false`), to prevent falling through to the Invalid case, which also does a `FatalError`.' |
| optout21 (contributor) | objection, correctness | An Error condition can return false while leaving state reported as valid. | open | changed | no | 2026-10-21: 'Here an Error can cause a `false` return with a valid `state`!' |
| optout21 (contributor) | suggestion, scope | Omit unrelated change in src/rpc/blockchain.cpp. | open | answered | no | 2026-10-21: 'Unrelated change, omit it.' |
| optout21 (contributor) | question, interface | Why is [[nodiscard]] not used here as on other methods? | open | answered | no | 2026-10-21: 'Why not `[[nodiscard]]` here as on other methods?' |
| optout21 (contributor) | suggestion, maintenance | Update documentation for the method. | open | answered | no | 2026-10-21: 'Also, the doc was not updated.' |
| optout21 (contributor) | question, correctness | Whether new validation reason strings are being silently introduced. | open | answered | no | 2026-10-21: 'Are we silently introducing new validation reason strings?' |
Support:
- optout21 (contributor) (Concept ACK, 2026-07-08) [read]: enforcement of returned errors -- some methods can return only runtime errors; reduced risk of accidental mix-up of validation and runtime errors.
- optout21 (contributor) (Concept ACK, 2026-09-08) [read]: PR stands on its own and complements #35570 by cleanly separating runtime errors.
- hodlinator (member) (Concept ACK, 2026-09-17) [read]: Extracting the error-part of BlockValidationState by using util::Expected is a promising direction.
- l0rinc (member) (2026-09-23) [read]: explicit, value-based error handling avoids hard-to-predict invisible control flow of exceptions in mission-critical applications.
- arejula27 (contributor) (Concept ACK, 2026-09-26) [read]: Likes the util::Expected direction over exceptions as it is easier to understand and follow.
Participants:
- maflcko (member, objection, 3 statements 2026-07-07 to 2026-09-17): concerned about flattening errors and RPC consistency, settled by FatalError type
- optout21 (contributor, objection, 55 statements 2026-07-08 to 2026-10-21): Concept ACK, but found multiple bugs, missing error states, and behavior changes in recent review
- hodlinator (member, support, 27 statements 2026-09-15 to 2026-09-24): Concept ACK on extracting errors via util::Expected; defended approach against exceptions
- l0rinc (member, support, 1 statements 2026-09-23 to 2026-09-23): defended value-based error handling against exceptions to avoid unpredictable control flow
- purpleKarrot (member, objection, 4 statements 2026-09-23 to 2026-10-09): Concept NACK against util::Expected, pushed back that returning expected risks chain splits and dual error handling
- arejula27 (contributor, objection, 2 statements 2026-09-26 to 2026-10-10): Concept ACK on util::Expected, but objected that adding a kernel FatalError type is out of scope and broadens kernel maintenance surface
- alexanderwiederin (member, objection, 3 statements 2026-10-09 to 2026-10-12): concerned that flattening results into booleans relies on reviewer checks rather than interface enforcement, and suggested settling fatal error mechanism in #36101
Dependencies
Enables:
Files
314 lines under test/bench/ci.
- src/validation.cpp +155/-156
- src/test/validation_block_tests.cpp +105/-5
- src/validation.h +24/-26
- src/node/blockstorage.cpp +23/-26
- src/test/validation_tests.cpp +48/-0
- src/kernel/notifications_interface.h +45/-1
- src/rpc/mining.cpp +22/-10
- src/rpc/blockchain.cpp +10/-19
- src/test/validation_chainstatemanager_tests.cpp +15/-9
- src/net_processing.cpp +14/-8
- src/test/chainstate_write_tests.cpp +7/-9
- src/node/interfaces.cpp +11/-4
- src/node/blockstorage.h +7/-6
- src/test/validation_chainstate_tests.cpp +7/-6
- src/consensus/validation.h +1/-10
- src/kernel/bitcoinkernel.cpp +4/-7
- src/node/miner.cpp +9/-2
- src/test/fuzz/block.cpp +6/-5
- src/test/fuzz/connect_block.cpp +3/-7
- src/node/block_template_manager.cpp +7/-2
- src/test/baseindex_tests.cpp +6/-3
- src/test/blockfilter_index_tests.cpp +6/-3
- src/test/util/setup_common.cpp +4/-5
- src/test/blockmanager_tests.cpp +4/-4
- src/test/txindex_tests.cpp +4/-4
- src/bench/readwriteblock.cpp +4/-3
- src/kernel/bitcoinkernel.h +3/-4
- src/test/util/chainstate.h +3/-4
- src/interfaces/mining.h +4/-2
- src/bench/connectblock.cpp +3/-2
- src/bitcoin-chainstate.cpp +0/-4
- src/test/kernel/test_kernel.cpp +0/-4
- src/wallet/test/wallet_tests.cpp +2/-2
- src/test/blockchain_tests.cpp +1/-2
- src/test/interfaces_tests.cpp +1/-2
- src/test/peerman_tests.cpp +2/-1
- src/test/util/mining.cpp +2/-1
- src/validationinterface.h +2/-0
- src/kernel/bitcoinkernel_wrapper.h +0/-1
- src/node/block_template_manager.h +1/-0
Card
The short text the ranking pass compares across PRs.
This pull request removes the runtime error state (M_ERROR) from ValidationState and routes fatal runtime and I/O failures through util::Expected<T, kernel::FatalError> instead. BlockValidationState previously conflated block validation failures with runtime system errors, forcing non-validating routines like FlushStateToDisk and InvalidateBlock to take validation state parameters to report disk failures. In addition to clarifying error reporting and updating the libbitcoinkernel API, this unblocks PR 35570 from returning BlockValidationState directly from validation methods.
Data
thread read through 2026-10-21 (122 statements), head 7a6ced08c6
labels: Brainstorming, Validation, Needs rebase
Categories
Kernel (libbitcoinkernel)
P2 · cleanup
Decoupling system runtime failures from validation state refactors the kernel's error reporting boundary and C API. The description highlights removing internal error enumerations from the kernel API and introducing a clean `kernel::FatalError` type, improving library boundary modularity.
Membership: Modifies the libbitcoinkernel C API, notifications interface, and error enumerations in src/kernel/.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 2
Validation
P2 · unblocks #35570
Decoupling fatal runtime errors from `BlockValidationState` removes an architectural hazard across consensus and block storage functions. The PR resolves an issue where '`BlockValidationState` conflates block validity with unexpected system failures like disk write or memory errors' and 'paves the way for #35570 to return `BlockValidationState` cleanly from validation methods'.
Membership: Labeled 'Validation' and refactors core routines like ConnectBlock, AcceptBlock, FlushStateToDisk, and ActivateBestChain in src/validation.cpp.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 2