Slight improve create wallet dialog #96

Sjors commented at 8:24 AM on September 18, 2020: member

Previously only users who needed a second wallet had to use to the create wallet dialog. With the merge of https://github.com/bitcoin/bitcoin/pull/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.

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">

jonatack commented at 8:31 AM on September 18, 2020: contributor

Concept ACK, seems much better.

I'm probably missing something, but why "blank wallet" instead of "watchonly"?

Sjors commented at 8:39 AM on September 18, 2020: member

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 :-)

jonatack commented at 8:44 AM on September 18, 2020: contributor

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.

GBKS commented at 9:23 AM on September 18, 2020: none

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?

Sjors commented at 9:42 AM on September 18, 2020: member

@GBKS I think I'll leave that for whoever implements #77

Bosch-0 commented at 9:43 AM on September 18, 2020: none

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.

Good changes. Could "Encrypt wallet" also be "Protect wallet with a password"?

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 https://github.com/bitcoin/bitcoin/issues/18085

Sjors force-pushed on Sep 18, 2020

Sjors commented at 10:21 AM on September 18, 2020: member

I updated same labels and tooltips.

harding commented at 11:00 AM on September 18, 2020: contributor

Concept ACK. I like the idea of separating out the confusing and less-often-needed features into an advanced section.

MarcoFalke commented at 5:58 PM on September 20, 2020: contributor

Concept ACK

in src/qt/createwalletdialog.cpp:45 in e84e07c810 outdated

  40 | +        // set to true, enable it when isDisablePrivateKeysChecked is false.
  41 | +        ui->encrypt_wallet_checkbox->setEnabled(!checked);
  42 | +
  43 | +        // Wallets without private keys start out blank
  44 | +        if (checked) {
  45 | +            ui->blank_wallet_checkbox->setChecked(true);

promag commented at 9:19 PM on September 20, 2020:

Should also disable blank_wallet_checkbox?

Sjors commented at 2:22 PM on September 21, 2020:

Neh, it gets too tedious. It doesn't matter what you do with this checkbox, it's ignored for watch-only.

in src/qt/forms/createwalletdialog.ui:77 in b00e468737 outdated

  73 | @@ -74,14 +74,27 @@
  74 |      <bool>false</bool>
  75 |     </property>
  76 |    </widget>
  77 | +  <widget class="QLabel" name="label">

hebasto commented at 1:05 PM on October 5, 2020:

The "label" name is already in use: see line 45.

hebasto commented at 1:09 PM on October 5, 2020: member

Concept ACK e84e07c810f3e187a8536d947254fab2e29107d0, tested on Linux Mint 20 (x86_64, Qt 5.12.8).

master (875e1ccc9fe01e026e564dfd39a64d9a4b332a89):
this PR (e84e07c810f3e187a8536d947254fab2e29107d0):

I'd prefer to see more designers (@Bosch-0, @GBKS) ACKs or comments after the recent update.

joernroeder commented at 10:24 PM on October 5, 2020: contributor

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. As users might not know the consequences, add a description explaining them and potential loss of funds next to the password input fields in the next step or – when users check it off, describe the potential risk next to/below the checkbox.

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 PascalCase in the placeholder?

GBKS commented at 7:47 AM on October 6, 2020: none

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.

Bosch-0 commented at 11:42 AM on October 7, 2020: none

Concept ACK, looks good on Windows 10. I agree with with Christoph on renaming 'Skip seed generation' to 'Manually import seed or keys', it's more action driven.

harding commented at 4:20 PM on October 7, 2020: contributor

@joernroeder

I don't like the idea to disable the encryption by default and would prefer a "fail safe" strategy here.

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 Manually import seed or keys.

laanwj commented at 4:52 PM on October 7, 2020: member

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.

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. In any case the kind of scenario where encryption works is "other people have physical access to my PC but won't use it to install a keylogger/backdoor", not so much remote exploitation. Or cases where the browser can grab a specific named file to upload but not otherwise gain access.

Bosch-0 commented at 3:58 AM on October 8, 2020: none

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 commented at 7:34 AM on October 8, 2020: contributor

Thank you @harding for all the details. It seems like I misinterpreted the first screenshot as the current default which has encryption checked. Also thanks @Bosch-0 for pointing me to #77 which is what I had in mind.

Sjors commented at 9:39 AM on October 8, 2020: member

@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 Manually import seed or keys. suggestion in mind in case I need to rebase, but prefer to conserve ACKs.

hebasto commented at 12:41 PM on October 15, 2020: member

@Sjors

... but prefer to conserve ACKs.

Which one? :smiley:

Sjors force-pushed on Oct 15, 2020

Sjors commented at 2:45 PM on October 15, 2020: member

Right... rebased and changed the label.

hebasto changes_requested

hebasto commented at 4:19 PM on October 15, 2020: member

ACK ad0874e0f53f7c9a731c701451cfa50da412304c, 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"?

in src/qt/forms/createwalletdialog.ui:122 in ad0874e0f5 outdated

 128 | -    <string>Make a blank wallet. Blank wallets do not initially have private keys or scripts. Private keys and addresses can be imported, or an HD seed can be set, at a later time.</string>
 129 | +    <string>Blank wallets do not initially have private keys or scripts. Private keys and addresses can be imported, or an HD seed can be set, at a later time.</string>
 130 |     </property>
 131 |     <property name="text">
 132 | -    <string>Make Blank Wallet</string>
 133 | +    <string>Manually import seed or keys</string>

jonatack commented at 4:34 PM on October 15, 2020:

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.

Sjors commented at 5:56 PM on October 15, 2020:

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.

in src/qt/forms/createwalletdialog.ui:138 in ad0874e0f5 outdated

 147 | -    <string>Use descriptors for scriptPubKey management</string>
 148 | +    <string>Use descriptors for scriptPubKey management.</string>
 149 |     </property>
 150 |     <property name="text">
 151 | -    <string>Descriptor Wallet</string>
 152 | +    <string>Use descriptors</string>

Sjors commented at 5:54 PM on October 15, 2020:

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)

