Moderate 59 Bitcoin Committed Feb 2, 2026 · 23:20 UTC

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

ec0f47b1 View source ↗

Public commit record

What the developer wrote

Authored by Ava Chow

73/100 · Adequate

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.

✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context

The short version

What changed, and why it matters

This change fixes how Bitcoin Core reads certain pieces of text inside Miniscript and descriptor expressions. Previously, the parser looked for the next closing parenthesis to decide where an argument ended. That simple scan broke when the argument itself contained nested parentheses, such as a musig(...) key expression inside pk(...). The new code uses proper expression-parsing helpers that understand matching parentheses, so nested structures are handled correctly. The commit message explicitly calls this a fix for parsing musig() inside pk(), pkh(), pk_k(), and pk_h().

Recommended action

Treat as a bug-fix patch that should be applied to maintained branches using Miniscript/descriptor parsing. Review whether the old parser could be induced to accept malformed descriptors or produce incorrect script structures when nested parentheses appear, and consider adding regression tests for valid nested musig() expressions inside pk()/pkh().

Security signals we found

Parser previously used naive 'next )' delimiter scan, which mishandles nested parentheses

Commit message states this fixes parsing of musig() nested inside pk()/pkh()/pk_k()/pk_h()

Fuzzer crash test case retained and its expected error message updated

Change is localized to Miniscript/descriptor parsing, not consensus or P2P

No explicit CVE, security advisory, or researcher attribution in commit or references

Technical analysis

Evidence from the diff

The patch replaces ad-hoc FindNextChar(…, ‘)’) scanning in Miniscript parsing with the existing script::Expr() and script::Func() helpers. ParseKeyEnd and ParseHexStrEnd are replaced by ParseKey and ParseHexStr, which extract a balanced sub-expression and then verify it starts with the expected function name. FromString callbacks now take a std::span& instead of iterator pairs. The affected fragments are pk(), pk_k(), pkh(), pk_h(), sha256(), ripemd160(), hash256(), hash160(), after(), and older(). A fuzzer crash test case’s expected error message is updated, confirming the parser now rejects malformed nested input through the function-parsing machinery rather than the old parenthesis scan.

Changed components

src/script/miniscript.hsrc/script/descriptor.cppsrc/test/descriptor_tests.cppsrc/test/fuzz/miniscript.cppsrc/test/miniscript_tests.cpp

Inspect captured patch +61 / −82

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index f761757f..0f14672a 100644
--- a/src/script/descriptor.cpp
+++ b/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;
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 1b7e84f4..8733d347 100644
--- a/src/script/miniscript.h
+++ b/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, ')');
-    if (key_size < 1) return {};
-    auto key = ctx.FromString(in.begin(), in.begin() + key_size);
-    if (!key) return {};
-    return {{std::move(*key), key_size}};
+    std::span<const char> expr = script::Expr(in);
+    if (!script::Func(func, expr)) return {};
+    return ctx.FromString(expr);
 }
 
-/** 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, ')');
-    if (hash_size < 1) return {};
-    std::string val = std::string(in.begin(), in.begin() + hash_size);
+    std::span<const char> expr = script::Expr(in);
+    if (!script::Func(func, expr)) return {};
+    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)) {
-                auto res = ParseKeyEnd<Key, Ctx>(in, ctx);
-                if (!res) 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)))));
-                in = in.subspan(key_size + 1);
+            } else if (Const("pk(", in, /*skip=*/false)) {
+                std::optional<Key> key = ParseKey<Key, Ctx>("pk", in, ctx);
+                if (!key) return {};
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(*key)))));
                 script_size += IsTapscript(ctx.MsContext()) ? 33 : 34;
-            } else if (Const("pkh(", in)) {
-                auto res = ParseKeyEnd<Key>(in, ctx);
-                if (!res) 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)))));
-                in = in.subspan(key_size + 1);
+            } else if (Const("pkh(", in, /*skip=*/false)) {
+                std::optional<Key> key = ParseKey<Key, Ctx>("pkh", in, ctx);
+                if (!key) return {};
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key)))));
                 script_size += 24;
-            } else if (Const("pk_k(", in)) {
-                auto res = ParseKeyEnd<Key>(in, ctx);
-                if (!res) return {};
-                auto& [key, key_size] = *res;
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(key)));
-                in = in.subspan(key_size + 1);
+            } else if (Const("pk_k(", in, /*skip=*/false)) {
+                std::optional<Key> key = ParseKey<Key, Ctx>("pk_k", in, ctx);
+                if (!key) return {};
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(*key)));
                 script_size += IsTapscript(ctx.MsContext()) ? 32 : 33;
-            } else if (Const("pk_h(", in)) {
-                auto res = ParseKeyEnd<Key>(in, ctx);
-                if (!res) return {};
-                auto& [key, key_size] = *res;
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(key)));
-                in = in.subspan(key_size + 1);
+            } else if (Const("pk_h(", in, /*skip=*/false)) {
+                std::optional<Key> key = ParseKey<Key, Ctx>("pk_h", in, ctx);
+                if (!key) return {};
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key)));
                 script_size += 23;
