-
Notifications
You must be signed in to change notification settings - Fork 36.4k
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
gui: Create wallet menu option #15450
Conversation
bb8f840
to
63f1309
Compare
src/qt/createwalletdialog.cpp
Outdated
dialog->show(); | ||
|
||
// Create the wallet | ||
std::unique_ptr<interfaces::Wallet> wallet = m_wallet_controller->createWallet(wallet_name, flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is too difficult to do in this PR, but if it's possible to avoid creating the wallet in the GUI event loop thread and calling createWallet, setNewHdSeed, topUpKeyPool synchronously that would be a nice of allowing the GUI to avoid hangs and be more responsive. I believe @promag has some changes which use worker threads, so he might be able to give specific suggestions.
There may also be reason to avoid the new dlg.exec() call as in #15313.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with the GUI code so I think it would best to let someone else do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestion I have is to do similar to #15478. I think it's fine as a follow up.
Maybe:
|
63f1309
to
668aa51
Compare
Done
There are tooltips that explain this. Hover over the selections. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Concept ACK 🎉 |
Concept ACK, will pick this shortly and push a branch with some changes for your consideration. |
General Concept ACK |
Works! Couple of things, nothing that can't wait for a followup, but since this is still very early in the 0.19 cycle might as well get it right:
I agree with @jonasschnelli that being able to enter descriptors here would be useful, but I suggest holding off on that until we have a descriptor based wallet. Otherwise we have to explain that once the keypool runs out they somehow need to top it up. For the most part I expect descriptors would be part of a recovery workflow, which could be another menu item like Recover wallet. But they could also be an advanced setting during wallet creation, if someone wants to override our default derivation paths. |
Concept ACK. |
This fails a linter on Travis ("A new circular dependency in the form of "qt/bitcoingui -> qt/createwalletdialog -> qt/bitcoingui" appears to have been introduced."), and also fails to build when rebased on master, since @achow101 - do you intend to keep working on this? |
Yes, I'll rebase and have a look at it again soon. |
668aa51
to
767a5f8
Compare
Rebased. I'm not entirely sure how to get rid of the circular dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for rebasing @achow101!
I'm not entirely sure how to get rid of the circular dependency.
I think for now it's ok to remove the call to m_parent->setCurrentWallet(model);
in CreateWalletDialog::accept()
. That would allow you to not have an m_wallet
member variable, and change the CreateWalletDialog::CreateWalletDialog()
signature to take a QWidget*
instead of a BitcoinGUI*
.
This matches the existing 'Open Wallet' behaviour, which does not select the new wallet when it's opened.
src/qt/forms/createwalletdialog.ui
Outdated
<string>Wallet Name</string> | ||
</property> | ||
</widget> | ||
<widget class="QCheckBox" name="disable_privkeys_checkbox"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @Sjors that these two boxes should be hidden in an 'Advanced options' section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't figure out how to do this.
767a5f8
to
b2d0546
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now segfaults for me when I try to create an encryted wallet.
I think that https://github.com/bitcoin/bitcoin/pull/15450/files#r287152838 needs to be addressed. If not, perhaps this PR could drop support for creating encrypted wallets to begin with, and we add that later.
dc13d00
to
76bc7ae
Compare
I've made two major changes to this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK da8a86c - Haven't reviewed all of the Qt changes in depth. However I would like to see this in for v0.19.0
. If this is merged soon, then we've still got time to shake out any bugs and get follow up changes in.
Tested creating the different types of wallets.
Checked that when Encrypt Wallet
is selected Disable Private Keys
is disabled. Also checked that if Disable Private Keys
was checked before Encrypt Wallet
is selected it gets unchecked.
Looks like my auto-focusing suggestion might have been lost in a rebase?
However at this point it's definitely not a blocker for merging:
m_passphrase_dialog->setWindowModality(Qt::ApplicationModal);
m_passphrase_dialog->show();
This is at least one inconsistency where if you create a wallet with a HTML escaped character in the name, i.e Alice & Bob
, then the &
will not be shown in some menus / dialogs.
I agree with fanquake, I'll review+test tomorrow but I think I will merge it after that to give proper time for testing as mentioned. Please review before then everyone :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK da8a86c. Built, ran all tests, manually tested the gui wallet with Debian, reviewed the code.
Verified:
- when "Encrypt Wallet" is selected, "Disable Private Keys" is disabled
- when "Disable Private Keys" is checked and then "Encrypt Wallet" is selected, "Disable" is unchecked
I'm not sure what auto-focus issue @fanquake is seeing in #15450 (review). The focus behaves well in my testing. It begins automatically in the name field, and tabbing moves it successively through all the fields in the correct order.
if you create a wallet with a HTML escaped character in the name, i.e Alice & Bob,
then the & will not be shown in some menus / dialogs.
Saw the same issue on Debian. Among the special characters I entered, only the &
seemed to be affected.
A few comments follow. Feel free to ignore or add them to the to-do list for the future.
</rect> | ||
</property> | ||
<property name="toolTip"> | ||
<string>Encrypt the wallet. The wallet will be encrypted with a password of your choice.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/password/passphrase/ given that the dialog in question asks the user to input passphrases, not passwords
return ui->wallet_name_line_edit->text(); | ||
} | ||
|
||
bool CreateWalletDialog::encrypt() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it would be clearer to name this as encryptWallet
here and makeBlankWallet
on line 58, the way disablePrivateKeys
is named.
QString walletName() const; | ||
bool encrypt() const; | ||
bool disablePrivateKeys() const; | ||
bool blank() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: following my comment above, naming these encryptWallet
and makeBlankWallet
(like disablePrivateKeys
) seems more precise/coherent/would improve git grepping.
@@ -5,6 +5,8 @@ | |||
#ifndef BITCOIN_QT_GUICONSTANTS_H | |||
#define BITCOIN_QT_GUICONSTANTS_H | |||
|
|||
#include <cstdint> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding, why is this now needed?
} | ||
|
||
QTimer::singleShot(500, worker(), [this, name, flags] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: would be nice to hoist the 4 occurrences of this magic value (500) to a helpfully named constant.
@achow101 please rebase, here's the trivial diff from my PR: diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
index d0ffd6ba3..88ccc8c2d 100644
--- a/src/qt/walletcontroller.cpp
+++ b/src/qt/walletcontroller.cpp
@@ -160,10 +160,6 @@ void WalletControllerActivity::showProgressDialog(const QString& label_text)
m_progress_dialog->setCancelButton(nullptr);
m_progress_dialog->setWindowModality(Qt::ApplicationModal);
GUIUtil::PolishProgressDialog(m_progress_dialog);
-
- connect(m_progress_dialog, &QObject::destroyed, [this] {
- m_progress_dialog = nullptr;
- });
}
OpenWalletActivity::OpenWalletActivity(WalletController* wallet_controller, QWidget* parent_widget)
@@ -192,11 +188,11 @@ void OpenWalletActivity::open(const std::string& path)
showProgressDialog(tr("Opening Wallet <b>%1</b>...").arg(name.toHtmlEscaped()));
- QTimer::singleShot(500, worker(), [this, path] {
+ QTimer::singleShot(0, worker(), [this, path] {
std::unique_ptr<interfaces::Wallet> wallet = node().loadWallet(path, m_error_message, m_warning_message);
if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
- QTimer::singleShot(500, this, &OpenWalletActivity::finish);
+ QTimer::singleShot(0, this, &OpenWalletActivity::finish);
});
}
diff --git a/src/qt/walletcontroller.h b/src/qt/walletcontroller.h
index da8fde9f5..dada9cfa6 100644
--- a/src/qt/walletcontroller.h
+++ b/src/qt/walletcontroller.h
@@ -46,10 +46,11 @@ public:
//! Returns wallet models currently open.
std::vector<WalletModel*> getOpenWallets() const;
+ WalletModel* getOrCreateWallet(std::unique_ptr<interfaces::Wallet> wallet);
+
//! Returns all wallet names in the wallet dir mapped to whether the wallet
//! is loaded.
std::map<std::string, bool> listWalletDir() const;
- WalletModel* getOrCreateWallet(std::unique_ptr<interfaces::Wallet> wallet);
void closeWallet(WalletModel* wallet_model, QWidget* parent = nullptr);
|
Tested ACK on macOS 10.14.3 (rebased with my branch), played around, looks good to me.
@fanquake something to fix next. |
Co-authored-by: João Barbosa <[email protected]>
Co-authored-by: João Barbosa <[email protected]>
Co-authored-by: João Barbosa <[email protected]>
da8a86c
to
613de61
Compare
Rebased onto #16261 |
Concept and approach ACK |
ACK 613de61 - re-reviewed on macOS. I'm going to merge this now. It's had a stack of review, and as mentioned multiple times above, lets get this into Checked that the gui: Refactor OpenWalletActivity commit in this PR matches the one in #16261. So will close #16261 post merge here. Re-rested creating the different wallet types. Checked that when Checked that there still aren't any issues when compiling with
At least on macOS, if you're creating an encrypted wallet, after clicking Screenshots: Follow up for this PR include:
-----BEGIN PGP SIGNED MESSAGE----- ACK 613de61 - re-reviewed on macOS. I'm going to merge this now. It's had a stack of review, and as mentioned multiple times above, lets get this into iQIzBAEBCAAdFiEEz7FuIclQ9n+pXlWPLuufXMCVJsEFAl1zUgAACgkQLuufXMCV |
613de61 Add Create Wallet menu action (Andrew Chow) 9b41cbb Expose wallet creation to the GUI via WalletController (Andrew Chow) 78863e2 Add CreateWalletDialog to create wallets from the GUI (Andrew Chow) 60adb21 Optionally allow AskPassphraseDialog to output the passphrase (Andrew Chow) bc6d8a3 gui: Refactor OpenWalletActivity (João Barbosa) Pull request description: This PR adds a menu option to create a new wallet. When clicked, a `CreateWalletDialog` will be created and prompt the user to name the wallet and choose whether to disable private keys, make a blank wallet, and encrypt the wallet. If the wallet is encrypted, the wallet will be born encrypted with the wallet first created blank, then encrypted, and then a new HD seed generated and set. To allow the newly created wallets to be encrypted, some changes to how encrypting a wallet works. Instead of encrypting and locking the wallet, the wallet will be encrypted and then unlocked. This is also an extra belt-and-suspenders check to make sure that encryption worked. ACKs for top commit: fanquake: ACK 613de61 - re-reviewed on macOS. I'm going to merge this now. It's had a stack of review, and as mentioned multiple times above, lets get this into `master` so it can get more testing pre `v0.19.0`. Tree-SHA512: 3f22cc20b13703ffc90d366ae9133114832fea77f4f319da7fd85eb454f2f0bd5d7e1e6e20284dea2f370d8574f83b45669dcbbe506b994410d32e8e7a6fa877
cad3ab5 gui: fix autofocus in CreateWalletActivity::askPassphrase() (Jon Atack) 539d940 gui: fix passphrase labels/tooltip in createwalletdialog/askpassphrasedialog (Jon Atack) 43aa9b0 gui: rename encrypt(), blank(), and askPasshprase() (Jon Atack) Pull request description: Closes bitcoin#16820. The wallet [name escaping issue](bitcoin#15450 (review)) in that issue predates bitcoin#15450 and is fixed by bitcoin#16826. - [x] rename encrypt() to encryptWallet(), and blank() to makeBlankWallet() // EDIT: updated to isEncryptWalletChecked() isDisablePrivateKeysChecked() isMakeBlankWalletChecked() - [x] fix naming of askPasshprase() to askPassphrase() - [x] fix passphrase labels and tooltip in createwalletdialog.ui and askpassphrasedialog.ui - [x] fix grammar of labels in askpassphrase dialog and WalletController::closeWallet - [x] fix autofocus in CreateWalletActivity::askPassphrase() Squashed down to three commits. Reviewers, to test manually: build, launch the gui wallet, and look at labels/tooltips/focus with the create wallet, encrypt wallet, change password, and close wallet commands. ACKs for top commit: jb55: Approach ACK cad3ab5 instagibbs: code review and tACK cad3ab5 fanquake: ACK cad3ab5 Tree-SHA512: b441fbf8f8cd370dd692bac24f0d3c1b32fc7d947b6c3a2c9ba7cf0bc175a72b3460440f2f10f7632c0e8e0f8e65fe15615a30c46e2c7763bf258c504b457dd6
cad3ab5 gui: fix autofocus in CreateWalletActivity::askPassphrase() (Jon Atack) 539d940 gui: fix passphrase labels/tooltip in createwalletdialog/askpassphrasedialog (Jon Atack) 43aa9b0 gui: rename encrypt(), blank(), and askPasshprase() (Jon Atack) Pull request description: Closes bitcoin#16820. The wallet [name escaping issue](bitcoin#15450 (review)) in that issue predates bitcoin#15450 and is fixed by bitcoin#16826. - [x] rename encrypt() to encryptWallet(), and blank() to makeBlankWallet() // EDIT: updated to isEncryptWalletChecked() isDisablePrivateKeysChecked() isMakeBlankWalletChecked() - [x] fix naming of askPasshprase() to askPassphrase() - [x] fix passphrase labels and tooltip in createwalletdialog.ui and askpassphrasedialog.ui - [x] fix grammar of labels in askpassphrase dialog and WalletController::closeWallet - [x] fix autofocus in CreateWalletActivity::askPassphrase() Squashed down to three commits. Reviewers, to test manually: build, launch the gui wallet, and look at labels/tooltips/focus with the create wallet, encrypt wallet, change password, and close wallet commands. ACKs for top commit: jb55: Approach ACK cad3ab5 instagibbs: code review and tACK cad3ab5 fanquake: ACK cad3ab5 Tree-SHA512: b441fbf8f8cd370dd692bac24f0d3c1b32fc7d947b6c3a2c9ba7cf0bc175a72b3460440f2f10f7632c0e8e0f8e65fe15615a30c46e2c7763bf258c504b457dd6
Summary: bitcoin/bitcoin@bc6d8a3 Partial backport of Core [[bitcoin/bitcoin#15450 | PR15450]] depends on D7108 Test Plan: ninja check Open bitcoin-qt open and close wallets to make sure it works as before Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7102
…he passphrase Summary: bitcoin/bitcoin@60adb21 --- Depends on D7102 Partial backport of Core [[bitcoin/bitcoin#15450 | PR15450]] Test Plan: ninja check Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7103
…e GUI Summary: Co-authored-by: João Barbosa <[email protected]> bitcoin/bitcoin@78863e2 --- Depends on D7103 Partial backport of Core [[bitcoin/bitcoin#15450 | PR15450]] Test Plan: ninja check Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7104
…roller Summary: Co-authored-by: João Barbosa <[email protected]> bitcoin/bitcoin@9b41cbb --- Depends on D7104 Partial backport of Core [[bitcoin/bitcoin#15450 | PR15450]] Test Plan: ninja check Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7105
Summary: Co-authored-by: João Barbosa <[email protected]> bitcoin/bitcoin@613de61 --- Depends on D7105 Concludes backport of Core [[bitcoin/bitcoin#15450 | PR15450]] Test Plan: ninja check check-functional Used the bitcoin-qt wallet on regtest mode to create wallets and make sure behavior is expected Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7106
This PR adds a menu option to create a new wallet. When clicked, a
CreateWalletDialog
will be created and prompt the user to name the wallet and choose whether to disable private keys, make a blank wallet, and encrypt the wallet. If the wallet is encrypted, the wallet will be born encrypted with the wallet first created blank, then encrypted, and then a new HD seed generated and set.To allow the newly created wallets to be encrypted, some changes to how encrypting a wallet works. Instead of encrypting and locking the wallet, the wallet will be encrypted and then unlocked. This is also an extra belt-and-suspenders check to make sure that encryption worked.