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

achow101 commented at 10:09 pm on May 26, 2025: member

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 https://github.com/bitcoin/bitcoin/pull/32620

DrahtBot commented at 10:09 pm on May 26, 2025: contributor

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.

achow101 force-pushed on May 26, 2025

DrahtBot added the label CI failed on May 26, 2025

DrahtBot commented at 10:18 pm on May 26, 2025: contributor

🚧 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.

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.

DrahtBot removed the label CI failed on May 26, 2025

Krellan commented at 3:17 am on May 27, 2025: contributor

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 commented at 5:47 pm on May 27, 2025: member

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 commented at 6:43 pm on May 27, 2025: member

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.

achow101 force-pushed on Jun 4, 2025

liamperfil commented at 1:34 pm on June 5, 2025: none

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.

DrahtBot added the label CI failed on Jun 13, 2025

DrahtBot removed the label CI failed on Jun 13, 2025

pablomartin4btc commented at 7:38 pm on June 20, 2025: contributor

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 commented at 7:55 pm on June 20, 2025: contributor

tACK 56407ca69e34778cfd0d054fbaa94ab0b28ffb6d

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…

wallet, interfaces, gui: Expose load_after_restore parameter

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

2acf773800

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

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

c13f1fbffb

gui: Add restore_and_migrate function to restore then migrate a wallet

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.

114b52d605

gui: Add a menu item to restore then migrate a wallet file

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.

c02e8a376d

achow101 force-pushed on Jun 20, 2025

achow101 marked this as ready for review on Jun 20, 2025

achow101 commented at 8:56 pm on June 20, 2025: member

Rebased and ready for review

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

Reviews

Conflicts