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

[ryanofsky]

Add util::Result support for returning more error information and make use of it in LoadChainstate method as an initial application. Followup PR #25722 uses it more broadly to return errors and warnings from wallet loading 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.

Footnotes

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

[ryanofsky]

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

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

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:

• #29817 (kernel: De-globalize fReindex by TheCharlatan)
• #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

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.

[ryanofsky]

Updated 7a4741e -> 28e2081 (pr/bresult2.52 -> pr/bresult2.53, compare) just improving some comments

[ryanofsky]

re: #25665 (comment)

Reviewers might be wondering what the benefits of the ResultTraits are. They are useful for potential future extensions of the move behaviour: 3951afc .

Thanks I added a comment to the traits struct to make this clearer. Traits are an old pattern in c++ and are commonly used. The standard library has std::allocator_traits, std::iterator_traits, std::pointer_traits, std::hash, std::formatter, std::tuple_element, and std::tuple_size trait structs which allow generic library functions to work with user defined types. They all work the same way, by defining a template struct in the library that user code can specialize to control the behavior of library functions when called with custom types. This is an old explanation of traits which may be helpful: http://erdani.org/publications/traits.html

[ryanofsky]

Converted to draft since first 2 commits were moved to a separate PR, #29906

---

Rebased 28e2081 -> 990f9d6 (pr/bresult2.53 -> pr/bresult2.54, compare) on top of updated base PR #29906, also adding more comments, simplifying code a little bit by util::MoveFrom helper, and using Update() method more places for consistency.

[stickies-v]

Posting my response to part of your comment on #29906 here, since Update() is no longer relevant to that PR:

Examples would be the BlockManager::SaveBlockToDisk function from #29700

Had just a quick look, but this looks like how I'd expect Update() to be used, indeed.

or the CompleteChainstateInitialization function from #25665

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:

git diff on 1376583

diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 0671f75515..8cc43c53e5 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -40,7 +40,6 @@ static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInit
     const CacheSizes& cache_sizes,
     const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 {
-    util::Result<InterruptResult, ChainstateLoadError> result;
     auto& pblocktree{chainman.m_blockman.m_block_tree_db};
     // new BlockTreeDB tries to delete the existing file, which
     // fails if it's still open from the previous loop. Close it first:
@@ -61,8 +60,7 @@ static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInit
     }
 
     if (chainman.m_interrupt) {
-        result.Update(Interrupted{});
-        return result;
+        return Interrupted{};
     }
 
     // LoadBlockIndex will load m_have_pruned if we've ever removed a
@@ -71,26 +69,23 @@ static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInit
     // From here on, fReindex and options.reindex values may be different!
     if (!chainman.LoadBlockIndex()) {
         if (chainman.m_interrupt) {
-            result.Update(Interrupted{});
+            return Interrupted{};
         } else {
-            result.Update({util::Error{_("Error loading block database")}, ChainstateLoadError::FAILURE});
+            return {util::Error{_("Error loading block database")}, ChainstateLoadError::FAILURE};
         }
-        return result;
     }
 
     if (!chainman.BlockIndex().empty() &&
             !chainman.m_blockman.LookupBlockIndex(chainman.GetConsensus().hashGenesisBlock)) {
         // If the loaded chain has a wrong genesis, bail out immediately
         // (we're likely using a testnet datadir, or the other way around).
-        result.Update({util::Error{_("Incorrect or no genesis block found. Wrong datadir for network?")}, ChainstateLoadError::FAILURE_INCOMPATIBLE_DB});
-        return result;
+        return {util::Error{_("Incorrect or no genesis block found. Wrong datadir for network?")}, ChainstateLoadError::FAILURE_INCOMPATIBLE_DB};
     }
 
     // Check for changed -prune state.  What we are concerned about is a user who has pruned blocks
     // in the past, but is now trying to run unpruned.
     if (chainman.m_blockman.m_have_pruned && !options.prune) {
-        result.Update({util::Error{_("You need to rebuild the database using -reindex to go back to unpruned mode.  This will redownload the entire blockchain")}, ChainstateLoadError::FAILURE});
-        return result;
+        return {util::Error{_("You need to rebuild the database using -reindex to go back to unpruned mode.  This will redownload the entire blockchain")}, ChainstateLoadError::FAILURE};
     }
 
     // At this point blocktree args are consistent with what's on disk.
@@ -98,8 +93,7 @@ static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInit
     // (otherwise we use the one already on disk).
     // This is called again in ImportBlocks after the reindex completes.
     if (!fReindex && !chainman.ActiveChainstate().LoadGenesisBlock()) {
-        result.Update({util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE});
-        return result;
+        return {util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE};
     }
 
     auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
