From f0a046094e4c4b5f3af0e453492077f4911e0132 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Thu, 22 May 2025 17:38:18 -0700 Subject: [PATCH 01/16] scripted-diff: refactor: CWallet::LoadWallet->PopulateWalletFromDB There are too many functions in CWallet with names like "Load" and "Create", disambiguate what CWallet::LoadWallet does by renaming it to PopulateWalletFromDB. -BEGIN VERIFY SCRIPT- sed -i 's|\bLoadWallet()|PopulateWalletFromDB()|g' $(git grep -l 'LoadWallet()' -- ':(exclude)src/wallet/walletdb.cpp') -END VERIFY SCRIPT- --- src/qt/test/addressbooktests.cpp | 2 +- src/qt/test/wallettests.cpp | 2 +- src/wallet/dump.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/test/walletload_tests.cpp | 4 ++-- src/wallet/wallet.cpp | 4 ++-- src/wallet/wallet.h | 2 +- src/wallet/wallettool.cpp | 2 +- 8 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index d2ac1306ae4..f0f2c5940b4 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -77,7 +77,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) test.m_node.wallet_loader = wallet_loader.get(); node.setContext(&test.m_node); const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockableWalletDatabase()); - wallet->LoadWallet(); + wallet->PopulateWalletFromDB(); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); { LOCK(wallet->cs_wallet); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 5e65dcca318..c5e6e116769 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -192,7 +192,7 @@ void SyncUpWallet(const std::shared_ptr& wallet, interfaces::Node& node std::shared_ptr SetupDescriptorsWallet(interfaces::Node& node, TestChain100Setup& test, bool watch_only = false) { std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockableWalletDatabase()); - wallet->LoadWallet(); + wallet->PopulateWalletFromDB(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); if (watch_only) { diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index 6b193ad72f3..ee800a043c9 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -199,7 +199,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: std::shared_ptr wallet(new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet); { LOCK(wallet->cs_wallet); - DBErrors load_wallet_ret = wallet->LoadWallet(); + DBErrors load_wallet_ret = wallet->PopulateWalletFromDB(); if (load_wallet_ret != DBErrors::LOAD_OK) { error = strprintf(_("Error creating %s"), name); return false; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index bcff25931a3..16fc8576f84 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->LoadWallet(), DBErrors::LOAD_OK); + BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(), 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 0c69849d0b6..ed33613fa2b 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -52,7 +52,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->LoadWallet(), DBErrors::UNKNOWN_DESCRIPTOR); + BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(), DBErrors::UNKNOWN_DESCRIPTOR); } // Test 2 @@ -78,7 +78,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->LoadWallet(), DBErrors::CORRUPT); + BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(), DBErrors::CORRUPT); BOOST_CHECK(found); // The error must be logged } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d08d6782c1b..4a464cdf7d6 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::LoadWallet() +DBErrors CWallet::PopulateWalletFromDB() { LOCK(cs_wallet); @@ -2855,7 +2855,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri // Load wallet bool rescan_required = false; - DBErrors nLoadWalletRet = walletInstance->LoadWallet(); + DBErrors nLoadWalletRet = walletInstance->PopulateWalletFromDB(); if (nLoadWalletRet != DBErrors::LOAD_OK) { if (nLoadWalletRet == DBErrors::CORRUPT) { error = strprintf(_("Error loading %s: Wallet corrupted"), walletFile); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index b341ac6da2d..e135b9d60b6 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 LoadWallet(); + DBErrors PopulateWalletFromDB(); /** 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 af32986ce65..b7012bd087f 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -54,7 +54,7 @@ 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->LoadWallet(); + load_wallet_ret = wallet_instance->PopulateWalletFromDB(); } catch (const std::runtime_error&) { tfm::format(std::cerr, "Error loading %s. Is wallet being used by another process?\n", name); return nullptr; From 0972785fd723b9b3c84844bf999d6e08e163ef9d Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Thu, 15 Jan 2026 17:50:05 -0800 Subject: [PATCH 02/16] wallet: Delete unnecessary PopulateWalletFromDB() calls --- src/qt/test/addressbooktests.cpp | 1 - src/qt/test/wallettests.cpp | 1 - src/wallet/dump.cpp | 7 ------- 3 files changed, 9 deletions(-) diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index f0f2c5940b4..58b8a6d2ea2 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -77,7 +77,6 @@ void TestAddAddressesToSendBook(interfaces::Node& node) test.m_node.wallet_loader = wallet_loader.get(); node.setContext(&test.m_node); const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockableWalletDatabase()); - wallet->PopulateWalletFromDB(); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); { LOCK(wallet->cs_wallet); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index c5e6e116769..227117618ee 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -192,7 +192,6 @@ void SyncUpWallet(const std::shared_ptr& wallet, interfaces::Node& node std::shared_ptr SetupDescriptorsWallet(interfaces::Node& node, TestChain100Setup& test, bool watch_only = false) { std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockableWalletDatabase()); - wallet->PopulateWalletFromDB(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); if (watch_only) { diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index ee800a043c9..ba71c559ea1 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -198,13 +198,6 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: bool ret = true; std::shared_ptr wallet(new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet); { - LOCK(wallet->cs_wallet); - DBErrors load_wallet_ret = wallet->PopulateWalletFromDB(); - if (load_wallet_ret != DBErrors::LOAD_OK) { - error = strprintf(_("Error creating %s"), name); - return false; - } - // Get the database handle WalletDatabase& db = wallet->GetDatabase(); std::unique_ptr batch = db.MakeBatch(); From a48e23f566ccaf9b81fe0684885972d9ee34afd3 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Thu, 22 May 2025 19:40:20 -0700 Subject: [PATCH 03/16] 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); From 411caf72815bdf2e176e790a4c63f745517c4bb4 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 22 Aug 2025 18:58:54 -0700 Subject: [PATCH 04/16] wallet: refactor: PopulateWalletFromDB use switch statement. Co-authored-by: @w0xlt --- src/wallet/wallet.cpp | 71 +++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 027bfeef90a..8e1ab28f1ee 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2312,36 +2312,47 @@ DBErrors CWallet::PopulateWalletFromDB(bilingual_str& error, std::vectorFilename(); - 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); - } - } + const auto wallet_file = m_database->Filename(); + switch (nLoadWalletRet) { + case DBErrors::LOAD_OK: + break; + case 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)); + break; + case DBErrors::NEED_RESCAN: + warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect." + " Rescanning wallet."), wallet_file)); + break; + case DBErrors::CORRUPT: + error = strprintf(_("Error loading %s: Wallet corrupted"), wallet_file); + break; + case DBErrors::TOO_NEW: + error = strprintf(_("Error loading %s: Wallet requires newer version of %s"), wallet_file, CLIENT_NAME); + break; + case DBErrors::EXTERNAL_SIGNER_SUPPORT_REQUIRED: + error = strprintf(_("Error loading %s: External signer wallet being loaded without external signer support compiled"), wallet_file); + break; + case DBErrors::NEED_REWRITE: + error = strprintf(_("Wallet needed to be rewritten: restart %s to complete"), CLIENT_NAME); + break; + case 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); + break; + case 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); + break; + case 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); + break; + case DBErrors::LOAD_FAIL: + error = strprintf(_("Error loading %s"), wallet_file); + break; + } // no default case, so the compiler can warn about missing cases return nLoadWalletRet; } From a02c4a82d88a3e9a24ec2aa0b828b8cc533dde58 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Tue, 27 May 2025 15:20:55 -0700 Subject: [PATCH 05/16] refactor: Move -walletbroadcast setting init Modifying `fBroadcastTransactions` does not require any locks, initialization of this wallet parameter can be relocated with all of the other argument parsing in this function. --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8e1ab28f1ee..d9cf77314b8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3069,6 +3069,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->m_confirm_target = args.GetIntArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET); walletInstance->m_spend_zero_conf_change = args.GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE); walletInstance->m_signal_rbf = args.GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF); + walletInstance->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); walletInstance->WalletLogPrintf("Wallet completed loading in %15dms\n", Ticks(SteadyClock::now() - start)); @@ -3090,7 +3091,6 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri { LOCK(walletInstance->cs_wallet); - walletInstance->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n", walletInstance->GetKeyPoolSize()); walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size()); walletInstance->WalletLogPrintf("m_address_book.size() = %u\n", walletInstance->m_address_book.size()); From b15a94a618c53041e97ccfface3045a0642777e1 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Thu, 22 May 2025 18:49:00 -0700 Subject: [PATCH 06/16] refactor: Split out wallet argument loading This section is necessarily repetitive, makes CWallet::Create() easier to read, and splits out functionality that will be useful when wallet creation and loading are separated. Review with `-color-moved=dimmed-zebra` --- src/wallet/wallet.cpp | 272 ++++++++++++++++++++++-------------------- src/wallet/wallet.h | 2 + 2 files changed, 144 insertions(+), 130 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d9cf77314b8..d95701fde8f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2881,6 +2881,146 @@ std::unique_ptr MakeWalletDatabase(const std::string& name, cons return MakeDatabase(*wallet_path, options, status, error_string); } +bool CWallet::LoadWalletArgs(std::shared_ptr wallet, const WalletContext& context, bilingual_str& error, std::vector& warnings) +{ + interfaces::Chain* chain = context.chain; + const ArgsManager& args = *Assert(context.args); + + if (!args.GetArg("-addresstype", "").empty()) { + std::optional parsed = ParseOutputType(args.GetArg("-addresstype", "")); + if (!parsed) { + error = strprintf(_("Unknown address type '%s'"), args.GetArg("-addresstype", "")); + return false; + } + wallet->m_default_address_type = parsed.value(); + } + + if (!args.GetArg("-changetype", "").empty()) { + std::optional parsed = ParseOutputType(args.GetArg("-changetype", "")); + if (!parsed) { + error = strprintf(_("Unknown change type '%s'"), args.GetArg("-changetype", "")); + return false; + } + wallet->m_default_change_type = parsed.value(); + } + + if (const auto arg{args.GetArg("-mintxfee")}) { + std::optional min_tx_fee = ParseMoney(*arg); + if (!min_tx_fee) { + error = AmountErrMsg("mintxfee", *arg); + return false; + } else if (min_tx_fee.value() > HIGH_TX_FEE_PER_KB) { + warnings.push_back(AmountHighWarn("-mintxfee") + Untranslated(" ") + + _("This is the minimum transaction fee you pay on every transaction.")); + } + + wallet->m_min_fee = CFeeRate{min_tx_fee.value()}; + } + + if (const auto arg{args.GetArg("-maxapsfee")}) { + const std::string& max_aps_fee{*arg}; + if (max_aps_fee == "-1") { + wallet->m_max_aps_fee = -1; + } else if (std::optional max_fee = ParseMoney(max_aps_fee)) { + if (max_fee.value() > HIGH_APS_FEE) { + warnings.push_back(AmountHighWarn("-maxapsfee") + Untranslated(" ") + + _("This is the maximum transaction fee you pay (in addition to the normal fee) to prioritize partial spend avoidance over regular coin selection.")); + } + wallet->m_max_aps_fee = max_fee.value(); + } else { + error = AmountErrMsg("maxapsfee", max_aps_fee); + return false; + } + } + + if (const auto arg{args.GetArg("-fallbackfee")}) { + std::optional fallback_fee = ParseMoney(*arg); + if (!fallback_fee) { + error = strprintf(_("Invalid amount for %s=: '%s'"), "-fallbackfee", *arg); + return false; + } else if (fallback_fee.value() > HIGH_TX_FEE_PER_KB) { + warnings.push_back(AmountHighWarn("-fallbackfee") + Untranslated(" ") + + _("This is the transaction fee you may pay when fee estimates are not available.")); + } + wallet->m_fallback_fee = CFeeRate{fallback_fee.value()}; + } + + // Disable fallback fee in case value was set to 0, enable if non-null value + wallet->m_allow_fallback_fee = wallet->m_fallback_fee.GetFeePerK() != 0; + + if (const auto arg{args.GetArg("-discardfee")}) { + std::optional discard_fee = ParseMoney(*arg); + if (!discard_fee) { + error = strprintf(_("Invalid amount for %s=: '%s'"), "-discardfee", *arg); + return false; + } else if (discard_fee.value() > HIGH_TX_FEE_PER_KB) { + warnings.push_back(AmountHighWarn("-discardfee") + Untranslated(" ") + + _("This is the transaction fee you may discard if change is smaller than dust at this level")); + } + wallet->m_discard_rate = CFeeRate{discard_fee.value()}; + } + + if (const auto arg{args.GetArg("-paytxfee")}) { + warnings.push_back(_("-paytxfee is deprecated and will be fully removed in v31.0.")); + + std::optional pay_tx_fee = ParseMoney(*arg); + if (!pay_tx_fee) { + error = AmountErrMsg("paytxfee", *arg); + return false; + } else if (pay_tx_fee.value() > HIGH_TX_FEE_PER_KB) { + warnings.push_back(AmountHighWarn("-paytxfee") + Untranslated(" ") + + _("This is the transaction fee you will pay if you send a transaction.")); + } + + wallet->m_pay_tx_fee = CFeeRate{pay_tx_fee.value(), 1000}; + + if (chain && wallet->m_pay_tx_fee < chain->relayMinFee()) { + error = strprintf(_("Invalid amount for %s=: '%s' (must be at least %s)"), + "-paytxfee", *arg, chain->relayMinFee().ToString()); + return false; + } + } + + if (const auto arg{args.GetArg("-maxtxfee")}) { + std::optional max_fee = ParseMoney(*arg); + if (!max_fee) { + error = AmountErrMsg("maxtxfee", *arg); + return false; + } else if (max_fee.value() > HIGH_MAX_TX_FEE) { + warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee")); + } + + if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) { + error = strprintf(_("Invalid amount for %s=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"), + "-maxtxfee", *arg, chain->relayMinFee().ToString()); + return false; + } + + wallet->m_default_max_tx_fee = max_fee.value(); + } + + if (const auto arg{args.GetArg("-consolidatefeerate")}) { + if (std::optional consolidate_feerate = ParseMoney(*arg)) { + wallet->m_consolidate_feerate = CFeeRate(*consolidate_feerate); + } else { + error = AmountErrMsg("consolidatefeerate", *arg); + return false; + } + } + + if (chain && chain->relayMinFee().GetFeePerK() > HIGH_TX_FEE_PER_KB) { + warnings.push_back(AmountHighWarn("-minrelaytxfee") + Untranslated(" ") + + _("The wallet will avoid paying less than the minimum relay fee.")); + } + + wallet->m_confirm_target = args.GetIntArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET); + wallet->m_spend_zero_conf_change = args.GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE); + wallet->m_signal_rbf = args.GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF); + wallet->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); + + return true; +} + std::shared_ptr CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) { interfaces::Chain* chain = context.chain; @@ -2939,138 +3079,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } } - if (!args.GetArg("-addresstype", "").empty()) { - std::optional parsed = ParseOutputType(args.GetArg("-addresstype", "")); - if (!parsed) { - error = strprintf(_("Unknown address type '%s'"), args.GetArg("-addresstype", "")); - return nullptr; - } - walletInstance->m_default_address_type = parsed.value(); + if (!LoadWalletArgs(walletInstance, context, error, warnings)) { + return nullptr; } - if (!args.GetArg("-changetype", "").empty()) { - std::optional parsed = ParseOutputType(args.GetArg("-changetype", "")); - if (!parsed) { - error = strprintf(_("Unknown change type '%s'"), args.GetArg("-changetype", "")); - return nullptr; - } - walletInstance->m_default_change_type = parsed.value(); - } - - if (const auto arg{args.GetArg("-mintxfee")}) { - std::optional min_tx_fee = ParseMoney(*arg); - if (!min_tx_fee) { - error = AmountErrMsg("mintxfee", *arg); - return nullptr; - } else if (min_tx_fee.value() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-mintxfee") + Untranslated(" ") + - _("This is the minimum transaction fee you pay on every transaction.")); - } - - walletInstance->m_min_fee = CFeeRate{min_tx_fee.value()}; - } - - if (const auto arg{args.GetArg("-maxapsfee")}) { - const std::string& max_aps_fee{*arg}; - if (max_aps_fee == "-1") { - walletInstance->m_max_aps_fee = -1; - } else if (std::optional max_fee = ParseMoney(max_aps_fee)) { - if (max_fee.value() > HIGH_APS_FEE) { - warnings.push_back(AmountHighWarn("-maxapsfee") + Untranslated(" ") + - _("This is the maximum transaction fee you pay (in addition to the normal fee) to prioritize partial spend avoidance over regular coin selection.")); - } - walletInstance->m_max_aps_fee = max_fee.value(); - } else { - error = AmountErrMsg("maxapsfee", max_aps_fee); - return nullptr; - } - } - - if (const auto arg{args.GetArg("-fallbackfee")}) { - std::optional fallback_fee = ParseMoney(*arg); - if (!fallback_fee) { - error = strprintf(_("Invalid amount for %s=: '%s'"), "-fallbackfee", *arg); - return nullptr; - } else if (fallback_fee.value() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-fallbackfee") + Untranslated(" ") + - _("This is the transaction fee you may pay when fee estimates are not available.")); - } - walletInstance->m_fallback_fee = CFeeRate{fallback_fee.value()}; - } - - // Disable fallback fee in case value was set to 0, enable if non-null value - walletInstance->m_allow_fallback_fee = walletInstance->m_fallback_fee.GetFeePerK() != 0; - - if (const auto arg{args.GetArg("-discardfee")}) { - std::optional discard_fee = ParseMoney(*arg); - if (!discard_fee) { - error = strprintf(_("Invalid amount for %s=: '%s'"), "-discardfee", *arg); - return nullptr; - } else if (discard_fee.value() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-discardfee") + Untranslated(" ") + - _("This is the transaction fee you may discard if change is smaller than dust at this level")); - } - walletInstance->m_discard_rate = CFeeRate{discard_fee.value()}; - } - - if (const auto arg{args.GetArg("-paytxfee")}) { - warnings.push_back(_("-paytxfee is deprecated and will be fully removed in v31.0.")); - - std::optional pay_tx_fee = ParseMoney(*arg); - if (!pay_tx_fee) { - error = AmountErrMsg("paytxfee", *arg); - return nullptr; - } else if (pay_tx_fee.value() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-paytxfee") + Untranslated(" ") + - _("This is the transaction fee you will pay if you send a transaction.")); - } - - walletInstance->m_pay_tx_fee = CFeeRate{pay_tx_fee.value(), 1000}; - - if (chain && walletInstance->m_pay_tx_fee < chain->relayMinFee()) { - error = strprintf(_("Invalid amount for %s=: '%s' (must be at least %s)"), - "-paytxfee", *arg, chain->relayMinFee().ToString()); - return nullptr; - } - } - - if (const auto arg{args.GetArg("-maxtxfee")}) { - std::optional max_fee = ParseMoney(*arg); - if (!max_fee) { - error = AmountErrMsg("maxtxfee", *arg); - return nullptr; - } else if (max_fee.value() > HIGH_MAX_TX_FEE) { - warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee")); - } - - if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) { - error = strprintf(_("Invalid amount for %s=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"), - "-maxtxfee", *arg, chain->relayMinFee().ToString()); - return nullptr; - } - - walletInstance->m_default_max_tx_fee = max_fee.value(); - } - - if (const auto arg{args.GetArg("-consolidatefeerate")}) { - if (std::optional consolidate_feerate = ParseMoney(*arg)) { - walletInstance->m_consolidate_feerate = CFeeRate(*consolidate_feerate); - } else { - error = AmountErrMsg("consolidatefeerate", *arg); - return nullptr; - } - } - - if (chain && chain->relayMinFee().GetFeePerK() > HIGH_TX_FEE_PER_KB) { - warnings.push_back(AmountHighWarn("-minrelaytxfee") + Untranslated(" ") + - _("The wallet will avoid paying less than the minimum relay fee.")); - } - - walletInstance->m_confirm_target = args.GetIntArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET); - walletInstance->m_spend_zero_conf_change = args.GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE); - walletInstance->m_signal_rbf = args.GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF); - walletInstance->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); - walletInstance->WalletLogPrintf("Wallet completed loading in %15dms\n", Ticks(SteadyClock::now() - start)); // Try to top up keypool. No-op if the wallet is locked. diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1351ee18810..27ce8957ff2 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -871,6 +871,8 @@ public: /** Mark a transaction as replaced by another transaction. */ bool MarkReplaced(const Txid& originalHash, const Txid& newHash); + static bool LoadWalletArgs(std::shared_ptr wallet, const WalletContext& context, bilingual_str& error, std::vector& warnings); + /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ static std::shared_ptr Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings); From b4a49cc7275efc16d4a4179ed34b50de5bb7367e Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Tue, 27 May 2025 15:32:22 -0700 Subject: [PATCH 07/16] wallet: Move argument parsing to before DB load `m_keypool_size` must be set before `CWallet::PopulateWalletFromDB()`, in order to move parsing of `-keypool` into `CWallet::LoadWalletArgs`, `LoadWalletArgs()` invocation in `CWallet::Create()` must be moved before `PopulateWalletFromDB()` is called. --- src/wallet/wallet.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d95701fde8f..d30bdb7c511 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3016,6 +3016,9 @@ bool CWallet::LoadWalletArgs(std::shared_ptr wallet, const WalletContex wallet->m_confirm_target = args.GetIntArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET); wallet->m_spend_zero_conf_change = args.GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE); wallet->m_signal_rbf = args.GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF); + + wallet->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{1}); + wallet->m_notify_tx_changed_script = args.GetArg("-walletnotify", ""); wallet->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); return true; @@ -3024,15 +3027,16 @@ bool CWallet::LoadWalletArgs(std::shared_ptr wallet, const WalletContex std::shared_ptr CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) { interfaces::Chain* chain = context.chain; - ArgsManager& args = *Assert(context.args); const std::string& walletFile = database->Filename(); const auto start{SteadyClock::now()}; // TODO: Can't use std::make_shared because we need a custom deleter but // should be possible to use std::allocate_shared. std::shared_ptr walletInstance(new CWallet(chain, name, std::move(database)), FlushAndDeleteWallet); - walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{1}); - walletInstance->m_notify_tx_changed_script = args.GetArg("-walletnotify", ""); + + if (!LoadWalletArgs(walletInstance, context, error, warnings)) { + return nullptr; + } // Load wallet auto nLoadWalletRet = walletInstance->PopulateWalletFromDB(error, warnings); @@ -3079,10 +3083,6 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } } - if (!LoadWalletArgs(walletInstance, context, error, warnings)) { - return nullptr; - } - walletInstance->WalletLogPrintf("Wallet completed loading in %15dms\n", Ticks(SteadyClock::now() - start)); // Try to top up keypool. No-op if the wallet is locked. From a9d64cd49c69dafd6496ccb5aef4cd6d8898966b Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Tue, 27 May 2025 16:07:42 -0700 Subject: [PATCH 08/16] wallet: Remove redundant birth time update Checking every SPKM in `CWallet::Create()` is not necessary, since the only way presently for an SPKM to get added to `m_spk_managers` (the return value of `GetAllScriptPubKeyMans()`) is through `AddScriptPubKeyMan()`, which already invokes `MaybeUpdateBirthTime()`. --- src/wallet/wallet.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d30bdb7c511..a0138381d81 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3088,14 +3088,6 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri // Try to top up keypool. No-op if the wallet is locked. walletInstance->TopUpKeyPool(); - // Cache the first key time - std::optional time_first_key; - for (auto spk_man : walletInstance->GetAllScriptPubKeyMans()) { - int64_t time = spk_man->GetTimeFirstKey(); - if (!time_first_key || time < *time_first_key) time_first_key = time; - } - if (time_first_key) walletInstance->MaybeUpdateBirthTime(*time_first_key); - if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) { walletInstance->m_chain_notifications_handler.reset(); // Reset this pointer so that the wallet will actually be unloaded return nullptr; From bc69070416c62a88d8f4029280ec10d6f9ec8d20 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Tue, 27 May 2025 16:46:00 -0700 Subject: [PATCH 09/16] refactor: Wallet stats logging in its own function This will avoid repetition when wallet creation and loading are separated. --- src/wallet/wallet.cpp | 7 +------ src/wallet/wallet.h | 8 ++++++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a0138381d81..8ee2302c121 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3093,12 +3093,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri return nullptr; } - { - LOCK(walletInstance->cs_wallet); - walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n", walletInstance->GetKeyPoolSize()); - walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size()); - walletInstance->WalletLogPrintf("m_address_book.size() = %u\n", walletInstance->m_address_book.size()); - } + WITH_LOCK(walletInstance->cs_wallet, walletInstance->LogStats()); return walletInstance; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 27ce8957ff2..e4072c44a85 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -940,6 +940,14 @@ public: LogInfo("[%s] %s", LogName(), tfm::format(wallet_fmt, params...)); }; + void LogStats() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) + { + AssertLockHeld(cs_wallet); + WalletLogPrintf("setKeyPool.size() = %u\n", GetKeyPoolSize()); + WalletLogPrintf("mapWallet.size() = %u\n", mapWallet.size()); + WalletLogPrintf("m_address_book.size() = %u\n", m_address_book.size()); + }; + //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers std::set GetActiveScriptPubKeyMans() const; bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const; From ae66e011646266abb67b31027bc29e0ce1d08ad4 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Tue, 27 May 2025 17:32:51 -0700 Subject: [PATCH 10/16] wallet: Create separate function for wallet load Splits out logic relevant only to existing wallets in `CWallet::Create()` into `CWallet::LoadExisting()` --- src/wallet/wallet.cpp | 44 +++++++++++++++++++++++++++++++++++++++++++ src/wallet/wallet.h | 5 ++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8ee2302c121..44afd1cd15d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3098,6 +3098,50 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri return walletInstance; } +std::shared_ptr CWallet::LoadExisting(WalletContext& context, const std::string& name, std::unique_ptr database, bilingual_str& error, std::vector& warnings) +{ + interfaces::Chain* chain = context.chain; + const std::string& walletFile = database->Filename(); + + const auto start{SteadyClock::now()}; + std::shared_ptr walletInstance(new CWallet(chain, name, std::move(database)), FlushAndDeleteWallet); + + if (!LoadWalletArgs(walletInstance, context, error, warnings)) { + return nullptr; + } + + // Load wallet + 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; + } + + if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) { + if (spk_man->HavePrivateKeys()) { + warnings.push_back(strprintf(_("Warning: Private keys detected in wallet {%s} with disabled private keys"), walletFile)); + break; + } + } + } + + walletInstance->WalletLogPrintf("Wallet completed loading in %15dms\n", Ticks(SteadyClock::now() - start)); + + // Try to top up keypool. No-op if the wallet is locked. + walletInstance->TopUpKeyPool(); + + if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) { + walletInstance->m_chain_notifications_handler.reset(); // Reset this pointer so that the wallet will actually be unloaded + return nullptr; + } + + WITH_LOCK(walletInstance->cs_wallet, walletInstance->LogStats()); + + return walletInstance; +} + + bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector& warnings) { LOCK(walletInstance->cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e4072c44a85..d09612a1f3f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -873,9 +873,12 @@ public: static bool LoadWalletArgs(std::shared_ptr wallet, const WalletContext& context, bilingual_str& error, std::vector& warnings); - /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ + /* Initializes, creates and returns a new CWallet; returns a null pointer in case of an error */ static std::shared_ptr Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings); + /* Initializes, loads, and returns a CWallet from an existing wallet; returns a null pointer in case of an error */ + static std::shared_ptr LoadExisting(WalletContext& context, const std::string& name, std::unique_ptr database, bilingual_str& error, std::vector& warnings); + /** * Wallet post-init setup * Gives the wallet a chance to register repetitive tasks and complete post-init tasks From 70dbc79b09acf7b1515532ee20c7533c938ffb70 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Tue, 27 May 2025 18:18:50 -0700 Subject: [PATCH 11/16] wallet: Use CWallet::LoadExisting() for loading existing wallets. --- src/wallet/load.cpp | 2 +- src/wallet/wallet.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index f3620644730..8d2234664c0 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -147,7 +147,7 @@ bool LoadWallets(WalletContext& context) } } chain.initMessage(_("Loading wallet…")); - std::shared_ptr pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings) : nullptr; + std::shared_ptr pwallet = database ? CWallet::LoadExisting(context, name, std::move(database), error, warnings) : nullptr; if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); if (!pwallet) { chain.initError(error); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 44afd1cd15d..e5c21dc08fe 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -282,7 +282,7 @@ std::shared_ptr LoadWalletInternal(WalletContext& context, const std::s } context.chain->initMessage(_("Loading wallet…")); - std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings); + std::shared_ptr wallet = CWallet::LoadExisting(context, name, std::move(database), error, warnings); if (!wallet) { error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_LOAD; @@ -4252,7 +4252,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } // Make the local wallet - std::shared_ptr local_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + std::shared_ptr local_wallet = CWallet::LoadExisting(empty_context, wallet_name, std::move(database), error, warnings); if (!local_wallet) { return util::Error{Untranslated("Wallet loading failed.") + Untranslated(" ") + error}; } From e12ff8aca049ec7b054cb3047a167c7ce8dbd421 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Tue, 27 May 2025 19:05:54 -0700 Subject: [PATCH 12/16] test: wallet: Split create and load --- src/bench/wallet_ismine.cpp | 2 +- src/bench/wallet_loading.cpp | 4 ++-- src/wallet/test/util.cpp | 33 ++++++++++++++++++++++++++++---- src/wallet/test/util.h | 4 +++- src/wallet/test/wallet_tests.cpp | 6 +++--- 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/bench/wallet_ismine.cpp b/src/bench/wallet_ismine.cpp index 6eef1efd992..b0b9c2c5ad0 100644 --- a/src/bench/wallet_ismine.cpp +++ b/src/bench/wallet_ismine.cpp @@ -36,7 +36,7 @@ static void WalletIsMine(benchmark::Bench& bench, int num_combo = 0) // Loading the wallet will also create it uint64_t create_flags = WALLET_FLAG_DESCRIPTORS; auto database = CreateMockableWalletDatabase(); - auto wallet = TestLoadWallet(std::move(database), context, create_flags); + auto wallet = TestCreateWallet(std::move(database), context, create_flags); // For a descriptor wallet, fill with num_combo combo descriptors with random keys // This benchmarks a non-HD wallet migrated to descriptors diff --git a/src/bench/wallet_loading.cpp b/src/bench/wallet_loading.cpp index 3397fb0030e..8131f192c7a 100644 --- a/src/bench/wallet_loading.cpp +++ b/src/bench/wallet_loading.cpp @@ -43,7 +43,7 @@ static void WalletLoadingDescriptors(benchmark::Bench& bench) // Loading the wallet will also create it uint64_t create_flags = WALLET_FLAG_DESCRIPTORS; auto database = CreateMockableWalletDatabase(); - auto wallet = TestLoadWallet(std::move(database), context, create_flags); + auto wallet = TestCreateWallet(std::move(database), context, create_flags); // Generate a bunch of transactions and addresses to put into the wallet for (int i = 0; i < 1000; ++i) { @@ -56,7 +56,7 @@ static void WalletLoadingDescriptors(benchmark::Bench& bench) TestUnloadWallet(std::move(wallet)); bench.epochs(5).run([&] { - wallet = TestLoadWallet(std::move(database), context, create_flags); + wallet = TestLoadWallet(std::move(database), context); // Cleanup database = DuplicateMockDatabase(wallet->GetDatabase()); diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index c06c6af5b55..db85b5efe8f 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -47,11 +47,36 @@ std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, CChain& cc return wallet; } -std::shared_ptr TestLoadWallet(std::unique_ptr database, WalletContext& context, uint64_t create_flags) +std::shared_ptr TestCreateWallet(std::unique_ptr database, WalletContext& context, uint64_t create_flags) +{ + bilingual_str _error; + std::vector _warnings; + auto wallet = CWallet::Create(context, "", std::move(database), create_flags, _error, _warnings); + NotifyWalletLoaded(context, wallet); + if (context.chain) { + wallet->postInitProcess(); + } + return wallet; +} + +std::shared_ptr TestCreateWallet(WalletContext& context) +{ + DatabaseOptions options; + options.require_create = true; + options.create_flags = WALLET_FLAG_DESCRIPTORS; + DatabaseStatus status; + bilingual_str error; + std::vector warnings; + auto database = MakeWalletDatabase("", options, status, error); + return TestCreateWallet(std::move(database), context, options.create_flags); +} + + +std::shared_ptr TestLoadWallet(std::unique_ptr database, WalletContext& context) { bilingual_str error; std::vector warnings; - auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings); + auto wallet = CWallet::LoadExisting(context, "", std::move(database), error, warnings); NotifyWalletLoaded(context, wallet); if (context.chain) { wallet->postInitProcess(); @@ -62,12 +87,12 @@ std::shared_ptr TestLoadWallet(std::unique_ptr database std::shared_ptr TestLoadWallet(WalletContext& context) { DatabaseOptions options; - options.create_flags = WALLET_FLAG_DESCRIPTORS; + options.require_existing = true; DatabaseStatus status; bilingual_str error; std::vector warnings; auto database = MakeWalletDatabase("", options, status, error); - return TestLoadWallet(std::move(database), context, options.create_flags); + return TestLoadWallet(std::move(database), context); } void TestUnloadWallet(std::shared_ptr&& wallet) diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index ecedbd90807..18305fb598e 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -32,8 +32,10 @@ const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqq std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key); +std::shared_ptr TestCreateWallet(WalletContext& context); +std::shared_ptr TestCreateWallet(std::unique_ptr database, WalletContext& context, uint64_t create_flags); std::shared_ptr TestLoadWallet(WalletContext& context); -std::shared_ptr TestLoadWallet(std::unique_ptr database, WalletContext& context, uint64_t create_flags); +std::shared_ptr TestLoadWallet(std::unique_ptr database, WalletContext& context); void TestUnloadWallet(std::shared_ptr&& wallet); // Creates a copy of the provided database diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 50d2f78b8d7..4c422904990 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -584,7 +584,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) WalletContext context; context.args = &m_args; context.chain = m_node.chain.get(); - auto wallet = TestLoadWallet(context); + auto wallet = TestCreateWallet(context); CKey key = GenerateRandomKey(); AddKey(*wallet, key); TestUnloadWallet(std::move(wallet)); @@ -681,7 +681,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup) { WalletContext context; context.args = &m_args; - auto wallet = TestLoadWallet(context); + auto wallet = TestCreateWallet(context); BOOST_CHECK(wallet); WaitForDeleteWallet(std::move(wallet)); } @@ -692,7 +692,7 @@ BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup) WalletContext context; context.args = &m_args; context.chain = m_node.chain.get(); - auto wallet = TestLoadWallet(context); + auto wallet = TestCreateWallet(context); CKey key = GenerateRandomKey(); AddKey(*wallet, key); From f35acc893fb3378b2ad39608fe254d33af6cce9f Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 22 Aug 2025 21:34:46 -0700 Subject: [PATCH 13/16] refactor: wallet: Factor out `WriteVersion()` from `PopulateWalletFromDB()` Writing the wallet's `CLIENT_VERSION` (which indicates the last version to have touched a wallet) needs to be done on both wallet creation and wallet loading. The next commit removes the `PopulateWalletFromDatabase()` call from wallet creation, but this behavior needs to be preserved, so this commit factors setting `CLIENT_VERSION` out of `PopulateWalletFromDatabase()` so that wallet creation can use it in the next commit. --- src/wallet/walletdb.cpp | 2 +- src/wallet/walletdb.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 9ca4d2da8bb..387568282da 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1175,7 +1175,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; if (!has_last_client || last_client != CLIENT_VERSION) // Update - m_batch->Write(DBKeys::VERSION, CLIENT_VERSION); + this->WriteVersion(CLIENT_VERSION); if (any_unordered) result = pwallet->ReorderTransactions(); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index a867a28b970..788d44866a5 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -265,6 +265,9 @@ public: DBErrors LoadWallet(CWallet* pwallet); + //! Write the given client_version. + bool WriteVersion(int client_version) { return m_batch->Write(DBKeys::VERSION, CLIENT_VERSION); } + //! Delete records of the given types bool EraseRecords(const std::unordered_set& types); From d8bec61be233b9cb6d5db886e8f1c1f058288fb5 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 22 Aug 2025 23:22:48 -0700 Subject: [PATCH 14/16] wallet: remove loading logic from CWallet::Create --- src/wallet/wallet.cpp | 28 +++++--------------------- test/functional/wallet_createwallet.py | 17 +++++++++------- 2 files changed, 15 insertions(+), 30 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e5c21dc08fe..a11f8511dd1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3038,18 +3038,11 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri return nullptr; } - // Load wallet - auto nLoadWalletRet = walletInstance->PopulateWalletFromDB(error, warnings); - bool rescan_required = nLoadWalletRet == DBErrors::NEED_RESCAN; - if (nLoadWalletRet != DBErrors::LOAD_OK && nLoadWalletRet != DBErrors::NONCRITICAL_ERROR && !rescan_required) { + // Initialize version key. + if(!WalletBatch(walletInstance->GetDatabase()).WriteVersion(CLIENT_VERSION)) { + error = strprintf(_("Error creating %s: Could not write version metadata."), walletFile); return nullptr; } - - // This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys - const bool fFirstRun = walletInstance->m_spk_managers.empty() && - !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && - !walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); - if (fFirstRun) { LOCK(walletInstance->cs_wallet); @@ -3070,25 +3063,14 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->SetLastBlockProcessed(*tip_height, chain->getBlockHash(*tip_height)); } } - } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) { - // Make it impossible to disable private keys after creation - error = strprintf(_("Error loading %s: Private keys can only be disabled during creation"), walletFile); - return nullptr; - } else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { - for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) { - if (spk_man->HavePrivateKeys()) { - warnings.push_back(strprintf(_("Warning: Private keys detected in wallet {%s} with disabled private keys"), walletFile)); - break; - } - } } - walletInstance->WalletLogPrintf("Wallet completed loading in %15dms\n", Ticks(SteadyClock::now() - start)); + walletInstance->WalletLogPrintf("Wallet completed creation in %15dms\n", Ticks(SteadyClock::now() - start)); // Try to top up keypool. No-op if the wallet is locked. walletInstance->TopUpKeyPool(); - if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) { + if (chain && !AttachChain(walletInstance, *chain, /*rescan_required=*/false, error, warnings)) { walletInstance->m_chain_notifications_handler.reset(); // Reset this pointer so that the wallet will actually be unloaded return nullptr; } diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py index cf0d8b4e0b8..44d523a40ef 100755 --- a/test/functional/wallet_createwallet.py +++ b/test/functional/wallet_createwallet.py @@ -167,14 +167,17 @@ class CreateWalletTest(BitcoinTestFramework): assert_raises_rpc_error(-4, 'descriptors argument must be set to "true"; it is no longer possible to create a legacy wallet.', self.nodes[0].createwallet, wallet_name="legacy", descriptors=False) self.log.info("Check that the version number is being logged correctly") - with node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Last client version = "]): - node.createwallet("version_check") - wallet = node.get_wallet_rpc("version_check") + + # Craft the expected version message. client_version = node.getnetworkinfo()["version"] - wallet.unloadwallet() - with node.assert_debug_log( - expected_msgs=[f"Last client version = {client_version}"] - ): + version_message = f"Last client version = {client_version}" + + # Should not be logged when creating. + with node.assert_debug_log(expected_msgs=[], unexpected_msgs=[version_message]): + node.createwallet("version_check") + node.unloadwallet("version_check") + # Should be logged when loading. + with node.assert_debug_log(expected_msgs=[version_message]): node.loadwallet("version_check") From 27e021ebc0dd3517a71f3ddb38ed265a19693d4c Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 22 Aug 2025 23:22:58 -0700 Subject: [PATCH 15/16] wallet: Correctly log stats for encrypted messages. Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2150021064 --- src/wallet/wallet.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a11f8511dd1..d7663ed0e62 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -413,7 +413,7 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& } // Make the wallet - context.chain->initMessage(_("Loading wallet…")); + context.chain->initMessage(_("Creating wallet…")); std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), wallet_creation_flags, error, warnings); if (!wallet) { error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error; @@ -447,6 +447,7 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& } } + WITH_LOCK(wallet->cs_wallet, wallet->LogStats()); NotifyWalletLoaded(context, wallet); AddWallet(context, wallet); wallet->postInitProcess(); @@ -3075,8 +3076,6 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri return nullptr; } - WITH_LOCK(walletInstance->cs_wallet, walletInstance->LogStats()); - return walletInstance; } From db2effaca4cf82bf806596d16f9797d3692e2da7 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 28 May 2025 16:23:35 -0700 Subject: [PATCH 16/16] scripted-diff: refactor: CWallet::Create() -> CreateNew() Aside from being more legible, changing the name of `CWallet::Create()` also validates that every instance where a new wallet is `Create()`'ed is handled in this branch. -BEGIN VERIFY SCRIPT- sed -i 's|\bCreate(|CreateNew(|g' src/wallet/wallet.cpp src/wallet/wallet.h src/wallet/test/util.cpp src/wallet/test/wallet_tests.cpp -END VERIFY SCRIPT- --- src/wallet/test/util.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 8 ++++---- src/wallet/wallet.h | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index db85b5efe8f..0cc9217bb8a 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -51,7 +51,7 @@ std::shared_ptr TestCreateWallet(std::unique_ptr databa { bilingual_str _error; std::vector _warnings; - auto wallet = CWallet::Create(context, "", std::move(database), create_flags, _error, _warnings); + auto wallet = CWallet::CreateNew(context, "", std::move(database), create_flags, _error, _warnings); NotifyWalletLoaded(context, wallet); if (context.chain) { wallet->postInitProcess(); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 4c422904990..e2d23602149 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -559,7 +559,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup) BOOST_CHECK_EXCEPTION(vr >> w_desc, std::ios_base::failure, malformed_descriptor); } -//! Test CWallet::Create() and its behavior handling potential race +//! Test CWallet::CreateNew() and its behavior handling potential race //! conditions if it's called the same time an incoming transaction shows up in //! the mempool or a new block. //! diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d7663ed0e62..a21109a6041 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -414,7 +414,7 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& // Make the wallet context.chain->initMessage(_("Creating wallet…")); - std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), wallet_creation_flags, error, warnings); + std::shared_ptr wallet = CWallet::CreateNew(context, name, std::move(database), wallet_creation_flags, error, warnings); if (!wallet) { error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_CREATE; @@ -3025,7 +3025,7 @@ bool CWallet::LoadWalletArgs(std::shared_ptr wallet, const WalletContex return true; } -std::shared_ptr CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) +std::shared_ptr CWallet::CreateNew(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) { interfaces::Chain* chain = context.chain; const std::string& walletFile = database->Filename(); @@ -4117,7 +4117,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, return false; } - data->watchonly_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + data->watchonly_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); if (!data->watchonly_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; @@ -4156,7 +4156,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, return false; } - data->solvable_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + data->solvable_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); if (!data->solvable_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d09612a1f3f..b6c70dc54f4 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -874,7 +874,7 @@ public: static bool LoadWalletArgs(std::shared_ptr wallet, const WalletContext& context, bilingual_str& error, std::vector& warnings); /* Initializes, creates and returns a new CWallet; returns a null pointer in case of an error */ - static std::shared_ptr Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings); + static std::shared_ptr CreateNew(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings); /* Initializes, loads, and returns a CWallet from an existing wallet; returns a null pointer in case of an error */ static std::shared_ptr LoadExisting(WalletContext& context, const std::string& name, std::unique_ptr database, bilingual_str& error, std::vector& warnings);