miniscript: Using Func and Expr when parsing keys, hashes, and locktimes

Since pk(), pk_k(), pkh(), pk_h(), sha256(), ripemd160(), hash256(),
hash160(), after(), and older() all are single argument expressions that
are parsed immediately, we can use the Expr and Func parsing functions
to determine what the arguments of these expressions are, rather than
searching for the next closing parentheses.

This fixes an issue when pk(), pk_k(), pkh(), and pk_h() include a
musig() expression as Expr properly handles nested expressions.

src/script/descriptor.cpp

@@ -2206,11 +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();
-        auto pk = ParsePubkey(m_expr_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;

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

@@ -1275,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/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 {};