DescriptorImpl::MakeScripts to PubkeyProvider::GetPubKey
#31243
Instead of having MakeScripts infer what pubkeys need to go into the output FlatSigningProvider, have each of the PubkeyProviders that have GetPubKey and GetPrivKey called fill it directly with relevant keys and origins.
This allows for keys and origins to be added that won’t directly appear in the output, which is necessary for musig() descriptors.
Split from #29675
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31243.
See 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.
Reviewers, 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.
211@@ -212,7 +212,7 @@ struct PubkeyProvider
212 virtual bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache = nullptr) const = 0;
213
214 /** Derive a private key, if private data is available in arg. */
215- virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
216+ virtual void GetPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const = 0;
711+ FlatSigningProvider subprovider;
712+ std::vector<CPubKey> pubkeys;
713+ pubkeys.reserve(m_pubkey_args.size());
714
715- // Construct temporary data in `entries`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
716+ // Construct temporary data in `pks`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
comment nit
0 // Construct temporary data in `pubkeys`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
110@@ -111,6 +111,9 @@ struct Descriptor {
111 /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
112 virtual bool IsSingleType() const = 0;
113
114+ /** Whether this descriptor only produces single key scripts (i.e. pk(), pkh(), wpkh(), sh() and wsh() nested of those, and combo() */
115+ virtual bool IsSingleKey() const = 0;
importmulti RPC call in particular) is gone? If not, could maybe add a TODO here to get rid of it in the future.
Code-review ACK f50470e33c1bb51531f3fa08f6d29c54df304d6b
To my understanding, the second and fourth commits (db270fa2a33d7c88e58682ab8a219a2e70503c9e and f50470e33c1bb51531f3fa08f6d29c54df304d6b) are pure refactors, while the third one actually changes behaviour for Descriptor::Expand call-sites w.r.t. by returning more public key data than before (stored in the FlatSigningProvider& out parameter), so the first commit (d1be90704559da45e4d76171230ecb9f4a6d9cf8) is needed to avoid breaking the importmulti RPC call behaviour. Without that, the “Imported scripts with pubkeys should not have their pubkeys go into the keypool” sub-test in wallet_importmulti.py fails.
1090@@ -1091,6 +1091,8 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
1091 std::tie(range_start, range_end) = ParseDescriptorRange(data["range"]);
1092 }
1093
1094+ bool can_keypool = parsed_descs.at(0)->IsSingleKey();
nit: Could add a comment here as well, something like
0 // Only single key descriptors are allowed to be imported into a legacy wallet's keypool
1 bool can_keypool = parsed_descs.at(0)->IsSingleKey();
806@@ -807,6 +807,7 @@ class AddressDescriptor final : public DescriptorImpl
807 return OutputTypeFromDestination(m_destination);
808 }
809 bool IsSingleType() const final { return true; }
210@@ -211,8 +211,8 @@ struct PubkeyProvider
211 */
212 virtual bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache = nullptr) const = 0;
213
214- /** Derive a private key, if private data is available in arg. */
215- virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
216+ /** Derive a private key, if private data is available in arg and put it into out. */
217+ virtual void GetPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const = 0;
1bfabb4f4ec07c8fb0dfc2515d413b5196e01348: maybe rename to FillPrivKey?
0/** Try to derive the private key at pos, if private data is available in arg, and fill out with it. */
1virtual void FillPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const = 0;
That also distinguishes it from ConstPubkeyProvider::GetPrivKey.
Get returning nothing but at the moment it’s a common pattern in the codebase to Get something and update a reference passed in one of the function arguments. The confusion, as highlighted above, that might arise with prefixing with Fill makes me not to want to use that.
Not a suggestion, just thinking out loud.
For the sake of having a function signature consistent with the corresponding public key function - I suppose returning the private key from this function is not an option because we’d prefer to have fewer instances of private keys moving around within the codebase?
Though I’m wondering can there be an use case later where the caller would need to know whether the private key was gotten in which case the boolean return value is needed; seems a bit odd that the caller would need to find the key in the out every time even if they don’t want to use the key.
186@@ -189,7 +187,7 @@ struct PubkeyProvider
187 * write_cache is the cache to write keys to (if not nullptr)
188 * Caches are not exclusive but this is not tested. Currently we use them exclusively
189 */
190- virtual bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key, KeyOriginInfo& info, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const = 0;
191+ virtual std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const = 0;
FillPubKey. The filling seems more important, and the return value makes it clear enough it also gets it.
In the function doc:
0-/** Derive a public key.
1+/** Derive a public key and put it into `out`
246- info.path.insert(info.path.begin(), m_origin.path.begin(), m_origin.path.end());
247- return true;
248+ std::optional<CPubKey> pub = m_provider->GetPubKey(pos, arg, out, read_cache, write_cache);
249+ if (!pub) return std::nullopt;
250+ auto& [pubkey, suborigin] = out.origins[pub->GetID()];
251+ Assert(pubkey == *pub); // All subproviders must be inserting a valid origin already
e743d11aae2e459058b1af70b6d19ab90293d1f4: I’m confused by this comment. I guess by “valid” you mean that a KeyOriginInfo field is present, but it can be a dummy?
It seems more intuitive if we insert an origin entry ourselves here … if possible.
0-// All subproviders must be inserting a valid origin already
1+// `m_provider` must be inserting a valid origin already
Using the word subproviders here removes this function’s individuality (to some extent) by bringing in context from its caller. IIUC, at any time, there would just be one subprovider (either ConstPubkeyProvider or BIP32PubkeyProvider) that is referenced by the OriginPubkeyProvider class member m_provider.
Concept ACK
Mostly happy with 1bfabb4f4ec07c8fb0dfc2515d413b5196e01348, but I’m confused about the key origin handling.
297@@ -298,6 +298,14 @@ class ConstPubkeyProvider final : public PubkeyProvider
298 CPubKey m_pubkey;
299 bool m_xonly;
300
301+ std::optional<CKey> GetPrivKey(const SigningProvider& arg) const
GetPrivKeys is better than one. The description of the commit only talks about the other one, not this one. For this one the caller still cares about the return value it seems. Can’t they be unified under the std::optional returning variant? It seems like the variant using FlatSigningProvider is just used in one place in ExpandPrivate and the whole benefit is saving us one line there.
The benefit is in the MuSig2 PRs where GetPrivKey can insert multiple private keys to the FlatSigningProvider, none of which are the private key for the MuSig2 aggregate pubkey itself.
For the ConstPubkeyProvider, it’s useful to have another GetPrivKey that returns the single private key since this is behavior is used in a few places within this class’s member functions. None of the other pubkey providers need this.
ConstPubkeyProvider, like GetConstPrivKey maybe, but completely optional.
The benefit is in the MuSig2 PRs where GetPrivKey can insert multiple private keys to the FlatSigningProvider, none of which are the private key for the MuSig2 aggregate pubkey itself.
Something around this can be mentioned either in the commit message and/or the PR description.
like GetConstPrivKey maybe
This is already inside ConstPubkeyProvider class, using Const again could feel repetitive. I think the function overloading (with different argument types) is differentiating enough. Though, some documentation for the new function would be helpful for the reader.
Something around this can be mentioned either in the commit message and/or the PR description.
Added to the commit message.
Concept ACK
Everything besides my GetPrivKey question looks good to me. Will do a final pass once I get an answer there.
110@@ -111,6 +111,11 @@ struct Descriptor {
111 /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
112 virtual bool IsSingleType() const = 0;
113
114+ /** Whether this descriptor only produces single key scripts (i.e. pk(), pkh(), wpkh(), sh() and wsh() nested of those, and combo()
nit: missing paren
0 /** Whether this descriptor only produces single key scripts (i.e. pk(), pkh(), wpkh(), sh() and wsh() nested of those, and combo())
975@@ -971,6 +976,7 @@ class ComboDescriptor final : public DescriptorImpl
976 {
977 return std::make_unique<ComboDescriptor>(m_pubkey_args.at(0)->Clone());
978 }
979+ bool IsSingleKey() const final { return true; }
IsSingleType, for consistency with other classes
882@@ -880,7 +883,6 @@ class PKHDescriptor final : public DescriptorImpl
883 std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
884 {
885 CKeyID id = keys[0].GetID();
886- out.pubkeys.emplace(id, keys[0]);
Now that out is not being used in this function.
0-std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
1+std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider&) const override
MakeScripts functions where it’s applicable - WPKHDescriptor.
747@@ -740,8 +748,7 @@ class DescriptorImpl : public Descriptor
748 {
749 for (const auto& p : m_pubkey_args) {
750 CKey key;
305@@ -305,6 +306,7 @@ class ConstPubkeyProvider final : public PubkeyProvider
306 CKeyID keyid = m_pubkey.GetID();
307 std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint);
308 out.origins.emplace(keyid, std::make_pair(m_pubkey, info));
309+ out.pubkeys.emplace(keyid, m_pubkey);
out with pubkeys and origins together in a function.
1109@@ -1107,8 +1110,10 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
1110 std::vector<CScript> scripts_temp;
1111 parsed_desc->Expand(i, keys, scripts_temp, out_keys);
1112 std::copy(scripts_temp.begin(), scripts_temp.end(), std::inserter(script_pub_keys, script_pub_keys.end()));
1113- for (const auto& key_pair : out_keys.pubkeys) {
1114- ordered_pubkeys.emplace_back(key_pair.first, desc_internal);
1115+ if (can_keypool) {
1116+ for (const auto& key_pair : out_keys.pubkeys) {
This new check here makes the flow more robust and avoids relying on implicit assumptions of what’s going on inside the Expand function.
Instead of relying on implicit assumptions about whether pubkeys show up after expanding a descriptor, just explicitly allow only single key descriptors to import keys into a legacy wallet’s keypool.
The commit message 6a31eb5dca1f8eb5fd0507852b49c7452031269e, however, was difficult for me to parse. The second clause, which I understand (and made me wonder why), didn’t seem to follow the first. Only after reading this #31243#pullrequestreview-2436530611 PR comment, I understood the need for this commit that made me want to investigate the Imported scripts with pubkeys should not have their pubkeys go into the keypool subtest in wallet_importmulti functional test that fails without this new check or relying only on the out_keys.pubkeys.size() == 1 check (that I thought of trying) as a 1-of-1 multisig scenario ends up passing that test, which is not desired.
Legacy wallet keypool should not be updated with individual keys from multisig type scripts as per my understanding from this test. Was this the contract in the legacy wallet?
Some rephrasing of the commit message as per the above mention comment would be helpful.
394@@ -394,18 +395,17 @@ class BIP32PubkeyProvider final : public PubkeyProvider
395 BIP32PubkeyProvider(uint32_t exp_index, const CExtPubKey& extkey, KeyPath path, DeriveType derive, bool apostrophe) : PubkeyProvider(exp_index), m_root_extkey(extkey), m_path(std::move(path)), m_derive(derive), m_apostrophe(apostrophe) {}
396 bool IsRange() const override { return m_derive != DeriveType::NO; }
397 size_t GetSize() const override { return 33; }
398- bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key_out, KeyOriginInfo& final_info_out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
399+ std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
400 {
401 // Info of parent of the to be derived pubkey
394@@ -394,18 +395,17 @@ class BIP32PubkeyProvider final : public PubkeyProvider
395 BIP32PubkeyProvider(uint32_t exp_index, const CExtPubKey& extkey, KeyPath path, DeriveType derive, bool apostrophe) : PubkeyProvider(exp_index), m_root_extkey(extkey), m_path(std::move(path)), m_derive(derive), m_apostrophe(apostrophe) {}
396 bool IsRange() const override { return m_derive != DeriveType::NO; }
397 size_t GetSize() const override { return 33; }
398- bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key_out, KeyOriginInfo& final_info_out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
399+ std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
400 {
401 // Info of parent of the to be derived pubkey
402- KeyOriginInfo parent_info;
403+ KeyOriginInfo info;
Took me some time to gather what’s going on here.
For reviewing and future reference as well, it would be easier if this commit 070e746c2f72db2cc24b1c267b91fafbcc586736 is split into 2:
Usage of just info object and the removal of parent_info, final_info_out_tmp objects with the changes related to it can be one commit. Rest of the commit changes can be part of the other.
405- std::copy(keyid.begin(), keyid.begin() + sizeof(parent_info.fingerprint), parent_info.fingerprint);
406- parent_info.path = m_path;
407+ std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint);
408+ info.path = m_path;
409
410 // Info of the derived key itself which is copied out upon successful completion
FlatSigningProvider& out has decreased now because there are fewer PubkeyProviders and more DescrImpls , which IMO is desirable.
GetPrivKey commit 72694b8d1a9863771cf1095037caeafb3f1a5635 changes deserves a mention in the PR description, currently only the pubkey changes are mentioned.
Probably in some follow up: The documentation of the MakeScripts function in DescriptorImpl class can be updated to get rid of the following line because the origin info is added in the GetPubKey functions now and was added in the ExpandHelper function previously.
0The origin info of the provided pubkeys is automatically added.
443@@ -442,6 +444,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider
444 if (!der) return std::nullopt;
445
446 out.origins.emplace(final_extkey.pubkey.GetID(), std::make_pair(final_extkey.pubkey, info));
447+ out.pubkeys.emplace(final_extkey.pubkey.GetID(), final_extkey.pubkey);
emplace() is fairly self explanatory.
298@@ -298,13 +299,13 @@ class ConstPubkeyProvider final : public PubkeyProvider
299
300 public:
301 ConstPubkeyProvider(uint32_t exp_index, const CPubKey& pubkey, bool xonly) : PubkeyProvider(exp_index), m_pubkey(pubkey), m_xonly(xonly) {}
302- bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key, KeyOriginInfo& info, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
303+ std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
0-const SigningProvider& arg
1+const SigningProvider&
arg is not used in this function, though not caused by this PR.
708- // Construct temporary data in `entries`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
709+ // Construct temporary data in `pubkeys`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
710 for (const auto& p : m_pubkey_args) {
711- entries.emplace_back();
712- if (!p->GetPubKey(pos, arg, entries.back().first, entries.back().second, read_cache, write_cache)) return false;
713+ std::optional<CPubKey> pubkey = p->GetPubKey(pos, arg, subprovider, read_cache, write_cache);
entries.back().first, entries.back().second as the output parameters to the GetPubKey(), which was not intuitive to read for me.
Legacy wallets should only import keys to the keypool if they came in a
single key descriptor. Instead of relying on assumptions about the
descriptor based on how many pubkeys show up after expanding the
descriptor, explicitly mark descriptors as being single key type and use
that for the check.
Instead of having ExpandHelper fill in the origins in the
FlatSigningProvider output, have GetPubKey do it by itself. This reduces
the extra variables needed in order to track and set origins in
ExpandHelper.
Also changes GetPubKey to return a std::optional<CPubKey> rather than
using a bool and output parameters.
Instead of MakeScripts inconsistently filling the output
FlatSigningProvider with the pubkeys involved, just do it in GetPubKey.
Instead of GetPrivKey returning a key and having the caller fill the
FlatSigningProvider, have GetPrivKey take the FlatSigningProvider and
fill it by itself. This will be necessary for descriptors such as
musig() where there are private keys that need to be added to the
FlatSigningProvider but do not directly appear in any resulting scripts.
GetPrivKey is now changed to void as the caller no longer cares whether
it succeeds or fails.
334 {
335 ret = ToString(StringType::PUBLIC);
336 return true;
337 }
338- bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
339+ void GetPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const override
Nit: Because pos is unused.
0- void GetPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const override
1+ void GetPrivKey(int, const SigningProvider& arg, FlatSigningProvider& out) const override
186
187 return a < b;
188 }
189
190- /** Derive a public key.
191+ /** Derive a public key and put it into out.
Nit:
0- /** Derive a public key and put it into out.
1+ /** Derive a public key and put it into out along with its origin info.
1109@@ -1107,8 +1110,10 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
1110 std::vector<CScript> scripts_temp;
1111 parsed_desc->Expand(i, keys, scripts_temp, out_keys);
1112 std::copy(scripts_temp.begin(), scripts_temp.end(), std::inserter(script_pub_keys, script_pub_keys.end()));
1113- for (const auto& key_pair : out_keys.pubkeys) {
1114- ordered_pubkeys.emplace_back(key_pair.first, desc_internal);
1115+ if (can_keypool) {
I’m looking at it after a month and have lost some context here.
What restriction in the previous code was disallowing the addition of public keys in the keypool for non-single-key type descriptors that made the “Imported scripts with pubkeys should not have their pubkeys go into the keypool” subtest pass? Sorry if this has been answered earlier but I didn’t find it.
out.pubkeys inside of MakeScripts. The pubkeys that are inserted to that FlatSigningProvider are the ones inserted into the keypool.
I see, they were being added for pkh, wpkh, combo, tr, miniscript ones in MakeScript but not for multisig & the like. I debugged this flow in on master while importing a multisig descriptor and found it quite strange that the origins of all the pubkeys involved in the multisig were being added in the out.origins inside ExpandHelper but not all the pubkeys were added in out.pubkeys!
Also, pubkeys being set inside MakeScript but origins being set in ExpandHelper made it difficult to reason about for me, the newer implementation makes it easier to follow.
0bitcoinclireg importdescriptors '[{"desc": "wsh(multi(2,tprv8ZgxMBicQKsPdzuc344mDaeUk5zseMcRK9Hst8xodskNu3YbQG5NxLa2X17PUU5yXQhptiBE7F5W5cgEmsfQg4Y21Y18w4DJhLxSb8CurDf,tpubD6NzVbkrYhZ4YiCvExLvH4yh1k3jFGf5irm6TsrArY8GYdEhYVdztQTBtTirmRc6XfSJpH9tayUdnngaJZKDaa2zbqEY29DfcGZW8iRVGUY))#a0tl2jdn", "timestamp": "now"}]'
I used importdescriptors but the Expand flow is the same as in importmulti.
LGTM, code review acee5c59e68f31acba111bc4d1892b08243ea5e0
Thank you for splitting the middle commit, makes it easier to review!
Broadly this pull updates the private & public key getters in the PubkeyProviders to also fill the passed SigningProvider out with the said keys; the effect of which is that it cleans the implementation of the call sites such as ExpandHelper in the Descriptors classes. This would result in more (than before) keys being set in the SigningProviders generally and the direct callers of these functions would need to accordingly adjust. I’m inclining to prefer the filling of the SigningProviders with the public key info (origin & key) together instead of having them filled in various functions like it was done previously.
There are few other minor improvements as well like highlighting the non-usage of the arguments in the function signature itself. I’ve started to look at the #29675 PR to find where the updated functionality of GetPrivKey is used as stated in the commit description.
Before I ack the PR, asked one question related to the first commit - #31243 (review).
173@@ -174,22 +174,20 @@ struct PubkeyProvider
174 * Used by the Miniscript descriptors to check for duplicate keys in the script.
175 */
176 bool operator<(PubkeyProvider& other) const {
other const if you retouch
re-ACK acee5c59e68f31acba111bc4d1892b08243ea5e0
case-sensitivity yocto-nit: in commit subject for 6268bde0af0aa9cfb3df08d6b9b67fdffa2a1a12: s/BIP32PUbKeyProvider/BIP32PubkeyProvider/
ACK acee5c5
Was that an ACK an you just forgot the word or did you just want to say you reviewed? :)
I wanted to convey I code reviewed & almost ack. There was one thing that I had not completely reasoned about though it made sense to me intuitively somehow, so was hesitant in acking earlier: #31243 (review)