From abaf1e37a79bdf7481cef1cd5ae5e102fdce09be Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 11 Dec 2024 13:05:21 -0500 Subject: [PATCH 01/11] refactor: remove sqlite dir path back-and-forth conversion Github-Pull: bitcoin/bitcoin#31423 Rebased-From: d04f6a97ba9a55aa9455e1a805feeed4d630f59a --- src/wallet/sqlite.cpp | 6 +++--- src/wallet/sqlite.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index a8c9f8a8ab..896a2fc0f3 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 78a3accf89..eb3c0217f5 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; From 01c04d32aa3e1c323b304b1c6a573dd933b8b598 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 11 Dec 2024 13:10:01 -0500 Subject: [PATCH 02/11] wallet: introduce method to return all db created files Github-Pull: bitcoin/bitcoin#31423 Rebased-From: 1de423e0a08bbc63eed36c8772e9ef8b48e80fb8 --- src/wallet/bdb.h | 15 +++++++++++++++ src/wallet/db.h | 3 +++ src/wallet/migrate.h | 1 + src/wallet/salvage.cpp | 1 + src/wallet/sqlite.h | 8 ++++++++ src/wallet/test/util.h | 1 + 6 files changed, 29 insertions(+) diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index f3fe8a19c1..ec773fd177 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -132,6 +132,21 @@ public: /** Return path to main database filename */ std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); } + std::vector Files() override + { + std::vector files; + files.emplace_back(env->Directory() / m_filename); + if (env->m_databases.size() == 1) { + files.emplace_back(env->Directory() / "db.log"); + files.emplace_back(env->Directory() / ".walletlock"); + files.emplace_back(env->Directory() / "database" / "log.0000000001"); + files.emplace_back(env->Directory() / "database"); + // Note that this list is not exhaustive as BDB may create more log files, and possibly other ones too + // However it should be good enough for the only calls to Files() + } + return files; + } + std::string Format() override { return "bdb"; } /** * Pointer to shared database environment. diff --git a/src/wallet/db.h b/src/wallet/db.h index e8790006a4..5f13ca29ff 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 16eadeb019..82359f9d4b 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 b924239073..443f80893f 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.h b/src/wallet/sqlite.h index eb3c0217f5..c78cd29afc 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -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 b055c6c693..59e3a9c75f 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -123,6 +123,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); } }; From cc324aa2bed30afa713625dfb9cf83c438dd15c1 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 6 Jan 2026 16:09:38 -0800 Subject: [PATCH 03/11] wallettool: do not use fs::remove_all in createfromdump cleanup Github-Pull: bitcoin/bitcoin#34215 Rebased-From: f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42 --- src/wallet/dump.cpp | 8 +++++++- test/functional/tool_wallet.py | 12 ++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index db2756e0ca..20aa5d453e 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -288,11 +288,17 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: dump_file.close(); } + // On failure, gather the paths to remove + std::vector paths_to_remove = wallet->GetDatabase().Files(); + if (!name.empty()) paths_to_remove.push_back(wallet_path); + wallet.reset(); // The pointer deleter will close the wallet for us. // Remove the wallet dir if we have a failure if (!ret) { - fs::remove_all(wallet_path); + for (const auto& p : paths_to_remove) { + fs::remove(p); + } } return ret; diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index c7abc2da8d..979804a5fe 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -409,6 +409,18 @@ class ToolWalletTest(BitcoinTestFramework): self.write_dump(dump_data, bad_sum_wallet_dump) self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump') assert not (self.nodes[0].wallets_path / "badload").is_dir() + if not self.options.descriptors: + os.rename(self.nodes[0].wallets_path / "wallet.dat", self.nodes[0].wallets_path / "default.wallet.dat") + self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump') + assert self.nodes[0].wallets_path.exists() + assert not (self.nodes[0].wallets_path / "wallet.dat").exists() + + self.log.info('Checking createfromdump with an unnamed wallet') + self.do_tool_createfromdump("", "wallet.dump") + assert (self.nodes[0].wallets_path / "wallet.dat").exists() + os.unlink(self.nodes[0].wallets_path / "wallet.dat") + if not self.options.descriptors: + os.rename(self.nodes[0].wallets_path / "default.wallet.dat", self.nodes[0].wallets_path / "wallet.dat") def test_chainless_conflicts(self): self.log.info("Test wallet tool when wallet contains conflicting transactions") From d91f56e1e3f1aee99b0c09c23db70622ad6ed1b4 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 5 Jan 2026 18:12:40 -0500 Subject: [PATCH 04/11] wallet: RestoreWallet failure, erase only what was created Track what RestoreWallet creates so only those files and directories are removed during a failure and nothing else. Preexisting paths must be left untouched. Note: Using fs::remove_all() instead of fs::remove() in RestoreWallet does not cause any problems currently, but the change is necessary for the next commit which extends RestoreWallet to work with existing directories, which may contain files that must not be deleted. Github-Pull: bitcoin/bitcoin#34156 Rebased-From: 4ed0693a3f2a427ef9e7ad016930ec29fa244995 --- src/wallet/wallet.cpp | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 09eda0c28e..32c902b768 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -501,6 +501,8 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name)); auto wallet_file = wallet_path / "wallet.dat"; std::shared_ptr wallet; + bool wallet_file_copied = false; + bool created_parent_dir = false; try { if (!fs::exists(backup_file)) { @@ -509,13 +511,22 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b return nullptr; } - if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) { + if (fs::exists(wallet_path)) { error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path))); status = DatabaseStatus::FAILED_ALREADY_EXISTS; return nullptr; + } else { + // The directory doesn't exist, create it + if (!TryCreateDirectories(wallet_path)) { + error = Untranslated(strprintf("Failed to restore database path '%s'.", fs::PathToString(wallet_path))); + status = DatabaseStatus::FAILED_ALREADY_EXISTS; + return nullptr; + } + created_parent_dir = true; } fs::copy_file(backup_file, wallet_file, fs::copy_options::none); + wallet_file_copied = true; if (load_after_restore) { wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); @@ -528,7 +539,13 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b // Remove created wallet path only when loading fails if (load_after_restore && !wallet) { - fs::remove_all(wallet_path); + if (wallet_file_copied) fs::remove(wallet_file); + // Clean up the parent directory if we created it during restoration. + // As we have created it, it must be empty after deleting the wallet file. + if (created_parent_dir) { + Assume(fs::is_empty(wallet_path)); + fs::remove(wallet_path); + } } return wallet; From a074d36254ab0c666f0438fe071cd213715f82de Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 26 Dec 2025 20:22:55 -0500 Subject: [PATCH 05/11] wallet: fix unnamed wallet migration failure When migrating any legacy unnamed wallet, a failed migration would cause the cleanup logic to remove its parent directory. Since this type of legacy wallet lives directly in the main '/wallets/' folder, this resulted in unintentionally erasing all wallets, including the backup file. To be fully safe, we will no longer call `fs::remove_all`. Instead, we only erase the individual db files we have created, leaving everything else intact. The created wallets parent directories are erased only if they are empty. As part of this last change, `RestoreWallet` was modified to allow an existing directory as the destination, since we no longer remove the original wallet directory (we only remove the files we created inside it). This also fixes the restore of top-level default wallets during failures, which were failing due to the directory existence check that always returns true for the /wallets/ directory. This bug started after: https://github.com/bitcoin/bitcoin/commit/f6ee59b6e2995a3916fb4f0d4cbe15ece2054494 Previously, the `fs::copy_file` call was failing for top-level wallets, which prevented the `fs::remove_all` call from being reached. Github-Pull: bitcoin/bitcoin#34156 Rebased-From: f4c7e28e80bf9af50b03a770b641fd309a801589 --- src/wallet/wallet.cpp | 72 +++++++++++++++++++++++--------- test/functional/wallet_backup.py | 2 +- 2 files changed, 54 insertions(+), 20 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 32c902b768..119a99d601 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -511,10 +511,22 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b return nullptr; } + // Wallet directories are allowed to exist, but must not contain a .dat file. + // Any existing wallet database is treated as a hard failure to prevent overwriting. if (fs::exists(wallet_path)) { - error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path))); - status = DatabaseStatus::FAILED_ALREADY_EXISTS; - return nullptr; + // If this is a file, it is the db and we don't want to overwrite it. + if (!fs::is_directory(wallet_path)) { + error = Untranslated(strprintf("Failed to restore wallet. Database file exists '%s'.", fs::PathToString(wallet_path))); + status = DatabaseStatus::FAILED_ALREADY_EXISTS; + return nullptr; + } + + // Check we are not going to overwrite an existing db file + if (fs::exists(wallet_file)) { + error = Untranslated(strprintf("Failed to restore wallet. Database file exists in '%s'.", fs::PathToString(wallet_file))); + status = DatabaseStatus::FAILED_ALREADY_EXISTS; + return nullptr; + } } else { // The directory doesn't exist, create it if (!TryCreateDirectories(wallet_path)) { @@ -4559,26 +4571,43 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } } - // In case of reloading failure, we need to remember the wallet dirs to remove - // Set is used as it may be populated with the same wallet directory paths multiple times, - // both before and after reloading. This ensures the set is complete even if one of the wallets - // fails to reload. - std::set wallet_dirs; + // In case of loading failure, we need to remember the wallet files we have created to remove. + // A `set` is used as it may be populated with the same wallet directory paths multiple times, + // both before and after loading. This ensures the set is complete even if one of the wallets + // fails to load. + std::set wallet_files_to_remove; + std::set wallet_empty_dirs_to_remove; + + // Helper to track wallet files and directories for cleanup on failure. + // Only directories of wallets created during migration (not the main wallet) are tracked. + auto track_for_cleanup = [&](const CWallet& wallet) { + const auto files = wallet.GetDatabase().Files(); + wallet_files_to_remove.insert(files.begin(), files.end()); + if (wallet.GetName() != wallet_name) { + // If this isn’t the main wallet, mark its directory for removal. + // This applies to the watch-only and solvable wallets. + // Wallets stored directly as files in the top-level directory + // (e.g. default unnamed wallets) don’t have a removable parent directory. + wallet_empty_dirs_to_remove.insert(fs::PathFromString(wallet.GetDatabase().Filename()).parent_path()); + } + }; + + 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()); + track_for_cleanup(*local_wallet); 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()); + track_for_cleanup(*res.watchonly_wallet); success = reload_wallet(res.watchonly_wallet); } if (success && res.solvables_wallet) { // Reload solvables - wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path()); + track_for_cleanup(*res.solvables_wallet); success = reload_wallet(res.solvables_wallet); } } @@ -4586,7 +4615,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Migration failed, cleanup // Before deleting the wallet's directory, copy the backup file to the top-level wallets dir fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename); - fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none); + fs::rename(backup_path, temp_backup_location); // Make list of wallets to cleanup std::vector> created_wallets; @@ -4595,8 +4624,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet)); // Get the directories to remove after unloading - for (std::shared_ptr& w : created_wallets) { - wallet_dirs.emplace(fs::PathFromString(w->GetDatabase().Filename()).parent_path()); + for (std::shared_ptr& wallet : created_wallets) { + track_for_cleanup(*wallet); } // Unload the wallets @@ -4615,9 +4644,15 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } } - // Delete the wallet directories - for (const fs::path& dir : wallet_dirs) { - fs::remove_all(dir); + // First, delete the db files we have created throughout this process and nothing else + for (const fs::path& file : wallet_files_to_remove) { + fs::remove(file); + } + + // Second, delete the created wallet directories and nothing else. They must be empty at this point. + for (const fs::path& dir : wallet_empty_dirs_to_remove) { + Assume(fs::is_empty(dir)); + fs::remove(dir); } // Restore the backup @@ -4631,8 +4666,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir - fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none); - fs::remove(temp_backup_location); + fs::rename(temp_backup_location, backup_path); // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null. // This check is performed after restoration to avoid an early error before saving the backup. diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 7c88f64dcf..7ad83bdf87 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -136,7 +136,7 @@ class WalletBackupTest(BitcoinTestFramework): backup_file = self.nodes[0].datadir_path / 'wallet.bak' wallet_name = "res0" wallet_file = node.wallets_path / wallet_name - error_message = "Failed to create database path '{}'. Database already exists.".format(wallet_file) + error_message = "Failed to restore wallet. Database file exists in '{}'.".format(wallet_file / "wallet.dat") assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file) assert wallet_file.exists() From 833848e9b8eab430629da116f753f8d4433f51e2 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 26 Dec 2025 20:23:02 -0500 Subject: [PATCH 06/11] test: add coverage for unnamed wallet migration failure Verifies that a failed migration of the unnamed (default) wallet does not erase the main /wallets/ directory, and also that the backup file exists. Github-Pull: bitcoin/bitcoin#34156 Rebased-From: 36093bde63286e19821a9e62cdff1712b6245dc7 --- test/functional/wallet_migration.py | 35 +++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index ce8dc19460..3ca053043b 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -4,6 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test Migrating a wallet from legacy to descriptor.""" +import os import random import shutil import struct @@ -548,6 +549,39 @@ class WalletMigrationTest(BitcoinTestFramework): self.master_node.setmocktime(0) + def test_default_wallet_failure(self): + self.log.info("Test failure during unnamed (default) wallet migration") + master_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) + wallet = self.create_legacy_wallet("", blank=True) + wallet.importaddress(master_wallet.getnewaddress(address_type="legacy")) + + # Create wallet directory with the watch-only name and a wallet file. + # Because the wallet dir exists, this will cause migration to fail. + watch_only_dir = self.master_node.wallets_path / "_watchonly" + os.mkdir(watch_only_dir) + shutil.copyfile(self.old_node.wallets_path / "wallet.dat", watch_only_dir / "wallet.dat") + + mocked_time = int(time.time()) + self.master_node.setmocktime(mocked_time) + assert_raises_rpc_error(-4, "Failed to create database", self.migrate_and_get_rpc, "") + self.master_node.setmocktime(0) + + # Verify the /wallets/ path exists + assert self.master_node.wallets_path.exists() + # Check backup file exists. Because the wallet has no name, the backup is prefixed with 'default_wallet' + backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak" + assert backup_path.exists() + # Verify the original unnamed wallet was restored + assert (self.master_node.wallets_path / "wallet.dat").exists() + # And verify it is still a BDB wallet + with open(self.master_node.wallets_path / "wallet.dat", "rb") as f: + data = f.read(16) + _, _, magic = struct.unpack("QII", data) + assert_equal(magic, BTREE_MAGIC) + + # Test cleanup: clear default wallet for next test + os.remove(self.old_node.wallets_path / "wallet.dat") + 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") @@ -1372,6 +1406,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_encrypted() self.test_nonexistent() self.test_unloaded_by_path() + self.test_default_wallet_failure() self.test_default_wallet() self.test_direct_file() self.test_addressbook() From 9ea84c08d7e24ffefa6f18d6bd2af28ec38cfd98 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 5 Jan 2026 16:08:13 -0500 Subject: [PATCH 07/11] test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure The first test verifies that restoring into an existing empty directory or a directory with no .dat db files succeeds, while restoring into a dir with a .dat file fails. The second test covers restoring into the default unnamed wallet (wallet.dat), which also implicitly exercises the recovery path used after a failed migration. The third test covers failure during restore on a prune node. When the wallet last sync was beyond the pruning height. Github-Pull: bitcoin/bitcoin#34156 Rebased-From: f011e0f0680a8c39988ae57dae57eb86e92dd449 --- test/functional/wallet_backup.py | 77 ++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 7ad83bdf87..3ff798d3d3 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -40,6 +40,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_raises_rpc_error, + sha256sum_file, ) @@ -140,6 +141,67 @@ class WalletBackupTest(BitcoinTestFramework): assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file) assert wallet_file.exists() + def test_restore_existent_dir(self): + self.log.info("Test restore on an existent empty directory") + node = self.nodes[3] + backup_file = self.nodes[0].datadir_path / 'wallet.bak' + wallet_name = "restored_wallet" + wallet_dir = node.wallets_path / wallet_name + os.mkdir(wallet_dir) + res = node.restorewallet(wallet_name, backup_file) + assert_equal(res['name'], wallet_name) + node.unloadwallet(wallet_name) + + self.log.info("Test restore succeeds when the target directory contains non-wallet files") + wallet_file = node.wallets_path / wallet_name / "wallet.dat" + os.remove(wallet_file) + extra_file = node.wallets_path / wallet_name / "not_a_wallet.txt" + extra_file.touch() + res = node.restorewallet(wallet_name, backup_file) + assert_equal(res['name'], wallet_name) + assert extra_file.exists() # extra file was not removed by mistake + node.unloadwallet(wallet_name) + + self.log.info("Test restore failure due to existing db file in the destination directory") + original_shasum = sha256sum_file(wallet_file) + error_message = "Failed to restore wallet. Database file exists in '{}'.".format(wallet_dir / "wallet.dat") + assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file) + # Ensure the wallet file remains untouched + assert wallet_dir.exists() + assert_equal(original_shasum, sha256sum_file(wallet_file)) + + self.log.info("Test restore succeeds when the .dat file in the destination has a different name") + second_wallet = wallet_dir / "hidden_storage.dat" + os.rename(wallet_dir / "wallet.dat", second_wallet) + original_shasum = sha256sum_file(second_wallet) + res = node.restorewallet(wallet_name, backup_file) + assert_equal(res['name'], wallet_name) + assert (wallet_dir / "hidden_storage.dat").exists() + assert_equal(original_shasum, sha256sum_file(second_wallet)) + node.unloadwallet(wallet_name) + + # Clean for follow-up tests + os.remove(wallet_file) + + def test_restore_into_unnamed_wallet(self): + self.log.info("Test restore into a default unnamed wallet") + # This is also useful to test the migration recovery after failure logic + node = self.nodes[3] + if not self.options.descriptors: + node.unloadwallet("") + os.rename(node.wallets_path / "wallet.dat", node.wallets_path / "default.wallet.dat") + backup_file = self.nodes[0].datadir_path / 'wallet.bak' + wallet_name = "" + res = node.restorewallet(wallet_name, backup_file) + assert_equal(res['name'], "") + assert (node.wallets_path / "wallet.dat").exists() + # Clean for follow-up tests + node.unloadwallet("") + os.remove(node.wallets_path / "wallet.dat") + if not self.options.descriptors: + os.rename(node.wallets_path / "default.wallet.dat", node.wallets_path / "wallet.dat") + node.loadwallet("") + def test_pruned_wallet_backup(self): self.log.info("Test loading backup on a pruned node when the backup was created close to the prune height of the restoring node") node = self.nodes[3] @@ -159,6 +221,19 @@ class WalletBackupTest(BitcoinTestFramework): # the backup to load successfully this close to the prune height node.restorewallet('pruned', node.datadir_path / 'wallet_pruned.bak') + self.log.info("Test restore on a pruned node when the backup was beyond the pruning point") + if not self.options.descriptors: + node.unloadwallet("") + os.rename(node.wallets_path / "wallet.dat", node.wallets_path / "default.wallet.dat") + backup_file = self.nodes[0].datadir_path / 'wallet.bak' + wallet_name = "" + error_message = "Wallet loading failed. Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)" + assert_raises_rpc_error(-4, error_message, node.restorewallet, wallet_name, backup_file) + assert node.wallets_path.exists() # ensure the wallets dir exists + if not self.options.descriptors: + os.rename(node.wallets_path / "default.wallet.dat", node.wallets_path / "wallet.dat") + node.loadwallet("") + def run_test(self): self.log.info("Generating initial blockchain") self.generate(self.nodes[0], 1) @@ -227,6 +302,8 @@ class WalletBackupTest(BitcoinTestFramework): assert_equal(res2_rpc.getbalance(), balance2) self.restore_wallet_existent_name() + self.test_restore_existent_dir() + self.test_restore_into_unnamed_wallet() if not self.options.descriptors: self.log.info("Restoring using dumped wallet") From a7e2d106db8f193259420bacbccec80ba3beebf1 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 27 Dec 2025 13:54:59 -0500 Subject: [PATCH 08/11] wallet: improve post-migration logging Right now, after migration the last message users see is "migration completed", but the migration isn't actually finished yet. We still need to load the new wallets to ensure consistency, and if that fails, the migration will be rolled back. This can be confusing for users. This change logs the post-migration loading step and if a wallet fails to load and the migration will be rolled back. Github-Pull: bitcoin/bitcoin#34156 Rebased-From: d70b159c42008ac3b63d1c43d99d4f1316d2f1ef --- src/wallet/wallet.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 119a99d601..b54cf0be0f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4515,7 +4515,12 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr std::string name = to_reload->GetName(); to_reload.reset(); to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); - return to_reload != nullptr; + if (!to_reload) { + LogError("Failed to load wallet '%s' after migration. Rolling back migration to preserve consistency. " + "Error cause: %s\n", wallet_name, error.original); + return false; + } + return true; }; // Before anything else, check if there is something to migrate. @@ -4596,6 +4601,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr if (success) { // Migration successful, unload all wallets locally, then reload them. // Reload the main wallet + LogInfo("Loading new wallets after migration...\n"); track_for_cleanup(*local_wallet); success = reload_wallet(local_wallet); res.wallet = local_wallet; From 5e8ad98163af9749e7a3c44a9107cc241c5bd7ab Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 27 Dec 2025 14:32:11 -0500 Subject: [PATCH 09/11] wallet: migration, fix watch-only and solvables wallets names Because the default wallet has no name, the watch-only and solvables wallets created during migration end up having no name either. This fixes it by applying the same prefix name we use for the backup file for an unnamed default wallet. Before: watch-only wallet named "_watchonly" After: watch-only wallet named "default_wallet_watchonly" Github-Pull: bitcoin/bitcoin#34156 Rebased-From: 82caa8193a3e36f248dcc949e0cd41def191efac --- src/wallet/wallet.cpp | 15 ++++++++-- test/functional/wallet_migration.py | 45 +++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b54cf0be0f..2397d84a6f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4326,6 +4326,15 @@ bool CWallet::CanGrindR() const return !IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); } +// Returns wallet prefix for migration. +// Used to name the backup file and newly created wallets. +// E.g. a watch-only wallet is named "_watchonly". +static std::string MigrationPrefixName(CWallet& wallet) +{ + const std::string& name{wallet.GetName()}; + return name.empty() ? "default_wallet" : name; +} + bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); @@ -4357,7 +4366,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector warnings; - std::string wallet_name = wallet.GetName() + "_watchonly"; + std::string wallet_name = MigrationPrefixName(wallet) + "_watchonly"; std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); if (!database) { error = strprintf(_("Wallet file creation failed: %s"), error); @@ -4394,7 +4403,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector warnings; - std::string wallet_name = wallet.GetName() + "_solvables"; + std::string wallet_name = MigrationPrefixName(wallet) + "_solvables"; std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); if (!database) { error = strprintf(_("Wallet file creation failed: %s"), error); @@ -4533,7 +4542,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Make a backup of the DB fs::path this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path(); - fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime())); + fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", MigrationPrefixName(*local_wallet), GetTime())); fs::path backup_path = this_wallet_dir / backup_filename; if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) { if (was_loaded) { diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 3ca053043b..c11986226d 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -5,6 +5,7 @@ """Test Migrating a wallet from legacy to descriptor.""" import os +from pathlib import Path import random import shutil import struct @@ -25,6 +26,7 @@ from test_framework.script import hash160 from test_framework.script_util import key_to_p2pkh_script, key_to_p2pk_script, script_to_p2sh_script, script_to_p2wsh_script from test_framework.util import ( assert_equal, + assert_greater_than, assert_raises_rpc_error, find_vout_for_address, sha256sum_file, @@ -523,6 +525,14 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(bals, wallet.getbalances()) + def clear_default_wallet(self, backup_file): + # Test cleanup: Clear unnamed default wallet for subsequent tests + (self.old_node.wallets_path / "wallet.dat").unlink() + (self.master_node.wallets_path / "wallet.dat").unlink(missing_ok=True) + shutil.rmtree(self.master_node.wallets_path / "default_wallet_watchonly", ignore_errors=True) + shutil.rmtree(self.master_node.wallets_path / "default_wallet_solvables", ignore_errors=True) + backup_file.unlink() + def test_default_wallet(self): self.log.info("Test migration of the wallet named as the empty string") wallet = self.create_legacy_wallet("") @@ -549,6 +559,36 @@ class WalletMigrationTest(BitcoinTestFramework): self.master_node.setmocktime(0) + wallet.unloadwallet() + self.clear_default_wallet(backup_file=Path(res["backup_path"])) + + def test_default_wallet_watch_only(self): + self.log.info("Test unnamed (default) watch-only wallet migration") + master_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) + wallet = self.create_legacy_wallet("", blank=True) + wallet.importaddress(master_wallet.getnewaddress(address_type="legacy")) + + res, def_wallet = self.migrate_and_get_rpc("") + wallet = self.master_node.get_wallet_rpc("default_wallet_watchonly") + + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], True) + assert_equal(info["format"], "sqlite") + assert_equal(info["private_keys_enabled"], False) + assert_equal(info["walletname"], "default_wallet_watchonly") + + # The default wallet will still exist and have newly generated descriptors + assert (self.master_node.wallets_path / "wallet.dat").exists() + def_wallet_info = def_wallet.getwalletinfo() + assert_equal(def_wallet_info["descriptors"], True) + assert_equal(def_wallet_info["format"], "sqlite") + assert_equal(def_wallet_info["private_keys_enabled"], True) + assert_equal(def_wallet_info["walletname"], "") + assert_greater_than(def_wallet_info["keypoolsize"], 0) + + wallet.unloadwallet() + self.clear_default_wallet(backup_file=Path(res["backup_path"])) + def test_default_wallet_failure(self): self.log.info("Test failure during unnamed (default) wallet migration") master_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) @@ -557,7 +597,7 @@ class WalletMigrationTest(BitcoinTestFramework): # Create wallet directory with the watch-only name and a wallet file. # Because the wallet dir exists, this will cause migration to fail. - watch_only_dir = self.master_node.wallets_path / "_watchonly" + watch_only_dir = self.master_node.wallets_path / "default_wallet_watchonly" os.mkdir(watch_only_dir) shutil.copyfile(self.old_node.wallets_path / "wallet.dat", watch_only_dir / "wallet.dat") @@ -580,7 +620,7 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(magic, BTREE_MAGIC) # Test cleanup: clear default wallet for next test - os.remove(self.old_node.wallets_path / "wallet.dat") + self.clear_default_wallet(backup_path) def test_direct_file(self): self.log.info("Test migration of a wallet that is not in a wallet directory") @@ -1408,6 +1448,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_unloaded_by_path() self.test_default_wallet_failure() self.test_default_wallet() + self.test_default_wallet_watch_only() self.test_direct_file() self.test_addressbook() self.test_migrate_raw_p2sh() From 9405e915e79d86d262779ea38104624d37add2a3 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 4 Jan 2026 12:25:21 -0500 Subject: [PATCH 10/11] test: coverage for migration failure when last sync is beyond prune height Github-Pull: bitcoin/bitcoin#34156 Rebased-From: b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb --- test/functional/wallet_migration.py | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index c11986226d..5424fda2ab 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -1431,6 +1431,42 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(addr_info["solvable"], True) assert "hex" in addr_info + def unsynced_wallet_on_pruned_node_fails(self): + self.log.info("Test migration of an unsynced wallet on a pruned node fails gracefully") + wallet = self.create_legacy_wallet("", load_on_startup=False) + last_wallet_synced_block = wallet.getwalletinfo()['lastprocessedblock']['height'] + wallet.unloadwallet() + + shutil.copyfile(self.old_node.wallets_path / "wallet.dat", self.master_node.wallets_path / "wallet.dat") + + # Generate blocks just so the wallet best block is pruned + self.restart_node(0, ["-fastprune", "-prune=1", "-nowallet"]) + self.connect_nodes(0, 1) + self.generate(self.master_node, 450, sync_fun=self.no_op) + self.master_node.pruneblockchain(250) + # Ensure next block to sync is unavailable + assert_raises_rpc_error(-1, "Block not available (pruned data)", self.master_node.getblock, self.master_node.getblockhash(last_wallet_synced_block + 1)) + + # Check migration failure + mocked_time = int(time.time()) + self.master_node.setmocktime(mocked_time) + assert_raises_rpc_error(-4, "last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)", self.master_node.migratewallet, wallet_name="") + self.master_node.setmocktime(0) + + # Verify the /wallets/ path exists, the wallet is still BDB and the backup file is there. + assert self.master_node.wallets_path.exists() + + with open(self.master_node.wallets_path / "wallet.dat", "rb") as f: + data = f.read(16) + _, _, magic = struct.unpack("QII", data) + assert_equal(magic, BTREE_MAGIC) + + backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak" + assert backup_path.exists() + + self.clear_default_wallet(backup_path) + + def run_test(self): self.master_node = self.nodes[0] self.old_node = self.nodes[1] @@ -1466,5 +1502,8 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_taproot() self.test_solvable_no_privs() + # Note: After this test the first 250 blocks of 'master_node' are pruned + self.unsynced_wallet_on_pruned_node_fails() + if __name__ == '__main__': WalletMigrationTest(__file__).main() From 76cdeb7b06232050c7d20ffa1395697cc4e53295 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 7 Jan 2026 16:02:58 -0800 Subject: [PATCH 11/11] wallet: test: Failed migration cleanup Refactor a common way to perform the failed migration test that exists for default wallets, and add relative-path wallets and absolute-path wallets. Github-Pull: 34226 Rebased-From: eeaf28dbe0e09819ab0e95bb7762b29536bdeef6 --- test/functional/wallet_migration.py | 79 ++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 18 deletions(-) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 5424fda2ab..8129baf438 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -589,38 +589,72 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.unloadwallet() self.clear_default_wallet(backup_file=Path(res["backup_path"])) - def test_default_wallet_failure(self): - self.log.info("Test failure during unnamed (default) wallet migration") + def test_migration_failure(self, wallet_name): + is_default = wallet_name == "" + wallet_pretty_name = "unnamed (default)" if is_default else f'"{wallet_name}"' + self.log.info(f"Test failure during migration of wallet named: {wallet_pretty_name}") + # Preface, set up legacy wallet and unload it master_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) - wallet = self.create_legacy_wallet("", blank=True) + wallet = self.create_legacy_wallet(wallet_name, blank=True) wallet.importaddress(master_wallet.getnewaddress(address_type="legacy")) + wallet.unloadwallet() - # Create wallet directory with the watch-only name and a wallet file. - # Because the wallet dir exists, this will cause migration to fail. - watch_only_dir = self.master_node.wallets_path / "default_wallet_watchonly" + if os.path.isabs(wallet_name): + old_path = master_path = Path(wallet_name) + else: + old_path = self.old_node.wallets_path / wallet_name + master_path = self.master_node.wallets_path / wallet_name + os.makedirs(master_path, exist_ok=True) + shutil.copyfile(old_path / "wallet.dat", master_path / "wallet.dat") + + # This will be the watch-only directory the migration tries to create, + # we make migration fail by placing a wallet.dat file there. + wo_prefix = wallet_name or "default_wallet" + # wo_prefix might have path characters in it, this corresponds with + # DoMigration(). + wo_dirname = f"{wo_prefix}_watchonly" + watch_only_dir = self.master_node.wallets_path / wo_dirname os.mkdir(watch_only_dir) - shutil.copyfile(self.old_node.wallets_path / "wallet.dat", watch_only_dir / "wallet.dat") + shutil.copyfile(old_path / "wallet.dat", watch_only_dir / "wallet.dat") + + # Make a file in the wallets dir that must still exist after migration + survive_path = self.master_node.wallets_path / "survive" + open(survive_path, "wb").close() + assert survive_path.exists() mocked_time = int(time.time()) self.master_node.setmocktime(mocked_time) - assert_raises_rpc_error(-4, "Failed to create database", self.migrate_and_get_rpc, "") + assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, wallet_name) self.master_node.setmocktime(0) - # Verify the /wallets/ path exists + # Verify the /wallets/ path exists. assert self.master_node.wallets_path.exists() - # Check backup file exists. Because the wallet has no name, the backup is prefixed with 'default_wallet' - backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak" + + # Verify survive is still there + assert survive_path.exists() + # Verify both wallet paths exist. + assert Path(old_path / "wallet.dat").exists() + assert Path(master_path / "wallet.dat").exists() + + backup_prefix = "default_wallet" if is_default else wallet_name + backup_path = master_path / f"{backup_prefix}_{mocked_time}.legacy.bak" assert backup_path.exists() - # Verify the original unnamed wallet was restored - assert (self.master_node.wallets_path / "wallet.dat").exists() - # And verify it is still a BDB wallet - with open(self.master_node.wallets_path / "wallet.dat", "rb") as f: + + with open(self.master_node.wallets_path / wallet_name / self.wallet_data_filename, "rb") as f: data = f.read(16) _, _, magic = struct.unpack("QII", data) assert_equal(magic, BTREE_MAGIC) - # Test cleanup: clear default wallet for next test - self.clear_default_wallet(backup_path) + + # Cleanup + if is_default: + self.clear_default_wallet(backup_path) + else: + backup_path.unlink() + Path(watch_only_dir / "wallet.dat").unlink() + Path(watch_only_dir).rmdir() + Path(master_path / "wallet.dat").unlink() + Path(old_path / "wallet.dat").unlink(missing_ok=True) def test_direct_file(self): self.log.info("Test migration of a wallet that is not in a wallet directory") @@ -1482,7 +1516,16 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_encrypted() self.test_nonexistent() self.test_unloaded_by_path() - self.test_default_wallet_failure() + + migration_failure_cases = [ + "", + "../", + os.path.abspath(self.master_node.datadir_path / "absolute_path"), + "normallynamedwallet" + ] + for wallet_name in migration_failure_cases: + self.test_migration_failure(wallet_name=wallet_name) + self.test_default_wallet() self.test_default_wallet_watch_only() self.test_direct_file()