Merge bitcoin#34141: miniscript: Use Func and Expr when parsing keys, hashes, and locktimes

4b53cbd test: Test for musig() in various miniscript expressions (Ava Chow)
ec0f47b miniscript: Using Func and Expr when parsing keys, hashes, and locktimes (Ava Chow)
6fd780d descriptors: Increment key_exp_index in ParsePubkey(Inner) (Ava Chow)
b12281b miniscript: Use a reference to key_exp_index in KeyParser (Ava Chow)
ce4c66e test: Test that key expression indexes match key count (Ava Chow)

Pull request description:

  The miniscript parser currently only looks for the next `)` when parsing key, hash, and locktime expressions. This fails to parse when the expressions contain a nested expression. Currently, this is only possible with `musig()` inside of key expressions. However, this pattern can be generalized to handling hashes and locktimes, so I implemented those too.

  Fixes bitcoin#34076

ACKs for top commit:
  rkrux:
    ACK 4b53cbd
  sipa:
    ACK 4b53cbd
  darosior:
    Other than that, Approach ACK 4b53cbd. That makes sense to me but i have not closely reviewed the code.

Tree-SHA512: 01040c7b07a59d8e3725ff11ab9543b256aea22535fb94059f490a5bb45319e859666af04c2f0a4edcb8cf1e6dfc7bd8a8271b21ad81143bafccd4d0a39cae9c

Author: sedited

src/script/descriptor.cpp

@@ -161,12 +161,11 @@ typedef std::vector<uint32_t> KeyPath;
 /** Interface for public key objects in descriptors. */
 struct PubkeyProvider
 {
-protected:
+public:
     //! Index of this key expression in the descriptor
     //! E.g. If this PubkeyProvider is key1 in multi(2, key1, key2, key3), then m_expr_index = 0
-    uint32_t m_expr_index;
+    const uint32_t m_expr_index;
 
-public:
     explicit PubkeyProvider(uint32_t exp_index) : m_expr_index(exp_index) {}
 
     virtual ~PubkeyProvider() = default;
@@ -229,6 +228,9 @@ struct PubkeyProvider
 
     /** Whether this PubkeyProvider is a BIP 32 extended key that can be derived from */
     virtual bool IsBIP32() const = 0;
+
+    /** Get the count of keys known by this PubkeyProvider. Usually one, but may be more for key aggregation schemes */
+    virtual size_t GetKeyCount() const { return 1; }
 };
 
 class OriginPubkeyProvider final : public PubkeyProvider
@@ -788,6 +790,10 @@ class MuSigPubkeyProvider final : public PubkeyProvider
         // musig() can only be a BIP 32 key if all participants are bip32 too
         return std::all_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsBIP32(); });
     }
+    size_t GetKeyCount() const override
+    {
+        return 1 + m_participants.size();
+    }
 };
 
 /** Base class for all Descriptor implementations. */
@@ -1041,6 +1047,40 @@ class DescriptorImpl : public Descriptor
         }
         return all;
     }
+
+    uint32_t GetMaxKeyExpr() const final
+    {
+        uint32_t max_key_expr{0};
+        std::vector<const DescriptorImpl*> todo = {this};
+        while (!todo.empty()) {
+            const DescriptorImpl* desc = todo.back();
+            todo.pop_back();
+            for (const auto& p : desc->m_pubkey_args) {
+                max_key_expr = std::max(max_key_expr, p->m_expr_index);
+            }
+            for (const auto& s : desc->m_subdescriptor_args) {
+                todo.push_back(s.get());
+            }
+        }
+        return max_key_expr;
+    }
+
+    size_t GetKeyCount() const final
+    {
+        size_t count{0};
+        std::vector<const DescriptorImpl*> todo = {this};
+        while (!todo.empty()) {
+            const DescriptorImpl* desc = todo.back();
+            todo.pop_back();
+            for (const auto& p : desc->m_pubkey_args) {
+                count += p->GetKeyCount();
+            }
+            for (const auto& s : desc->m_subdescriptor_args) {
+                todo.push_back(s.get());
+            }
+        }
+        return count;
+    }
 };
 
 /** A parsed addr(A) descriptor. */
@@ -1833,7 +1873,7 @@ static DeriveType ParseDeriveType(std::vector<std::span<const char>>& split, boo
 }
 
 /** Parse a public key that excludes origin information. */
