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

4b53cbd692 test: Test for musig() in various miniscript expressions (Ava Chow)
ec0f47b15c miniscript: Using Func and Expr when parsing keys, hashes, and locktimes (Ava Chow)
6fd780d4fb descriptors: Increment key_exp_index in ParsePubkey(Inner) (Ava Chow)
b12281bd86 miniscript: Use a reference to key_exp_index in KeyParser (Ava Chow)
ce4c66eb7c 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 #34076

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

Tree-SHA512: 01040c7b07a59d8e3725ff11ab9543b256aea22535fb94059f490a5bb45319e859666af04c2f0a4edcb8cf1e6dfc7bd8a8271b21ad81143bafccd4d0a39cae9c

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 @@ public:
 
     /** 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 @@ public:
         // 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 @@ public:
         }
         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.
+    //! The current key expression index
-    uint32_t m_offset;
+    uint32_t& m_expr_index;
 
     KeyParser(FlatSigningProvider* out LIFETIMEBOUND, const SigningProvider* in LIFETIMEBOUND,
-              miniscript::MiniscriptContext ctx, uint32_t offset = 0)
+              miniscript::MiniscriptContext ctx, uint32_t& key_exp_index LIFETIMEBOUND)
-        : m_out(out), m_in(in), m_script_ctx(ctx), m_offset(offset) {}
+        : 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(m_expr_index, in, ParseContext(), *m_out, m_key_parsing_error);
-        auto pk = ParsePubkey(exp_index, {&*begin, &*end}, 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`.

src/script/miniscript.h

@@ -1806,29 +1806,27 @@ enum class ParseContext {
 
 int FindNextChar(std::span<const char> in, char m);
 
-/** Parse a key string ending at the end of the fragment's text representation. */
+/** Parse a key expression fully contained within a fragment with the name given by 'func' */
 template<typename Key, typename Ctx>
-std::optional<std::pair<Key, int>> ParseKeyEnd(std::span<const char> in, const Ctx& ctx)
+std::optional<Key> ParseKey(const std::string& func, std::span<const char>& in, const Ctx& ctx)
 {
-    int key_size = FindNextChar(in, ')');
+    std::span<const char> expr = script::Expr(in);
-    if (key_size < 1) return {};
+    if (!script::Func(func, expr)) return {};
-    auto key = ctx.FromString(in.begin(), in.begin() + key_size);
+    return ctx.FromString(expr);
-    if (!key) return {};
-    return {{std::move(*key), key_size}};
 }
 
-/** Parse a hex string ending at the end of the fragment's text representation. */
+/** Parse a hex string fully contained within a fragment with the name given by 'func' */
 template<typename Ctx>
-std::optional<std::pair<std::vector<unsigned char>, int>> ParseHexStrEnd(std::span<const char> in, const size_t expected_size,
+std::optional<std::vector<unsigned char>> ParseHexStr(const std::string& func, std::span<const char>& in, const size_t expected_size,
                                                                          const Ctx& ctx)
 {
-    int hash_size = FindNextChar(in, ')');
+    std::span<const char> expr = script::Expr(in);
-    if (hash_size < 1) return {};
+    if (!script::Func(func, expr)) return {};
-    std::string val = std::string(in.begin(), in.begin() + hash_size);
+    std::string val = std::string(expr.begin(), expr.end());
     if (!IsHex(val)) return {};
     auto hash = ParseHex(val);
     if (hash.size() != expected_size) return {};
-    return {{std::move(hash), hash_size}};
+    return hash;
 }
 
 /** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
@@ -1891,7 +1889,8 @@ inline std::optional<Node<Key>> Parse(std::span<const char> in, const Ctx& ctx)
             next_comma = FindNextChar(in, ',');
             int key_length = (next_comma == -1) ? FindNextChar(in, ')') : next_comma;
             if (key_length < 1) return false;
-            auto key = ctx.FromString(in.begin(), in.begin() + key_length);
+            std::span<const char> sp{in.begin(), in.begin() + key_length};
+            auto key = ctx.FromString(sp);
             if (!key) return false;
             keys.push_back(std::move(*key));
             in = in.subspan(key_length + 1);
@@ -1978,77 +1977,59 @@ inline std::optional<Node<Key>> Parse(std::span<const char> in, const Ctx& ctx)
                 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0);
             } else if (Const("1", in)) {
                 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_1);
-            } else if (Const("pk(", in)) {
+            } else if (Const("pk(", in, /*skip=*/false)) {
-                auto res = ParseKeyEnd<Key, Ctx>(in, ctx);
+                std::optional<Key> key = ParseKey<Key, Ctx>("pk", in, ctx);
-                if (!res) return {};
+                if (!key) return {};
-                auto& [key, key_size] = *res;
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(*key)))));
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(key)))));
-                in = in.subspan(key_size + 1);
                 script_size += IsTapscript(ctx.MsContext()) ? 33 : 34;
