#35646 RFC: Separate out runtime errors from BlockValidationState using `util::Expected`
https://github.com/bitcoin/bitcoin/pull/35646 · · +617/-384 in 41 files, 5 commits · labels: Brainstorming, Validation, CI failed
Goal
- Stop conflating internal runtime failures like disk write errors with peer-penalizing block invalidity
- Separate internal system faults from consensus rejection logic across block processing
Removes the `M_ERROR` runtime error state from `ValidationState` and `BlockValidationState`, introducing a move-only `kernel::FatalError` returned via `util::Expected`. Non-validation functions such as `FlushStateToDisk`, `DisconnectTip`, and `ActivateBestChain` no longer take `BlockValidationState` out-parameters, and fatal errors are channeled explicitly through notifications and return types.
Problem: `BlockValidationState` currently combines two unrelated concepts: consensus/policy invalidity and internal runtime failures like disk write errors. This forces callers to inspect validation objects for non-validation operations and conflates system faults with peer-penalizing block invalidity.
Category: Kernel (libbitcoinkernel) (#9 of 18)
P3 · cleanup
- P3 because it clarifies the kernel interface boundary without fixing bugs or improving performance
- Ensures validation callbacks strictly signal block validity rather than internal faults
Clarifies the kernel API boundary by ensuring validation callbacks only convey validity and delegating internal aborts to the fatal error notification.
Membership: Changes the libbitcoinkernel C API and wrapper by removing INTERNAL_ERROR from btck_ValidationMode and adds kernel/fatal_error.h.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1
Category: Validation (#22 of 48)
P3 · cleanup
- P3 because it cleans up core validation invariants without fixing an active bug or performance bottleneck
- Unblocks further modernization of validation routines and error propagation
Separates system error handling from consensus rejection logic across validation routines. While it does not fix an active bug or performance bottleneck, it cleans up core invariants and unblocks further return-type modernization in validation.
Membership: Directly refactors BlockValidationState, ConnectBlock, AcceptBlock, and chainstate execution error pathways.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 2
Reviewability: Stale: CI failing
- CI is failing and reviewers asked to split the large commit into smaller steps before detailed review
CI has failed on the latest push, and reviewers have requested breaking up the large commit before detailed review.
Author status: active
Open concerns:
- The main commit touching validation methods is very large and reviewers requested splitting it into smaller, incremental commits by function or return type.
- Ensuring callback behavior like `BlockChecked` handles fatal failures consistently without altering peer-ban or notification expectations.
Resolved concerns:
- Enforcing single notification firing via `FatalError::Raise` to avoid duplicate fatal error signals.
- Preserving existing RPC error mapping and diagnostics across mining and blockchain RPCs.
Agreement: Positive
- Broad concept support for separating runtime errors from block validation results
- Concept approval citing enforced error handling and less risk of conflating errors (optout21)
- Concept approval welcoming the direction but requesting piecemeal commits (hodlinator)
Concept ACKs from hodlinator and optout21; reviewers requested splitting the large commit into smaller steps.
Reviewers agree with the concept of using Expected to decouple runtime errors from validation results, but have requested commit restructuring and smaller reviewable steps.
- optout21 concept ACKed, noting enforcement of returned errors and reduced risk of mixing up validation and runtime errors.
- hodlinator concept ACKed, calling the extraction via util::Expected a promising direction and offering a suggested piecemeal branch.
- maflcko asked for compile-time enforcement of fatal error notification firing, which the author incorporated via FatalError::Raise.
Review verdicts (DrahtBot): 0
- Concept ACK: optout21, hodlinator
Dependencies
Enables:
Files
328 lines under test/bench/ci.
- src/validation.cpp +156/-155
- src/test/validation_block_tests.cpp +109/-7
- src/kernel/fatal_error.h +58/-0
- src/validation.h +24/-26
- src/test/validation_tests.cpp +48/-0
- src/rpc/mining.cpp +28/-14
- src/node/blockstorage.cpp +19/-21
- src/net_processing.cpp +16/-12
- src/rpc/blockchain.cpp +8/-18
- src/test/validation_chainstatemanager_tests.cpp +15/-9
- src/node/miner.cpp +18/-5
- src/kernel/bitcoinkernel.cpp +9/-10
- 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/test/fuzz/block.cpp +6/-5
- src/validationinterface.h +6/-4
- src/test/baseindex_tests.cpp +6/-3
- src/test/blockfilter_index_tests.cpp +6/-3
- src/test/util/setup_common.cpp +4/-5
- src/bench/readwriteblock.cpp +5/-3
- src/test/blockmanager_tests.cpp +4/-4
- src/test/fuzz/utxo_snapshot.cpp +4/-4
- src/test/txindex_tests.cpp +4/-4
- src/kernel/bitcoinkernel.h +3/-4
- src/test/util/chainstate.h +3/-4
- src/test/util/mining.cpp +4/-3
- src/bench/connectblock.cpp +4/-2
- src/interfaces/mining.h +4/-2
- src/bitcoin-chainstate.cpp +0/-4
- src/test/kernel/test_kernel.cpp +0/-4
- src/test/validationinterface_tests.cpp +2/-2
- src/validationinterface.cpp +2/-2
- src/wallet/test/wallet_tests.cpp +2/-2
- src/kernel/bitcoinkernel_wrapper.h +1/-2
- src/node/miner.h +2/-1
- src/test/blockchain_tests.cpp +1/-2
- src/test/interfaces_tests.cpp +1/-2
- src/test/peerman_tests.cpp +2/-1
Card
This PR removes runtime errors (M_ERROR) from BlockValidationState, replacing them with util::Expected<T, kernel::FatalError> across validation routines such as ConnectBlock and ActivateBestChain. It addresses the architectural conflation of consensus invalidity with system/disk failures, stopping non-validating routines from accepting validation state objects. While reviewers support the concept (Concept ACKs from hodlinator and optout21), they have requested that the large validation commit be broken into smaller pieces. CI is currently failing on the latest push.