Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add menu option to migrate a wallet #738

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

achow101
Copy link
Member

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 13, 2023

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.

src/wallet/interfaces.cpp Outdated Show resolved Hide resolved
@hebasto
Copy link
Member

hebasto commented Jun 14, 2023

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

@maflcko
Copy link
Contributor

maflcko commented Jun 14, 2023

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
Copy link
Member

hebasto commented Jun 14, 2023

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
Copy link
Member

hebasto commented Jun 14, 2023

Concept ACK.

@hebasto
Copy link
Member

hebasto commented Jun 14, 2023

cc @furszy @ryanofsky @S3RK

src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved
@S3RK
Copy link
Contributor

S3RK commented Jun 19, 2023

Concept ACK

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

@kristapsk
Copy link
Contributor

Concept ACK

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, important feature

src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved
src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved
box.setWindowTitle(tr("Migrate wallet"));
box.setText(tr("Are you sure you wish to migrate the wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName())));
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"
"If this wallet contains any watchonly those scripts, a new wallet will be created which contains those watchonly scripts.\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved
src/qt/walletcontroller.cpp Show resolved Hide resolved
Copy link
Contributor

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/qt/walletcontroller.cpp Show resolved Hide resolved
AskPassphraseDialog has an optional parameter for the caller to get the
passphrase. Make this available for Unlocking.
@@ -423,6 +427,15 @@ struct WalletTxOut
bool is_spent = false;
};

//! Migrated wallet info
struct WalletMigrationResult
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
index 54eb720d0..d6a92cba7 100644
--- a/src/interfaces/wallet.h
+++ b/src/interfaces/wallet.h
@@ -428,8 +428,7 @@ struct WalletTxOut
 };
 
 //! Migrated wallet info
-struct WalletMigrationResult
-{
+struct WalletMigrationResult {
     std::unique_ptr<Wallet> wallet;
     std::optional<std::string> watchonly_wallet_name;
     std::optional<std::string> solvables_wallet_name;

"The migration process will create a backup of the wallet before migrating. This backup file will be named "
"<wallet name>-<timestamp>.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 \"Restore Wallet\" functionality."));
box.setStandardButtons(QMessageBox::Yes|QMessageBox::Cancel);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
box.setStandardButtons(QMessageBox::Yes|QMessageBox::Cancel);
box.setStandardButtons(QMessageBox::Yes | QMessageBox::Cancel);

QMessageBox box(m_parent_widget);
box.setWindowTitle(tr("Migrate wallet"));
box.setText(tr("Are you sure you wish to migrate the wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName())));
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

<message>
        <location line="+1"/>
        <source>Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.
If this wallet contains any watchonly scripts, a new wallet will be created which contains those watchonly scripts.
If this wallet contains any solvable but not watched scripts, a different and new wallet will be created which contains those scripts.

The 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>
        <translation type="unfinished"></translation>
</message>

Copy link

@D33r-Gee D33r-Gee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 48aae2c

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 DrahtBot removed the CI failed label Sep 4, 2023
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 48aae2c

Very nice feature.

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

image

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

image
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:

image

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

image

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

image

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

image

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

image

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:

image

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?

Comment on lines +170 to +172
if (m_passphrase_out) {
m_passphrase_out->assign(oldpass);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
if (m_passphrase_out) {
m_passphrase_out->assign(oldpass);
}
if (m_passphrase_out) m_passphrase_out->assign(oldpass);

Comment on lines +471 to +477
m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(res->wallet->getWalletName()));
if (res->watchonly_wallet_name) {
m_success_message += tr(" Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->watchonly_wallet_name.value()));
}
if (res->solvables_wallet_name) {
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()));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 48aae2c

@hebasto hebasto merged commit 1d4846a into bitcoin-core:master Sep 20, 2023
13 of 15 checks passed
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 25, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 26, 2023
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants