Add menu option to migrate a wallet

achow101 commented at 7:06 pm on June 13, 2023: member

GUI users need to be able to migrate wallets without going to the RPC console.

DrahtBot commented at 7:06 pm on June 13, 2023: 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	jarolrod, pablomartin4btc, hebasto
Concept ACK	S3RK, kristapsk, johnny9, D33r-Gee

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label CI failed on Jun 13, 2023

in src/wallet/interfaces.cpp:645 in f7edbc5c30 outdated

640+            .wallet = MakeWallet(m_context, res->wallet),
641+            .watchonly_wallet_name = res->watchonly_wallet ? std::make_optional(res->watchonly_wallet->GetName()) : std::nullopt,
642+            .solvables_wallet_name = res->solvables_wallet ? std::make_optional(res->solvables_wallet->GetName()) : std::nullopt,
643+            .backup_path = res->backup_path,
644+        };
645+        return out;

maflcko commented at 9:16 pm on June 13, 2023:

0        return {std::move(out)}; // std::move to work around clang bug

No idea if this compiles or works around the clang bug, but I left a comment

achow101 commented at 11:05 pm on June 13, 2023:

Done as suggested

interfaces, wallet: Expose migrate wallet 5b3a85b4c6

achow101 force-pushed on Jun 13, 2023

hebasto added the label Wallet on Jun 14, 2023

hebasto commented at 9:30 am on June 14, 2023: member

Usually, we submit changes to non-qt subdirectory, i.e., the first commit of this PR, to the main repo, no?

maflcko commented at 9:38 am on June 14, 2023: contributor

Usually, we submit changes to non-qt subdirectory, i.e., the first commit of this PR, to the main repo, no?

I don’t think this rule applies to interface changes that only affect the gui. Otherwise, it just seems like extra work for reviewers and authors to artificially rip stuff apart for no good reason, with the extra risk of ending up merging just one ripped-off half, but not the other?

hebasto commented at 9:40 am on June 14, 2023: member

Usually, we submit changes to non-qt subdirectory, i.e., the first commit of this PR, to the main repo, no?

I don’t think this rule applies to interface changes that only affect the gui. Otherwise, it just seems like extra work for reviewers and authors to artificially rip stuff apart for no good reason, with the extra risk of ending up merging just one ripped-off half, but not the other?

I agree. It would be better to just document this case for further references.

hebasto commented at 9:40 am on June 14, 2023: member

Concept ACK.

hebasto commented at 9:41 am on June 14, 2023: member

cc @furszy @ryanofsky @S3RK

in src/qt/walletcontroller.cpp:442 in 06003512fb outdated

