Migrate legacy wallets that are not loaded #824

achow101 · 2024-06-10T20:43:07Z

Currently the Migrate Wallet menu item can only be used to migrate the currently loaded wallet. This is not suitable for the future when legacy wallets can no longer be loaded at all, but should still be able to be migrated. This PR changes that menu item into a menu list like Open Wallet and lets users migrate any legacy wallet in their wallet directory regardless of the wallets loaded.

One issue I ran into was dealing with encrypted wallets. Ideally, we would detect whether a wallet is encrypted, and prompt the user for their passphrase at that time. However, that's actually difficult to do in the GUI since migration will unload the wallet if it was already loaded, and reload it without connecting it to any signals or interfaces. Only then can it detect whether a wallet is encrypted, but then there is no WalletModel or even an interfaces::Wallet that the GUI could use to unlock it via a callback.

To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted. If the user enters the wrong passphrase or clicked the other button that does not prompt for the passphrase, migration will fail with a message indicating that the passphrase was incorrect.

DrahtBot · 2024-06-10T20:43:10Z

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

#753 (Add new "address type" column to the "receiving tab" address book page by pablomartin4btc)

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.

hebasto · 2024-06-11T07:14:57Z

Concept ACK.

luke-jr · 2024-06-12T20:32:05Z

To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted.

If we're preemptively making UX changes just to avoid changing UX in the future, it seems like we shouldn't take shortcuts like this... :/

achow101 · 2024-06-12T21:24:34Z

To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted.

If we're preemptively making UX changes just to avoid changing UX in the future, it seems like we shouldn't take shortcuts like this... :/

We already do this in the RPC too, for exactly the same reason.

hebasto · 2024-07-14T10:44:52Z

src/qt/walletcontroller.h

-    void migrateWallet(WalletModel* wallet_model, QWidget* parent = nullptr);
-


Apparently, this code has been dead since introducing in #738.

hebasto · 2024-07-14T11:02:55Z

src/qt/bitcoingui.cpp

+                // Menu items remove single &. Single & are shown when && is in
+                // the string, but only the first occurrence. So replace only
+                // the first & with &&.
+                name.replace(name.indexOf(QChar('&')), 1, QString("&&"));


#828 can relevant to this code.

Since #828 has been merged, please keep in mind this update...

Suggested change

// Menu items remove single &. Single & are shown when && is in

// the string, but only the first occurrence. So replace only

// the first & with &&.

name.replace(name.indexOf(QChar('&')), 1, QString("&&"));

// Single & are shown when && is in the string. So replace & with &&.

name.replace(QChar('&'), QString("&&"));

Fixed during rebase.

Fixed during rebase.

It still needs to be fixed.

Ah, missed one, fixed.

src/qt/askpassphrasedialog.h

src/interfaces/wallet.h

hebasto · 2024-07-14T11:26:12Z

This PR changes that menu item into a menu list like Open Wallet and lets users migrate any legacy wallet in their wallet directory regardless of the wallets loaded.

Tested d45ac03. The "Migrate Wallet" menu item is still disabled when no wallet is loaded.

hebasto · 2024-07-29T09:42:34Z

Please rebase.

pablomartin4btc

tACK d45ac03

Created a couple of legacy wallets (encrypted and not) to begin with.

edit: running ./configure with: --with-bdb.

Using bitcoind (./src/bitcoind -regtest -deprecatedrpc=create_bdb ) and bitcoin-cli (./src/bitcoin-cli -regtest createwallet legacyWallet false false "" false false ) or directly on the rpc-console (createwallet legacyWalletEnc false false "passphrase" false false) in bitcoin-qt (./src/qt/bitcoin-qt -regtest -deprecatedrpc=create_bdb).

{
  "name": "legacyWalletEnc",
  "warnings": [
    "Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."
  ]
}

{
  "name": "legacyWallet",
  "warnings": [
    "Empty string given as passphrase, wallet will not be encrypted.",
    "Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."
  ]
}

The menu item "Migrate Wallet" is shown when there are already wallets loaded.

But "Migrate Wallet" is disabled when there are no wallets loaded as @hebasto mentioned above.

I think that should be solved by removing the migration action from BitcoinGUI::setWalletActionsEnabled (the migration action has the same behaviour as the restore action which is not there).

"Migrate Wallet" is shown when there are no wallets available at all.

Tested migration of encrypted wallet with and without providing the passphrase, working both cases.

Every migration failure on a particular wallet still creates a backup file. Do we want this?

achow101 · 2024-08-08T14:44:47Z

Tested d45ac03. The "Migrate Wallet" menu item is still disabled when no wallet is loaded.

Should be fixed now.

"Migrate Wallet" is shown when there are no wallets available at all.

I think that's ok.

pablomartin4btc · 2024-08-08T16:04:58Z

"Migrate Wallet" is shown when there are no wallets available at all.

I think that's ok.

Yeah, it was just an observation (mainly to compare when the menu is disabled), thanks!

pablomartin4btc

