RPC: Better safety with newkeypool command and wallet backups #23341

1. 
   meshcollider commented at 1:38 AM on October 23, 2021: contributor

   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 that the RPC help text should include a warning to the users about the interaction between newkeypool.

2. meshcollider added the label RPC/REST/ZMQ on Oct 23, 2021
3. meshcollider added the label Wallet on Oct 23, 2021
4. meshcollider cross-referenced this on Oct 23, 2021 from issue Add ability to flush keypool and always flush when upgrading non-HD to HD by meshcollider
5. lsilva01 approved
6. 
   lsilva01 commented at 2:28 AM on October 25, 2021: contributor

   Code Review ACK 04153bc

7. 
   stratospher commented at 1:42 PM on October 28, 2021: contributor

   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.
   

8. luke-jr changes_requested
9. 
   luke-jr commented at 4:52 PM on October 29, 2021: member

   Concept NACK. Users of non-HD wallets expect to make new backups regularly. No reason to deny them this RPC method...

10. 
    meshcollider commented at 8:42 PM on October 29, 2021: contributor

    @luke-jr can you provide a reason for its usefulness? It was only just added, so I can't imagine anyone relying on it...

11. 
    luke-jr commented at 11:08 PM on October 29, 2021: member

    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.

12. 
    meshcollider commented at 4:40 AM on October 30, 2021: contributor

    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?

13. DrahtBot cross-referenced this on Dec 2, 2021 from issue MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet by meshcollider
14. 
    DrahtBot commented at 2:38 PM on December 2, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

15. DrahtBot cross-referenced this on Dec 4, 2021 from issue Split up rpcwallet by meshcollider
16. 
    meshcollider commented at 3:56 AM on December 8, 2021: contributor

    Rebased.

17. 
    achow101 commented at 4:57 PM on December 10, 2021: member

    I agree with @luke-jr. This RPC is useful for non-HD wallets too. It just need to come with large warnings about backups.

18. rpc: Add warning to user about newkeypool command a2a92317ad
19. 
    meshcollider commented at 6:05 AM on December 12, 2021: contributor

    Done 👍

20. meshcollider added this to the milestone 23.0 on Dec 20, 2021
21. 
    achow101 commented at 7:39 PM on December 20, 2021: member

    ACK a2a92317ad4ab88cfca234f68337a7cd37897f10

22. achow101 merged this on Dec 20, 2021
23. achow101 closed this on Dec 20, 2021

    ---

24. meshcollider deleted the branch on Dec 20, 2021
25. sidhujag referenced this in commit 673b1dcd79 on Dec 20, 2021
26. bitcoin locked this on Dec 20, 2022

Contributors

meshcolliderlsilva01stratospherluke-jrDrahtBotachow101

Milestone
23.0