-            } else if (Const("pkh(", in)) {
+            } else if (Const("pkh(", in, /*skip=*/false)) {
-                auto res = ParseKeyEnd<Key>(in, ctx);
+                std::optional<Key> key = ParseKey<Key, Ctx>("pkh", in, ctx);
-                if (!res) return {};
+                if (!key) return {};
-                auto& [key, key_size] = *res;
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key)))));
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(key)))));
-                in = in.subspan(key_size + 1);
                 script_size += 24;
-            } else if (Const("pk_k(", in)) {
+            } else if (Const("pk_k(", in, /*skip=*/false)) {
-                auto res = ParseKeyEnd<Key>(in, ctx);
+                std::optional<Key> key = ParseKey<Key, Ctx>("pk_k", in, ctx);
-                if (!res) return {};
+                if (!key) return {};
-                auto& [key, key_size] = *res;
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(*key)));
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(key)));
-                in = in.subspan(key_size + 1);
                 script_size += IsTapscript(ctx.MsContext()) ? 32 : 33;
-            } else if (Const("pk_h(", in)) {
+            } else if (Const("pk_h(", in, /*skip=*/false)) {
-                auto res = ParseKeyEnd<Key>(in, ctx);
+                std::optional<Key> key = ParseKey<Key, Ctx>("pk_h", in, ctx);
-                if (!res) return {};
+                if (!key) return {};
-                auto& [key, key_size] = *res;
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key)));
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(key)));
-                in = in.subspan(key_size + 1);
                 script_size += 23;
-            } else if (Const("sha256(", in)) {
+            } else if (Const("sha256(", in, /*skip=*/false)) {
-                auto res = ParseHexStrEnd(in, 32, ctx);
+                std::optional<std::vector<unsigned char>> hash = ParseHexStr("sha256", in, 32, ctx);
-                if (!res) return {};
+                if (!hash) return {};
-                auto& [hash, hash_size] = *res;
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, std::move(*hash));
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, std::move(hash));
-                in = in.subspan(hash_size + 1);
                 script_size += 38;
-            } else if (Const("ripemd160(", in)) {
+            } else if (Const("ripemd160(", in, /*skip=*/false)) {
-                auto res = ParseHexStrEnd(in, 20, ctx);
+                std::optional<std::vector<unsigned char>> hash = ParseHexStr("ripemd160", in, 20, ctx);
-                if (!res) return {};
+                if (!hash) return {};
-                auto& [hash, hash_size] = *res;
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, std::move(*hash));
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, std::move(hash));
-                in = in.subspan(hash_size + 1);
                 script_size += 26;
-            } else if (Const("hash256(", in)) {
+            } else if (Const("hash256(", in, /*skip=*/false)) {
-                auto res = ParseHexStrEnd(in, 32, ctx);
+                std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash256", in, 32, ctx);
-                if (!res) return {};
+                if (!hash) return {};
-                auto& [hash, hash_size] = *res;
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, std::move(*hash));
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, std::move(hash));
-                in = in.subspan(hash_size + 1);
                 script_size += 38;
-            } else if (Const("hash160(", in)) {
+            } else if (Const("hash160(", in, /*skip=*/false)) {
-                auto res = ParseHexStrEnd(in, 20, ctx);
+                std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash160", in, 20, ctx);
-                if (!res) return {};
+                if (!hash) return {};
-                auto& [hash, hash_size] = *res;
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, std::move(*hash));
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, std::move(hash));
-                in = in.subspan(hash_size + 1);
                 script_size += 26;
-            } else if (Const("after(", in)) {
+            } else if (Const("after(", in, /*skip=*/false)) {
-                int arg_size = FindNextChar(in, ')');
+                auto expr = Expr(in);
-                if (arg_size < 1) return {};
+                if (!Func("after", expr)) return {};
-                const auto num{ToIntegral<int64_t>(std::string_view(in.data(), arg_size))};
+                const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
                 if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
                 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, *num);
-                in = in.subspan(arg_size + 1);
                 script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