Sjors commented at 6:09 PM on October 15, 2020:

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.

jonatack commented at 8:57 AM on October 16, 2020: contributor

"I'd rather not finetune this" / "I don't think we should bikeshed this to death."

Ok sorry. I wanted to be help move forward but one of the changes doesn't make sense to me. I've removed my review feedback except for the one comment. These kind of comments weren't seen as bikeshedding earlier in the discussion, so I thought it was ok to discuss these changes.

hebasto commented at 9:37 AM on October 16, 2020: member

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.

We have a nice opportunity to avoid bikeshedding in this repo completely. All that we need is to realize that the GUI repo is a new home for collaboration of devs and designers. Code is devs' responsibility. Look-and-feel is designers' responsibility. Let's rely on opinion of designers (@Bosch-0 and @GBKS in this particular PR).

Let bikeshedding be designers' burden :)

Sjors commented at 9:59 AM on October 16, 2020: member

That only works until there's more designers :-)

hebasto commented at 10:02 AM on October 16, 2020: member

That only works until there's more designers :-)

The extrapolation is optimistic :)

jonatack commented at 10:08 AM on October 16, 2020: contributor

Code is devs' responsibility. Look-and-feel is designers' responsibility. Let's rely on opinion of designers (@Bosch-0 and @GBKS in this particular PR).

Well, this is about text and I'm somewhat competent at communicating by writing, as I think Sjors is as well, so I disagree that wording is the exclusive domain of designers. I also don't see why these comments were fine earlier on and are now called bikeshedding. But whatever, when I see the word "bikeshedding" I'm outtie.

Sjors commented at 10:11 AM on October 16, 2020: member

@jonatack I've been resisting text changes to this PR since the start: #96 (comment)

I occasionally make them anyway if I need to fix a bug or rebase. But some of these comments are going in circles, where I change it and then someone asks why not use the original.

hebasto commented at 10:12 AM on October 16, 2020: member

@jonatack

Well, this is about text and I'm somewhat competent at communicating by writing...

Being a non-native English speaker, I always rely on that your competence :)

MarcoFalke commented at 10:56 AM on October 16, 2020: contributor

Concept ACK

Sjors commented at 11:47 AM on October 16, 2020: member

