Merge 0284396a42 into a50af6e4c4

src/addrdb.cpp

@@ -188,7 +188,7 @@ void ReadFromStream(AddrMan& addr, DataStream& ssPeers)
     DeserializeDB(ssPeers, addr, false);
 }
 
-util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args)
+util::ResultPtr<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args)
 {
     auto check_addrman = std::clamp<int32_t>(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
     bool deterministic = HasTestOption(args, "addrman"); // use a deterministic addrman only for tests

src/addrdb.h

@@ -49,7 +49,7 @@ public:
 };
 
 /** Returns an error string on failure */
-util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args);
+util::ResultPtr<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args);
 
 /**
  * Dump the anchor IP address database (anchors.dat)

src/bench/wallet_create.cpp

@@ -41,19 +41,14 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
         options.create_passphrase = random.rand256().ToString();
     }
 
-    DatabaseStatus status;
-    bilingual_str error_string;
-    std::vector<bilingual_str> warnings;
-
     auto wallet_path = fs::PathToString(test_setup->m_path_root / "test_wallet");
     bench.run([&] {
-        auto wallet = CreateWallet(context, wallet_path, /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
+        auto wallet{CreateWallet(context, wallet_path, /*load_on_start=*/std::nullopt, options)};
-        assert(status == DatabaseStatus::SUCCESS);
+        assert(wallet);
-        assert(wallet != nullptr);
 
         // Release wallet
-        RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt);
+        RemoveWallet(context, wallet.value(), /*load_on_start=*/ std::nullopt);
-        WaitForDeleteWallet(std::move(wallet));
+        WaitForDeleteWallet(std::move(wallet.value()));
         fs::remove_all(wallet_path);
     });
 }

src/bitcoin-chainstate.cpp

@@ -129,13 +129,13 @@ int main(int argc, char* argv[])
     ChainstateManager chainman{interrupt, chainman_opts, blockman_opts};
 
     node::ChainstateLoadOptions options;
-    auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options);
+    auto load_result{node::LoadChainstate(chainman, cache_sizes, options)};
-    if (status != node::ChainstateLoadStatus::SUCCESS) {
+    if (!load_result) {
         std::cerr << "Failed to load Chain state from your datadir." << std::endl;
         goto epilogue;
     } else {
-        std::tie(status, error) = node::VerifyLoadedChainstate(chainman, options);
+        auto verify_result{node::VerifyLoadedChainstate(chainman, options)};
-        if (status != node::ChainstateLoadStatus::SUCCESS) {
+        if (!verify_result) {
             std::cerr << "Failed to verify loaded Chain state from your datadir." << std::endl;
             goto epilogue;
         }

src/init.cpp

@@ -120,12 +120,11 @@
 using common::AmountErrMsg;
 using common::InvalidPortErrMsg;
 using common::ResolveErrMsg;
-
+using kernel::InterruptResult;
 using node::ApplyArgsManOptions;
 using node::BlockManager;
 using node::CalculateCacheSizes;
-using node::ChainstateLoadResult;
+using node::ChainstateLoadError;
-using node::ChainstateLoadStatus;
 using node::DEFAULT_PERSIST_MEMPOOL;
 using node::DEFAULT_PRINT_MODIFIED_FEE;
 using node::DEFAULT_STOPATHEIGHT;
@@ -1221,7 +1220,7 @@ bool CheckHostPortOptions(const ArgsManager& args) {
 
 // A GUI user may opt to retry once with do_reindex set if there is a failure during chainstate initialization.
 // The function therefore has to support re-entry.
-static ChainstateLoadResult InitAndLoadChainstate(
+util::Result<kernel::InterruptResult, ChainstateLoadError> InitAndLoadChainstate(
     NodeContext& node,
     bool do_reindex,
     const bool do_reindex_chainstate,
@@ -1237,7 +1236,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
     bilingual_str mempool_error;
     node.mempool = std::make_unique<CTxMemPool>(mempool_opts, mempool_error);
     if (!mempool_error.empty()) {
-        return {ChainstateLoadStatus::FAILURE_FATAL, mempool_error};
+        return {util::Error{mempool_error}, ChainstateLoadError::FAILURE_FATAL};
     }
     LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));
     ChainstateManager::Options chainman_opts{
@@ -1267,12 +1266,12 @@ static ChainstateLoadResult InitAndLoadChainstate(
         node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown_signal), chainman_opts, blockman_opts);
     } catch (dbwrapper_error& e) {
         LogError("%s", e.what());
-        return {ChainstateLoadStatus::FAILURE, _("Error opening block database")};
+        return {util::Error{_("Error opening block database")}, ChainstateLoadError::FAILURE};
     } catch (std::exception& e) {
-        return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated(strprintf("Failed to initialize ChainstateManager: %s", e.what()))};
+        return {util::Error{Untranslated(strprintf("Failed to initialize ChainstateManager: %s", e.what()))}, ChainstateLoadError::FAILURE_FATAL};
     }
     ChainstateManager& chainman = *node.chainman;
-    if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
+    if (chainman.m_interrupt) return kernel::Interrupted{};
 
     // This is defined and set here instead of inline in validation.h to avoid a hard
     // dependency between validation and index/base, since the latter is not in
@@ -1307,27 +1306,27 @@ static ChainstateLoadResult InitAndLoadChainstate(
             "", CClientUIInterface::MSG_ERROR);
     };
     uiInterface.InitMessage(_("Loading block index…"));
-    auto catch_exceptions = [](auto&& f) -> ChainstateLoadResult {
+    auto catch_exceptions = [](auto&& f) -> util::Result<InterruptResult, node::ChainstateLoadError> {
         try {
             return f();
         } catch (const std::exception& e) {
             LogError("%s\n", e.what());
-            return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error loading databases"));
+            return {util::Error{_("Error loading databases")}, node::ChainstateLoadError::FAILURE};
         }
     };
-    auto [status, error] = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); });
+    auto result = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); });
-    if (status == node::ChainstateLoadStatus::SUCCESS) {
+    if (result && !IsInterrupted(*result)) {
         uiInterface.InitMessage(_("Verifying blocks…"));
         if (chainman.m_blockman.m_have_pruned && options.check_blocks > MIN_BLOCKS_TO_KEEP) {
             LogWarning("pruned datadir may not have more than %d blocks; only checking available blocks\n",
                        MIN_BLOCKS_TO_KEEP);
         }
-        std::tie(status, error) = catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); });
+        result.Update(catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); }));
-        if (status == node::ChainstateLoadStatus::SUCCESS) {
+        if (result && !IsInterrupted(*result)) {
             LogInfo("Block index and chainstate loaded");
         }
     }
-    return {status, error};
+    return result;
 };
 
 bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
@@ -1483,7 +1482,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         uiInterface.InitMessage(_("Loading P2P addresses…"));
         auto addrman{LoadAddrman(*node.netgroupman, args)};
         if (!addrman) return InitError(util::ErrorString(addrman));
-        node.addrman = std::move(*addrman);
+        node.addrman = std::move(addrman.value());
     }
 
     FastRandomContext rng;
@@ -1685,14 +1684,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     const bool do_reindex_chainstate{args.GetBoolArg("-reindex-chainstate", false)};
 
     // Chainstate initialization and loading may be retried once with reindexing by GUI users
-    auto [status, error] = InitAndLoadChainstate(
+    auto result = InitAndLoadChainstate(
         node,
         do_reindex,
         do_reindex_chainstate,
         kernel_cache_sizes,
         args);
-    if (status == ChainstateLoadStatus::FAILURE && !do_reindex && !ShutdownRequested(node)) {
+    if (!result && result.GetFailure() == ChainstateLoadError::FAILURE && !do_reindex && !ShutdownRequested(node)) {
         // suggest a reindex
+        const auto& error = util::ErrorString(result);
         bool do_retry = uiInterface.ThreadSafeQuestion(
             error + Untranslated(".\n\n") + _("Do you want to rebuild the databases now?"),
             error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
@@ -1704,15 +1704,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         if (!Assert(node.shutdown_signal)->reset()) {
             LogError("Internal error: failed to reset shutdown signal.\n");
         }
-        std::tie(status, error) = InitAndLoadChainstate(
+        result.Update(InitAndLoadChainstate(
             node,
             do_reindex,
             do_reindex_chainstate,
             kernel_cache_sizes,
-            args);
+            args));
     }
-    if (status != ChainstateLoadStatus::SUCCESS && status != ChainstateLoadStatus::INTERRUPTED) {
+    if (!result) {
-        return InitError(error);
+        return InitError(util::ErrorString(result));
     }
 
     // As LoadBlockIndex can take several minutes, it's possible the user

src/interfaces/wallet.h

@@ -328,16 +328,16 @@ class WalletLoader : public ChainClient
 {
 public:
     //! Create new wallet.
-    virtual util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) = 0;
+    virtual util::ResultPtr<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags) = 0;
 
     //! Load existing wallet.
-    virtual util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) = 0;
+    virtual util::ResultPtr<std::unique_ptr<Wallet>> loadWallet(const std::string& name) = 0;
 
     //! Return default wallet directory.
     virtual std::string getWalletDir() = 0;
 
     //! Restore backup wallet
-    virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
+    virtual util::ResultPtr<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name) = 0;
 
     //! Migrate a wallet
     virtual util::Result<WalletMigrationResult> migrateWallet(const std::string& name, const SecureString& passphrase) = 0;

src/kernel/CMakeLists.txt

@@ -68,6 +68,7 @@ add_library(bitcoinkernel
   ../util/hasher.cpp
   ../util/moneystr.cpp
   ../util/rbf.cpp
+  ../util/result.cpp
   ../util/serfloat.cpp
   ../util/signalinterrupt.cpp
   ../util/strencodings.cpp

src/node/chainstate.cpp

@@ -27,35 +27,40 @@
 #include <vector>
 
 using kernel::CacheSizes;
+using kernel::Interrupted;
+using kernel::InterruptResult;
 
 namespace node {
 // Complete initialization of chainstates after the initial call has been made
 // to ChainstateManager::InitializeChainstate().
-static ChainstateLoadResult CompleteChainstateInitialization(
+static util::Result<InterruptResult, ChainstateLoadError> CompleteChainstateInitialization(
     ChainstateManager& chainman,
     const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 {
-    if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
+    if (chainman.m_interrupt) return Interrupted{};
 
     // LoadBlockIndex will load m_have_pruned if we've ever removed a
     // block file from disk.
     // Note that it also sets m_blockfiles_indexed based on the disk flag!
     if (!chainman.LoadBlockIndex()) {
-        if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
+        if (chainman.m_interrupt) {
-        return {ChainstateLoadStatus::FAILURE, _("Error loading block database")};
+            return Interrupted{};
+        } else {
+            return {util::Error{_("Error loading block database")}, ChainstateLoadError::FAILURE};
+        }
     }
 
     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).
-        return {ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB, _("Incorrect or no genesis block found. Wrong datadir for network?")};
+        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) {
-        return {ChainstateLoadStatus::FAILURE, _("You need to rebuild the database using -reindex to go back to unpruned mode.  This will redownload the entire blockchain")};
+        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.
@@ -63,7 +68,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     // (otherwise we use the one already on disk).
     // This is called again in ImportBlocks after the reindex completes.
     if (chainman.m_blockman.m_blockfiles_indexed && !chainman.ActiveChainstate().LoadGenesisBlock()) {
-        return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")};
+        return {util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE};
     }
 
     auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
@@ -93,7 +98,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
                 /*should_wipe=*/options.wipe_chainstate_db);
         } catch (dbwrapper_error& err) {
             LogError("%s\n", err.what());
-            return {ChainstateLoadStatus::FAILURE, _("Error opening coins database")};
+            return {util::Error{_("Error opening coins database")}, ChainstateLoadError::FAILURE};
         }
 
         if (options.coins_error_cb) {
@@ -103,14 +108,15 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         // 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()) {
-            return {ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB, _("Unsupported chainstate database format found. "
+            return {util::Error{_("Unsupported chainstate database format found. "
                                          "Please restart with -reindex-chainstate. This will "
-                                                                     "rebuild the chainstate database.")};
+                                         "rebuild the chainstate database.")},
+                                       ChainstateLoadError::FAILURE_INCOMPATIBLE_DB};
         }
 
         // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
         if (!chainstate->ReplayBlocks()) {
-            return {ChainstateLoadStatus::FAILURE, _("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.")};
+            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
@@ -120,7 +126,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         if (!is_coinsview_empty(chainstate)) {
             // LoadChainTip initializes the chain based on CoinsTip()'s best block
             if (!chainstate->LoadChainTip()) {
-                return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")};
+                return {util::Error{_("Error initializing block database")}, ChainstateLoadError::FAILURE};
             }
             assert(chainstate->m_chain.Tip() != nullptr);
         }
@@ -129,8 +135,8 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     auto chainstates{chainman.GetAll()};
     if (std::any_of(chainstates.begin(), chainstates.end(),
                     [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
-        return {ChainstateLoadStatus::FAILURE, strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
+        return {util::Error{strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
-                                                         chainman.GetConsensus().SegwitHeight)};
+                                      chainman.GetConsensus().SegwitHeight)}, ChainstateLoadError::FAILURE};
     };
 
     // Now that chainstates are loaded and we're able to flush to
@@ -138,12 +144,13 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     // on the condition of each chainstate.
     chainman.MaybeRebalanceCaches();
 
-    return {ChainstateLoadStatus::SUCCESS, {}};
+    return {};
 }
 
-ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes,
+util::Result<InterruptResult, ChainstateLoadError> LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes,
                                                                   const ChainstateLoadOptions& options)
 {
+    util::Result<InterruptResult, ChainstateLoadError> result;
     if (!chainman.AssumedValidBlock().IsNull()) {
         LogPrintf("Assuming ancestors of block %s have valid signatures.\n", chainman.AssumedValidBlock().GetHex());
     } else {
@@ -173,13 +180,14 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     if (has_snapshot && options.wipe_chainstate_db) {
         LogPrintf("[snapshot] deleting snapshot chainstate due to reindexing\n");
         if (!chainman.DeleteSnapshotChainstate()) {
-            return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")};
+            result.Update({util::Error{Untranslated("Couldn't remove snapshot chainstate.")}, ChainstateLoadError::FAILURE_FATAL});
+            return result;
         }
     }
 
-    auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options);
+    result.Update(CompleteChainstateInitialization(chainman, options));
-    if (init_status != ChainstateLoadStatus::SUCCESS) {
+    if (!result || IsInterrupted(*result)) {
-        return {init_status, init_error};
+        return result;
     }
 
     // If a snapshot chainstate was fully validated by a background chainstate during
@@ -197,7 +205,8 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     } else if (snapshot_completion == SnapshotCompletionResult::SUCCESS) {
         LogPrintf("[snapshot] cleaning up unneeded background chainstate, then reinitializing\n");
         if (!chainman.ValidatedSnapshotCleanup()) {
-            return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Background chainstate cleanup failed unexpectedly.")};
+            result.Update({util::Error{Untranslated("Background chainstate cleanup failed unexpectedly.")}, ChainstateLoadError::FAILURE_FATAL});
+            return result;
         }
 
         // Because ValidatedSnapshotCleanup() has torn down chainstates with
@@ -213,20 +222,22 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
         // for the fully validated chainstate.
         chainman.ActiveChainstate().ClearBlockIndexCandidates();
 
-        auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options);
+        auto result{CompleteChainstateInitialization(chainman, options)};
-        if (init_status != ChainstateLoadStatus::SUCCESS) {
+        if (!result || IsInterrupted(*result)) {
-            return {init_status, init_error};
+            return result;
         }
     } else {
-        return {ChainstateLoadStatus::FAILURE_FATAL, _(
+        result.Update({util::Error{_(
            "UTXO snapshot failed to validate. "
-           "Restart to resume normal initial block download, or try loading a different snapshot.")};
+           "Restart to resume normal initial block download, or try loading a different snapshot.")},
+           ChainstateLoadError::FAILURE_FATAL});
+        return result;
     }
 
-    return {ChainstateLoadStatus::SUCCESS, {}};
+    return result;
 }
 
-ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options)
+util::Result<InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options)
 {
     auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
         return options.wipe_chainstate_db || chainstate->CoinsTip().GetBestBlock().IsNull();
@@ -234,36 +245,42 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C
 
     LOCK(cs_main);
 
+    util::Result<InterruptResult, ChainstateLoadError> result;
     for (Chainstate* chainstate : chainman.GetAll()) {
         if (!is_coinsview_empty(chainstate)) {
             const CBlockIndex* tip = chainstate->m_chain.Tip();
             if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {
-                return {ChainstateLoadStatus::FAILURE, _("The block database contains a block which appears to be from the future. "
+                result.Update({util::Error{_("The block database contains a block which appears to be from the future. "
                                              "This may be due to your computer's date and time being set incorrectly. "
-                                                         "Only rebuild the block database if you are sure that your computer's date and time are correct")};
+                                             "Only rebuild the block database if you are sure that your computer's date and time are correct")},
+                               ChainstateLoadError::FAILURE});
+                return result;
             }
 
-            VerifyDBResult result = CVerifyDB(chainman.GetNotifications()).VerifyDB(
+            VerifyDBResult verify_result = CVerifyDB(chainman.GetNotifications()).VerifyDB(
                 *chainstate, chainman.GetConsensus(), chainstate->CoinsDB(),
                 options.check_level,
                 options.check_blocks);
-            switch (result) {
+            switch (verify_result) {
             case VerifyDBResult::SUCCESS:
             case VerifyDBResult::SKIPPED_MISSING_BLOCKS:
                 break;
             case VerifyDBResult::INTERRUPTED:
-                return {ChainstateLoadStatus::INTERRUPTED, _("Block verification was interrupted")};
+                result.Update(Interrupted{});
+                return result;
             case VerifyDBResult::CORRUPTED_BLOCK_DB:
-                return {ChainstateLoadStatus::FAILURE, _("Corrupted block database detected")};
+                result.Update({util::Error{_("Corrupted block database detected")}, ChainstateLoadError::FAILURE});
+                return result;
             case VerifyDBResult::SKIPPED_L3_CHECKS:
                 if (options.require_full_verification) {
-                    return {ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE, _("Insufficient dbcache for block verification")};
+                    result.Update({util::Error{_("Insufficient dbcache for block verification")}, ChainstateLoadError::FAILURE_INSUFFICIENT_DBCACHE});
+                    return result;
                 }
                 break;
             } // no default case, so the compiler can warn about missing cases
         }
     }
 
-    return {ChainstateLoadStatus::SUCCESS, {}};
+    return result;
 }
 } // namespace node

src/node/chainstate.h

@@ -5,6 +5,8 @@
 #ifndef BITCOIN_NODE_CHAINSTATE_H
 #define BITCOIN_NODE_CHAINSTATE_H
 
+#include <kernel/notifications_interface.h>
+#include <util/result.h>
 #include <util/translation.h>
 #include <validation.h>
 
@@ -41,34 +43,16 @@ struct ChainstateLoadOptions {
 //! case, and treat other cases as errors. More complex applications may want to
 //! try reindexing in the generic failure case, and pass an interrupt callback
 //! and exit cleanly in the interrupted case.
-enum class ChainstateLoadStatus {
+enum class ChainstateLoadError {
-    SUCCESS,
     FAILURE, //!< Generic failure which reindexing may fix
     FAILURE_FATAL, //!< Fatal error which should not prompt to reindex
     FAILURE_INCOMPATIBLE_DB,
     FAILURE_INSUFFICIENT_DBCACHE,
-    INTERRUPTED,
 };
 
-//! Chainstate load status code and optional error string.
+util::Result<kernel::InterruptResult, ChainstateLoadError> LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes,
-using ChainstateLoadResult = std::tuple<ChainstateLoadStatus, bilingual_str>;
-
-/** This sequence can have 4 types of outcomes:
- *
- *  1. Success
- *  2. Shutdown requested
- *    - nothing failed but a shutdown was triggered in the middle of the
- *      sequence
- *  3. Soft failure
- *    - a failure that might be recovered from with a reindex
- *  4. Hard failure
- *    - a failure that definitively cannot be recovered from with a reindex
- *
- *  LoadChainstate returns a (status code, error string) tuple.
- */
-ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes,
                                                                           const ChainstateLoadOptions& options);
-ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options);
+util::Result<kernel::InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options);
 } // namespace node
 
 #endif // BITCOIN_NODE_CHAINSTATE_H