-            } else if (Const("older(", in)) {
+            } else if (Const("older(", in, /*skip=*/false)) {
-                int arg_size = FindNextChar(in, ')');
+                auto expr = Expr(in);
-                if (arg_size < 1) return {};
+                if (!Func("older", expr)) return {};
-                const auto num{ToIntegral<int64_t>(std::string_view(in.data(), arg_size))};
+                const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
                 if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
                 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num);
-                in = in.subspan(arg_size + 1);
                 script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
             } else if (Const("multi(", in)) {
                 if (!parse_multi_exp(in, /* is_multi_a = */false)) return {};

src/test/descriptor_tests.cpp

@@ -283,6 +283,14 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
     BOOST_CHECK_EQUAL(parse_pub->IsRange(), (flags & RANGE) != 0);
     BOOST_CHECK_EQUAL(parse_priv->IsRange(), (flags & RANGE) != 0);
 
+    // Check that the highest key expression index matches the number of keys in the descriptor
+    BOOST_TEST_INFO("Pub desc: " + pub);
+    uint32_t key_exprs = parse_pub->GetMaxKeyExpr();
+    BOOST_CHECK_EQUAL(key_exprs + 1, parse_pub->GetKeyCount());
+    BOOST_TEST_INFO("Priv desc: " + prv);
+    BOOST_CHECK_EQUAL(key_exprs, parse_priv->GetMaxKeyExpr());
+    BOOST_CHECK_EQUAL(key_exprs + 1, parse_priv->GetKeyCount());
+
     // * For ranged descriptors,  the `scripts` parameter is a list of expected result outputs, for subsequent
     //   positions to evaluate the descriptors on (so the first element of `scripts` is for evaluating the
     //   descriptor at 0; the second at 1; and so on). To verify this, we evaluate the descriptors once for
@@ -1267,7 +1275,7 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
     CheckUnparsable("tr(musig(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/*)/0)","tr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/*)/0)", "tr(): musig(): Cannot have ranged participant keys if musig() also has derivation");
 
     // Fuzzer crash test cases
-    CheckUnparsable("pk(musig(dd}uue/00/)k(", "pk(musig(dd}uue/00/)k(", "Invalid musig() expression");
+    CheckUnparsable("pk(musig(dd}uue/00/)k(", "pk(musig(dd}uue/00/)k(", "'pk(musig(dd}uue/00/)k(' is not a valid descriptor function");
     CheckUnparsable("tr(musig(tuus(oldepk(gg)ggggfgg)<,z(((((((((((((((((((((st)", "tr(musig(tuus(oldepk(gg)ggggfgg)<,z(((((((((((((((((((((st)","tr(): Too many ')' in musig() expression");
 }
 

src/test/fuzz/descriptor_parse.cpp

@@ -61,6 +61,10 @@ static void TestDescriptor(const Descriptor& desc, FlatSigningProvider& sig_prov
     const bool is_nontop_or_nonsolvable{!*is_solvable || !desc.GetOutputType()};
     const bool is_input_size_info_set{max_sat_maxsig && max_sat_nonmaxsig && max_elems};
     assert(is_input_size_info_set || is_nontop_or_nonsolvable);
+
+    auto max_key_expr = desc.GetMaxKeyExpr();
+    auto key_count = desc.GetKeyCount();
+    assert((max_key_expr == 0 && key_count == 0) || max_key_expr + 1 == key_count);
 }
 
 void initialize_descriptor_parse()

src/test/fuzz/miniscript.cpp

@@ -154,10 +154,9 @@ struct ParserContext {
         return {h.begin(), h.end()};
     }
 
