From ec0f47b15cb3269015523e6fab8ae9241f4181a1 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 22 Dec 2025 13:30:24 -0800 Subject: [PATCH] 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 | 4 +- src/script/miniscript.h | 125 ++++++++++++++-------------------- src/test/descriptor_tests.cpp | 2 +- src/test/fuzz/miniscript.cpp | 7 +- src/test/miniscript_tests.cpp | 5 +- 5 files changed, 61 insertions(+), 82 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index f761757f3f5..0f14672afa6 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -2206,11 +2206,11 @@ struct KeyParser { assert(false); } - template std::optional FromString(I begin, I end) const + std::optional FromString(std::span& 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 1b7e84f471d..8733d347136 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -1806,29 +1806,27 @@ enum class ParseContext { int FindNextChar(std::span 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 -std::optional> ParseKeyEnd(std::span in, const Ctx& ctx) +std::optional ParseKey(const std::string& func, std::span& 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 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 -std::optional, int>> ParseHexStrEnd(std::span in, const size_t expected_size, +std::optional> ParseHexStr(const std::string& func, std::span& 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 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> Parse(std::span 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 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> Parse(std::span 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(in, ctx); - if (!res) return {}; - auto& [key, key_size] = *res; - constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node(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 = ParseKey("pk", in, ctx); + if (!key) return {}; + constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node(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(in, ctx); - if (!res) return {}; - auto& [key, key_size] = *res; - constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node(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 = ParseKey("pkh", in, ctx); + if (!key) return {}; + constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key))))); script_size += 24; - } else if (Const("pk_k(", in)) { - auto res = ParseKeyEnd(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 = ParseKey("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(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 = ParseKey("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> 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> 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> 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> 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(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(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(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(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 00782c37215..a646fc83476 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 d20bf9fbd84..a7934f7e50f 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 - std::optional FromString(I first, I last) const { - if (last - first != 2) return {}; - auto idx = ParseHex(std::string(first, last)); + std::optional FromString(std::span& 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 757f89b63bc..9a3e9ea0789 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 - std::optional FromString(I first, I last) const { - auto bytes = ParseHex(std::string(first, last)); + std::optional FromString(std::span& in) const { + auto bytes = ParseHex(std::string(in.begin(), in.end())); Key key{bytes.begin(), bytes.end()}; if (key.IsValid()) return key; return {};