diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 58488fdbfaa4..c1b7bff8d880 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -331,6 +332,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 51b0d7fd7987..2d9bf4ef0e33 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -129,6 +129,8 @@ class BerkeleyDatabase : public WalletDatabase /** Return path to main database filename */ std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); } + std::vector Files() override; + std::string Format() override { return "bdb"; } /** * Pointer to shared database environment. diff --git a/src/wallet/db.h b/src/wallet/db.h index 323c506970cb..55558ab87efe 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -162,6 +162,9 @@ class WalletDatabase /** 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/dump.cpp b/src/wallet/dump.cpp index b55b64859c09..2b13e5d93d4c 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -280,11 +280,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/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 8bd79b5fbd9d..804813097a9d 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -66,6 +66,7 @@ class DummyDatabase : public WalletDatabase 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 c534693af164..84ab061e4138 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -109,12 +109,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_use_unsafe_sync(options.use_unsafe_sync) + : WalletDatabase(), m_mock(mock), m_dir_path(dir_path), m_file_path(fs::PathToString(file_path)), 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 @@ -250,7 +250,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 a5350b1ecec4..b5d142434b59 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -76,7 +76,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; @@ -133,6 +133,14 @@ class SQLiteDatabase : public WalletDatabase 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 52ef7553f7fc..fa58f061f785 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -117,6 +117,7 @@ class MockableDatabase : public WalletDatabase 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/transaction.cpp b/src/wallet/transaction.cpp index a46846c1d4a3..4f78fe752054 100644 --- a/src/wallet/transaction.cpp +++ b/src/wallet/transaction.cpp @@ -24,4 +24,9 @@ int64_t CWalletTx::GetTxTime() const int64_t n = nTimeSmart; return n ? n : nTimeReceived; } + +void CWalletTx::CopyFrom(const CWalletTx& _tx) +{ + *this = _tx; +} } // namespace wallet diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index db823f8c0dbe..cf9eb10ed1b1 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -341,11 +341,15 @@ class CWalletTx bool IsCoinBase() const { return tx->IsCoinBase(); } bool IsPlatformTransfer() const { return tx->IsPlatformTransfer(); } +private: // Disable copying of CWalletTx objects to prevent bugs where instances get // copied in and out of the mapWallet map, and fields are updated in the // wrong copy. - CWalletTx(CWalletTx const &) = delete; - void operator=(CWalletTx const &x) = delete; + CWalletTx(const CWalletTx&) = default; + CWalletTx& operator=(const CWalletTx&) = default; +public: + // Instead have an explicit copy function + void CopyFrom(const CWalletTx&); }; struct WalletTxOrderComparator { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 671d5a987687..7378090c1050 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -517,6 +517,7 @@ 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 { @@ -526,14 +527,34 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b return nullptr; } - if (fs::exists(wallet_path) || !TryCreateDirectories(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; + // 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)) { + // 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)) { + error = Untranslated(strprintf("Failed to restore database path '%s'.", fs::PathToString(wallet_path))); + status = DatabaseStatus::FAILED_ALREADY_EXISTS; + return nullptr; + } + created_parent_dir = true; } - created_parent_dir = true; fs::copy_file(backup_file, wallet_file, fs::copy_options::none); + wallet_file_copied = true; wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); } catch (const std::exception& e) { @@ -542,8 +563,19 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b error += strprintf(Untranslated("Unexpected exception: %s"), e.what()); status = DatabaseStatus::FAILED_LOAD; } - if (!wallet && created_parent_dir) { - fs::remove_all(wallet_path); + + // Remove created wallet path only when loading fails + if (!wallet) { + if (wallet_file_copied) fs::remove(wallet_file); + // Clean up the parent directory if we created it during restoration. + // Verifying a corrupt BDB file leaves its environment files (db.log, + // .walletlock, database/) next to the wallet file, so the directory is + // not necessarily empty. It did not exist before this call, so removing + // it recursively only touches what this restore created. + // TODO(bitcoin#28710): once BDB is gone nothing but wallet.dat can be + // created here; restore upstream's `Assume(fs::is_empty(wallet_path)); + // fs::remove(wallet_path);` (bitcoin#34156, 4ed0693a3f). + if (created_parent_dir) fs::remove_all(wallet_path); } return wallet; @@ -4835,9 +4867,36 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) } } + // Get best block locator so that we can copy it to the watchonly and solvables + CBlockLocator best_block_locator; + if (!WalletBatch(GetDatabase()).ReadBestBlock(best_block_locator)) { + error = _("Error: Unable to read wallet's best block locator record"); + return false; + } + // Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet. // We need to go through these in the tx insertion order so that lookups to spends works. std::vector txids_to_delete; + std::unique_ptr watchonly_batch; + if (data.watchonly_wallet) { + watchonly_batch = std::make_unique(data.watchonly_wallet->GetDatabase()); + // Copy the next tx order pos to the watchonly wallet + LOCK(data.watchonly_wallet->cs_wallet); + data.watchonly_wallet->nOrderPosNext = nOrderPosNext; + watchonly_batch->WriteOrderPosNext(data.watchonly_wallet->nOrderPosNext); + // Write the best block locator to avoid rescanning on reload + if (!watchonly_batch->WriteBestBlock(best_block_locator)) { + error = _("Error: Unable to write watchonly wallet best block locator record"); + return false; + } + } + if (data.solvable_wallet) { + // Write the best block locator to avoid rescanning on reload + if (!WalletBatch(data.solvable_wallet->GetDatabase()).WriteBestBlock(best_block_locator)) { + error = _("Error: Unable to write solvable wallet best block locator record"); + return false; + } + } for (const auto& [_pos, wtx] : wtxOrdered) { // Check it is the watchonly wallet's // solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for @@ -4846,13 +4905,21 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) LOCK(data.watchonly_wallet->cs_wallet); if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) { // Add to watchonly wallet - if (!data.watchonly_wallet->AddToWallet(wtx->tx, wtx->m_state)) { - error = _("Error: Could not add watchonly tx to watchonly wallet"); + const uint256& hash = wtx->GetHash(); + const CWalletTx& to_copy_wtx = *wtx; + if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) { + if (!new_tx) return false; + ins_wtx.SetTx(to_copy_wtx.tx); + ins_wtx.CopyFrom(to_copy_wtx); + return true; + })) { + error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex()); return false; } + watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash)); // Mark as to remove from the migrated wallet only if it does not also belong to it if (!is_mine) { - txids_to_delete.push_back(wtx->GetHash()); + txids_to_delete.push_back(hash); } continue; } @@ -4863,6 +4930,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return false; } } + watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { std::vector deleted_txids; @@ -4986,6 +5054,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); @@ -4999,6 +5076,10 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseOptions options; options.require_existing = false; options.require_create = true; + options.require_format = DatabaseFormat::SQLITE; + + WalletContext empty_context; + empty_context.args = context.args; // Make the wallets options.create_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET | WALLET_FLAG_DESCRIPTORS; @@ -5013,9 +5094,15 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector warnings; - std::string wallet_name = wallet.GetName() + "_watchonly"; - data->watchonly_wallet = CreateWallet(context, wallet_name, std::nullopt, options, status, error, warnings); - if (status != DatabaseStatus::SUCCESS) { + 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); + return false; + } + + data->watchonly_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + if (!data->watchonly_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; } @@ -5044,9 +5131,15 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector warnings; - std::string wallet_name = wallet.GetName() + "_solvables"; - data->solvable_wallet = CreateWallet(context, wallet_name, std::nullopt, options, status, error, warnings); - if (status != DatabaseStatus::SUCCESS) { + 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); + return false; + } + + data->solvable_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + if (!data->solvable_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; } @@ -5119,7 +5212,12 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle 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", name, error.original); + return false; + } + return true; }; // Before anything else, check if there is something to migrate. @@ -5132,10 +5230,13 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // Make a backup of the DB in the wallet's directory with a unique filename // based on the wallet name and current timestamp. The backup filename uses - // the basename of the wallet path (or "wallet" if the name is blank) so that - // path-style wallet names cannot escape the wallet directory. - // TODO: backport bitcoin/bitcoin#32273 to also move the backup into the - // top-level walletdir and unify with upstream. + // the basename of the wallet path (or "default_wallet" if the name is blank) + // so that path-style wallet names cannot escape the wallet directory. + // TODO(bitcoin#32273): backups move to the top-level walletdir and are named + // from the wallet path. When backported, drop the basename reduction + // below, replace test_wallet_name_with_slashes with upstream's path + // tests, re-derive the bitcoin#34226 path cases from master, then + // take bitcoin#33104 and bitcoin#33122. fs::path this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path(); std::string backup_prefix; if (!wallet_name.empty()) { @@ -5144,9 +5245,9 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle backup_prefix = fs::PathToString(legacy_wallet_path.filename()); } // Fall back when the name was blank or canonicalizes to an empty filename - // (e.g. "/", "."), so we never emit a file like "-1748432384.legacy.bak". - if (backup_prefix.empty()) backup_prefix = "wallet"; - fs::path backup_filename = fs::PathFromString(strprintf("%s-%d.legacy.bak", backup_prefix, GetTime())); + // (e.g. "/", "."), so we never emit a file like "_1748432384.legacy.bak". + if (backup_prefix.empty()) backup_prefix = MigrationPrefixName(*local_wallet); + fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", backup_prefix, GetTime())); fs::path backup_path = this_wallet_dir / backup_filename; if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) { if (was_loaded) { @@ -5179,66 +5280,123 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // 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 + // TODO(bitcoin#32273): this block exists only on the 28.x/29.x branches. Remove + // it together with the backup naming TODO above: once backups live in + // the top-level walletdir they never need to be moved. + 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, and cleanup if it fails success = DoMigration(*local_wallet, context, error, res); } + // 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 the wallet locally, then reload it. - assert(local_wallet.use_count() == 1); - local_wallet.reset(); - res.wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + // 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; res.wallet_name = wallet_name; - } else { + if (success && res.watchonly_wallet) { + // Reload watchonly + track_for_cleanup(*res.watchonly_wallet); + success = reload_wallet(res.watchonly_wallet); + } + if (success && res.solvables_wallet) { + // Reload solvables + track_for_cleanup(*res.solvables_wallet); + success = reload_wallet(res.solvables_wallet); + } + } + if (!success) { // Migration failed, cleanup - // Copy the backup to the actual wallet dir - fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename); - fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none); - - // Remember this wallet's walletdir to remove after unloading - std::vector wallet_dirs; - wallet_dirs.emplace_back(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path()); - // Unload the wallet locally - assert(local_wallet.use_count() == 1); - local_wallet.reset(); + // Drop the second reference to the reloaded main wallet: UnloadWallet() below + // waits until the last reference is gone, and this one would only die on return. + // Dash-only: upstream 28.x/29.x carry the same latent hang when a watch-only or + // solvables reload fails after the main wallet reloaded, and master only moves + // the pointer into res.wallet, which then skips the rollback instead. + res.wallet.reset(); // Make list of wallets to cleanup std::vector> created_wallets; + if (local_wallet) created_wallets.push_back(std::move(local_wallet)); if (res.watchonly_wallet) created_wallets.push_back(std::move(res.watchonly_wallet)); 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_back(fs::PathFromString(w->GetDatabase().Filename()).parent_path()); + for (std::shared_ptr& wallet : created_wallets) { + track_for_cleanup(*wallet); } // Unload the wallets for (std::shared_ptr& w : created_wallets) { - if (!RemoveWallet(context, w, /*load_on_start=*/false)) { - error += _("\nUnable to cleanup failed migration"); - return util::Error{error}; + if (w->HaveChain()) { + // Unloading for wallets that were loaded for normal use + if (!RemoveWallet(context, w, /*load_on_start=*/false)) { + error += _("\nUnable to cleanup failed migration"); + return util::Error{error}; + } + UnloadWallet(std::move(w)); + } else { + // Unloading for wallets in local context + assert(w.use_count() == 1); + w.reset(); } - UnloadWallet(std::move(w)); } - // Delete the wallet directories - for (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 - DatabaseStatus status; - std::vector warnings; - if (!RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, error, warnings)) { - error += _("\nUnable to restore backup of wallet."); + // 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. + // TODO(bitcoin#31451): RestoreWallet loads the wallet even when it was not loaded + // before migration. When backporting bitcoin#31451, add its load_after_restore + // parameter to RestoreWallet, pass was_loaded here, assert that the wallet is + // reloaded only if was_loaded, and re-derive this function from the 29.x commits + // d91f56e1e3 and a074d36254 (bitcoin#34222), which already have that shape. + bilingual_str restore_error; + const auto& ptr_wallet = RestoreWallet(context, backup_path, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings); + if (!restore_error.empty()) { + error += restore_error + _("\nUnable to restore backup of wallet."); return util::Error{error}; } - // Move the backup to the wallet dir - fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none); - fs::remove(temp_backup_location); - return util::Error{error}; } return res; diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index b4bb58a0ac77..b4c61be7846b 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -401,6 +401,31 @@ def test_dump_createfromdump(self): 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.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") + + 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") diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 8f8c2da40ea5..9048ec24d210 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -37,7 +37,11 @@ from test_framework.blocktools import COINBASE_MATURITY from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal, assert_raises_rpc_error +from test_framework.util import ( + assert_equal, + assert_raises_rpc_error, + sha256sum_file, +) class WalletBackupTest(BitcoinTestFramework): def add_options(self, parser): @@ -155,17 +159,84 @@ def restore_preserves_existing_directory(self): with open(sentinel, encoding='utf8') as f: assert_equal(f.read(), 'preserve existing data') assert_raises_rpc_error(-8, 'Backup file does not exist', node.restorewallet, wallet_name, backup_file + '.missing') - assert_raises_rpc_error(-36, 'Database already exists', node.restorewallet, wallet_name, backup_file) + existing_db = os.path.join(destination, 'wallet.dat') + with open(existing_db, 'w', encoding='utf8') as f: + f.write('not a wallet') + assert_raises_rpc_error(-36, 'Database file exists', node.restorewallet, wallet_name, backup_file) assert os.path.isfile(sentinel) + with open(existing_db, encoding='utf8') as f: + assert_equal(f.read(), 'not a wallet') def restore_wallet_existent_name(self): node = self.nodes[3] backup_file = os.path.join(self.nodes[0].datadir, 'wallet.bak') wallet_name = "res0" - wallet_file = os.path.join(node.wallets_path, wallet_name) - error_message = "Failed to create database path '{}'. Database already exists.".format(wallet_file) + wallet_file = node.wallets_path / wallet_name + 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 os.path.exists(wallet_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 run_test(self): self.log.info("Generating initial blockchain") @@ -243,6 +314,8 @@ def run_test(self): 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") diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 419772242846..3e11e8d27836 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -4,10 +4,14 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test Migrating a wallet from legacy to descriptor.""" +from pathlib import Path import os import random import shutil +import struct + from test_framework.address import key_to_p2pkh, script_to_p2sh +from test_framework.bdb import BTREE_MAGIC from test_framework.descriptors import descsum_create from test_framework.key import ECPubKey from test_framework.test_framework import BitcoinTestFramework @@ -15,8 +19,13 @@ from test_framework.script_util import key_to_p2pkh_script, script_to_p2sh_script from test_framework.util import ( assert_equal, + assert_greater_than, assert_raises_rpc_error, find_vout_for_address, + sha256sum_file, +) +from test_framework.wallet_util import ( + get_generate_key, ) @@ -42,8 +51,8 @@ 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, disable_private_keys=False, blank=False): - self.nodes[0].createwallet(wallet_name=wallet_name, descriptors=False, disable_private_keys=disable_private_keys, blank=blank) + def create_legacy_wallet(self, wallet_name, disable_private_keys=False, blank=False, **kwargs): + self.nodes[0].createwallet(wallet_name=wallet_name, descriptors=False, disable_private_keys=disable_private_keys, blank=blank, **kwargs) wallet = self.nodes[0].get_wallet_rpc(wallet_name) info = wallet.getwalletinfo() assert_equal(info["descriptors"], False) @@ -350,12 +359,17 @@ def test_other_watchonly(self): watchonly_spendable_txid = default.send(outputs=[{received_addr: 1}, {import_addr:1}])["txid"] self.generate(self.nodes[0], 2) + received_watchonly_tx_info = imports0.gettransaction(received_watchonly_txid, True) + received_sent_watchonly_tx_info = imports0.gettransaction(received_sent_watchonly_txid, True) balances = imports0.getbalances() spendable_bal = balances["mine"]["trusted"] watchonly_bal = balances["watchonly"]["trusted"] assert_equal(len(imports0.listtransactions(include_watchonly=True)), 6) + # Mock time forward a bit so we can check that tx metadata is preserved + self.bump_mocktime(100) + # Migrate self.migrate_wallet(imports0) assert_equal(imports0.getwalletinfo()["descriptors"], True) @@ -374,8 +388,12 @@ def test_other_watchonly(self): assert_equal(watchonly_info["descriptors"], True) self.assert_is_sqlite("imports0_watchonly") assert_equal(watchonly_info["private_keys_enabled"], False) - watchonly.gettransaction(received_watchonly_txid) - watchonly.gettransaction(received_sent_watchonly_txid) + received_migrated_watchonly_tx_info = watchonly.gettransaction(received_watchonly_txid) + assert_equal(received_watchonly_tx_info["time"], received_migrated_watchonly_tx_info["time"]) + assert_equal(received_watchonly_tx_info["timereceived"], received_migrated_watchonly_tx_info["timereceived"]) + received_sent_migrated_watchonly_tx_info = watchonly.gettransaction(received_sent_watchonly_txid) + assert_equal(received_sent_watchonly_tx_info["time"], received_sent_migrated_watchonly_tx_info["time"]) + assert_equal(received_sent_watchonly_tx_info["timereceived"], received_sent_migrated_watchonly_tx_info["timereceived"]) watchonly.gettransaction(sent_watchonly_txid) watchonly.gettransaction(watchonly_spendable_txid) assert_equal(watchonly.getbalance(), watchonly_bal) @@ -557,20 +575,125 @@ def test_wallet_name_with_slashes(self): # Basename must use the sanitized prefix derived from the wallet # name's last component, not the full "nested/..." path. backup_basename = os.path.basename(backup_path) - assert backup_basename.startswith("subdir_wallet-"), backup_basename + assert backup_basename.startswith("subdir_wallet_"), backup_basename assert backup_basename.endswith(".legacy.bak"), backup_basename self.assert_is_sqlite(nested_name) + def clear_default_wallet(self, backup_file): + # Test cleanup: Clear unnamed default wallet for subsequent tests + (self.nodes[0].wallets_path / "wallet.dat").unlink() + shutil.rmtree(self.nodes[0].wallets_path / "default_wallet_watchonly", ignore_errors=True) + shutil.rmtree(self.nodes[0].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("") - self.migrate_wallet(wallet) + res = self.migrate_wallet(wallet) info = wallet.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["format"], "sqlite") + 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") + def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + wallet = self.create_legacy_wallet("") + wallet.importaddress(def_wallet.getnewaddress()) + + res = self.migrate_wallet(wallet) + wallet = self.nodes[0].get_wallet_rpc("") + watchonly_wallet = self.nodes[0].get_wallet_rpc("default_wallet_watchonly") + + info = watchonly_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.nodes[0].wallets_path / "wallet.dat").exists() + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], True) + assert_equal(info["format"], "sqlite") + assert_equal(info["private_keys_enabled"], True) + assert_equal(info["walletname"], "") + assert_greater_than(info["keypoolsize"], 0) + + wallet.unloadwallet() + watchonly_wallet.unloadwallet() + self.clear_default_wallet(backup_file=Path(res["backup_path"])) + + 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.nodes[0].get_wallet_rpc(self.default_wallet_name) + wallet = self.create_legacy_wallet(wallet_name, blank=True) + wallet.importaddress(master_wallet.getnewaddress()) + wallet.unloadwallet() + + if os.path.isabs(wallet_name): + wallet_path = Path(wallet_name) + else: + wallet_path = self.nodes[0].wallets_path / wallet_name + + # 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.nodes[0].wallets_path / wo_dirname + os.mkdir(watch_only_dir) + shutil.copyfile(wallet_path / "wallet.dat", watch_only_dir / "wallet.dat") + + # Make a file in the wallets dir that must still exist after migration + survive_path = self.nodes[0].wallets_path / "survive" + open(survive_path, "wb").close() + assert survive_path.exists() + + # The backup is named after the node's time, which is the framework's mocktime + mocked_time = self.mocktime + assert_raises_rpc_error(-4, "Failed to create database", self.nodes[0].migratewallet, wallet_name) + wallet.unloadwallet() + + # Verify the /wallets/ path exists. + assert self.nodes[0].wallets_path.exists() + + # Verify survive is still there + assert survive_path.exists() + # Verify the wallet path exists. + assert Path(wallet_path / "wallet.dat").exists() + + # The backup is named after the last component of the wallet's path, so + # path-style wallet names cannot escape the wallet directory. + backup_prefix = "default_wallet" if is_default else wallet_path.resolve().name + backup_path = wallet_path / f"{backup_prefix}_{mocked_time}.legacy.bak" + assert backup_path.exists() + + with open(self.nodes[0].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) + + survive_path.unlink() + + # 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(wallet_path / "wallet.dat").unlink() + + 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") @@ -877,6 +1000,65 @@ def check(info, node): for addr_info in [addr_external, addr_external_with_label]: check(addr_info, wallet_solvables) + def test_failed_migration_cleanup(self): + self.log.info("Test that a failed migration is cleaned up") + wallet = self.create_legacy_wallet("failed") + + # Make a copy of the wallet with the solvables wallet name so that we are unable + # to create the solvables wallet when migrating, thus failing to migrate + wallet.unloadwallet() + solvables_path = self.nodes[0].wallets_path / "failed_solvables" + shutil.copytree(self.nodes[0].wallets_path / "failed", solvables_path) + original_shasum = sha256sum_file(solvables_path / "wallet.dat") + + self.nodes[0].loadwallet("failed") + + # Add a multisig so that a solvables wallet is created + wallet.addmultisigaddress(2, [wallet.getnewaddress(), get_generate_key().pubkey]) + wallet.importaddress(get_generate_key().p2pkh_addr) + + assert_raises_rpc_error(-4, "Failed to create database", wallet.migratewallet) + + assert "failed" in self.nodes[0].listwallets() + assert "failed_watchonly" not in self.nodes[0].listwallets() + assert "failed_solvables" not in self.nodes[0].listwallets() + + assert not (self.nodes[0].wallets_path / "failed_watchonly").exists() + # Since the file in failed_solvables is one that we put there, migration shouldn't touch it + assert solvables_path.exists() + new_shasum = sha256sum_file(solvables_path / "wallet.dat") + assert_equal(original_shasum, new_shasum) + + wallet.unloadwallet() + # Check the wallet we tried to migrate is still BDB + with open(self.nodes[0].wallets_path / "failed" / "wallet.dat", "rb") as f: + data = f.read(16) + _, _, magic = struct.unpack("QII", data) + assert_equal(magic, BTREE_MAGIC) + + def unsynced_wallet_on_pruned_node_fails(self): + self.log.info("Test migration of an unsynced wallet on a pruned node fails gracefully") + shutil.rmtree(self.nodes[0].wallets_path / "database", ignore_errors=True) + wallet = self.create_legacy_wallet("", load_on_startup=False) + last_wallet_synced_block = wallet.getwalletinfo()['lastprocessedblock']['height'] + + # Generate blocks just so the wallet best block is pruned. + # Dash's small early regtest blocks fill the first -fastprune block file up + # to height ~320, so mine and prune further than upstream to remove it. + self.restart_node(0, ["-fastprune", "-prune=1", "-nowallet"]) + self.generate(self.nodes[0], 600, sync_fun=self.no_op) + self.nodes[0].pruneblockchain(400) + # Ensure next block to sync is unavailable + assert_raises_rpc_error(-1, "Block not available (pruned data)", self.nodes[0].getblock, self.nodes[0].getblockhash(last_wallet_synced_block + 1)) + + # Check migration failure + mocked_time = self.mocktime + 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)\nUnable to restore backup of wallet.", self.nodes[0].migratewallet, wallet_name="") + + # The wallet will not have been restored, and the backup will be in the wallets directory. + backup_path = self.nodes[0].wallets_path / f"default_wallet_{mocked_time}.legacy.bak" + assert backup_path.exists() + def run_test(self): self.generate(self.nodes[0], 101) @@ -890,12 +1072,27 @@ def run_test(self): self.test_unloaded() self.test_unloaded_by_path() self.test_wallet_name_with_slashes() + + migration_failure_cases = [ + "", + "../", + os.path.abspath(self.nodes[0].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() self.test_migrate_raw_p2sh() self.test_hybrid_pubkey() self.test_conflict_txs() self.test_addressbook() + self.test_failed_migration_cleanup() + + # Note: After this test the first block file of 'nodes[0]' is pruned + self.unsynced_wallet_on_pruned_node_fails() if __name__ == '__main__': WalletMigrationTest().main()