wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors
#29130
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
achow101
force-pushed
the
createwalletdescriptor-without-new-records
branch
2 times, most recently
from
7207185
to
3b7a264
Compare
achow101
force-pushed
the
createwalletdescriptor-without-new-records
branch
2 times, most recently
from
e55e228
to
08b9b41
Compare
src/wallet/rpc/wallet.cpp
Outdated
| "gethdkeys", | ||
| "\nList all BIP 32 HD keys in the wallet and which descriptors use them.\n", | ||
| { | ||
| {"active_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show the keys for only active descriptors"}, |
There was a problem hiding this comment.
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad)
Note: This is better than what I suggested. I was originally thinking this default should be true, to make it more convenient to get active hd key and ignore other keys. But defaulting to false is actually better, because it's still easy to get the active hd key with an option, and it's probably more confusing to see missing keys than extra keys. Also this default makes gethdkeys output more consistent with listdescriptors output.
achow101
force-pushed
the
createwalletdescriptor-without-new-records
branch
4 times, most recently
from
8286b43
to
e0ad5bc
Compare
| }; | ||
| } | ||
|
|
||
| static RPCHelpMan createwalletdescriptor() |
There was a problem hiding this comment.
Since this PR is adding the createwalletdescriptor method, maybe this is a good place to list some ways it could be extended in the future:
-
Probably it would be good not to require wallet to be unlocked when dealing with public keys. Currently the specified
hdkeyis unencrypted and reencrypted, but this shouldn't be necessary because the key is already in the wallet. (The only reason this seems to happen now is to copy the key, because internally we store keys in a slightly denormalized format, once per descriptor.) -
It would be nice if
hdkeyparameter accepted not just public hd keys, but also private hd keys, and a "generate" value in a blank wallet so users don't need to chain together multiple commands to accomplish simple tasks, and so we only need two RPC methods which add descriptors and keys to the wallet:importdescriptorsandcreatewalletdescriptor, without a thirdaddhdkeymethod. Examples:# In a blank wallet, generate an hd key and use it in a new descriptor createwalletdescriptor bech32 generate # In a blank wallet, import an hd key and use it in a new descriptor createwalletdescriptor bech32 xprv...
-
It sounds like we want to discourage having multiple hd keys per wallet, and encourage having separate wallets instead. But if we did want to allow these, we could add a
forceoption to allow creating descriptors with new hd keys even when existing hd keys are present. We could also allow arotateoption to allow this and additionally set descriptors using old keys to be inactive. -
The
hdkeyparameter could accept hd master keys in different formats, for example as seed keys like the currentsethdseedmethod, or seed hex strings like #29054, or seed shares like #27351. -
It would be nice if
typeparameter accepted a "defaults" value to set up descriptors for all default output types. Examples:# In a blank wallet, generate an hd key and generate default set of descriptors createwalletdescriptor defaults generate # Add missing descriptors to an existing wallet. For example, upgrade # an older wallet not supporting bech32m to support it. createwalletdescriptor defaults
-
To support multisig,
typeparameter could accept an "hdkey" value to generate an unused hd key in a blank wallet:# In a blank wallet, generate an hd key in an unused descriptor and output unused(xpub...) createwalletdescriptor hdkey generate # Alternately, import an unused key createwalletdescriptor hdkey xprv... # Import multisig descriptor with wallet public keys importdescriptors [...]
-
Maybe in the future to make multisig setup easier,
typeparameter could accept a "multisig" value and additional options to make it easier to create the descriptor using the right keys without doing extra work or using an outside tool. -
Not recommending it, but I could imagine
typeandhdkeyparameters being extended to accept other values in the future. For example maybe with #27351, a "codex32" type could be useful. And if a wallet didn't have the same hdkey for every output type, it might be useful to be able specify "default' for the hdkey parameter to use the hd key from the default output type, likegetnewaddress. -
It could make sense for
createwalletdescriptorandimportdescriptorsto have other options in common. For example, it might make sense forcreatewalletdescriptorto accept atimestampoption when adding a descriptor with an existing hdkey, to rescan for transactions with the new descriptor. In generalcreatewalletdescriptorcould be a higher level alternative toimportdescriptorsthat is less flexible but easier to use for common tasks. -
It could make sense to add an "external" type and
hd_accountoption (see #29129 (comment)) for external signer wallets :# Add descriptors from the external signer with the specified BIP44 account createwalletdescriptor type=external hd_account=123
Given a ScriptPubKeyMan, it's useful to ask the wallet whether it is currently active.
achow101
force-pushed
the
createwalletdescriptor-without-new-records
branch
from
36c82cd
to
bb1b405
Compare
There was a problem hiding this comment.
Users will not be able to derive the last hardened child only with the root xpub info on the descriptor. Is there any rationale behind this or was just an overlook?
This was intentional since the last hardened child is different for each script type. I think future work, such as a revival of #22341, can make it possible to get keys at other derivation paths.
But how the current output is useful? No outputted descriptor can be imported in another wallet. The wallet will throw a "Cannot expand descriptor. Probably because of hardened derivations without private keys provided". What do you think about also retrieving the normalized descriptor in another field desc_normalized?
achow101
force-pushed
the
createwalletdescriptor-without-new-records
branch
from
bb1b405
to
d7fc8a8
Compare
Sorry, misread your comment. The key is supposed to be the root, but the descriptors should be normalized. I've changed that. |
| UniValue response(UniValue::VARR); | ||
| for (const auto& [xpub, descs] : wallet_xpubs) { | ||
| bool has_xprv = false; | ||
| UniValue descriptors(UniValue::VARR); | ||
| for (const auto& [desc, active, has_priv] : descs) { | ||
| UniValue d(UniValue::VOBJ); | ||
| d.pushKV("desc", desc); |
There was a problem hiding this comment.
In 21d0bc3:
Better to return the descriptors sorted:
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
--- a/src/wallet/rpc/wallet.cpp (revision d7fc8a8bec5edb657f65ef35042d801e2913ab81)
+++ b/src/wallet/rpc/wallet.cpp (date 1710943877002)
@@ -874,7 +874,7 @@
spkms = wallet->GetAllScriptPubKeyMans();
}
- std::map<CExtPubKey, std::set<std::tuple<std::string, bool, bool>>> wallet_xpubs;
+ std::map<CExtPubKey, std::vector<std::tuple<std::string, bool, bool>>> wallet_xpubs;
std::map<CExtPubKey, CExtKey> wallet_xprvs;
for (auto* spkm : spkms) {
auto* desc_spkm{dynamic_cast<DescriptorScriptPubKeyMan*>(spkm)};
@@ -889,7 +889,7 @@
for (const CExtPubKey& xpub : desc_xpubs) {
std::string desc_str;
desc_spkm->GetDescriptorString(desc_str, false);
- wallet_xpubs[xpub].emplace(desc_str, wallet->IsActiveScriptPubKeyMan(*spkm), desc_spkm->HasPrivKey(xpub.pubkey.GetID()));
+ wallet_xpubs[xpub].emplace_back(desc_str, wallet->IsActiveScriptPubKeyMan(*spkm), desc_spkm->HasPrivKey(xpub.pubkey.GetID()));
if (std::optional<CKey> key = priv ? desc_spkm->GetKey(xpub.pubkey.GetID()) : std::nullopt) {
wallet_xprvs[xpub] = CExtKey(xpub, *key);
}
@@ -897,9 +897,15 @@
}
UniValue response(UniValue::VARR);
- for (const auto& [xpub, descs] : wallet_xpubs) {
- bool has_xprv = false;
+ for (auto& [xpub, descs] : wallet_xpubs) {
UniValue descriptors(UniValue::VARR);
+
+ // Sort descriptors
+ std::sort(descs.begin(), descs.end(), [](const auto& a, const auto& b) {
+ return std::get<0>(a) < std::get<0>(b);
+ });
+
+ bool has_xprv = false;
for (const auto& [desc, active, has_priv] : descs) {
UniValue d(UniValue::VOBJ);
d.pushKV("desc", desc);There was a problem hiding this comment.
I don't think it's meaningfully useful to explicitly sort them, and we don't do that anywhere else. Also, std::set does sorting itself, so I don't think this would even change the output.
There was a problem hiding this comment.
I don't think it's meaningfully useful to explicitly sort them, and we don't do that anywhere else.
We sort them in listdescriptors: src/wallet/rpc/backup.cpp#L1827.
Also,
std::setdoes sorting itself, so I don't think this would even change the output.
std::set sorts based on the top element, the std::tuple in this case. Not based on the descriptor string.
Yet, this was just an UX nit. Either way is fine for me.
There was a problem hiding this comment.
Sorting on std::tuple will sort lexicographically on the items in the tuple. Since the descriptor string is the first item in the tuple, and are unique, it should be sorted by the descriptor strings.
There was a problem hiding this comment.
Sorting on
std::tuplewill sort lexicographically on the items in the tuple. Since the descriptor string is the first item in the tuple, and are unique, it should be sorted by the descriptor strings.
Hmm, I was sure I saw a different ordering this morning. But 🤷🏼 ..
There was a problem hiding this comment.
Just tried and they're neatly grouped by type. tr() is listed before wpkh which takes getting used to since we introduced tr() later, but such is the alphabet.
| @@ -942,6 +942,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati | |||
|
|
|||
| //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers | |||
| std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const; | |||
| bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const; | |||
There was a problem hiding this comment.
tiny nit:
To follow the same pattern used across this class, should pass the spkm pointer instead of the const ref (and return early if it is nullptr).
Side note:
It is not really something that we should tackle nor discuss here but I think it would be better to return the is_active, and other spkm information, on the initial lookup function instead of introducing multiple functions that retrieves the information after wise. E.g. GetAllScriptPubKeyMansInfo() could return a struct instead of the bare spkm pointers.
There was a problem hiding this comment.
It's probably not a great idea to be passing around raw pointers and we should be moving away from doing that.
| @@ -4486,4 +4486,25 @@ void CWallet::TopUpCallback(const std::set<CScript>& spks, ScriptPubKeyMan* spkm | |||
| // Update scriptPubKey cache | |||
| CacheNewScriptPubKeys(spks, spkm); | |||
| } | |||
|
|
|||
| std::set<CExtPubKey> CWallet::GetActiveHDPubKeys() const | |||
There was a problem hiding this comment.
I do agree on encapsulating this function but have to say that it's inconsistent to how previous commits on this PR were implemented (gethdkeys accesses the wallet internals to obtain the same information).
Yet, this is not blocking. Just a comment. We lack of an structure here.
There was a problem hiding this comment.
gethdkeys can get pubkeys for inactive descriptors, so it can't use this function.
gethdkeys retrieves all HD keys stored in the wallet's descriptors.
We will need access to a function that sets up a singular DescriptorSPKM, so refactor this out of the multiple DescriptorSPKM setup function.
Adds CWallet::GetKey which retrieves a single key from the descriptors stored in the wallet.
achow101
force-pushed
the
createwalletdescriptor-without-new-records
branch
from
d7fc8a8
to
746b6d8
Compare
|
re-utACK 746b6d8
That's much better. It would be nice if you can call But we might as well do that in a followup along with some of the other suggestions in the that comment. For setting a up a multisig it's useful to have a seed without descriptor and a way to get an xpub at an arbitrary path from it. Right now, with or without this PR, you can only achieve that by creating a dummy wallet and taking the root xpriv. You then manually create the multisig descriptor, with the xpriv as your key and the xpub from your counterpary and import that that in the eventual multisig wallet. Then you use But if they also use Bitcoin Core you have a chicken-egg problem (which involves more workarounds). |
Agree. Also @Sjors, another possibility could be adding a 'generate-seed' command to the |
There was a problem hiding this comment.
Code review ACK 746b6d8, and this looks ready to merge. There were various suggested changes since last review where main change seems to be switching gethdkeys output to use normalized descriptors (removing hardened path components).
It does seem good to normalize descriptors so they can be imported without private keys, and to make gethdkeys consistent with listdescriptors. But I'm a little unclear about whether we always normalize descriptors in RPC output, or if there are cases where they are not normalized? It does seem like it could be potentially confusing to normalize descriptors and make them look different, but I'm not sure if we always do this.
Merged but still curious about this. I wonder if we should rename descriptor ToString() method to something like ToInternalString() or OriginalString() to prevent this mistake from happening again, if preference is to output normalized descriptors. |
|
🎉 Normalized strings (and with public keys only) should be the default behavior imo. There's (at least) one reason to deviate:
|
This PR adds a
createwalletdescriptorRPC which allows users to add new automatically generated descriptors to their wallet, e.g. to upgrade a 0.21.x wallet to contain a taproot descriptor. This RPC takes 3 arguments: the output type to create a descriptor for, whether the descriptor will be internal or external, and the HD key to use if the user wishes to use a specific key. The HD key is an optional parameter. If it is not specified, the wallet will use the key shared by the active descriptors, if they are all single key. For most users in the expected upgrade scenario, this should be sufficient. In more advanced cases, the user must specify the HD key to use.Currently, specified HD keys must already exist in the wallet. To make it easier for the user to know,
gethdkeysis also added to list out the HD keys in use by all of the descriptors in the wallet. This will include all HD keys, whether we have the private key, for it, which descriptors use it and their activeness, and optionally the extended private key. In this way, users with more complex wallets will be still be able to get HD keys from their wallet for use in other scenarios, and if they want to usecreatewalletdescriptor, they can easily get the keys that they can specify to it.See also #26728 (comment)