From 0695a4f156799eadd06ef15b433a3dcc77af98e1 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 7 Jul 2023 17:23:09 +0100 Subject: [PATCH 1/5] Merge bitcoin/bitcoin#28038: wallet: address book migration bug fixes 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9 test: wallet, add coverage for addressbook migration (furszy) a277f8357ad8b0eb26f33fc36f919d868c06847b wallet: migration bugfix, persist empty labels (furszy) 1b64f6498c394a143df196172a14204fe3b8a744 wallet: migration bugfix, clone 'send' record label to all wallets (furszy) Pull request description: Addressing two specific bugs encountered during the wallet migration process, related to the address book, and improves the test coverage for it. Bug 1: Non-Cloning of External 'Send' Records The external 'send' records were not being correctly cloned to all wallets. Bug 2: Persistence of Empty Labels As address book entries without associated db label records can be treated as change (the `label` field inside the `CAddressBookData` class is optional, `nullopt` labels make `CAddressBookData ::IsChange()` return true), we must persist empty labels during the migration process. The user might have called `setlabel` with an "" string for an external address and that must be retained during migration. ACKs for top commit: achow101: ACK 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9 Tree-SHA512: b8a8483a4178a37c49af11eb7ba8a82ca95e54a6cd799e155e33f9fbe7f37b259e28372c77d6944d46b6765f9eaca6b8ca8d1cdd9d223120a3653e4e41d0b6b7 Co-authored-by: fanquake --- src/wallet/wallet.cpp | 10 +- test/functional/wallet_migration.py | 136 ++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d12810de74bd..7b80b6ef696a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4552,7 +4552,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return false; } } else { - // Labels for everything else (send) should be cloned to all + // Labels for everything else ("send") should be cloned to all if (data.watchonly_wallet) { LOCK(data.watchonly_wallet->cs_wallet); // Add to the watchonly. Preserve the labels, purpose, and change-ness @@ -4564,7 +4564,6 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) if (!addr_pair.second.IsChange()) { data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label); } - continue; } if (data.solvable_wallet) { LOCK(data.solvable_wallet->cs_wallet); @@ -4577,7 +4576,6 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) if (!addr_pair.second.IsChange()) { data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label); } - continue; } } } @@ -4589,10 +4587,10 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) for (const auto& [destination, addr_book_data] : wallet.m_address_book) { auto address{EncodeDestination(destination)}; auto purpose{addr_book_data.purpose}; - auto label{addr_book_data.GetLabel()}; - // don't bother writing default values (unknown purpose, empty label) + std::optional label = addr_book_data.IsChange() ? std::nullopt : std::make_optional(addr_book_data.GetLabel()); + // don't bother writing default values (unknown purpose) if (purpose != "unknown") batch.WritePurpose(address, purpose); - if (!label.empty()) batch.WriteName(address, label); + if (label) batch.WriteName(address, *label); } }; if (data.watchonly_wallet) persist_address_book(*data.watchonly_wallet); diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index d3bb2d88498e..f5ff0762b6bc 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -62,6 +62,15 @@ def assert_list_txs_equal(self, received_list_txs, expected_list_txs): del d["parent_descs"] assert_equal(received_list_txs, expected_list_txs) + def check_address(self, wallet, addr, is_mine, is_change, label): + addr_info = wallet.getaddressinfo(addr) + assert_equal(addr_info['ismine'], is_mine) + assert_equal(addr_info['ischange'], is_change) + if label is not None: + assert_equal(addr_info['labels'], [label]), + else: + assert_equal(addr_info['labels'], []), + def test_basic(self): default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) @@ -542,6 +551,132 @@ def test_wallet_name_with_slashes(self): self.assert_is_sqlite(nested_name) + def test_addressbook(self): + df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + self.log.info("Test migration of address book data") + wallet = self.create_legacy_wallet("legacy_addrbook") + df_wallet.sendtoaddress(wallet.getnewaddress(), 3) + + # Import watch-only script to create a watch-only wallet after migration + watch_addr = df_wallet.getnewaddress() + wallet.importaddress(watch_addr) + df_wallet.sendtoaddress(watch_addr, 2) + + # Import solvable script + multi_addr1 = wallet.getnewaddress() + multi_addr2 = wallet.getnewaddress() + multi_addr3 = df_wallet.getnewaddress() + wallet.importpubkey(df_wallet.getaddressinfo(multi_addr3)["pubkey"]) + ms_addr_info = wallet.addmultisigaddress(2, [multi_addr1, multi_addr2, multi_addr3]) + + self.generate(self.nodes[0], 1) + + # Test vectors + addr_external = { + "addr": df_wallet.getnewaddress(), + "is_mine": False, + "is_change": False, + "label": "" + } + addr_external_with_label = { + "addr": df_wallet.getnewaddress(), + "is_mine": False, + "is_change": False, + "label": "external" + } + addr_internal = { + "addr": wallet.getnewaddress(), + "is_mine": True, + "is_change": False, + "label": "" + } + addr_internal_with_label = { + "addr": wallet.getnewaddress(), + "is_mine": True, + "is_change": False, + "label": "internal" + } + change_address = { + "addr": wallet.getrawchangeaddress(), + "is_mine": True, + "is_change": True, + "label": None + } + watch_only_addr = { + "addr": watch_addr, + "is_mine": False, + "is_change": False, + "label": "imported" + } + ms_addr = { + "addr": ms_addr_info['address'], + "is_mine": False, + "is_change": False, + "label": "multisig" + } + + # To store the change address in the addressbook need to send coins to it + wallet.send(outputs=[{wallet.getnewaddress(): 2}], options={"change_address": change_address['addr']}) + self.generate(self.nodes[0], 1) + + # Util wrapper func for 'addr_info' + def check(info, node): + self.check_address(node, info['addr'], info['is_mine'], info['is_change'], info["label"]) + + # Pre-migration: set label and perform initial checks + for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, watch_only_addr, ms_addr]: + if not addr_info['is_change']: + wallet.setlabel(addr_info['addr'], addr_info["label"]) + check(addr_info, wallet) + + # Migrate wallet + info_migration = wallet.migratewallet() + wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) + wallet_solvables = self.nodes[0].get_wallet_rpc(info_migration["solvables_name"]) + + ######################### + # Post migration checks # + ######################### + + # First check the main wallet + for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, ms_addr]: + check(addr_info, wallet) + + # Watch-only wallet will contain the watch-only entry (with 'is_mine=True') and all external addresses ('send') + self.check_address(wallet_wo, watch_only_addr['addr'], is_mine=True, is_change=watch_only_addr['is_change'], label=watch_only_addr["label"]) + for addr_info in [addr_external, addr_external_with_label, ms_addr]: + check(addr_info, wallet_wo) + + # Solvables wallet will contain the multisig entry (with 'is_mine=True') and all external addresses ('send') + self.check_address(wallet_solvables, ms_addr['addr'], is_mine=True, is_change=ms_addr['is_change'], label=ms_addr["label"]) + for addr_info in [addr_external, addr_external_with_label]: + check(addr_info, wallet_solvables) + + ######################################################################################## + # Now restart migrated wallets and verify that the addressbook entries are still there # + ######################################################################################## + + # First the main wallet + self.nodes[0].unloadwallet("legacy_addrbook") + self.nodes[0].loadwallet("legacy_addrbook") + for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, ms_addr]: + check(addr_info, wallet) + + # Watch-only wallet + self.nodes[0].unloadwallet(info_migration["watchonly_name"]) + self.nodes[0].loadwallet(info_migration["watchonly_name"]) + self.check_address(wallet_wo, watch_only_addr['addr'], is_mine=True, is_change=watch_only_addr['is_change'], label=watch_only_addr["label"]) + for addr_info in [addr_external, addr_external_with_label, ms_addr]: + check(addr_info, wallet_wo) + + # Solvables wallet + self.nodes[0].unloadwallet(info_migration["solvables_name"]) + self.nodes[0].loadwallet(info_migration["solvables_name"]) + self.check_address(wallet_solvables, ms_addr['addr'], is_mine=True, is_change=ms_addr['is_change'], label=ms_addr["label"]) + for addr_info in [addr_external, addr_external_with_label]: + check(addr_info, wallet_solvables) + def run_test(self): self.generate(self.nodes[0], 101) @@ -555,6 +690,7 @@ def run_test(self): self.test_unloaded() self.test_unloaded_by_path() self.test_wallet_name_with_slashes() + self.test_addressbook() if __name__ == '__main__': WalletMigrationTest().main() From a084e3755ad21f08786a77eab3742d11e8707462 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 20 Sep 2023 22:25:37 +0100 Subject: [PATCH 2/5] Merge bitcoin-core/gui#738: Add menu option to migrate a wallet 48aae2cffeb91add75a70ac4d5075c38054452fa gui: Add File > Migrate Wallet (Andrew Chow) 577be889cd52fc2d896a5f39c66bc2cadb8622e4 gui: Optionally return passphrase after unlocking (Andrew Chow) 5b3a85b4c6ffd1f29a917d4c1af4bff6c0ea2ef5 interfaces, wallet: Expose migrate wallet (Andrew Chow) Pull request description: GUI users need to be able to migrate wallets without going to the RPC console. ACKs for top commit: jarolrod: ACK 48aae2cffeb91add75a70ac4d5075c38054452fa pablomartin4btc: tACK 48aae2cffeb91add75a70ac4d5075c38054452fa hebasto: ACK 48aae2cffeb91add75a70ac4d5075c38054452fa Tree-SHA512: 2d02b1e85e7d6cfbf503f417f150cdaa0c63822942e9a6fe28c0ad3e7f40a957bb01a375c909a60432dc600e84574881aa446c7ec983b56f0bb23f07ef15de54 Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> --- src/interfaces/wallet.h | 13 +++++++ src/qt/askpassphrasedialog.cpp | 7 ++-- src/qt/bitcoingui.cpp | 13 ++++++- src/qt/bitcoingui.h | 2 ++ src/qt/walletcontroller.cpp | 64 ++++++++++++++++++++++++++++++++++ src/qt/walletcontroller.h | 22 ++++++++++++ src/wallet/interfaces.cpp | 13 +++++++ src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 1 + 9 files changed, 132 insertions(+), 5 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 134180f3728a..363f961eab3d 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -62,6 +62,7 @@ struct WalletBalances; struct WalletTx; struct WalletTxOut; struct WalletTxStatus; +struct WalletMigrationResult; namespace CoinJoin { class Loader; } @@ -401,6 +402,9 @@ class WalletLoader : public ChainClient //! Restore backup wallet virtual util::Result> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) = 0; + //! Migrate a wallet + virtual util::Result migrateWallet(const std::string& name, const SecureString& passphrase) = 0; + //! Return available wallets in wallet directory. virtual std::vector listWalletDir() = 0; @@ -504,6 +508,15 @@ struct WalletTxOut bool is_spent = false; }; +//! Migrated wallet info +struct WalletMigrationResult +{ + std::unique_ptr wallet; + std::optional watchonly_wallet_name; + std::optional solvables_wallet_name; + fs::path backup_path; +}; + //! Return implementation of Wallet interface. This function is defined in //! dummywallet.cpp and throws if the wallet component is not compiled. std::unique_ptr MakeWallet(wallet::WalletContext& context, const std::shared_ptr& wallet); diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index ffdc26c12dfc..f53543aed34e 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -195,9 +195,10 @@ void AskPassphraseDialog::accept() "the first null character. If this is successful, please set a new " "passphrase to avoid this issue in the future.")); } - } - else - { + } else { + if (m_passphrase_out) { + m_passphrase_out->assign(oldpass); + } QDialog::accept(); // Success } } catch (const std::runtime_error& e) { diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 4c581111961a..6bb1c0314d5a 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -493,6 +493,10 @@ void BitcoinGUI::createActions() m_close_all_wallets_action = new QAction(tr("Close All Wallets…"), this); m_close_all_wallets_action->setStatusTip(tr("Close all wallets")); + m_migrate_wallet_action = new QAction(tr("Migrate Wallet"), this); + m_migrate_wallet_action->setEnabled(false); + m_migrate_wallet_action->setStatusTip(tr("Migrate a wallet")); + showHelpMessageAction = new QAction(tr("&Command-line options"), this); showHelpMessageAction->setMenuRole(QAction::NoRole); showHelpMessageAction->setStatusTip(tr("Show the %1 help message to get a list with possible Dash command-line options").arg(PACKAGE_NAME)); @@ -615,7 +619,11 @@ void BitcoinGUI::createActions() connect(m_close_all_wallets_action, &QAction::triggered, [this] { m_wallet_controller->closeAllWallets(this); }); - + connect(m_migrate_wallet_action, &QAction::triggered, [this] { + auto activity = new MigrateWalletActivity(m_wallet_controller, this); + connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet); + activity->migrate(walletFrame->currentWalletModel()); + }); connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::setPrivacy); } #endif // ENABLE_WALLET @@ -633,6 +641,7 @@ void BitcoinGUI::createMenuBar() file->addAction(m_open_wallet_action); file->addAction(m_close_wallet_action); file->addAction(m_close_all_wallets_action); + file->addAction(m_migrate_wallet_action); file->addSeparator(); file->addAction(backupWalletAction); file->addAction(m_restore_wallet_action); @@ -1051,6 +1060,7 @@ void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model) } } updateWindowTitle(); + m_migrate_wallet_action->setEnabled(wallet_model->wallet().isLegacy()); } void BitcoinGUI::setCurrentWalletBySelectorIndex(int index) @@ -1101,6 +1111,7 @@ void BitcoinGUI::setWalletActionsEnabled(bool enabled) openAction->setEnabled(enabled); m_close_wallet_action->setEnabled(enabled); m_close_all_wallets_action->setEnabled(enabled); + m_migrate_wallet_action->setEnabled(enabled); updateWidth(); } diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h index 333ef538d412..19df08e5bbe0 100644 --- a/src/qt/bitcoingui.h +++ b/src/qt/bitcoingui.h @@ -185,6 +185,8 @@ class BitcoinGUI : public QMainWindow QAction* m_close_all_wallets_action{nullptr}; QAction* m_wallet_selector_action = nullptr; QAction* m_mask_values_action{nullptr}; + QAction* m_migrate_wallet_action{nullptr}; + QMenu* m_migrate_wallet_menu{nullptr}; QComboBox* m_wallet_selector = nullptr; diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 68e2cf7fb4e4..88394f3dc776 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -558,3 +558,67 @@ void RescanWalletActivity::finish() Q_EMIT finished(); } + +void MigrateWalletActivity::migrate(WalletModel* wallet_model) +{ + // Warn the user about migration + QMessageBox box(m_parent_widget); + box.setWindowTitle(tr("Migrate wallet")); + box.setText(tr("Are you sure you wish to migrate the wallet %1?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName()))); + box.setInformativeText(tr("Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.\n" + "If this wallet contains any watchonly scripts, a new wallet will be created which contains those watchonly scripts.\n" + "If this wallet contains any solvable but not watched scripts, a different and new wallet will be created which contains those scripts.\n\n" + "The migration process will create a backup of the wallet before migrating. This backup file will be named " + "-.legacy.bak and can be found in the directory for this wallet. In the event of " + "an incorrect migration, the backup can be restored with the \"Restore Wallet\" functionality.")); + box.setStandardButtons(QMessageBox::Yes|QMessageBox::Cancel); + box.setDefaultButton(QMessageBox::Yes); + if (box.exec() != QMessageBox::Yes) return; + + // Get the passphrase if it is encrypted regardless of it is locked or unlocked. We need the passphrase itself. + SecureString passphrase; + WalletModel::EncryptionStatus enc_status = wallet_model->getEncryptionStatus(); + if (enc_status == WalletModel::EncryptionStatus::Locked || enc_status == WalletModel::EncryptionStatus::Unlocked) { + AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, m_parent_widget, &passphrase); + dlg.setModel(wallet_model); + dlg.exec(); + } + + // GUI needs to remove the wallet so that it can actually be unloaded by migration + const std::string name = wallet_model->wallet().getWalletName(); + m_wallet_controller->removeAndDeleteWallet(wallet_model); + + showProgressDialog(tr("Migrate Wallet"), tr("Migrating Wallet %1…").arg(GUIUtil::HtmlEscape(name))); + + QTimer::singleShot(0, worker(), [this, name, passphrase] { + auto res{node().walletLoader().migrateWallet(name, passphrase)}; + + if (res) { + m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(res->wallet->getWalletName())); + if (res->watchonly_wallet_name) { + m_success_message += QChar(' ') + tr("Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->watchonly_wallet_name.value())); + } + if (res->solvables_wallet_name) { + m_success_message += QChar(' ') + tr("Solvable but not watched scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->solvables_wallet_name.value())); + } + m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(res->wallet)); + } else { + m_error_message = util::ErrorString(res); + } + + QTimer::singleShot(0, this, &MigrateWalletActivity::finish); + }); +} + +void MigrateWalletActivity::finish() +{ + if (!m_error_message.empty()) { + QMessageBox::critical(m_parent_widget, tr("Migration failed"), QString::fromStdString(m_error_message.translated)); + } else { + QMessageBox::information(m_parent_widget, tr("Migration Successful"), m_success_message); + } + + if (m_wallet_model) Q_EMIT migrated(m_wallet_model); + + Q_EMIT finished(); +} diff --git a/src/qt/walletcontroller.h b/src/qt/walletcontroller.h index c5c4fa831bf1..e6087162cfdb 100644 --- a/src/qt/walletcontroller.h +++ b/src/qt/walletcontroller.h @@ -39,6 +39,7 @@ class path; class AskPassphraseDialog; class CreateWalletActivity; class CreateWalletDialog; +class MigrateWalletActivity; class OpenWalletActivity; class WalletControllerActivity; @@ -64,6 +65,8 @@ class WalletController : public QObject void closeWallet(WalletModel* wallet_model, QWidget* parent = nullptr); void closeAllWallets(QWidget* parent = nullptr); + void migrateWallet(WalletModel* wallet_model, QWidget* parent = nullptr); + Q_SIGNALS: void walletAdded(WalletModel* wallet_model); void walletRemoved(WalletModel* wallet_model); @@ -81,6 +84,7 @@ class WalletController : public QObject std::unique_ptr m_handler_load_wallet; friend class WalletControllerActivity; + friend class MigrateWalletActivity; }; class WalletControllerActivity : public QObject @@ -193,4 +197,22 @@ class RescanWalletActivity : public WalletControllerActivity wallet::RescanStatus m_rescan_status{}; }; +class MigrateWalletActivity : public WalletControllerActivity +{ + Q_OBJECT + +public: + MigrateWalletActivity(WalletController* wallet_controller, QWidget* parent) : WalletControllerActivity(wallet_controller, parent) {} + + void migrate(WalletModel* wallet_model); + +Q_SIGNALS: + void migrated(WalletModel* wallet_model); + +private: + QString m_success_message; + + void finish(); +}; + #endif // BITCOIN_QT_WALLETCONTROLLER_H diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index d79475001c88..7e6e1568ec28 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -52,6 +52,7 @@ using interfaces::Wallet; using interfaces::WalletAddress; using interfaces::WalletBalances; using interfaces::WalletLoader; +using interfaces::WalletMigrationResult; using interfaces::WalletOrderForm; using interfaces::WalletTx; using interfaces::WalletTxOut; @@ -809,6 +810,18 @@ class WalletLoaderImpl : public WalletLoader return util::Error{error}; } } + util::Result migrateWallet(const std::string& name, const SecureString& passphrase) override + { + auto res = wallet::MigrateLegacyToDescriptor(name, passphrase, m_context); + if (!res) return util::Error{util::ErrorString(res)}; + WalletMigrationResult out{ + .wallet = MakeWallet(m_context, res->wallet), + .watchonly_wallet_name = res->watchonly_wallet ? std::make_optional(res->watchonly_wallet->GetName()) : std::nullopt, + .solvables_wallet_name = res->solvables_wallet ? std::make_optional(res->solvables_wallet->GetName()) : std::nullopt, + .backup_path = res->backup_path, + }; + return {std::move(out)}; // std::move to work around clang bug + } std::string getWalletDir() override { return fs::PathToString(GetWalletDir()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7b80b6ef696a..68f00f6a8bab 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4796,7 +4796,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // Migration successful, unload the wallet locally, then reload it. assert(local_wallet.use_count() == 1); local_wallet.reset(); - LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + res.wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); res.wallet_name = wallet_name; } else { // Migration failed, cleanup diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7e15a2fd9585..1affd13a8980 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1193,6 +1193,7 @@ bool FillInputToWeight(CTxIn& txin, int64_t target_weight); struct MigrationResult { std::string wallet_name; + std::shared_ptr wallet; std::shared_ptr watchonly_wallet; std::shared_ptr solvables_wallet; fs::path backup_path; From 3c17eb4526befdfa35ea027b82e8b389f73abd03 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 20 Dec 2022 18:05:53 -0500 Subject: [PATCH 3/5] Merge bitcoin/bitcoin#26638: test: prefer sqlite for wallet tests 17554efb6095f6ec273e52568efe1678253cb7c0 test: prefer sqlite for wallet tests (S3RK) 8e0fabaabf8f45b82256c1934c2507d03351acaa test: make wallet_migration.py pass with both wallet flags (S3RK) Pull request description: Fixes #26511 ACKs for top commit: MarcoFalke: review ACK 17554efb6095f6ec273e52568efe1678253cb7c0 achow101: ACK 17554efb6095f6ec273e52568efe1678253cb7c0 Tree-SHA512: 97cae275998f07032feffe1b533d4747b8ff03c3c1fb830af69ee38cadb75fd58532956f66f79c0d275b00620ce53b0b5240f885e4f29b8bd4d0b6e6cbc683fa Co-authored-by: Andrew Chow --- .../test_framework/test_framework.py | 8 ++--- test/functional/wallet_migration.py | 33 +++++++------------ 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index d6216c12b9b1..0adb9257d424 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -256,11 +256,11 @@ def parse_args(self): self.options.descriptors = None elif self.options.descriptors is None: # Some wallet is either required or optionally used by the test. - # Prefer BDB unless it isn't available - if self.is_bdb_compiled(): - self.options.descriptors = False - elif self.is_sqlite_compiled(): + # Prefer SQLite unless it isn't available + if self.is_sqlite_compiled(): self.options.descriptors = True + elif self.is_bdb_compiled(): + self.options.descriptors = False else: # If neither are compiled, tests requiring a wallet will be skipped and the value of self.options.descriptors won't matter # It still needs to exist and be None in order for tests to work however. diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index f5ff0762b6bc..c657614d936b 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -37,11 +37,13 @@ def assert_is_sqlite(self, wallet_name): assert_equal(file_magic, b'SQLite format 3\x00') assert_equal(self.nodes[0].get_wallet_rpc(wallet_name).getwalletinfo()["format"], "sqlite") - def create_legacy_wallet(self, wallet_name): - self.nodes[0].createwallet(wallet_name=wallet_name) + def create_legacy_wallet(self, wallet_name, disable_private_keys=False, blank=False): + self.nodes[0].createwallet(wallet_name=wallet_name, descriptors=False, disable_private_keys=disable_private_keys, blank=blank) wallet = self.nodes[0].get_wallet_rpc(wallet_name) - assert_equal(wallet.getwalletinfo()["descriptors"], False) - assert_equal(wallet.getwalletinfo()["format"], "bdb") + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], False) + assert_equal(info["format"], "bdb") + assert_equal(info["private_keys_enabled"], not disable_private_keys) return wallet def assert_addr_info_equal(self, addr_info, addr_info_old): @@ -183,8 +185,7 @@ def test_basic(self): BASIC2_SEED_WIF = "cMai6KJ8sHnNejZctqjiYdg2KSLeiaKcuTbZQrJNEjmMY5JQw6eP" BASIC2_SEED_XPRV = "tprv8ZgxMBicQKsPe48qChGvN9oKqP6PP2Ecaso2tZ254CTd85JyX3dPfaWw3vWN4wgQeaNrX8ZfK3Zq2Q5LHvEoyfZv3mmJpyUz1caSFLya1Ca" - self.nodes[0].createwallet(wallet_name="basic2", blank=True) - basic2 = self.nodes[0].get_wallet_rpc("basic2") + basic2 = self.create_legacy_wallet("basic2", blank=True) basic2.sethdseed(True, BASIC2_SEED_WIF) assert_equal(basic2.getbalance(), 0) @@ -237,11 +238,9 @@ def test_multisig(self): # Some keys in multisig do not belong to this wallet self.log.info("Test migration of a wallet that has some keys in a multisig") - self.nodes[0].createwallet(wallet_name="multisig1") - multisig1 = self.nodes[0].get_wallet_rpc("multisig1") + multisig1 = self.create_legacy_wallet("multisig1") ms_info = multisig1.addmultisigaddress(2, [multisig1.getnewaddress(), pub1, pub2]) ms_info2 = multisig1.addmultisigaddress(2, [multisig1.getnewaddress(), pub1, pub2]) - assert_equal(multisig1.getwalletinfo()["descriptors"], False) addr1 = ms_info["address"] addr2 = ms_info2["address"] @@ -306,9 +305,7 @@ def test_other_watchonly(self): # Wallet with an imported address. Should be the same thing as the multisig test self.log.info("Test migration of a wallet with watchonly imports") - self.nodes[0].createwallet(wallet_name="imports0") - imports0 = self.nodes[0].get_wallet_rpc("imports0") - assert_equal(imports0.getwalletinfo()["descriptors"], False) + imports0 = self.create_legacy_wallet("imports0") # External address label imports0.setlabel(default.getnewaddress(), "external") @@ -375,11 +372,7 @@ def test_no_privkeys(self): # Migrating an actual watchonly wallet should not create a new watchonly wallet self.log.info("Test migration of a pure watchonly wallet") - self.nodes[0].createwallet(wallet_name="watchonly0", disable_private_keys=True) - watchonly0 = self.nodes[0].get_wallet_rpc("watchonly0") - info = watchonly0.getwalletinfo() - assert_equal(info["descriptors"], False) - assert_equal(info["private_keys_enabled"], False) + watchonly0 = self.create_legacy_wallet("watchonly0", disable_private_keys=True) addr = default.getnewaddress() desc = default.getaddressinfo(addr)["desc"] @@ -402,11 +395,7 @@ def test_no_privkeys(self): # Migrating a wallet with pubkeys added to the keypool self.log.info("Test migration of a pure watchonly wallet with pubkeys in keypool") - self.nodes[0].createwallet(wallet_name="watchonly1", disable_private_keys=True) - watchonly1 = self.nodes[0].get_wallet_rpc("watchonly1") - info = watchonly1.getwalletinfo() - assert_equal(info["descriptors"], False) - assert_equal(info["private_keys_enabled"], False) + watchonly1 = self.create_legacy_wallet("watchonly1", disable_private_keys=True) addr1 = default.getnewaddress() addr2 = default.getnewaddress() From 92d651e70f3969fe9b51f12926ca8a59cc56515b Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 9 Apr 2026 21:35:30 +0700 Subject: [PATCH 4/5] fix: follow-up backporting bitcoin#17261 - missing break --- src/wallet/wallet.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 68f00f6a8bab..22af0a0a17bd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3240,6 +3240,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri 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; } } } From 16071beef77e15899b409a1f3560629e7064b383 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 20 Jun 2023 15:41:17 -0400 Subject: [PATCH 5/5] Merge bitcoin/bitcoin#26740: wallet: Migrate wallets that are not in a wallet dir a1e653828bc59351b2a0dd5a70f519e6b61199bc test: Add test for migrating default wallet and plain file wallet (Andrew Chow) bdbe3fd76b4b9186503dc1926a2fa3f8178d00a5 wallet: Generated migrated wallet's path from walletdir and name (Andrew Chow) Pull request description: This PR fixes an assertion error that is hit during the setup of the new database during migration of a wallet that was not contained in a wallet dir. Also added a test for this case as well as one for migrating the default wallet. ACKs for top commit: ryanofsky: Code review ACK a1e653828bc59351b2a0dd5a70f519e6b61199bc furszy: ACK a1e65382 Tree-SHA512: 96b218c0de8567d8650ec96e1bf58b0f8ca4c4726f5efc6362453979b56b9d569baea0bb09befb3a5aed8d16d29bf75ed5cd8ffc432bbd4cbcad3ac5574bc479 Co-authored-by: Ryan Ofsky --- src/wallet/wallet.cpp | 7 ++++-- test/functional/wallet_migration.py | 37 +++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 22af0a0a17bd..d84831b6575b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4364,16 +4364,19 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) // Close this database and delete the file fs::path db_path = fs::PathFromString(m_database->Filename()); - fs::path db_dir = db_path.parent_path(); m_database->Close(); fs::remove(db_path); + // Generate the path for the location of the migrated wallet + // Wallets that are plain files rather than wallet directories will be migrated to be wallet directories. + const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(m_name)); + // Make new DB DatabaseOptions opts; opts.require_create = true; opts.require_format = DatabaseFormat::SQLITE; DatabaseStatus db_status; - std::unique_ptr new_db = MakeDatabase(db_dir, opts, db_status, error); + std::unique_ptr new_db = MakeDatabase(wallet_path, opts, db_status, error); assert(new_db); // This is to prevent doing anything further with this wallet. The original file was deleted, but a backup exists. m_database.reset(); m_database = std::move(new_db); diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index c657614d936b..e92404ce5008 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -6,6 +6,7 @@ import os import random +import shutil from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -540,6 +541,40 @@ def test_wallet_name_with_slashes(self): self.assert_is_sqlite(nested_name) + def test_default_wallet(self): + self.log.info("Test migration of the wallet named as the empty string") + wallet = self.create_legacy_wallet("") + + wallet.migratewallet() + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], True) + assert_equal(info["format"], "sqlite") + + def test_direct_file(self): + self.log.info("Test migration of a wallet that is not in a wallet directory") + wallet = self.create_legacy_wallet("plainfile") + wallet.unloadwallet() + + wallets_dir = os.path.join(self.nodes[0].datadir, "regtest", "wallets") + wallet_path = os.path.join(wallets_dir, "plainfile") + wallet_dat_path = os.path.join(wallet_path, "wallet.dat") + shutil.copyfile(wallet_dat_path, os.path.join(wallets_dir, "plainfile.bak")) + shutil.rmtree(wallet_path) + shutil.move(os.path.join(wallets_dir, "plainfile.bak"), wallet_path) + + self.nodes[0].loadwallet("plainfile") + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], False) + assert_equal(info["format"], "bdb") + + wallet.migratewallet() + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], True) + assert_equal(info["format"], "sqlite") + + assert os.path.isdir(wallet_path) + assert os.path.isfile(wallet_dat_path) + def test_addressbook(self): df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) @@ -679,6 +714,8 @@ def run_test(self): self.test_unloaded() self.test_unloaded_by_path() self.test_wallet_name_with_slashes() + self.test_default_wallet() + self.test_direct_file() self.test_addressbook() if __name__ == '__main__':