-
Notifications
You must be signed in to change notification settings - Fork 37.6k
Have createwalletdescriptor auto-detect an unused(KEY) #32861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
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.
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. 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. |
Once the unused(KEY) descriptor is used, doesn't it become used? Keeping it as an unused descriptor later might come across as confusing. |
} else if (wallet_xpubs.size() > 1) { | ||
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to determine which HD key to use. Please specify with 'hdkey'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In e8b1440 "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) |
The
createwalletdescriptor
was introduced in #29130 to let users add atr()
descriptor to an existing SegWit wallet. The newaddhdkey
method 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
gethdkeys
and copy-paste the xpub.This PR makes
createwalletdescriptor
a bit smarter so it just finds theunused(KEY)
generated byhdkey
and 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-segwit
andbech32
addresses.(I might do that in #32784, which introduces some other improvements to the tutorial. The situation is still not great; ideally the
importdescriptors
RPC is enhanced to detect when anxpub
is a child of an hd key and then treats it as if an xpriv was imported.)Builds on #29136.