-    template<typename I>
+    std::optional<Key> FromString(std::span<const char>& in) const {
-    std::optional<Key> FromString(I first, I last) const {
+        if (in.size() != 2) return {};
-        if (last - first != 2) return {};
+        auto idx = ParseHex(std::string(in.begin(), in.end()));
-        auto idx = ParseHex(std::string(first, last));
         if (idx.size() != 1) return {};
         return TEST_DATA.dummy_keys[idx[0]];
     }

src/test/miniscript_tests.cpp

@@ -159,9 +159,8 @@ struct KeyConverter {
     }
 
     //! Parse a public key from a range of hex characters.
-    template<typename I>
+    std::optional<Key> FromString(std::span<const char>& in) const {
-    std::optional<Key> FromString(I first, I last) const {
+        auto bytes = ParseHex(std::string(in.begin(), in.end()));
-        auto bytes = ParseHex(std::string(first, last));
         Key key{bytes.begin(), bytes.end()};
         if (key.IsValid()) return key;
         return {};

src/wallet/test/walletload_tests.cpp

@@ -37,6 +37,8 @@ public:
     std::optional<int64_t> MaxSatisfactionElems() const override { return {}; }
     void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const override {}
     std::vector<std::string> Warnings() const override { return {}; }
+    uint32_t GetMaxKeyExpr() const override { return 0; }
+    size_t GetKeyCount() const override { return 0; }
 };
 
 BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup)

test/functional/wallet_musig.py

@@ -7,6 +7,7 @@ import re
 
 from test_framework.descriptors import descsum_create
 from test_framework.key import H_POINT
+from test_framework.script import hash160
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -221,7 +222,7 @@ class WalletMuSigTest(BitcoinTestFramework):
                 utxo = wallet.listunspent()[0]
             else:
                 assert_equal(utxo, wallet.listunspent()[0])
-        psbt = wallets[0].walletcreatefundedpsbt(outputs=[{self.def_wallet.getnewaddress(): 5}], inputs=[utxo], change_type="bech32m", changePosition=1)["psbt"]
+        psbt = wallets[0].walletcreatefundedpsbt(outputs=[{self.def_wallet.getnewaddress(): 5}], inputs=[utxo], change_type="bech32m", changePosition=1, locktime=self.nodes[0].getblockcount())["psbt"]
 
         dec_psbt = self.nodes[0].decodepsbt(psbt)
         assert_equal(len(dec_psbt["inputs"]), 1)
@@ -261,6 +262,8 @@ class WalletMuSigTest(BitcoinTestFramework):
         assert_equal(len(dec_psbt["inputs"][0]["musig2_pubnonces"]), expected_pubnonces)
         for pn in dec_psbt["inputs"][0]["musig2_pubnonces"]:
             pubkey = pn["aggregate_pubkey"][2:]
+            if "pkh" in pattern or "pk_h" in pattern:
+                pubkey = hash160(bytes.fromhex(pubkey)).hex()
             if pubkey in dec_psbt["inputs"][0]["witness_utxo"]["scriptPubKey"]["hex"]:
                 continue
             elif "taproot_scripts" in dec_psbt["inputs"][0]:
@@ -287,6 +290,8 @@ class WalletMuSigTest(BitcoinTestFramework):
         assert_equal(len(dec_psbt["inputs"][0]["musig2_partial_sigs"]), expected_partial_sigs)
         for ps in dec_psbt["inputs"][0]["musig2_partial_sigs"]:
             pubkey = ps["aggregate_pubkey"][2:]
+            if "pkh" in pattern or "pk_h" in pattern:
+                pubkey = hash160(bytes.fromhex(pubkey)).hex()
             if pubkey in dec_psbt["inputs"][0]["witness_utxo"]["scriptPubKey"]["hex"]:
                 continue
             elif "taproot_scripts" in dec_psbt["inputs"][0]:
@@ -328,6 +333,11 @@ class WalletMuSigTest(BitcoinTestFramework):
         self.test_success_case("tr(H,{pk(musig/*), pk(same keys different musig/*)})", "tr($H,{pk(musig($0,$1,$2)/<0;1>/*),pk(musig($1,$2)/0/*)})", scriptpath=True)
         self.test_success_case("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})}", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})")
         self.test_success_case("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})} script-path", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})", scriptpath=True, nosign_wallets=[0])
+        self.test_success_case("tr(H,and(pk(musig/*),after(1)))", "tr($H,and_v(v:pk(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
+        self.test_success_case("tr(H,and(pk_k(musig/*),after(1)))", "tr($H,and_v(vc:pk_k(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
+        self.test_success_case("tr(H,and(pkh(musig/*),after(1)))", "tr($H,and_v(v:pkh(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
+        self.test_success_case("tr(H,and(pk_h(musig/*),after(1)))", "tr($H,and_v(vc:pk_h(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
+        self.test_success_case("tr(H,{and(pk(musig/*),after(1)),and(pk(musig/*),after(1))})", "tr($H,{and_v(v:pk(musig($0,$2)/0/*),after(1)),and_v(v:pk(musig($1,$2)/0/*),after(1))})", scriptpath=True)
 
         self.test_failure_case_1("missing participant nonce", "tr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))")
         self.test_failure_case_2("insufficient partial signatures", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))")