Both @GBKS and @harding suggested the change to Manually import seed or keys, provided reasoning for that, nobody objected for a week, so I made the change. I don't really care either way, because these options are incomprehensible and useless without a manual. What I want is for new users to ignore these options, which is why I put them under "advanced". What I don't want to do is change the text once, and then change it back, and then change it again and miss the feature freeze deadline. The net result of that is keeping the unsafe version (with encryption enabled by default). Everyone is welcome to leave suggestions about text, I just stated again (and again) that I don't necessary plan to honour those suggestions.

in src/qt/forms/createwalletdialog.ui:119 in ad0874e0f5 outdated

 124 |       <height>22</height>
 125 |      </rect>
 126 |     </property>
 127 |     <property name="toolTip">
 128 | -    <string>Make a blank wallet. Blank wallets do not initially have private keys or scripts. Private keys and addresses can be imported, or an HD seed can be set, at a later time.</string>
 129 | +    <string>Blank wallets do not initially have private keys or scripts. Private keys and addresses can be imported, or an HD seed can be set, at a later time.</string>

ryanofsky commented at 12:55 PM on October 19, 2020:

"Blank wallet" is no longer part of option name, so would suggest dropping "Blank wallet" from tooltip as well.
IMO, I thought original "Skip seed generation" in original version of this PR was better than "Manually import seed or keys." If I'm getting a hamburger and don't want fries a "Don't include fries" option is clearer than "Maybe add a milkshake"

ryanofsky approved

ryanofsky commented at 1:03 PM on October 19, 2020: contributor

Code review ACK e84e07c810f3e187a8536d947254fab2e29107d0. Ignore my bike shed color list! @jonatack, in the future could you maybe edit comments in ~strikethrough~ to update them instead of deleting them entirely? Deleted comments make github threads even harder to follow than normal. (Also, I happened to agree with the comments)

Rspigler commented at 11:13 PM on October 24, 2020: contributor

Checking Encrypt private keys disables Watch-only :heavy_check_mark:

Checking Watch-only disables Encrypt private keys and enables Manually import seed or keys. That makes sense to me when Manually import... was originally labelled Blank Wallet - but you can't import seed or keys to a watch-only wallet, so this seems like an error. Because of this, I think naming it back to Blank Wallet makes more sense?

Unchecking Watch-only re-enables Encrypt private keys, and keeps Manually import seed or keys enabled. :heavy_check_mark:

What is the difference between a wallet that has Watch-only enabled, and one that has Watch-only and Manually import seed or keys enabled? In other words, should unchecking Manually import seed or keys also uncheck Watch-only? I believe so, and this is another reason why remaining Manually import seed or keys to Blank Wallet makes more sense IMO. (Unchecking Blank Wallet should uncheck Watch-only).

I also like @ryanofsky 's suggestion of Skip seed generation.

But I also understand I am coming in late, and there has been frustrations over bikeshedding.

Sjors commented at 11:59 AM on October 26, 2020: member

Given that we missed the translations freeze, it seems best to revert all text to their originals. @jonasschnelli?

DrahtBot added the label Needs rebase on Oct 29, 2020

[gui] create wallet: smarter checkbox toggling 5bff82540b

Sjors force-pushed on Oct 30, 2020

Sjors commented at 10:28 AM on October 30, 2020: member

I had to rebase. I dropped ad0874e with the new strings, because I'd still like to get this into 0.21. For a followup, here's the last list of strings I used:

Encrypt private keys: Private keys in the wallet will be encrypted with a passphrase of your choice. Addresses, transaction history and notes are not encrypted.

Watch-only Disable private keys for this wallet. Watch-only wallets have no private keys and cannot have an HD seed or imported private keys.

Manually import seed or keys (but see discussion above): Blank wallets do not initially have private keys or scripts. Private keys and addresses can be imported, or an HD seed can be set, at a later time.

Use descriptors: Use descriptors for scriptPubKey management. @hebasto: I squashed those two commits

Sjors force-pushed on Oct 30, 2020

DrahtBot removed the label Needs rebase on Oct 30, 2020

ryanofsky approved

ryanofsky commented at 12:09 AM on November 11, 2020: contributor

Code review ACK d69a436565c74484cac8b2814d2bee1bfeb59e72. Since last review, this was rebased and all changes to existing strings were reverted