-std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_index, const std::span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, bool& apostrophe, std::string& error)
+std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t& key_exp_index, const std::span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, bool& apostrophe, std::string& error)
 {
     std::vector<std::unique_ptr<PubkeyProvider>> ret;
     bool permit_uncompressed = ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH;
@@ -1858,6 +1898,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
             if (pubkey.IsFullyValid()) {
                 if (permit_uncompressed || pubkey.IsCompressed()) {
                     ret.emplace_back(std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, false));
+                    ++key_exp_index;
                     return ret;
                 } else {
                     error = "Uncompressed keys are not allowed";
@@ -1869,6 +1910,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
                 pubkey.Set(std::begin(fullkey), std::end(fullkey));
                 if (pubkey.IsFullyValid()) {
                     ret.emplace_back(std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, true));
+                    ++key_exp_index;
                     return ret;
                 }
             }
@@ -1881,6 +1923,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
                 CPubKey pubkey = key.GetPubKey();
                 out.keys.emplace(pubkey.GetID(), key);
                 ret.emplace_back(std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, ctx == ParseScriptContext::P2TR));
+                ++key_exp_index;
                 return ret;
             } else {
                 error = "Uncompressed keys are not allowed";
@@ -1904,6 +1947,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
     for (auto& path : paths) {
         ret.emplace_back(std::make_unique<BIP32PubkeyProvider>(key_exp_index, extpubkey, std::move(path), type, apostrophe));
     }
+    ++key_exp_index;
     return ret;
 }
 
@@ -1961,7 +2005,6 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
             max_multipath_len = std::max(max_multipath_len, pk.size());
 
             providers.emplace_back(std::move(pk));
-            key_exp_index++;
         }
         if (!any_key_parsed) {
             error = "musig(): Must contain key expressions";
@@ -2053,6 +2096,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
             // No multipath derivation, MuSigPubkeyProvider uses the first (and only) participant pubkey providers, and the first (and only) path
             emplace_final_provider(0, 0);
         }
+        ++key_exp_index; // Increment key expression index for the MuSigPubkeyProvider too
         return ret;
     }
 
@@ -2093,7 +2137,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
     if (providers.empty()) return {};
     ret.reserve(providers.size());
     for (auto& prov : providers) {
-        ret.emplace_back(std::make_unique<OriginPubkeyProvider>(key_exp_index, info, std::move(prov), apostrophe));
+        ret.emplace_back(std::make_unique<OriginPubkeyProvider>(prov->m_expr_index, info, std::move(prov), apostrophe));
     }
     return ret;
 }
@@ -2143,12 +2187,12 @@ struct KeyParser {
     mutable std::string m_key_parsing_error;
     //! The script context we're operating within (Tapscript or P2WSH).
     const miniscript::MiniscriptContext m_script_ctx;
-    //! The number of keys that were parsed before starting to parse this Miniscript descriptor.
-    uint32_t m_offset;
+    //! The current key expression index
+    uint32_t& m_expr_index;
 
     KeyParser(FlatSigningProvider* out LIFETIMEBOUND, const SigningProvider* in LIFETIMEBOUND,
-              miniscript::MiniscriptContext ctx, uint32_t offset = 0)
-        : m_out(out), m_in(in), m_script_ctx(ctx), m_offset(offset) {}
+              miniscript::MiniscriptContext ctx, uint32_t& key_exp_index LIFETIMEBOUND)
+        : m_out(out), m_in(in), m_script_ctx(ctx), m_expr_index(key_exp_index) {}
 
     bool KeyCompare(const Key& a, const Key& b) const {
         return *m_keys.at(a).at(0) < *m_keys.at(b).at(0);
@@ -2162,12 +2206,11 @@ struct KeyParser {
         assert(false);
     }
 
-    template<typename I> std::optional<Key> FromString(I begin, I end) const
+    std::optional<Key> FromString(std::span<const char>& in) const
     {
         assert(m_out);
         Key key = m_keys.size();
-        uint32_t exp_index = m_offset + key;
-        auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
+        auto pk = ParsePubkey(m_expr_index, in, ParseContext(), *m_out, m_key_parsing_error);
         if (pk.empty()) return {};
         m_keys.emplace_back(std::move(pk));
         return key;
@@ -2239,7 +2282,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             error = strprintf("pk(): %s", error);
             return {};
         }
-        ++key_exp_index;
         for (auto& pubkey : pubkeys) {
             ret.emplace_back(std::make_unique<PKDescriptor>(std::move(pubkey), ctx == ParseScriptContext::P2TR));
         }
@@ -2251,7 +2293,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             error = strprintf("pkh(): %s", error);
             return {};
         }