src/qt/walletcontroller.cpp

@@ -267,12 +267,15 @@ void CreateWalletActivity::createWallet()
     }
 
     QTimer::singleShot(500ms, worker(), [this, name, flags] {
-        auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
+        auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags)};
-
+        m_error_message.clear();
+        m_warning_message.clear();
+        if (wallet.GetMessages()) {
+            m_error_message = Join(wallet.GetMessages()->errors, Untranslated(" "));
+            m_warning_message = wallet.GetMessages()->warnings;
+        }
         if (wallet) {
-            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value()));
-        } else {
-            m_error_message = util::ErrorString(wallet);
         }
 
         QTimer::singleShot(500ms, this, &CreateWalletActivity::finish);
@@ -356,12 +359,15 @@ void OpenWalletActivity::open(const std::string& path)
         tr("Opening Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
 
     QTimer::singleShot(0, worker(), [this, path] {
-        auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
+        auto wallet{node().walletLoader().loadWallet(path)};
-
+        m_error_message.clear();
+        m_warning_message.clear();
+        if (wallet.GetMessages()) {
+            m_error_message = Join(wallet.GetMessages()->errors, Untranslated(" "));
+            m_warning_message = wallet.GetMessages()->warnings;
+        }
         if (wallet) {
-            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value()));
-        } else {
-            m_error_message = util::ErrorString(wallet);
         }
 
         QTimer::singleShot(0, this, &OpenWalletActivity::finish);
@@ -409,12 +415,15 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri
         tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
 
     QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
-        auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
+        auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name)};
-
+        m_error_message.clear();
+        m_warning_message.clear();
+        if (wallet.GetMessages()) {
+            m_error_message = Join(wallet.GetMessages()->errors, Untranslated(" "));
+            m_warning_message = wallet.GetMessages()->warnings;
+        }
         if (wallet) {
-            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value()));
-        } else {
-            m_error_message = util::ErrorString(wallet);
         }
 
         QTimer::singleShot(0, this, &RestoreWalletActivity::finish);

src/test/result_tests.cpp

@@ -2,9 +2,17 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <tinyformat.h>
+#include <util/check.h>
 #include <util/result.h>
+#include <util/translation.h>
 
+#include <algorithm>
 #include <boost/test/unit_test.hpp>
+#include <memory>
+#include <ostream>
+#include <string>
+#include <utility>
 
 inline bool operator==(const bilingual_str& a, const bilingual_str& b)
 {
@@ -33,6 +41,34 @@ std::ostream& operator<<(std::ostream& os, const NoCopy& o)
     return os << "NoCopy(" << *o.m_n << ")";
 }
 
+struct NoCopyNoMove {
+    NoCopyNoMove(int n) : m_n{n} {}
+    NoCopyNoMove(const NoCopyNoMove&) = delete;
+    NoCopyNoMove(NoCopyNoMove&&) = delete;
+    int m_n;
+};
+
+bool operator==(const NoCopyNoMove& a, const NoCopyNoMove& b)
+{
+    return a.m_n == b.m_n;
+}
+
+std::ostream& operator<<(std::ostream& os, const NoCopyNoMove& o)
+{
+    os << "NoCopyNoMove(" << o.m_n << ")";
+    return os;
+}
+
+util::Result<void> VoidSuccessFn()
+{
+    return {};
+}
+
+util::Result<void> VoidFailFn()
+{
+    return util::Error{Untranslated("void fail.")};
+}
+
 util::Result<int> IntFn(int i, bool success)
 {
     if (success) return i;
@@ -45,21 +81,91 @@ util::Result<bilingual_str> StrFn(bilingual_str s, bool success)
     return util::Error{Untranslated(strprintf("str %s error.", s.original))};
 }
 
-util::Result<NoCopy> NoCopyFn(int i, bool success)
+util::Result<NoCopy, NoCopy> NoCopyFn(int i, bool success)
 {
     if (success) return {i};
-    return util::Error{Untranslated(strprintf("nocopy %i error.", i))};
+    return {util::Error{Untranslated(strprintf("nocopy %i error.", i))}, i};
 }
 
-template <typename T>
+util::Result<NoCopyNoMove, NoCopyNoMove> NoCopyNoMoveFn(int i, bool success)
-void ExpectResult(const util::Result<T>& result, bool success, const bilingual_str& str)
+{
+    if (success) return {i};
+    return {util::Error{Untranslated(strprintf("nocopynomove %i error.", i))}, i};
+}
+
+enum FnError { ERR1, ERR2 };
+
+util::Result<int, FnError> IntFailFn(int i, bool success)
+{
+    if (success) return {util::Warning{Untranslated(strprintf("int %i warn.", i))}, i};
+    return {util::Error{Untranslated(strprintf("int %i error.", i))}, i % 2 ? ERR1 : ERR2};
+}
+
+util::Result<std::string, FnError> StrFailFn(int i, bool success)
+{
+    util::Result<std::string, FnError> result;
+    if (auto int_result{IntFailFn(i, success) >> result}) {
+        result.Update(strprintf("%i", *int_result));
+    } else {
+        result.Update({util::Error{Untranslated("str error")}, int_result.GetFailure()});
+    }
+    return result;
+}
+
+util::Result<NoCopyNoMove, FnError> EnumFailFn(FnError ret)
+{
+    return {util::Error{Untranslated("enum fail.")}, ret};
+}
+
+util::Result<void> WarnFn()
+{
+    return {util::Warning{Untranslated("warn.")}};
+}
+
+util::Result<int> MultiWarnFn(int ret)
+{
+    util::Result<int> result;
+    for (int i = 0; i < ret; ++i) {
+        result.AddWarning(Untranslated(strprintf("warn %i.", i)));
+    }
+    result.Update(ret);
+    return result;
+}
+
+util::Result<void, int> ChainedFailFn(FnError arg, int ret)
+{
+    util::Result<void, int> result{util::Error{Untranslated("chained fail.")}, ret};
+    EnumFailFn(arg) >> result;
+    WarnFn() >> result;
+    return result;
+}
+
+util::Result<int, FnError> AccumulateFn(bool success)
+{
+    util::Result<int, FnError> result;
+    util::Result<int> x = MultiWarnFn(1) >> result;
+    BOOST_REQUIRE(x);
+    util::Result<int> y = MultiWarnFn(2) >> result;
+    BOOST_REQUIRE(y);
+    result.Update(IntFailFn(*x + *y, success));
+    return result;
+}
+
+util::Result<int, int> TruthyFalsyFn(int i, bool success)
+{
+    if (success) return i;
+    return {util::Error{Untranslated(strprintf("failure value %i.", i))}, i};
+}
+
+template <typename T, typename F>
+void ExpectResult(const util::Result<T, F>& result, bool success, const bilingual_str& str)
 {
     BOOST_CHECK_EQUAL(bool(result), success);
     BOOST_CHECK_EQUAL(util::ErrorString(result), str);
 }
 
-template <typename T, typename... Args>
+template <typename T, typename F, typename... Args>
-void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args&&... args)
+void ExpectSuccess(const util::Result<T, F>& result, const bilingual_str& str, Args&&... args)
 {
     ExpectResult(result, true, str);
     BOOST_CHECK_EQUAL(result.has_value(), true);
@@ -68,20 +174,74 @@ void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args
     BOOST_CHECK_EQUAL(&result.value(), &*result);
 }
 
-template <typename T, typename... Args>
+template <typename T, typename F, typename... Args>
-void ExpectFail(const util::Result<T>& result, const bilingual_str& str)
+void ExpectFail(const util::Result<T, F>& result, bilingual_str str, Args&&... args)
 {
     ExpectResult(result, false, str);
+    F expect_failure{std::forward<Args>(args)...};
+    BOOST_CHECK_EQUAL(result.GetFailure(), expect_failure);
+}
+
+BOOST_AUTO_TEST_CASE(check_sizes)
+{
+    static_assert(sizeof(util::Result<int>) == sizeof(void*)*2);
+    static_assert(sizeof(util::Result<void>) == sizeof(void*));
 }
 
 BOOST_AUTO_TEST_CASE(check_returned)
 {
+    ExpectResult(VoidSuccessFn(), true, {});
+    ExpectResult(VoidFailFn(), false, Untranslated("void fail."));
     ExpectSuccess(IntFn(5, true), {}, 5);
-    ExpectFail(IntFn(5, false), Untranslated("int 5 error."));
+    ExpectResult(IntFn(5, false), false, Untranslated("int 5 error."));
     ExpectSuccess(NoCopyFn(5, true), {}, 5);
-    ExpectFail(NoCopyFn(5, false), Untranslated("nocopy 5 error."));
+    ExpectFail(NoCopyFn(5, false), Untranslated("nocopy 5 error."), 5);
+    ExpectSuccess(NoCopyNoMoveFn(5, true), {}, 5);
+    ExpectFail(NoCopyNoMoveFn(5, false), Untranslated("nocopynomove 5 error."), 5);
     ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));
-    ExpectFail(StrFn(Untranslated("S"), false), Untranslated("str S error."));
+    ExpectResult(StrFn(Untranslated("S"), false), false, Untranslated("str S error."));
+    ExpectSuccess(StrFailFn(1, true), Untranslated("int 1 warn."), "1");
+    ExpectFail(StrFailFn(2, false), Untranslated("int 2 error. str error"), ERR2);
+    ExpectFail(EnumFailFn(ERR2), Untranslated("enum fail."), ERR2);
+    ExpectFail(ChainedFailFn(ERR1, 5), Untranslated("chained fail. enum fail. warn."), 5);
+    ExpectSuccess(MultiWarnFn(3), Untranslated("warn 0. warn 1. warn 2."), 3);
+    ExpectSuccess(AccumulateFn(true), Untranslated("warn 0. warn 0. warn 1. int 3 warn."), 3);
+    ExpectFail(AccumulateFn(false), Untranslated("int 3 error. warn 0. warn 0. warn 1."), ERR1);
+    ExpectSuccess(TruthyFalsyFn(0, true), {}, 0);
+    ExpectFail(TruthyFalsyFn(0, false), Untranslated("failure value 0."), 0);
+    ExpectSuccess(TruthyFalsyFn(1, true), {}, 1);
+    ExpectFail(TruthyFalsyFn(1, false), Untranslated("failure value 1."), 1);
+}
+
+BOOST_AUTO_TEST_CASE(check_update)
+{
+    // Test using Update method to change a result value from success -> failure,
+    // and failure->success.
+    util::Result<int, FnError> result;
+    ExpectSuccess(result, {}, 0);
+    result.Update({util::Error{Untranslated("error")}, ERR1});
+    ExpectFail(result, Untranslated("error"), ERR1);
+    result.Update(2);
+    ExpectSuccess(result, Untranslated("error"), 2);
+
+    // Test the same thing but with non-copyable success and failure types.
+    util::Result<NoCopy, NoCopy> result2{0};
+    ExpectSuccess(result2, {}, 0);
+    result2.Update({util::Error{Untranslated("error")}, 3});
+    ExpectFail(result2, Untranslated("error"), 3);
+    result2.Update(4);
+    ExpectSuccess(result2, Untranslated("error"), 4);
+}
+
+BOOST_AUTO_TEST_CASE(check_dereference_operators)
+{
+    util::Result<std::pair<int, std::string>> mutable_result;
+    const auto& const_result{mutable_result};
+    mutable_result.value() = {1, "23"};
+    BOOST_CHECK_EQUAL(mutable_result->first, 1);
+    BOOST_CHECK_EQUAL(const_result->second, "23");
+    (*mutable_result).first = 5;
+    BOOST_CHECK_EQUAL((*const_result).first, 5);
 }
 
 BOOST_AUTO_TEST_CASE(check_value_or)
