refactor: Add util::Result failure types and ability to merge result values #25665

ryanofsky · 2022-07-21T12:46:09Z

Add util::Result support for returning more error information and make use of it in LoadChainstate method as an initial application. Followup PRs #25722 and #29700 use it more broadly to return errors and warnings from wallet and kernel functions as well.

This change adds two major features to the result class:

For better error handling, adds the ability to return a value on failure, not just a value on success. This is a key missing feature that makes the result class not useful for functions like LoadChainstate() which produce different errors that need to be handled differently ¹.

For better error reporting, adds the ability to return warning messages and multiple errors, not just a single error string. This provides a way for functions to report errors and warnings in a standard way, and simplifies interfaces:

-virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
+virtual util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name) = 0;

-std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+util::Result<std::unique_ptr<WalletDatabase>, DatabaseError> MakeDatabase(const fs::path& path, const DatabaseOptions& options);

This change also makes some more minor improvements:

Smaller type size. util::Result<int> is 16 bytes, and util::Result<void> is 8 bytes. Previously both were 72 bytes.
Broader type compatibility. Supports noncopyable and nonmovable success and error types.

Comparison with `std::expected`

Currently, the util::Result class is useful for error reporting—returning translated, user-facing error strings—but not as useful for error handling, where callers need structured failure information to decide how to proceed. By contrast, std::expected / util::Expected (#34005, #34006) is good for error handling but less helpful for reporting. This PR closes the gap between the two classes.

With this PR, std::expected remains a good choice for lower-level functions or simple functions that fail in only a few well-defined ways. Meanwhile,

Result becomes a better choice for operations that can fail in many different ways—loading a wallet, connecting a block, etc.—where callers may want complete error traces, not just low-level errors without context or high-level errors without detail. (Followup #25722 applies this to wallet-loading functions.)
Result can also be more useful than expected when merging status information from multiple calls. (Followup #29700 uses this to reliably propagate flush and fatal-error status from validation code that can internally trigger flushes or shutdowns.)
Result may also be a better choice than std::expected when returning pointer values. (Followup #26022 adds a ResultPtr specialization that avoids double dereferencing.)
Result can be more efficient than std::expected when failures are rare but failure objects are large, since failure values are heap-allocated.

This PR effectively makes Result a superset of expected, making the representations compatible and allowing them to be used together.

Ability to return error values was actually present in the original implementation of #25218, but unfortunately removed in later iterations. See design notes. ↩

src/bench/wallet_loading.cpp

ryanofsky · 2022-07-21T13:18:25Z

Draft PR since I want to add a commit taking advantages of ability to return chain results and return warnings.

DrahtBot · 2022-07-21T18:24:49Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/25665.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	arejula27
Approach ACK	hebasto
Stale ACK	w0xlt, stickies-v, hernanmarino, jonatack, maflcko, laanwj, achow101, sedited

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34004 (Implementation of SwiftSync by rustaceanrob)
#32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
#28690 (build: Introduce internal kernel library by sedited)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

"helper functions types that can be used with" -> "helper functions and types that can be used with" [“functions types” is invalid; needs "functions and types" to be grammatical]
"to for dealing with error and warning messages." -> "for dealing with error and warning messages." [extra "to" makes the phrase ungrammatical]
"allowing each to be understood to changed more easily." -> "allowing each to be understood or changed more easily." ["to changed" is ungrammatical; "or changed" restores intended meaning]

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

ExpectSuccess(IntFn(5, true), {}, 5) in src/test/result_tests.cpp
ExpectSuccess(NoCopyFn(5, true), {}, 5) in src/test/result_tests.cpp
ExpectError(NoCopyFn(5, false), Untranslated("nocopy 5 error."), 5) in src/test/result_tests.cpp
ExpectSuccess(NoCopyNoMoveFn(5, true), {}, 5) in src/test/result_tests.cpp
ExpectError(NoCopyNoMoveFn(5, false), Untranslated("nocopynomove 5 error."), 5) in src/test/result_tests.cpp
ExpectError(ChainedErrorFn(ERR1, 5), Untranslated("chained error. enum error. warn."), 5) in src/test/result_tests.cpp
ExpectSuccess(MultiWarnFn(3), Untranslated("warn 0. warn 1. warn 2."), 3) in src/test/result_tests.cpp
ExpectSuccess(AccumulateFn(true), Untranslated("warn 0. warn 0. warn 1. int 3 warn."), 3) in src/test/result_tests.cpp
ExpectSuccess(TruthyFalsyFn(0, true), {}, 0) in src/test/result_tests.cpp
ExpectError(TruthyFalsyFn(0, false), Untranslated("error value 0."), 0) in src/test/result_tests.cpp
ExpectSuccess(TruthyFalsyFn(1, true), {}, 1) in src/test/result_tests.cpp
ExpectError(TruthyFalsyFn(1, false), Untranslated("error value 1."), 1) in src/test/result_tests.cpp
ExpectSuccess(result, {}, 0) in src/test/result_tests.cpp

^2026-01-07

arejula27 · 2025-12-13T01:07:33Z

Re @ryanofsky

TBH I don't know of code that would directly benefit from combining ErrorString with std::expected because I think validation and wallet code are better off using util::Result

If you don't see any benefits keep your implementation, a generalisation can be implemented later if required 😄

hodlinator

Thanks for the updates!

Annoyed at myself for not investigating my assumed either/or-ness of your proposed util::Result sooner. Somehow I'd interpreted FailDataHolders:

explicit operator bool() const { return !m_fail_data || !m_fail_data->failure; }

as plainly:

explicit operator bool() const { return !m_fail_data; }

The former makes it a clearer departure from util::Expected - warnings can be added and merged into parent results while still returning successful values.

hodlinator · 2025-12-12T19:35:46Z

src/test/result_tests.cpp

nit in ab94499 commit message:

Smaller type size section could mention that given values are for 64-bit platforms, since we still ship ARM32 releases.

hodlinator · 2025-12-15T14:02:17Z

src/node/chainstate.h

+util::Result<kernel::InterruptResult, ChainstateLoadError> LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes,
+                                                                          const ChainstateLoadOptions& options);
+util::Result<kernel::InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options);


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 as in my linked commit.

It would be better to use a different example in this PR unless I'm still missing something.

Avoid false positive maybe-uninitialized errors in 00_setup_env_native_fuzz_with_valgrind CI jobs. Similar errors also happen in 00_setup_env_arm and 00_setup_env_win64 jobs. Problem was pointed out and fix was suggested by maflcko in bitcoin#25665 (comment) CI errors look like: https://github.com/bitcoin/bitcoin/actions/runs/18881332233/job/53885646274?pr=25665 /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp: In function ‘wallet::DescriptorScriptPubKeyMan* wallet::CreateDescriptor(CWallet&, const std::string&, bool)’: /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp:213:29: error: ‘*(const std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>*)((char*)&spkm + offsetof(util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>,util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>)).std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>::_M_data’ may be used uninitialized [-Werror=maybe-uninitialized] 213 | return &spkm.value().get(); | ~~~~~~~~~~~~~~~~^~ /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp:212:10: note: ‘*(const std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>*)((char*)&spkm + offsetof(util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>,util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>)).std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>::_M_data’ was declared here 212 | auto spkm = Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false)); | ^~~~

Use result.Update in CompleteChainstateInit() and ChainstateManager::ActivateSnapshot() so it is possible for them to return warning messages (about flush failures) in upcoming commits. CompleteChainstateInit() was previously changed to use util::Result in bitcoin#25665 and ChainstateManager::ActivateSnapshot() was changed to use it in bitcoin#30267, but previously these functions only returned Result values themselves, and did not call other functions that return Result values. Now, some functions they are calling will also return Result values, so refactor these functions to use result.Update so they can merge results and return complete error and warning messages.

hodlinator · 2025-12-17T13:36:33Z

src/util/result.h

+{
+protected:
+    struct FailData {
+        std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> failure{};


nit: Maybe this could be made more distinct from warnings by changing the name?

Suggested change

std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> failure{};

std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> error{};

Would hopefully decrease misunderstandings like I had myself in #25665 (review), since in that case it would be:

explicit operator bool() const { return !m_fail_data || !m_fail_data->error; }

Add util::Result support for returning more error information. For better error handling, adds the ability to return a value on failure, not just a value on success. This is a key missing feature that made the result class not useful for functions like LoadChainstate() which produce different errors that need to be handled differently. This change also makes some more minor improvements: - Smaller type size. On 64-bit platforms, util::Result<int> is 16 bytes, and util::Result<void> is 8 bytes. Previously util::Result<int> and util::Result<void> were 72 bytes. - More documentation and tests.

Previous commit causes the warning to be triggered, but the suboptimal return from const statement was added earlier in 517e204. Warning was causing CI failure https://cirrus-ci.com/task/6159914970644480

Add util::Result Update method to make it possible to change the value of an existing Result object. This will be useful for functions that can return multiple error and warning messages, and want to be able to change the result value while keeping existing messages.

…ainstate

Add util::Result support for returning warning messages and multiple errors, not just a single error string. This provides a way for functions to report errors and warnings in a standard way, and simplifies interfaces. The functionality is unit tested here, and put to use in followup PR bitcoin#25722

Suggested by Martin Leitner-Ankerl <martin.ankerl@gmail.com> bitcoin#25722 (comment) Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>

Avoid false positive maybe-uninitialized errors in 00_setup_env_native_fuzz_with_valgrind CI jobs. Similar errors also happen in 00_setup_env_arm and 00_setup_env_win64 jobs. Problem was pointed out and fix was suggested by maflcko in bitcoin#25665 (comment) CI errors look like: https://github.com/bitcoin/bitcoin/actions/runs/18881332233/job/53885646274?pr=25665 /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp: In function ‘wallet::DescriptorScriptPubKeyMan* wallet::CreateDescriptor(CWallet&, const std::string&, bool)’: /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp:213:29: error: ‘*(const std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>*)((char*)&spkm + offsetof(util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>,util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>)).std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>::_M_data’ may be used uninitialized [-Werror=maybe-uninitialized] 213 | return &spkm.value().get(); | ~~~~~~~~~~~~~~~~^~ /home/admin/actions-runner/_work/_temp/src/wallet/test/util.cpp:212:10: note: ‘*(const std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>*)((char*)&spkm + offsetof(util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>,util::Result<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>.util::detail::SuccessHolder<std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>, void, util::Messages>::<unnamed>)).std::reference_wrapper<wallet::DescriptorScriptPubKeyMan>::_M_data’ was declared here 212 | auto spkm = Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false)); | ^~~~

DrahtBot · 2026-01-07T03:41:24Z

🚧 At least one of the CI tasks failed.
_{Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/20769214790/job/59641680094}
_{LLM reason (✨ experimental): IWYU detected include issues (failure generated from IWYU), causing the CI to fail.}

Hints

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.

Use result.Update in CompleteChainstateInit() and ChainstateManager::ActivateSnapshot() so it is possible for them to return warning messages (about flush failures) in upcoming commits. CompleteChainstateInit() was previously changed to use util::Result in bitcoin#25665 and ChainstateManager::ActivateSnapshot() was changed to use it in bitcoin#30267, but previously these functions only returned Result values themselves, and did not call other functions that return Result values. Now, some functions they are calling will also return Result values, so refactor these functions to use result.Update so they can merge results and return complete error and warning messages.

Avoid false positive maybe-uninitialized errors in native_previous_releases CI job. Fix was suggested by maflcko in bitcoin#25665 (comment) CI errors look like: https://github.com/bitcoin/bitcoin/actions/runs/21052613538/job/60542048502?pr=29700 /usr/include/c++/12/variant:1788:15: error: ‘((const std::__detail::__variant::_Variant_storage<true, VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>::__index_type*)((char*)&verify_result + offsetof(util::Result<std::variant<VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>, void, kernel::FlushStatus, util::Messages>,util::Result<std::variant<VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>, void, kernel::FlushStatus, util::Messages>::<unnamed>.util::detail::SuccessHolder<std::variant<VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>, void, kernel::FlushStatus, util::Messages>::<unnamed>)))[1]’ may be used uninitialized [-Werror=maybe-uninitialized] 1788 | switch (__v0.index()) | ^~~~~~ /home/admin/actions-runner/_work/_temp/src/node/chainstate.cpp: In function ‘kernel::FlushResult<std::variant<std::monostate, kernel::Interrupted>, node::ChainstateLoadError> node::VerifyLoadedChainstate(ChainstateManager&, const ChainstateLoadOptions&)’: /home/admin/actions-runner/_work/_temp/src/node/chainstate.cpp:281:18: note: ‘((const std::__detail::__variant::_Variant_storage<true, VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>::__index_type*)((char*)&verify_result + offsetof(util::Result<std::variant<VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>, void, kernel::FlushStatus, util::Messages>,util::Result<std::variant<VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>, void, kernel::FlushStatus, util::Messages>::<unnamed>.util::detail::SuccessHolder<std::variant<VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>, void, kernel::FlushStatus, util::Messages>::<unnamed>)))[1]’ was declared here 281 | auto verify_result{CVerifyDB(chainman.GetNotifications()).VerifyDB( | ^~~~~~~~~~~~~

This was referenced Jul 21, 2022

BResult improvements, allow returning values on failure #25608

Closed

refactor: wallet: return util::Result from GetReservedDestination methods #25656

Merged

maflcko reviewed Jul 21, 2022

View reviewed changes

src/bench/wallet_loading.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/bresult2 branch from 6a06a7c to 6e84015 Compare July 21, 2022 17:02

This was referenced Jul 21, 2022

assumeutxo: snapshot initialization #25667

Merged

refactor: wallet, do not translate init options names #25666

Merged

DrahtBot mentioned this pull request Jul 21, 2022

wallet: return change from SelectionResult #25647

Merged

ryanofsky force-pushed the pr/bresult2 branch from 6e84015 to 9ba210b Compare July 21, 2022 18:49

This was referenced Jul 21, 2022

refactor: Return util::Result from WalletLoader methods #25616

Merged

util: Make BResult error a generic type instead of only bilingual_str #25601

Closed

ryanofsky force-pushed the pr/bresult2 branch from 9ba210b to 10e78ee Compare July 21, 2022 19:43

DrahtBot added the Needs rebase label Jul 22, 2022

ryanofsky force-pushed the pr/bresult2 branch from 10e78ee to 358e0fe Compare July 22, 2022 10:43

DrahtBot removed the Needs rebase label Jul 22, 2022

ryanofsky force-pushed the pr/bresult2 branch from 358e0fe to 32dd8eb Compare July 25, 2022 22:22

ryanofsky force-pushed the pr/bresult2 branch from 32dd8eb to 33d5f03 Compare July 26, 2022 15:36

hodlinator reviewed Dec 15, 2025

View reviewed changes

ryanofsky force-pushed the pr/bresult2 branch from 074cb32 to bbcabe3 Compare December 16, 2025 02:10

DrahtBot added Needs rebase and removed CI failed labels Dec 16, 2025

kevkevinpal mentioned this pull request Dec 16, 2025

refactor: Improve assumeutxo state representation kevkevinpal/bitcoin#231

Closed

hodlinator reviewed Dec 17, 2025

View reviewed changes

ryanofsky and others added 7 commits January 6, 2026 05:45

wallet: fix clang-tidy warning performance-no-automatic-move

173c0eb

Previous commit causes the warning to be triggered, but the suboptimal return from const statement was added earlier in 517e204. Warning was causing CI failure https://cirrus-ci.com/task/6159914970644480

refactor: Use util::Result class in LoadChainstate and VerifyLoadedCh…

0263614

…ainstate

test: add static test for util::Result memory usage

339f0d9

Suggested by Martin Leitner-Ankerl <martin.ankerl@gmail.com> bitcoin#25722 (comment) Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>

ryanofsky force-pushed the pr/bresult2 branch 2 times, most recently from 2db8bd9 to a3c37e6 Compare January 7, 2026 03:39

DrahtBot added the CI failed label Jan 7, 2026

DrahtBot removed Needs rebase CI failed labels Jan 7, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Add util::Result failure types and ability to merge result values #25665

refactor: Add util::Result failure types and ability to merge result values #25665

ryanofsky commented Jul 21, 2022 •

edited

Loading

Uh oh!

Uh oh!

ryanofsky commented Jul 21, 2022

Uh oh!

DrahtBot commented Jul 21, 2022 •

edited

Loading

Uh oh!

arejula27 commented Dec 13, 2025 •

edited

Loading

Uh oh!

hodlinator left a comment

Uh oh!

hodlinator Dec 12, 2025

Uh oh!

hodlinator Dec 15, 2025

Uh oh!

hodlinator Dec 17, 2025

Uh oh!

DrahtBot commented Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

18 participants

	std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> failure{};
	std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> error{};

refactor: Add util::Result failure types and ability to merge result values #25665

Are you sure you want to change the base?

refactor: Add util::Result failure types and ability to merge result values #25665

Conversation

ryanofsky commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comparison with std::expected

Footnotes

Uh oh!

Uh oh!

ryanofsky commented Jul 21, 2022

Uh oh!

DrahtBot commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

arejula27 commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

hodlinator Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

hodlinator Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

hodlinator Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

18 participants

ryanofsky commented Jul 21, 2022 •

edited

Loading

Comparison with `std::expected`

DrahtBot commented Jul 21, 2022 •

edited

Loading

arejula27 commented Dec 13, 2025 •

edited

Loading