diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index dd093e984a3..869f9614c56 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -468,7 +468,7 @@ void MigrateWalletActivity::migrate(const std::string& name) auto res{node().walletLoader().migrateWallet(name, passphrase)}; if (res) { - m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->wallet->getWalletName()))); + m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(name))); if (res->watchonly_wallet_name) { m_success_message += QChar(' ') + tr("Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->watchonly_wallet_name.value()))); } diff --git a/src/wallet/db.h b/src/wallet/db.h index f494a71552f..ac7ba3324e0 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -155,6 +155,9 @@ public: /** Return path to main database file for logs and error messages. */ virtual std::string Filename() = 0; + /** Return paths to all database created files */ + virtual std::vector Files() = 0; + virtual std::string Format() = 0; /** Make a DatabaseBatch connected to this database */ diff --git a/src/wallet/migrate.h b/src/wallet/migrate.h index 6f388809f0c..e72df288521 100644 --- a/src/wallet/migrate.h +++ b/src/wallet/migrate.h @@ -50,6 +50,7 @@ public: /** Return path to main database file for logs and error messages. */ std::string Filename() override { return fs::PathToString(m_filepath); } + std::vector Files() override { return {m_filepath}; } std::string Format() override { return "bdb_ro"; } diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index b902dd82c1d..993f58420cb 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -112,12 +112,12 @@ Mutex SQLiteDatabase::g_sqlite_mutex; int SQLiteDatabase::g_sqlite_count = 0; SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock) - : WalletDatabase(), m_mock(mock), m_dir_path(fs::PathToString(dir_path)), m_file_path(fs::PathToString(file_path)), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync) + : WalletDatabase(), m_mock(mock), m_dir_path(dir_path), m_file_path(fs::PathToString(file_path)), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync) { { LOCK(g_sqlite_mutex); LogPrintf("Using SQLite Version %s\n", SQLiteDatabaseVersion()); - LogPrintf("Using wallet %s\n", m_dir_path); + LogPrintf("Using wallet %s\n", fs::PathToString(m_dir_path)); if (++g_sqlite_count == 1) { // Setup logging @@ -253,7 +253,7 @@ void SQLiteDatabase::Open() if (m_db == nullptr) { if (!m_mock) { - TryCreateDirectories(fs::PathFromString(m_dir_path)); + TryCreateDirectories(m_dir_path); } int ret = sqlite3_open_v2(m_file_path.c_str(), &m_db, flags, nullptr); if (ret != SQLITE_OK) { diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index 14ad38792c4..895cfb12dd4 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -104,7 +104,7 @@ class SQLiteDatabase : public WalletDatabase private: const bool m_mock{false}; - const std::string m_dir_path; + const fs::path m_dir_path; const std::string m_file_path; @@ -147,6 +147,14 @@ public: bool Backup(const std::string& dest) const override; std::string Filename() override { return m_file_path; } + /** Return paths to all database created files */ + std::vector Files() override + { + std::vector files; + files.emplace_back(m_dir_path / fs::PathFromString(m_file_path)); + files.emplace_back(m_dir_path / fs::PathFromString(m_file_path + "-journal")); + return files; + } std::string Format() override { return "sqlite"; } /** Make a SQLiteBatch connected to this database */ diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index 7a19806731b..4c84a1713f2 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -109,6 +109,7 @@ public: void Close() override {} std::string Filename() override { return "mockable"; } + std::vector Files() override { return {}; } std::string Format() override { return "mock"; } std::unique_ptr MakeBatch() override { return std::make_unique(m_records, m_pass); } }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 36da4396106..3f2d1a38f00 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3846,6 +3846,9 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))}; } + // Note: when the legacy wallet has no spendable scripts, it must be empty at the end of the process. + bool has_spendable_material = !data.desc_spkms.empty() || data.master_key.key.IsValid(); + // Get all invalid or non-watched scripts that will not be migrated std::set not_migrated_dests; for (const auto& script : legacy_spkm->GetNotMineScriptPubKeys()) { @@ -3877,9 +3880,9 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, m_external_spk_managers.clear(); m_internal_spk_managers.clear(); - // Setup new descriptors + // Setup new descriptors (only if we are migrating any key material) SetWalletFlagWithDB(local_wallet_batch, WALLET_FLAG_DESCRIPTORS); - if (!IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + if (has_spendable_material && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { // Use the existing master key if we have it if (data.master_key.key.IsValid()) { SetupDescriptorScriptPubKeyMans(local_wallet_batch, data.master_key); @@ -4033,6 +4036,14 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, } } + // If there was no key material in the main wallet, there should be no records on it anymore. + // This wallet will be discarded at the end of the process. Only wallets that contain the + // migrated records will be presented to the user. + if (!has_spendable_material) { + if (!m_address_book.empty()) return util::Error{_("Error: Not all address book records were migrated")}; + if (!mapWallet.empty()) return util::Error{_("Error: Not all transaction records were migrated")}; + } + return {}; // all good } @@ -4270,6 +4281,14 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } } + // Indicates whether the current wallet is empty after migration. + // Notes: + // When non-empty: the local wallet becomes the main spendable wallet. + // When empty: The local wallet is excluded from the result, as the + // user does not expect an empty spendable wallet after + // migrating only watch-only scripts. + bool empty_local_wallet = false; + { LOCK(local_wallet->cs_wallet); // First change to using SQLite @@ -4278,6 +4297,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Do the migration of keys and scripts for non-empty wallets, and cleanup if it fails if (HasLegacyRecords(*local_wallet)) { success = DoMigration(*local_wallet, context, error, res); + // No scripts mean empty wallet after migration + empty_local_wallet = local_wallet->GetAllScriptPubKeyMans().empty(); } else { // Make sure that descriptors flag is actually set local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -4291,21 +4312,31 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // fails to reload. std::set wallet_dirs; if (success) { - // Migration successful, unload all wallets locally, then reload them. - // Reload the main wallet - wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path()); - success = reload_wallet(local_wallet); - res.wallet = local_wallet; - res.wallet_name = wallet_name; - if (success && res.watchonly_wallet) { - // Reload watchonly - wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path()); - success = reload_wallet(res.watchonly_wallet); + Assume(!res.wallet); // We will set it here. + // Check if the local wallet is empty after migration + if (empty_local_wallet) { + // This wallet has no records. We can safely remove it. + std::vector paths_to_remove = local_wallet->GetDatabase().Files(); + local_wallet.reset(); + for (const auto& path_to_remove : paths_to_remove) fs::remove_all(path_to_remove); } - if (success && res.solvables_wallet) { - // Reload solvables - wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path()); - success = reload_wallet(res.solvables_wallet); + + // Migration successful, unload all wallets locally, then reload them. + // Note: We use a pointer to the shared_ptr to avoid increasing its reference count, + // as 'reload_wallet' expects to be the sole owner (use_count == 1). + for (std::shared_ptr* wallet_ptr : {&local_wallet, &res.watchonly_wallet, &res.solvables_wallet}) { + if (success && *wallet_ptr) { + std::shared_ptr& wallet = *wallet_ptr; + // Save db path and reload wallet + wallet_dirs.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path()); + success = reload_wallet(wallet); + + // When no wallet is set, set the main wallet. + if (!res.wallet) { + res.wallet_name = wallet->GetName(); + res.wallet = std::move(wallet); + } + } } } if (!success) { diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index b32862b0f77..cd43ecb9596 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -3,7 +3,7 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test Migrating a wallet from legacy to descriptor.""" - +import os.path import random import shutil import struct @@ -121,9 +121,14 @@ class WalletMigrationTest(BitcoinTestFramework): # Migrate, checking that rescan does not occur with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]): migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs) + # Update wallet name in case the initial wallet was completely migrated to a watch-only wallet + # (in which case the wallet name would be suffixed by the 'watchonly' term) + wallet_name = migrate_info['wallet_name'] wallet = self.master_node.get_wallet_rpc(wallet_name) assert_equal(wallet.getwalletinfo()["descriptors"], True) self.assert_is_sqlite(wallet_name) + # Always verify the backup path exist after migration + assert os.path.exists(migrate_info['backup_path']) return migrate_info, wallet def test_basic(self): @@ -1024,8 +1029,15 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.importaddress(address=p2pk_script.hex()) # Migrate wallet in the latest node res, _ = self.migrate_and_get_rpc("bare_p2pk") - wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name']) + wo_wallet = self.master_node.get_wallet_rpc(res['wallet_name']) assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})')) + assert_equal(wo_wallet.getwalletinfo()["private_keys_enabled"], False) + + # Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet. + assert_equal('bare_p2pk_watchonly', res['wallet_name']) + assert "bare_p2pk" not in self.master_node.listwallets() + assert "bare_p2pk" not in [w["name"] for w in self.master_node.listwalletdir()["wallets"]] + wo_wallet.unloadwallet() def test_manual_keys_import(self): @@ -1055,7 +1067,9 @@ class WalletMigrationTest(BitcoinTestFramework): res, _ = self.migrate_and_get_rpc("import_pubkeys") # Same as before, there should be descriptors in the watch-only wallet for the imported pubkey - wo_wallet = self.nodes[0].get_wallet_rpc(res['watchonly_name']) + wo_wallet = self.nodes[0].get_wallet_rpc(res['wallet_name']) + # Assert this is a watch-only wallet + assert_equal(wo_wallet.getwalletinfo()["private_keys_enabled"], False) # As we imported the pubkey only, there will be no key origin in the following descriptors pk_desc = descsum_create(f'pk({pubkey_hex})') pkh_desc = descsum_create(f'pkh({pubkey_hex})') @@ -1066,6 +1080,10 @@ class WalletMigrationTest(BitcoinTestFramework): # Verify all expected descriptors were migrated migrated_desc = [item['desc'] for item in wo_wallet.listdescriptors()['descriptors']] assert_equal(expected_descs, migrated_desc) + # Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet. + assert_equal('import_pubkeys_watchonly', res['wallet_name']) + assert "import_pubkeys" not in self.master_node.listwallets() + assert "import_pubkeys" not in [w["name"] for w in self.master_node.listwalletdir()["wallets"]] wo_wallet.unloadwallet() def test_p2wsh(self):