laanwj added this to the milestone 0.21.0 on Nov 12, 2020

in src/qt/forms/createwalletdialog.ui:90 in d69a436565 outdated

  83 | +     <width>130</width>
  84 | +     <height>21</height>
  85 | +    </rect>
  86 | +   </property>
  87 | +   <property name="text">
  88 | +    <string>Advanced options</string>

ysangkok commented at 7:11 PM on November 12, 2020:

you say that there are no new strings, but this looks like a new string. why is it not a new string?

ysangkok commented at 7:20 PM on November 12, 2020:

why is it not a heading like h1-6? seems like it is supposed to cover multiple items below?

Sjors commented at 8:34 PM on November 12, 2020:

There's no modified strings, but indeed this is added. It will show in English for all users.

Sjors commented at 8:59 PM on November 12, 2020:

I changed it to bold font, same as with Coin Selection in the send screen.

jonasschnelli commented at 7:37 AM on November 13, 2020:

Hmm... thats unfortunate. Would using "Options" (already translated) be an option?

in src/qt/createwalletdialog.cpp:55 in d69a436565 outdated

  55 | +            ui->encrypt_wallet_checkbox->setChecked(false);
  56 | +        }
  57 | +    });
  58 | +
  59 | +    #ifndef USE_SQLITE
  60 | +        ui->descriptor_checkbox->setToolTip(tr("Compiled without sqlite support (required for descriptor wallets)"));

ysangkok commented at 7:14 PM on November 12, 2020:

this also looks like a new string ?

Sjors commented at 8:36 PM on November 12, 2020:

No, it's moved. Try viewing with ignored whitespace (add ?w=1 to github URL)

in src/qt/forms/createwalletdialog.ui:77 in d69a436565 outdated

  70 | @@ -68,17 +71,30 @@
  71 |      <string>Encrypt Wallet</string>
  72 |     </property>
  73 |     <property name="checked">
  74 | -    <bool>true</bool>
  75 | +    <bool>false</bool>
  76 | +   </property>
  77 | +  </widget>
  78 | +  <widget class="QLabel" name="encrypt_wallet_label">

ysangkok commented at 7:21 PM on November 12, 2020:

why is the property with text "Advanced options" under a widget named 'encrypt_wallet_label' in the XML hierarchy? encrypting the wallet is just one option.

Sjors commented at 8:52 PM on November 12, 2020:

That should have been advanced_options_label. Fixed

ysangkok changes_requested

ysangkok commented at 7:30 PM on November 12, 2020: none

@Sjors did you know about QGroupBox? was it a conscious choice not to use it? it could be better for accessibility, since Qt would know the "Advanced options" label is a heading and could tell a screen reader that.

Sjors commented at 8:54 PM on November 12, 2020: member

QGroupBox looks like a better approach, but I'm not handy enough with manually editing .ui files or using the QT graphical editor, dealing with size constraints and such. So would prefer to leave that to a followup.

