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.
| Type | Reviewers |
|---|---|
| ACK | theStack |
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.
Instead of relying on implicit assumptions about whether pubkeys show up
or now 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.