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
ACK	furszy, achow101
Stale 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:

#32273 (wallet: Fix relative path backup during migration. by davidgumberg)

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:4232 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

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.

rkrux force-pushed on Jun 17, 2025

pablomartin4btc commented at 1:58 pm on June 21, 2025: member

re-ACK 0635f107c4e1442ce6b953308723b58044c558df

fanquake requested review from furszy on Jun 24, 2025

furszy commented at 4:50 pm on June 24, 2025: member

Concept ACK

I think the second commit would look nicer and simpler after something like e86d71b749c08bde6002b9aa2baee824975a518a (which is part of #31423). We’d be able to inline the function at that point.

rkrux commented at 1:01 pm on June 25, 2025: contributor

Thanks for highlighting. I’ll take a look at #31423 soon, it had fallen off my radar for some reason.

achow101 commented at 11:03 pm on June 25, 2025: member

ACK 0635f107c4e1442ce6b953308723b58044c558df

DrahtBot requested review from furszy on Jun 25, 2025

DrahtBot added the label Needs rebase on Jul 2, 2025

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.

0f86da382d

rkrux force-pushed on Jul 3, 2025

rkrux commented at 9:16 am on July 3, 2025: contributor

Rebased over master to incorporate changes from #31423.

DrahtBot removed the label Needs rebase on Jul 3, 2025

in src/wallet/wallet.cpp:4315 in aaa362ec90 outdated

4313             if (success && *wallet_ptr) {
4314                 std::shared_ptr<CWallet>& wallet = *wallet_ptr;
4315                 // Save db path and reload wallet
4316                 wallet_dirs.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path());
4317-                success = reload_wallet(wallet);
4318+                success = load_wallet(wallet);

furszy commented at 2:07 pm on July 3, 2025:

as it is used only here now, could just inline it?

rkrux commented at 9:07 am on July 7, 2025:

I kind of liked the separation of concerns here but done this because multiple people are interested.

rkrux force-pushed on Jul 7, 2025

in src/wallet/wallet.cpp:4231 in 92b387caaa outdated

4225@@ -4226,15 +4226,6 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4226 
4227     const std::string wallet_name = local_wallet->GetName();
4228 
4229-    // Helper to reload as normal for some of our exit scenarios
4230-    const auto& reload_wallet = [&](std::shared_ptr<CWallet>& to_reload) {
4231-        assert(to_reload.use_count() == 1);

furszy commented at 1:39 pm on July 7, 2025:

would keep the assertion. It would be bad if this is ever not the case and we end up resetting the pointer.

rkrux commented at 2:59 pm on July 7, 2025:

Done

rkrux force-pushed on Jul 7, 2025

in src/wallet/wallet.cpp:4319 in 2b8b658308 outdated

4315@@ -4316,7 +4316,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4316         }
4317     }
4318     if (!success) {
4319-        // Migration failed, cleanup
4320+        // Either migration failed or loading of the migrated wallets failed, cleanup either way

furszy commented at 0:33 am on July 8, 2025:

In 2b8b658308c8fbfd9d41d8eb7b9491b615c43872: Based on the commit description, this change seems more like you reasoning about the code flow than something that benefits the reader? I’m not sure it’s adding much value.

pablomartin4btc commented at 1:05 am on July 8, 2025:

I don’t have a strong preference either way, but I agree, the loading failure is part of the broader migration process. Also, if we (ever) later refactor the migration logic into a separate component, the current comment might no longer reflect the actual flow, making it prone to becoming outdated.

rkrux commented at 10:39 am on July 8, 2025:

Dropped the commit.

furszy commented at 0:34 am on July 8, 2025: member

code reviewed, looks good. Just left a comment about the last commit.

wallet: replace `reload_wallet` with inline functionality

Also, update related comments because a reload is not happening
anymore. It 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.

150b5c99ca

rkrux force-pushed on Jul 8, 2025

furszy commented at 1:33 pm on July 8, 2025: member

ACK 150b5c99ca11885ef15d04139d919d734e2c211a

DrahtBot requested review from achow101 on Jul 8, 2025

DrahtBot requested review from pablomartin4btc on Jul 8, 2025

achow101 commented at 9:25 pm on July 10, 2025: member

ACK 150b5c99ca11885ef15d04139d919d734e2c211a

achow101 merged this on Jul 10, 2025

achow101 closed this on Jul 10, 2025

wallet: remove dead code in legacy wallet migration #32758

Code Coverage & Benchmarks

Reviews

Conflicts