@@ -94,4 +254,63 @@ BOOST_AUTO_TEST_CASE(check_value_or)
     BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B"));
 }
 
+BOOST_AUTO_TEST_CASE(check_message_accessors)
+{
+    util::Result<void> result{util::Error{Untranslated("Error.")}, util::Warning{Untranslated("Warning.")}};
+    BOOST_CHECK_EQUAL(Assert(result.GetMessages())->errors.size(), 1);
+    BOOST_CHECK_EQUAL(Assert(result.GetMessages())->errors[0], Untranslated("Error."));
+    BOOST_CHECK_EQUAL(Assert(result.GetMessages())->warnings.size(), 1);
+    BOOST_CHECK_EQUAL(Assert(result.GetMessages())->warnings[0], Untranslated("Warning."));
+    BOOST_CHECK_EQUAL(util::ErrorString(result), Untranslated("Error. Warning."));
+}
+
+struct Derived : NoCopyNoMove {
+    using NoCopyNoMove::NoCopyNoMove;
+};
+
+util::Result<std::unique_ptr<NoCopyNoMove>> DerivedToBaseFn(util::Result<std::unique_ptr<Derived>> derived)
+{
+    return derived;
+}
+
+BOOST_AUTO_TEST_CASE(derived_to_base)
+{
+    // Check derived to base conversions work for util::Result
+    BOOST_CHECK_EQUAL(*Assert(*Assert(DerivedToBaseFn(std::make_unique<Derived>(5)))), 5);
+
+    // Check conversions work between util::Result and util::ResultPtr
+    util::ResultPtr<std::unique_ptr<Derived>> derived{std::make_unique<Derived>(5)};
+    util::ResultPtr<std::unique_ptr<NoCopyNoMove>> base{DerivedToBaseFn(std::move(derived))};
+    BOOST_CHECK_EQUAL(*Assert(base), 5);
+}
+
+//! For testing ResultPtr, return pointer to a pair of ints, or return pointer to null, or return an error message.
+util::ResultPtr<std::unique_ptr<std::pair<int, int>>> PtrFn(std::optional<std::pair<int, int>> i, bool success)
+{
+    if (success) return i ? std::make_unique<std::pair<int, int>>(*i) : nullptr;
+    return util::Error{Untranslated(strprintf("PtrFn(%s) error.", i ? strprintf("%i, %i", i->first, i->second) : "nullopt"))};
+}
+
+BOOST_AUTO_TEST_CASE(check_ptr)
+{
+    auto result_pair = PtrFn(std::pair{1, 2}, true);
+    ExpectResult(result_pair, true, {});
+    BOOST_CHECK(result_pair);
+    BOOST_CHECK_EQUAL(result_pair->first, 1);
+    BOOST_CHECK_EQUAL(result_pair->second, 2);
+    BOOST_CHECK(*result_pair == std::pair(1,2));
+
+    auto result_null = PtrFn(std::nullopt, true);
+    ExpectResult(result_null, true, {});
+    BOOST_CHECK(!result_null);
+
+    auto result_error_pair = PtrFn(std::pair{1, 2}, false);
+    ExpectResult(result_error_pair, false, Untranslated("PtrFn(1, 2) error."));
+    BOOST_CHECK(!result_error_pair);
+
+    auto result_error_null = PtrFn(std::nullopt, false);
+    ExpectResult(result_error_null, false, Untranslated("PtrFn(nullopt) error."));
+    BOOST_CHECK(!result_error_null);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/test/util/setup_common.cpp

@@ -290,11 +290,11 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
     options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
     options.check_level = m_args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL);
     options.require_full_verification = m_args.IsArgSet("-checkblocks") || m_args.IsArgSet("-checklevel");
-    auto [status, error] = LoadChainstate(chainman, m_kernel_cache_sizes, options);
+    auto load_result{LoadChainstate(chainman, m_kernel_cache_sizes, options)};
-    assert(status == node::ChainstateLoadStatus::SUCCESS);
+    Assert(load_result);
 
-    std::tie(status, error) = VerifyLoadedChainstate(chainman, options);
+    auto verify_result{VerifyLoadedChainstate(chainman, options)};
-    assert(status == node::ChainstateLoadStatus::SUCCESS);
+    Assert(verify_result);
 
     BlockValidationState state;
     if (!chainman.ActiveChainstate().ActivateBestChain(state)) {

src/util/CMakeLists.txt

@@ -17,6 +17,7 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL
   moneystr.cpp
   rbf.cpp
   readwritefile.cpp
+  result.cpp
   serfloat.cpp
   signalinterrupt.cpp
   sock.cpp

src/util/result.cpp

@@ -0,0 +1,33 @@
+// Copyright (c) 2024 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <util/result.h>
+
+#include <algorithm>
+#include <initializer_list>
+#include <iterator>
+#include <util/translation.h>
+
+namespace util {
+namespace detail {
+bilingual_str JoinMessages(const Messages& messages)
+{
+    bilingual_str result;
+    for (const auto& messages : {messages.errors, messages.warnings}) {
+        for (const auto& message : messages) {
+            if (!result.empty()) result += Untranslated(" ");
+            result += message;
+        }
+    }
+    return result;
+}
+} // namespace detail
+
+void ResultTraits<Messages>::Update(Messages& dst, Messages& src) {
+    dst.errors.insert(dst.errors.end(), std::make_move_iterator(src.errors.begin()), std::make_move_iterator(src.errors.end()));
+    dst.warnings.insert(dst.warnings.end(), std::make_move_iterator(src.warnings.begin()), std::make_move_iterator(src.warnings.end()));
+    src.errors.clear();
+    src.warnings.clear();
+}
+} // namespace util

src/util/result.h

@@ -8,91 +8,425 @@
 #include <attributes.h>
 #include <util/translation.h>
 
-#include <variant>
+#include <cassert>
+#include <memory>
+#include <new>
+#include <optional>
+#include <type_traits>
+#include <utility>
+#include <vector>
 
 namespace util {
+//! Default MessagesType, simple list of errors and warnings.
+struct Messages {
+    std::vector<bilingual_str> errors{};
+    std::vector<bilingual_str> warnings{};
+};
 
+//! The Result<SuccessType, FailureType, MessagesType> class provides
+//! an efficient way for functions to return structured result information, as
+//! well as descriptive error and warning messages.
+//!
+//! Logically, a result object is equivalent to:
+//!
+//!     tuple<variant<SuccessType, FailureType>, MessagesType>
+//!
+//! But the physical representation is more efficient because it avoids
+//! allocating memory for FailureType and MessagesType fields unless there is an
+//! error.
+//!
+//! Result<SuccessType> objects support the same operators as
+//! std::optional<SuccessType>, such as !result, *result, result->member, to
+//! make SuccessType values easy to access. They also provide
+//! result.GetFailure() and result.GetMessages() methods to
+//! access other parts of the result. A simple usage example is:
+//!
+//!    util::Result<int> AddNumbers(int a, int b)
+//!    {
+//!        if (b == 0) return util::Error{_("Not adding 0, that's dumb.")};
+//!        return a + b;
+//!    }
+//!
+//!    void TryAddNumbers(int a, int b)
+//!    {
+//!        if (auto result = AddNumbers(a, b)) {
+//!            LogPrintf("%i + %i = %i\n", a, b, *result);
+//!        } else {
+//!            LogPrintf("Error: %s\n", util::ErrorString(result).translated);
+//!        }
+//!    }
+//!
+//! The `Result` class is intended to be used for high-level functions that need
+//! to report error messages to end users. Low-level functions that don't need
+//! error-reporting and only need error-handling should avoid `Result` and
+//! instead use standard classes like `std::optional`, `std::variant`,
+//! `std::expected`, and `std::tuple`, or custom structs and enum types to
+//! return function results.
+//!
+//! Usage examples can be found in \example ../test/result_tests.cpp.
+template <typename SuccessType = void, typename FailureType = void, typename MessagesType = Messages>
+class Result;
+
+//! Wrapper object to pass an error string to the Result constructor.
 struct Error {
     bilingual_str message;
 };
+//! Wrapper object to pass a warning string to the Result constructor.
+struct Warning {
+    bilingual_str message;
+};
 
-//! The util::Result class provides a standard way for functions to return
+//! Type trait that can be specialized to control the way SuccessType /
-//! either error messages or result values.
+//! FailureType / MessagesType values are combined. Default behavior
-//!
+//! is for new values to overwrite existing ones, but this can be specialized
-//! It is intended for high-level functions that need to report error strings to
+//! for custom behavior when Result::Update() method is called or << operator is
-//! end users. Lower-level functions that don't need this error-reporting and
+//! used. For example, this is specialized for Messages struct below to append
-//! only need error-handling should avoid util::Result and instead use standard
+//! error and warning messages instead of overwriting them. It can also be used,
-//! classes like std::optional, std::variant, and std::tuple, or custom structs
+//! for example, to merge FailureType values when a function can return multiple
-//! and enum types to return function results.
+//! failures.
-//!
+template<typename T>
-//! Usage examples can be found in \example ../test/result_tests.cpp, but in
+struct ResultTraits {
-//! general code returning `util::Result<T>` values is very similar to code
+    template<typename O>
-//! returning `std::optional<T>` values. Existing functions returning
+    static void Update(O& dst, T& src)
-//! `std::optional<T>` can be updated to return `util::Result<T>` and return
-//! error strings usually just replacing `return std::nullopt;` with `return
-//! util::Error{error_string};`.
-template <class M>
-class Result
     {
-private:
+        dst = std::move(src);
-    using T = std::conditional_t<std::is_same_v<M, void>, std::monostate, M>;
+    }
+};
 
-    std::variant<bilingual_str, T> m_variant;
+//! Type trait that can be specialized to let the Result class work with custom
+//! MessagesType types holding error and warning messages.
+template<typename MessagesType>
+struct MessagesTraits;
 
-    //! Disallow copy constructor, require Result to be moved for efficiency.
+//! ResultTraits specialization for Messages struct.
-    Result(const Result&) = delete;
+template<>
+struct ResultTraits<Messages> {
+    static void Update(Messages& dst, Messages& src);
+};
 
-    //! Disallow operator= to avoid confusion in the future when the Result
+//! MessagesTraits specialization for Messages struct.
-    //! class gains support for richer error reporting, and callers should have
+template<>
-    //! ability to set a new result value without clearing existing error
+struct MessagesTraits<Messages> {
-    //! messages.
+    static void AddError(Messages& messages, bilingual_str error)
-    Result& operator=(const Result&) = delete;
+    {
-    Result& operator=(Result&&) = delete;
+        messages.errors.emplace_back(std::move(error));
+    }
+    static void AddWarning(Messages& messages, bilingual_str warning)
+    {
+        messages.warnings.emplace_back(std::move(warning));
+    }
+    static bool HasMessages(const Messages& messages)
+    {
+        return messages.errors.size() || messages.warnings.size();
+    }
+};
 
-    template <typename FT>
+namespace detail {
-    friend bilingual_str ErrorString(const Result<FT>& result);
+//! Helper function to join messages in space separated string.
+bilingual_str JoinMessages(const Messages& messages);
+
+//! Substitute for std::monostate that doesn't depend on std::variant.
+struct Monostate{};
+
+//! Implemention note: Result class inherits from a FailDataHolder class holding
+//! a unique_ptr to FailureType and MessagesTypes values, and a SuccessHolder
+//! class holding a SuccessType value in an anonymous union.
+//! @{
+//! Container for FailureType and MessagesType, providing public operator
+//! bool(), GetFailure(), GetMessages(), and EnsureMessages() methods.
+template <typename FailureType, typename MessagesType>
+class FailDataHolder
+{
+protected:
+    struct FailData {
+        std::optional<std::conditional_t<std::is_same_v<FailureType, void>, Monostate, FailureType>> failure{};
+        MessagesType messages{};
+    };
+    std::unique_ptr<FailData> m_fail_data;
+
+    // Private accessor, create FailData if it doesn't exist.
+    FailData& EnsureFailData() LIFETIMEBOUND
+    {
+        if (!m_fail_data) m_fail_data = std::make_unique<FailData>();
+        return *m_fail_data;
+    }
 
 public:
-    Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {}  // constructor for void
+    // Public accessors.
-    Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}
+    explicit operator bool() const { return !m_fail_data || !m_fail_data->failure; }
-    Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {}
+    const auto& GetFailure() const LIFETIMEBOUND { assert(!*this); return *m_fail_data->failure; }
-    Result(Result&&) = default;
+    auto& GetFailure() LIFETIMEBOUND { assert(!*this); return *m_fail_data->failure; }
-    ~Result() = default;
+    const auto* GetMessages() const LIFETIMEBOUND { return m_fail_data ? &m_fail_data->messages : nullptr; }
+    auto* GetMessages() LIFETIMEBOUND { return m_fail_data ? &m_fail_data->messages : nullptr; }
+    auto& EnsureMessages() LIFETIMEBOUND { return EnsureFailData().messages; }
+};
 
-    //! std::optional methods, so functions returning optional<T> can change to
+//! Container for SuccessType, providing public accessor methods similar to
-    //! return Result<T> with minimal changes to existing code, and vice versa.
+//! std::optional methods to access the success value.
-    bool has_value() const noexcept { return m_variant.index() == 1; }
+template <typename SuccessType, typename FailureType, typename MessagesType>
-    const T& value() const LIFETIMEBOUND
+class SuccessHolder : public FailDataHolder<FailureType, MessagesType>
 {
-        assert(has_value());
+protected:
-        return std::get<1>(m_variant);
+    //! Success value embedded in an anonymous union so it doesn't need to be
-    }
+    //! constructed if the result is holding a failure value,
-    T& value() LIFETIMEBOUND
+    union { SuccessType m_success; };
-    {
+
-        assert(has_value());
+    //! Empty constructor that needs to be declared because the class contains a union.
-        return std::get<1>(m_variant);
+    SuccessHolder() {}
-    }
+    ~SuccessHolder() { if (*this) m_success.~SuccessType(); }
+
+public:
+    // Public accessors.
+    bool has_value() const { return bool{*this}; }
+    const SuccessType& value() const LIFETIMEBOUND { assert(has_value()); return m_success; }
+    SuccessType& value() LIFETIMEBOUND { assert(has_value()); return m_success; }
     template <class U>
-    T value_or(U&& default_value) const&
+    SuccessType value_or(U&& default_value) const&
     {
         return has_value() ? value() : std::forward<U>(default_value);
     }
     template <class U>
-    T value_or(U&& default_value) &&
+    SuccessType value_or(U&& default_value) &&
     {
         return has_value() ? std::move(value()) : std::forward<U>(default_value);
     }
-    explicit operator bool() const noexcept { return has_value(); }
+    const SuccessType* operator->() const LIFETIMEBOUND { return &value(); }
-    const T* operator->() const LIFETIMEBOUND { return &value(); }
+    const SuccessType& operator*() const LIFETIMEBOUND { return value(); }
-    const T& operator*() const LIFETIMEBOUND { return value(); }
+    SuccessType* operator->() LIFETIMEBOUND { return &value(); }
-    T* operator->() LIFETIMEBOUND { return &value(); }
+    SuccessType& operator*() LIFETIMEBOUND { return value(); }
-    T& operator*() LIFETIMEBOUND { return value(); }
 };
 
-template <typename T>
+//! Specialization of SuccessHolder when SuccessType is void.
-bilingual_str ErrorString(const Result<T>& result)
+template <typename FailureType, typename MessagesType>
+class SuccessHolder<void, FailureType, MessagesType> : public FailDataHolder<FailureType, MessagesType>
 {
-    return result ? bilingual_str{} : std::get<0>(result.m_variant);
+};
+//! @}
+} // namespace detail
+
+// Result type class, documented at the top of this file.
+template <typename SuccessType_, typename FailureType_, typename MessagesType_>
+class Result : public detail::SuccessHolder<SuccessType_, FailureType_, MessagesType_>
+{
+public:
+    using SuccessType = SuccessType_;
+    using FailureType = FailureType_;
+    using MessagesType = MessagesType_;
+    static constexpr bool is_result{true};
+
+    //! Construct a Result object setting a success or failure value and
+    //! optional warning and error messages. Initial util::Error and
+    //! util::Warning arguments are processed first to add warning and error
+    //! messages. Then, any remaining arguments are passed to the SuccessType
+    //! constructor and used to construct a success value in the success case.
+    //! In the failure case, if any util::Error arguments were passed, any
+    //! remaining arguments are passed to the FailureType constructor and used
+    //! to construct a failure value.
+    template <typename... Args>
+    Result(Args&&... args)
+    {
+        Construct</*Failure=*/false>(*this, std::forward<Args>(args)...);
+    }
+
+    //! Move-construct a Result object from another Result object, moving the
+    //! success or failure value and any error or warning messages.
+    template <typename O>
+    requires (std::decay_t<O>::is_result)
+    Result(O&& other)
+    {
+        Move</*Constructed=*/false>(*this, other);
+    }
+
+    //! Update this result by moving from another result object. Existing
+    //! success, failure, and messages values are updated (using
+    //! ResultTraits::Update specializations), so errors and warning messages
+    //! get appended instead of overwriting existing ones.
+    Result& Update(Result&& other) LIFETIMEBOUND
+    {
+        Move</*Constructed=*/true>(*this, other);
+        return *this;
+    }
+
+    //! Disallow operator= and require explicit Result::Update() calls to avoid
+    //! accidentally clearing error and warning messages when combining results.
+    Result& operator=(Result&&) = delete;
+
+    void AddError(bilingual_str error)
+    {
+        if (!error.empty()) MessagesTraits<MessagesType>::AddError(this->EnsureFailData().messages, std::move(error));
+    }
+    void AddWarning(bilingual_str warning)
+    {
+        if (!warning.empty()) MessagesTraits<MessagesType>::AddWarning(this->EnsureFailData().messages, std::move(warning));
+    }
+
+protected:
+    template <typename, typename, typename>
+    friend class Result;
+
+    //! Helper function to construct a new success or failure value using the
+    //! arguments provided.
+    template <bool Failure, typename Result, typename... Args>
+    static void Construct(Result& result, Args&&... args)
+    {
+        if constexpr (Failure) {
+            static_assert(sizeof...(args) > 0 || !std::is_scalar_v<FailureType>,
+                "Refusing to default-construct failure value with int, float, enum, or pointer type, please specify an explicit failure value.");
+            result.EnsureFailData().failure.emplace(std::forward<Args>(args)...);
+        } else if constexpr (std::is_same_v<typename Result::SuccessType, void>) {
+            static_assert(sizeof...(args) == 0, "Error: Arguments cannot be passed to a Result<void> constructor.");
+        } else {
+            new (&result.m_success) typename Result::SuccessType{std::forward<Args>(args)...};
+        }
+    }
+
+    //! Construct() overload peeling off a util::Error constructor argument.
+    template <bool Failure, typename Result, typename... Args>
+    static void Construct(Result& result, util::Error error, Args&&... args)
+    {
+        result.AddError(std::move(error.message));
+        Construct</*Failure=*/true>(result, std::forward<Args>(args)...);
+    }
+
+    //! Construct() overload peeling off a util::Warning constructor argument.
+    template <bool Failure, typename Result, typename... Args>
+    static void Construct(Result& result, util::Warning warning, Args&&... args)
+    {
+        result.AddWarning(std::move(warning.message));
+        Construct<Failure>(result, std::forward<Args>(args)...);
+    }
+
+    //! Move success, failure, and messages from source Result object to
+    //! destination object. Existing values are updated (using
+    //! ResultTraits::Update specializations), so destination errors and warning
+    //! messages get appended to instead of overwritten. The source and
+    //! destination results are not required to have the same types, and
+    //! assigning void source types to non-void destinations type is allowed,
+    //! since no source information is lost. But assigning non-void source types
+    //! to void destination types is not allowed, since this would discard
+    //! source information.
+    template <bool DstConstructed, typename DstResult, typename SrcResult>
+    static void Move(DstResult& dst, SrcResult& src)
+    {
+        // Use operator>> to move messages value first, then move
+        // success or failure value below.
+        src >> dst;
+        // If DstConstructed is true, it means dst has either a success value or
+        // a failure value set, which needs to be updated or replaced. If
+        // DstConstructed is false, then dst is being constructed now and has no
+        // values set.
+        if constexpr (DstConstructed) {
+            if (dst && src) {
+                // dst and src both hold success values, so update dst from src and return
+                if constexpr (!std::is_same_v<typename SrcResult::SuccessType, void>) {
+                    ResultTraits<typename SrcResult::SuccessType>::Update(*dst, *src);
+                }
+                return;
+            } else if (!dst && !src) {
+                // dst and src both hold failure values, so update dst from src and return
+                if constexpr (!std::is_same_v<typename SrcResult::FailureType, void>) {
+                    ResultTraits<typename SrcResult::FailureType>::Update(dst.GetFailure(), src.GetFailure());
+                }
+                return;
+            } else if (dst) {
+                // dst has a success value, so destroy it before replacing it with src failure value
+                if constexpr (!std::is_same_v<typename DstResult::SuccessType, void>) {
+                    using DstSuccess = typename DstResult::SuccessType;
+                    dst.m_success.~DstSuccess();
+                }
+            } else {
+                // dst has a failure value, so reset it before replacing it with src success value
+                dst.m_fail_data->failure.reset();
+            }
+        }
+        // At this point dst has no success or failure value set. Can assert
+        // there is no failure value.
+        assert(dst);
+        if (src) {
+            // src has a success value, so move it to dst. If the src success
+            // type is void and the dst success type is non-void, just
+            // initialize the dst success value by default initialization.
+            if constexpr (!std::is_same_v<typename SrcResult::SuccessType, void>) {
+               new (&dst.m_success) typename DstResult::SuccessType{std::move(src.m_success)};
+            } else if constexpr (!std::is_same_v<typename DstResult::SuccessType, void>) {
+               new (&dst.m_success) typename DstResult::SuccessType{};
+            }
+            assert(dst);
+        } else {
+            // src has a failure value, so move it to dst. If the src failure
+            // type is void, just initialize the dst failure value by default
+            // initialization.
+            if constexpr (!std::is_same_v<typename SrcResult::FailureType, void>) {
+                dst.EnsureFailData().failure.emplace(std::move(src.GetFailure()));
+            } else {
+                dst.EnsureFailData().failure.emplace();
+            }
+            assert(!dst);
+        }
+    }
+};
+
+//! Move information from a source Result object to a destination object. It
+//! only moves MessagesType values without affecting SuccessType or
+//! FailureType values of either Result object.
+//!
+//! This is useful for combining error and warning messages from multiple result
+//! objects into a single object, e.g.:
+//!
+//!    util::Result<void> result;
+//!    auto r1 = DoSomething() >> result;
+//!    auto r2 = DoSomethingElse() >> result;
+//!    ...
+//!    return result;
+//!
+template <typename SrcResult, typename DstResult>
+requires (std::decay_t<SrcResult>::is_result)
+decltype(auto) operator>>(SrcResult&& src LIFETIMEBOUND, DstResult&& dst)
+{
+    using SrcType = std::decay_t<SrcResult>;
+    if (src.GetMessages() && MessagesTraits<typename SrcType::MessagesType>::HasMessages(*src.GetMessages())) {
+        ResultTraits<typename SrcType::MessagesType>::Update(dst.EnsureMessages(), *src.GetMessages());
+    }
+    return static_cast<SrcType&&>(src);
+}
+
+//! Wrapper around util::Result that is less awkward to use with pointer types.
+//!
+//! It overloads pointer and bool operators so it isn't necessary to dereference
+//! the result object twice to access the result value, so it possible to call
+//! methods with `result->Method()` rather than `(*result)->Method()` and check
+//! whether the pointer is null with `if (result)` rather than `if (result &&
+//! *result)`.
+//!
+//! The `ResultPtr` class just adds syntax sugar to `Result` class. It is still
+//! possible to access the result pointer directly using `ResultPtr` `value()`
+//! and `has_value()` methods.
+template <typename T, typename F = void>
+class ResultPtr : public Result<T, F>
+{
+public:
+    // Inherit Result constructors (excluding copy and move constructors).
+    using Result<T, F>::Result;
+
+    // Inherit Result copy and move constructors.
+    template<typename O>
+    ResultPtr(O&& other) : Result<T, F>{std::forward<O>(other)} {}
+
+    explicit operator bool() const noexcept { return this->has_value() && this->value(); }
+    auto* operator->() const { assert(this->value()); return &*this->value(); }
+    auto& operator*() const { assert(this->value()); return *this->value(); }
+};
+
+//! Join error and warning messages in a space separated string. This is
+//! intended for simple applications where there's probably only one error or
+//! warning message to report, but multiple messages should not be lost if they
+//! are present. More complicated applications should use Result::GetMessages()
+//! method directly.
+template <typename Result>
+bilingual_str ErrorString(const Result& result)
+{
+    const auto* messages{result.GetMessages()};
+    return messages ? detail::JoinMessages(*messages) : bilingual_str{};
 }
 } // namespace util
 

