From 0f86da382d3fdca6fc69ff277794acbc3f1e928d Mon Sep 17 00:00:00 2001 From: rkrux Date: Mon, 16 Jun 2025 12:55:32 +0530 Subject: [PATCH] wallet: remove dead code in legacy wallet migration A discussion on a previous PR 32481 related to legacy wallet dead code removal made me realize that checking if the legacy wallet was loaded prior to the start of the migration is not required ever since legacy wallets can't be loaded in the first place. I also verified that the `load_on_start` persistent setting can also not cause the legacy wallets to be loaded, which further makes the case for removal of the above mentioned checks during migration. The current test coverage also shows these lines uncovered. --- src/bench/wallet_migration.cpp | 2 +- src/wallet/wallet.cpp | 37 ++++++++-------------------------- src/wallet/wallet.h | 2 +- 3 files changed, 10 insertions(+), 31 deletions(-) diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp index 52da05367f2..0ce23a49b4e 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 3f2d1a38f00..ca73486aeee 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4174,18 +4174,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); @@ -4219,10 +4211,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; @@ -4245,9 +4237,6 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // 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")}; } @@ -4256,9 +4245,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; @@ -4267,9 +4253,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 { @@ -4379,23 +4362,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 2db522c4f4d..0d5d1fba1db 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