refactor: wallet: move error handling to PopulateWalletFromDB()
What changed, and why it matters
This commit is a straightforward internal code cleanup in Bitcoin Core's wallet module. It moves the code that translates database load errors into user-facing error and warning messages from two separate callers into the single function that actually loads the wallet data. There is no change to what errors are detected, what messages are shown, or how the wallet behaves when problems occur.
No security action required. Treat as a normal maintainability refactor.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch refactors CWallet::PopulateWalletFromDB() to accept bilingual_str& error and std::vector
Changed components
src/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/wallettool.cppsrc/wallet/test/wallet_tests.cppsrc/wallet/test/walletload_tests.cppInspect captured patch +53 / −76
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 16fc8576..50d2f78b 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<bilingual_str> warnings;
auto database{MakeWalletDatabase(name, options, status, error)};
auto wallet{std::make_shared<CWallet>(chain.get(), "", std::move(database))};
- BOOST_CHECK_EQUAL(wallet->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 ed33613f..dadbb07e 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<bilingual_str> _warnings;
std::unique_ptr<WalletDatabase> 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<CWallet> 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<CWallet> 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 4a464cdf..027bfeef 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<bilingual_str>& 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> 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 e135b9d6..1351ee18 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<bilingual_str>& warnings);
/** Erases the provided transactions from the wallet. */
util::Result<void> RemoveTxs(std::vector<Txid>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp
index b7012bd0..e0de4fc9 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 <bitcoin-build-config.h> // IWYU pragma: keep
-
#include <wallet/wallettool.h>
#include <common/args.h>
@@ -44,6 +42,7 @@ static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::pa
{
DatabaseStatus status;
bilingual_str error;
+ std::vector<bilingual_str> warnings;
std::unique_ptr<WalletDatabase> database = MakeDatabase(path, options, status, error);
if (!database) {
tfm::format(std::cerr, "%s\n", error.original);
@@ -54,35 +53,22 @@ static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::pa
std::shared_ptr<CWallet> 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);
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.