src/wallet/bdb.cpp

@@ -143,18 +143,17 @@ BerkeleyEnvironment::~BerkeleyEnvironment()
     Close();
 }
 
-bool BerkeleyEnvironment::Open(bilingual_str& err)
+util::Result<void> BerkeleyEnvironment::Open()
 {
     if (fDbEnvInit) {
-        return true;
+        return {};
     }
 
     fs::path pathIn = fs::PathFromString(strPath);
     TryCreateDirectories(pathIn);
     if (util::LockDirectory(pathIn, ".walletlock") != util::LockResult::Success) {
         LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance may be using it.\n", strPath);
-        err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory())));
+        return {util::Error{strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory())))}};
-        return false;
     }
 
     fs::path pathLogDir = pathIn / "database";
@@ -194,16 +193,16 @@ bool BerkeleyEnvironment::Open(bilingual_str& err)
             LogPrintf("BerkeleyEnvironment::Open: Error %d closing failed database environment: %s\n", ret2, DbEnv::strerror(ret2));
         }
         Reset();
-        err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory())));
+        auto err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory())));
         if (ret == DB_RUNRECOVERY) {
             err += Untranslated(" ") + _("This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newer version of Berkeley DB. If so, please use the software that last loaded this wallet");
         }
-        return false;
+        return {util::Error{std::move(err)}};
     }
 
     fDbEnvInit = true;
     fMockDb = false;
-    return true;
+    return {};
 }
 
 //! Construct an in-memory mock Berkeley environment for testing
@@ -312,7 +311,7 @@ BerkeleyDatabase::BerkeleyDatabase(std::shared_ptr<BerkeleyEnvironment> env, fs:
     assert(inserted.second);
 }
 
-bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
+util::Result<void> BerkeleyDatabase::Verify()
 {
     fs::path walletDir = env->Directory();
     fs::path file_path = walletDir / m_filename;
@@ -320,9 +319,8 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
     LogPrintf("Using BerkeleyDB version %s\n", BerkeleyDatabaseVersion());
     LogPrintf("Using wallet %s\n", fs::PathToString(file_path));
 
-    if (!env->Open(errorStr)) {
+    util::Result<void> opened = env->Open();
-        return false;
+    if (!opened) return opened;
-    }
 
     if (fs::exists(file_path))
     {
@@ -332,12 +330,11 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
         const std::string strFile = fs::PathToString(m_filename);
         int result = db.verify(strFile.c_str(), nullptr, nullptr, 0);
         if (result != 0) {
-            errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), fs::quoted(fs::PathToString(file_path)));
+            return {util::Error{strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), fs::quoted(fs::PathToString(file_path)))}};
-            return false;
         }
     }
     // also return true if files does not exists
-    return true;
+    return {};
 }
 
 void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile)
@@ -377,9 +374,10 @@ void BerkeleyDatabase::Open()
 
     {
         LOCK(cs_db);
-        bilingual_str open_err;
+        auto opened = env->Open();
-        if (!env->Open(open_err))
+        if (!opened) {
-            throw std::runtime_error("BerkeleyDatabase: Failed to open database environment.");
+            throw std::runtime_error("BerkeleyDatabase: Failed to open database environment. " + util::ErrorString(opened).original);
+        }
 
         if (m_db == nullptr) {
             int ret;
@@ -499,8 +497,8 @@ void BerkeleyEnvironment::ReloadDbEnv()
     // Reset the environment
     Flush(true); // This will flush and close the environment
     Reset();
-    bilingual_str open_err;
+    auto opened = Open();
-    Open(open_err);
+    if (!opened) LogPrintf("BerkeleyEnvironment::ReloadDbEnv Open failed: %s\n", util::ErrorString(opened).original);
 }
 
 DbTxn* BerkeleyEnvironment::TxnBegin(int flags)
@@ -945,8 +943,10 @@ std::unique_ptr<DatabaseBatch> BerkeleyDatabase::MakeBatch(bool flush_on_close)
     return std::make_unique<BerkeleyBatch>(*this, false, flush_on_close);
 }
 
-std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error)
+util::ResultPtr<std::unique_ptr<BerkeleyDatabase>, DatabaseError> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options)
 {
+    util::ResultPtr<std::unique_ptr<BerkeleyDatabase>, DatabaseError> result;
+
     fs::path data_file = BDBDataFile(path);
     std::unique_ptr<BerkeleyDatabase> db;
     {
@@ -954,19 +954,19 @@ std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, con
         fs::path data_filename = data_file.filename();
         std::shared_ptr<BerkeleyEnvironment> env = GetBerkeleyEnv(data_file.parent_path(), options.use_shared_memory);
         if (env->m_databases.count(data_filename)) {
-            error = Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", fs::PathToString(env->Directory() / data_filename)));
+            result.Update({util::Error{Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", fs::PathToString(env->Directory() / data_filename)))},
-            status = DatabaseStatus::FAILED_ALREADY_LOADED;
+                                       DatabaseError::FAILED_ALREADY_LOADED});
-            return nullptr;
+            return result;
         }
         db = std::make_unique<BerkeleyDatabase>(std::move(env), std::move(data_filename), options);
     }
 
-    if (options.verify && !db->Verify(error)) {
+    if (options.verify && !(db->Verify() >> result)) {
-        status = DatabaseStatus::FAILED_VERIFY;
+        result.Update({util::Error{}, DatabaseError::FAILED_VERIFY});
-        return nullptr;
+        return result;
     }
 
-    status = DatabaseStatus::SUCCESS;
+    result.Update(std::move(db));
-    return db;
+    return result;
 }
 } // namespace wallet

src/wallet/bdb.h

@@ -11,6 +11,7 @@
 #include <serialize.h>
 #include <streams.h>
 #include <util/fs.h>
+#include <util/result.h>
 #include <wallet/db.h>
 
 #include <atomic>
@@ -66,7 +67,7 @@ public:
     bool IsInitialized() const { return fDbEnvInit; }
     fs::path Directory() const { return fs::PathFromString(strPath); }
 
-    bool Open(bilingual_str& error);
+    util::Result<void> Open();
     void Close();
     void Flush(bool fShutdown);
     void CheckpointLSN(const std::string& strFile);
@@ -127,7 +128,7 @@ public:
     void ReloadDbEnv() override;
 
     /** Verifies the environment and database file */
-    bool Verify(bilingual_str& error);
+    util::Result<void> Verify();
 
     /** Return path to main database filename */
     std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); }
