wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor
#29136
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29136. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
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)" 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 (comment)). But this seems like a very good start and very functional. |
5338f09 to
18c2d9a
Compare
18c2d9a to
3e6a00d
Compare
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.
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. |
42f1fc8 to
22c8984
Compare
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 Another approach that might be acceptable could be to add a 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. |
51d18d6 to
d457faf
Compare
Indeed, this is a bit of a hack that happens to work nicely with our wallet design. If other wallets end up using it too, we could reconsider that. |
d457faf to
b23cefb
Compare
|
utACK b23cefb |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
unused() descriptors do not have scriptPubKeys. Instead, the wallet uses them to store keys without having any scripts to watch for.
b23cefb to
b4ef67a
Compare
|
Rebased for silent merge conflict |
|
There might be a silent conflict with #32514. |
One use case for this is a multisig setup that starts from a blank wallet with an unused(KEY) descriptor (proposed in bitcoin#29136). A descriptor is then imported that contains both a key from this wallet and that of an external signer.
b4ef67a to
6100108
Compare
|
re-ACK 6100108 |
|
While working on #32784 I noticed we don't seem to be rotating Another thing I noticed is that |
One use case for this is a multisig setup that starts from a blank wallet with an unused(KEY) descriptor (proposed in bitcoin#29136). A descriptor is then imported that contains both a key from this wallet and that of an external signer.
One use case for this is a multisig setup that starts from a blank wallet with an unused(KEY) descriptor (proposed in bitcoin#29136). A descriptor is then imported that contains both a key from this wallet and that of an external signer.
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
unused(KEY)descriptor which allows for a key to be specified, but produces no scripts. These can be imported into the wallet, and subsequently retrieved withgethdkeys. Additionally,listdescriptorswill 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
addhdkeyRPC is also added. Without arguments, it will generate a new HD key and add it to the wallet via aunused(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
gethdkeysis useful for testing this.