refactor: Add util::Result failure values, multiple error and warning messages #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.

Alternatives & design notes

The main goal for the util::Result class is to provide a standard way for functions to report error and warnings strings. The interface tries to make it easy to provide detailed error feedback to end users, without cluttering code or making it harder to return other result information. There have been multiple iterations of the design using different internal representations and providing different capabilities:

	Representation (approximate)	Notes
1	`tuple<T,optional<bilingual_str>>`	Original #25218 implementation in `da98fd2`. Good capabilities, but larger type size. Supports error standardized error reporting and customized error handling with failure values.
2	`variant<T,bilingual_str>`	Final #25218 implementation in `7a45c33`. No support for returning failure values so not useful in many cases.
3	`variant<monostate,T,F>`	Pending #25601 implementation that allows choosing whether to use standardized error reporting or return custom failure values, but doesn't support having both at the same time, like approaches (1), (4), (5), (6) do.
4	`tuple<variant<T,F>,bilingual_str>`	Original #25608 implementation in `c29d300`. Basically the same as (1) except it uses separate success and failure types instead of the same type. Using separate success and failure types makes the result class a little less flexible but can help avoid some ambiguity and inconsistent result states.
5	`tuple<T,optional<bilingual_str>>`	Final #25608 implementation in `dd91f29`. Essentially the same as original implementation (1) except has some usability improvements.
6	`tuple<T,unique_ptr<tuple<F,bilingual_str>>`	Pending #25665 implementation (this PR). Supports returning failure values, uses separate success and failure types to avoid ambiguity, uses `unique_ptr` to reduce result type size, and avoids heap allocation in the happy path.

Prior discussions & history

furszy introduced a BResult class providing a standard error reporting mechanism in #25218. It was renamed to util::Result in #25721 but kept the same representation and capabilities.
MarcoFalke suggested using BResult for the LoadChainstate function in #25308 (comment). Inability to use BResult there due to lack of support for failure values led to initial work on #25608.
w0xlt wrote a StructuredResult class in #25308 adding the ability to return failure values but sacrificing standard error reporting, which led to more work on #25608.
martinus suggested a space optimization in #25608 (comment) that also made it easier to support distinct failure & success types, leading to #25665 as a replacement for #25608.

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

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
Approach ACK	hebasto
Stale ACK	w0xlt, stickies-v, hernanmarino, jonatack, achow101, maflcko, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31616 (init,log: Unify block index log line by l0rinc)
#31483 (kernel: Move kernel-related cache constants to kernel cache by TheCharlatan)
#31061 (refactor: Check translatable format strings at compile-time by maflcko)
#30965 (kernel: Move block tree db open to block manager by TheCharlatan)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#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.

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.

ryanofsky · 2024-12-06T11:36:33Z

Rebased 7e2b357 -> 58c33f0 (pr/bresult2.61 -> pr/bresult2.62, compare) due to conflict with #30377
Rebased 58c33f0 -> 5e14665 (pr/bresult2.62 -> pr/bresult2.63, compare) due to conflict with #31072

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

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.

DrahtBot · 2025-01-17T13:26:08Z

🐙 This pull request conflicts with the target branch and needs rebase.

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

DrahtBot removed the CI failed label Nov 4, 2024

This was referenced Nov 5, 2024

refactor: Clean up messy strformat and bilingual_str usages #31072

Merged

refactor: Check translatable format strings at compile-time #31061

Merged

kernel: Move block tree db open to block manager #30965

Merged

stickies-v mentioned this pull request Nov 8, 2024

Ephemeral Dust #30239

Merged

This was referenced Nov 13, 2024

scripted-diff: Type-safe settings retrieval #31260

Open

refactor: Prepare compile-time check of bilingual format strings #31295

Merged

ci, iwyu: Treat warnings as errors for specific targets #31308

Open

ci: Update Clang in "tidy" job #31306

Merged

DrahtBot mentioned this pull request Nov 25, 2024

kernel, refactor: Replace goto with RAII in bitcoin-chainstate #31362

Closed

DrahtBot added the Needs rebase label Dec 5, 2024

ryanofsky force-pushed the pr/bresult2 branch from 7e2b357 to 58c33f0 Compare December 6, 2024 11:34

ryanofsky and others added 6 commits December 6, 2024 07:38

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

988d7e5

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…

2dd4849

…ainstate

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

5e14665

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 from 58c33f0 to 5e14665 Compare December 6, 2024 12:03

DrahtBot removed the Needs rebase label Dec 6, 2024

DrahtBot mentioned this pull request Dec 13, 2024

kernel: Move kernel-related cache constants to kernel cache #31483

Merged

DrahtBot mentioned this pull request Jan 7, 2025

init,log: Unify block index log line #31616

Merged

DrahtBot added the Needs rebase label Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Add util::Result failure values, multiple error and warning messages #25665

refactor: Add util::Result failure values, multiple error and warning messages #25665

ryanofsky commented Jul 21, 2022 •

edited

Loading

ryanofsky commented Jul 21, 2022

DrahtBot commented Jul 21, 2022 •

edited

Loading

ryanofsky commented Dec 6, 2024 •

edited

Loading

DrahtBot commented Jan 17, 2025

refactor: Add util::Result failure values, multiple error and warning messages #25665

Are you sure you want to change the base?

refactor: Add util::Result failure values, multiple error and warning messages #25665

Conversation

ryanofsky commented Jul 21, 2022 • edited Loading

Alternatives & design notes

Prior discussions & history

Footnotes

ryanofsky commented Jul 21, 2022

DrahtBot commented Jul 21, 2022 • edited Loading

Code Coverage & Benchmarks

Reviews

Conflicts

ryanofsky commented Dec 6, 2024 • edited Loading

DrahtBot commented Jan 17, 2025

ryanofsky commented Jul 21, 2022 •

edited

Loading

DrahtBot commented Jul 21, 2022 •

edited

Loading

ryanofsky commented Dec 6, 2024 •

edited

Loading