-            } else if (Const("sha256(", in)) {
-                auto res = ParseHexStrEnd(in, 32, ctx);
-                if (!res) return {};
-                auto& [hash, hash_size] = *res;
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, std::move(hash));
-                in = in.subspan(hash_size + 1);
+            } else if (Const("sha256(", in, /*skip=*/false)) {
+                std::optional<std::vector<unsigned char>> hash = ParseHexStr("sha256", in, 32, ctx);
+                if (!hash) return {};
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, std::move(*hash));
                 script_size += 38;
-            } else if (Const("ripemd160(", in)) {
-                auto res = ParseHexStrEnd(in, 20, ctx);
-                if (!res) return {};
-                auto& [hash, hash_size] = *res;
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, std::move(hash));
-                in = in.subspan(hash_size + 1);
+            } else if (Const("ripemd160(", in, /*skip=*/false)) {
+                std::optional<std::vector<unsigned char>> hash = ParseHexStr("ripemd160", in, 20, ctx);
+                if (!hash) return {};
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, std::move(*hash));
                 script_size += 26;
-            } else if (Const("hash256(", in)) {
-                auto res = ParseHexStrEnd(in, 32, ctx);
-                if (!res) return {};
-                auto& [hash, hash_size] = *res;
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, std::move(hash));
-                in = in.subspan(hash_size + 1);
+            } else if (Const("hash256(", in, /*skip=*/false)) {
+                std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash256", in, 32, ctx);
+                if (!hash) return {};
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, std::move(*hash));
                 script_size += 38;
-            } else if (Const("hash160(", in)) {
-                auto res = ParseHexStrEnd(in, 20, ctx);
-                if (!res) return {};
-                auto& [hash, hash_size] = *res;
-                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, std::move(hash));
-                in = in.subspan(hash_size + 1);
+            } else if (Const("hash160(", in, /*skip=*/false)) {
+                std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash160", in, 20, ctx);
+                if (!hash) return {};
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, std::move(*hash));
                 script_size += 26;
-            } else if (Const("after(", in)) {
-                int arg_size = FindNextChar(in, ')');
-                if (arg_size < 1) return {};
-                const auto num{ToIntegral<int64_t>(std::string_view(in.data(), arg_size))};
+            } else if (Const("after(", in, /*skip=*/false)) {
+                auto expr = Expr(in);
+                if (!Func("after", expr)) return {};
+                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)) {
-                int arg_size = FindNextChar(in, ')');
-                if (arg_size < 1) return {};
-                const auto num{ToIntegral<int64_t>(std::string_view(in.data(), arg_size))};
+            } else if (Const("older(", in, /*skip=*/false)) {
+                auto expr = Expr(in);
+                if (!Func("older", expr)) return {};
+                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 {};
diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 00782c37..a646fc83 100644
--- a/src/test/descriptor_tests.cpp
+++ b/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");
 }
 
diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp
index d20bf9fb..a7934f7e 100644
--- a/src/test/fuzz/miniscript.cpp
+++ b/src/test/fuzz/miniscript.cpp
@@ -154,10 +154,9 @@ struct ParserContext {
         return {h.begin(), h.end()};
     }
 
-    template<typename I>
-    std::optional<Key> FromString(I first, I last) const {
-        if (last - first != 2) return {};
-        auto idx = ParseHex(std::string(first, last));
+    std::optional<Key> FromString(std::span<const char>& in) const {
+        if (in.size() != 2) return {};
+        auto idx = ParseHex(std::string(in.begin(), in.end()));
         if (idx.size() != 1) return {};
         return TEST_DATA.dummy_keys[idx[0]];
     }
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 757f89b6..9a3e9ea0 100644
--- a/src/test/miniscript_tests.cpp
+++ b/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(I first, I last) const {
-        auto bytes = ParseHex(std::string(first, last));
+    std::optional<Key> FromString(std::span<const char>& in) const {
+        auto bytes = ParseHex(std::string(in.begin(), in.end()));
         Key key{bytes.begin(), bytes.end()};
         if (key.IsValid()) return key;
         return {};

Risk score

Why this scored 59/100

Our methodology →

Potential impact 18/30

Exploitability 12/25

Stealth signal 8/15

Affected reach 10/15

Confidence 7/10

Evidence quality 4/5

Human-validated context

Community notes

Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.

No validated notes yet.

The AI analysis stands alone for now. Submit a note if you can add evidence or important context.

What the developer wrote

What changed, and why it matters

Security signals we found

Evidence from the diff

Changed components

Why this scored 59/100

Community notes

Related watches

Merge bitcoin/bitcoin#35928: doc: mention -DWITH_ZMQ=ON in macOS build guide

Merge bitcoin/bitcoin#35482: fuzz: exercise the transaction-handling path in process_message(s)

Merge bitcoin/bitcoin#35914: test, fuzz: Remove unused variables