@@ -219,7 +220,7 @@ std::string BerkeleyDatabaseVersion();
 bool BerkeleyDatabaseSanityCheck();
 
 //! Return object giving access to Berkeley database at specified path.
-std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+util::ResultPtr<std::unique_ptr<BerkeleyDatabase>, DatabaseError> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_BDB_H

src/wallet/db.h

@@ -10,6 +10,7 @@
 #include <streams.h>
 #include <support/allocators/secure.h>
 #include <util/fs.h>
+#include <util/result.h>
 
 #include <atomic>
 #include <memory>
@@ -202,8 +203,7 @@ struct DatabaseOptions {
     int64_t max_log_mb = 100;       //!< Max log size to allow before consolidating.
 };
 
-enum class DatabaseStatus {
+enum class DatabaseError {
-    SUCCESS,
     FAILED_BAD_PATH,
     FAILED_BAD_FORMAT,
     FAILED_ALREADY_LOADED,
@@ -220,7 +220,7 @@ enum class DatabaseStatus {
 std::vector<std::pair<fs::path, std::string>> ListDatabases(const fs::path& path);
 
 void ReadDatabaseArgs(const ArgsManager& args, DatabaseOptions& options);
-std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+util::ResultPtr<std::unique_ptr<WalletDatabase>, DatabaseError> MakeDatabase(const fs::path& path, const DatabaseOptions& options);
 
 fs::path BDBDataFile(const fs::path& path);
 fs::path SQLiteDataFile(const fs::path& path);

src/wallet/dump.cpp

@@ -21,37 +21,36 @@ namespace wallet {
 static const std::string DUMP_MAGIC = "BITCOIN_CORE_WALLET_DUMP";
 uint32_t DUMP_VERSION = 1;
 
-bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error)
+util::Result<void> DumpWallet(const ArgsManager& args, WalletDatabase& db)
 {
+    util::Result<void> result;
     // Get the dumpfile
     std::string dump_filename = args.GetArg("-dumpfile", "");
     if (dump_filename.empty()) {
-        error = _("No dump file provided. To use dump, -dumpfile=<filename> must be provided.");
+        result.Update(util::Error{_("No dump file provided. To use dump, -dumpfile=<filename> must be provided.")});
-        return false;
+        return result;
     }
 
     fs::path path = fs::PathFromString(dump_filename);
     path = fs::absolute(path);
     if (fs::exists(path)) {
-        error = strprintf(_("File %s already exists. If you are sure this is what you want, move it out of the way first."), fs::PathToString(path));
+        result.Update(util::Error{strprintf(_("File %s already exists. If you are sure this is what you want, move it out of the way first."), fs::PathToString(path))});
-        return false;
+        return result;
     }
     std::ofstream dump_file;
     dump_file.open(path);
     if (dump_file.fail()) {
-        error = strprintf(_("Unable to open %s for writing"), fs::PathToString(path));
+        result.Update(util::Error{strprintf(_("Unable to open %s for writing"), fs::PathToString(path))});
-        return false;
+        return result;
     }
 
     HashWriter hasher{};
 
     std::unique_ptr<DatabaseBatch> batch = db.MakeBatch();
 
-    bool ret = true;
     std::unique_ptr<DatabaseCursor> cursor = batch->GetNewCursor();
     if (!cursor) {
-        error = _("Error: Couldn't create cursor into database");
+        result.Update(util::Error{_("Error: Couldn't create cursor into database")});
-        ret = false;
     }
 
     // Write out a magic string with version
@@ -70,7 +69,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro
     dump_file.write(line.data(), line.size());
     hasher << Span{line};
 
-    if (ret) {
+    if (result) {
 
         // Read the records
         while (true) {
@@ -78,11 +77,10 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro
             DataStream ss_value{};
             DatabaseCursor::Status status = cursor->Next(ss_key, ss_value);
             if (status == DatabaseCursor::Status::DONE) {
-                ret = true;
+                result.Update({});
                 break;
             } else if (status == DatabaseCursor::Status::FAIL) {
-                error = _("Error reading next record from wallet database");
+                result.Update(util::Error{_("Error reading next record from wallet database")});
-                ret = false;
                 break;
             }
             std::string key_str = HexStr(ss_key);
@@ -96,7 +94,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro
     cursor.reset();
     batch.reset();
 
-    if (ret) {
+    if (result) {
         // Write the hash
         tfm::format(dump_file, "checksum,%s\n", HexStr(hasher.GetHash()));
         dump_file.close();
@@ -106,7 +104,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro
         fs::remove(path);
     }
 
-    return ret;
+    return result;
 }
 
 // The standard wallet deleter function blocks on the validation interface
@@ -119,20 +117,21 @@ static void WalletToolReleaseWallet(CWallet* wallet)
     delete wallet;
 }
 
-bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector<bilingual_str>& warnings)
+util::Result<void> CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path)
 {
+    util::Result<void> result;
     // Get the dumpfile
     std::string dump_filename = args.GetArg("-dumpfile", "");
     if (dump_filename.empty()) {
-        error = _("No dump file provided. To use createfromdump, -dumpfile=<filename> must be provided.");
+        result.Update(util::Error{_("No dump file provided. To use createfromdump, -dumpfile=<filename> must be provided.")});
-        return false;
+        return result;
     }
 
     fs::path dump_path = fs::PathFromString(dump_filename);
     dump_path = fs::absolute(dump_path);
     if (!fs::exists(dump_path)) {
-        error = strprintf(_("Dump file %s does not exist."), fs::PathToString(dump_path));
+        result.Update(util::Error{strprintf(_("Dump file %s does not exist."), fs::PathToString(dump_path))});
-        return false;
+        return result;
     }
     std::ifstream dump_file{dump_path};
 
@@ -146,21 +145,21 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
     std::string version_value;
     std::getline(dump_file, version_value, '\n');
     if (magic_key != DUMP_MAGIC) {
-        error = strprintf(_("Error: Dumpfile identifier record is incorrect. Got \"%s\", expected \"%s\"."), magic_key, DUMP_MAGIC);
         dump_file.close();
-        return false;
+        result.Update(util::Error{strprintf(_("Error: Dumpfile identifier record is incorrect. Got \"%s\", expected \"%s\"."), magic_key, DUMP_MAGIC)});
+        return result;
     }
     // Check the version number (value of first record)
     uint32_t ver;
     if (!ParseUInt32(version_value, &ver)) {
-        error =strprintf(_("Error: Unable to parse version %u as a uint32_t"), version_value);
         dump_file.close();
-        return false;
+        result.Update(util::Error{strprintf(_("Error: Unable to parse version %u as a uint32_t"), version_value)});
+        return result;
     }
     if (ver != DUMP_VERSION) {
-        error = strprintf(_("Error: Dumpfile version is not supported. This version of bitcoin-wallet only supports version 1 dumpfiles. Got dumpfile with version %s"), version_value);
         dump_file.close();
-        return false;
+        result.Update(util::Error{strprintf(_("Error: Dumpfile version is not supported. This version of bitcoin-wallet only supports version 1 dumpfiles. Got dumpfile with version %s"), version_value)});
+        return result;
     }
     std::string magic_hasher_line = strprintf("%s,%s\n", magic_key, version_value);
     hasher << Span{magic_hasher_line};
@@ -171,15 +170,15 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
     std::string format_value;
     std::getline(dump_file, format_value, '\n');
     if (format_key != "format") {
-        error = strprintf(_("Error: Dumpfile format record is incorrect. Got \"%s\", expected \"format\"."), format_key);
         dump_file.close();
-        return false;
+        result.Update(util::Error{strprintf(_("Error: Dumpfile format record is incorrect. Got \"%s\", expected \"format\"."), format_key)});
+        return result;
     }
     // Get the data file format with format_value as the default
     std::string file_format = args.GetArg("-format", format_value);
     if (file_format.empty()) {
-        error = _("No wallet file format provided. To use createfromdump, -format=<format> must be provided.");
+        result.Update(util::Error{_("No wallet file format provided. To use createfromdump, -format=<format> must be provided.")});
-        return false;
+        return result;
     }
     DatabaseFormat data_format;
     if (file_format == "bdb") {
@@ -189,32 +188,33 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
     } else if (file_format == "bdb_swap") {
         data_format = DatabaseFormat::BERKELEY_SWAP;
     } else {
-        error = strprintf(_("Unknown wallet file format \"%s\" provided. Please provide one of \"bdb\" or \"sqlite\"."), file_format);
+        result.Update(util::Error{strprintf(_("Unknown wallet file format \"%s\" provided. Please provide one of \"bdb\" or \"sqlite\"."), file_format)});
-        return false;
+        return result;
     }
     if (file_format != format_value) {
-        warnings.push_back(strprintf(_("Warning: Dumpfile wallet format \"%s\" does not match command line specified format \"%s\"."), format_value, file_format));
+        result.AddWarning(strprintf(_("Warning: Dumpfile wallet format \"%s\" does not match command line specified format \"%s\"."), format_value, file_format));
     }
     std::string format_hasher_line = strprintf("%s,%s\n", format_key, format_value);
     hasher << Span{format_hasher_line};
 
     DatabaseOptions options;
-    DatabaseStatus status;
     ReadDatabaseArgs(args, options);
     options.require_create = true;
     options.require_format = data_format;
-    std::unique_ptr<WalletDatabase> database = MakeDatabase(wallet_path, options, status, error);
+    auto database{MakeDatabase(wallet_path, options) >> result};
-    if (!database) return false;
+    if (!database) {
+        result.Update(util::Error{});
+        return result;
+    }
 
     // dummy chain interface
-    bool ret = true;
+    std::shared_ptr<CWallet> wallet(new CWallet(/*chain=*/nullptr, name, std::move(database.value())), WalletToolReleaseWallet);
-    std::shared_ptr<CWallet> wallet(new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet);
     {
         LOCK(wallet->cs_wallet);
         DBErrors load_wallet_ret = wallet->LoadWallet();
         if (load_wallet_ret != DBErrors::LOAD_OK) {
-            error = strprintf(_("Error creating %s"), name);
+            result.Update(util::Error{strprintf(_("Error creating %s"), name)});
-            return false;
+            return result;
         }
 
         // Get the database handle
@@ -232,8 +232,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
             if (key == "checksum") {
                 std::vector<unsigned char> parsed_checksum = ParseHex(value);
                 if (parsed_checksum.size() != checksum.size()) {
-                    error = Untranslated("Error: Checksum is not the correct size");
+                    result.Update(util::Error{Untranslated("Error: Checksum is not the correct size")});
-                    ret = false;
                     break;
                 }
                 std::copy(parsed_checksum.begin(), parsed_checksum.end(), checksum.begin());
@@ -248,37 +247,32 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
             }
 
             if (!IsHex(key)) {
-                error = strprintf(_("Error: Got key that was not hex: %s"), key);
+                result.Update(util::Error{strprintf(_("Error: Got key that was not hex: %s"), key)});
-                ret = false;
                 break;
             }
             if (!IsHex(value)) {
-                error = strprintf(_("Error: Got value that was not hex: %s"), value);
+                result.Update(util::Error{strprintf(_("Error: Got value that was not hex: %s"), value)});
-                ret = false;
                 break;
             }
 
             std::vector<unsigned char> k = ParseHex(key);
             std::vector<unsigned char> v = ParseHex(value);
             if (!batch->Write(Span{k}, Span{v})) {
-                error = strprintf(_("Error: Unable to write record to new wallet"));
+                result.Update(util::Error{strprintf(_("Error: Unable to write record to new wallet"))});
-                ret = false;
                 break;
             }
         }
 
-        if (ret) {
+        if (result) {
             uint256 comp_checksum = hasher.GetHash();
             if (checksum.IsNull()) {
-                error = _("Error: Missing checksum");
+                result.Update(util::Error{_("Error: Missing checksum")});
-                ret = false;
             } else if (checksum != comp_checksum) {
-                error = strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum));
+                result.Update(util::Error{strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum))});
-                ret = false;
             }
         }
 
-        if (ret) {
+        if (result) {
             batch->TxnCommit();
         } else {
             batch->TxnAbort();
@@ -291,10 +285,10 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
     wallet.reset(); // The pointer deleter will close the wallet for us.
 
     // Remove the wallet dir if we have a failure
-    if (!ret) {
+    if (!result) {
         fs::remove_all(wallet_path);
     }
 
-    return ret;
+    return result;
 }
 } // namespace wallet

src/wallet/dump.h

@@ -6,6 +6,7 @@
 #define BITCOIN_WALLET_DUMP_H
 
 #include <util/fs.h>
+#include <util/result.h>
 
 #include <string>
 #include <vector>
@@ -16,8 +17,8 @@ class ArgsManager;
 namespace wallet {
 class WalletDatabase;
 
-bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error);
+util::Result<void> DumpWallet(const ArgsManager& args, WalletDatabase& db);
-bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector<bilingual_str>& warnings);
+util::Result<void> CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_DUMP_H

src/wallet/interfaces.cpp

@@ -282,15 +282,19 @@ public:
         int& change_pos,
         CAmount& fee) override
     {
+        util::Result<CTransactionRef> result;
         LOCK(m_wallet->cs_wallet);
-        auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos),
+        auto created{CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos),
-                                     coin_control, sign);
+                                       coin_control, sign) >> result};
-        if (!res) return util::Error{util::ErrorString(res)};
+        if (!created) {
-        const auto& txr = *res;
+            result.Update(util::Error{});
+            return result;
+        }
+        const auto& txr = *created;
         fee = txr.fee;
         change_pos = txr.change_pos ? int(*txr.change_pos) : -1;
-
+        result.Update(txr.tx);
-        return txr.tx;
+        return result;
     }
     void commitTransaction(CTransactionRef tx,
         WalletValueMap value_map,
@@ -599,58 +603,58 @@ public:
     void schedulerMockForward(std::chrono::seconds delta) override { Assert(m_context.scheduler)->MockForward(delta); }
 
     //! WalletLoader methods
-    util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) override
+    util::ResultPtr<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags) override
     {
+        util::ResultPtr<std::unique_ptr<Wallet>> result;
         DatabaseOptions options;
-        DatabaseStatus status;
         ReadDatabaseArgs(*m_context.args, options);
         options.require_create = true;
         options.create_flags = wallet_creation_flags;
         options.create_passphrase = passphrase;
-        bilingual_str error;
+        if (auto wallet{CreateWallet(m_context, name, true /* load_on_start */, options) >> result}) {
-        std::unique_ptr<Wallet> wallet{MakeWallet(m_context, CreateWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))};
+            result.Update(MakeWallet(m_context, wallet.value()));
-        if (wallet) {
-            return wallet;
         } else {
-            return util::Error{error};
+            result.Update(util::Error{});
         }
+        return result;
     }
-    util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) override
+    util::ResultPtr<std::unique_ptr<Wallet>> loadWallet(const std::string& name) override
     {
+        util::ResultPtr<std::unique_ptr<Wallet>> result;
         DatabaseOptions options;
-        DatabaseStatus status;
         ReadDatabaseArgs(*m_context.args, options);
         options.require_existing = true;
-        bilingual_str error;
+        if (auto wallet{LoadWallet(m_context, name, true /* load_on_start */, options) >> result}) {
-        std::unique_ptr<Wallet> wallet{MakeWallet(m_context, LoadWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))};
+            result.Update(MakeWallet(m_context, wallet.value()));
-        if (wallet) {
-            return wallet;
         } else {
-            return util::Error{error};
+            result.Update(util::Error{});
         }
+        return result;
     }