@@ -133,17 +127,15 @@ static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInit
         // Refuse to load unsupported database format.
         // This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
         if (chainstate->CoinsDB().NeedsUpgrade()) {
-            result.Update({util::Error{_("Unsupported chainstate database format found. "
+            return {util::Error{_("Unsupported chainstate database format found. "
                                          "Please restart with -reindex-chainstate. This will "
                                          "rebuild the chainstate database.")},
-                                       ChainstateLoadError::FAILURE_INCOMPATIBLE_DB});
-            return result;
+                                       ChainstateLoadError::FAILURE_INCOMPATIBLE_DB};
         }
 
         // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
         if (!chainstate->ReplayBlocks()) {
-            result.Update({util::Error{_("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.")}, ChainstateLoadError::FAILURE});
-            return result;
+            return {util::Error{_("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.")}, ChainstateLoadError::FAILURE};
         }
 
         // The on-disk coinsdb is now in a good state, create the cache
@@ -153,8 +145,7 @@ static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInit
         if (!is_coinsview_empty(chainstate)) {
             // LoadChainTip initializes the chain based on CoinsTip()'s best block
             if (!chainstate->LoadChainTip()) {
-                result.Update({util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE});
-                return result;
+                return {util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE};
             }
             assert(chainstate->m_chain.Tip() != nullptr);
         }
@@ -164,9 +155,8 @@ static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInit
         auto chainstates{chainman.GetAll()};
         if (std::any_of(chainstates.begin(), chainstates.end(),
                         [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
-            result.Update({util::Error{strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
-                                                 chainman.GetConsensus().SegwitHeight)}, ChainstateLoadError::FAILURE});
-            return result;
+            return {util::Error{strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
+                                                 chainman.GetConsensus().SegwitHeight)}, ChainstateLoadError::FAILURE};
         };
     }
 
@@ -175,7 +165,7 @@ static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInit
     // on the condition of each chainstate.
     chainman.MaybeRebalanceCaches();
 
-    return result;
+    return {};
 }
 
 util::Result<InterruptResult, ChainstateLoadError> LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes,

[ryanofsky]

re: #25665 (comment)

or the CompleteChainstateInitialization function from #25665

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:

git diff on 1376583

The diff you suggested is actually the way this function was implemented before #29700. The problem is that #29700 updates some validation functions to return util::Result instead of bool. So instead of CompleteChainstateInitialization being a function that just returns a util::Result value, it becomes a function that returns a util::Result value and also calls other functions returning util::Result. Which is the scenario where the result.Update() pattern is useful, because it allows returning warning and error messages from functions being called instead of discarding them, so more complete error information is returned by default when a problem happens.

If you think this pattern is not readable enough or too error prone, we could probably come up with an alternate pattern that doesn't require updating a variable (but may require copying around data more manually). In general, I'd agree that all other things being equal, it's better to initialize variables once than change them over time. But in this case, if the goal is to accumulate error and warning messages, I think a pattern where you declare a single variable at the top of the function holding the messages, and update the variable over the course of the function, and then return the variable at the end is a pretty simple and readable pattern. I'm sure other patterns could work too, though.

I guess I'd want to know if there's another pattern you'd prefer, or what more specific problems you see with the current code.

[stickies-v]

The problem is that #29700 updates some validation functions to return util::Result instead of bool.

I see, that was not apparent in the link you shared earlier. Would it make sense to do that refactoring in #29700 then, instead of in this PR? Because in this PR, I don't think the change makes a lot of sense on its own.

I think a pattern where you declare a single variable at the top of the function holding the messages, and update the variable over the course of the function, and then return the variable at the end is a pretty simple and readable pattern.

I agree, I think that's a reasonable pattern. For this situation, I would prefer using two functions: Update() when chaining is happening, and Set(), Replace(), operator=(), ... when we expect Result to be empty. This way, we can more easily identify potential bugs when e.g. Set() is used but the Result has already had a value assigned to it earlier. Potentially, could even do a run-time assertion (I don't think compile time is possible?).

Combining both would be suitable in this case, I think. Pseudo-code diff (since Set() isn't implemented) where we only keep Update() for the chaining bits:

git diff on 4d2c9de

diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 014f6915ad..bc8d547153 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -64,7 +64,7 @@ static FlushResult<InterruptResult, ChainstateLoadError> CompleteChainstateIniti
     }
 
     if (chainman.m_interrupt) {
-        result.Update(Interrupted{});
+        result.Set(Interrupted{});
         return result;
     }
 
@@ -85,14 +85,14 @@ static FlushResult<InterruptResult, ChainstateLoadError> CompleteChainstateIniti
             !chainman.m_blockman.LookupBlockIndex(chainman.GetConsensus().hashGenesisBlock)) {
         // If the loaded chain has a wrong genesis, bail out immediately
         // (we're likely using a testnet datadir, or the other way around).
-        result.Update({util::Error{_("Incorrect or no genesis block found. Wrong datadir for network?")}, ChainstateLoadError::FAILURE_INCOMPATIBLE_DB});
+        result.Set({util::Error{_("Incorrect or no genesis block found. Wrong datadir for network?")}, ChainstateLoadError::FAILURE_INCOMPATIBLE_DB});
         return result;
     }
 
     // Check for changed -prune state.  What we are concerned about is a user who has pruned blocks
     // in the past, but is now trying to run unpruned.
     if (chainman.m_blockman.m_have_pruned && !options.prune) {
-        result.Update({util::Error{_("You need to rebuild the database using -reindex to go back to unpruned mode.  This will redownload the entire blockchain")}, ChainstateLoadError::FAILURE});
+        result.Set({util::Error{_("You need to rebuild the database using -reindex to go back to unpruned mode.  This will redownload the entire blockchain")}, ChainstateLoadError::FAILURE});
         return result;
     }
 
