diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp index 52da05367f..0ce23a49b4 100644 --- a/src/bench/wallet_migration.cpp +++ b/src/bench/wallet_migration.cpp @@ -66,7 +66,7 @@ static void WalletMigration(benchmark::Bench& bench) bench.epochs(/*numEpochs=*/1).epochIterations(/*numIters=*/1) // run the migration exactly once .run([&] { - auto res{MigrateLegacyToDescriptor(std::move(wallet), /*passphrase=*/"", *loader->context(), /*was_loaded=*/false)}; + auto res{MigrateLegacyToDescriptor(std::move(wallet), /*passphrase=*/"", *loader->context())}; assert(res); assert(res->wallet); assert(res->watchonly_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index de525cdfef..3d83f356f6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4172,18 +4172,10 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle std::vector warnings; bilingual_str error; - // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it - bool was_loaded = false; + // The only kind of wallets that could be loaded are descriptor ones, which don't need to be migrated. if (auto wallet = GetWallet(context, wallet_name)) { - if (wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - return util::Error{_("Error: This wallet is already a descriptor wallet")}; - } - - if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { - return util::Error{_("Unable to unload the wallet before migrating")}; - } - WaitForDeleteWallet(std::move(wallet)); - was_loaded = true; + assert(wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); + return util::Error{_("Error: This wallet is already a descriptor wallet")}; } else { // Check if the wallet is BDB const auto& wallet_path = GetWalletPath(wallet_name); @@ -4217,10 +4209,10 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle return util::Error{Untranslated("Wallet loading failed.") + Untranslated(" ") + error}; } - return MigrateLegacyToDescriptor(std::move(local_wallet), passphrase, context, was_loaded); + return MigrateLegacyToDescriptor(std::move(local_wallet), passphrase, context); } -util::Result MigrateLegacyToDescriptor(std::shared_ptr local_wallet, const SecureString& passphrase, WalletContext& context, bool was_loaded) +util::Result MigrateLegacyToDescriptor(std::shared_ptr local_wallet, const SecureString& passphrase, WalletContext& context) { MigrationResult res; bilingual_str error; @@ -4232,20 +4224,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr const std::string wallet_name = local_wallet->GetName(); - // Helper to reload as normal for some of our exit scenarios - const auto& reload_wallet = [&](std::shared_ptr& to_reload) { - assert(to_reload.use_count() == 1); - 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; - }; - // Before anything else, check if there is something to migrate. if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - if (was_loaded) { - reload_wallet(local_wallet); - } return util::Error{_("Error: This wallet is already a descriptor wallet")}; } @@ -4254,9 +4234,6 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime())); fs::path backup_path = this_wallet_dir / backup_filename; if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) { - if (was_loaded) { - reload_wallet(local_wallet); - } return util::Error{_("Error: Unable to make a backup of your wallet")}; } res.backup_path = backup_path; @@ -4265,9 +4242,6 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Unlock the wallet if needed if (local_wallet->IsLocked() && !local_wallet->Unlock(passphrase)) { - if (was_loaded) { - reload_wallet(local_wallet); - } if (passphrase.find('\0') == std::string::npos) { return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect.")}; } else { @@ -4304,10 +4278,10 @@ 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. + // In case of loading failure, we need to remember the wallet dirs 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_dirs; if (success) { Assume(!res.wallet); // We will set it here. @@ -4319,15 +4293,17 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr for (const auto& path_to_remove : paths_to_remove) fs::remove_all(path_to_remove); } - // Migration successful, unload all wallets locally, then reload them. - // Note: We use a pointer to the shared_ptr to avoid increasing its reference count, - // as 'reload_wallet' expects to be the sole owner (use_count == 1). + // Migration successful, load all the migrated wallets. for (std::shared_ptr* wallet_ptr : {&local_wallet, &res.watchonly_wallet, &res.solvables_wallet}) { if (success && *wallet_ptr) { std::shared_ptr& wallet = *wallet_ptr; - // Save db path and reload wallet + // Save db path and load wallet wallet_dirs.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path()); - success = reload_wallet(wallet); + assert(wallet.use_count() == 1); + std::string wallet_name = wallet->GetName(); + wallet.reset(); + wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + success = (wallet != nullptr); // When no wallet is set, set the main wallet. if (!res.wallet) { @@ -4377,23 +4353,19 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Restore the backup // 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, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/false); if (!restore_error.empty()) { error += restore_error + _("\nUnable to restore backup of wallet."); return util::Error{error}; } + // Verify that the legacy wallet is not loaded after restoring from the backup. + assert(!ptr_wallet); // 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); - // 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; - assert(was_loaded == wallet_reloaded); - return util::Error{error}; } return res; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 2db522c4f4..0d5d1fba1d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1137,7 +1137,7 @@ struct MigrationResult { //! Do all steps to migrate a legacy wallet to a descriptor wallet [[nodiscard]] util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context); //! Requirement: The wallet provided to this function must be isolated, with no attachment to the node's context. -[[nodiscard]] util::Result MigrateLegacyToDescriptor(std::shared_ptr local_wallet, const SecureString& passphrase, WalletContext& context, bool was_loaded); +[[nodiscard]] util::Result MigrateLegacyToDescriptor(std::shared_ptr local_wallet, const SecureString& passphrase, WalletContext& context); } // namespace wallet #endif // BITCOIN_WALLET_WALLET_H