Merge #14934: Descriptor expansion cache clarifications

2e68ffa [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost)
2290269 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost)

Pull request description:

  I found the name `m_script_arg` to be confusing while reviewing bitcoin/bitcoin#14646 (comment). @sipa let me know if `m_subdescriptor_arg` is completely wrong.

  I also added an explanation of why we call `GetPubKey` when we don't ask it for a public key.

ACKs for top commit:
  laanwj:
    ACK 2e68ffa

Tree-SHA512: 06698e9a91cdda93c043a82732793f0ad3cd91daa2513565953e9fa048d5573322fb534e9d0ea9ab736e6366be5921e2b8699c4f4b3693edab48039aaae06f78

Author: laanwj

src/script/descriptor.cpp

@@ -335,10 +335,12 @@ class BIP32PubkeyProvider final : public PubkeyProvider
 /** Base class for all Descriptor implementations. */
 class DescriptorImpl : public Descriptor
 {
-    //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size of Multisig).
+    //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size for Multisig).
     const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkey_args;
     //! The sub-descriptor argument (nullptr for everything but SH and WSH).
-    const std::unique_ptr<DescriptorImpl> m_script_arg;
+    //! In doc/descriptors.m this is referred to as SCRIPT expressions sh(SCRIPT)
+    //! and wsh(SCRIPT), and distinct from KEY expressions and ADDR expressions.
+    const std::unique_ptr<DescriptorImpl> m_subdescriptor_arg;
     //! The string name of the descriptor function.
     const std::string m_name;
 
@@ -349,23 +351,23 @@ class DescriptorImpl : public Descriptor
     /** A helper function to construct the scripts for this descriptor.
      *
      *  This function is invoked once for every CScript produced by evaluating
-     *  m_script_arg, or just once in case m_script_arg is nullptr.
+     *  m_subdescriptor_arg, or just once in case m_subdescriptor_arg is nullptr.
 
      *  @param pubkeys The evaluations of the m_pubkey_args field.
-     *  @param script The evaluation of m_script_arg (or nullptr when m_script_arg is nullptr).
+     *  @param script The evaluation of m_subdescriptor_arg (or nullptr when m_subdescriptor_arg is nullptr).
      *  @param out A FlatSigningProvider to put scripts or public keys in that are necessary to the solver.
      *             The script arguments to this function are automatically added, as is the origin info of the provided pubkeys.
      *  @return A vector with scriptPubKeys for this descriptor.
      */
     virtual std::vector<CScript> MakeScripts(const std::vector<CPubKey>& pubkeys, const CScript* script, FlatSigningProvider& out) const = 0;
 
 public:
-    DescriptorImpl(std::vector<std::unique_ptr<PubkeyProvider>> pubkeys, std::unique_ptr<DescriptorImpl> script, const std::string& name) : m_pubkey_args(std::move(pubkeys)), m_script_arg(std::move(script)), m_name(name) {}
+    DescriptorImpl(std::vector<std::unique_ptr<PubkeyProvider>> pubkeys, std::unique_ptr<DescriptorImpl> script, const std::string& name) : m_pubkey_args(std::move(pubkeys)), m_subdescriptor_arg(std::move(script)), m_name(name) {}
 
     bool IsSolvable() const override
     {
-        if (m_script_arg) {
-            if (!m_script_arg->IsSolvable()) return false;
+        if (m_subdescriptor_arg) {
+            if (!m_subdescriptor_arg->IsSolvable()) return false;
         }
         return true;
     }
@@ -375,8 +377,8 @@ class DescriptorImpl : public Descriptor
         for (const auto& pubkey : m_pubkey_args) {
             if (pubkey->IsRange()) return true;
         }
-        if (m_script_arg) {
-            if (m_script_arg->IsRange()) return true;
+        if (m_subdescriptor_arg) {
+            if (m_subdescriptor_arg->IsRange()) return true;
         }
         return false;
     }
@@ -396,10 +398,10 @@ class DescriptorImpl : public Descriptor
             }
             ret += std::move(tmp);
         }
-        if (m_script_arg) {
+        if (m_subdescriptor_arg) {
             if (pos++) ret += ",";
             std::string tmp;
-            if (!m_script_arg->ToStringHelper(arg, tmp, priv)) return false;
+            if (!m_subdescriptor_arg->ToStringHelper(arg, tmp, priv)) return false;
             ret += std::move(tmp);
         }
         out = std::move(ret) + ")";
@@ -428,6 +430,8 @@ class DescriptorImpl : public Descriptor
         // Construct temporary data in `entries` and `subscripts`, to avoid producing output in case of failure.
         for (const auto& p : m_pubkey_args) {
             entries.emplace_back();
+            // If we have a cache, we don't need GetPubKey to compute the public key.
+            // Pass in nullptr to signify only origin info is desired.
             if (!p->GetPubKey(pos, arg, cache_read ? nullptr : &entries.back().first, entries.back().second)) return false;
             if (cache_read) {
                 // Cached expanded public key exists, use it.
@@ -444,9 +448,9 @@ class DescriptorImpl : public Descriptor
             }
         }
         std::vector<CScript> subscripts;
-        if (m_script_arg) {
+        if (m_subdescriptor_arg) {
             FlatSigningProvider subprovider;
-            if (!m_script_arg->ExpandHelper(pos, arg, cache_read, subscripts, subprovider, cache_write)) return false;
+            if (!m_subdescriptor_arg->ExpandHelper(pos, arg, cache_read, subscripts, subprovider, cache_write)) return false;
             out = Merge(out, subprovider);
         }
 
@@ -456,7 +460,7 @@ class DescriptorImpl : public Descriptor
             pubkeys.push_back(entry.first);
             out.origins.emplace(entry.first.GetID(), std::make_pair<CPubKey, KeyOriginInfo>(CPubKey(entry.first), std::move(entry.second)));
         }
-        if (m_script_arg) {
+        if (m_subdescriptor_arg) {
             for (const auto& subscript : subscripts) {
                 out.scripts.emplace(CScriptID(subscript), subscript);
                 std::vector<CScript> addscripts = MakeScripts(pubkeys, &subscript, out);

src/script/descriptor.h

@@ -47,17 +47,17 @@ struct Descriptor {
      *
      * pos: the position at which to expand the descriptor. If IsRange() is false, this is ignored.
      * provider: the provider to query for private keys in case of hardened derivation.
-     * output_script: the expanded scriptPubKeys will be put here.
+     * output_scripts: the expanded scriptPubKeys will be put here.
      * out: scripts and public keys necessary for solving the expanded scriptPubKeys will be put here (may be equal to provider).
-     * cache: vector which will be overwritten with cache data necessary to-evaluate the descriptor at this point without access to private keys.
+     * cache: vector which will be overwritten with cache data necessary to evaluate the descriptor at this point without access to private keys.
      */
     virtual bool Expand(int pos, const SigningProvider& provider, std::vector<CScript>& output_scripts, FlatSigningProvider& out, std::vector<unsigned char>* cache = nullptr) const = 0;
 
     /** Expand a descriptor at a specified position using cached expansion data.
      *
      * pos: the position at which to expand the descriptor. If IsRange() is false, this is ignored.
      * cache: vector from which cached expansion data will be read.
-     * output_script: the expanded scriptPubKeys will be put here.
+     * output_scripts: the expanded scriptPubKeys will be put here.
      * out: scripts and public keys necessary for solving the expanded scriptPubKeys will be put here (may be equal to provider).
      */
     virtual bool ExpandFromCache(int pos, const std::vector<unsigned char>& cache, std::vector<CScript>& output_scripts, FlatSigningProvider& out) const = 0;