Update: made advanced section title bold, added missing tabstop for descriptor checkbox (though tabs don't work for me in this screen on macOS)

Sjors force-pushed on Nov 12, 2020

jonatack commented at 9:13 PM on November 12, 2020: contributor

Will review/test tomorrow for 0.21

in src/qt/forms/createwalletdialog.ui:42 in d393708c9b outdated

  37 | @@ -38,6 +38,9 @@
  38 |       <height>24</height>
  39 |      </rect>
  40 |     </property>
  41 | +   <property name="placeholderText">
  42 | +     <string>MyWallet</string>

jonasschnelli commented at 7:40 AM on November 13, 2020:

this is also a new string,... though probably acceptable. Could use "Wallet Name" though,... but we already have this as label for the textfield.

Sjors commented at 7:42 AM on November 13, 2020:

Ah yes, one alternative could be to allow empty names.

jonasschnelli commented at 7:43 AM on November 13, 2020: contributor

Tested ACK d393708c9bf1d2ea2a953a236816dabc48a072f1 module the missing string for "Advanced options". If we are going to merge this for 0.21 (I don't see a pressing reason to do so but fine with it), we could switch to just "Option" (already translated) and change that post 0.21 branch-off.

Sjors commented at 7:58 AM on November 13, 2020: member

I pushed an additional commit that allows the wallet name to be empty, which creates the default wallet (or throws an error if you already have it). That gets rid of the "MyWallet" string.

I prefer to stick to "Advanced options" rather than just "Options" in order to discourage blindly using them. My guess is that if someone doesn't understand the English term, they'll be discouraged :-)

ryanofsky changes_requested

ryanofsky commented at 2:22 PM on November 13, 2020: contributor

Code review ACK d393708c9bf1d2ea2a953a236816dabc48a072f1 which just tweaks createdialog label id, tabstop & stylesheet property since last review.

I don't think newly added commit 03ebe1b5938a73b41e34a1251520a2db29fafd95 is a good idea, though. Main motivation seems be avoiding adding a translation string (https://github.com/bitcoin-core/gui/pull/96#discussion_r522738216), but the new commit increases the scope of the PR beyond "slightly improving the create wallet dialog", to adding a GUI feature: creating and encouraging creation of a top level unnamed wallet which contains other wallets. I don't think the change is great, but even if it were a good idea, I think it would make more sense in its own feature PR, not a PR that's just consisted of dialog box tweaks up to this point and is still described that way.

Sjors commented at 2:42 PM on November 13, 2020: member

creation of a top level unnamed wallet which contains other wallets

I'm not sure what you mean. This is basically the original behaviour where the first wallet is wallet.dat. The UI already handles it.

ryanofsky commented at 2:53 PM on November 13, 2020: contributor

I'm not sure what you mean. This is basically the original behaviour where the first wallet is wallet.dat. The UI already handles it.

Right, and it was intentionally disabled in https://github.com/bitcoin/bitcoin/pull/15454 and 78863e290006e61060622dbdbecc5b58c0fefa05 from https://github.com/bitcoin/bitcoin/pull/15450. The new commit 03ebe1b5938a73b41e34a1251520a2db29fafd95 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

Sjors commented at 5:37 PM on November 13, 2020: member

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):

Instead of automatically creating and loading a default wallet, users should instead explicitly create their wallet or load it on start.

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.

ryanofsky commented at 6:26 PM on November 13, 2020: contributor

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 03ebe1b5938a73b41e34a1251520a2db29fafd95 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 03ebe1b5938a73b41e34a1251520a2db29fafd95 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 03ebe1b5938a73b41e34a1251520a2db29fafd95 is beyond scope of this PR.

jonatack commented at 8:35 PM on November 13, 2020: contributor

Tested ACK d393708c9bf1d2ea2a953a236816dabc48a072f1 "gui: create wallet: add advanced section" with the following reservations:

I agree with @ryanofsky about dropping the last commit.
Clicking on the watch-only option does indeed auto-check the blank wallet option but also violates the principle of least surprise and may look to many users like a bug in the code, which may reduce user confidence in the wallet. Perhaps remove this auto-checking, or maybe append (auto-selects "Make Blank Wallet") to the watch-only text to make clear to users what is going on, but the dialog may need to be larger to accomodate that.

For a followup, here's the last list of strings I used:

Encrypt private keys: Private keys in the wallet will be encrypted with a passphrase of your choice. Addresses, transaction history and notes are not encrypted.

Watch-only Disable private keys for this wallet. Watch-only wallets have no private keys and cannot have an HD seed or imported private keys.

For now or later, I like these two changes.

Another minor question: Why PascalCase in the placeholder?

Had the same thought. Maybe "My Wallet".

gui: create wallet: name placeholder c99d6f644a

gui: create wallet: add advanced section ac64cec4ce

in src/qt/forms/createwalletdialog.ui:87 in 03ebe1b593 outdated

  80 | +     <width>130</width>
  81 | +     <height>21</height>
  82 | +    </rect>
  83 | +   </property>
  84 | +   <property name="styleSheet">
  85 | +    <string notr="true">font-weight:bold;</string>

jonatack commented at 8:41 PM on November 13, 2020:

(QT newb question: what is notr?)

jonatack commented at 8:47 PM on November 13, 2020:

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.

Sjors commented at 11:53 AM on November 14, 2020:

I just copy-pasted that from another ui file :-)

