Migrate legacy wallets that are not loaded

achow101 commented at 8:43 pm on June 10, 2024: member

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 commented at 8:43 pm on June 10, 2024: 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
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.

achow101 added this to the milestone 28.0 on Jun 10, 2024

achow101 removed this from the milestone 28.0 on Jun 10, 2024

achow101 force-pushed on Jun 10, 2024

hebasto renamed this:
~~gui: Migrate legacy wallets that are not loaded~~
Migrate legacy wallets that are not loaded
on Jun 11, 2024

hebasto commented at 7:14 am on June 11, 2024: member

Concept ACK.

luke-jr commented at 8:32 pm on June 12, 2024: member

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 commented at 9:24 pm on June 12, 2024: member

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.

achow101 force-pushed on Jul 11, 2024

in src/qt/walletcontroller.h:70 in d45ac03a89 outdated

67 
68     void closeWallet(WalletModel* wallet_model, QWidget* parent = nullptr);
69     void closeAllWallets(QWidget* parent = nullptr);
70 
71-    void migrateWallet(WalletModel* wallet_model, QWidget* parent = nullptr);
72-

hebasto commented at 10:44 am on July 14, 2024:

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

in src/qt/bitcoingui.cpp:472 in d45ac03a89 outdated

467+                const auto& [loaded, format] = info;
468+                QString name = GUIUtil::WalletDisplayName(wallet_name);
469+                // Menu items remove single &. Single & are shown when && is in
470+                // the string, but only the first occurrence. So replace only
471+                // the first & with &&.
472+                name.replace(name.indexOf(QChar('&')), 1, QString("&&"));

hebasto commented at 11:02 am on July 14, 2024:

#828 can relevant to this code.

pablomartin4btc commented at 10:57 am on July 31, 2024:

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

0                // Single & are shown when && is in the string. So replace & with &&.
1                name.replace(QChar('&'), QString("&&"));

achow101 commented at 2:44 pm on August 8, 2024:

Fixed during rebase.

hebasto commented at 8:48 pm on August 13, 2024:

Fixed during rebase.

It still needs to be fixed.

achow101 commented at 11:18 pm on August 13, 2024:

Ah, missed one, fixed.

in src/qt/askpassphrasedialog.h:29 in d45ac03a89 outdated

25@@ -26,6 +26,7 @@ class AskPassphraseDialog : public QDialog
26         Encrypt,    /**< Ask passphrase twice and encrypt */
27         Unlock,     /**< Ask passphrase and unlock */
28         ChangePass, /**< Ask old passphrase + new passphrase twice */
29+        UnlockMigration, // Ask passphrase for unlocking during migration

hebasto commented at 11:03 am on July 14, 2024:

Mind following the Doxygen comment formatting above?

achow101 commented at 2:44 pm on August 8, 2024:

Done

in src/interfaces/wallet.h:49 in d45ac03a89 outdated

45@@ -46,6 +46,7 @@ enum isminetype : unsigned int;
46 struct CRecipient;
47 struct WalletContext;
48 using isminefilter = std::underlying_type<isminetype>::type;
49+enum class DatabaseFormat;

hebasto commented at 11:06 am on July 14, 2024:

Why is this line needed?

achow101 commented at 2:44 pm on August 8, 2024:

Removed

hebasto commented at 11:26 am on July 14, 2024: member

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 d45ac03a89cc500cd461f878f092cf8ec99e7760. The “Migrate Wallet” menu item is still disabled when no wallet is loaded.

hebasto commented at 9:42 am on July 29, 2024: member

Please rebase.

DrahtBot added the label Needs rebase on Jul 29, 2024

pablomartin4btc commented at 9:42 pm on July 31, 2024: contributor

tACK d45ac03a89cc500cd461f878f092cf8ec99e7760

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

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

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

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

DrahtBot requested review from hebasto on Jul 31, 2024

achow101 added this to the milestone 28.0 on Aug 8, 2024

achow101 force-pushed on Aug 8, 2024

achow101 commented at 2:44 pm on August 8, 2024: member

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.

DrahtBot removed the label Needs rebase on Aug 8, 2024

pablomartin4btc commented at 4:04 pm on August 8, 2024: contributor

“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 approved

pablomartin4btc commented at 4:31 pm on August 9, 2024: contributor

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

Should be fixed now.

re-ACK bcf47b9f34e4f21e5ac75521ddd4f25ff3812412

in src/qt/bitcoingui.cpp:466 in bcf47b9f34 outdated

473+
474+                if (format != "bdb") {
475+                    // This wallet is already migrated
476+                    action->setEnabled(false);
477+                    continue;
478+                }

furszy commented at 4:41 pm on August 11, 2024:

