diff --git a/src/Makefile.am b/src/Makefile.am index c2e0c7b5b87..90202f0853f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -746,6 +746,7 @@ libbitcoin_util_a_SOURCES = \ util/moneystr.cpp \ util/rbf.cpp \ util/readwritefile.cpp \ + util/result.cpp \ util/signalinterrupt.cpp \ util/thread.cpp \ util/threadinterrupt.cpp \ @@ -984,6 +985,7 @@ libbitcoinkernel_la_SOURCES = \ 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/addrdb.cpp b/src/addrdb.cpp index 14dc314c365..9296291c043 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -190,7 +190,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 32f55f51e15..ff06435e766 100644 --- a/src/bench/wallet_create.cpp +++ b/src/bench/wallet_create.cpp @@ -32,18 +32,13 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted) options.create_passphrase = random.rand256().ToString(); } - DatabaseStatus status; - bilingual_str error_string; - std::vector warnings; - fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str(); bench.run([&] { - auto wallet = CreateWallet(context, wallet_path.utf8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings); - assert(status == DatabaseStatus::SUCCESS); - assert(wallet != nullptr); + auto wallet = CreateWallet(context, wallet_path.utf8string(), /*load_on_start=*/std::nullopt, options); + assert(wallet); // Cleanup - wallet.reset(); + wallet.value().reset(); fs::remove_all(wallet_path); }); } diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 642af06e82d..e193f17bffd 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -122,13 +122,13 @@ int main(int argc, char* argv[]) cache_sizes.coins_db = 2 << 22; cache_sizes.coins = (450 << 20) - (2 << 20) - (2 << 22); node::ChainstateLoadOptions options; - auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options); - if (status != node::ChainstateLoadStatus::SUCCESS) { + auto result = node::LoadChainstate(chainman, cache_sizes, options); + if (!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) { + result = node::VerifyLoadedChainstate(chainman, options); + if (!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 c19d596c7fd..5cf840b314c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -994,7 +994,7 @@ bool AppInitParameterInteraction(const ArgsManager& args) // ********************************************************* Step 3: parameter-to-internal-flags auto result = init::SetLoggingCategories(args); if (!result) return InitError(util::ErrorString(result)); - result = init::SetLoggingLevel(args); + result.Set(init::SetLoggingLevel(args)); if (!result) return InitError(util::ErrorString(result)); nConnectTimeout = args.GetIntArg("-timeout", DEFAULT_CONNECT_TIMEOUT); @@ -1261,7 +1261,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) uiInterface.InitMessage(_("Loading P2P addresses…").translated); auto addrman{LoadAddrman(*node.netgroupman, args)}; if (!addrman) return InitError(util::ErrorString(addrman)); - node.addrman = std::move(*addrman); + node.addrman = std::move(addrman.value()); } assert(!node.banman); @@ -1575,33 +1575,36 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) uiInterface.InitMessage(_("Loading block index…").translated); const auto load_block_index_start_time{SteadyClock::now()}; - auto catch_exceptions = [](auto&& f) { + auto catch_exceptions = [](auto&& f) -> util::Result { try { return f(); } catch (const std::exception& e) { LogPrintf("%s\n", e.what()); - return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error opening block database")); + return {util::Error{_("Error opening block database")}, 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) { uiInterface.InitMessage(_("Verifying blocks…").translated); 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 = catch_exceptions([&]{ return VerifyLoadedChainstate(chainman, options);}); + if (result) { fLoaded = true; LogPrintf(" block index %15dms\n", Ticks(SteadyClock::now() - load_block_index_start_time)); } } - if (status == node::ChainstateLoadStatus::FAILURE_FATAL || status == node::ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB || status == node::ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE) { - return InitError(error); + if (!result && (result.GetFailure() == node::ChainstateLoadError::FAILURE_FATAL || + result.GetFailure() == node::ChainstateLoadError::FAILURE_INCOMPATIBLE_DB || + result.GetFailure() == node::ChainstateLoadError::FAILURE_INSUFFICIENT_DBCACHE)) { + return InitError(util::ErrorString(result)); } if (!fLoaded && !ShutdownRequested(node)) { + bilingual_str error = util::ErrorString(result); // first suggest a reindex if (!options.reindex) { bool fRet = uiInterface.ThreadSafeQuestion( diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index c41f35829d7..f86b603a8b6 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -323,16 +323,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/node/chainstate.cpp b/src/node/chainstate.cpp index bf1fc06b0b5..d2c485e8124 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -32,7 +32,7 @@ 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 CacheSizes& cache_sizes, const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) @@ -56,28 +56,28 @@ static ChainstateLoadResult CompleteChainstateInitialization( } } - if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}}; + if (chainman.m_interrupt) return {util::Error{}, ChainstateLoadError::INTERRUPTED}; // LoadBlockIndex will load m_have_pruned if we've ever removed a // block file from disk. // Note that it also sets fReindex global based on the disk flag! // From here on, fReindex and options.reindex values may be different! if (!chainman.LoadBlockIndex()) { - if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}}; - return {ChainstateLoadStatus::FAILURE, _("Error loading block database")}; + if (chainman.m_interrupt) return {util::Error{}, ChainstateLoadError::INTERRUPTED}; + 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. @@ -85,7 +85,7 @@ static ChainstateLoadResult CompleteChainstateInitialization( // (otherwise we use the one already on disk). // This is called again in ImportBlocks after the reindex completes. if (!fReindex && !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) { @@ -119,14 +119,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 @@ -136,7 +137,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); } @@ -146,8 +147,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}; }; } @@ -156,11 +157,11 @@ 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) { if (!chainman.AssumedValidBlock().IsNull()) { LogPrintf("Assuming ancestors of block %s have valid signatures.\n", chainman.AssumedValidBlock().GetHex()); @@ -191,13 +192,13 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize if (has_snapshot && (options.reindex || options.reindex_chainstate)) { LogPrintf("[snapshot] deleting snapshot chainstate due to reindexing\n"); if (!chainman.DeleteSnapshotChainstate()) { - return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")}; + return {util::Error{Untranslated("Couldn't remove snapshot chainstate.")}, ChainstateLoadError::FAILURE_FATAL}; } } - auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options); - if (init_status != ChainstateLoadStatus::SUCCESS) { - return {init_status, init_error}; + auto result{CompleteChainstateInitialization(chainman, cache_sizes, options)}; + if (!result) { + return result; } // If a snapshot chainstate was fully validated by a background chainstate during @@ -215,7 +216,7 @@ 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.")}; + return {util::Error{Untranslated("Background chainstate cleanup failed unexpectedly.")}, ChainstateLoadError::FAILURE_FATAL}; } // Because ValidatedSnapshotCleanup() has torn down chainstates with @@ -231,20 +232,20 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // for the fully validated chainstate. chainman.ActiveChainstate().ClearBlockIndexCandidates(); - auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options); - if (init_status != ChainstateLoadStatus::SUCCESS) { - return {init_status, init_error}; + auto result{CompleteChainstateInitialization(chainman, cache_sizes, options)}; + if (!result) { + return result; } } else { - return {ChainstateLoadStatus::FAILURE, _( - "UTXO snapshot failed to validate. " + return util::Error{ + _("UTXO snapshot failed to validate. " "Restart to resume normal initial block download, or try loading a different snapshot.")}; } - return {ChainstateLoadStatus::SUCCESS, {}}; + return {}; } -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.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull(); @@ -256,9 +257,10 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C 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")}; + return {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}; } VerifyDBResult result = CVerifyDB(chainman.GetNotifications()).VerifyDB( @@ -270,18 +272,18 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C case VerifyDBResult::SKIPPED_MISSING_BLOCKS: break; case VerifyDBResult::INTERRUPTED: - return {ChainstateLoadStatus::INTERRUPTED, _("Block verification was interrupted")}; + return {util::Error{_("Block verification was interrupted")}, ChainstateLoadError::INTERRUPTED}; case VerifyDBResult::CORRUPTED_BLOCK_DB: - return {ChainstateLoadStatus::FAILURE, _("Corrupted block database detected")}; + return {util::Error{_("Corrupted block database detected")}, ChainstateLoadError::FAILURE}; case VerifyDBResult::SKIPPED_L3_CHECKS: if (options.require_full_verification) { - return {ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE, _("Insufficient dbcache for block verification")}; + return {util::Error{_("Insufficient dbcache for block verification")}, ChainstateLoadError::FAILURE_INSUFFICIENT_DBCACHE}; } break; } // no default case, so the compiler can warn about missing cases } } - return {ChainstateLoadStatus::SUCCESS, {}}; + return {}; } } // namespace node diff --git a/src/node/chainstate.h b/src/node/chainstate.h index a6e9a0331bc..40982c68c5e 100644 --- a/src/node/chainstate.h +++ b/src/node/chainstate.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_NODE_CHAINSTATE_H #define BITCOIN_NODE_CHAINSTATE_H +#include #include #include @@ -35,12 +36,11 @@ struct ChainstateLoadOptions { std::function coins_error_cb; }; -//! Chainstate load status. Simple applications can just check for the success -//! 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, +//! Chainstate load errors. Simple applications can just treat all errors as +//! failures. More complex applications may want to try reindexing in the +//! generic error case, and pass an interrupt callback and exit cleanly in the +//! interrupted case. +enum class ChainstateLoadError { FAILURE, //!< Generic failure which reindexing may fix FAILURE_FATAL, //!< Fatal error which should not prompt to reindex FAILURE_INCOMPATIBLE_DB, @@ -48,25 +48,9 @@ enum class ChainstateLoadStatus { 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 CacheSizes& cache_sizes, - const ChainstateLoadOptions& options); -ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options); +util::Result LoadChainstate(ChainstateManager& chainman, const 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/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index c52ef7cd67d..baafd8b9eef 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -377,7 +377,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con editStatus = WALLET_UNLOCK_FAILURE; return QString(); } - op_dest = walletModel->wallet().getNewDestination(address_type, strLabel); + op_dest.Set(walletModel->wallet().getNewDestination(address_type, strLabel)); if (!op_dest) { editStatus = KEY_GENERATION_FAILURE; return QString(); diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index c7fe62f4e91..613b9fe8231 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -263,12 +263,11 @@ 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 = Join(wallet.GetErrors(), Untranslated(" ")); + m_warning_message = wallet.GetWarnings(); 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); @@ -352,12 +351,11 @@ 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 = Join(wallet.GetErrors(), Untranslated(" ")); + m_warning_message = wallet.GetWarnings(); 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); @@ -405,12 +403,11 @@ 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 = Join(wallet.GetErrors(), Untranslated(" ")); + m_warning_message = wallet.GetWarnings(); 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/mempool_tests.cpp b/src/test/mempool_tests.cpp index 217e4a6d223..59f139d561e 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -260,7 +260,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) tx10.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; tx10.vout[0].nValue = 10 * COIN; - ancestors_calculated = pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits()); + ancestors_calculated.Set(pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits())); BOOST_REQUIRE(ancestors_calculated); BOOST_CHECK(*ancestors_calculated == setAncestors); diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index 6a23a7b8950..acc6569fe5b 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,83 @@ util::Result StrFn(bilingual_str s, bool success) return util::Error{strprintf(Untranslated("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) +{ + auto result = IntFailFn(i, success); + if (!success) return {util::MoveMessages(result), util::Error{Untranslated("str error")}, result.GetFailure()}; + return {util::MoveMessages(result), strprintf("%i", *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(strprintf(Untranslated("warn %i."), i)); + } + return {util::MoveMessages(result), ret}; +} + +util::Result ChainedFailFn(FnError arg, int ret) +{ + return {util::Error{Untranslated("chained fail.")}, util::MoveMessages(EnumFailFn(arg)), util::MoveMessages(WarnFn()), ret}; +} + +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.Set(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); @@ -67,20 +165,73 @@ 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); + BOOST_CHECK_EQUAL(result.GetFailure(), F{std::forward(args)...}); +} + +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_set) +{ + // Test using Set method to change a result value from success -> failure, + // and failure->success. + util::Result result; + ExpectSuccess(result, {}, 0); + result.Set({util::Error{Untranslated("error")}, ERR1}); + ExpectFail(result, Untranslated("error"), ERR1); + result.Set(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.Set({util::Error{Untranslated("error")}, 3}); + ExpectFail(result2, Untranslated("error"), 3); + result2.Set(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) @@ -93,4 +244,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(result.GetErrors().size(), 1); + BOOST_CHECK_EQUAL(result.GetErrors()[0], Untranslated("Error.")); + BOOST_CHECK_EQUAL(result.GetWarnings().size(), 1); + BOOST_CHECK_EQUAL(result.GetWarnings()[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{strprintf(Untranslated("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 38350b33cc5..6693314def5 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -282,11 +282,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_cache_sizes, options); - assert(status == node::ChainstateLoadStatus::SUCCESS); + auto result = LoadChainstate(chainman, m_cache_sizes, options); + assert(result); - std::tie(status, error) = VerifyLoadedChainstate(chainman, options); - assert(status == node::ChainstateLoadStatus::SUCCESS); + result = VerifyLoadedChainstate(chainman, options); + assert(result); BlockValidationState state; if (!chainman.ActiveChainstate().ActivateBestChain(state)) { diff --git a/src/util/result.cpp b/src/util/result.cpp new file mode 100644 index 00000000000..e1b7eb213e4 --- /dev/null +++ b/src/util/result.cpp @@ -0,0 +1,32 @@ +// Copyright (c) 2022 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 std::vector& errors, const std::vector& warnings) +{ + bilingual_str result; + for (const auto& messages : {errors, warnings}) { + for (const auto& message : messages) { + if (!result.empty()) result += Untranslated(" "); + result += message; + } + } + return result; +} + +void MoveMessages(std::vector& src, std::vector& dest) +{ + dest.insert(dest.end(), std::make_move_iterator(src.begin()), std::make_move_iterator(src.end())); + src.clear(); +} +} // namespace detail +} // namespace util diff --git a/src/util/result.h b/src/util/result.h index b99995c7e56..84bbda52f0c 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -8,58 +8,258 @@ #include #include -#include +#include +#include +#include +#include +#include +#include +#include namespace util { +//! The `Result` class provides a standard way for functions to return error and +//! warning strings in addition to optional result values. +//! +//! The easiest way to understand `Result` is to think of it as a substitute +//! for `std::optional`, that just contains error and warning strings in +//! addition to the optional return value. For example: +//! +//! 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, but in +//! general code using `Result` return values is similar to code using +//! `std::optional` return values. Existing functions returning +//! `std::optional` can be updated to return `Result` usually just +//! replacing `return std::nullopt;` with `return util::Error{error_string};`. +//! +//! An optional failure type `F` can be passed as a `Result` template +//! argument. The default type `F = void` is sufficient for most code that just +//! needs an error description without more complicated error handling. But code +//! that does need more fine-grained failure information can set custom error +//! values of type `F` and retrieve them with the `GetFailure()` method. +template +class Result; + +//! Wrapper types to pass error and warning strings to Result constructors. struct Error { bilingual_str message; }; +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 +//! Wrapper type to move error and warning strings from an existing Result object to a new Result constructor. +template +struct MoveMessages { + MoveMessages(Result&& result) : m_result(result) {} + Result& m_result; +}; + +//! Template deduction guide for MoveMessages class. +template +MoveMessages(Result&& result) -> MoveMessages; + +namespace detail { +//! Empty string list +const std::vector EMPTY_LIST{}; + +//! Helper function to join messages in space separated string. +bilingual_str JoinMessages(const std::vector& errors, const std::vector& warnings); + +//! Helper function to move messages from one vector to another. +void MoveMessages(std::vector& src, std::vector& dest); + +//! Substitute for std::monostate that doesn't depend on std::variant. +struct MonoState{}; + +//! Error information only allocated if there are errors or warnings. +template +struct ErrorInfo { + std::optional, MonoState, F>> failure{}; + std::vector errors{}; + std::vector warnings{}; +}; + +//! Result base class which is inherited by Result. +//! T is the type of the success return value, or void if there is none. +//! F is the type of the failure return value, or void if there is none. +template +class ResultBase; + +//! Result base specialization for empty (T=void) value type. Holds error +//! information and provides accessor methods. +template +class ResultBase { -private: - using T = std::conditional_t, std::monostate, M>; +protected: + std::unique_ptr> m_info; - std::variant m_variant; + ErrorInfo& Info() LIFETIMEBOUND + { + if (!m_info) m_info = std::make_unique>(); + return *m_info; + } - template - friend bilingual_str ErrorString(const Result& result); + //! Value setter methods which do nothing because this ResultBase class is + //! specialized for type T=void, so there is no result value for it to hold. + //! The other ResultBase specialization below for non-void T overrides these + //! methods to manage the result value it contains. + void ConstructValue() {} + template + void MoveValue(O&& other) {} + void DestroyValue() {} + + //! Helper function to construct a new Result success value or failure value + //! using the arguments provided. + template + static void ConstructResult(Result& result, Args&&... args) + { + if constexpr (Failure) { + result.Info().failure.emplace(std::forward(args)...); + } else { + result.ConstructValue(std::forward(args)...); + } + } + + //! ConstructResult() overload peeling off a util::Error constructor argument. + template + static void ConstructResult(Result& result, util::Error error, Args&&... args) + { + result.AddError(std::move(error.message)); + ConstructResult(result, std::forward(args)...); + } + + //! ConstructResult() overload peeling off a util::Warning constructor argument. + template + static void ConstructResult(Result& result, util::Warning warning, Args&&... args) + { + result.AddWarning(std::move(warning.message)); + ConstructResult(result, std::forward(args)...); + } + + //! ConstructResult() overload peeling off a util::MoveMessages constructor argument. + template + static void ConstructResult(Result& result, util::MoveMessages messages, Args&&... args) + { + result.MoveMessages(std::move(messages.m_result)); + ConstructResult(result, std::forward(args)...); + } + + //! Helper function to move success or failure values and any error and + //! warning messages from one result object to another. The two result + //! objects must have compatible success and failure types. + template + static void MoveResult(DstResult& dst, SrcResult&& src) + { + if constexpr (Constructed) { + if (dst) { + dst.DestroyValue(); + } else { + dst.m_info->failure.reset(); + } + } + dst.MoveMessages(src); + if (src) { + dst.MoveValue(std::move(src)); + } else { + dst.Info().failure = std::move(src.m_info->failure); + } + } + + //! Disallow potentially dangerous assignment operators which might erase + //! error and warning messages. The Result::Set() method can be used instead + //! of operator= to assign result values while keeping any existing errors + //! and warnings. + template + ResultBase& operator=(Result&&) = delete; + + template + friend class ResultBase; 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)} {} + //! Success check. + explicit operator bool() const { return !m_info || !m_info->failure; } + //! Error retrieval. + const auto& GetFailure() const LIFETIMEBOUND { assert(!*this); return *m_info->failure; } + const std::vector& GetErrors() const LIFETIMEBOUND { return m_info ? m_info->errors : EMPTY_LIST; } + const std::vector& GetWarnings() const LIFETIMEBOUND { return m_info ? m_info->warnings : EMPTY_LIST; } + + //! Add error and warning messages. + void AddError(bilingual_str error) + { + if (!error.empty()) this->Info().errors.emplace_back(std::move(error)); + } + void AddWarning(bilingual_str warning) + { + if (!warning.empty()) this->Info().warnings.emplace_back(std::move(warning)); + } + + //! Move error and warning messages from another result to this one. + template + void MoveMessages(Result&& other) + { + if (other.m_info) { + // Check that errors and warnings are empty before calling + // MoveMessages to avoid allocating memory in this->Info() in the + // typical case when there are no errors or warnings. + if (!other.m_info->errors.empty()) detail::MoveMessages(other.m_info->errors, this->Info().errors); + if (!other.m_info->warnings.empty()) detail::MoveMessages(other.m_info->warnings, this->Info().warnings); + } + } + + static constexpr bool is_result{true}; +}; + +//! Result base class for T value type. Holds value and provides accessor methods. +template +class ResultBase : public ResultBase +{ +protected: + //! Result success value. Uses anonymous union so success value is never + //! constructed in failure case. + union { T m_value; }; + + template + void ConstructValue(Args&&... args) { new (&m_value) T{std::forward(args)...}; } + template + void MoveValue(O&& other) { new (&m_value) T{std::move(other.m_value)}; } + void DestroyValue() { m_value.~T(); } + + //! Empty constructor that needs to be declared because the class contains a union. + ResultBase() {} + ~ResultBase() { if (*this) DestroyValue(); } + + template + friend class ResultBase; + +public: //! 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); - } + bool has_value() const { return bool{*this}; } + const T& value() const LIFETIMEBOUND { assert(has_value()); return m_value; } + T& value() LIFETIMEBOUND { assert(has_value()); return m_value; } template T value_or(U&& default_value) const& { @@ -70,18 +270,110 @@ public: { 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(); } }; +} // namespace detail -template -bilingual_str ErrorString(const Result& result) +template +class Result : public detail::ResultBase { - return result ? bilingual_str{} : std::get<0>(result.m_variant); +protected: + using Base = detail::ResultBase; + +public: + //! Construct a Result object setting a success or failure value and + //! optional warning and error messages. Initial util::Error, util::Warning, + //! and util::MoveMessages arguments are processed first to add warning and + //! error messages. Then, any remaining arguments are passed to the type `T` + //! constructor and used to construct a success value in the success case. + //! In the failure case, any remaining arguments are passed to the type `F` + //! constructor and used to construct a failure value. + template + Result(Args&&... args) + { + Base::template ConstructResult(*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) + { + Base::template MoveResult(*this, std::move(other)); + } + + //! Move success or failure values from another Result object to this + //! object. Also move any error and warning messages from the other Result + //! object to this one. If this Result object has an existing success or + //! failure value, it is cleared and replaced by the other value. If this + //! Result object has any error or warning messages, they are kept and + //! appended to. + Result& Set(Result&& other) LIFETIMEBOUND + { + Base::template MoveResult(*this, std::move(other)); + return *this; + } +}; + +//! Operator moving warning and error messages from a source result object +//! to a destination result object. Only moves message strings, does not +//! change success or failure 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) +S&& operator>>(S&& src LIFETIMEBOUND, D&& dst) +{ + dst.MoveMessages(src); + return std::move(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 GetErrors() and +//! GetWarning() methods directly. +template +bilingual_str ErrorString(const Result& result) { return detail::JoinMessages(result.GetErrors(), result.GetWarnings()); } } // namespace util #endif // BITCOIN_UTIL_RESULT_H diff --git a/src/validation.cpp b/src/validation.cpp index 903f9caf13a..43a651d3e29 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -970,7 +970,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } - ancestors = m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits); + ancestors.Set(m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits)); if (!ancestors) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 38cca32f80a..06b1f614c42 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -141,18 +141,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"; @@ -192,16 +191,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 @@ -306,7 +305,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; @@ -314,9 +313,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)) { @@ -326,12 +324,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) @@ -371,9 +368,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; @@ -489,8 +487,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) @@ -931,7 +929,7 @@ 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) { fs::path data_file = BDBDataFile(path); std::unique_ptr db; @@ -940,19 +938,17 @@ 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; + return {util::Error{Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", fs::PathToString(env->Directory() / data_filename)))}, + DatabaseError::FAILED_ALREADY_LOADED}; } db = std::make_unique(std::move(env), std::move(data_filename), options); } - if (options.verify && !db->Verify(error)) { - status = DatabaseStatus::FAILED_VERIFY; - return nullptr; + util::Result verified; + if (options.verify && !(verified = db->Verify())) { + return {util::Error{}, util::MoveMessages(verified), DatabaseError::FAILED_VERIFY}; } - status = DatabaseStatus::SUCCESS; - return db; + return {std::move(db)}; } } // namespace wallet diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 630630ebe01..7d5921e12a7 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); } @@ -215,7 +216,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 084fcadc24a..3c5abc75b57 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -193,8 +194,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, @@ -211,7 +211,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 7a36910dc12..a794fe57d38 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -21,37 +21,33 @@ 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) { // 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; + return {util::Error{_("No dump file provided. To use dump, -dumpfile= must be provided.")}}; } 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; + return {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))}}; } 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; + return {util::Error{strprintf(_("Unable to open %s for writing"), fs::PathToString(path))}}; } HashWriter hasher{}; std::unique_ptr batch = db.MakeBatch(); - bool ret = true; std::unique_ptr cursor = batch->GetNewCursor(); + util::Result ret; if (!cursor) { - error = _("Error: Couldn't create cursor into database"); - ret = false; + ret = {util::Error{_("Error: Couldn't create cursor into database")}}; } // Write out a magic string with version @@ -72,11 +68,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; + ret.Set({}); break; } else if (status == DatabaseCursor::Status::FAIL) { - error = _("Error reading next record from wallet database"); - ret = false; + ret.Set(util::Error{_("Error reading next record from wallet database")}); break; } std::string key_str = HexStr(ss_key); @@ -113,20 +108,18 @@ 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) { // 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; + return {util::Error{_("No dump file provided. To use createfromdump, -dumpfile= must be provided.")}}; } 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; + return {util::Error{strprintf(_("Dump file %s does not exist."), fs::PathToString(dump_path))}}; } std::ifstream dump_file{dump_path}; @@ -140,21 +133,18 @@ 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; + return {util::Error{strprintf(_("Error: Dumpfile identifier record is incorrect. Got \"%s\", expected \"%s\"."), magic_key, DUMP_MAGIC)}}; } // 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; + return {util::Error{strprintf(_("Error: Unable to parse version %u as a uint32_t"), version_value)}}; } 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; + return {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)}}; } std::string magic_hasher_line = strprintf("%s,%s\n", magic_key, version_value); hasher << Span{magic_hasher_line}; @@ -165,15 +155,13 @@ 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; + return {util::Error{strprintf(_("Error: Dumpfile format record is incorrect. Got \"%s\", expected \"format\"."), format_key)}}; } // 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; + return {util::Error{_("No wallet file format provided. To use createfromdump, -format= must be provided.")}}; } DatabaseFormat data_format; if (file_format == "bdb") { @@ -181,32 +169,29 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: } else if (file_format == "sqlite") { data_format = DatabaseFormat::SQLITE; } else { - error = strprintf(_("Unknown wallet file format \"%s\" provided. Please provide one of \"bdb\" or \"sqlite\"."), file_format); - return false; + return {util::Error{strprintf(_("Unknown wallet file format \"%s\" provided. Please provide one of \"bdb\" or \"sqlite\"."), file_format)}}; } + util::Result ret; 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)); + ret.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) >> ret; + if (!database) return {util::Error{}, util::MoveMessages(ret)}; // 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; + return {util::Error{strprintf(_("Error creating %s"), name)}, util::MoveMessages(ret)}; } // Get the database handle @@ -224,8 +209,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; + ret = {util::Error{Untranslated("Error: Checksum is not the correct size")}}; break; } std::copy(parsed_checksum.begin(), parsed_checksum.end(), checksum.begin()); @@ -240,21 +224,18 @@ 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; + ret = {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; + ret = {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; + ret = {util::Error{strprintf(_("Error: Unable to write record to new wallet"))}}; break; } } @@ -262,11 +243,9 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: if (ret) { uint256 comp_checksum = hasher.GetHash(); if (checksum.IsNull()) { - error = _("Error: Missing checksum"); - ret = false; + ret = {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; + ret = {util::Error{strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum))}}; } } 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 0c1cae7253c..a4403d18e06 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -283,7 +283,7 @@ public: 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)}; + if (!res) return {util::Error{}, util::MoveMessages(res)}; const auto& txr = *res; fee = txr.fee; change_pos = txr.change_pos ? int(*txr.change_pos) : -1; @@ -597,46 +597,31 @@ 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 { 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; - } else { - return util::Error{error}; - } + auto wallet = CreateWallet(m_context, name, true /* load_on_start */, options); + if (!wallet) return {util::Error{}, util::MoveMessages(wallet)}; + return {util::MoveMessages(wallet), MakeWallet(m_context, wallet.value())}; } - util::Result> loadWallet(const std::string& name, std::vector& warnings) override + util::ResultPtr> loadWallet(const std::string& name) override { 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; - } else { - return util::Error{error}; - } + auto wallet = LoadWallet(m_context, name, true /* load_on_start */, options); + if (!wallet) return {util::Error{}, util::MoveMessages(wallet)}; + return {util::MoveMessages(wallet), MakeWallet(m_context, wallet.value())}; } - 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; - } else { - return util::Error{error}; - } + auto wallet = RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true); + if (!wallet) return {util::Error{}, util::MoveMessages(wallet)}; + return {util::MoveMessages(wallet), MakeWallet(m_context, wallet.value())}; } util::Result migrateWallet(const std::string& name, const SecureString& passphrase) override { diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index 8b78a670e49..427f54c23d3 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -56,12 +56,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 @@ -84,16 +82,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; } } @@ -113,26 +110,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…").translated); - 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.GetWarnings().empty()) chain.initWarning(Join(result.GetWarnings(), Untranslated("\n"))); if (!pwallet) { - chain.initError(error); + chain.initError(Join(result.GetErrors(), Untranslated(" "))); return false; } - NotifyWalletLoaded(context, pwallet); - AddWallet(context, pwallet); + NotifyWalletLoaded(context, pwallet.value()); + AddWallet(context, pwallet.value()); } return true; } catch (const std::runtime_error& e) { @@ -174,8 +169,7 @@ 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); + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt); UnloadWallet(std::move(wallet)); } } diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index ae2dfe57951..37c94564842 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1931,17 +1931,13 @@ 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); + PushWarnings(wallet.GetWarnings(), obj); return obj; diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 1252843e9d0..39e221c064c 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -152,30 +152,30 @@ void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, entry.pushKV("parent_descs", 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 2fdba043522..111978c6b81 100644 --- a/src/wallet/rpc/util.h +++ b/src/wallet/rpc/util.h @@ -7,6 +7,7 @@ #include #include