Tested d45ac03. The "Migrate Wallet" menu item is still disabled when no wallet is loaded.

Should be fixed now.

re-ACK bcf47b9

furszy

Few comments. Will continue reviewing.

src/qt/bitcoingui.cpp

furszy · 2024-08-12T14:05:53Z

src/qt/bitcoingui.cpp

+                connect(action, &QAction::triggered, [this, wallet_name] {
+                    auto activity = new MigrateWalletActivity(m_wallet_controller, this);
+                    connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet);
+                    activity->migrate(wallet_name);


Compilation error: error: 'wallet_name' in capture list does not name a variable.

It seems the lambda expression cannot capture the structured binding. A local reference of the variable fixes it.

Same happens on the m_open_wallet_menu action lambda with the "path" variable.

What compiler? I'm not seeing this error, and it seems neither did CI.

What compiler? I'm not seeing this error, and it seems neither did CI.

clang 14.0.3 (clang-1403.0.22.14.1).

src/qt/walletcontroller.cpp

furszy

Worked further on your 8d0b5c1 to eliminate the need for the extra 'Yes, encrypted' button in the migration dialog, which I found quite unfriendly. Could replace it directly for furszy/bitcoin-core@e8d147e. It detects the wallet encryption status automatically regardless of its loaded/unloaded state.

Other than this, code is looking good.

Instead of having the code for the wallet display name being copy and pasted, use a GUIUtil function to get that for us.

To prepare for migrating wallets that are not loaded, when migration occurs in the GUI, it should not rely on a WalletModel existing. Co-authored-by: furszy <[email protected]>

achow101 · 2024-08-13T15:26:03Z

Could replace it directly for furszy/bitcoin-core@e8d147e. It detects the wallet encryption status automatically regardless of its loaded/unloaded state.

Cherry picked.

Also fixed test failure.

furszy

ACK fd087d4

hebasto

Almost ACK fd087d4.

hebasto · 2024-08-13T20:48:18Z

src/qt/bitcoingui.cpp

+                // Menu items remove single &. Single & are shown when && is in
+                // the string, but only the first occurrence. So replace only
+                // the first & with &&.
+                name.replace(name.indexOf(QChar('&')), 1, QString("&&"));


Fixed during rebase.

It still needs to be fixed.

Once legacy wallets can no longer be loaded, we need to be able to migrate them without loading. Thus we should use a menu that lists the wallets in the wallet directory instead of an action which migrates the currently loaded wallet.

hebasto

ACK 8f2522d.

hebasto · 2024-08-14T09:11:45Z

cc @furszy

furszy

ACK 8f2522d

MarnixCroes · 2024-08-14T16:19:43Z

src/qt/bitcoingui.cpp

+                });
+            }
+            if (m_migrate_wallet_menu->isEmpty()) {
+                QAction* action = m_migrate_wallet_menu->addAction(tr("No wallets available"));


nit: maybe a more descriptive text would be good?
something like No wallets that need to be migrated

for people who don't know about the migration and why/what it is.
seeing No wallets available while they have wallet(s) might seem wrong/confusing.

sry for commenting post-merge

pablomartin4btc

post-merge tACK 8f2522d

Checked changes proposed by @furszy.

Perhaps the PR's description needs to be updated (or a note added) since the previous extra button to enter the passphrase was removed (isEncrypted is in place). Nice solution, less prone to errors too.

achow101 added this to the 28.0 milestone Jun 10, 2024

achow101 mentioned this pull request Jun 10, 2024

Proposed Timeline for Legacy Wallet and BDB removal bitcoin/bitcoin#20160

Open

44 tasks

achow101 removed this from the 28.0 milestone Jun 10, 2024

achow101 force-pushed the gui-migrate-unloaded branch from bb24232 to bc8a218 Compare June 10, 2024 21:01

achow101 mentioned this pull request Jun 10, 2024

Remove the legacy wallet and BDB dependency bitcoin/bitcoin#28710

Draft

DrahtBot mentioned this pull request Jun 11, 2024

Add new "address type" column to the "receiving tab" address book page #753

Open

hebasto changed the title ~~gui: Migrate legacy wallets that are not loaded~~ Migrate legacy wallets that are not loaded Jun 11, 2024

DrahtBot mentioned this pull request Jun 11, 2024

wallet: Fix listwalletdir listing of migrated default wallets and generated backup files bitcoin/bitcoin#30265

Merged

achow101 force-pushed the gui-migrate-unloaded branch from bc8a218 to d45ac03 Compare July 11, 2024 19:26

DrahtBot mentioned this pull request Jul 13, 2024

Rendering an amp characters in the wallet name for QMenu #828

Merged

hebasto reviewed Jul 14, 2024

View reviewed changes

DrahtBot added the Needs rebase label Jul 29, 2024

pablomartin4btc reviewed Jul 31, 2024

View reviewed changes

DrahtBot requested a review from hebasto July 31, 2024 21:42

achow101 added this to the 28.0 milestone Aug 8, 2024

achow101 force-pushed the gui-migrate-unloaded branch from d45ac03 to bcf47b9 Compare August 8, 2024 14:44

DrahtBot removed the Needs rebase label Aug 8, 2024

pablomartin4btc approved these changes Aug 9, 2024

View reviewed changes

furszy reviewed Aug 12, 2024

View reviewed changes

DrahtBot added the Needs rebase label Aug 12, 2024

achow101 force-pushed the gui-migrate-unloaded branch from bcf47b9 to df9c1ef Compare August 12, 2024 17:20

DrahtBot removed the Needs rebase label Aug 12, 2024

DrahtBot added the CI failed label Aug 12, 2024

furszy reviewed Aug 13, 2024

View reviewed changes

achow101 force-pushed the gui-migrate-unloaded branch from df9c1ef to be93d63 Compare August 13, 2024 15:25

achow101 and others added 4 commits August 13, 2024 11:25

wallet, interfaces: Include database format in listWalletDir

28fc562

gui: Consolidate wallet display name to GUIUtil function

bfba638

Instead of having the code for the wallet display name being copy and pasted, use a GUIUtil function to get that for us.

gui: don't remove wallet manually before migration

c391858

gui: Use wallet name for wallet migration rather than WalletModel

d56a450

To prepare for migrating wallets that are not loaded, when migration occurs in the GUI, it should not rely on a WalletModel existing. Co-authored-by: furszy <[email protected]>

achow101 force-pushed the gui-migrate-unloaded branch from be93d63 to fd087d4 Compare August 13, 2024 15:25

DrahtBot removed the CI failed label Aug 13, 2024

furszy reviewed Aug 13, 2024

View reviewed changes

DrahtBot requested a review from pablomartin4btc August 13, 2024 19:33

hebasto reviewed Aug 13, 2024

View reviewed changes

gui: Use menu for wallet migration

8f2522d

Once legacy wallets can no longer be loaded, we need to be able to migrate them without loading. Thus we should use a menu that lists the wallets in the wallet directory instead of an action which migrates the currently loaded wallet.

achow101 force-pushed the gui-migrate-unloaded branch from fd087d4 to 8f2522d Compare August 13, 2024 23:18

hebasto approved these changes Aug 14, 2024

View reviewed changes

DrahtBot requested a review from furszy August 14, 2024 09:11

furszy reviewed Aug 14, 2024

View reviewed changes

hebasto merged commit e682e7d into bitcoin-core:master Aug 14, 2024
16 checks passed

hebasto added the Wallet label Aug 14, 2024

hebasto mentioned this pull request Aug 14, 2024

Update translation source file for v28.0 string freeze #833

Merged

MarnixCroes reviewed Aug 14, 2024

View reviewed changes

pablomartin4btc reviewed Aug 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate legacy wallets that are not loaded #824

Migrate legacy wallets that are not loaded #824

achow101 commented Jun 10, 2024

DrahtBot commented Jun 10, 2024 •

edited

Loading

hebasto commented Jun 11, 2024

luke-jr commented Jun 12, 2024

achow101 commented Jun 12, 2024

hebasto Jul 14, 2024

hebasto Jul 14, 2024

pablomartin4btc Jul 31, 2024

achow101 Aug 8, 2024

hebasto Aug 13, 2024

achow101 Aug 13, 2024

hebasto commented Jul 14, 2024

hebasto commented Jul 29, 2024

pablomartin4btc left a comment •

edited

Loading

achow101 commented Aug 8, 2024 •

edited

Loading

pablomartin4btc commented Aug 8, 2024

pablomartin4btc left a comment

furszy left a comment

furszy Aug 12, 2024

achow101 Aug 12, 2024

furszy Aug 13, 2024

furszy left a comment

achow101 commented Aug 13, 2024

furszy left a comment •

edited

Loading

hebasto left a comment

hebasto Aug 13, 2024

hebasto left a comment

hebasto commented Aug 14, 2024

furszy left a comment

MarnixCroes Aug 14, 2024

pablomartin4btc left a comment

		void migrateWallet(WalletModel* wallet_model, QWidget* parent = nullptr);

Migrate legacy wallets that are not loaded #824

Migrate legacy wallets that are not loaded #824

Conversation

achow101 commented Jun 10, 2024

DrahtBot commented Jun 10, 2024 • edited Loading

Reviews

Conflicts

hebasto commented Jun 11, 2024

luke-jr commented Jun 12, 2024

achow101 commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Jul 14, 2024

hebasto commented Jul 29, 2024

pablomartin4btc left a comment • edited Loading

Choose a reason for hiding this comment

achow101 commented Aug 8, 2024 • edited Loading

pablomartin4btc commented Aug 8, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

achow101 commented Aug 13, 2024

furszy left a comment • edited Loading

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

hebasto commented Aug 14, 2024

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

DrahtBot commented Jun 10, 2024 •

edited

Loading

pablomartin4btc left a comment •

edited

Loading

achow101 commented Aug 8, 2024 •

edited

Loading

furszy left a comment •

edited

Loading