I don’t think showing the already migrated wallets here adds any value. The user cannot do anything with them and they do not present any extra information. I would skip them:

 0diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 1--- a/src/qt/bitcoingui.cpp
 2+++ b/src/qt/bitcoingui.cpp
 3@@ -460,6 +460,10 @@
 4             m_migrate_wallet_menu->clear();
 5             for (const auto& [wallet_name, info] : m_wallet_controller->listWalletDir()) {
 6                 const auto& [loaded, format] = info;
 7+                if (format != "bdb") { // skip already migrated wallets
 8+                    continue;
 9+                }
10+
11                 QString name = GUIUtil::WalletDisplayName(wallet_name);
12                 // Menu items remove single &. Single & are shown when && is in
13                 // the string, but only the first occurrence. So replace only
14@@ -467,12 +471,6 @@
15                 name.replace(name.indexOf(QChar('&')), 1, QString("&&"));
16                 QAction* action = m_migrate_wallet_menu->addAction(name);
17-
18-                if (format != "bdb") {
19-                    // This wallet is already migrated
20-                    action->setEnabled(false);
21-                    continue;
22-                }
23-
24                 connect(action, &QAction::triggered, [this, wallet_name] {
25                     auto activity = new MigrateWalletActivity(m_wallet_controller, this);
26                     connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet);

achow101 commented at 5:20 pm on August 12, 2024:

Done

in src/qt/bitcoingui.cpp:477 in bcf47b9f34 outdated

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

furszy commented at 2:05 pm on August 12, 2024:

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.

achow101 commented at 5:08 pm on August 12, 2024:

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

furszy commented at 1:17 pm on August 13, 2024:

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

clang 14.0.3 (clang-1403.0.22.14.1).

in src/qt/walletcontroller.cpp:478 in 2c116ea99f outdated

487     // GUI needs to remove the wallet so that it can actually be unloaded by migration
488-    const std::string name = wallet_model->wallet().getWalletName();
489-    m_wallet_controller->removeAndDeleteWallet(wallet_model);
490+    if (WalletModel* wallet = m_wallet_controller->getWallet(name)) {
491+        m_wallet_controller->removeAndDeleteWallet(wallet);
492+    };

furszy commented at 3:22 pm on August 12, 2024:

We don’t actually need to do this. Could cherry-pick https://github.com/furszy/bitcoin-core/commit/f5c0c58977a8989fe17db361d179281d051b0806 and remove the introduced getWallet function.

achow101 commented at 5:21 pm on August 12, 2024:

Cherry picked.

furszy commented at 3:33 pm on August 12, 2024: member

Few comments. Will continue reviewing.

DrahtBot added the label Needs rebase on Aug 12, 2024

achow101 force-pushed on Aug 12, 2024

DrahtBot removed the label Needs rebase on Aug 12, 2024

DrahtBot added the label CI failed on Aug 12, 2024

furszy commented at 2:29 pm on August 13, 2024: member

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 https://github.com/furszy/bitcoin-core/commit/e8d147eeaacac7d434aad8dbbc5022aadf7d112a. It detects the wallet encryption status automatically regardless of its loaded/unloaded state.

Other than this, code is looking good.

achow101 force-pushed on Aug 13, 2024

wallet, interfaces: Include database format in listWalletDir 28fc562f26

gui: Consolidate wallet display name to GUIUtil function

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

bfba63880f

gui: don't remove wallet manually before migration c3918583dd

gui: Use wallet name for wallet migration rather than WalletModel

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 <matiasfurszyfer@protonmail.com>

d56a450bf5

achow101 force-pushed on Aug 13, 2024

achow101 commented at 3:26 pm on August 13, 2024: member

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.

DrahtBot removed the label CI failed on Aug 13, 2024

furszy commented at 7:33 pm on August 13, 2024: member

ACK fd087d4

DrahtBot requested review from pablomartin4btc on Aug 13, 2024

hebasto commented at 8:48 pm on August 13, 2024: member

Almost ACK fd087d4667b94051d00617189dcecc4f88d849ce.

gui: Use menu for wallet migration

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.

8f2522d242

achow101 force-pushed on Aug 13, 2024

hebasto approved

hebasto commented at 9:11 am on August 14, 2024: member

ACK 8f2522d242961ceb9e79672aa43e856863a1a6dd.

DrahtBot requested review from furszy on Aug 14, 2024

hebasto commented at 9:11 am on August 14, 2024: member

cc @furszy

furszy commented at 12:55 pm on August 14, 2024: member

ACK 8f2522d

hebasto merged this on Aug 14, 2024

hebasto closed this on Aug 14, 2024

hebasto added the label Wallet on Aug 14, 2024

in src/qt/bitcoingui.cpp:481 in 8f2522d242

480+                    connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet);
481+                    activity->migrate(wallet_name);
482+                });
483+            }
484+            if (m_migrate_wallet_menu->isEmpty()) {
485+                QAction* action = m_migrate_wallet_menu->addAction(tr("No wallets available"));

MarnixCroes commented at 4:19 pm on August 14, 2024:

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 commented at 3:37 pm on August 15, 2024: contributor

post-merge tACK 8f2522d242961ceb9e79672aa43e856863a1a6dd

Checked changes proposed by @furszy.

Migrate legacy wallets that are not loaded #824

Reviews

Conflicts