-        ++key_exp_index;
         for (auto& pubkey : pubkeys) {
             ret.emplace_back(std::make_unique<PKHDescriptor>(std::move(pubkey)));
         }
@@ -2263,7 +2304,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             error = strprintf("combo(): %s", error);
             return {};
         }
-        ++key_exp_index;
         for (auto& pubkey : pubkeys) {
             ret.emplace_back(std::make_unique<ComboDescriptor>(std::move(pubkey)));
         }
@@ -2303,7 +2343,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             script_size += pks.at(0)->GetSize() + 1;
             max_providers_len = std::max(max_providers_len, pks.size());
             providers.emplace_back(std::move(pks));
-            key_exp_index++;
         }
         if ((multi || sortedmulti) && (providers.empty() || providers.size() > MAX_PUBKEYS_PER_MULTISIG)) {
             error = strprintf("Cannot have %u keys in multisig; must have between 1 and %d keys, inclusive", providers.size(), MAX_PUBKEYS_PER_MULTISIG);
@@ -2373,7 +2412,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             error = strprintf("wpkh(): %s", error);
             return {};
         }
-        key_exp_index++;
         for (auto& pubkey : pubkeys) {
             ret.emplace_back(std::make_unique<WPKHDescriptor>(std::move(pubkey)));
         }
@@ -2426,7 +2464,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             return {};
         }
         size_t max_providers_len = internal_keys.size();
-        ++key_exp_index;
         std::vector<std::vector<std::unique_ptr<DescriptorImpl>>> subscripts; //!< list of multipath expanded script subexpressions
         std::vector<int> depths; //!< depth in the tree of each subexpression (same length subscripts)
         if (expr.size()) {
@@ -2530,7 +2567,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             error = strprintf("rawtr(): %s", error);
             return {};
         }
-        ++key_exp_index;
         for (auto& pubkey : output_keys) {
             ret.emplace_back(std::make_unique<RawTRDescriptor>(std::move(pubkey)));
         }
@@ -2594,7 +2630,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             // A signature check is required for a miniscript to be sane. Therefore no sane miniscript
             // may have an empty list of public keys.
             CHECK_NONFATAL(!parser.m_keys.empty());
-            key_exp_index += parser.m_keys.size();
             // Make sure all vecs are of the same length, or exactly length 1
             // For length 1 vectors, clone subdescs until vector is the same length
             size_t num_multipath = std::max_element(parser.m_keys.begin(), parser.m_keys.end(),
@@ -2769,7 +2804,8 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
 
     if (ctx == ParseScriptContext::P2WSH || ctx == ParseScriptContext::P2TR) {
         const auto script_ctx{ctx == ParseScriptContext::P2WSH ? miniscript::MiniscriptContext::P2WSH : miniscript::MiniscriptContext::TAPSCRIPT};
-        KeyParser parser(/* out = */nullptr, /* in = */&provider, /* ctx = */script_ctx);
+        uint32_t key_exp_index = 0;
+        KeyParser parser(/* out = */nullptr, /* in = */&provider, /* ctx = */script_ctx, key_exp_index);
         auto node = miniscript::FromScript(script, parser);
         if (node && node->IsSane()) {
             std::vector<std::unique_ptr<PubkeyProvider>> keys;

src/script/descriptor.h

@@ -181,6 +181,12 @@ struct Descriptor {
 
     /** Semantic/safety warnings (includes subdescriptors). */
     virtual std::vector<std::string> Warnings() const = 0;
+
+    /** Get the maximum key expression index. Used only for tests */
+    virtual uint32_t GetMaxKeyExpr() const = 0;
+
+    /** Get the number of key expressions in this descriptor. Used only for tests */
+    virtual size_t GetKeyCount() const = 0;
 };
 
 /** Parse a `descriptor` string. Included private keys are put in `out`.