From b09e5684cad2ff469ce241e9d532953a6863d5d4 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 6 Oct 2023 15:58:13 -0400 Subject: [PATCH] wallet: Reload watchonly and solvables wallets after migration When migrating, create the watchonly and solvables wallets without a context. Then unload and reload them after migration completes, as we do for the actual wallet. There is also additional handling for a failed reload. --- src/wallet/wallet.cpp | 84 +++++++++++++++++++++-------- test/functional/wallet_migration.py | 2 +- 2 files changed, 62 insertions(+), 24 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0ab79b27a5e79a..af81ada440b054 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4082,6 +4082,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; @@ -4097,8 +4101,14 @@ 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::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; } @@ -4128,8 +4138,14 @@ 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::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; } @@ -4232,47 +4248,69 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle success = DoMigration(*local_wallet, context, error, res); } + // 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; 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. + const auto& reload_wallet = [&](std::shared_ptr& to_reload) { + assert(to_reload.use_count() == 1); + std::string name = to_reload->GetName(); + wallet_dirs.insert(fs::PathFromString(to_reload->GetDatabase().Filename()).parent_path()); + to_reload.reset(); + to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + return to_reload != nullptr; + }; + // Reload the main wallet + success = reload_wallet(local_wallet); + res.wallet = local_wallet; res.wallet_name = wallet_name; - } else { + if (success && res.watchonly_wallet) { + // Reload watchonly + success = reload_wallet(res.watchonly_wallet); + } + if (success && res.solvables_wallet) { + // Reload solvables + 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(); - // 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()); + wallet_dirs.emplace(fs::PathFromString(w->GetDatabase().Filename()).parent_path()); } // 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) { + for (const fs::path& dir : wallet_dirs) { fs::remove_all(dir); } diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 33ab8bf3f8bee2..ad8dbe78edb833 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -848,7 +848,7 @@ def test_failed_migration_cleanup(self): wallet.addmultisigaddress(2, [wallet.getnewaddress(), get_generate_key().pubkey]) wallet.importaddress(get_generate_key().p2pkh_addr) - assert_raises_rpc_error(-4, "Failed to create new watchonly wallet", wallet.migratewallet) + 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()