wallet: remove dead code in legacy wallet migration

rkrux commented at 7:26 am on June 16, 2025: contributor

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.

DrahtBot commented at 7:26 am on June 16, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32758.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31423 (wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Jun 16, 2025

rkrux force-pushed on Jun 16, 2025

rkrux renamed this:
~~wallet: wip~~
wallet: remove dead code in legacy wallet migration
on Jun 16, 2025

rkrux marked this as ready for review on Jun 16, 2025

rkrux commented at 9:17 am on June 16, 2025: contributor

previous releases, depends DEBUGFailing after 4m — Task Summary

This seems unrelated as the error is 0curl: (6) Could not resolve host: github.com in the CI run.

in src/wallet/wallet.cpp:4329 in 67845e0411 outdated

4325@@ -4344,10 +4326,10 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4326         fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none);
4327         fs::remove(temp_backup_location);
4328 
4329-        // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null.
4330+        // Verify that the legacy wallet is not loaded.

pablomartin4btc commented at 6:28 pm on June 16, 2025:

nit (and I’d remove the following commented line starting with // This check…):

0        // Verify that the legacy wallet is not loaded after the restore.

rkrux commented at 11:06 am on June 17, 2025:

Done

in src/wallet/wallet.cpp:4332 in 67845e0411 outdated

4325@@ -4344,10 +4326,10 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4326         fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none);
4327         fs::remove(temp_backup_location);
4328 
4329-        // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null.
4330+        // Verify that the legacy wallet is not loaded.
4331         // This check is performed after restoration to avoid an early error before saving the backup.
4332         bool wallet_reloaded = ptr_wallet != nullptr;
4333-        assert(was_loaded == wallet_reloaded);
4334+        assert(!wallet_reloaded);

pablomartin4btc commented at 7:25 pm on June 16, 2025:

0        assert(!ptr_wallet);

… and (continuing with the previous nit comment) I would remove: https://github.com/bitcoin/bitcoin/blob/67845e0411241d83c770fafa933b11cf6b8b27b8/src/wallet/wallet.cpp#L4330-L4331

rkrux commented at 11:06 am on June 17, 2025:

Done

in src/wallet/wallet.cpp:4205 in 67845e0411 outdated

4209@@ -4218,9 +4210,6 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4210 
4211     // Before anything else, check if there is something to migrate.
4212     if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
4213-        if (was_loaded) {
4214-            reload_wallet(local_wallet);
4215-        }
4216         return util::Error{_("Error: This wallet is already a descriptor wallet")};
4217     }

pablomartin4btc commented at 7:31 pm on June 16, 2025:

This could be removed entirely if MigrateLegacyToDescriptor(std::shared_ptr<CWallet> local_wallet, ... was only called from MigrateLegacyToDescriptor(const std::string& wallet_name, ... but currently this function is also being called from the bench wallet_migration.cpp.

pablomartin4btc commented at 8:00 pm on June 16, 2025: member

Code review ACK

I’ve left some comments.

I think now the reload_wallet helper could be moved into the if (success) { condition: https://github.com/bitcoin/bitcoin/blob/67845e0411241d83c770fafa933b11cf6b8b27b8/src/wallet/wallet.cpp#L4202-L4209

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.

d22b2c2faa

rkrux force-pushed on Jun 17, 2025

rkrux commented at 11:07 am on June 17, 2025: contributor

I think now the reload_wallet helper could be moved into the if (success) { condition:

Makes sense, I have done this. Also, ended up doing few related changes in the subsequent commits.

wallet: narrow scope of `reload_wallet` after successful migration

Also, rename it to `load_wallet` and update related comments.
Renaming is done because the legacy wallets could not have been
loaded prior to migration, so I don't think a reload is happening
post a successful migration, it's just load IMO.

47b16bfece

wallet: update comment in case of loading of migrated wallet failure

I have not checked if this is an intended behaviour of failing the
whole migration process in case we are unable to load the migrated
wallets after a successful migration. But IIUC this is what the
code is doing because the `success` variable is the same for
`DoMigration` & the subsequent loading of the migrated wallets,
so this commit updates the comment related to it.

0635f107c4

rkrux force-pushed on Jun 17, 2025

wallet: remove dead code in legacy wallet migration #32758

Code Coverage & Benchmarks

Reviews

Conflicts