-    util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) override
+    util::ResultPtr<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name) override
     {
-        DatabaseStatus status;
+        util::ResultPtr<std::unique_ptr<Wallet>> result;
-        bilingual_str error;
+        if (auto wallet{RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true) >> result}) {
-        std::unique_ptr<Wallet> wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))};
+            result.Update(MakeWallet(m_context, wallet.value()));
-        if (wallet) {
-            return wallet;
         } else {
-            return util::Error{error};
+            result.Update(util::Error{});
         }
+        return result;
     }
     util::Result<WalletMigrationResult> migrateWallet(const std::string& name, const SecureString& passphrase) override
     {
-        auto res = wallet::MigrateLegacyToDescriptor(name, passphrase, m_context);
+        util::Result<WalletMigrationResult> result;
-        if (!res) return util::Error{util::ErrorString(res)};
+        if (auto res{wallet::MigrateLegacyToDescriptor(name, passphrase, m_context) >> result}) {
-        WalletMigrationResult out{
+            result.Update(WalletMigrationResult{
             .wallet = MakeWallet(m_context, res->wallet),
             .watchonly_wallet_name = res->watchonly_wallet ? std::make_optional(res->watchonly_wallet->GetName()) : std::nullopt,
             .solvables_wallet_name = res->solvables_wallet ? std::make_optional(res->solvables_wallet->GetName()) : std::nullopt,
             .backup_path = res->backup_path,
-        };
+            });
-        return out;
+        } else {
+            result.Update(util::Error{});
+        }
+        return result;
     }
     bool isEncrypted(const std::string& wallet_name) override
     {
@@ -661,9 +665,7 @@ public:
         // Unloaded wallet, read db
         DatabaseOptions options;
         options.require_existing = true;
-        DatabaseStatus status;
+        auto db{MakeWalletDatabase(wallet_name, options)};
-        bilingual_str error;
-        auto db = MakeWalletDatabase(wallet_name, options, status, error);
         if (!db) return false;
         return WalletBatch(*db).IsEncrypted();
     }

src/wallet/load.cpp

@@ -58,12 +58,10 @@ bool VerifyWallets(WalletContext& context)
     // wallets directory, include it in the default list of wallets to load.
     if (!args.IsArgSet("wallet")) {
         DatabaseOptions options;
-        DatabaseStatus status;
         ReadDatabaseArgs(args, options);
-        bilingual_str error_string;
         options.require_existing = true;
         options.verify = false;
-        if (MakeWalletDatabase("", options, status, error_string)) {
+        if (MakeWalletDatabase("", options)) {
             common::SettingsValue wallets(common::SettingsValue::VARR);
             wallets.push_back(""); // Default wallet name is ""
             // Pass write=false because no need to write file and probably
@@ -91,16 +89,15 @@ bool VerifyWallets(WalletContext& context)
         }
 
         DatabaseOptions options;
-        DatabaseStatus status;
         ReadDatabaseArgs(args, options);
         options.require_existing = true;
         options.verify = true;
-        bilingual_str error_string;
+        auto result{MakeWalletDatabase(wallet_file, options)};
-        if (!MakeWalletDatabase(wallet_file, options, status, error_string)) {
+        if (!result) {
-            if (status == DatabaseStatus::FAILED_NOT_FOUND) {
+            if (result.GetFailure() == DatabaseError::FAILED_NOT_FOUND) {
-                chain.initWarning(Untranslated(strprintf("Skipping -wallet path that doesn't exist. %s", error_string.original)));
+                chain.initWarning(Untranslated(strprintf("Skipping -wallet path that doesn't exist. %s", util::ErrorString(result).original)));
             } else {
-                chain.initError(error_string);
+                chain.initError(util::ErrorString(result));
                 return false;
             }
         }
@@ -125,26 +122,24 @@ bool LoadWallets(WalletContext& context)
                 continue;
             }
             DatabaseOptions options;
-            DatabaseStatus status;
             ReadDatabaseArgs(*context.args, options);
             options.require_existing = true;
             options.verify = false; // No need to verify, assuming verified earlier in VerifyWallets()
-            bilingual_str error;
+            util::Result<void> result;
-            std::vector<bilingual_str> warnings;
+            auto database{MakeWalletDatabase(name, options) >> result};
-            std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(name, options, status, error);
+            if (!database && database.GetFailure() == DatabaseError::FAILED_NOT_FOUND) {
-            if (!database && status == DatabaseStatus::FAILED_NOT_FOUND) {
                 continue;
             }
             chain.initMessage(_("Loading wallet…"));
-            std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings) : nullptr;
+            auto pwallet{database ? CWallet::Create(context, name, std::move(database.value()), options.create_flags) >> result : nullptr};
-            if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
+            if (result.GetMessages() && !result.GetMessages()->warnings.empty()) chain.initWarning(Join(result.GetMessages()->warnings, Untranslated("\n")));
             if (!pwallet) {
-                chain.initError(error);
+                chain.initError(Join(result.GetMessages()->errors, Untranslated("\n")));
                 return false;
             }
 
-            NotifyWalletLoaded(context, pwallet);
+            NotifyWalletLoaded(context, pwallet.value());
-            AddWallet(context, pwallet);
+            AddWallet(context, pwallet.value());
         }
         return true;
     } catch (const std::runtime_error& e) {
@@ -186,8 +181,8 @@ void UnloadWallets(WalletContext& context)
     while (!wallets.empty()) {
         auto wallet = wallets.back();
         wallets.pop_back();
-        std::vector<bilingual_str> warnings;
+        // Note: Warnings returned from RemoveWallet are silently discarded.
-        RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt, warnings);
+        RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt);
         WaitForDeleteWallet(std::move(wallet));
     }
 }

src/wallet/migrate.cpp

@@ -768,17 +768,13 @@ std::unique_ptr<DatabaseCursor> BerkeleyROBatch::GetNewPrefixCursor(Span<const s
     return std::make_unique<BerkeleyROCursor>(m_database, prefix);
 }
 
-std::unique_ptr<BerkeleyRODatabase> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error)
+util::ResultPtr<std::unique_ptr<BerkeleyRODatabase>, DatabaseError> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options)
 {
     fs::path data_file = BDBDataFile(path);
     try {
-        std::unique_ptr<BerkeleyRODatabase> db = std::make_unique<BerkeleyRODatabase>(data_file);
+        return std::make_unique<BerkeleyRODatabase>(data_file);
-        status = DatabaseStatus::SUCCESS;
-        return db;
     } catch (const std::runtime_error& e) {
-        error.original = e.what();
+        return {util::Error{Untranslated(e.what())}, DatabaseError::FAILED_LOAD};
-        status = DatabaseStatus::FAILED_LOAD;
-        return nullptr;
     }
 }
 } // namespace wallet

src/wallet/migrate.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_WALLET_MIGRATE_H
 #define BITCOIN_WALLET_MIGRATE_H
 
+#include <util/result.h>
 #include <wallet/db.h>
 
 #include <optional>
@@ -119,7 +120,7 @@ public:
 };
 
 //! Return object giving access to Berkeley Read Only database at specified path.
-std::unique_ptr<BerkeleyRODatabase> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+util::ResultPtr<std::unique_ptr<BerkeleyRODatabase>, DatabaseError> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_MIGRATE_H

src/wallet/rpc/backup.cpp

@@ -1963,17 +1963,15 @@ RPCHelpMan restorewallet()
 
     std::optional<bool> load_on_start = request.params[2].isNull() ? std::nullopt : std::optional<bool>(request.params[2].get_bool());
 
-    DatabaseStatus status;
+    auto wallet{RestoreWallet(context, backup_file, wallet_name, load_on_start)};
-    bilingual_str error;
-    std::vector<bilingual_str> warnings;
 
-    const std::shared_ptr<CWallet> wallet = RestoreWallet(context, backup_file, wallet_name, load_on_start, status, error, warnings);
+    HandleWalletError(wallet);
-
-    HandleWalletError(wallet, status, error);
 
     UniValue obj(UniValue::VOBJ);
     obj.pushKV("name", wallet->GetName());
-    PushWarnings(warnings, obj);
+    if (wallet.GetMessages()) {
+        PushWarnings(wallet.GetMessages()->warnings, obj);
+    }
 
     return obj;
 

src/wallet/rpc/util.cpp

@@ -136,30 +136,30 @@ void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey,
     entry.pushKV("parent_descs", std::move(parent_descs));
 }
 
-void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error)
+void HandleWalletError(const util::ResultPtr<std::shared_ptr<CWallet>, DatabaseError>& wallet)
 {
     if (!wallet) {
         // Map bad format to not found, since bad format is returned when the
         // wallet directory exists, but doesn't contain a data file.
         RPCErrorCode code = RPC_WALLET_ERROR;
-        switch (status) {
+        switch (wallet.GetFailure()) {
-            case DatabaseStatus::FAILED_NOT_FOUND:
+            case DatabaseError::FAILED_NOT_FOUND:
-            case DatabaseStatus::FAILED_BAD_FORMAT:
+            case DatabaseError::FAILED_BAD_FORMAT:
                 code = RPC_WALLET_NOT_FOUND;
                 break;
-            case DatabaseStatus::FAILED_ALREADY_LOADED:
+            case DatabaseError::FAILED_ALREADY_LOADED:
                 code = RPC_WALLET_ALREADY_LOADED;
                 break;
-            case DatabaseStatus::FAILED_ALREADY_EXISTS:
+            case DatabaseError::FAILED_ALREADY_EXISTS:
                 code = RPC_WALLET_ALREADY_EXISTS;
                 break;
-            case DatabaseStatus::FAILED_INVALID_BACKUP_FILE:
+            case DatabaseError::FAILED_INVALID_BACKUP_FILE:
                 code = RPC_INVALID_PARAMETER;
                 break;
             default: // RPC_WALLET_ERROR is returned for all other cases.
                 break;
         }
-        throw JSONRPCError(code, error.original);
+        throw JSONRPCError(code, util::ErrorString(wallet).original);
     }
 }
 

src/wallet/rpc/util.h

@@ -7,6 +7,7 @@
 
 #include <rpc/util.h>
 #include <script/script.h>
+#include <util/result.h>
 #include <wallet/wallet.h>
 
 #include <any>
@@ -20,7 +21,7 @@ struct bilingual_str;
 
 namespace wallet {
 class LegacyScriptPubKeyMan;
-enum class DatabaseStatus;
+enum class DatabaseError;
 struct WalletContext;
 
 extern const std::string HELP_REQUIRING_PASSPHRASE;
@@ -50,7 +51,7 @@ std::string LabelFromValue(const UniValue& value);
 //! Fetch parent descriptors of this scriptPubKey.
 void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry);
 
-void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error);
+void HandleWalletError(const util::ResultPtr<std::shared_ptr<CWallet>, DatabaseError>& wallet);
 void AppendLastProcessedBlock(UniValue& entry, const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 } //  namespace wallet
 

src/wallet/rpc/wallet.cpp

@@ -250,11 +250,8 @@ static RPCHelpMan loadwallet()
     const std::string name(request.params[0].get_str());
 
     DatabaseOptions options;
-    DatabaseStatus status;
     ReadDatabaseArgs(*context.args, options);
     options.require_existing = true;
-    bilingual_str error;
-    std::vector<bilingual_str> warnings;
     std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());
 
     {
@@ -264,13 +261,14 @@ static RPCHelpMan loadwallet()
         }
     }
 
-    std::shared_ptr<CWallet> const wallet = LoadWallet(context, name, load_on_start, options, status, error, warnings);
+    auto wallet{LoadWallet(context, name, load_on_start, options)};
-
+    HandleWalletError(wallet);
-    HandleWalletError(wallet, status, error);
 
     UniValue obj(UniValue::VOBJ);
     obj.pushKV("name", wallet->GetName());
-    PushWarnings(warnings, obj);
+    if (wallet.GetMessages()) {
+        PushWarnings(wallet.GetMessages()->warnings, obj);
+    }
 
     return obj;
 },
@@ -390,12 +388,12 @@ static RPCHelpMan createwallet()
     }
     SecureString passphrase;
     passphrase.reserve(100);
-    std::vector<bilingual_str> warnings;
+    util::Result<void> result;
     if (!request.params[3].isNull()) {
         passphrase = std::string_view{request.params[3].get_str()};
         if (passphrase.empty()) {
             // Empty string means unencrypted
-            warnings.emplace_back(Untranslated("Empty string given as passphrase, wallet will not be encrypted."));
+            result.AddWarning(Untranslated("Empty string given as passphrase, wallet will not be encrypted."));
         }
     }
 
@@ -428,22 +426,22 @@ static RPCHelpMan createwallet()
 #endif
 
     DatabaseOptions options;
-    DatabaseStatus status;
     ReadDatabaseArgs(*context.args, options);
     options.require_create = true;
     options.create_flags = flags;
     options.create_passphrase = passphrase;
-    bilingual_str error;
     std::optional<bool> load_on_start = request.params[6].isNull() ? std::nullopt : std::optional<bool>(request.params[6].get_bool());
-    const std::shared_ptr<CWallet> wallet = CreateWallet(context, request.params[0].get_str(), load_on_start, options, status, error, warnings);
+    auto wallet{CreateWallet(context, request.params[0].get_str(), load_on_start, options) >> result};
     if (!wallet) {
-        RPCErrorCode code = status == DatabaseStatus::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR;
+        RPCErrorCode code = wallet.GetFailure() == DatabaseError::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR;
-        throw JSONRPCError(code, error.original);
+        throw JSONRPCError(code, util::ErrorString(result).original);
     }
 
     UniValue obj(UniValue::VOBJ);
     obj.pushKV("name", wallet->GetName());
-    PushWarnings(warnings, obj);
+    if (result.GetMessages()) {
+        PushWarnings(result.GetMessages()->warnings, obj);
+    }
 
     return obj;
 },
@@ -486,7 +484,7 @@ static RPCHelpMan unloadwallet()
         throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
     }
 
-    std::vector<bilingual_str> warnings;
+    util::Result<void> result;
     {
         WalletRescanReserver reserver(*wallet);
         if (!reserver.reserve()) {
@@ -497,17 +495,19 @@ static RPCHelpMan unloadwallet()
         // Note that any attempt to load the same wallet would fail until the wallet
         // is destroyed (see CheckUniqueFileid).
         std::optional<bool> load_on_start{self.MaybeArg<bool>("load_on_startup")};
-        if (!RemoveWallet(context, wallet, load_on_start, warnings)) {
+        if (!(RemoveWallet(context, wallet, load_on_start) >> result)) {
             throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
         }
     }
 
     WaitForDeleteWallet(std::move(wallet));
 
-    UniValue result(UniValue::VOBJ);
+    UniValue ret(UniValue::VOBJ);
-    PushWarnings(warnings, result);
+    if (result.GetMessages()) {
+        PushWarnings(result.GetMessages()->warnings, ret);
+    }
 
-    return result;
+    return ret;
 },
     };
 }

src/wallet/salvage.cpp

@@ -67,23 +67,27 @@ public:
     std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override { return std::make_unique<DummyBatch>(); }
 };
 
-bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bilingual_str& error, std::vector<bilingual_str>& warnings)
+util::Result<void> RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path)
 {
+    util::Result<void> result;
     DatabaseOptions options;
-    DatabaseStatus status;
     ReadDatabaseArgs(args, options);
     options.require_existing = true;
     options.verify = false;
     options.require_format = DatabaseFormat::BERKELEY;
-    std::unique_ptr<WalletDatabase> database = MakeDatabase(file_path, options, status, error);
+    auto database{MakeDatabase(file_path, options) >> result};
-    if (!database) return false;
+    if (!database) {
+        result.Update(util::Error{});
+        return result;
+    }
 
     BerkeleyDatabase& berkeley_database = static_cast<BerkeleyDatabase&>(*database);
     std::string filename = berkeley_database.Filename();
     std::shared_ptr<BerkeleyEnvironment> env = berkeley_database.env;
 
-    if (!env->Open(error)) {
+    if (!(env->Open() >> result)) {
-        return false;
+        result.Update(util::Error{});
+        return result;
     }
 
     // Recovery procedure:
@@ -96,12 +100,12 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
     int64_t now = GetTime();
     std::string newFilename = strprintf("%s.%d.bak", filename, now);
 
-    int result = env->dbenv->dbrename(nullptr, filename.c_str(), nullptr,
+    int ret = env->dbenv->dbrename(nullptr, filename.c_str(), nullptr,
                                    newFilename.c_str(), DB_AUTO_COMMIT);
-    if (result != 0)
+    if (ret != 0)
     {
-        error = Untranslated(strprintf("Failed to rename %s to %s", filename, newFilename));
+        result.Update(util::Error{Untranslated(strprintf("Failed to rename %s to %s", filename, newFilename))});
-        return false;
+        return result;
     }
 
     /**
@@ -115,13 +119,13 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
     std::stringstream strDump;
 
     Db db(env->dbenv.get(), 0);
-    result = db.verify(newFilename.c_str(), nullptr, &strDump, DB_SALVAGE | DB_AGGRESSIVE);
+    ret = db.verify(newFilename.c_str(), nullptr, &strDump, DB_SALVAGE | DB_AGGRESSIVE);
-    if (result == DB_VERIFY_BAD) {
+    if (ret == DB_VERIFY_BAD) {
-        warnings.emplace_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable."));
+        result.AddWarning(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable."));
     }
-    if (result != 0 && result != DB_VERIFY_BAD) {
+    if (ret != 0 && ret != DB_VERIFY_BAD) {
-        error = Untranslated(strprintf("Salvage: Database salvage failed with result %d.", result));
+        result.Update(util::Error{Untranslated(strprintf("Salvage: Database salvage failed with result %d.", ret))});
-        return false;
+        return result;
     }
 
     // Format of bdb dump is ascii lines:
@@ -144,38 +148,36 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
                 break;
             getline(strDump, valueHex);
             if (valueHex == DATA_END) {
-                warnings.emplace_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values."));
+                result.AddWarning(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values."));
                 break;
             }
             salvagedData.emplace_back(ParseHex(keyHex), ParseHex(valueHex));
         }
     }
 
-    bool fSuccess;
     if (keyHex != DATA_END) {
-        warnings.emplace_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output."));
+        result.Update({util::Error{}, util::Warning{Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output.")}});
-        fSuccess = false;
+    } else if (ret != 0) {
-    } else {
+        result.Update(util::Error{});
-        fSuccess = (result == 0);
     }
 
     if (salvagedData.empty())
     {
-        error = Untranslated(strprintf("Salvage(aggressive) found no records in %s.", newFilename));
+        result.Update(util::Error{Untranslated(strprintf("Salvage(aggressive) found no records in %s.", newFilename))});
-        return false;
+        return result;
     }
 
     std::unique_ptr<Db> pdbCopy = std::make_unique<Db>(env->dbenv.get(), 0);
-    int ret = pdbCopy->open(nullptr,               // Txn pointer
+    ret = pdbCopy->open(nullptr,                // Txn pointer
                             filename.c_str(),   // Filename
                             "main",             // Logical db name
                             DB_BTREE,           // Database type
                             DB_CREATE,          // Flags
                             0);
     if (ret > 0) {
-        error = Untranslated(strprintf("Cannot create database file %s", filename));
         pdbCopy->close(0);
-        return false;
+        result.Update(util::Error{Untranslated(strprintf("Cannot create database file %s", filename))});
+        return result;
     }
 
     DbTxn* ptxn = env->TxnBegin(DB_TXN_WRITE_NOSYNC);
@@ -204,18 +206,19 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
 
         if (!fReadOK)
         {
-            warnings.push_back(Untranslated(strprintf("WARNING: WalletBatch::Recover skipping %s: %s", strType, strErr)));
+            result.AddWarning(Untranslated(strprintf("WARNING: WalletBatch::Recover skipping %s: %s", strType, strErr)));
             continue;
         }
         Dbt datKey(row.first.data(), row.first.size());
         Dbt datValue(row.second.data(), row.second.size());
         int ret2 = pdbCopy->put(ptxn, &datKey, &datValue, DB_NOOVERWRITE);
-        if (ret2 > 0)
+        if (ret2 > 0) {
-            fSuccess = false;
+            result.Update(util::Error{});
+        }
     }
     ptxn->commit(0);
     pdbCopy->close(0);
 
-    return fSuccess;
+    return result;
 }
 } // namespace wallet

src/wallet/salvage.h

@@ -13,7 +13,7 @@ class ArgsManager;
 struct bilingual_str;
 
 namespace wallet {
-bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bilingual_str& error, std::vector<bilingual_str>& warnings);
+util::Result<void> RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_SALVAGE_H

src/wallet/sqlite.cpp

@@ -78,21 +78,19 @@ static bool BindBlobToStatement(sqlite3_stmt* stmt,
     return true;
 }
 
-static std::optional<int> ReadPragmaInteger(sqlite3* db, const std::string& key, const std::string& description, bilingual_str& error)
+static util::Result<int> ReadPragmaInteger(sqlite3* db, const std::string& key, const std::string& description)
 {
     std::string stmt_text = strprintf("PRAGMA %s", key);
     sqlite3_stmt* pragma_read_stmt{nullptr};
     int ret = sqlite3_prepare_v2(db, stmt_text.c_str(), -1, &pragma_read_stmt, nullptr);
     if (ret != SQLITE_OK) {
         sqlite3_finalize(pragma_read_stmt);
-        error = Untranslated(strprintf("SQLiteDatabase: Failed to prepare the statement to fetch %s: %s", description, sqlite3_errstr(ret)));
+        return {util::Error{Untranslated(strprintf("SQLiteDatabase: Failed to prepare the statement to fetch %s: %s", description, sqlite3_errstr(ret)))}};
-        return std::nullopt;
     }
     ret = sqlite3_step(pragma_read_stmt);
     if (ret != SQLITE_ROW) {
         sqlite3_finalize(pragma_read_stmt);
-        error = Untranslated(strprintf("SQLiteDatabase: Failed to fetch %s: %s", description, sqlite3_errstr(ret)));
+        return {util::Error{Untranslated(strprintf("SQLiteDatabase: Failed to fetch %s: %s", description, sqlite3_errstr(ret)))}};
-        return std::nullopt;
     }
     int result = sqlite3_column_int(pragma_read_stmt, 0);
     sqlite3_finalize(pragma_read_stmt);
@@ -187,35 +185,42 @@ void SQLiteDatabase::Cleanup() noexcept
     }
 }
 
-bool SQLiteDatabase::Verify(bilingual_str& error)
+util::Result<void> SQLiteDatabase::Verify()
 {
     assert(m_db);
+    util::Result<void> result;
 
     // Check the application ID matches our network magic
-    auto read_result = ReadPragmaInteger(m_db, "application_id", "the application id", error);
+    auto read_app_id{ReadPragmaInteger(m_db, "application_id", "the application id") >> result};
-    if (!read_result.has_value()) return false;
+    if (!read_app_id) {
-    uint32_t app_id = static_cast<uint32_t>(read_result.value());
+        result.Update(util::Error{});
+        return result;
+    }
+    uint32_t app_id = static_cast<uint32_t>(*read_app_id);
     uint32_t net_magic = ReadBE32(Params().MessageStart().data());
     if (app_id != net_magic) {
-        error = strprintf(_("SQLiteDatabase: Unexpected application id. Expected %u, got %u"), net_magic, app_id);
+        result.Update(util::Error{strprintf(_("SQLiteDatabase: Unexpected application id. Expected %u, got %u"), net_magic, app_id)});
-        return false;
+        return result;
     }
 
     // Check our schema version
-    read_result = ReadPragmaInteger(m_db, "user_version", "sqlite wallet schema version", error);
+    auto read_user_ver{ReadPragmaInteger(m_db, "user_version", "sqlite wallet schema version") >> result};
-    if (!read_result.has_value()) return false;
+    if (!read_user_ver) {
-    int32_t user_ver = read_result.value();
+        result.Update(util::Error{});
+        return result;
+    }
+    int32_t user_ver = *read_user_ver;
     if (user_ver != WALLET_SCHEMA_VERSION) {
-        error = strprintf(_("SQLiteDatabase: Unknown sqlite wallet schema version %d. Only version %d is supported"), user_ver, WALLET_SCHEMA_VERSION);
+        result.Update(util::Error{strprintf(_("SQLiteDatabase: Unknown sqlite wallet schema version %d. Only version %d is supported"), user_ver, WALLET_SCHEMA_VERSION)});
-        return false;
+        return result;
     }
 
     sqlite3_stmt* stmt{nullptr};
     int ret = sqlite3_prepare_v2(m_db, "PRAGMA integrity_check", -1, &stmt, nullptr);
     if (ret != SQLITE_OK) {
         sqlite3_finalize(stmt);
-        error = strprintf(_("SQLiteDatabase: Failed to prepare statement to verify database: %s"), sqlite3_errstr(ret));
+        result.Update(util::Error{strprintf(_("SQLiteDatabase: Failed to prepare statement to verify database: %s"), sqlite3_errstr(ret))});
-        return false;
+        return result;
     }
     while (true) {
         ret = sqlite3_step(stmt);
@@ -223,25 +228,25 @@ bool SQLiteDatabase::Verify(bilingual_str& error)
             break;
         }
         if (ret != SQLITE_ROW) {
-            error = strprintf(_("SQLiteDatabase: Failed to execute statement to verify database: %s"), sqlite3_errstr(ret));
+            result.Update(util::Error{strprintf(_("SQLiteDatabase: Failed to execute statement to verify database: %s"), sqlite3_errstr(ret))});
             break;
         }
         const char* msg = (const char*)sqlite3_column_text(stmt, 0);
         if (!msg) {
-            error = strprintf(_("SQLiteDatabase: Failed to read database verification error: %s"), sqlite3_errstr(ret));
+            result.Update(util::Error{strprintf(_("SQLiteDatabase: Failed to read database verification error: %s"), sqlite3_errstr(ret))});
             break;
         }
         std::string str_msg(msg);
         if (str_msg == "ok") {
             continue;
         }
-        if (error.empty()) {
+        if (result) {
-            error = _("Failed to verify database") + Untranslated("\n");
+            result.Update(util::Error{_("Failed to verify database")});
         }
-        error += Untranslated(strprintf("%s\n", str_msg));
+        result.AddError(Untranslated(str_msg));
     }
     sqlite3_finalize(stmt);
-    return error.empty();
+    return result;
 }
 
 void SQLiteDatabase::Open()
@@ -691,22 +696,21 @@ bool SQLiteBatch::TxnAbort()
     return res == SQLITE_OK;
 }
 
-std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error)
+util::ResultPtr<std::unique_ptr<SQLiteDatabase>, DatabaseError> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options)
 {
+    util::ResultPtr<std::unique_ptr<SQLiteDatabase>, DatabaseError> result;
     try {
         fs::path data_file = SQLiteDataFile(path);
         auto db = std::make_unique<SQLiteDatabase>(data_file.parent_path(), data_file, options);
-        if (options.verify && !db->Verify(error)) {
+        if (options.verify && !(db->Verify() >> result)) {
-            status = DatabaseStatus::FAILED_VERIFY;
+            result.Update({util::Error{}, DatabaseError::FAILED_VERIFY});
-            return nullptr;
+        } else {
+            result.Update(std::move(db));
         }
-        status = DatabaseStatus::SUCCESS;
-        return db;
     } catch (const std::runtime_error& e) {
-        status = DatabaseStatus::FAILED_LOAD;
+        result.Update({util::Error{Untranslated(e.what())}, DatabaseError::FAILED_LOAD});
-        error = Untranslated(e.what());
-        return nullptr;
     }
+    return result;
 }
 
 std::string SQLiteDatabaseVersion()

src/wallet/sqlite.h

@@ -6,6 +6,7 @@
 #define BITCOIN_WALLET_SQLITE_H
 
 #include <sync.h>
+#include <util/result.h>
 #include <wallet/db.h>
 
 struct bilingual_str;
@@ -132,7 +133,7 @@ public:
     // This ensures that only one batch is modifying the database at a time.
     CSemaphore m_write_semaphore;
 
-    bool Verify(bilingual_str& error);
+    util::Result<void> Verify();
 
     /** Open the database if it is not already opened */
     void Open() override;
@@ -178,7 +179,7 @@ public:
     bool m_use_unsafe_sync;
 };
 
-std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+util::ResultPtr<std::unique_ptr<SQLiteDatabase>, DatabaseError> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options);
 
 std::string SQLiteDatabaseVersion();
 } // namespace wallet

src/wallet/test/db_tests.cpp

@@ -131,15 +131,13 @@ static std::vector<std::unique_ptr<WalletDatabase>> TestDatabases(const fs::path
 {
     std::vector<std::unique_ptr<WalletDatabase>> dbs;
     DatabaseOptions options;
-    DatabaseStatus status;
-    bilingual_str error;
 #ifdef USE_BDB
-    dbs.emplace_back(MakeBerkeleyDatabase(path_root / "bdb", options, status, error));
+    dbs.emplace_back(std::move(MakeBerkeleyDatabase(path_root / "bdb", options).value()));
     // Needs BDB to make the DB to read
     dbs.emplace_back(std::make_unique<BerkeleyRODatabase>(BDBDataFile(path_root / "bdb"), /*open=*/false));
 #endif
 #ifdef USE_SQLITE
-    dbs.emplace_back(MakeSQLiteDatabase(path_root / "sqlite", options, status, error));
+    dbs.emplace_back(std::move(MakeSQLiteDatabase(path_root / "sqlite", options).value()));
 #endif
     dbs.emplace_back(CreateMockableWalletDatabase());
     return dbs;
@@ -316,9 +314,7 @@ BOOST_AUTO_TEST_CASE(txn_close_failure_dangling_txn)
     // Verifies that there is no active dangling, to-be-reversed db txn
     // after the batch object that initiated it is destroyed.
     DatabaseOptions options;
-    DatabaseStatus status;
+    auto database = MakeSQLiteDatabase(m_path_root / "sqlite", options);
-    bilingual_str error;
-    std::unique_ptr<SQLiteDatabase> database = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error);
 
     std::string key = "key";
     std::string value = "value";
@@ -351,9 +347,7 @@ BOOST_AUTO_TEST_CASE(concurrent_txn_dont_interfere)
     std::string value2 = "value_2";
 
     DatabaseOptions options;
-    DatabaseStatus status;
+    const auto& database = MakeSQLiteDatabase(m_path_root / "sqlite", options);
-    bilingual_str error;
-    const auto& database = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error);
 
     std::unique_ptr<DatabaseBatch> handler = Assert(database)->MakeBatch();
 

src/wallet/test/fuzz/wallet_bdb_parser.cpp

@@ -26,7 +26,6 @@
 #endif
 
 using wallet::DatabaseOptions;
-using wallet::DatabaseStatus;
 
 namespace {
 TestingSetup* g_setup;
@@ -48,9 +47,6 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser)
     }
 
     const DatabaseOptions options{};
-    DatabaseStatus status;
-    bilingual_str error;
-
     fs::path bdb_ro_dumpfile{g_setup->m_args.GetDataDirNet() / "fuzzed_dumpfile_bdb_ro.dump"};
     if (fs::exists(bdb_ro_dumpfile)) { // Writing into an existing dump file will throw an exception
         remove(bdb_ro_dumpfile);
@@ -61,13 +57,14 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser)
     bool bdb_ro_err = false;
     bool bdb_ro_strict_err = false;
 #endif
