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 copy-paste 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. LLM Linter (✨ experimental)Possible places where named args for integral literals may be used (e.g.
2026-04-29 22:16:01 |
|
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. |
439734e to
6928cad
Compare
|
re-utACK 6928cad Just a rebase. |
fjahr
left a comment
There was a problem hiding this comment.
Concept ACK
Looks already pretty good to me.
6928cad to
23f5529
Compare
|
This probably needs a rebase due to |
unused() descriptors do not have scriptPubKeys. Instead, the wallet uses them to store keys without having any scripts to watch for.
|
rebased |
23f5529 to
a39cc16
Compare
|
utACK a39cc16 |
| def skip_test_if_missing_module(self): | ||
| self.skip_if_no_wallet() | ||
|
|
||
| def test_addhdkey(self): |
There was a problem hiding this comment.
In f3f8bcb "wallet: Add addhdkey RPC"
- unload wallets after their use is done
- asserting both the unused desc in listdescriptors RPC
- calling the RPCs with private option
Can be done in follow-up.
Functional Test Diff
diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py
index e5227e6f49..548d6b0575 100755
--- a/test/functional/wallet_hd.py
+++ b/test/functional/wallet_hd.py
@@ -40,6 +40,7 @@ class WalletHDTest(BitcoinTestFramework):
assert_equal(len(xpub_info), 2)
for x in xpub_info:
if len(x["descriptors"]) == 1 and x["descriptors"][0]["desc"].startswith("unused("):
+ unused_desc_pub_1 = x["descriptors"][0]["desc"]
break
else:
assert False, "Did not find HD key with no descriptors"
@@ -50,32 +51,38 @@ class WalletHDTest(BitcoinTestFramework):
assert_raises_rpc_error(-5, "Extended public key (xpub) provided, but extended private key (xprv) is required", wallet.addhdkey, imp_xpub)
add_res = wallet.addhdkey(imp_xprv)
- expected_unused_desc = descsum_create(f"unused({imp_xpub})")
+ unused_desc_pub_2 = descsum_create(f"unused({imp_xpub})")
assert_equal(add_res["xpub"], imp_xpub)
xpub_info = wallet.gethdkeys()
assert_equal(len(xpub_info), 3)
for x in xpub_info:
if x["xpub"] == imp_xpub:
assert_equal(len(x["descriptors"]), 1)
- assert_equal(x["descriptors"][0]["desc"], expected_unused_desc)
+ assert_equal(x["descriptors"][0]["desc"], unused_desc_pub_2)
break
else:
assert False, "Added HD key was not found in wallet"
+ unused_descs_num = 0
for d in wallet.listdescriptors()["descriptors"]:
- if d["desc"] == expected_unused_desc:
+ if d["desc"] == unused_desc_pub_1 or d["desc"] == unused_desc_pub_2:
assert_equal(d["active"], False)
- break
- else:
- assert False, "Added HD key's descriptor was not found in wallet"
+ unused_descs_num += 1
+ assert_equal(unused_descs_num, 2)
+
+ # Calling the same RPCs with private=True should not fail
+ wallet.gethdkeys(private=True)
+ wallet.listdescriptors(private=True)
assert_raises_rpc_error(-4, "HD key already exists", wallet.addhdkey, imp_xprv)
+ wallet.unloadwallet()
def test_addhdkey_noprivs(self):
self.log.info("Test addhdkey is not available for wallets without privkeys")
self.nodes[0].createwallet("hdkey_noprivs", disable_private_keys=True)
wallet = self.nodes[0].get_wallet_rpc("hdkey_noprivs")
assert_raises_rpc_error(-4, "addhdkey is not available for wallets without private keys", wallet.addhdkey)
+ wallet.unloadwallet()
def run_test(self):
# Make sure we use hd, keep masterkeyid|
I just realized the feedback in #32861 (review) was for this PR, so I implemented both suggestions in #32861. |
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.