Disable and uncheck blank when private keys are disabled #739

achow101 · 2023-06-13T19:53:22Z

Unify the GUI's create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled.

DrahtBot · 2023-06-13T19:53:25Z

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.

hebasto · 2023-06-14T09:24:25Z

Concept ACK.

hebasto · 2023-06-14T09:25:02Z

cc @furszy @ryanofsky @S3RK

src/qt/createwalletdialog.cpp

S3RK · 2023-06-21T07:46:07Z

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 · 2023-06-21T12:40:37Z

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

Concept ACK

pablomartin4btc

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.

src/qt/createwalletdialog.cpp

Unify the GUI's create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled.

S3RK · 2023-06-26T07:14:49Z

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 9ea31eb

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 · 2023-06-28T00:54:41Z

@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 · 2023-06-28T07:04:22Z

@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

ACK 9ea31eb

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

pablomartin4btc

tACK 9ea31eb

pablomartin4btc · 2023-09-20T04:25:50Z

src/qt/createwalletdialog.cpp

+        if (checked) {
+            ui->disable_privkeys_checkbox->setChecked(false);
        }


nit:

Suggested change

if (checked) {

ui->disable_privkeys_checkbox->setChecked(false);

}

if (checked) ui->disable_privkeys_checkbox->setChecked(false);

pablomartin4btc · 2023-09-20T04:30:23Z

src/qt/createwalletdialog.cpp

        if (checked) {
-            ui->blank_wallet_checkbox->setChecked(true);
+            ui->blank_wallet_checkbox->setChecked(false);
        }


nit:

Suggested change

if (checked) {

ui->blank_wallet_checkbox->setChecked(true);

ui->blank_wallet_checkbox->setChecked(false);

}

if (checked) ui->blank_wallet_checkbox->setChecked(false);

…ys are disabled 9ea31eb gui: Disable and uncheck blank when private keys are disabled (Andrew Chow) Pull request description: Unify the GUI's create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled. ACKs for top commit: S3RK: Code review ACK 9ea31eb jarolrod: ACK 9ea31eb pablomartin4btc: tACK 9ea31eb Tree-SHA512: 0c90dbd77e66f088c6a57711a4b91e254814c4ee301ab703807f281cacd4b08712d2dfeac7661f28bc0e93acc55d486a17b8b4a53ffa57093d992e7a3c51f4e8

achow101 mentioned this pull request Jun 13, 2023

wallet, tests: Expand and test when the blank wallet flag should be un/set bitcoin/bitcoin#25634

Merged

DrahtBot added the CI failed label Jun 13, 2023

hebasto changed the title ~~gui: Disable and uncheck blank when private keys are disabled~~ Disable and uncheck blank when private keys are disabled Jun 14, 2023

hebasto added the Wallet label Jun 14, 2023

S3RK reviewed Jun 21, 2023

View reviewed changes

src/qt/createwalletdialog.cpp Show resolved Hide resolved

hernanmarino reviewed Jun 21, 2023

View reviewed changes

pablomartin4btc reviewed Jun 23, 2023

View reviewed changes

src/qt/createwalletdialog.cpp Show resolved Hide resolved

gui: Disable and uncheck blank when private keys are disabled

9ea31eb

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 the gui-dont-blank-noprivkeys branch from 296f0a0 to 9ea31eb Compare June 23, 2023 17:41

DrahtBot removed the CI failed label Jun 23, 2023

jarolrod reviewed Jul 5, 2023

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Aug 17, 2023

DrahtBot added CI failed and removed CI failed labels Aug 31, 2023

pablomartin4btc approved these changes Sep 20, 2023

View reviewed changes

hebasto merged commit bce7b08 into bitcoin-core:master Sep 22, 2023

bitcoin-core locked and limited conversation to collaborators Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Disable and uncheck blank when private keys are disabled #739

Disable and uncheck blank when private keys are disabled #739

Uh oh!

achow101 commented Jun 13, 2023

Uh oh!

DrahtBot commented Jun 13, 2023 •

edited

Loading

Uh oh!

hebasto commented Jun 14, 2023

Uh oh!

hebasto commented Jun 14, 2023

Uh oh!

Uh oh!

S3RK commented Jun 21, 2023

Uh oh!

ryanofsky commented Jun 21, 2023 •

edited

Loading

Uh oh!

hernanmarino left a comment

Uh oh!

pablomartin4btc left a comment

Uh oh!

Uh oh!

S3RK commented Jun 26, 2023

Uh oh!

ryanofsky commented Jun 28, 2023

Uh oh!

S3RK commented Jun 28, 2023 •

edited

Loading

Uh oh!

jarolrod left a comment

Uh oh!

pablomartin4btc left a comment

Uh oh!

pablomartin4btc Sep 20, 2023

Uh oh!

pablomartin4btc Sep 20, 2023

Uh oh!

Uh oh!

Disable and uncheck blank when private keys are disabled #739

Disable and uncheck blank when private keys are disabled #739

Uh oh!

Conversation

achow101 commented Jun 13, 2023

Uh oh!

DrahtBot commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

hebasto commented Jun 14, 2023

Uh oh!

hebasto commented Jun 14, 2023

Uh oh!

Uh oh!

S3RK commented Jun 21, 2023

Uh oh!

ryanofsky commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

S3RK commented Jun 26, 2023

Uh oh!

ryanofsky commented Jun 28, 2023

Uh oh!

S3RK commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jun 13, 2023 •

edited

Loading

ryanofsky commented Jun 21, 2023 •

edited

Loading

S3RK commented Jun 28, 2023 •

edited

Loading