wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor #29136

1. 
   achow101 commented at 11:40 pm on December 22, 2023: member

   It is sometimes useful for the wallet to have keys that it can sign with but are not (initially) involved in any scripts, e.g. for setting up a multisig. Ryanofsky suggested A void(KEY) descriptor which allows for a key to be specified, but produces no scripts. These can be imported into the wallet, and subsequently retrieved with gethdkeys. Additionally, listdescriptors will output these descriptors so that they can be easily backed up.

   In order to make it easier for people to add HD keys to their wallet, and to generate a new one if they want to rotate their descriptors, an addhdkey RPC is also added. Without arguments, it will generate a new HD key and add it to the wallet via a void(KEY) descriptor. If provided a private key, it will construct the descriptor and add it to the wallet.

   See also: #26728 (comment)

   Based on #29130 as gethdkeys is useful for testing this.

2. 
   DrahtBot commented at 11:40 pm on December 22, 2023: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29136.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	Sjors

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30243 (descriptors: taproot partial descriptors by Eunovo)
   • #28710 (Remove the legacy wallet and BDB dependency by achow101)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. DrahtBot added the label Wallet on Dec 22, 2023
4. DrahtBot added the label CI failed on Dec 23, 2023
5. 
   ryanofsky commented at 4:40 am on December 23, 2023: contributor

   This is great, amazed you could implement this so quickly!

   I was thinking about the void(KEY) idea more today, and think it would be good to make 3 tweaks:

   1 - rename “void(KEY)” to “unused(KEY)” 2 - add logic to importdescriptors and generatewalletdescriptor to delete any unused(KEY) descriptor as soon as the KEY which it references is used by an new descriptor (taking into account both public and private parts of the key) 3 - add logic to importdescriptors to disallow importing an unused(KEY) if KEY is used by another descriptor.

   I think these changes would help make these descriptors easier to understand and also enhance backwards compatibility, because IIUC wallets containing unknown descriptor types can’t be opened by older software. Also keeping redundant descriptors in the wallet that were only temporarily needed is confusing.

   If making these tweaks isn’t possible or is not a good idea. I still think probably we should rename void(KEY) to data(KEY) or something like that. I think while “void” makes a certain amount of sense as programming jargon, it’s not really a familiar term and doesn’t describe the purpose of these descriptors, which is just to hold an inert piece of data, and not be used for generating or matching scriptpubkeys.

   I also have a number of ideas to improve the RPC interface for generating keys and descriptors, and will try to post them soon. (EDIT: now posted #29130 (review)). But this seems like a very good start and very functional.

6. achow101 force-pushed on Jan 3, 2024
7. DrahtBot removed the label CI failed on Jan 3, 2024
8. achow101 force-pushed on Jan 4, 2024
9. 
   achow101 commented at 8:03 pm on January 4, 2024: member

   1 - rename “void(KEY)” to “unused(KEY)” 3 - add logic to importdescriptors to disallow importing an unused(KEY) if KEY is used by another descriptor.

   Done these. I’ve also added a further restriction that unused() cannot be import to a wallet without private keys. It isn’t useful in such wallets so I think it makes sense to disallow their import.

   2 - add logic to importdescriptors and generatewalletdescriptor to delete any unused(KEY) descriptor as soon as the KEY which it references is used by an new descriptor (taking into account both public and private parts of the key)

   Still working and thinking on this. However we’ve generally held to the policy of not deleting any records from the wallet since that could result in private keys being accidentally deleted. The only exception to that was adding encryption.

10. achow101 force-pushed on Jan 4, 2024
11. achow101 force-pushed on Jan 4, 2024
12. DrahtBot added the label CI failed on Jan 4, 2024
13. DrahtBot removed the label CI failed on Jan 4, 2024
14. 
    ryanofsky commented at 5:41 pm on January 8, 2024: contributor

    However we’ve generally held to the policy of not deleting any records from the wallet since that could result in private keys being accidentally deleted. The only exception to that was adding encryption.

    Oh, I didn’t know that but it makes sense.

    One approach you could take would just be to delete the descriptor from memory without actually deleting it from the database, and ignore it the next time the wallet is loaded. But a drawback of this would be that once addhdkey was used the wallet would be forever incompatible with older versions of the software, when one of the benefits of deleting the descriptor was to make wallets backward compatible.

    Another approach that might be acceptable could be to add a DatabaseBatch::MarkErased method to call instead of DatabaseBatch::Erase. This could just prepend a serialized string like const std::string TOMBSTONE{"tombstone"} to the key and otherwise leave the record unchanged.

    Or maybe just decide in this case that it is ok to delete a record after verifying all the information it contains is present in other records.

    Would also want to think about it more, though.

15. DrahtBot added the label CI failed on Jan 15, 2024
16. DrahtBot added the label Needs rebase on Feb 20, 2024
17. achow101 force-pushed on Feb 20, 2024
18. DrahtBot removed the label Needs rebase on Feb 20, 2024
19. DrahtBot removed the label CI failed on Feb 20, 2024
20. DrahtBot added the label Needs rebase on Mar 29, 2024
21. achow101 force-pushed on Mar 29, 2024
22. achow101 marked this as ready for review on Mar 29, 2024
23. DrahtBot removed the label Needs rebase on Mar 29, 2024
24. DrahtBot added the label Needs rebase on Aug 28, 2024
25. achow101 force-pushed on Aug 29, 2024
26. DrahtBot removed the label Needs rebase on Aug 29, 2024
27. 
    DrahtBot commented at 4:27 am on August 30, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29446941801

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

28. DrahtBot added the label CI failed on Aug 30, 2024
29. achow101 force-pushed on Sep 3, 2024
30. achow101 force-pushed on Sep 10, 2024
31. achow101 force-pushed on Sep 18, 2024
32. DrahtBot removed the label CI failed on Sep 18, 2024
33. DrahtBot added the label CI failed on Oct 20, 2024
34. DrahtBot removed the label CI failed on Oct 24, 2024
35. 
    Sjors commented at 3:18 pm on January 16, 2025: member

    Concept ACK. This seems like a step in the right direction.

    I tried updating the multisig tutorial to take advantage of this in https://github.com/Sjors/bitcoin/commit/7bb9a172c6539de5c02ac70f9e38724a0e3e82dc. It also uses gethdkeys and the new <0;1> notation.

    But it’s still a massive pain for two reasons:

    1. There’s no way to get the xpub at m/87'/1'/0'; I created a workaround which inserts a dummy pk() with that derivation, but this is ugly and requires passing the master xprv around. So we still need something similiar to gethdkey PATH like I attempted in #22341.
    2. When importing a descriptor with only xpubs the wallet isn’t smart enough to know it owns one of them. So each participant either: a) creates a watch-only wallet, and signs with the “empty” private key wallet; or b) inserts their xpriv into the right position of the multisig descriptor

    2a is what the tutorial does. I played around with 2b but completely messed up the test wallet.

    That doesn’t have to be solved in this PR though.

36. DrahtBot added the label CI failed on Mar 13, 2025
37. 
    DrahtBot commented at 11:34 am on March 15, 2025: contributor

    Needs rebase, if still relevant
38. descriptor: Add unused(KEY) descriptor

    unused() descriptors do not have scriptPubKeys. Instead, the wallet uses
    them to store keys without having any scripts to watch for.

    11767a65d0
39. test: Simple test for importing unused(KEY) f02f6cefad
40. wallet: Add addhdkey RPC 504c5f0df8
41. test: Test for addhdkey 9e70f515a0
42. wallet, rpc: Disallow import of unused() if key already exists 09e5a684fe
43. wallet, rpc: Disallow importing unused() to wallets without privkeys cbf4dd3e41
44. achow101 force-pushed on Apr 10, 2025
45. DrahtBot removed the label CI failed on Apr 10, 2025

Contributors
achow101 DrahtBot ryanofsky Sjors

Labels
Wallet