Disable and uncheck blank when private keys are disabled

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

Unify 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: member

Concept ACK.

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

cc @furszy @ryanofsky @S3RK

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 disable blank checkbox when disable_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:

Done

S3RK commented at 7:46 am on June 21, 2023: contributor

Unify 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 because disable_privkeys is more restrictive than blank. There will be no keys generated and no keys could be imported, so there is no keys for the wallet software to manage and hence blank (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: contributor

Concept 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: contributor

Concept ACK

pablomartin4btc commented at 5:17 pm on June 23, 2023: contributor

Tested 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” in true) 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.

9ea31eba04

achow101 force-pushed on Jun 23, 2023

DrahtBot removed the label CI failed on Jun 23, 2023

S3RK commented at 7:14 am on June 26, 2023: contributor

I’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: member

ACK 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, 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/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 approved

pablomartin4btc commented at 4:33 am on September 20, 2023: contributor

tACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177

hebasto merged this on Sep 22, 2023

hebasto closed this on Sep 22, 2023

Frank-GER referenced this in commit 3afad83964 on Sep 25, 2023

sidhujag referenced this in commit 2669ce4188 on Sep 26, 2023

bitcoin-core locked this on Sep 21, 2024

Disable and uncheck blank when private keys are disabled #739

Reviews