gui: Add a menu action to restore then migrate a legacy wallet #877

achow101 · 2025-05-26T22:09:48Z

Some users will have a backup of their legacy wallet. These cannot be restored since the "Restore Wallet" action expects to be able to load the wallet after restoring, and this fails for legacy wallets now that they are deleted. Furthermore, the "Migrate Wallet" action only allows users to migrate wallets that are in the wallets directory, so such backups cannot be migrated from the GUI.

This PR resolves this issue by adding a menu item in the "Migrate Wallet" menu which allows users to select their backup file so that it will first be restored but not loaded, and then migrated.

Depends on bitcoin/bitcoin#32620

DrahtBot · 2025-05-26T22:09:58Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
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:

#bitcoin/bitcoin/26022 (Add util::ResultPtr class by ryanofsky)

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 · 2025-05-26T22:18:37Z

🚧 At least one of the CI tasks failed.
_{Task ARM, unit tests, no functional tests: https://github.com/bitcoin-core/gui/runs/42923129558}
_{LLM reason (✨ experimental): The CI failure is caused by a compilation error in bitcoingui.cpp due to incorrect usage of the Qt connect() function.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Krellan · 2025-05-27T03:17:44Z

Good idea. I had this happen to me just recently. I encountered a Catch-22: couldn't restore an old wallet file because it wasn't in a currently recognized format (it told me to use the migrate wallet option instead), but I also couldn't access the wallet in the migrate menu because it hadn't successfully been loaded yet!

I tried manually copying the wallet.dat file into a new directory created in my Bitcoin datadir, but that just produced an error at startup saying the wallet was not in a recognized format. So, it's clear we need an all-in-one "restore the old wallet.dat file AND migrate it simultaneously" feature.

luke-jr · 2025-05-27T17:47:30Z

by adding a menu item in the "Migrate Wallet" menu

IMO this is bad UX. Users will be looking to restore the wallet, and the details of how it does that shouldn't require unintuitive steps.

achow101 · 2025-05-27T18:43:16Z

IMO this is bad UX. Users will be looking to restore the wallet, and the details of how it does that shouldn't require unintuitive steps.

While it is an extra click, I don't think it is unintuitive.

When the user goes to restore a legacy wallet, they will be informed that the restore failed and that they will need to migrate the wallet via the corresponding menu. When the user goes there, this option to restore and migrate will be presented to them.

liamperfil · 2025-06-05T13:34:11Z

I support the proposal, but I also consider that this extra click is a bad experience for the user. The ideal would be to directly integrate this functionality to automatically detect if it is a legacy wallet and offer the migration option in the same flow. If you need someone on your team I am available.

pablomartin4btc · 2025-06-20T19:38:20Z

couldn't restore an old wallet file because it wasn't in a currently recognized format (it told me to use the migrate wallet option instead),

If you copy the .dat file or the backup file into the wallets dir (default: datadir/wallets) you will see the option in the "Migrate Wallet" menu item (you can check also with bitcoin-cli or the RPC console and if you call listwalletdir would show it in the output).

but I also couldn't access the wallet in the migrate menu because it hadn't successfully been loaded yet!

It's no longer possible to load a legacy wallet on the master branch and future releases, but you don't need to load the wallet in order to migrate it as I explained above, however you could do it in previous versions of QT or bircoind (v29 or earlier).

I tried manually copying the wallet.dat file into a new directory created in my Bitcoin datadir, but that just produced an error at startup saying the wallet was not in a recognized format.

Just tried and it works, if you launched QT with a test chain you need to add that into the path (eg if you used -regtest, the wallets dir path would be /datadir/regtest/wallets).

pablomartin4btc

tACK 56407ca

I'll try to review the code later (first commit is not longer needed, perhaps rebase?).

Regarding the approach IMHO:

integrate this functionality to automatically detect if it is a legacy wallet and offer the migration option in the same flow.

I thought the same at first glance as an additional to the current PR logic, if the user knows that has the backup to restore maybe goes directly there first.

I'd update the menu item title to "Wallet file...", since you could restore a .dat file or migrate a backup file directly - checked with CLI.

Perhaps @GBKS could add his view here...

RestoreWallet has a load_after_restore parameter, expose this to callers using it through the wallet interface as well.

We will need to use the same migration code in a later commit, so first move it to a separate function.

restore_and_migrate first restores a wallet file to the wallets directory in the expected layout, then it performs legacy to descriptor wallet migration on the restored wallet.

Some users will have backups of a legacy wallet which cannot be restored due to being a legacy wallet, and therefore cannot be migrated from the GUI. This menu item allows such users to restore and migrate their wallets in a single action.

achow101 · 2025-06-20T20:56:46Z

Rebased and ready for review

achow101 force-pushed the gui-migrate-path branch from 7da603a to 7c0bb7f Compare May 26, 2025 22:18

DrahtBot added the CI failed label May 26, 2025

DrahtBot removed the CI failed label May 26, 2025

DrahtBot mentioned this pull request May 27, 2025

Add util::ResultPtr class bitcoin/bitcoin#26022

Draft

achow101 force-pushed the gui-migrate-path branch from 7c0bb7f to 56407ca Compare June 4, 2025 23:56

DrahtBot added CI failed and removed CI failed labels Jun 13, 2025

pablomartin4btc reviewed Jun 20, 2025

View reviewed changes

achow101 added 4 commits June 20, 2025 13:56

wallet, interfaces, gui: Expose load_after_restore parameter

2acf773

RestoreWallet has a load_after_restore parameter, expose this to callers using it through the wallet interface as well.

gui: Move actual migration part of migrate() to its own function

c13f1fb

We will need to use the same migration code in a later commit, so first move it to a separate function.

gui: Add restore_and_migrate function to restore then migrate a wallet

114b52d

restore_and_migrate first restores a wallet file to the wallets directory in the expected layout, then it performs legacy to descriptor wallet migration on the restored wallet.

achow101 force-pushed the gui-migrate-path branch from 56407ca to c02e8a3 Compare June 20, 2025 20:56

achow101 marked this pull request as ready for review June 20, 2025 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

gui: Add a menu action to restore then migrate a legacy wallet #877

gui: Add a menu action to restore then migrate a legacy wallet #877

achow101 commented May 26, 2025

Uh oh!

DrahtBot commented May 26, 2025 •

edited

Loading

Uh oh!

DrahtBot commented May 26, 2025

Uh oh!

Krellan commented May 27, 2025

Uh oh!

luke-jr commented May 27, 2025

Uh oh!

achow101 commented May 27, 2025

Uh oh!

liamperfil commented Jun 5, 2025

Uh oh!

pablomartin4btc commented Jun 20, 2025

Uh oh!

pablomartin4btc left a comment

Uh oh!

achow101 commented Jun 20, 2025

Uh oh!

Uh oh!

gui: Add a menu action to restore then migrate a legacy wallet #877

Are you sure you want to change the base?

gui: Add a menu action to restore then migrate a legacy wallet #877

Conversation

achow101 commented May 26, 2025

Uh oh!

DrahtBot commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

DrahtBot commented May 26, 2025

Uh oh!

Krellan commented May 27, 2025

Uh oh!

luke-jr commented May 27, 2025

Uh oh!

achow101 commented May 27, 2025

Uh oh!

liamperfil commented Jun 5, 2025

Uh oh!

pablomartin4btc commented Jun 20, 2025

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 20, 2025

Uh oh!

Uh oh!

DrahtBot commented May 26, 2025 •

edited

Loading