diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index f3fe8a19c19..65d80fffb23 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -131,6 +131,16 @@ public: /** Return path to main database filename */ std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); } + /** Return paths to all database created files */ + std::vector Files() override + { + std::vector files; + files.emplace_back(env->Directory() / m_filename); + files.emplace_back(env->Directory() / "database"); + files.emplace_back(env->Directory() / "db.log"); + files.emplace_back(env->Directory() / ".walletlock"); + return files; + } std::string Format() override { return "bdb"; } /** diff --git a/src/wallet/db.h b/src/wallet/db.h index e8790006a4d..5f13ca29ff9 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -170,6 +170,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; std::atomic nUpdateCounter; diff --git a/src/wallet/migrate.h b/src/wallet/migrate.h index 16eadeb019d..82359f9d4bb 100644 --- a/src/wallet/migrate.h +++ b/src/wallet/migrate.h @@ -65,6 +65,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/salvage.cpp b/src/wallet/salvage.cpp index b924239073c..443f80893ff 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -63,6 +63,7 @@ public: void IncrementUpdateCounter() override { ++nUpdateCounter; } void ReloadDbEnv() override {} std::string Filename() override { return "dummy"; } + std::vector Files() override { return {}; } std::string Format() override { return "dummy"; } std::unique_ptr MakeBatch(bool flush_on_close = true) override { return std::make_unique(); } }; diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index a8c9f8a8ab6..896a2fc0f33 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 78a3accf890..c78cd29afc2 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -105,7 +105,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; @@ -166,6 +166,14 @@ public: void IncrementUpdateCounter() override { ++nUpdateCounter; } 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 801dbacaf19..baf6aea9edf 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -121,6 +121,7 @@ public: void ReloadDbEnv() override {} std::string Filename() override { return "mockable"; } + std::vector Files() override { return {}; } std::string Format() override { return "mock"; } std::unique_ptr MakeBatch(bool flush_on_close = true) override { return std::make_unique(m_records, m_pass); } }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 09eda0c28e4..cefb15a3d9e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4106,6 +4106,9 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))}; } + // When the legacy wallet has no spendable scripts, it will be replaced by the watch-only/solvable wallets. + bool empty_local_wallet = 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()) { @@ -4137,9 +4140,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 (!empty_local_wallet && !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); @@ -4289,6 +4292,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 (empty_local_wallet) { + 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 } @@ -4297,7 +4308,7 @@ bool CWallet::CanGrindR() const return !IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); } -bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) +bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res, bool& empty_local_wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); @@ -4406,6 +4417,11 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, return false; } wallet.WalletLogPrintf("Wallet migration complete.\n"); + + // When the legacy wallet had no spendable scripts, the local wallet will contain no information after migration. + // In such case, we notify the caller to not add it to the result. The user is not expecting to have a spendable + // wallet. + empty_local_wallet = data->desc_spkms.empty() && !data->master_key.key.IsValid(); return true; }); } @@ -4527,6 +4543,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 a spendable wallet after migrating + // only watch-only scripts. + bool empty_local_wallet = false; + { LOCK(local_wallet->cs_wallet); // First change to using SQLite @@ -4535,7 +4559,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); if (!success) { - success = DoMigration(*local_wallet, context, error, res); + success = DoMigration(*local_wallet, context, error, res, empty_local_wallet); } else { // Make sure that descriptors flag is actually set local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -4549,20 +4573,32 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr 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; + // Reload and set the main spendable wallet only if needed + if (!empty_local_wallet) { + success = reload_wallet(local_wallet); + res.wallet = local_wallet; + res.wallet_name = wallet_name; + } else { + // At this point, the main wallet is empty after migration, meaning it has no records. + // Therefore, 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.watchonly_wallet) { // Reload watchonly wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(res.watchonly_wallet); + // When no spendable wallet is created, set the wallet name to the watch-only wallet name + if (res.wallet_name.empty()) res.wallet_name = res.watchonly_wallet->GetName(); } 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); + // When no spendable neither watch-only wallets are created, set the wallet name to the solvables-only wallet name + if (res.wallet_name.empty()) res.wallet_name = res.solvables_wallet->GetName(); } } if (!success) { diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index ce8dc19460d..e7e1dafcf39 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 @@ -114,9 +114,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 backup path exist after migration + assert os.path.exists(migrate_info['backup_path']) return migrate_info, wallet def test_basic(self): @@ -1021,6 +1026,12 @@ class WalletMigrationTest(BitcoinTestFramework): res, _ = self.migrate_and_get_rpc("bare_p2pk") wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name']) assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})')) + + # 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): @@ -1061,6 +1072,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):