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 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; }
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.
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.
GetPrivKey is now changed to void as the caller no longer cares whether
it succeeds or fails.
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.
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.
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.
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.
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