-    auto db{MakeBerkeleyRODatabase(wallet_path, options, status, error)};
+    auto db{MakeBerkeleyRODatabase(wallet_path, options)};
     if (db) {
-        assert(DumpWallet(g_setup->m_args, *db, error));
+        assert(DumpWallet(g_setup->m_args, *db));
     } else {
 #ifdef USE_BDB_NON_MSVC
         bdb_ro_err = true;
 #endif
+        bilingual_str error{util::ErrorString(db)};
         if (error.original.starts_with("AutoFile::ignore: end of file") ||
             error.original.starts_with("AutoFile::read: end of file") ||
             error.original.starts_with("AutoFile::seek: ") ||
@@ -114,7 +111,7 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser)
     g_setup->m_args.ForceSetArg("-dumpfile", fs::PathToString(bdb_dumpfile));
 
     try {
-        auto db{MakeBerkeleyDatabase(wallet_path, options, status, error)};
+        auto db{MakeBerkeleyDatabase(wallet_path, options)};
         if (bdb_ro_err && !db) {
             return;
         }
@@ -124,7 +121,7 @@ FUZZ_TARGET(wallet_bdb_parser, .init = initialize_wallet_bdb_parser)
             return;
         }
         assert(!bdb_ro_err);
-        assert(DumpWallet(g_setup->m_args, *db, error));
+        assert(DumpWallet(g_setup->m_args, *db));
     } catch (const std::runtime_error& e) {
         if (bdb_ro_err) return;
         throw e;

src/wallet/test/util.cpp

@@ -49,25 +49,21 @@ std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cc
 
 std::shared_ptr<CWallet> TestLoadWallet(std::unique_ptr<WalletDatabase> database, WalletContext& context, uint64_t create_flags)
 {
-    bilingual_str error;
+    auto wallet{CWallet::Create(context, "", std::move(database), create_flags)};
-    std::vector<bilingual_str> warnings;
+    NotifyWalletLoaded(context, wallet.value());
-    auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings);
-    NotifyWalletLoaded(context, wallet);
     if (context.chain) {
         wallet->postInitProcess();
     }
-    return wallet;
+    return wallet.value();
 }
 
 std::shared_ptr<CWallet> TestLoadWallet(WalletContext& context)
 {
     DatabaseOptions options;
     options.create_flags = WALLET_FLAG_DESCRIPTORS;
-    DatabaseStatus status;
-    bilingual_str error;
     std::vector<bilingual_str> warnings;
-    auto database = MakeWalletDatabase("", options, status, error);
+    auto database{MakeWalletDatabase("", options)};
-    return TestLoadWallet(std::move(database), context, options.create_flags);
+    return TestLoadWallet(std::move(database.value()), context, options.create_flags);
 }
 
 void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)

src/wallet/test/wallet_tests.cpp

@@ -449,11 +449,8 @@ void TestLoadWallet(const std::string& name, DatabaseFormat format, std::functio
     auto chain{interfaces::MakeChain(node)};
     DatabaseOptions options;
     options.require_format = format;
-    DatabaseStatus status;
+    auto database{MakeWalletDatabase(name, options)};
-    bilingual_str error;
+    auto wallet{std::make_shared<CWallet>(chain.get(), "", std::move(database.value()))};
-    std::vector<bilingual_str> warnings;
-    auto database{MakeWalletDatabase(name, options, status, error)};
-    auto wallet{std::make_shared<CWallet>(chain.get(), "", std::move(database))};
     BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::LOAD_OK);
     WITH_LOCK(wallet->cs_wallet, f(wallet));
 }

src/wallet/test/walletdb_tests.cpp

@@ -36,12 +36,11 @@ BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock)
         // Context setup
         DatabaseOptions options;
         options.require_format = db_format;
-        DatabaseStatus status;
         bilingual_str error_string;
-        std::unique_ptr<WalletDatabase> db = MakeDatabase(m_path_root / strprintf("wallet_%d_.dat", db_format).c_str(), options, status, error_string);
+        auto db = MakeDatabase(m_path_root / strprintf("wallet_%d_.dat", db_format).c_str(), options);
-        BOOST_CHECK_EQUAL(status, DatabaseStatus::SUCCESS);
+        BOOST_CHECK(db);
 
-        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", std::move(db)));
+        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", std::move(db.value())));
         wallet->m_keypool_size = 4;
 
         // Create legacy spkm

src/wallet/wallet.h

@@ -88,17 +88,16 @@ struct WalletContext;
 void WaitForDeleteWallet(std::shared_ptr<CWallet>&& wallet);
 
 bool AddWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet);
-bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet, std::optional<bool> load_on_start, std::vector<bilingual_str>& warnings);
+util::Result<void> RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet, std::optional<bool> load_on_start);
-bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet, std::optional<bool> load_on_start);
 std::vector<std::shared_ptr<CWallet>> GetWallets(WalletContext& context);
 std::shared_ptr<CWallet> GetDefaultWallet(WalletContext& context, size_t& count);
 std::shared_ptr<CWallet> GetWallet(WalletContext& context, const std::string& name);
-std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
+util::ResultPtr<std::shared_ptr<CWallet>, DatabaseError> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options);
-std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
+util::ResultPtr<std::shared_ptr<CWallet>, DatabaseError> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options);
-std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings, bool load_after_restore = true);
+util::ResultPtr<std::shared_ptr<CWallet>, DatabaseError> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, bool load_after_restore = true);
 std::unique_ptr<interfaces::Handler> HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet);
 void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr<CWallet>& wallet);
-std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+util::ResultPtr<std::unique_ptr<WalletDatabase>, DatabaseError> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options);
 
 //! -paytxfee default
 constexpr CAmount DEFAULT_PAY_TX_FEE = 0;
@@ -433,7 +432,7 @@ private:
      * block locator and m_last_block_processed, and registering for
      * notifications about new blocks and transactions.
      */
-    static bool AttachChain(const std::shared_ptr<CWallet>& wallet, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector<bilingual_str>& warnings);
+    static util::Result<void> AttachChain(const std::shared_ptr<CWallet>& wallet, interfaces::Chain& chain, const bool rescan_required);
 
     static NodeClock::time_point GetDefaultNextResend();
 
@@ -877,7 +876,7 @@ public:
     bool MarkReplaced(const uint256& originalHash, const uint256& newHash);
 
     /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
-    static std::shared_ptr<CWallet> Create(WalletContext& context, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings);
+    static util::ResultPtr<std::shared_ptr<CWallet>> Create(WalletContext& context, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags);
 
     /**
      * Wallet post-init setup
@@ -1046,10 +1045,10 @@ public:
      * A backup is not created.
      * May crash if something unexpected happens in the filesystem.
      */
-    bool MigrateToSQLite(bilingual_str& error) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    util::Result<void> MigrateToSQLite() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     //! Get all of the descriptors from a legacy wallet
-    std::optional<MigrationData> GetDescriptorsForLegacy(bilingual_str& error) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    util::Result<MigrationData> GetDescriptorsForLegacy() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     //! Adds the ScriptPubKeyMans given in MigrationData to this wallet, removes LegacyScriptPubKeyMan,
     //! and where needed, moves tx and address book entries to watchonly_wallet or solvable_wallet

src/wallet/walletdb.cpp

@@ -1405,15 +1405,13 @@ void WalletBatch::RegisterTxnListener(const DbTxnListener& l)
     m_txn_listeners.emplace_back(l);
 }
 
-std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error)
+util::ResultPtr<std::unique_ptr<WalletDatabase>, DatabaseError> MakeDatabase(const fs::path& path, const DatabaseOptions& options)
 {
     bool exists;
     try {
         exists = fs::symlink_status(path).type() != fs::file_type::not_found;
     } catch (const fs::filesystem_error& e) {
-        error = Untranslated(strprintf("Failed to access database path '%s': %s", fs::PathToString(path), fsbridge::get_filesystem_error_message(e)));
+        return {util::Error{Untranslated(strprintf("Failed to access database path '%s': %s", fs::PathToString(path), fsbridge::get_filesystem_error_message(e)))}, DatabaseError::FAILED_BAD_PATH};
-        status = DatabaseStatus::FAILED_BAD_PATH;
-        return nullptr;
     }
 
     std::optional<DatabaseFormat> format;
@@ -1423,28 +1421,20 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
         }
         if (IsSQLiteFile(SQLiteDataFile(path))) {
             if (format) {
-                error = Untranslated(strprintf("Failed to load database path '%s'. Data is in ambiguous format.", fs::PathToString(path)));
+                return {util::Error{Untranslated(strprintf("Failed to load database path '%s'. Data is in ambiguous format.", fs::PathToString(path)))}, DatabaseError::FAILED_BAD_FORMAT};
-                status = DatabaseStatus::FAILED_BAD_FORMAT;
-                return nullptr;
             }
             format = DatabaseFormat::SQLITE;
         }
     } else if (options.require_existing) {
-        error = Untranslated(strprintf("Failed to load database path '%s'. Path does not exist.", fs::PathToString(path)));
+        return {util::Error{Untranslated(strprintf("Failed to load database path '%s'. Path does not exist.", fs::PathToString(path)))}, DatabaseError::FAILED_NOT_FOUND};
-        status = DatabaseStatus::FAILED_NOT_FOUND;
-        return nullptr;
     }
 
     if (!format && options.require_existing) {
-        error = Untranslated(strprintf("Failed to load database path '%s'. Data is not in recognized format.", fs::PathToString(path)));
+        return {util::Error{Untranslated(strprintf("Failed to load database path '%s'. Data is not in recognized format.", fs::PathToString(path)))}, DatabaseError::FAILED_BAD_FORMAT};
-        status = DatabaseStatus::FAILED_BAD_FORMAT;
-        return nullptr;
     }
 
     if (format && options.require_create) {
-        error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(path)));
+        return {util::Error{Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(path)))}, DatabaseError::FAILED_ALREADY_EXISTS};
-        status = DatabaseStatus::FAILED_ALREADY_EXISTS;
-        return nullptr;
     }
 
     // If BERKELEY was the format, then change the format from BERKELEY to BERKELEY_RO
@@ -1454,9 +1444,7 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
 
     // A db already exists so format is set, but options also specifies the format, so make sure they agree
     if (format && options.require_format && format != options.require_format) {
-        error = Untranslated(strprintf("Failed to load database path '%s'. Data is not in required format.", fs::PathToString(path)));
+        return {util::Error{Untranslated(strprintf("Failed to load database path '%s'. Data is not in required format.", fs::PathToString(path)))}, DatabaseError::FAILED_BAD_FORMAT};
-        status = DatabaseStatus::FAILED_BAD_FORMAT;
-        return nullptr;
     }
 
     // Format is not set when a db doesn't already exist, so use the format specified by the options if it is set.
@@ -1475,29 +1463,27 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
     if (format == DatabaseFormat::SQLITE) {
 #ifdef USE_SQLITE
         if constexpr (true) {
-            return MakeSQLiteDatabase(path, options, status, error);
+            return MakeSQLiteDatabase(path, options);
         } else
 #endif
         {
-            error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path)));
+            return {util::Error{Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path)))},
-            status = DatabaseStatus::FAILED_BAD_FORMAT;
+                    DatabaseError::FAILED_BAD_FORMAT};
-            return nullptr;
         }
     }
 
     if (format == DatabaseFormat::BERKELEY_RO) {
-        return MakeBerkeleyRODatabase(path, options, status, error);
+        return MakeBerkeleyRODatabase(path, options);
     }
 
 #ifdef USE_BDB
     if constexpr (true) {
-        return MakeBerkeleyDatabase(path, options, status, error);
+        return MakeBerkeleyDatabase(path, options);
     } else
 #endif
     {
-        error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)));
+        return {util::Error{Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)))},
-        status = DatabaseStatus::FAILED_BAD_FORMAT;
+                DatabaseError::FAILED_BAD_FORMAT};
-        return nullptr;
     }
 }
 } // namespace wallet

src/wallet/wallettool.cpp

@@ -47,16 +47,14 @@ static void WalletCreate(CWallet* wallet_instance, uint64_t wallet_creation_flag
 
 static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::path& path, DatabaseOptions options)
 {
-    DatabaseStatus status;
+    auto database{MakeDatabase(path, options)};
-    bilingual_str error;
-    std::unique_ptr<WalletDatabase> database = MakeDatabase(path, options, status, error);
     if (!database) {
-        tfm::format(std::cerr, "%s\n", error.original);
+        tfm::format(std::cerr, "%s\n", util::ErrorString(database).original);
         return nullptr;
     }
 
     // dummy chain interface
-    std::shared_ptr<CWallet> wallet_instance{new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet};
+    std::shared_ptr<CWallet> wallet_instance{new CWallet(/*chain=*/nullptr, name, std::move(database.value())), WalletToolReleaseWallet};
     DBErrors load_wallet_ret;
     try {
         load_wallet_ret = wallet_instance->LoadWallet();
@@ -171,18 +169,16 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
         wallet_instance->Close();
     } else if (command == "salvage") {
 #ifdef USE_BDB
-        bilingual_str error;
+        auto ret{RecoverDatabaseFile(args, path)};
-        std::vector<bilingual_str> warnings;
-        bool ret = RecoverDatabaseFile(args, path, error, warnings);
         if (!ret) {
-            for (const auto& warning : warnings) {
+            for (const auto& warning : ret.GetMessages()->warnings) {
                 tfm::format(std::cerr, "%s\n", warning.original);
             }
-            if (!error.empty()) {
+            for (const auto& error : ret.GetMessages()->errors) {
                 tfm::format(std::cerr, "%s\n", error.original);
             }
         }
-        return ret;
+        return bool{ret};
 #else
         tfm::format(std::cerr, "Salvage command is not available as BDB support is not compiled");
         return false;
@@ -191,37 +187,33 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
         DatabaseOptions options;
         ReadDatabaseArgs(args, options);
         options.require_existing = true;
-        DatabaseStatus status;
-
         if (args.GetBoolArg("-withinternalbdb", false) && IsBDBFile(BDBDataFile(path))) {
             options.require_format = DatabaseFormat::BERKELEY_RO;
         }
-
+        auto database{MakeDatabase(path, options)};
-        bilingual_str error;
-        std::unique_ptr<WalletDatabase> database = MakeDatabase(path, options, status, error);
         if (!database) {
-            tfm::format(std::cerr, "%s\n", error.original);
+            tfm::format(std::cerr, "%s\n", util::ErrorString(database).original);
             return false;
         }
 
-        bool ret = DumpWallet(args, *database, error);
+        auto ret{DumpWallet(args, *database)};
-        if (!ret && !error.empty()) {
+        if (!ret) {
-            tfm::format(std::cerr, "%s\n", error.original);
+            tfm::format(std::cerr, "%s\n", util::ErrorString(ret).original);
-            return ret;
+            return false;
         }
         tfm::format(std::cout, "The dumpfile may contain private keys. To ensure the safety of your Bitcoin, do not share the dumpfile.\n");
-        return ret;
+        return bool{ret};
     } else if (command == "createfromdump") {
-        bilingual_str error;
+        auto ret{CreateFromDump(args, name, path)};
-        std::vector<bilingual_str> warnings;
+        if (ret.GetMessages()) {
-        bool ret = CreateFromDump(args, name, path, error, warnings);
+            for (const auto& warning : ret.GetMessages()->warnings) {
-        for (const auto& warning : warnings) {
                 tfm::format(std::cout, "%s\n", warning.original);
             }
-        if (!ret && !error.empty()) {
+            for (const auto& error : ret.GetMessages()->errors) {
                 tfm::format(std::cerr, "%s\n", error.original);
             }
-        return ret;
+        }
+        return bool{ret};
     } else {
         tfm::format(std::cerr, "Invalid command: %s\n", command);
         return false;