diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 889f7b3859b..11a6ddd1191 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -188,7 +188,7 @@ void ReadFromStream(AddrMan& addr, DataStream& ssPeers) DeserializeDB(ssPeers, addr, false); } -util::Result> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args) +util::ResultPtr> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args) { auto check_addrman = std::clamp(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); bool deterministic = HasTestOption(args, "addrman"); // use a deterministic addrman only for tests diff --git a/src/addrdb.h b/src/addrdb.h index cc3014dce29..c6c28257c5c 100644 --- a/src/addrdb.h +++ b/src/addrdb.h @@ -49,7 +49,7 @@ public: }; /** Returns an error string on failure */ -util::Result> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args); +util::ResultPtr> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args); /** * Dump the anchor IP address database (anchors.dat) diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp index 3b916d7c39b..27b93520e12 100644 --- a/src/bench/wallet_create.cpp +++ b/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 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); - assert(status == DatabaseStatus::SUCCESS); - assert(wallet != nullptr); + auto wallet{CreateWallet(context, wallet_path, /*load_on_start=*/std::nullopt, options)}; + assert(wallet); // Release wallet - RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt); - WaitForDeleteWallet(std::move(wallet)); + RemoveWallet(context, wallet.value(), /*load_on_start=*/ std::nullopt); + WaitForDeleteWallet(std::move(wallet.value())); fs::remove_all(wallet_path); }); } diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index e657f018715..8be48d733fd 100644 --- a/src/bitcoin-chainstate.cpp +++ b/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); - if (status != node::ChainstateLoadStatus::SUCCESS) { + auto load_result{node::LoadChainstate(chainman, cache_sizes, options)}; + 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); - if (status != node::ChainstateLoadStatus::SUCCESS) { + auto verify_result{node::VerifyLoadedChainstate(chainman, options)}; + if (!verify_result) { std::cerr << "Failed to verify loaded Chain state from your datadir." << std::endl; goto epilogue; } diff --git a/src/init.cpp b/src/init.cpp index 7fdbf75dc66..c6048b3cae1 100644 --- a/src/init.cpp +++ b/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::ChainstateLoadStatus; +using node::ChainstateLoadError; 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 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(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(*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 { 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); }); - if (status == node::ChainstateLoadStatus::SUCCESS) { + auto result = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); }); + 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); }); - if (status == node::ChainstateLoadStatus::SUCCESS) { + result.Update(catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); })); + 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) { - return InitError(error); + if (!result) { + return InitError(util::ErrorString(result)); } // As LoadBlockIndex can take several minutes, it's possible the user diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index df1ced48a71..c62d127bc94 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -328,16 +328,16 @@ class WalletLoader : public ChainClient { public: //! Create new wallet. - virtual util::Result> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector& warnings) = 0; + virtual util::ResultPtr> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags) = 0; //! Load existing wallet. - virtual util::Result> loadWallet(const std::string& name, std::vector& warnings) = 0; + virtual util::ResultPtr> loadWallet(const std::string& name) = 0; //! Return default wallet directory. virtual std::string getWalletDir() = 0; //! Restore backup wallet - virtual util::Result> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) = 0; + virtual util::ResultPtr> restoreWallet(const fs::path& backup_file, const std::string& wallet_name) = 0; //! Migrate a wallet virtual util::Result migrateWallet(const std::string& name, const SecureString& passphrase) = 0; diff --git a/src/kernel/CMakeLists.txt b/src/kernel/CMakeLists.txt index b9f37969d37..4d1d689de21 100644 --- a/src/kernel/CMakeLists.txt +++ b/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 diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index c88bd5bad23..89437db5a3d 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -27,35 +27,40 @@ #include 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 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, {}}; - return {ChainstateLoadStatus::FAILURE, _("Error loading block database")}; + if (chainman.m_interrupt) { + 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. " - "Please restart with -reindex-chainstate. This will " - "rebuild the chainstate database.")}; + return {util::Error{_("Unsupported chainstate database format found. " + "Please restart with -reindex-chainstate. This will " + "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."), - chainman.GetConsensus().SegwitHeight)}; + return {util::Error{strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."), + 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, - const ChainstateLoadOptions& options) +util::Result LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes, + const ChainstateLoadOptions& options) { + util::Result 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); - if (init_status != ChainstateLoadStatus::SUCCESS) { - return {init_status, init_error}; + result.Update(CompleteChainstateInitialization(chainman, options)); + if (!result || IsInterrupted(*result)) { + 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); - if (init_status != ChainstateLoadStatus::SUCCESS) { - return {init_status, init_error}; + auto result{CompleteChainstateInitialization(chainman, options)}; + if (!result || IsInterrupted(*result)) { + 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 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 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. " - "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")}; + 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")}, + 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 diff --git a/src/node/chainstate.h b/src/node/chainstate.h index 843e8e09a66..dbaa8c3ad03 100644 --- a/src/node/chainstate.h +++ b/src/node/chainstate.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_NODE_CHAINSTATE_H #define BITCOIN_NODE_CHAINSTATE_H +#include +#include #include #include @@ -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 { - SUCCESS, +enum class ChainstateLoadError { 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. -using ChainstateLoadResult = std::tuple; - -/** 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 LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes, + const ChainstateLoadOptions& options); +util::Result VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options); } // namespace node #endif // BITCOIN_NODE_CHAINSTATE_H diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index dd093e984a3..08b41c4e8a2 100644 --- a/src/qt/walletcontroller.cpp +++ b/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)); - } else { - m_error_message = util::ErrorString(wallet); + m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value())); } QTimer::singleShot(500ms, this, &CreateWalletActivity::finish); @@ -356,12 +359,15 @@ void OpenWalletActivity::open(const std::string& path) tr("Opening Wallet %1…").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)); - } else { - m_error_message = util::ErrorString(wallet); + m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value())); } QTimer::singleShot(0, this, &OpenWalletActivity::finish); @@ -409,12 +415,15 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri tr("Restoring Wallet %1…").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)); - } else { - m_error_message = util::ErrorString(wallet); + m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet.value())); } QTimer::singleShot(0, this, &RestoreWalletActivity::finish); diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index 4284864422d..327625f90d3 100644 --- a/src/test/result_tests.cpp +++ b/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 +#include #include +#include +#include #include +#include +#include +#include +#include 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 VoidSuccessFn() +{ + return {}; +} + +util::Result VoidFailFn() +{ + return util::Error{Untranslated("void fail.")}; +} + util::Result IntFn(int i, bool success) { if (success) return i; @@ -45,21 +81,91 @@ util::Result StrFn(bilingual_str s, bool success) return util::Error{Untranslated(strprintf("str %s error.", s.original))}; } -util::Result NoCopyFn(int i, bool success) +util::Result 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 -void ExpectResult(const util::Result& result, bool success, const bilingual_str& str) +util::Result NoCopyNoMoveFn(int i, bool success) +{ + if (success) return {i}; + return {util::Error{Untranslated(strprintf("nocopynomove %i error.", i))}, i}; +} + +enum FnError { ERR1, ERR2 }; + +util::Result 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 StrFailFn(int i, bool success) +{ + util::Result 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 EnumFailFn(FnError ret) +{ + return {util::Error{Untranslated("enum fail.")}, ret}; +} + +util::Result WarnFn() +{ + return {util::Warning{Untranslated("warn.")}}; +} + +util::Result MultiWarnFn(int ret) +{ + util::Result result; + for (int i = 0; i < ret; ++i) { + result.AddWarning(Untranslated(strprintf("warn %i.", i))); + } + result.Update(ret); + return result; +} + +util::Result ChainedFailFn(FnError arg, int ret) +{ + util::Result result{util::Error{Untranslated("chained fail.")}, ret}; + EnumFailFn(arg) >> result; + WarnFn() >> result; + return result; +} + +util::Result AccumulateFn(bool success) +{ + util::Result result; + util::Result x = MultiWarnFn(1) >> result; + BOOST_REQUIRE(x); + util::Result y = MultiWarnFn(2) >> result; + BOOST_REQUIRE(y); + result.Update(IntFailFn(*x + *y, success)); + return result; +} + +util::Result TruthyFalsyFn(int i, bool success) +{ + if (success) return i; + return {util::Error{Untranslated(strprintf("failure value %i.", i))}, i}; +} + +template +void ExpectResult(const util::Result& result, bool success, const bilingual_str& str) { BOOST_CHECK_EQUAL(bool(result), success); BOOST_CHECK_EQUAL(util::ErrorString(result), str); } -template -void ExpectSuccess(const util::Result& result, const bilingual_str& str, Args&&... args) +template +void ExpectSuccess(const util::Result& 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& result, const bilingual_str& str, Args BOOST_CHECK_EQUAL(&result.value(), &*result); } -template -void ExpectFail(const util::Result& result, const bilingual_str& str) +template +void ExpectFail(const util::Result& result, bilingual_str str, Args&&... args) { ExpectResult(result, false, str); + F expect_failure{std::forward(args)...}; + BOOST_CHECK_EQUAL(result.GetFailure(), expect_failure); +} + +BOOST_AUTO_TEST_CASE(check_sizes) +{ + static_assert(sizeof(util::Result) == sizeof(void*)*2); + static_assert(sizeof(util::Result) == 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 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 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> 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 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> DerivedToBaseFn(util::Result> 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(5)))), 5); + + // Check conversions work between util::Result and util::ResultPtr + util::ResultPtr> derived{std::make_unique(5)}; + util::ResultPtr> 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>> PtrFn(std::optional> i, bool success) +{ + if (success) return i ? std::make_unique>(*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() diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index bf26997c076..db20688ae17 100644 --- a/src/test/util/setup_common.cpp +++ b/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); - assert(status == node::ChainstateLoadStatus::SUCCESS); + auto load_result{LoadChainstate(chainman, m_kernel_cache_sizes, options)}; + Assert(load_result); - std::tie(status, error) = VerifyLoadedChainstate(chainman, options); - assert(status == node::ChainstateLoadStatus::SUCCESS); + auto verify_result{VerifyLoadedChainstate(chainman, options)}; + Assert(verify_result); BlockValidationState state; if (!chainman.ActiveChainstate().ActivateBestChain(state)) { diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 4999dbf13f0..dc5121f77cf 100644 --- a/src/util/CMakeLists.txt +++ b/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 diff --git a/src/util/result.cpp b/src/util/result.cpp new file mode 100644 index 00000000000..ea79375aa3a --- /dev/null +++ b/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 + +#include +#include +#include +#include + +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::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 diff --git a/src/util/result.h b/src/util/result.h index 122a7638fad..dc7ee05d686 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -8,91 +8,425 @@ #include #include -#include +#include +#include +#include +#include +#include +#include +#include namespace util { +//! Default MessagesType, simple list of errors and warnings. +struct Messages { + std::vector errors{}; + std::vector warnings{}; +}; +//! The Result 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, MessagesType> +//! +//! But the physical representation is more efficient because it avoids +//! allocating memory for FailureType and MessagesType fields unless there is an +//! error. +//! +//! Result objects support the same operators as +//! std::optional, 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 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 +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 -//! either error messages or result values. -//! -//! It is intended for high-level functions that need to report error strings to -//! end users. Lower-level functions that don't need this error-reporting and -//! only need error-handling should avoid util::Result and instead use standard -//! classes like std::optional, std::variant, and std::tuple, or custom structs -//! and enum types to return function results. -//! -//! Usage examples can be found in \example ../test/result_tests.cpp, but in -//! general code returning `util::Result` values is very similar to code -//! returning `std::optional` values. Existing functions returning -//! `std::optional` can be updated to return `util::Result` and return -//! error strings usually just replacing `return std::nullopt;` with `return -//! util::Error{error_string};`. -template -class Result +//! Type trait that can be specialized to control the way SuccessType / +//! FailureType / MessagesType values are combined. Default behavior +//! is for new values to overwrite existing ones, but this can be specialized +//! for custom behavior when Result::Update() method is called or << operator is +//! used. For example, this is specialized for Messages struct below to append +//! error and warning messages instead of overwriting them. It can also be used, +//! for example, to merge FailureType values when a function can return multiple +//! failures. +template +struct ResultTraits { + template + static void Update(O& dst, T& src) + { + dst = std::move(src); + } +}; + +//! Type trait that can be specialized to let the Result class work with custom +//! MessagesType types holding error and warning messages. +template +struct MessagesTraits; + +//! ResultTraits specialization for Messages struct. +template<> +struct ResultTraits { + static void Update(Messages& dst, Messages& src); +}; + +//! MessagesTraits specialization for Messages struct. +template<> +struct MessagesTraits { + static void AddError(Messages& messages, bilingual_str error) + { + 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(); + } +}; + +namespace detail { +//! 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 +class FailDataHolder { -private: - using T = std::conditional_t, std::monostate, M>; +protected: + struct FailData { + std::optional, Monostate, FailureType>> failure{}; + MessagesType messages{}; + }; + std::unique_ptr m_fail_data; - std::variant m_variant; - - //! Disallow copy constructor, require Result to be moved for efficiency. - Result(const Result&) = delete; - - //! Disallow operator= to avoid confusion in the future when the Result - //! class gains support for richer error reporting, and callers should have - //! ability to set a new result value without clearing existing error - //! messages. - Result& operator=(const Result&) = delete; - Result& operator=(Result&&) = delete; - - template - friend bilingual_str ErrorString(const Result& result); + // Private accessor, create FailData if it doesn't exist. + FailData& EnsureFailData() LIFETIMEBOUND + { + if (!m_fail_data) m_fail_data = std::make_unique(); + return *m_fail_data; + } public: - Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {} // constructor for void - Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {} - Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {} - Result(Result&&) = default; - ~Result() = default; + // Public accessors. + explicit operator bool() const { return !m_fail_data || !m_fail_data->failure; } + const auto& GetFailure() const LIFETIMEBOUND { assert(!*this); return *m_fail_data->failure; } + auto& GetFailure() LIFETIMEBOUND { assert(!*this); return *m_fail_data->failure; } + 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 can change to - //! return Result with minimal changes to existing code, and vice versa. - bool has_value() const noexcept { return m_variant.index() == 1; } - const T& value() const LIFETIMEBOUND - { - assert(has_value()); - return std::get<1>(m_variant); - } - T& value() LIFETIMEBOUND - { - assert(has_value()); - return std::get<1>(m_variant); - } +//! Container for SuccessType, providing public accessor methods similar to +//! std::optional methods to access the success value. +template +class SuccessHolder : public FailDataHolder +{ +protected: + //! Success value embedded in an anonymous union so it doesn't need to be + //! constructed if the result is holding a failure value, + union { SuccessType m_success; }; + + //! Empty constructor that needs to be declared because the class contains a union. + 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 - T value_or(U&& default_value) const& + SuccessType value_or(U&& default_value) const& { return has_value() ? value() : std::forward(default_value); } template - T value_or(U&& default_value) && + SuccessType value_or(U&& default_value) && { return has_value() ? std::move(value()) : std::forward(default_value); } - explicit operator bool() const noexcept { return has_value(); } - const T* operator->() const LIFETIMEBOUND { return &value(); } - const T& operator*() const LIFETIMEBOUND { return value(); } - T* operator->() LIFETIMEBOUND { return &value(); } - T& operator*() LIFETIMEBOUND { return value(); } + const SuccessType* operator->() const LIFETIMEBOUND { return &value(); } + const SuccessType& operator*() const LIFETIMEBOUND { return value(); } + SuccessType* operator->() LIFETIMEBOUND { return &value(); } + SuccessType& operator*() LIFETIMEBOUND { return value(); } }; -template -bilingual_str ErrorString(const Result& result) +//! Specialization of SuccessHolder when SuccessType is void. +template +class SuccessHolder : public FailDataHolder { - return result ? bilingual_str{} : std::get<0>(result.m_variant); +}; +//! @} +} // namespace detail + +// Result type class, documented at the top of this file. +template +class Result : public detail::SuccessHolder +{ +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 + Result(Args&&... args) + { + Construct(*this, std::forward(args)...); + } + + //! Move-construct a Result object from another Result object, moving the + //! success or failure value and any error or warning messages. + template + requires (std::decay_t::is_result) + Result(O&& other) + { + Move(*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(*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::AddError(this->EnsureFailData().messages, std::move(error)); + } + void AddWarning(bilingual_str warning) + { + if (!warning.empty()) MessagesTraits::AddWarning(this->EnsureFailData().messages, std::move(warning)); + } + +protected: + template + friend class Result; + + //! Helper function to construct a new success or failure value using the + //! arguments provided. + template + static void Construct(Result& result, Args&&... args) + { + if constexpr (Failure) { + static_assert(sizeof...(args) > 0 || !std::is_scalar_v, + "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)...); + } else if constexpr (std::is_same_v) { + static_assert(sizeof...(args) == 0, "Error: Arguments cannot be passed to a Result constructor."); + } else { + new (&result.m_success) typename Result::SuccessType{std::forward(args)...}; + } + } + + //! Construct() overload peeling off a util::Error constructor argument. + template + static void Construct(Result& result, util::Error error, Args&&... args) + { + result.AddError(std::move(error.message)); + Construct(result, std::forward(args)...); + } + + //! Construct() overload peeling off a util::Warning constructor argument. + template + static void Construct(Result& result, util::Warning warning, Args&&... args) + { + result.AddWarning(std::move(warning.message)); + Construct(result, std::forward(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 + 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) { + ResultTraits::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) { + ResultTraits::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) { + 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) { + new (&dst.m_success) typename DstResult::SuccessType{std::move(src.m_success)}; + } else if constexpr (!std::is_same_v) { + 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) { + 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 result; +//! auto r1 = DoSomething() >> result; +//! auto r2 = DoSomethingElse() >> result; +//! ... +//! return result; +//! +template +requires (std::decay_t::is_result) +decltype(auto) operator>>(SrcResult&& src LIFETIMEBOUND, DstResult&& dst) +{ + using SrcType = std::decay_t; + if (src.GetMessages() && MessagesTraits::HasMessages(*src.GetMessages())) { + ResultTraits::Update(dst.EnsureMessages(), *src.GetMessages()); + } + return static_cast(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 +class ResultPtr : public Result +{ +public: + // Inherit Result constructors (excluding copy and move constructors). + using Result::Result; + + // Inherit Result copy and move constructors. + template + ResultPtr(O&& other) : Result{std::forward(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 +bilingual_str ErrorString(const Result& result) +{ + const auto* messages{result.GetMessages()}; + return messages ? detail::JoinMessages(*messages) : bilingual_str{}; } } // namespace util diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 79851dff33f..a54b39c1063 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -143,18 +143,17 @@ BerkeleyEnvironment::~BerkeleyEnvironment() Close(); } -bool BerkeleyEnvironment::Open(bilingual_str& err) +util::Result 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 false; + return {util::Error{strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory())))}}; } 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 env, fs: assert(inserted.second); } -bool BerkeleyDatabase::Verify(bilingual_str& errorStr) +util::Result 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)) { - return false; - } + util::Result opened = env->Open(); + 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 false; + 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)))}}; } } // 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; - if (!env->Open(open_err)) - throw std::runtime_error("BerkeleyDatabase: Failed to open database environment."); + auto opened = env->Open(); + if (!opened) { + 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; - Open(open_err); + auto opened = Open(); + 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 BerkeleyDatabase::MakeBatch(bool flush_on_close) return std::make_unique(*this, false, flush_on_close); } -std::unique_ptr MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) +util::ResultPtr, DatabaseError> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options) { + util::ResultPtr, DatabaseError> result; + fs::path data_file = BDBDataFile(path); std::unique_ptr db; { @@ -954,19 +954,19 @@ std::unique_ptr MakeBerkeleyDatabase(const fs::path& path, con fs::path data_filename = data_file.filename(); std::shared_ptr 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))); - status = DatabaseStatus::FAILED_ALREADY_LOADED; - return nullptr; + result.Update({util::Error{Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", fs::PathToString(env->Directory() / data_filename)))}, + DatabaseError::FAILED_ALREADY_LOADED}); + return result; } db = std::make_unique(std::move(env), std::move(data_filename), options); } - if (options.verify && !db->Verify(error)) { - status = DatabaseStatus::FAILED_VERIFY; - return nullptr; + if (options.verify && !(db->Verify() >> result)) { + result.Update({util::Error{}, DatabaseError::FAILED_VERIFY}); + return result; } - status = DatabaseStatus::SUCCESS; - return db; + result.Update(std::move(db)); + return result; } } // namespace wallet diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index f3fe8a19c19..97cf8fa4e45 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -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 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 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 MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +util::ResultPtr, DatabaseError> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options); } // namespace wallet #endif // BITCOIN_WALLET_BDB_H diff --git a/src/wallet/db.h b/src/wallet/db.h index e8790006a4d..2149e018eb3 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -202,8 +203,7 @@ struct DatabaseOptions { int64_t max_log_mb = 100; //!< Max log size to allow before consolidating. }; -enum class DatabaseStatus { - SUCCESS, +enum class DatabaseError { FAILED_BAD_PATH, FAILED_BAD_FORMAT, FAILED_ALREADY_LOADED, @@ -220,7 +220,7 @@ enum class DatabaseStatus { std::vector> ListDatabases(const fs::path& path); void ReadDatabaseArgs(const ArgsManager& args, DatabaseOptions& options); -std::unique_ptr MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +util::ResultPtr, DatabaseError> MakeDatabase(const fs::path& path, const DatabaseOptions& options); fs::path BDBDataFile(const fs::path& path); fs::path SQLiteDataFile(const fs::path& path); diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index db2756e0ca8..a7968b08369 100644 --- a/src/wallet/dump.cpp +++ b/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 DumpWallet(const ArgsManager& args, WalletDatabase& db) { + util::Result result; // Get the dumpfile std::string dump_filename = args.GetArg("-dumpfile", ""); if (dump_filename.empty()) { - error = _("No dump file provided. To use dump, -dumpfile= must be provided."); - return false; + result.Update(util::Error{_("No dump file provided. To use dump, -dumpfile= must be provided.")}); + 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)); - return false; + 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 result; } std::ofstream dump_file; dump_file.open(path); if (dump_file.fail()) { - error = strprintf(_("Unable to open %s for writing"), fs::PathToString(path)); - return false; + result.Update(util::Error{strprintf(_("Unable to open %s for writing"), fs::PathToString(path))}); + return result; } HashWriter hasher{}; std::unique_ptr batch = db.MakeBatch(); - bool ret = true; std::unique_ptr cursor = batch->GetNewCursor(); if (!cursor) { - error = _("Error: Couldn't create cursor into database"); - ret = false; + result.Update(util::Error{_("Error: Couldn't create cursor into database")}); } // 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"); - ret = false; + result.Update(util::Error{_("Error reading next record from wallet database")}); 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& warnings) +util::Result CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path) { + util::Result result; // Get the dumpfile std::string dump_filename = args.GetArg("-dumpfile", ""); if (dump_filename.empty()) { - error = _("No dump file provided. To use createfromdump, -dumpfile= must be provided."); - return false; + result.Update(util::Error{_("No dump file provided. To use createfromdump, -dumpfile= must be provided.")}); + 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)); - return false; + result.Update(util::Error{strprintf(_("Dump file %s does not exist."), fs::PathToString(dump_path))}); + 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= must be provided."); - return false; + result.Update(util::Error{_("No wallet file format provided. To use createfromdump, -format= must be provided.")}); + 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); - return false; + result.Update(util::Error{strprintf(_("Unknown wallet file format \"%s\" provided. Please provide one of \"bdb\" or \"sqlite\"."), file_format)}); + 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 database = MakeDatabase(wallet_path, options, status, error); - if (!database) return false; + auto database{MakeDatabase(wallet_path, options) >> result}; + if (!database) { + result.Update(util::Error{}); + return result; + } // dummy chain interface - bool ret = true; - std::shared_ptr wallet(new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet); + std::shared_ptr wallet(new CWallet(/*chain=*/nullptr, name, std::move(database.value())), WalletToolReleaseWallet); { LOCK(wallet->cs_wallet); DBErrors load_wallet_ret = wallet->LoadWallet(); if (load_wallet_ret != DBErrors::LOAD_OK) { - error = strprintf(_("Error creating %s"), name); - return false; + result.Update(util::Error{strprintf(_("Error creating %s"), name)}); + 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 parsed_checksum = ParseHex(value); if (parsed_checksum.size() != checksum.size()) { - error = Untranslated("Error: Checksum is not the correct size"); - ret = false; + result.Update(util::Error{Untranslated("Error: Checksum is not the correct size")}); 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); - ret = false; + result.Update(util::Error{strprintf(_("Error: Got key that was not hex: %s"), key)}); break; } if (!IsHex(value)) { - error = strprintf(_("Error: Got value that was not hex: %s"), value); - ret = false; + result.Update(util::Error{strprintf(_("Error: Got value that was not hex: %s"), value)}); break; } std::vector k = ParseHex(key); std::vector v = ParseHex(value); if (!batch->Write(Span{k}, Span{v})) { - error = strprintf(_("Error: Unable to write record to new wallet")); - ret = false; + result.Update(util::Error{strprintf(_("Error: Unable to write record to new wallet"))}); break; } } - if (ret) { + if (result) { uint256 comp_checksum = hasher.GetHash(); if (checksum.IsNull()) { - error = _("Error: Missing checksum"); - ret = false; + result.Update(util::Error{_("Error: Missing checksum")}); } else if (checksum != comp_checksum) { - error = strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum)); - ret = false; + result.Update(util::Error{strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum))}); } } - 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 diff --git a/src/wallet/dump.h b/src/wallet/dump.h index 9b44af922e1..9f641acd1ec 100644 --- a/src/wallet/dump.h +++ b/src/wallet/dump.h @@ -6,6 +6,7 @@ #define BITCOIN_WALLET_DUMP_H #include +#include #include #include @@ -16,8 +17,8 @@ class ArgsManager; namespace wallet { class WalletDatabase; -bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error); -bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector& warnings); +util::Result DumpWallet(const ArgsManager& args, WalletDatabase& db); +util::Result CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path); } // namespace wallet #endif // BITCOIN_WALLET_DUMP_H diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 21e8a0b3bd2..c1fa8c70260 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -282,15 +282,19 @@ public: int& change_pos, CAmount& fee) override { + util::Result result; LOCK(m_wallet->cs_wallet); - auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos), - coin_control, sign); - if (!res) return util::Error{util::ErrorString(res)}; - const auto& txr = *res; + auto created{CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos), + coin_control, sign) >> result}; + if (!created) { + result.Update(util::Error{}); + return result; + } + const auto& txr = *created; fee = txr.fee; change_pos = txr.change_pos ? int(*txr.change_pos) : -1; - - return txr.tx; + result.Update(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> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector& warnings) override + util::ResultPtr> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags) override { + util::ResultPtr> 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; - std::unique_ptr wallet{MakeWallet(m_context, CreateWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))}; - if (wallet) { - return wallet; + if (auto wallet{CreateWallet(m_context, name, true /* load_on_start */, options) >> result}) { + result.Update(MakeWallet(m_context, wallet.value())); } else { - return util::Error{error}; + result.Update(util::Error{}); } + return result; } - util::Result> loadWallet(const std::string& name, std::vector& warnings) override + util::ResultPtr> loadWallet(const std::string& name) override { + util::ResultPtr> result; DatabaseOptions options; - DatabaseStatus status; ReadDatabaseArgs(*m_context.args, options); options.require_existing = true; - bilingual_str error; - std::unique_ptr wallet{MakeWallet(m_context, LoadWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))}; - if (wallet) { - return wallet; + if (auto wallet{LoadWallet(m_context, name, true /* load_on_start */, options) >> result}) { + result.Update(MakeWallet(m_context, wallet.value())); } else { - return util::Error{error}; + result.Update(util::Error{}); } + return result; } - util::Result> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) override + util::ResultPtr> restoreWallet(const fs::path& backup_file, const std::string& wallet_name) override { - DatabaseStatus status; - bilingual_str error; - std::unique_ptr wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))}; - if (wallet) { - return wallet; + util::ResultPtr> result; + if (auto wallet{RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true) >> result}) { + result.Update(MakeWallet(m_context, wallet.value())); } else { - return util::Error{error}; + result.Update(util::Error{}); } + return result; } util::Result migrateWallet(const std::string& name, const SecureString& passphrase) override { - auto res = wallet::MigrateLegacyToDescriptor(name, passphrase, m_context); - if (!res) return util::Error{util::ErrorString(res)}; - WalletMigrationResult out{ + util::Result result; + if (auto res{wallet::MigrateLegacyToDescriptor(name, passphrase, m_context) >> result}) { + 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; - bilingual_str error; - auto db = MakeWalletDatabase(wallet_name, options, status, error); + auto db{MakeWalletDatabase(wallet_name, options)}; if (!db) return false; return WalletBatch(*db).IsEncrypted(); } diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index e77999b1115..309b59e7871 100644 --- a/src/wallet/load.cpp +++ b/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; - if (!MakeWalletDatabase(wallet_file, options, status, error_string)) { - if (status == DatabaseStatus::FAILED_NOT_FOUND) { - chain.initWarning(Untranslated(strprintf("Skipping -wallet path that doesn't exist. %s", error_string.original))); + auto result{MakeWalletDatabase(wallet_file, options)}; + if (!result) { + if (result.GetFailure() == DatabaseError::FAILED_NOT_FOUND) { + 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; - std::vector warnings; - std::unique_ptr database = MakeWalletDatabase(name, options, status, error); - if (!database && status == DatabaseStatus::FAILED_NOT_FOUND) { + util::Result result; + auto database{MakeWalletDatabase(name, options) >> result}; + if (!database && database.GetFailure() == DatabaseError::FAILED_NOT_FOUND) { continue; } chain.initMessage(_("Loading wallet…")); - std::shared_ptr pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings) : nullptr; - if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); + auto pwallet{database ? CWallet::Create(context, name, std::move(database.value()), options.create_flags) >> result : nullptr}; + 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); - AddWallet(context, pwallet); + NotifyWalletLoaded(context, pwallet.value()); + 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 warnings; - RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt, warnings); + // Note: Warnings returned from RemoveWallet are silently discarded. + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt); WaitForDeleteWallet(std::move(wallet)); } } diff --git a/src/wallet/migrate.cpp b/src/wallet/migrate.cpp index d2c163027d2..49fb20354c3 100644 --- a/src/wallet/migrate.cpp +++ b/src/wallet/migrate.cpp @@ -768,17 +768,13 @@ std::unique_ptr BerkeleyROBatch::GetNewPrefixCursor(Span(m_database, prefix); } -std::unique_ptr MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) +util::ResultPtr, DatabaseError> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options) { fs::path data_file = BDBDataFile(path); try { - std::unique_ptr db = std::make_unique(data_file); - status = DatabaseStatus::SUCCESS; - return db; + return std::make_unique(data_file); } catch (const std::runtime_error& e) { - error.original = e.what(); - status = DatabaseStatus::FAILED_LOAD; - return nullptr; + return {util::Error{Untranslated(e.what())}, DatabaseError::FAILED_LOAD}; } } } // namespace wallet diff --git a/src/wallet/migrate.h b/src/wallet/migrate.h index 16eadeb019d..ca00e317067 100644 --- a/src/wallet/migrate.h +++ b/src/wallet/migrate.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_WALLET_MIGRATE_H #define BITCOIN_WALLET_MIGRATE_H +#include #include #include @@ -119,7 +120,7 @@ public: }; //! Return object giving access to Berkeley Read Only database at specified path. -std::unique_ptr MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +util::ResultPtr, DatabaseError> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options); } // namespace wallet #endif // BITCOIN_WALLET_MIGRATE_H diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index ac23b092d40..21077b5c908 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1963,17 +1963,15 @@ RPCHelpMan restorewallet() std::optional load_on_start = request.params[2].isNull() ? std::nullopt : std::optional(request.params[2].get_bool()); - DatabaseStatus status; - bilingual_str error; - std::vector warnings; + auto wallet{RestoreWallet(context, backup_file, wallet_name, load_on_start)}; - const std::shared_ptr wallet = RestoreWallet(context, backup_file, wallet_name, load_on_start, status, error, warnings); - - HandleWalletError(wallet, status, error); + HandleWalletError(wallet); UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); - PushWarnings(warnings, obj); + if (wallet.GetMessages()) { + PushWarnings(wallet.GetMessages()->warnings, obj); + } return obj; diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 414f0deeb2d..377c7093e71 100644 --- a/src/wallet/rpc/util.cpp +++ b/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 wallet, DatabaseStatus& status, bilingual_str& error) +void HandleWalletError(const util::ResultPtr, 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) { - case DatabaseStatus::FAILED_NOT_FOUND: - case DatabaseStatus::FAILED_BAD_FORMAT: + switch (wallet.GetFailure()) { + case DatabaseError::FAILED_NOT_FOUND: + 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); } } diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h index 002d0355e50..ab0671e9c13 100644 --- a/src/wallet/rpc/util.h +++ b/src/wallet/rpc/util.h @@ -7,6 +7,7 @@ #include #include