@@ -101,7 +101,7 @@ static FlushResult<InterruptResult, ChainstateLoadError> CompleteChainstateIniti
     // (otherwise we use the one already on disk).
     // This is called again in ImportBlocks after the reindex completes.
     if (!fReindex && !chainman.ActiveChainstate().LoadGenesisBlock()) {
-        result.Update({util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE});
+        result.Set({util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE});
         return result;
     }
 
@@ -136,7 +136,7 @@ static FlushResult<InterruptResult, ChainstateLoadError> CompleteChainstateIniti
         // Refuse to load unsupported database format.
         // This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
         if (chainstate->CoinsDB().NeedsUpgrade()) {
-            result.Update({util::Error{_("Unsupported chainstate database format found. "
+            result.Set({util::Error{_("Unsupported chainstate database format found. "
                                          "Please restart with -reindex-chainstate. This will "
                                          "rebuild the chainstate database.")},
                                        ChainstateLoadError::FAILURE_INCOMPATIBLE_DB});
@@ -145,7 +145,7 @@ static FlushResult<InterruptResult, ChainstateLoadError> CompleteChainstateIniti
 
         // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
         if (!chainstate->ReplayBlocks()) {
-            result.Update({util::Error{_("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.")}, ChainstateLoadError::FAILURE});
+            result.Set({util::Error{_("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.")}, ChainstateLoadError::FAILURE});
             return result;
         }
 
@@ -156,7 +156,7 @@ static FlushResult<InterruptResult, ChainstateLoadError> CompleteChainstateIniti
         if (!is_coinsview_empty(chainstate)) {
             // LoadChainTip initializes the chain based on CoinsTip()'s best block
             if (!chainstate->LoadChainTip()) {
-                result.Update({util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE});
+                result.Set({util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE});
                 return result;
             }
             assert(chainstate->m_chain.Tip() != nullptr);
@@ -167,7 +167,7 @@ static FlushResult<InterruptResult, ChainstateLoadError> CompleteChainstateIniti
         auto chainstates{chainman.GetAll()};
         if (std::any_of(chainstates.begin(), chainstates.end(),
                         [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
-            result.Update({util::Error{strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
+            result.Set({util::Error{strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
                                                  chainman.GetConsensus().SegwitHeight)}, ChainstateLoadError::FAILURE});
             return result;
         };

It's kind of a similar situation to having a std::vector<>&out-parameter that we use here and there in the codebase. When that happens, first you need to check if the vector is supposed to be empty or not. Then, you need to check if it actually is empty. Sometimes we call .clear() in the beginning of the function, sometimes we don't. It's just confusing and time consuming and error prone (see e.g. #26289). With differentiated Update() and Set() functions we allow the developer to signal intent, which makes it easier to understand the code and to identify potential bugs (manually or through run-time/fuzzing checks).

I've ranted about this already a bit too long, so please do let me know if you (dis)agree but I'll stop commenting further until I've (re)-reviewed the rest of this PR and I maybe get new insights.

[ryanofsky]

Rebased 0c8a1bb -> db91dbb (pr/bresult2.56 -> pr/bresult2.57, compare) after #29906 was merged

[ryanofsky]

re: #25665 (comment)

Thanks @stickies-v. I think since Update() is not required immediately I might move the first and third commits of this PR to another PR without Update(). The only downside will be some churn in the CompleteChainstateInitialization function, since I believe at some point it will need to be changed again to use the Update() pattern or a similar pattern, when other functions it calls are changed to return util::Result.

On your latest feedback, I think I understand the scenario you are concerned about. It seems like you are worried about future uses of util::Result where sometimes completely resetting a result object is useful, and other times updating pieces of the result object is useful. And you want developers to clearly indicate their intent in both cases by calling Set() in the former case and Update() in the latter case. If developers were required to express their intent this way, it could avoid bugs when existing fields of a result object are unintentionally cleared by a Set() call, or unintentionaly left with stale values after an Update() call.

I think I might have agreed with that before writing #25722 and #29700. But after writing these PRs, I don't think a Set method would be useful in practice. The most straightforward way to write functions that return util::Result and also pass along error messages from other functions returning util::Result is to use result.Update(). The most straightforward way to write functions that just return util::Result without forwarding messages from other functions is just to construct separate result objects and not mutate existing objects. I didn't encounter scenarios where code would be clearer or safer if it reset an existing object instead just declaring a new object, or just updating the relevant fields in an existing object.

Basically, I think the result.Update() pattern is a safe, simple pattern to follow that was general enough to handle the cases I encountered in #25722 and #29700 without the need the need for set, reset, or assignment. If that statement is not convincing, I understand, and think we could move forward with a smaller PR that does not include Update(). You might also be interested to look at a random commit from #25722 and #29700 and see if there's another way you think some of that code should be written. A lot of the code in #25722 especially was not using result.Update() pattern initially and was just using separate result objects for everything.

[DrahtBot]

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