Disable and uncheck blank when private keys are disabled #739
pull achow101 wants to merge 1 commits into bitcoin-core:master from achow101:gui-dont-blank-noprivkeys changing 1 files +9 −8-
achow101 commented at 7:53 pm on June 13, 2023: memberUnify the GUI’s create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled.
-
DrahtBot commented at 7:53 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 S3RK, jarolrod, pablomartin4btc Concept ACK hebasto, ryanofsky, hernanmarino If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
-
DrahtBot added the label CI failed on Jun 13, 2023
-
hebasto renamed this:
gui: Disable and uncheck blank when private keys are disabled
Disable and uncheck blank when private keys are disabled
on Jun 14, 2023 -
hebasto added the label Wallet on Jun 14, 2023
-
hebasto commented at 9:24 am on June 14, 2023: memberConcept ACK.
-
hebasto commented at 9:25 am on June 14, 2023: member
-
in src/qt/createwalletdialog.cpp:70 in 296f0a0572 outdated
70 // set to true, enable it when isDisablePrivateKeysChecked is false. 71 ui->encrypt_wallet_checkbox->setEnabled(!checked); 72 73- // Wallets without private keys start out blank 74+ // Wallets without private keys cannot set blank 75+ ui->blank_wallet_checkbox->setEnabled(!checked);
S3RK commented at 7:23 am on June 21, 2023:If we disableblank
checkbox whendisable_privkeys
is checked should we do the reverse?
pablomartin4btc commented at 4:05 pm on June 23, 2023:Perhaps this is visually clearer for the user.
achow101 commented at 5:41 pm on June 23, 2023:DoneS3RK commented at 7:46 am on June 21, 2023: contributorUnify the GUI’s create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled.
Do I understand correctly that we want to make these two flags mutually exclusive?
Let me make sure I correctly understand the rational. I guess the state when both are
true
is not meaningful becausedisable_privkeys
is more restrictive thanblank
. There will be no keys generated and no keys could be imported, so there is no keys for the wallet software to manage and henceblank
(aka manual) is not needed.Assuming the above is correct, do we want also to make them exclusive in RPC? If I’m not mistaken one can set both today.
Also, IIUC existing wallets that set both are not a problem, because it’s just redundant information and will not lead to unexpected behaviour. Am I correct in my understanding?
ryanofsky commented at 12:40 pm on June 21, 2023: contributorConcept ACK. This change seems like an improvement, and I think it is good to make it clear that when private keys are disabled, the “blank” option is not useful and effectively meaningless.
I think these options are still confusing, though, and instead of having checkboxes it would be better if you could choose what actual behavior you want:
- Generate private keys
- Do not generate private keys (blank wallet)
- Do not generate private keys and disallow them (watch-only wallet)
I notice there was a similar proposal in #69 (comment):
re: #739 (comment)
Do I understand correctly that we want to make these two flags mutually exclusive? […]
Yes if the disable private keys option is checked the blank option is basically useless, so it’s confusing to leave it as option that can be toggled on an off but effectively doesn’t do anything.
Assuming the above is correct, do we want also to make them exclusive in RPC? If I’m not mistaken one can set both today.
RPC is a lower level interface than the GUI and already exposes both flags directly, so maybe it could warn when blank flag is set uselessly, but as an API I don’t think it should make an error out of a configuration that makes sense but is a little redundant, or that it should break code that worked previously.
Also setting the flags independently through RPC can have some effect and may not be completely useless. For example, the blank flag was introduced in a later software release than the disable private keys flag, so setting the blank flag prevents the wallet from opening the wallet in older software. Also, IIUC, it is possible in RPC interface to disable the disable private keys option later, so maybe setting the blank flag could be useful if you anticipate doing that.
hernanmarino commented at 8:16 pm on June 21, 2023: contributorConcept ACKpablomartin4btc commented at 5:17 pm on June 23, 2023: contributorTested ACK.
I think there’s a concise description on identifying these “disable private keys” and “blank wallet” concepts by @achow101 here.
I like the approach on replacing the checkboxes by radiobuttons proposed in #69 and bringing it here by @ryanofsky, I also like the fact having the description under each optiion, and more practical (removing the other 2 checkboxes that are disabled already: “external signer” in
false
and “descriptor wallet” intrue
) it seems clearer, wouldn’t it be on a different PR?.On the RPC side of this, I find this comment from @ryanofsky very useful:
Also setting the flags independently through RPC can have some effect and may not be completely useless. For example, the blank flag was introduced in a later software release than the disable private keys flag, so setting the blank flag prevents the wallet from opening the wallet in older software. Also, IIUC, it is possible in RPC interface to disable the disable private keys option later, so maybe setting the blank flag could be useful if you anticipate doing that.
gui: Disable and uncheck blank when private keys are disabled
Unify the GUI's create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled.
achow101 force-pushed on Jun 23, 2023DrahtBot removed the label CI failed on Jun 23, 2023S3RK commented at 7:14 am on June 26, 2023: contributorI’m nor a GUI code expert neither a UX (user experience) expert, so take all of this with a healthy skepticism. The code changes look good to me, but I’m unsure about whether this is a better UX. Code review ACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177
I’m unsure if it’s a clear win for UX because there’s no hint to the user why the checkboxes change their state. I also like the idea with radio buttons and I’d be happy to review a new PR or re-review this one though.
Maybe change the description of the PR, it seems like we actually want to keep the behaviour of RPC and GUI somewhat different, so the unification part is confusing.
ryanofsky commented at 0:54 am on June 28, 2023: contributor@S3RK do you think the UI changes here are actually worse, or just not an improvement? I agree the changes aren’t ideal and aren’t a clear win, but I think they don’t make it worse necessarily. This PR disables the “make blank wallet” checkbox when “disable private keys” checkbox is enabled, instead of just automatically checking it. I think disabling the checkbox makes sense because it makes it clear than once private keys are disabled, the choice of whether to have a blank wallet is moot because the wallet will necessarily be empty.
On the other hand, maybe the current UI makes it clearer that disabling private keys implies having a blank wallet, because it automatically checks blank wallet when you check disable private keys, and automatically unchecks disable private keys when you uncheck blank wallet. I guess both UI’s seem reasonable to me, even if neither is ideal.
I do agree that PR description could be clearer, and could describe what the UI change is, along with mentioning that the leaving the blank flag unset when the disable private keys flag is set makes GUI behavior match default RPC behavior.
S3RK commented at 7:04 am on June 28, 2023: contributor@ryanofsky no no, I don’t think this change is making it worse.
From my perspective, it’s an improvement for core developers, because it provides clarity on the wallet flags usage. UX wise it’s ~0 as the flags are implementation details. Both pre- and post-PR whether blank checkbox is disabled or automatically checked is equally not explained to the users.
jarolrod commented at 6:03 am on July 5, 2023: memberACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177
This is correct, and an improvement.
Visual and UX improvements on wallet creation can be handled for a follow-up here if someone wants to take it. The issue around UX of wallet creation will be tackled in https://github.com/bitcoin-core/gui-qml
DrahtBot added the label CI failed on Aug 17, 2023DrahtBot removed the label CI failed on Aug 22, 2023DrahtBot added the label CI failed on Aug 31, 2023DrahtBot removed the label CI failed on Sep 4, 2023in src/qt/createwalletdialog.cpp:87 in 9ea31eba04
84+ // Disable the disable_privkeys_checkbox when blank_wallet_checkbox is checked 85+ // as blank-ness only pertains to wallets with private keys. 86+ ui->disable_privkeys_checkbox->setEnabled(!checked); 87+ if (checked) { 88+ ui->disable_privkeys_checkbox->setChecked(false); 89 }
pablomartin4btc commented at 4:25 am on September 20, 2023:nit:
0 if (checked) ui->disable_privkeys_checkbox->setChecked(false);
in src/qt/createwalletdialog.cpp:73 in 9ea31eba04
74+ // Wallets without private keys cannot set blank 75+ ui->blank_wallet_checkbox->setEnabled(!checked); 76 if (checked) { 77- ui->blank_wallet_checkbox->setChecked(true); 78+ ui->blank_wallet_checkbox->setChecked(false); 79 }
pablomartin4btc commented at 4:30 am on September 20, 2023:nit:
0 if (checked) ui->blank_wallet_checkbox->setChecked(false);
pablomartin4btc approvedpablomartin4btc commented at 4:33 am on September 20, 2023: contributortACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177hebasto merged this on Sep 22, 2023hebasto closed this on Sep 22, 2023
Frank-GER referenced this in commit 3afad83964 on Sep 25, 2023sidhujag referenced this in commit 2669ce4188 on Sep 26, 2023bitcoin-core locked this on Sep 21, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-11 12:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me