RPC: Better safety with newkeypool command and wallet backups#23341
RPC: Better safety with newkeypool command and wallet backups#23341achow101 merged 1 commit intobitcoin:masterfrom meshcollider:202110_newkeypool_documentation
Conversation
lsilva01
left a comment
lsilva01
left a comment
There was a problem hiding this comment.
Code Review ACK 04153bc
stratospher
left a comment
stratospher
left a comment
There was a problem hiding this comment.
Tested ACK 04153bc on a non HD wallet. The warning is really helpful.
$ src/bitcoin-cli --testnet -rpcwallet=mynonHD newkeypool
error code: -4
error message:
Error: Cannot use newkeypool on non-HD wallets.
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
Concept NACK. Users of non-HD wallets expect to make new backups regularly. No reason to deny them this RPC method...
|
@luke-jr can you provide a reason for its usefulness? It was only just added, so I can't imagine anyone relying on it... |
|
Maybe I'm missing the point of it entirely? I'm just saying artificially blocking non-HD wallets from using it seems unreasonable. I can't think of a use case for this with a HD wallet (won't the new keypool just end up with the same keys?), but it seems useful for non-HD wallets to intentionally avoid new funds from being accessible with old (compromised?) backups. |
|
HD wallets will be restocked with entirely new keys. That's an interesting use-case, I mainly had in mind people who have upgraded non-HD to HD and want to clear out all their non-HD keys in one go. But I can just add some large warnings in the non-HD case. In that case though @luke-jr why not just make a new wallet? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
Rebased. |
|
I agree with @luke-jr. This RPC is useful for non-HD wallets too. It just need to come with large warnings about backups. |
|
Done 👍 |
|
ACK a2a9231 |
…allet backups a2a9231 rpc: Add warning to user about newkeypool command (Samuel Dobson) Pull request description: This PR prevents `newkeypool` from being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe. David Harding also suggested [here](bitcoin#23093 (comment)) that the RPC help text should include a warning to the users about the interaction between newkeypool. ACKs for top commit: achow101: ACK a2a9231 Tree-SHA512: 0aa497900f1d179764bce13ffce0bb081ba2ca354492bf2e04b21d0212e960b3ed13a386fbf65602b6b50461f4056a0285752ef707d312da28e82449cd8ea048
6 participants
This PR prevents
newkeypoolfrom being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe.David Harding also suggested here that the RPC help text should include a warning to the users about the interaction between newkeypool.