hebasto commented at 4:34 PM on November 14, 2020:

string is not translated :)

Sjors force-pushed on Nov 14, 2020

Sjors commented at 12:18 PM on November 14, 2020: member

@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 "" wallet was intentional, it is now :-)

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.

Clicking on the watch-only option does indeed auto-check the blank wallet option but also violates the principle of least surprise

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).

hebasto approved

hebasto commented at 6:08 PM on November 14, 2020: member

re-ACK ac64cec4ce35cb7e6fbf3678c1ffeac0137791ae

Two new strings are added after translation freeze:

I'm ok with them though, as the main goal--to disable wallet encryption by default--could be achieved in 0.21. @harding

FYI, no released version of Bitcoin Core has ever created encrypted wallets by default; this PR just preserves that legacy.

A screenshot from 0.20.1: Screenshot from 2020-11-14 18-10-17

I agree that implementation details could be improved (https://github.com/bitcoin-core/gui/pull/96#issuecomment-726337165) in a followup. @Sjors I apologize for the delay in the review.

Rspigler commented at 8:14 PM on November 14, 2020: contributor

Encrypt Wallet is still incorrectly described.

Checking Disable Private Keys checks Make Blank Wallet (and disables Encrypt Wallet). (This is correct). However, unchecking Make Blank Wallet should uncheck Disable Private Keys (and re-enable for checking Encrypt Wallet). Currently, it does nothing.

Currently, you are left with settings: Disable Private Keys checked, Make Blank Wallet unchecked, and Encrypt Wallet disabled. This seems like a bug when compared to the second sentence in my post.

If we are trying to fit this in 0.21 I guess these can be fixed in another PR

fjahr commented at 12:48 AM on November 16, 2020: contributor

Tested ACK ac64cec4ce35cb7e6fbf3678c1ffeac0137791ae

Tested on MacOS 10.15.7. Seems like a great improvement!

ryanofsky approved

ryanofsky commented at 11:35 AM on November 16, 2020: contributor

Code review ACK ac64cec4ce35cb7e6fbf3678c1ffeac0137791ae. 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

jonatack commented at 11:39 AM on November 16, 2020: contributor

re-ACK ac64cec4ce35cb7e6fbf3678c1ffeac0137791ae, 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 #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.

Clicking on the watch-only option does indeed auto-check the blank wallet option but also violates the principle of least surprise and may look to many users like a bug in the code, which may reduce user confidence in the wallet. Perhaps remove this auto-checking, or maybe append (auto-selects "Make Blank Wallet") to the watch-only text to make clear to users what is going on, but the dialog may need to be larger to accomodate that.

For a followup, here's the last list of strings I used:

Encrypt private keys: Private keys in the wallet will be encrypted with a passphrase of your choice. Addresses, transaction history and notes are not encrypted.

Watch-only Disable private keys for this wallet. Watch-only wallets have no private keys and cannot have an HD seed or imported private keys.

For now or later, I like these two changes.

jonatack commented at 11:49 AM on November 16, 2020: contributor

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

Agree, I'm seeing the same. And also, in signet the wallets do not even have their own wallets subdirectory.

Sjors commented at 3:23 PM on November 16, 2020: member

in signet the wallets do not even have their own wallets subdirectory

That would be very odd, maybe worth a seperate github issue. Did you start with pre-existing /signet directory?

jonatack commented at 3:53 PM on November 16, 2020: contributor

in signet the wallets do not even have their own wallets subdirectory

That would be very odd, maybe worth a seperate github issue. Did you start with pre-existing /signet directory?

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.

MarcoFalke merged this on Nov 17, 2020

MarcoFalke closed this on Nov 17, 2020

Sjors commented at 12:25 PM on November 17, 2020: member

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.

Sjors deleted the branch on Nov 17, 2020

sidhujag referenced this in commit 66e8029eba on Nov 17, 2020

apoelstra referenced this in commit aeb428eddf on Dec 3, 2020

MarcoFalke referenced this in commit 53bbbe5a20 on Jan 21, 2021

sidhujag referenced this in commit a271d8cf5b on Jan 21, 2021

gwillen referenced this in commit 50dafece94 on Mar 23, 2021

bitcoin-core locked this on Feb 15, 2022