Have createwalletdescriptor auto-detect an unused(KEY)#32861
Have createwalletdescriptor auto-detect an unused(KEY)#32861Sjors wants to merge 8 commits intobitcoin:masterfrom
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/32861. 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-02-02 15:54:16 |
Once the unused(KEY) descriptor is used, doesn't it become used? Keeping it as an unused descriptor later might come across as confusing. |
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
In e8b1440aa3ed7efe3a9c2d10afb05e7247fff1f6 "rpc: make createwalletdescriptor smarter"
Though this check here seems more thorough, but the presence of more than one spkm also seems sufficient to throw this error?
I agree, and it was also brought up here: #29136 (comment). It's orthogonal to this PR. |
|
Thanks, I recall reading this comment earlier but forgot about it later. If I am not missing anything, I don't suppose this point is orthogonal to this PR? The linked comment also states that any RPC using the unused descriptor should delete it immediately afterwords.
I don't believe diff --git a/test/functional/wallet_createwalletdescriptor.py b/test/functional/wallet_createwalletdescriptor.py
index 6de0ca4782..a7086c5b9e 100755
--- a/test/functional/wallet_createwalletdescriptor.py
+++ b/test/functional/wallet_createwalletdescriptor.py
@@ -125,7 +125,11 @@ class WalletCreateDescriptorTest(BitcoinTestFramework):
# Create unused(KEY) descriptor and try again
w1.addhdkey()
+ print("listdescriptors: ", w1.listdescriptors())
+ print("gethdkeys: ", w1.gethdkeys())
w1.createwalletdescriptor(type="bech32")
+ print("listdescriptors: ", w1.listdescriptors())
+ print("gethdkeys: ", w1.gethdkeys())
self.nodes[0].createwallet("w2", blank=True)
w2 = self.nodes[0].get_wallet_rpc("w2")
(END) |
e8b1440 to
6049553
Compare
That should be done in #29136 which introduces |
6049553 to
f2752ef
Compare
unused() descriptors do not have scriptPubKeys. Instead, the wallet uses them to store keys without having any scripts to watch for.
A helper method to obtain all unused(key) descriptor SPKMs.
When a wallet contains only an unused(KEY) descriptor, use it. Previously the user would have to call listdescriptors and manually specify it.
f2752ef to
c8b6d4e
Compare
| // Check both only have one pubkey | ||
| std::set<CPubKey> prv_pubkeys; | ||
| std::set<CExtPubKey> prv_extpubs; | ||
| parse_pub->GetPubKeys(prv_pubkeys, prv_extpubs); |
There was a problem hiding this comment.
Should this call use parse_priv instead of parse_pub?
| } | ||
| } | ||
|
|
||
| LOCK(wallet->cs_wallet); |
There was a problem hiding this comment.
I think a duplicate check is needed. What if the wallet already has this xprv through an active descriptor and user calls addhdkey with the same xprv? The descriptor check would not catch that, because unused(xprv) is a different descriptor string. A check is needed here to prevent adding the same private key twice.
const CExtPubKey hd_xpub{hdkey.Nexuter()};
if (wallet->GetKey(hd_xpub.pubkey.GetID())) {
throw JSONRPCError(RPC_WALLET_ERROR, "HD key already exists");
}
| wallet = self.nodes[0].get_wallet_rpc("hdkey") | ||
|
|
||
| assert_equal(len(wallet.gethdkeys()), 1) | ||
|
|
There was a problem hiding this comment.
Cover the case where addhdkey is called with existing xprv. Expected behavior is to reject it as "HD key already exists"
existing_wallet_xprv = wallet.gethdkeys(private=True)[0]["xprv"]
assert_raises_rpc_error(-4, "HD key already exists", wallet.addhdkey, existing_wallet_xprv)
|
Review and tested c8b6d4e Concept ACK, Approach ACK. I think I have found a small issue where a xprv already in wallet via active descriptors, and addhdkey(xprv) is called from RPC with the same xprv. Suggested a fix in the review comments. |
The
createwalletdescriptorwas introduced in #29130 to let users add atr()descriptor to an existing SegWit wallet. The newaddhdkeymethod from #29136 introduces a new potential workflow: start from a blank wallet, generate an HD and then add only the descriptors you need, e.g.:Before this PR the last line would fail, requiring the user to call
gethdkeysand copy-paste the xpub.This PR makes
createwalletdescriptora bit smarter so it just finds theunused(KEY)generated byhdkeyand uses that.If multiple
unused(KEY)descriptors are present the user still has to pick one.A potential followup is to make our multisig tutorial slightly safer to use. Rather than creating a full wallet, the instruction could be changed to start with a blank wallet and only generate the default legacy descriptor. This avoids accidental use of single sig
p2sh-segwitandbech32addresses.(I might do that in #32784, which introduces some other improvements to the tutorial. The situation is still not great; ideally the
importdescriptorsRPC is enhanced to detect when anxpubis a child of an hd key and then treats it as if an xpriv was imported.)Builds on #29136.