Small and simple PR, updating the encryptwallet help message.
Better to notify users about the HD seed rotation and the new backup requirement before executing the encryption process. Ensuring they are prepared to update previous backups and securely safeguard the updated wallet file.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | S3RK, achow101 |
| Concept ACK | luke-jr |
| Stale ACK | pablomartin4btc, jonatack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
219 | @@ -220,7 +220,11 @@ RPCHelpMan encryptwallet() 220 | "After this, any calls that interact with private keys such as sending or signing \n" 221 | "will require the passphrase to be set prior the making these calls.\n" 222 | "Use the walletpassphrase call for this, and then walletlock call.\n" 223 | - "If the wallet is already encrypted, use the walletpassphrasechange call.\n", 224 | + "If the wallet is already encrypted, use the walletpassphrasechange call.\n" 225 | + "** IMPORTANT **\n" 226 | + "For security reasons, the encryption process will generate a new HD seed, leading to the creation of a fresh set of active descriptors. " 227 | + "Therefore, it is crucial to replace any existing backups of your wallet file with the newly generated, encrypted wallet file.\n"
"Therefore, it is crucial to replace any existing backups of your wallet file with the newly generated wallet file, containing the new encrypted keys.\n"
I read it as the wallet file itself being encrypted. I'd tweak the wording a bit to prevent confusion.
Sure. Done as suggested.
219 | @@ -220,7 +220,11 @@ RPCHelpMan encryptwallet() 220 | "After this, any calls that interact with private keys such as sending or signing \n" 221 | "will require the passphrase to be set prior the making these calls.\n" 222 | "Use the walletpassphrase call for this, and then walletlock call.\n" 223 | - "If the wallet is already encrypted, use the walletpassphrasechange call.\n", 224 | + "If the wallet is already encrypted, use the walletpassphrasechange call.\n" 225 | + "** IMPORTANT **\n" 226 | + "For security reasons, the encryption process will generate a new HD seed, leading to the creation of a fresh set of active descriptors. "
"For security reasons, the encryption process will generate a new HD seed, leading to the creation of a fresh set of active descriptors.\n"
nit, new line forgotten? (sry didn't notice at previous review)
That was on purpose. To continue the paragraph.
optional nit
"For security reasons, the encryption process will generate a new HD seed, resulting in the creation of a fresh set of active descriptors. "
I like the wording by @pablomartin4btc
done
ack
ACK 40a5117c020e6ecfc5b57909900eb7afb8a486d9
re ACK 30983e4e30a136ce7d2efc3639f0c17ed89ea38b
219 | @@ -220,7 +220,11 @@ RPCHelpMan encryptwallet() 220 | "After this, any calls that interact with private keys such as sending or signing \n" 221 | "will require the passphrase to be set prior the making these calls.\n" 222 | "Use the walletpassphrase call for this, and then walletlock call.\n" 223 | - "If the wallet is already encrypted, use the walletpassphrasechange call.\n", 224 | + "If the wallet is already encrypted, use the walletpassphrasechange call.\n" 225 | + "** IMPORTANT **\n" 226 | + "For security reasons, the encryption process will generate a new HD seed, resulting in the creation of a fresh set of active descriptors. " 227 | + "Therefore, it is crucial to replace any existing backups with the newly generated wallet file, containing the new encrypted keys.\n"
Replace is not correct, because the old keys can still have coins on them.
We should recommend:
a) making a new backup, bot not removing/replacing the old one just yet
b) optional. In case the user believes the old keys could be compromised then direct them to sweep the old keys, e.g. with sendall RPC
UPD: After thinking more, I understood it depends on how the users performs the back up. If they back up the whole wallet file - it's safe. But just backing up the new HD seed is not safe. So, it seems recommending replacing backup is still slightly dangerous.
UPD: After thinking more, I understood it depends on how the users performs the back up. If they back up the whole wallet file - it's safe. But just backing up the new HD seed is not safe. So, it seems recommending replacing backup is still slightly dangerous.
Well, core currently lacks a mechanism for retrieving the HD seed. It refrains from outputting the seed post wallet creation and advises the user to back up the entire wallet file instead. The same procedure can be seen after encryption at the GUI level too: link.
And yeah, replacing the old wallet file with the new one is safe, as it retains old keys after encryption. The only change is the storage of the new master seed and its derived descriptors.
s/file, containing/file that contains/
Suggest:
"Therefore, it is crucial to make a fresh backup of the newly generated wallet file."
To make it even more clear to the end user that i) a backup is imperative and ii) that a new backup will contain both sets of descriptors, you could consider something like:
"After wallet encryption is complete and before making any transactions, you must immediately make a new backup of the wallet file, which will contain the new and old descriptors, using the `backupwallet` RPC.\n"
"Therefore, it is crucial to make a fresh backup of the newly generated wallet file."
The goal behind the "replace" wording is to avoid situations where users restore the wallet from the previous backup, thinking it is the latest one, and blames core for not detecting the balance.
and blames core for not detecting the balance
I think I would avoid recommending that users delete (replace) their wallet files, as potential loss of funds is a worse case.
To make it even more clear to the end user that i) a backup is imperative and ii) that a new backup will contain both sets of descriptors, you could consider something like:
"After wallet encryption is complete and before making any transactions, you must immediately make a new backup of the wallet file, which will contain the new and old descriptors, using the `backupwallet` RPC.\n"
I think the current text is slightly better because it alerts users without scaring them. But, if there is a higher preference for it, could change it.
Aside from that, agree on mentioning backupwallet there 👍🏼 .
219 | @@ -220,7 +220,11 @@ RPCHelpMan encryptwallet() 220 | "After this, any calls that interact with private keys such as sending or signing \n" 221 | "will require the passphrase to be set prior the making these calls.\n" 222 | "Use the walletpassphrase call for this, and then walletlock call.\n" 223 | - "If the wallet is already encrypted, use the walletpassphrasechange call.\n", 224 | + "If the wallet is already encrypted, use the walletpassphrasechange call.\n" 225 | + "** IMPORTANT **\n" 226 | + "For security reasons, the encryption process will generate a new HD seed, resulting in the creation of a fresh set of active descriptors. " 227 | + "Therefore, it is crucial to replace any existing backups with the newly generated wallet file, containing the new encrypted keys.\n" 228 | + "Please exercise caution and ensure that you securely backup the updated wallet file to safeguard your funds and wallet information.\n",
"backup" is a noun spelled as a single word, while "back up" is a verb spelled as 2 words
"Please exercise caution and ensure that you securely back up the updated wallet file to safeguard your funds and wallet information.\n",
Sure. Done as suggested
Concept ACK, suggest the doc: prefix or rpc, doc: for the pull title
Updated per feedback. Thanks everyone.
utACK
219 | @@ -220,7 +220,11 @@ RPCHelpMan encryptwallet() 220 | "After this, any calls that interact with private keys such as sending or signing \n" 221 | "will require the passphrase to be set prior the making these calls.\n" 222 | "Use the walletpassphrase call for this, and then walletlock call.\n" 223 | - "If the wallet is already encrypted, use the walletpassphrasechange call.\n", 224 | + "If the wallet is already encrypted, use the walletpassphrasechange call.\n" 225 | + "** IMPORTANT **\n" 226 | + "For security reasons, the encryption process will generate a new HD seed, resulting in the creation of a fresh set of active descriptors. " 227 | + "Therefore, it is crucial to replace any existing backups with the newly generated wallet file that contains the new encrypted keys.\n" 228 | + "Please exercise caution and ensure that you securely back up the updated wallet file to safeguard your funds and wallet information. See `backupwallet` RPC.\n",
This is now a bit verbose and redundant, and does not address #28980 (review).
Suggest:
"Therefore, it is crucial to securely back up the newly generated wallet file using the backupwallet RPC."
Also suggest the doc: prefix or rpc, doc: for the pull title.
It seems that we commented at the same time and I missed #28980 (review). Thanks for pointing it out.
Suggestions applied. Thanks.
Updated per feedback. Thanks. Applied #28980 (review) suggestions.
reACK e6b9a6dba79144cbe77c59515541f9019d1db4b5
ACK e6b9a6dba79144cbe77c59515541f9019d1db4b5
This change would result in the following help:
$ ./src/bitcoin-cli -signet help encryptwallet
encryptwallet "passphrase"
Encrypts the wallet with 'passphrase'. This is for first time encryption.
After this, any calls that interact with private keys such as sending or signing
will require the passphrase to be set prior the making these calls.
Use the walletpassphrase call for this, and then walletlock call.
If the wallet is already encrypted, use the walletpassphrasechange call.
** IMPORTANT **
For security reasons, the encryption process will generate a new HD seed, resulting in the creation of a fresh set of active descriptors. Therefore, it is crucial to securely back up the newly generated wallet file using the backupwallet RPC.
If you agree and/or need to retouch, maybe do
- "For security reasons, the encryption process will generate a new HD seed, resulting in the creation of a fresh set of active descriptors. "
- "Therefore, it is crucial to securely back up the newly generated wallet file using the backupwallet RPC.\n",
+ "For security reasons, the encryption process will generate a new HD seed, resulting\n"
+ "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n"
+ "securely back up the newly generated wallet file using the backupwallet RPC.\n",
for a more harmonious output
$ ./src/bitcoin-cli -signet help encryptwallet
encryptwallet "passphrase"
Encrypts the wallet with 'passphrase'. This is for first time encryption.
After this, any calls that interact with private keys such as sending or signing
will require the passphrase to be set prior the making these calls.
Use the walletpassphrase call for this, and then walletlock call.
If the wallet is already encrypted, use the walletpassphrasechange call.
** IMPORTANT **
For security reasons, the encryption process will generate a new HD seed, resulting
in the creation of a fresh set of active descriptors. Therefore, it is crucial to
securely back up the newly generated wallet file using the backupwallet RPC.
Also, should we change this as well?
},
@@ -271,7 +272,7 @@ RPCHelpMan encryptwallet()
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Failed to encrypt the wallet.");
}
- return "wallet encrypted; The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup.";
+ return "wallet encrypted; The keypool has been flushed and a new HD seed was generated. You need to make a new backup with the backupwallet RPC.";
Better to notify users about the HD seed rotation and the new
backup requirement before executing the encryption process.
Ensuring they are prepared to update previous backups and
securely safeguard the updated wallet file.
Co-authored-by: jonatack <jon@atack.com>
ACK ca09415e630f0f7de9160cab234bd5ba6968ff2d
ack ca09415e630f0f7de9160cab234bd5ba6968ff2d
ack ca09415e630f0f7de9160cab234bd5ba6968ff2d
ACK ca09415e630f0f7de9160cab234bd5ba6968ff2d
Post-merge ACK