From 7475d134f6a3a6039ab6b9d39706ade47c764aa8 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 15 Jan 2026 19:01:19 +0000 Subject: [PATCH 1/5] Wallet/bdb: Safely and correctly list files only used by the single wallet If any other files exist in the directory, we cannot assume the sharable files are exclusively for this wallet. But if they are, this also cleans up other log.* files --- src/wallet/bdb.cpp | 48 ++++++++++++++++++++++++++++++++++++++++++++++ src/wallet/bdb.h | 15 +-------------- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 79851dff33f..f5a18266edb 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -340,6 +341,53 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr) return true; } +std::vector BerkeleyDatabase::Files() +{ + std::vector files; + // If the wallet is the *only* file, clean up the entire BDB environment + constexpr auto build_files_list = [](std::vector& files, const std::shared_ptr& env, const fs::path& filename) { + if (env->m_databases.size() != 1) return false; + + const auto env_dir = env->Directory(); + const auto db_subdir = env_dir / "database"; + if (fs::exists(db_subdir)) { + if (!fs::is_directory(db_subdir)) return false; + for (const auto& entry : fs::directory_iterator(db_subdir)) { + const auto& path = entry.path().filename(); + if (!fs::PathToString(path).starts_with("log.")) { + return false; + } + files.emplace_back(entry.path()); + } + } + const std::set allowed_paths = { + filename, + "db.log", + ".walletlock", + "database" + }; + for (const auto& entry : fs::directory_iterator(env_dir)) { + const auto& path = entry.path().filename(); + if (allowed_paths.contains(path)) { + files.emplace_back(entry.path()); + } else if (fs::is_directory(entry.path())) { + // Subdirectories can't possibly be using this db env, and is expected if this is a non-directory wallet + // Do not include them in Files, but still allow the env cleanup + } else { + return false; + } + } + return true; + }; + try { + if (build_files_list(files, env, m_filename)) return files; + } catch (...) { + // Give up building the comprehensive file list if any error occurs + } + // Otherwise, it's only really safe to delete the one wallet file + return {env->Directory() / m_filename}; +} + void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile) { dbenv->txn_checkpoint(0, 0, 0); diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index ec773fd1770..a7cf953ed21 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -132,20 +132,7 @@ 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::vector Files() override; std::string Format() override { return "bdb"; } /** From 60f529027c6eacbdc298fab50192f8c60d7082a1 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 15 Jan 2026 19:03:23 +0000 Subject: [PATCH 2/5] Wallet/Migration: If loading the new watchonly or solvables wallet fails, log the correct wallet name in error message --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 913c7453205..992b02a9895 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4532,7 +4532,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); 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); + "Error cause: %s\n", name, error.original); return false; } return true; From cef01d0be5223e9d33efc897d7fbe5d0a08692c0 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 15 Jan 2026 19:27:23 +0000 Subject: [PATCH 3/5] Wallet/Migration: Skip moving the backup file back and forth for no reason Since we no longer delete the wallet directory, there's no need to vacate it The moving only served to risk errors by crossing filesystem boundaries (which fs::rename can't handle) --- src/wallet/wallet.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 992b02a9895..df53f904cc2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4634,9 +4634,6 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } if (!success) { // 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::rename(backup_path, temp_backup_location); // Make list of wallets to cleanup std::vector> created_wallets; @@ -4680,15 +4677,12 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory. // Reload it into memory if the wallet was previously loaded. bilingual_str restore_error; - const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded); + const auto& ptr_wallet = RestoreWallet(context, backup_path, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded); if (!restore_error.empty()) { error += restore_error + _("\nUnable to restore backup of wallet."); return util::Error{error}; } - // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir - 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. bool wallet_reloaded = ptr_wallet != nullptr; From 69a6b9b1152ba0bb3edab6d2a54509fd416b24c8 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 20 Jan 2026 18:20:14 +0000 Subject: [PATCH 4/5] Bugfix: Wallet/Migration: Move backup into wallet directory when migrating from non-directory While 30.x+ keep backup files in walletdir, 29.x places them in the migrated wallet directory --- src/wallet/wallet.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index df53f904cc2..45adda65ead 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4581,6 +4581,12 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // First change to using SQLite if (!local_wallet->MigrateToSQLite(error)) return util::Error{error}; + // In case we're migrating from file to directory, move the backup into it + this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path(); + backup_path = this_wallet_dir / backup_filename; + fs::rename(res.backup_path, backup_path); + res.backup_path = backup_path; + // 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) { From 65173944ed60df3b9cffca95932aed8720921478 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Wed, 21 Jan 2026 21:32:40 +0000 Subject: [PATCH 5/5] QA: tool_wallet: Check that db.log is deleted with a lone legacy wallet, but not with a shared db environment --- test/functional/tool_wallet.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 979804a5fea..788d9b0ee8d 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -410,17 +410,30 @@ class ToolWalletTest(BitcoinTestFramework): 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") + os.rename(self.nodes[0].wallets_path / "wallet.dat", self.nodes[0].wallets_path / "../default.wallet.dat") + (self.nodes[0].wallets_path / "db.log").unlink(missing_ok=True) 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() + if not self.options.descriptors: + assert not (self.nodes[0].wallets_path / "db.log").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") + os.rename(self.nodes[0].wallets_path / "../default.wallet.dat", self.nodes[0].wallets_path / "wallet.dat") + + self.log.info('Checking createfromdump with multiple non-directory wallets') + assert not (self.nodes[0].wallets_path / "wallet.dat").is_dir() + assert (self.nodes[0].wallets_path / "db.log").exists() + os.rename(self.nodes[0].wallets_path / "wallet.dat", self.nodes[0].wallets_path / "test.dat") + self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump') + assert not (self.nodes[0].wallets_path / "wallet.dat").exists() + assert (self.nodes[0].wallets_path / "test.dat").exists() + assert (self.nodes[0].wallets_path / "db.log").exists() + os.rename(self.nodes[0].wallets_path / "test.dat", self.nodes[0].wallets_path / "wallet.dat") def test_chainless_conflicts(self): self.log.info("Test wallet tool when wallet contains conflicting transactions")