-
Notifications
You must be signed in to change notification settings - Fork 269
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
Slight improve create wallet dialog #96
Conversation
Concept ACK, seems much better. I'm probably missing something, but why "blank wallet" instead of "watchonly"? |
A blank wallet has no keys at all, but you can add private keys to it. A watch-only wallet has no private keys. A watch-only wallet starts out blank, and then you can't add private keys to it. It's confusing, and probably deserves an even more obscure place in the UI :-) |
Thanks. Will test. |
Good changes. Could "Encrypt wallet" also be "Protect wallet with a password"? Would it be possible to add links to resources that explain the advanced options to more easily read up on what they do? |
Concept ACK, could you not call 'disable private keys' watch-only wallet? It's both technically correct and I'd assume is a more familiar term to users.
Wouldn't Encrypt private keys be more accurate? The wallet isn't being encrypted only the private keys from my understanding - though I think wallet contents a long with meta data like tx history should also be encrypted at some point (again, this is also a familiar process to many users). If this change is made Encrypt wallet or protect wallet with a password is more suitable. Relevant discussion bitcoin/bitcoin#18085 |
b0e58cd
to
e84e07c
Compare
I updated same labels and tooltips. |
Concept ACK. I like the idea of separating out the confusing and less-often-needed features into an advanced section. |
Concept ACK |
|
||
// Wallets without private keys start out blank | ||
if (checked) { | ||
ui->blank_wallet_checkbox->setChecked(true); |
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.
Should also disable blank_wallet_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.
Neh, it gets too tedious. It doesn't matter what you do with this checkbox, it's ignored for watch-only.
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 am only following the discussion here, therefore I might miss something. First of all, I like the separation into advanced features, but I don't like the idea to disable the encryption by default and would prefer a "fail safe" strategy here. Both decisions have their pro and cons, I would at least explain why encryption was the default in the first place. Another minor question: Why |
I'm wondering if "Manually import seed or keys" would be a more helpful label since that is what the user wants to do (Skipping the seed generation is what allows them to import a seed or keys, but it's not their actual goal)? What do you think? Otherwise, looks great. |
FYI, no released version of Bitcoin Core has ever created encrypted wallets by default; this PR just preserves that legacy. As background, there's an open question between experts about whether or not the use of wallet encryption in typical user wallets saves more money than it loses. It saves money if someone gets a hold of just your wallet file (if they get direct access to your computer, they can observe your passphrase and steal your funds any way). It loses money if you forget your passphrase. Some experts believe the number of occasions where a wallet file has fallen into a attacker's hands without that attacker getting direct access to the user's computer is small compared to the large number of occasions where a user has forgotten their passphrase, so it's on average safer not to use encryption. (I don't hold a strong opinion here myself. I assume that any bitcoins stored in my hot wallet will be stolen someday and prefer leaving my wallet unencrypted so I'm likely to learn about the theft as early as possible.) I also like @GBKS's suggested label rename to |
If it's anything to go by I've received way more sob stories about people losing their wallet passphrase than about stolen funds, regarding bitcoin core. I'm not sure that's a reliable measure, but because of this I'm partial to not encrypting by default. |
I agree with not encrypting by default. Hot wallets should only be used for small amounts and should not warrant the extra security complexity. An external signer (coming soon to the GUI #4) should be used for larger amounts at a minimum. Though what consists of a larger amount will vary from person to person. The various security levels and trade offs of each wallet type should be communicated to users clearly (Getting there #77). |
@joernroeder the default for the (only recently introduced) wallet creation dialog was to enable encryption. But the default wallet that Bitcoin Core creates for new users has encryption disabled. Until recently the wallet creation dialog was only used by people creating a second wallet, which presumable is a more experienced subset of users. I'll keep the |
Which one? 😃 |
560236a
to
ad0874e
Compare
Right... rebased and changed the label. |
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 ad0874e, tested on Linux Mint 20 (x86_64, Qt 5.12.8).
For the nice commit history mind squashing commits "gui: create wallet: add advanced section" and "gui: rename duplicate label in createwalletdialog.ui"?
src/qt/forms/createwalletdialog.ui
Outdated
</property> | ||
<property name="text"> | ||
<string>Descriptor Wallet</string> | ||
<string>Use descriptors</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.
The word "wallet" seems redundant. Also, you can mix and match, e.g. a watch-only descriptor wallet (not a watch-only wallet descriptor wallet)
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.
Watch-only implies blank (checkbox enforces that), other than that there's no restriction. See also the createwallet
RPC documentation, which this is dumb mimic of. These features are almost impossible to use without a deep understanding of the wallet or a tutorial telling you what to do, I don't expect any on screen text to fix that.
The main point of this PR is to make v0.21 slightly more usable than it is now, because we no longer create a default wallet and every new users has to plough through this dialog. I'd rather not fine tune this too much, that's what #77 is for.
src/qt/forms/createwalletdialog.ui
Outdated
</property> | ||
<property name="text"> | ||
<string>Make Blank Wallet</string> | ||
<string>Manually import seed or keys</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.
This change seems confusing, even really misleading (and wasn't in your original proposal), nothing is being imported at this step.. "Blank" or "Skip..." along with the previous tooltip was more clear IMO. Where there is doubt as to the change to make, it's better to not change it.
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.
Anyone using this feature will probably be following a tutorial, so I don't think we should bikeshed this to death. I can see a followup design improvement adding a wizard where you can copy-paste descriptors at this step.
Right, and it was intentionally disabled in bitcoin/bitcoin#15454 and 78863e2 from bitcoin/bitcoin#15450. The new commit 03ebe1b removes the code which disables it. I don't think the change is a good idea and wrote some reasons why. Would be happy to discuss more, but the point is this change goes beyond tweaking the dialog and shouldn't be part of this PR |
bitcoin/bitcoin#15450 introduced the ability to add additional wallets in the GUI, basically an advanced feature. It wasn't designed to be part of the onboarding flow. It makes sense that it didn't allow a blank wallet name, because it would never be used to create the initial wallet. The PR description of bitcoin/bitcoin#15454 says (emphasis mine):
It wasn't clear to me that intention of that PR was to get rid of the unnamed wallet entirely. It seemed more like an oversight to me. That said, I don't object to ditching the unnamed wallet. But it does create the annoying problem of the user having to pick a name. I can drop the last commit (or the maintainer can just omit it from the merge) if needed. |
PR title "Slight improve create wallet dialog" title is currently misleading. Right now this PR is also adding back ability to create a top-level unnamed wallet containing other wallets from the GUI that was removed in #15454 (definitely intentional for me, though I can't speak for other participants). I don't think commit 03ebe1b is a good idea. I think it is confusing for a wallet to contain other wallets. I think wallets that are unnamed are more likely to get lost and not backed up properly than wallets that have recognizable names. I think if the last commit 03ebe1b is desirable at all, it should be a standalone PR, which should have some rationale stating why it is desirable. It shouldn't be tacked on to a PR doing other cleanup as an extra commit with no rationale or description. Even from discussion in this PR, I haven't seen any justification of the commit, except as workaround to avoid adding a translation string. It seems fine to just drop the placeholder string if we can't have a translation now and don't want an untranslated string. I think commit 03ebe1b is beyond scope of this PR. |
Tested ACK d393708 "gui: create wallet: add advanced section" with the following reservations:
For now or later, I like these two changes.
Had the same thought. Maybe "My Wallet". |
</rect> | ||
</property> | ||
<property name="styleSheet"> | ||
<string notr="true">font-weight:bold;</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.
(QT newb question: what is notr
?)
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.
Sorry, should have looked it up. Per https://doc.qt.io/qt-5/designer-creating-custom-widgets.html, if the notr
optional attribute is true, the value is not meant to be translated.
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 just copy-pasted that from another ui file :-)
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.
string is not translated :)
03ebe1b
to
ac64cec
Compare
@ryanofsky I still don't understand what you mean with "wallet to contain other wallets". It's just one wallet. I dropped the last commit. To the degree it wasn't clear whether disallowing the I changed "MyWallet" to "Wallet" to avoid CamelCase and at the same avoid spaces in directory names, which tend to trip up badly written backup commands. For future versions we can translate it, but I suggest hinting to the translators to avoid spaces. Unicode characters (e.g. Angličtina) are probably fine, because afaik we have tests for those. But Arab seems to have a space: محفظة نقود I don't think dropping the placeholder completely, yet not allowing a blank name, is a good idea: users won't know why Create is disabled and they'll probably start selecting random options. (that might still happen, but the placeholder gives at least some hint that picking a name is mandatory) Of course it's just a placeholder text so the user might type something completely different. I'm still not a fan of forcing users to pick a name, but agree it should be out of scope for this PR.
It's similar to the pre-existing behaviour of clicking on Encrypt wallet. Explaining the behaviour better makes sense, but since I can't change the strings, that would be for a followup (I also don't want to risk whack-a-mole by touching window size, though that's probably safe). |
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.
re-ACK ac64cec
Two new strings are added after translation freeze:
gui/src/qt/forms/createwalletdialog.ui
Line 42 in ac64cec
<string>Wallet</string> gui/src/qt/forms/createwalletdialog.ui
Line 90 in ac64cec
<string>Advanced options</string>
I'm ok with them though, as the main goal--to disable wallet encryption by default--could be achieved in 0.21.
FYI, no released version of Bitcoin Core has ever created encrypted wallets by default; this PR just preserves that legacy.
I agree that implementation details could be improved (#96 (comment)) in a followup.
@Sjors
I apologize for the delay in the review.
Checking Currently, you are left with settings: If we are trying to fit this in 0.21 I guess these can be fixed in another PR |
Tested ACK ac64cec Tested on MacOS 10.15.7. Seems like a great improvement! |
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.
Code review ACK ac64cec. Only changes since last review are tweaking placeholder text and dropping "allow nameless" commit
@ryanofsky I still don't understand what you mean with "wallet to contain other wallets". It's just one wallet.
Sorry, didn't see earlier question. This is just referring to one wallet directory containing another wallet directory, e.g:
wallets/wallet.dat
wallets/db.log
wallets/wallet2/wallet.dat
wallets/wallet2/db.log
instead of
wallets/wallet1/wallet.dat
wallets/wallet1/db.log
wallets/wallet2/wallet.dat
wallets/wallet2/db.log
re-ACK ac64cec, per
|
Agree, I'm seeing the same. And also, in signet the wallets do not even have their own |
That would be very odd, maybe worth a seperate github issue. Did you start with pre-existing |
Thanks for circling back on that. I tested several different versions of the signet implementation PR before it was merged, but also rm -rf'ed that dir as many times. I guess I should start once again with a fresh signet dir to check. |
Thanks for the merge! I'm leaving the text changes followup up for grabs. Might be worth adding a checkbox for the "avoid reuse" flag too, but it will need some explanation. |
ac64cec gui: create wallet: add advanced section (Sjors Provoost) c99d6f6 gui: create wallet: name placeholder (Sjors Provoost) 5bff825 [gui] create wallet: smarter checkbox toggling (Sjors Provoost) Pull request description: Previously only users who needed a second wallet had to use to the create wallet dialog. With the merge of bitcoin#15454 now all new users have to. I don't think it was user-friendly enough for that. <img width="403" alt="Schermafbeelding 2020-09-18 om 09 41 44" src="https://user-images.githubusercontent.com/10217/93574129-52ef9680-f998-11ea-9a6f-31144f66d3bf.png"> This PR makes a few simple improvements so that new users don't have to think too much: <img width="369" alt="Schermafbeelding 2020-10-15 om 16 45 22" src="https://user-images.githubusercontent.com/10217/96145959-0c914700-0f06-11eb-9526-cf447d841d7a.png"> It's lightly inspired by #77. It would be better if those changes made it into the upcoming release, but this PR is a good start imo. * wallet encryption is no longer checked by default, because such a change in the default needs a separate discussion (fwiw, I suspect it increases the number of users losing access to coins) * watch-only and descriptor wallet stuff is moved to advanced, so new users know they can safely ignore these check boxes * bonus: when you click on "disable private keys" it disables encrypt wallet and checks blank wallet * label changes: see screenshot * tooltip changes: see code diff Note that a blank wallet name isn't allowed in the dialog; I haven't addressed that. _Update 2020-10-30_, dropped the new strings for now: <img width="450" alt="Schermafbeelding 2020-10-30 om 11 26 55" src="https://user-images.githubusercontent.com/10217/97694591-1b99fc80-1aa3-11eb-8b85-e19f1ad5add4.png"> ACKs for top commit: fjahr: Tested ACK ac64cec jonatack: re-ACK ac64cec, per `git diff d393708 ac64cec` only change since my last review is improving the placeholder from "MyWallet" to "Wallet" and dropping the last commit. Tested creating a dozen wallets in signet with different combinations of options and then verifying/comparing their characteristics in the console with getwalletinfo. My remaining caveats are (1) the need for less user surprise by either (a) improving the user info or (b) with less auto-(un)selecting as mentioned in bitcoin-core/gui#96 (comment) and (2) I prefer the "Encrypt private keys" and "Watch-only" wording and descriptions below over the current ones; hopefully these can be addressed in a follow-up. hebasto: re-ACK ac64cec ryanofsky: Code review ACK ac64cec. Only changes since last review are tweaking placeholder text and dropping "allow nameless" commit Tree-SHA512: a25f84eb66ee4f99af441d73e33928df9d9cf592177398ef48f0037f5913699e47a162cf1301c83b34501546d43ff4ae12607fd078c5c03b92f573bf7604a9f2
d4feb68 qt: Use layout manager for Create Wallet dialog (Hennadii Stepanov) Pull request description: On master (e75f91e) not using layout manager causes problems with resizing: ![Screenshot from 2021-01-01 13-03-13](https://user-images.githubusercontent.com/32963518/103437728-ce1d4580-4c33-11eb-8915-1e9482775653.png) ![Screenshot from 2021-01-01 13-03-26](https://user-images.githubusercontent.com/32963518/103437730-d6758080-4c33-11eb-9e0f-87d0dd487fcb.png) Also text labels are not resized properly on some window managers (#20777), or if their lengths are changed (after translation). This PR introduces a standard layout manager for the "Create Wallet" dialog that fixes all layout issues (actually, the `createwalletdialog.ui` has been re-written from scratch): ![Screenshot from 2021-01-01 13-10-03](https://user-images.githubusercontent.com/32963518/103437822-d0cc6a80-4c34-11eb-84fd-fcb10a16d9ef.png) ![Screenshot from 2021-01-06 23-50-36](https://user-images.githubusercontent.com/32963518/103823090-0b416780-507a-11eb-89dd-3f48a358e168.png) Additional visual changes: - advanced options are grouped in `QGroupBox` (bitcoin-core/gui#96 (comment)) - enabled the [size grip](https://doc.qt.io/qt-5/qsizegrip.html#details) Fix #20777 ACKs for top commit: jarolrod: ACK d4feb68 Sjors: re-tACK d4feb68 promag: Tested ACK d4feb68 on macos. Tree-SHA512: 4c055962e49f88624900b880b33a866976d224628784593428b712d2e94563d77ddefddea3397134d20e72f738a8cf9aa885c1272fd9ffc90213c104435fb9f4
d4feb68 qt: Use layout manager for Create Wallet dialog (Hennadii Stepanov) Pull request description: On master (e75f91e) not using layout manager causes problems with resizing: ![Screenshot from 2021-01-01 13-03-13](https://user-images.githubusercontent.com/32963518/103437728-ce1d4580-4c33-11eb-8915-1e9482775653.png) ![Screenshot from 2021-01-01 13-03-26](https://user-images.githubusercontent.com/32963518/103437730-d6758080-4c33-11eb-9e0f-87d0dd487fcb.png) Also text labels are not resized properly on some window managers (bitcoin#20777), or if their lengths are changed (after translation). This PR introduces a standard layout manager for the "Create Wallet" dialog that fixes all layout issues (actually, the `createwalletdialog.ui` has been re-written from scratch): ![Screenshot from 2021-01-01 13-10-03](https://user-images.githubusercontent.com/32963518/103437822-d0cc6a80-4c34-11eb-84fd-fcb10a16d9ef.png) ![Screenshot from 2021-01-06 23-50-36](https://user-images.githubusercontent.com/32963518/103823090-0b416780-507a-11eb-89dd-3f48a358e168.png) Additional visual changes: - advanced options are grouped in `QGroupBox` (bitcoin-core/gui#96 (comment)) - enabled the [size grip](https://doc.qt.io/qt-5/qsizegrip.html#details) Fix bitcoin#20777 ACKs for top commit: jarolrod: ACK d4feb68 Sjors: re-tACK d4feb68 promag: Tested ACK d4feb68 on macos. Tree-SHA512: 4c055962e49f88624900b880b33a866976d224628784593428b712d2e94563d77ddefddea3397134d20e72f738a8cf9aa885c1272fd9ffc90213c104435fb9f4
Previously only users who needed a second wallet had to use to the create wallet dialog. With the merge of bitcoin/bitcoin#15454 now all new users have to. I don't think it was user-friendly enough for that.
This PR makes a few simple improvements so that new users don't have to think too much:
It's lightly inspired by #77. It would be better if those changes made it into the upcoming release, but this PR is a good start imo.
Note that a blank wallet name isn't allowed in the dialog; I haven't addressed that.
Update 2020-10-30, dropped the new strings for now: