Misleading "Encrypt wallet" GUI process

[kallerosenbaum]

I've tried to understand what's happening under the hood when encrypting a wallet. Valuable resources has been #8389 and #8383.

Encrypting the wallet will

• Create a new HD seed
• Save a new HD seed and the old HD seed to an encrypted wallet.dat file on disk.

If the above is correct, then the GUI is misleading (because the underlying process has changed since this GUI was created?). The process in v0.16.0 is as follows, including my suggested improvements:

1. Select File --> Encrypt wallet...
   This is misleading because it gives the impresstion that it's just encrypting your existing seed. I suggest the menu item text "Create encrypted wallet..."

2. Dialog

The window title is misleading here too. It should read "Create encrypted wallet". The text in the content area should mention that this will create a new seed and that the old seed will be kept too, but is not used to generate new keys. Maybe a checkbox "Save old seed too" could be useful here (default: checked)?

3. Dialog

The title should be "Confirm create encrypted wallet". The questions should be replaced by eg "Are you sure you want to create a new encrypted wallet? Your old keys will [keepSeed?"still":"not"] be available in your new wallet"

4. Dialog

Window title should possibly read "New wallet encrypted", but existing text is probably ok. The IMPORTANT warning is very confusing. The old unencrypted backups will be perfectly usable, but you can't access funds held by keys created since encryption. The warning makes it sound like they are somehow destroyed.

[glaksmono]

@MarcoFalke Anyone is looking at this issue?

@kallerosenbaum what OS are you at by the way? I presume this problem happens in all platforms? (Linux, Windows, OSX)

[kallerosenbaum]

@glaksmono I'm on Linux, but it should be identical on all OS I guess.

[eudisd]

Hey @kallerosenbaum @glaksmono, I took an initial stab at this. Sorry about any issues that come up with the PR, as this is my first time contributing.

[jonasschnelli]

I'm not sure about this.
For me, creating a new HD seed is not identical to "create a new wallet".
Partial rotating the seed – which is what basically happens during encrypting HD wallets – seems okay to me if checked against what the GUI words users tell.

Using the term "New" may mislead users. Because it will not create a new wallet (in the context of multiwallet, etc.).

I think your not entirely wrong... I guess the main question is what context-level we are using for talking to the user at this point.

[kallerosenbaum]

Thanks @jonasschnelli for elaborating. I agree with you that creating a new seed is not the same as creating a new wallet. But the GUI texts during encryption process had me thinking "What!!! Does it really just encrypt the seed already written to disk???", so I had to dig deeper and search through GitHub etc to figure out what Bitcoin Core actually does before I could feel comfortable with the process. So I do think there's a problem with the GUI.

I think it's right to communicate on the "wallet" context-level, rather than "seed" level. How about this instead:

1. Select File --> Encrypt wallet... (As in 0.16.0)
2. The "Encrypt wallet" dialog should explain what happens during encryption. Either as a button, eg "Explain this...", or directly in the dialog
3. "Confirm wallet encryption" (As in 0.16.0)
4. The "Wallet encrypted" info dialog should have a clearer IMPORTANT: text. Otherwise as in 0.16.0.

Suggested explainer text:

"When the wallet file is encrypted, your old seed (random data that is used to generate addresses) will be preserved, but deprecated for security reasons. A new seed is generated that all future addresses are derived from."

Suggested IMPORTANT text:

"IMPORTANT: Any previous backups you have made of your wallet file should be replaced with the newly generated, encrypted wallet file. For security reasons, previous backups of the unencrypted wallet file will become obsolete as soon as you start using the encrypted wallet. Please make sure you backup your newly encrypted wallet file before using it."

[jonasschnelli]

Yes. Something like this.

In general, I think it should tell users that encrypting a wallet after creations and especially after addresses has been used is generally a bad thing. Without being to specific about internals (like "seed", etc.)
We could warn more bold if the wallet has used addresses.

But, since encryption during creation is not yet possible (soon, createwallet RPC is now merged), I think the function of crypting later should stay (probably also once there is a function to encrypt during creation).

[eudisd]

Going to close my PR since there is contention as to what we should do. I don't think this is a good first-issue

[maflcko]

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.