#25665 refactor: Add util::Result failure types and ability to merge result values
https://github.com/bitcoin/bitcoin/pull/25665 · · +732/-169 in 13 files, 7 commits · labels: Refactoring
Goal
- Allow functions to return structured failure types alongside multiple warnings and error messages
- Eliminate ad-hoc out-parameters and tuple returns in multi-step operations like chainstate loading
This PR expands the util::Result utility class to support returning structured failure values (instead of only void on failure), multiple error and warning messages, and result merging via an Update() method. It optimizes memory layout so happy-path Result objects are significantly smaller, and converts LoadChainstate and VerifyLoadedChainstate to use the updated class.
Problem: Functions across the codebase that report errors and warnings or distinguish between different failure reasons currently rely on awkward combinations of out-parameters (such as bilingual_str& and vector<bilingual_str>&) and status tuples. The existing util::Result class could only return user-facing error strings, making it unsuitable for functions requiring structured error handling or warning propagation.
Category: Utilities (logging, arguments, libraries) (#5 of 66)
P2 · cleanup
- P3 because it improves common error-handling primitives and lowers result memory use
- Unblocks cleaner error bubbling in wallet and validation code but is deferrable
Major maintainability improvement that establishes a standard pattern for error and warning propagation, removing out-parameter boilerplate across subsystems. It unblocks critical downstream error-handling work in wallet (#25722) and libbitcoinkernel (#29700), while shrinking the memory footprint of Result on 64-bit platforms from 72 bytes to 16 bytes on the happy path.
Membership: Redesigns src/util/result.h and adds src/util/messages.h to provide generalized error and warning handling primitives.
Factors: security/stability 0, bug 0, performance 1, user value 0, leverage 3
Reviewability: Ready
- Ready for review, clean, and passing CI
- Very long comment thread makes navigation difficult
The PR is rebased cleanly on current master with green CI and no blocking requests pending on the author.
Author status: Active, rebasing and maintaining the branch against master conflicts.
Open concerns:
- Reviewers noted the discussion thread has accumulated over 300 comments over multiple years, suggesting closing and reopening a clean PR to reset review fatigue.
Resolved concerns:
- Separated earlier naming and interface refactors into standalone PR #25721.
- Addressed concern regarding Update() usage in non-chaining contexts by simplifying CompleteChainstateInitialization call sites.
- Reconciled the role of util::Result with std::expected/util::Expected (#34006), aligning method names and separating message helpers into util/messages.h.
Agreement: Mild
- Broad support across revisions from maintainers (laanwj, achow101, sedited)
- Debate over whether error payloads belong in util::Expected instead (hodlinator, maflcko)
- Suggestions to close and reopen cleanly due to thread length (maflcko, polespinasa, sedited)
Mild: nonblocking objection open (maflcko, hodlinator)
Multiple maintainers have ACKed the code and concept. Earlier approach questions regarding compatibility with std::expected were resolved when #34006 merged and the author adapted Result to mirror expected's interface. Remaining reviewer remarks concern PR length and review noise rather than unaddressed technical harms.
- laanwj (2025-10-23): 'Concept and code review ACK'
- achow101 (2025-10-27): 'ACK 8b892d41fdeb5756fd83f6050f27a170338d260a'
- polespinasa (2026-05-11): 'Concept ACK: This is useful not only for cleaning the wallet code, but also for new interface methods with complex results'
- sedited (2026-05-29): 'I've ACKed this pull request a few times, but I agree with the preceding comments here: This is clearly not getting review at the moment, so should be closed and re-attempted again in a fresh pull request.'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| stickies-v | interface | Using Update() when not chaining can mask bugs where a fresh assignment is intended | resolved | no | yes | 2024-04-26: 'I don't think this is good usage of Update(), since there is no chaining happening here. I would find list initialization more readable and less error-prone' Settled: 2024-07-09: 'Took suggestions to simplify usage in CompleteChainstateInitialization.' |
| hodlinator | approach | Conflating error values with multi-message reporting when util::Expected could suffice | resolved | no | yes | 2025-12-05: 'I think util::Result is conflating 2 related but orthogonal needs... At this point I would prefer util::Expected be merged first (#34006).' Settled: 2025-12-15: 'The former makes it a clearer departure from util::Expected - warnings can be added and merged into parent results while still returning successful values.' |
| maflcko | approach | Adding complexity to util::Result when simpler Expected or smaller patches could be used | open | no | yes | 2024-07-04: 'My recommendation would be to keep the code as simple as possible and not add complicated features or complication that isn't required.' Settled: 2025-12-12: Author aligned Result naming with std::expected after #34006 merged and moved message logic to util/messages.h. |
| hodlinator | approach | conflates distinct error-handling and error-reporting concepts; LoadChainstate does not motivate the extra complexity of Result over Expected | open | no | yes | 2025-12-15: 'I maintain that the current example in this PR of applying util::Result to LoadChainstate & VerifyLoadedChainstate is not well motivated as they don't use warnings or multiple errors, and are thus a better fit for util::Expected' |
Support:
- laanwj: Concept and code review ACK
- sedited: Provided continuous code reviews and multiple re-ACKs
- achow101: ACK on code changes [not substantive]
- polespinasa: Noted it is useful for cleaning wallet code and for new interface methods with complex results
- hernanmarino: Tested ACK and code review ACK
- arejula27: concept ACK for merging results and error reporting capability
- Riahiamirreza: approved code after review [not substantive]
Participants: maflcko (objection), stickies-v (objection), Riahiamirreza (support), dongcarl (neutral), AryanJ-NYC (neutral), sipa (neutral), hernanmarino (support), pablomartin4btc (neutral), w0xlt (support), hebasto (support), sedited (support), jonatack (support), achow101 (support), laanwj (support), hodlinator (objection), arejula27 (support), polespinasa (support)
State derived from the lists: nonblocking objection open (maflcko, hodlinator) (model's own read: Strong)
Review verdicts (DrahtBot): 0 (+8)
- Stale ACK: w0xlt, stickies-v, hernanmarino, jonatack, maflcko, laanwj, achow101, sedited
- Approach ACK: hebasto
- Concept ACK: arejula27, polespinasa
Dependencies
Enables:
- PR #25722: Use util::Result class for wallet loading
- PR #29700: kernel: return error status on all fatal errors
- PR #26022: Add util::ResultPtr class
Base for: #29700
Files
216 lines under test/bench/ci.
- src/util/result.h +355/-63
- src/test/result_tests.cpp +195/-11
- src/node/chainstate.cpp +56/-39
- src/util/messages.h +53/-0
- src/init.cpp +21/-20
- src/util/messages.cpp +30/-0
- src/node/chainstate.h +6/-22
- src/kernel/bitcoinkernel.cpp +7/-7
- src/test/util/setup_common.cpp +4/-4
- src/wallet/wallet.cpp +2/-2
- ci/test/00_setup_env_native_previous_releases.sh +1/-1
- src/kernel/CMakeLists.txt +1/-0
- src/util/CMakeLists.txt +1/-0
Card
This PR expands util::Result to support returning structured failure types, multiple error and warning messages, and result merging via an Update() method. It solves the recurring boilerplate of ad-hoc error and warning out-parameters across high-level operations like wallet loading and chainstate initialization. Downstream, it directly unblocks kernel fatal error reporting in #29700 and wallet loading refactoring in #25722. The code has strong support and multiple ACKs from maintainers, but has experienced review friction due to thread length spanning several years.