From a48e23f566ccaf9b81fe0684885972d9ee34afd3 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Thu, 22 May 2025 19:40:20 -0700 Subject: [PATCH] refactor: wallet: move error handling to PopulateWalletFromDB() --- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/test/walletload_tests.cpp | 6 ++- src/wallet/wallet.cpp | 81 ++++++++++++---------------- src/wallet/wallet.h | 2 +- src/wallet/wallettool.cpp | 38 +++++-------- 5 files changed, 53 insertions(+), 76 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 16fc8576f84..50d2f78b8d7 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -306,7 +306,7 @@ void TestLoadWallet(const std::string& name, DatabaseFormat format, std::functio std::vector warnings; auto database{MakeWalletDatabase(name, options, status, error)}; auto wallet{std::make_shared(chain.get(), "", std::move(database))}; - BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(), DBErrors::LOAD_OK); + BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(error, warnings), DBErrors::LOAD_OK); WITH_LOCK(wallet->cs_wallet, f(wallet)); } diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index ed33613fa2b..dadbb07eceb 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -39,6 +39,8 @@ public: BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup) { + bilingual_str _error; + std::vector _warnings; std::unique_ptr database = CreateMockableWalletDatabase(); { // Write unknown active descriptor @@ -52,7 +54,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup) { // Now try to load the wallet and verify the error. const std::shared_ptr wallet(new CWallet(m_node.chain.get(), "", std::move(database))); - BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(), DBErrors::UNKNOWN_DESCRIPTOR); + BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(_error, _warnings), DBErrors::UNKNOWN_DESCRIPTOR); } // Test 2 @@ -78,7 +80,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup) { // Now try to load the wallet and verify the error. const std::shared_ptr wallet(new CWallet(m_node.chain.get(), "", std::move(database))); - BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(), DBErrors::CORRUPT); + BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(_error, _warnings), DBErrors::CORRUPT); BOOST_CHECK(found); // The error must be logged } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4a464cdf7d6..027bfeef90a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2290,7 +2290,7 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve } } -DBErrors CWallet::PopulateWalletFromDB() +DBErrors CWallet::PopulateWalletFromDB(bilingual_str& error, std::vector& warnings) { LOCK(cs_wallet); @@ -2312,6 +2312,36 @@ DBErrors CWallet::PopulateWalletFromDB() assert(m_internal_spk_managers.empty()); } + if (nLoadWalletRet != DBErrors::LOAD_OK) { + const auto wallet_file = m_database->Filename(); + if (nLoadWalletRet == DBErrors::CORRUPT) { + error = strprintf(_("Error loading %s: Wallet corrupted"), wallet_file); + } else if (nLoadWalletRet == DBErrors::NONCRITICAL_ERROR) { + warnings.push_back(strprintf(_("Error reading %s! All keys read correctly, but transaction data" + " or address metadata may be missing or incorrect."), + wallet_file)); + } else if (nLoadWalletRet == DBErrors::TOO_NEW) { + error = strprintf(_("Error loading %s: Wallet requires newer version of %s"), wallet_file, CLIENT_NAME); + } else if (nLoadWalletRet == DBErrors::EXTERNAL_SIGNER_SUPPORT_REQUIRED) { + error = strprintf(_("Error loading %s: External signer wallet being loaded without external signer support compiled"), wallet_file); + } else if (nLoadWalletRet == DBErrors::NEED_REWRITE) { + error = strprintf(_("Wallet needed to be rewritten: restart %s to complete"), CLIENT_NAME); + } else if (nLoadWalletRet == DBErrors::NEED_RESCAN) { + warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect." + " Rescanning wallet."), wallet_file)); + } else if (nLoadWalletRet == DBErrors::UNKNOWN_DESCRIPTOR) { + error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n" + "The wallet might have been created on a newer version.\n" + "Please try running the latest software version.\n"), wallet_file); + } else if (nLoadWalletRet == DBErrors::UNEXPECTED_LEGACY_ENTRY) { + error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n" + "The wallet might have been tampered with or created with malicious intent.\n"), wallet_file); + } else if (nLoadWalletRet == DBErrors::LEGACY_WALLET) { + error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), wallet_file); + } else { + error = strprintf(_("Error loading %s"), wallet_file); + } + } return nLoadWalletRet; } @@ -2854,51 +2884,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->m_notify_tx_changed_script = args.GetArg("-walletnotify", ""); // Load wallet - bool rescan_required = false; - DBErrors nLoadWalletRet = walletInstance->PopulateWalletFromDB(); - if (nLoadWalletRet != DBErrors::LOAD_OK) { - if (nLoadWalletRet == DBErrors::CORRUPT) { - error = strprintf(_("Error loading %s: Wallet corrupted"), walletFile); - return nullptr; - } - else if (nLoadWalletRet == DBErrors::NONCRITICAL_ERROR) - { - warnings.push_back(strprintf(_("Error reading %s! All keys read correctly, but transaction data" - " or address metadata may be missing or incorrect."), - walletFile)); - } - else if (nLoadWalletRet == DBErrors::TOO_NEW) { - error = strprintf(_("Error loading %s: Wallet requires newer version of %s"), walletFile, CLIENT_NAME); - return nullptr; - } - else if (nLoadWalletRet == DBErrors::EXTERNAL_SIGNER_SUPPORT_REQUIRED) { - error = strprintf(_("Error loading %s: External signer wallet being loaded without external signer support compiled"), walletFile); - return nullptr; - } - else if (nLoadWalletRet == DBErrors::NEED_REWRITE) - { - error = strprintf(_("Wallet needed to be rewritten: restart %s to complete"), CLIENT_NAME); - return nullptr; - } else if (nLoadWalletRet == DBErrors::NEED_RESCAN) { - warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect." - " Rescanning wallet."), walletFile)); - rescan_required = true; - } else if (nLoadWalletRet == DBErrors::UNKNOWN_DESCRIPTOR) { - error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n" - "The wallet might have been created on a newer version.\n" - "Please try running the latest software version.\n"), walletFile); - return nullptr; - } else if (nLoadWalletRet == DBErrors::UNEXPECTED_LEGACY_ENTRY) { - error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n" - "The wallet might have been tampered with or created with malicious intent.\n"), walletFile); - return nullptr; - } else if (nLoadWalletRet == DBErrors::LEGACY_WALLET) { - error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), walletFile); - return nullptr; - } else { - error = strprintf(_("Error loading %s"), walletFile); - return nullptr; - } + auto nLoadWalletRet = walletInstance->PopulateWalletFromDB(error, warnings); + bool rescan_required = nLoadWalletRet == DBErrors::NEED_RESCAN; + if (nLoadWalletRet != DBErrors::LOAD_OK && nLoadWalletRet != DBErrors::NONCRITICAL_ERROR && !rescan_required) { + return nullptr; } // This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e135b9d60b6..1351ee18810 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -797,7 +797,7 @@ public: bool IsFromMe(const CTransaction& tx) const; CAmount GetDebit(const CTransaction& tx) const; - DBErrors PopulateWalletFromDB(); + DBErrors PopulateWalletFromDB(bilingual_str& error, std::vector& warnings); /** Erases the provided transactions from the wallet. */ util::Result RemoveTxs(std::vector& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index b7012bd087f..e0de4fc958d 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -2,8 +2,6 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include // IWYU pragma: keep - #include #include @@ -44,6 +42,7 @@ static std::shared_ptr MakeWallet(const std::string& name, const fs::pa { DatabaseStatus status; bilingual_str error; + std::vector warnings; std::unique_ptr database = MakeDatabase(path, options, status, error); if (!database) { tfm::format(std::cerr, "%s\n", error.original); @@ -54,35 +53,22 @@ static std::shared_ptr MakeWallet(const std::string& name, const fs::pa std::shared_ptr wallet_instance{new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet}; DBErrors load_wallet_ret; try { - load_wallet_ret = wallet_instance->PopulateWalletFromDB(); + load_wallet_ret = wallet_instance->PopulateWalletFromDB(error, warnings); } catch (const std::runtime_error&) { tfm::format(std::cerr, "Error loading %s. Is wallet being used by another process?\n", name); return nullptr; } - if (load_wallet_ret != DBErrors::LOAD_OK) { - if (load_wallet_ret == DBErrors::CORRUPT) { - tfm::format(std::cerr, "Error loading %s: Wallet corrupted", name); - return nullptr; - } else if (load_wallet_ret == DBErrors::NONCRITICAL_ERROR) { - tfm::format(std::cerr, "Error reading %s! All keys read correctly, but transaction data" - " or address book entries might be missing or incorrect.", - name); - } else if (load_wallet_ret == DBErrors::TOO_NEW) { - tfm::format(std::cerr, "Error loading %s: Wallet requires newer version of %s", - name, CLIENT_NAME); - return nullptr; - } else if (load_wallet_ret == DBErrors::NEED_REWRITE) { - tfm::format(std::cerr, "Wallet needed to be rewritten: restart %s to complete", CLIENT_NAME); - return nullptr; - } else if (load_wallet_ret == DBErrors::NEED_RESCAN) { - tfm::format(std::cerr, "Error reading %s! Some transaction data might be missing or" - " incorrect. Wallet requires a rescan.", - name); - } else { - tfm::format(std::cerr, "Error loading %s", name); - return nullptr; - } + if (!error.empty()) { + tfm::format(std::cerr, "%s", error.original); + } + + for (const auto &warning : warnings) { + tfm::format(std::cerr, "%s", warning.original); + } + + if (load_wallet_ret != DBErrors::LOAD_OK && load_wallet_ret != DBErrors::NONCRITICAL_ERROR && load_wallet_ret != DBErrors::NEED_RESCAN) { + return nullptr; } if (options.require_create) WalletCreate(wallet_instance.get(), options.create_flags);