437+{
438+    // Warn the user about migration
439+    QMessageBox box(m_parent_widget);
440+    box.setWindowTitle(tr("Migrate wallet"));
441+    box.setText(tr("Are you sure you wish to migrate the wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName())));
442+    box.setInformativeText(tr("Migrating the wallet will convert this wallet to a descriptor wallet. A new wallet backup will need to be made.\n"

S3RK commented at 7:30 am on June 19, 2023:

.. convert this wallet to one or more descriptor wallets

achow101 commented at 3:23 pm on June 19, 2023:

Done as suggested.

S3RK commented at 7:31 am on June 19, 2023: contributor

Concept ACK

Not qualified to review GUI code, but agree it’s a good idea to show migration option in GUI.

achow101 force-pushed on Jun 19, 2023

DrahtBot removed the label CI failed on Jun 19, 2023

kristapsk commented at 6:24 pm on June 19, 2023: contributor

Concept ACK

in src/qt/walletcontroller.cpp:443 in c574e72f0a outdated

438+    // Warn the user about migration
439+    QMessageBox box(m_parent_widget);
440+    box.setWindowTitle(tr("Migrate wallet"));
441+    box.setText(tr("Are you sure you wish to migrate the wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName())));
442+    box.setInformativeText(tr("Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.\n"
443+                "If this wallet contains any watchonly those scripts, a new wallet will be created which contains those watchonly scripts.\n"

jarolrod commented at 10:52 pm on June 22, 2023:

🤗

0                "If this wallet contains any watchonly scripts, a new wallet will be created which contains those watchonly scripts.\n"

jarolrod commented at 11:01 pm on June 22, 2023:

I do think this is quite wordy, and maybe more information than is needed. But, no need to reduce the amount of information here.

I know that for some users, they continue to use the Bitcoin Core GUI because their wallet was generated with it in some early version, and they are scared to use anything else but that. So, migrating can be a scary task.

When implementing this functionality for the gui-qml we’d probably make this less intimidating, perhaps as a wizard, and with more interaction/feedback with the process. CC @GBKS @mouxdesign

Screen Shot 2023-06-22 at 6 55 11 PM

achow101 commented at 6:24 pm on June 23, 2023:

Done

in src/qt/walletcontroller.cpp:444 in c574e72f0a outdated

439+    QMessageBox box(m_parent_widget);
440+    box.setWindowTitle(tr("Migrate wallet"));
441+    box.setText(tr("Are you sure you wish to migrate the wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName())));
442+    box.setInformativeText(tr("Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.\n"
443+                "If this wallet contains any watchonly those scripts, a new wallet will be created which contains those watchonly scripts.\n"
444+                "If this wallet contains any solvable but not watched scripts, a different new wallet will be created which contains those scripts.\n\n"

jarolrod commented at 10:54 pm on June 22, 2023:

maybe?

0                "If this wallet contains any solvable but not watched scripts, a different and new wallet will be created which contains those scripts.\n\n"

achow101 commented at 6:24 pm on June 23, 2023:

Done

in src/qt/walletcontroller.cpp:447 in c574e72f0a outdated

442+    box.setInformativeText(tr("Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.\n"
443+                "If this wallet contains any watchonly those scripts, a new wallet will be created which contains those watchonly scripts.\n"
444+                "If this wallet contains any solvable but not watched scripts, a different new wallet will be created which contains those scripts.\n\n"
445+                "The migration process will create a backup of the wallet before migrating. This backup file will be named "
446+                "<wallet name>-<timestamp>.legacy.bak and can be found in the directory for this wallet. In the event of "
447+                "an incorrect migration, the backup can be restored with the Restore Wallet functionality."));

jarolrod commented at 11:06 pm on June 22, 2023:

maybe? for legibility.

0                "an incorrect migration, the backup can be restored with the \"Restore Wallet\" functionality."));

achow101 commented at 6:24 pm on June 23, 2023:

Done

in src/qt/walletcontroller.cpp:454 in c574e72f0a outdated

449+    box.setDefaultButton(QMessageBox::Yes);
450+    if (box.exec() != QMessageBox::Yes) return;
451+
452+    // Get the passphrase if it is encrypted regardless of it is locked or unlocked. We need the passphrase itself.
453+    SecureString passphrase;
454+    WalletModel::EncryptionStatus enc_status = wallet_model->getEncryptionStatus();

jarolrod commented at 11:16 pm on June 22, 2023:

This seems to always fail and the wallet gets removed, or am I the only one?

cc @johnny9 @promag

johnny9 commented at 1:58 am on June 23, 2023:

I am unable to migrate an encrypted wallet as well. The error message that appears for me is “Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect”

achow101 commented at 6:25 pm on June 23, 2023:

Fixed. Apparently AskpassphraseDialog didn’t set m_passphrase_out for Unlock. I’ve added a commit to do that and migration of encrypted wallets now works.

jarolrod commented at 11:17 pm on June 22, 2023: member

Concept ACK, important feature

johnny9 commented at 2:00 am on June 23, 2023: contributor

Concept ACK. Very useful feature.

Wallet migration doesn’t work for me if I have a legacy sqlite wallet. Unsure if the assumption is that this only works with a bdb legacy wallet.

gui: Optionally return passphrase after unlocking

AskPassphraseDialog has an optional parameter for the caller to get the
passphrase. Make this available for Unlocking.

577be889cd

gui: Add File > Migrate Wallet 48aae2cffe

achow101 force-pushed on Jun 23, 2023

in src/interfaces/wallet.h:432 in 48aae2cffe

426@@ -423,6 +427,15 @@ struct WalletTxOut
427     bool is_spent = false;
428 };
429 
430+//! Migrated wallet info
431+struct WalletMigrationResult
432+{

jarolrod commented at 2:00 am on June 24, 2023:

we seem to use this style now, although it’s not officially documented, so only nit

 0diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
 1index 54eb720d0..d6a92cba7 100644
 2--- a/src/interfaces/wallet.h
 3+++ b/src/interfaces/wallet.h
 4@@ -428,8 +428,7 @@ struct WalletTxOut
 5 };
 6 
 7 //! Migrated wallet info
 8-struct WalletMigrationResult
 9-{
10+struct WalletMigrationResult {
11     std::unique_ptr<Wallet> wallet;
12     std::optional<std::string> watchonly_wallet_name;
13     std::optional<std::string> solvables_wallet_name;

in src/qt/walletcontroller.cpp:448 in 48aae2cffe

443+                "If this wallet contains any watchonly scripts, a new wallet will be created which contains those watchonly scripts.\n"
444+                "If this wallet contains any solvable but not watched scripts, a different and new wallet will be created which contains those scripts.\n\n"
445+                "The migration process will create a backup of the wallet before migrating. This backup file will be named "
446+                "<wallet name>-<timestamp>.legacy.bak and can be found in the directory for this wallet. In the event of "
447+                "an incorrect migration, the backup can be restored with the \"Restore Wallet\" functionality."));
448+    box.setStandardButtons(QMessageBox::Yes|QMessageBox::Cancel);

jarolrod commented at 2:10 am on June 24, 2023:

nit

0    box.setStandardButtons(QMessageBox::Yes | QMessageBox::Cancel);

in src/qt/walletcontroller.cpp:442 in 48aae2cffe

437+{
438+    // Warn the user about migration
439+    QMessageBox box(m_parent_widget);
440+    box.setWindowTitle(tr("Migrate wallet"));
441+    box.setText(tr("Are you sure you wish to migrate the wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName())));
442+    box.setInformativeText(tr("Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.\n"

jarolrod commented at 3:36 am on June 24, 2023:

Confirmed that all the new strings introduced here are nicely transferred over to be translated after running make translate

While not a blocker; the longer I look at this, the more I think it can be slimmed down. Will have another look tomorrow.

At least this is a chunky boy to translate, and will need a good translator comment:

0<message>
1        <location line="+1"/>
2        <source>Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.
3If this wallet contains any watchonly scripts, a new wallet will be created which contains those watchonly scripts.
4If this wallet contains any solvable but not watched scripts, a different and new wallet will be created which contains those scripts.
5
6The migration process will create a backup of the wallet before migrating. This backup file will be named &lt;wallet name&gt;-&lt;timestamp&gt;.legacy.bak and can be found in the directory for this wallet. In the event of an incorrect migration, the backup can be restored with the &quot;Restore Wallet&quot; functionality.</source>
7        <translation type="unfinished"></translation>
8</message>

D33r-Gee commented at 5:04 pm on June 26, 2023: none

tested ACK on WSL Ubuntu 22.04 Migrated a few test wallets using the GUI on regtest, both encrypted and not-encrypted wallet yelled successful results.

jarolrod approved

jarolrod commented at 5:29 am on July 5, 2023: member

ACK 48aae2cffeb91add75a70ac4d5075c38054452fa

This introduces the functionality as promised, works well, and didn’t have any issues with legacy wallets + watch only addrs.

Improving the text and it’s translatability to can occur in a follow-up.

DrahtBot added the label CI failed on Aug 17, 2023

DrahtBot removed the label CI failed on Aug 22, 2023

DrahtBot added the label CI failed on Aug 31, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

in src/qt/askpassphrasedialog.cpp:172 in 48aae2cffe

166@@ -167,6 +167,9 @@ void AskPassphraseDialog::accept()
167                                              "passphrase to avoid this issue in the future."));
168                 }
169             } else {
170+                if (m_passphrase_out) {
171+                    m_passphrase_out->assign(oldpass);
172+                }

pablomartin4btc commented at 2:40 pm on September 20, 2023:

nit:

0                if (m_passphrase_out) m_passphrase_out->assign(oldpass);

in src/qt/walletcontroller.cpp:477 in 48aae2cffe

472+            if (res->watchonly_wallet_name) {
473+                m_success_message += tr(" Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->watchonly_wallet_name.value()));
474+            }
475+            if (res->solvables_wallet_name) {
476+                m_success_message += tr(" Solvable but not watched scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->solvables_wallet_name.value()));
477+            }

pablomartin4btc commented at 6:17 pm on September 20, 2023:

Perhaps we can refactor this piece a bit, maybe extracting the message building to other functions?

pablomartin4btc commented at 6:18 pm on September 20, 2023: contributor

tACK 48aae2cffeb91add75a70ac4d5075c38054452fa

Very nice feature.

The menu option is disabled correctly if the selected wallet is not a Legacy one:

I had to ./configure --with-dbd in order to create a Legacy wallet first:

Not related with this PR: Do I need to use an older version to make it “portable”? It’s referring to the backup of a legacy wallet or the copy of the files?

And ofc this is something we already saw in previous versions of bitcoin-qt before the “Descriptor Wallet” became checked by default and disabled so user is forced to use descriptor instead of legacy wallets:

Once a Legacy wallet is selected we can see the new menu option “Migrate Wallet”:

Regarding the long message, perhaps can be restructured a bit with some bullets points?

Perhaps as a follow-up we can show a proper progress being updated (we have already these open issues related with that: #651, #679):

It works as expected and I was able to restore the migrated wallet successfully.

One more detail I noticed is that the migration process ends up saving a .bak extension file, when I wanted to try the restore of it, I had to change that extension to .dat, otherwise it won’t be seen by the open/ restore file dialog as there’s only one type of file possible in the bottom right combo-box:

Should we add another type there or perhaps set the migrated file extension as .dat too? The filename itself is automatically defined, should we give the user the option to name it instead?

hebasto approved

hebasto commented at 9:23 pm on September 20, 2023: member

ACK 48aae2cffeb91add75a70ac4d5075c38054452fa

hebasto merged this on Sep 20, 2023

hebasto closed this on Sep 20, 2023

Frank-GER referenced this in commit 64ed740917 on Sep 25, 2023

sidhujag referenced this in commit 56be2155d2 on Sep 26, 2023

bitcoin-core locked this on Sep 19, 2024

Add menu option to migrate a wallet #738

Reviews

Conflicts