From 4cb8f9a92c0ab5d45f2e545bbc13fc3f2ec611ea Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 10 Jun 2022 17:30:22 -0400 Subject: [PATCH 1/2] Permit delaying duplicate key check in miniscript::Node construction --- src/script/miniscript.h | 226 ++++++++++++++++++++-------------- src/test/miniscript_tests.cpp | 3 + 2 files changed, 139 insertions(+), 90 deletions(-) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index f783d1dafc..21e2959e94 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -276,6 +276,8 @@ struct StackSize { StackSize(MaxInt in_sat, MaxInt in_dsat) : sat(in_sat), dsat(in_dsat) {}; }; +struct NoDupCheck {}; + } // namespace internal //! A node in a miniscript expression. @@ -301,8 +303,13 @@ private: const Type typ; //! Cached script length (computed by CalcScriptLen). const size_t scriptlen; - //! Whether a public key appears more than once in this node. - const bool duplicate_key; + //! Whether a public key appears more than once in this node. This value is initialized + //! by all constructors except the NoDupCheck ones. The NoDupCheck ones skip the + //! computation, requiring it to be done manually by invoking DuplicateKeyCheck(). + //! DuplicateKeyCheck(), or a non-NoDupCheck constructor, will compute has_duplicate_keys + //! for all subnodes as well. + mutable std::optional has_duplicate_keys; + //! Compute the length of the script for this miniscript (including children). size_t CalcScriptLen() const { @@ -654,6 +661,7 @@ public: return TreeEvalMaybe(false, downfn, upfn); } +private: internal::Ops CalcOps() const { switch (fragment) { case Fragment::JUST_1: return {0, 0, {}}; @@ -777,11 +785,14 @@ public: assert(false); } - /** Check whether any key is repeated. +public: + /** Update duplicate key information in this Node. + * * This uses a custom key comparator provided by the context in order to still detect duplicates * for more complicated types. */ - template bool ContainsDuplicateKey(const Ctx& ctx) const { + template void DuplicateKeyCheck(const Ctx& ctx) const + { // We cannot use a lambda here, as lambdas are non assignable, and the set operations // below require moving the comparators around. struct Comp { @@ -789,31 +800,55 @@ public: Comp(const Ctx& ctx) : ctx_ptr(&ctx) {} bool operator()(const Key& a, const Key& b) const { return ctx_ptr->KeyCompare(a, b); } }; - using set = std::set; - auto upfn = [this, &ctx](const Node& node, Span subs) -> std::optional { - if (&node != this && node.duplicate_key) return {}; + // state in the recursive computation: + // - std::nullopt means "this node has duplicates" + // - an std::set means "this node has no duplicate keys, and they are: ...". + using keyset = std::set; + using state = std::optional; - size_t keys_count = node.keys.size(); - set key_set{node.keys.begin(), node.keys.end(), Comp(ctx)}; - if (key_set.size() != keys_count) return {}; + auto upfn = [&ctx](const Node& node, Span subs) -> state { + // If this node is already known to have duplicates, nothing left to do. + if (node.has_duplicate_keys.has_value() && *node.has_duplicate_keys) return {}; - for (auto& sub: subs) { - keys_count += sub.size(); - // Small optimization: std::set::merge is linear in the size of the second arg but - // logarithmic in the size of the first. - if (key_set.size() < sub.size()) std::swap(key_set, sub); - key_set.merge(sub); - if (key_set.size() != keys_count) return {}; + // Check if one of the children is already known to have duplicates. + for (auto& sub : subs) { + if (!sub.has_value()) { + node.has_duplicate_keys = true; + return {}; + } } + // Start building the set of keys involved in this node and children. + // Start by keys in this node directly. + size_t keys_count = node.keys.size(); + keyset key_set{node.keys.begin(), node.keys.end(), Comp(ctx)}; + if (key_set.size() != keys_count) { + // It already has duplicates; bail out. + node.has_duplicate_keys = true; + return {}; + } + + // Merge the keys from the children into this set. + for (auto& sub : subs) { + keys_count += sub->size(); + // Small optimization: std::set::merge is linear in the size of the second arg but + // logarithmic in the size of the first. + if (key_set.size() < sub->size()) std::swap(key_set, *sub); + key_set.merge(*sub); + if (key_set.size() != keys_count) { + node.has_duplicate_keys = true; + return {}; + } + } + + node.has_duplicate_keys = false; return key_set; }; - return !TreeEvalMaybe(upfn); + TreeEval(upfn); } -public: //! Return the size of the script for this expression (faster than ToScript().size()). size_t ScriptSize() const { return scriptlen; } @@ -858,7 +893,7 @@ public: bool CheckTimeLocksMix() const { return GetType() << "k"_mst; } //! Check whether there is no duplicate key across this fragment and all its sub-fragments. - bool CheckDuplicateKey() const { return !duplicate_key; } + bool CheckDuplicateKey() const { return has_duplicate_keys && !*has_duplicate_keys; } //! Whether successful non-malleable satisfactions are guaranteed to be valid. bool ValidSatisfactions() const { return IsValid() && CheckOpsLimit() && CheckStackSize(); } @@ -872,13 +907,21 @@ public: //! Equality testing. bool operator==(const Node& arg) const { return Compare(*this, arg) == 0; } - // Constructors with various argument combinations. - template Node(const Ctx& ctx, Fragment nt, std::vector> sub, std::vector arg, uint32_t val = 0) : fragment(nt), k(val), data(std::move(arg)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()), duplicate_key(ContainsDuplicateKey(ctx)) {} - template Node(const Ctx& ctx, Fragment nt, std::vector arg, uint32_t val = 0) : fragment(nt), k(val), data(std::move(arg)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()), duplicate_key(ContainsDuplicateKey(ctx)) {} - template Node(const Ctx& ctx, Fragment nt, std::vector> sub, std::vector key, uint32_t val = 0) : fragment(nt), k(val), keys(std::move(key)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()), duplicate_key(ContainsDuplicateKey(ctx)) {} - template Node(const Ctx& ctx, Fragment nt, std::vector key, uint32_t val = 0) : fragment(nt), k(val), keys(std::move(key)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()), duplicate_key(ContainsDuplicateKey(ctx)) {} - template Node(const Ctx& ctx, Fragment nt, std::vector> sub, uint32_t val = 0) : fragment(nt), k(val), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()), duplicate_key(ContainsDuplicateKey(ctx)) {} - template Node(const Ctx& ctx, Fragment nt, uint32_t val = 0) : fragment(nt), k(val), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()), duplicate_key(ContainsDuplicateKey(ctx)) {} + // Constructors with various argument combinations, which bypass the duplicate key check. + Node(internal::NoDupCheck, Fragment nt, std::vector> sub, std::vector arg, uint32_t val = 0) : fragment(nt), k(val), data(std::move(arg)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(internal::NoDupCheck, Fragment nt, std::vector arg, uint32_t val = 0) : fragment(nt), k(val), data(std::move(arg)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(internal::NoDupCheck, Fragment nt, std::vector> sub, std::vector key, uint32_t val = 0) : fragment(nt), k(val), keys(std::move(key)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(internal::NoDupCheck, Fragment nt, std::vector key, uint32_t val = 0) : fragment(nt), k(val), keys(std::move(key)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(internal::NoDupCheck, Fragment nt, std::vector> sub, uint32_t val = 0) : fragment(nt), k(val), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(internal::NoDupCheck, Fragment nt, uint32_t val = 0) : fragment(nt), k(val), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + + // Constructors with various argument combinations, which do perform the duplicate key check. + template Node(const Ctx& ctx, Fragment nt, std::vector> sub, std::vector arg, uint32_t val = 0) : Node(internal::NoDupCheck{}, nt, std::move(sub), std::move(arg), val) { DuplicateKeyCheck(ctx); } + template Node(const Ctx& ctx, Fragment nt, std::vector arg, uint32_t val = 0) : Node(internal::NoDupCheck{}, nt, std::move(arg), val) { DuplicateKeyCheck(ctx);} + template Node(const Ctx& ctx, Fragment nt, std::vector> sub, std::vector key, uint32_t val = 0) : Node(internal::NoDupCheck{}, nt, std::move(sub), std::move(key), val) { DuplicateKeyCheck(ctx); } + template Node(const Ctx& ctx, Fragment nt, std::vector key, uint32_t val = 0) : Node(internal::NoDupCheck{}, nt, std::move(key), val) { DuplicateKeyCheck(ctx); } + template Node(const Ctx& ctx, Fragment nt, std::vector> sub, uint32_t val = 0) : Node(internal::NoDupCheck{}, nt, std::move(sub), val) { DuplicateKeyCheck(ctx); } + template Node(const Ctx& ctx, Fragment nt, uint32_t val = 0) : Node(internal::NoDupCheck{}, nt, val) { DuplicateKeyCheck(ctx); } }; namespace internal { @@ -965,15 +1008,15 @@ std::optional, int>> ParseHexStrEnd(Span -void BuildBack(const Ctx& ctx, Fragment nt, std::vector>& constructed, const bool reverse = false) +template +void BuildBack(Fragment nt, std::vector>& constructed, const bool reverse = false) { NodeRef child = std::move(constructed.back()); constructed.pop_back(); if (reverse) { - constructed.back() = MakeNodeRef(ctx, nt, Vector(std::move(child), std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, nt, Vector(std::move(child), std::move(constructed.back()))); } else { - constructed.back() = MakeNodeRef(ctx, nt, Vector(std::move(constructed.back()), std::move(child))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, nt, Vector(std::move(constructed.back()), std::move(child))); } } @@ -1030,7 +1073,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) to_parse.emplace_back(ParseContext::WRAP_T, -1, -1); } else if (in[j] == 'l') { // The l: wrapper is equivalent to or_i(0,X) - constructed.push_back(MakeNodeRef(ctx, Fragment::JUST_0)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::JUST_0)); to_parse.emplace_back(ParseContext::OR_I, -1, -1); } else { return {}; @@ -1042,56 +1085,56 @@ inline NodeRef Parse(Span in, const Ctx& ctx) } case ParseContext::EXPR: { if (Const("0", in)) { - constructed.push_back(MakeNodeRef(ctx, Fragment::JUST_0)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::JUST_0)); } else if (Const("1", in)) { - constructed.push_back(MakeNodeRef(ctx, Fragment::JUST_1)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::JUST_1)); } else if (Const("pk(", in)) { auto res = ParseKeyEnd(in, ctx); if (!res) return {}; auto& [key, key_size] = *res; - constructed.push_back(MakeNodeRef(ctx, Fragment::WRAP_C, Vector(MakeNodeRef(ctx, Fragment::PK_K, Vector(std::move(key)))))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_C, Vector(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_K, Vector(std::move(key)))))); in = in.subspan(key_size + 1); } else if (Const("pkh(", in)) { auto res = ParseKeyEnd(in, ctx); if (!res) return {}; auto& [key, key_size] = *res; - constructed.push_back(MakeNodeRef(ctx, Fragment::WRAP_C, Vector(MakeNodeRef(ctx, Fragment::PK_H, Vector(std::move(key)))))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_C, Vector(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_H, Vector(std::move(key)))))); in = in.subspan(key_size + 1); } else if (Const("pk_k(", in)) { auto res = ParseKeyEnd(in, ctx); if (!res) return {}; auto& [key, key_size] = *res; - constructed.push_back(MakeNodeRef(ctx, Fragment::PK_K, Vector(std::move(key)))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_K, Vector(std::move(key)))); in = in.subspan(key_size + 1); } else if (Const("pk_h(", in)) { auto res = ParseKeyEnd(in, ctx); if (!res) return {}; auto& [key, key_size] = *res; - constructed.push_back(MakeNodeRef(ctx, Fragment::PK_H, Vector(std::move(key)))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_H, Vector(std::move(key)))); in = in.subspan(key_size + 1); } else if (Const("sha256(", in)) { auto res = ParseHexStrEnd(in, 32, ctx); if (!res) return {}; auto& [hash, hash_size] = *res; - constructed.push_back(MakeNodeRef(ctx, Fragment::SHA256, std::move(hash))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::SHA256, std::move(hash))); in = in.subspan(hash_size + 1); } else if (Const("ripemd160(", in)) { auto res = ParseHexStrEnd(in, 20, ctx); if (!res) return {}; auto& [hash, hash_size] = *res; - constructed.push_back(MakeNodeRef(ctx, Fragment::RIPEMD160, std::move(hash))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::RIPEMD160, std::move(hash))); in = in.subspan(hash_size + 1); } else if (Const("hash256(", in)) { auto res = ParseHexStrEnd(in, 32, ctx); if (!res) return {}; auto& [hash, hash_size] = *res; - constructed.push_back(MakeNodeRef(ctx, Fragment::HASH256, std::move(hash))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::HASH256, std::move(hash))); in = in.subspan(hash_size + 1); } else if (Const("hash160(", in)) { auto res = ParseHexStrEnd(in, 20, ctx); if (!res) return {}; auto& [hash, hash_size] = *res; - constructed.push_back(MakeNodeRef(ctx, Fragment::HASH160, std::move(hash))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::HASH160, std::move(hash))); in = in.subspan(hash_size + 1); } else if (Const("after(", in)) { int arg_size = FindNextChar(in, ')'); @@ -1099,7 +1142,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) int64_t num; if (!ParseInt64(std::string(in.begin(), in.begin() + arg_size), &num)) return {}; if (num < 1 || num >= 0x80000000L) return {}; - constructed.push_back(MakeNodeRef(ctx, Fragment::AFTER, num)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::AFTER, num)); in = in.subspan(arg_size + 1); } else if (Const("older(", in)) { int arg_size = FindNextChar(in, ')'); @@ -1107,7 +1150,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) int64_t num; if (!ParseInt64(std::string(in.begin(), in.begin() + arg_size), &num)) return {}; if (num < 1 || num >= 0x80000000L) return {}; - constructed.push_back(MakeNodeRef(ctx, Fragment::OLDER, num)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::OLDER, num)); in = in.subspan(arg_size + 1); } else if (Const("multi(", in)) { // Get threshold @@ -1128,7 +1171,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) } if (keys.size() < 1 || keys.size() > 20) return {}; if (k < 1 || k > (int64_t)keys.size()) return {}; - constructed.push_back(MakeNodeRef(ctx, Fragment::MULTI, std::move(keys), k)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::MULTI, std::move(keys), k)); } else if (Const("thresh(", in)) { int next_comma = FindNextChar(in, ','); if (next_comma < 1) return {}; @@ -1172,69 +1215,69 @@ inline NodeRef Parse(Span in, const Ctx& ctx) break; } case ParseContext::ALT: { - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_A, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_A, Vector(std::move(constructed.back()))); break; } case ParseContext::SWAP: { - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_S, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_S, Vector(std::move(constructed.back()))); break; } case ParseContext::CHECK: { - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_C, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_C, Vector(std::move(constructed.back()))); break; } case ParseContext::DUP_IF: { - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_D, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_D, Vector(std::move(constructed.back()))); break; } case ParseContext::NON_ZERO: { - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_J, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_J, Vector(std::move(constructed.back()))); break; } case ParseContext::ZERO_NOTEQUAL: { - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_N, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_N, Vector(std::move(constructed.back()))); break; } case ParseContext::VERIFY: { - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_V, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_V, Vector(std::move(constructed.back()))); break; } case ParseContext::WRAP_U: { - constructed.back() = MakeNodeRef(ctx, Fragment::OR_I, Vector(std::move(constructed.back()), MakeNodeRef(ctx, Fragment::JUST_0))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::OR_I, Vector(std::move(constructed.back()), MakeNodeRef(internal::NoDupCheck{}, Fragment::JUST_0))); break; } case ParseContext::WRAP_T: { - constructed.back() = MakeNodeRef(ctx, Fragment::AND_V, Vector(std::move(constructed.back()), MakeNodeRef(ctx, Fragment::JUST_1))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::AND_V, Vector(std::move(constructed.back()), MakeNodeRef(internal::NoDupCheck{}, Fragment::JUST_1))); break; } case ParseContext::AND_B: { - BuildBack(ctx, Fragment::AND_B, constructed); + BuildBack(Fragment::AND_B, constructed); break; } case ParseContext::AND_N: { auto mid = std::move(constructed.back()); constructed.pop_back(); - constructed.back() = MakeNodeRef(ctx, Fragment::ANDOR, Vector(std::move(constructed.back()), std::move(mid), MakeNodeRef(ctx, Fragment::JUST_0))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::ANDOR, Vector(std::move(constructed.back()), std::move(mid), MakeNodeRef(ctx, Fragment::JUST_0))); break; } case ParseContext::AND_V: { - BuildBack(ctx, Fragment::AND_V, constructed); + BuildBack(Fragment::AND_V, constructed); break; } case ParseContext::OR_B: { - BuildBack(ctx, Fragment::OR_B, constructed); + BuildBack(Fragment::OR_B, constructed); break; } case ParseContext::OR_C: { - BuildBack(ctx, Fragment::OR_C, constructed); + BuildBack(Fragment::OR_C, constructed); break; } case ParseContext::OR_D: { - BuildBack(ctx, Fragment::OR_D, constructed); + BuildBack(Fragment::OR_D, constructed); break; } case ParseContext::OR_I: { - BuildBack(ctx, Fragment::OR_I, constructed); + BuildBack(Fragment::OR_I, constructed); break; } case ParseContext::ANDOR: { @@ -1242,7 +1285,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) constructed.pop_back(); auto mid = std::move(constructed.back()); constructed.pop_back(); - constructed.back() = MakeNodeRef(ctx, Fragment::ANDOR, Vector(std::move(constructed.back()), std::move(mid), std::move(right))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::ANDOR, Vector(std::move(constructed.back()), std::move(mid), std::move(right))); break; } case ParseContext::THRESH: { @@ -1261,7 +1304,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) constructed.pop_back(); } std::reverse(subs.begin(), subs.end()); - constructed.push_back(MakeNodeRef(ctx, Fragment::THRESH, std::move(subs), k)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::THRESH, std::move(subs), k)); } else { return {}; } @@ -1283,7 +1326,9 @@ inline NodeRef Parse(Span in, const Ctx& ctx) // Sanity checks on the produced miniscript assert(constructed.size() == 1); if (in.size() > 0) return {}; - return std::move(constructed.front()); + const NodeRef tl_node = std::move(constructed.front()); + tl_node->DuplicateKeyCheck(ctx); + return tl_node; } /** Decode a script into opcode/push pairs. @@ -1394,12 +1439,12 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) // Constants if (in[0].first == OP_1) { ++in; - constructed.push_back(MakeNodeRef(ctx, Fragment::JUST_1)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::JUST_1)); break; } if (in[0].first == OP_0) { ++in; - constructed.push_back(MakeNodeRef(ctx, Fragment::JUST_0)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::JUST_0)); break; } // Public keys @@ -1407,14 +1452,14 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) auto key = ctx.FromPKBytes(in[0].second.begin(), in[0].second.end()); if (!key) return {}; ++in; - constructed.push_back(MakeNodeRef(ctx, Fragment::PK_K, Vector(std::move(*key)))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_K, Vector(std::move(*key)))); break; } if (last - in >= 5 && in[0].first == OP_VERIFY && in[1].first == OP_EQUAL && in[3].first == OP_HASH160 && in[4].first == OP_DUP && in[2].second.size() == 20) { auto key = ctx.FromPKHBytes(in[2].second.begin(), in[2].second.end()); if (!key) return {}; in += 5; - constructed.push_back(MakeNodeRef(ctx, Fragment::PK_H, Vector(std::move(*key)))); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_H, Vector(std::move(*key)))); break; } // Time locks @@ -1422,31 +1467,31 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY && (num = ParseScriptNumber(in[1]))) { in += 2; if (*num < 1 || *num > 0x7FFFFFFFL) return {}; - constructed.push_back(MakeNodeRef(ctx, Fragment::OLDER, *num)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::OLDER, *num)); break; } if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) { in += 2; if (num < 1 || num > 0x7FFFFFFFL) return {}; - constructed.push_back(MakeNodeRef(ctx, Fragment::AFTER, *num)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::AFTER, *num)); break; } // Hashes if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && (num = ParseScriptNumber(in[5])) && num == 32 && in[6].first == OP_SIZE) { if (in[2].first == OP_SHA256 && in[1].second.size() == 32) { - constructed.push_back(MakeNodeRef(ctx, Fragment::SHA256, in[1].second)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::SHA256, in[1].second)); in += 7; break; } else if (in[2].first == OP_RIPEMD160 && in[1].second.size() == 20) { - constructed.push_back(MakeNodeRef(ctx, Fragment::RIPEMD160, in[1].second)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::RIPEMD160, in[1].second)); in += 7; break; } else if (in[2].first == OP_HASH256 && in[1].second.size() == 32) { - constructed.push_back(MakeNodeRef(ctx, Fragment::HASH256, in[1].second)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::HASH256, in[1].second)); in += 7; break; } else if (in[2].first == OP_HASH160 && in[1].second.size() == 20) { - constructed.push_back(MakeNodeRef(ctx, Fragment::HASH160, in[1].second)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::HASH160, in[1].second)); in += 7; break; } @@ -1467,7 +1512,7 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) if (!k || *k < 1 || *k > *n) return {}; in += 3 + *n; std::reverse(keys.begin(), keys.end()); - constructed.push_back(MakeNodeRef(ctx, Fragment::MULTI, std::move(keys), *k)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::MULTI, std::move(keys), *k)); break; } /** In the following wrappers, we only need to push SINGLE_BKV_EXPR rather @@ -1562,63 +1607,63 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) case DecodeContext::SWAP: { if (in >= last || in[0].first != OP_SWAP || constructed.empty()) return {}; ++in; - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_S, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_S, Vector(std::move(constructed.back()))); break; } case DecodeContext::ALT: { if (in >= last || in[0].first != OP_TOALTSTACK || constructed.empty()) return {}; ++in; - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_A, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_A, Vector(std::move(constructed.back()))); break; } case DecodeContext::CHECK: { if (constructed.empty()) return {}; - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_C, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_C, Vector(std::move(constructed.back()))); break; } case DecodeContext::DUP_IF: { if (constructed.empty()) return {}; - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_D, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_D, Vector(std::move(constructed.back()))); break; } case DecodeContext::VERIFY: { if (constructed.empty()) return {}; - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_V, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_V, Vector(std::move(constructed.back()))); break; } case DecodeContext::NON_ZERO: { if (constructed.empty()) return {}; - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_J, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_J, Vector(std::move(constructed.back()))); break; } case DecodeContext::ZERO_NOTEQUAL: { if (constructed.empty()) return {}; - constructed.back() = MakeNodeRef(ctx, Fragment::WRAP_N, Vector(std::move(constructed.back()))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_N, Vector(std::move(constructed.back()))); break; } case DecodeContext::AND_V: { if (constructed.size() < 2) return {}; - BuildBack(ctx, Fragment::AND_V, constructed, /*reverse=*/true); + BuildBack(Fragment::AND_V, constructed, /*reverse=*/true); break; } case DecodeContext::AND_B: { if (constructed.size() < 2) return {}; - BuildBack(ctx, Fragment::AND_B, constructed, /*reverse=*/true); + BuildBack(Fragment::AND_B, constructed, /*reverse=*/true); break; } case DecodeContext::OR_B: { if (constructed.size() < 2) return {}; - BuildBack(ctx, Fragment::OR_B, constructed, /*reverse=*/true); + BuildBack(Fragment::OR_B, constructed, /*reverse=*/true); break; } case DecodeContext::OR_C: { if (constructed.size() < 2) return {}; - BuildBack(ctx, Fragment::OR_C, constructed, /*reverse=*/true); + BuildBack(Fragment::OR_C, constructed, /*reverse=*/true); break; } case DecodeContext::OR_D: { if (constructed.size() < 2) return {}; - BuildBack(ctx, Fragment::OR_D, constructed, /*reverse=*/true); + BuildBack(Fragment::OR_D, constructed, /*reverse=*/true); break; } case DecodeContext::ANDOR: { @@ -1628,7 +1673,7 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) NodeRef right = std::move(constructed.back()); constructed.pop_back(); NodeRef mid = std::move(constructed.back()); - constructed.back() = MakeNodeRef(ctx, Fragment::ANDOR, Vector(std::move(left), std::move(mid), std::move(right))); + constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::ANDOR, Vector(std::move(left), std::move(mid), std::move(right))); break; } case DecodeContext::THRESH_W: { @@ -1652,7 +1697,7 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) constructed.pop_back(); subs.push_back(std::move(sub)); } - constructed.push_back(MakeNodeRef(ctx, Fragment::THRESH, std::move(subs), k)); + constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::THRESH, std::move(subs), k)); break; } case DecodeContext::ENDIF: { @@ -1702,7 +1747,7 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) if (in >= last) return {}; if (in[0].first == OP_IF) { ++in; - BuildBack(ctx, Fragment::OR_I, constructed, /*reverse=*/true); + BuildBack(Fragment::OR_I, constructed, /*reverse=*/true); } else if (in[0].first == OP_NOTIF) { ++in; to_parse.emplace_back(DecodeContext::ANDOR, -1, -1); @@ -1717,6 +1762,7 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) } if (constructed.size() != 1) return {}; const NodeRef tl_node = std::move(constructed.front()); + tl_node->DuplicateKeyCheck(ctx); // Note that due to how ComputeType works (only assign the type to the node if the // subs' types are valid) this would fail if any node of tree is badly typed. if (!tl_node->IsValidTopLevel()) return {}; diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index 95e8476b77..9387c01e73 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -296,6 +296,9 @@ BOOST_AUTO_TEST_CASE(fixed_tests) // Same when the duplicates are on different levels in the tree const auto ms_dup4 = miniscript::FromString("thresh(2,pkh(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),s:pk(03fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),a:and_b(dv:older(1),s:pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65)))", CONVERTER); BOOST_CHECK(ms_dup4 && !ms_dup4->IsSane() && !ms_dup4->CheckDuplicateKey()); + // Sanity check the opposite is true, too. An otherwise sane Miniscript with no duplicate keys is sane. + const auto ms_nondup = miniscript::FromString("pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65)", CONVERTER); + BOOST_CHECK(ms_nondup && ms_nondup->CheckDuplicateKey() && ms_nondup->IsSane()); // Test we find the first insane sub closer to be a leaf node. This fragment is insane for two reasons: // 1. It can be spent without a signature // 2. It contains timelock mixes From e8cc2e4afc1142aa2b3da19cd5c17deea9963244 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 5 Jul 2022 10:28:18 +0200 Subject: [PATCH 2/2] Make miniscript string parsing account for exact script size as bound Co-Authored-by: Antoine Poinsot --- src/script/miniscript.h | 62 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 21e2959e94..ab25fa67b7 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -1030,6 +1030,18 @@ inline NodeRef Parse(Span in, const Ctx& ctx) { using namespace spanparsing; + // Account for the minimum script size for all parsed fragments so far. It "borrows" 1 + // script byte from all leaf nodes, counting it instead whenever a space for a recursive + // expression is added (through andor, and_*, or_*, thresh). This guarantees that all fragments + // increment the script_size by at least one, except for: + // - "0", "1": these leafs are only a single byte, so their subtracted-from increment is 0. + // This is not an issue however, as "space" for them has to be created by combinators, + // which do increment script_size. + // - "v:": the v wrapper adds nothing as in some cases it results in no opcode being added + // (instead transforming another opcode into its VERIFY form). However, the v: wrapper has + // to be interleaved with other fragments to be valid, so this is not a concern. + size_t script_size{1}; + // The two integers are used to hold state for thresh() std::vector> to_parse; std::vector> constructed; @@ -1037,14 +1049,16 @@ inline NodeRef Parse(Span in, const Ctx& ctx) to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1); while (!to_parse.empty()) { + if (script_size > MAX_STANDARD_P2WSH_SCRIPT_SIZE) return {}; + // Get the current context we are decoding within auto [cur_context, n, k] = to_parse.back(); to_parse.pop_back(); switch (cur_context) { case ParseContext::WRAPPED_EXPR: { - int colon_index = -1; - for (int i = 1; i < (int)in.size(); ++i) { + std::optional colon_index{}; + for (size_t i = 1; i < in.size(); ++i) { if (in[i] == ':') { colon_index = i; break; @@ -1052,35 +1066,50 @@ inline NodeRef Parse(Span in, const Ctx& ctx) if (in[i] < 'a' || in[i] > 'z') break; } // If there is no colon, this loop won't execute - for (int j = 0; j < colon_index; ++j) { + bool last_was_v{false}; + for (size_t j = 0; colon_index && j < *colon_index; ++j) { + if (script_size > MAX_STANDARD_P2WSH_SCRIPT_SIZE) return {}; if (in[j] == 'a') { + script_size += 2; to_parse.emplace_back(ParseContext::ALT, -1, -1); } else if (in[j] == 's') { + script_size += 1; to_parse.emplace_back(ParseContext::SWAP, -1, -1); } else if (in[j] == 'c') { + script_size += 1; to_parse.emplace_back(ParseContext::CHECK, -1, -1); } else if (in[j] == 'd') { + script_size += 3; to_parse.emplace_back(ParseContext::DUP_IF, -1, -1); } else if (in[j] == 'j') { + script_size += 4; to_parse.emplace_back(ParseContext::NON_ZERO, -1, -1); } else if (in[j] == 'n') { + script_size += 1; to_parse.emplace_back(ParseContext::ZERO_NOTEQUAL, -1, -1); } else if (in[j] == 'v') { + // do not permit "...vv...:"; it's not valid, and also doesn't trigger early + // failure as script_size isn't incremented. + if (last_was_v) return {}; to_parse.emplace_back(ParseContext::VERIFY, -1, -1); } else if (in[j] == 'u') { + script_size += 4; to_parse.emplace_back(ParseContext::WRAP_U, -1, -1); } else if (in[j] == 't') { + script_size += 1; to_parse.emplace_back(ParseContext::WRAP_T, -1, -1); } else if (in[j] == 'l') { // The l: wrapper is equivalent to or_i(0,X) + script_size += 4; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::JUST_0)); to_parse.emplace_back(ParseContext::OR_I, -1, -1); } else { return {}; } + last_was_v = (in[j] == 'v'); } to_parse.emplace_back(ParseContext::EXPR, -1, -1); - in = in.subspan(colon_index + 1); + if (colon_index) in = in.subspan(*colon_index + 1); break; } case ParseContext::EXPR: { @@ -1094,48 +1123,56 @@ inline NodeRef Parse(Span in, const Ctx& ctx) auto& [key, key_size] = *res; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_C, Vector(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_K, Vector(std::move(key)))))); in = in.subspan(key_size + 1); + script_size += 34; } else if (Const("pkh(", in)) { auto res = ParseKeyEnd(in, ctx); if (!res) return {}; auto& [key, key_size] = *res; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_C, Vector(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_H, Vector(std::move(key)))))); in = in.subspan(key_size + 1); + script_size += 24; } else if (Const("pk_k(", in)) { auto res = ParseKeyEnd(in, ctx); if (!res) return {}; auto& [key, key_size] = *res; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_K, Vector(std::move(key)))); in = in.subspan(key_size + 1); + script_size += 33; } else if (Const("pk_h(", in)) { auto res = ParseKeyEnd(in, ctx); if (!res) return {}; auto& [key, key_size] = *res; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::PK_H, Vector(std::move(key)))); in = in.subspan(key_size + 1); + script_size += 23; } else if (Const("sha256(", in)) { auto res = ParseHexStrEnd(in, 32, ctx); if (!res) return {}; auto& [hash, hash_size] = *res; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::SHA256, std::move(hash))); in = in.subspan(hash_size + 1); + script_size += 38; } else if (Const("ripemd160(", in)) { auto res = ParseHexStrEnd(in, 20, ctx); if (!res) return {}; auto& [hash, hash_size] = *res; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::RIPEMD160, std::move(hash))); in = in.subspan(hash_size + 1); + script_size += 26; } else if (Const("hash256(", in)) { auto res = ParseHexStrEnd(in, 32, ctx); if (!res) return {}; auto& [hash, hash_size] = *res; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::HASH256, std::move(hash))); in = in.subspan(hash_size + 1); + script_size += 38; } else if (Const("hash160(", in)) { auto res = ParseHexStrEnd(in, 20, ctx); if (!res) return {}; auto& [hash, hash_size] = *res; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::HASH160, std::move(hash))); in = in.subspan(hash_size + 1); + script_size += 26; } else if (Const("after(", in)) { int arg_size = FindNextChar(in, ')'); if (arg_size < 1) return {}; @@ -1144,6 +1181,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) if (num < 1 || num >= 0x80000000L) return {}; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, 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 {}; @@ -1152,6 +1190,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) if (num < 1 || num >= 0x80000000L) return {}; constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, 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)) { // Get threshold int next_comma = FindNextChar(in, ','); @@ -1171,6 +1210,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) } if (keys.size() < 1 || keys.size() > 20) return {}; if (k < 1 || k > (int64_t)keys.size()) return {}; + script_size += 2 + (keys.size() > 16) + (k > 16) + 34 * keys.size(); constructed.push_back(MakeNodeRef(internal::NoDupCheck{}, Fragment::MULTI, std::move(keys), k)); } else if (Const("thresh(", in)) { int next_comma = FindNextChar(in, ','); @@ -1181,6 +1221,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) // n = 1 here because we read the first WRAPPED_EXPR before reaching THRESH to_parse.emplace_back(ParseContext::THRESH, 1, k); to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1); + script_size += 2 + (k > 16); } else if (Const("andor(", in)) { to_parse.emplace_back(ParseContext::ANDOR, -1, -1); to_parse.emplace_back(ParseContext::CLOSE_BRACKET, -1, -1); @@ -1189,21 +1230,29 @@ inline NodeRef Parse(Span in, const Ctx& ctx) to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1); to_parse.emplace_back(ParseContext::COMMA, -1, -1); to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1); + script_size += 5; } else { if (Const("and_n(", in)) { to_parse.emplace_back(ParseContext::AND_N, -1, -1); + script_size += 5; } else if (Const("and_b(", in)) { to_parse.emplace_back(ParseContext::AND_B, -1, -1); + script_size += 2; } else if (Const("and_v(", in)) { to_parse.emplace_back(ParseContext::AND_V, -1, -1); + script_size += 1; } else if (Const("or_b(", in)) { to_parse.emplace_back(ParseContext::OR_B, -1, -1); + script_size += 2; } else if (Const("or_c(", in)) { to_parse.emplace_back(ParseContext::OR_C, -1, -1); + script_size += 3; } else if (Const("or_d(", in)) { to_parse.emplace_back(ParseContext::OR_D, -1, -1); + script_size += 4; } else if (Const("or_i(", in)) { to_parse.emplace_back(ParseContext::OR_I, -1, -1); + script_size += 4; } else { return {}; } @@ -1239,6 +1288,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) break; } case ParseContext::VERIFY: { + script_size += (constructed.back()->GetType() << "x"_mst); constructed.back() = MakeNodeRef(internal::NoDupCheck{}, Fragment::WRAP_V, Vector(std::move(constructed.back()))); break; } @@ -1294,6 +1344,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) in = in.subspan(1); to_parse.emplace_back(ParseContext::THRESH, n+1, k); to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1); + script_size += 2; } else if (in[0] == ')') { if (k > n) return {}; in = in.subspan(1); @@ -1325,6 +1376,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) // Sanity checks on the produced miniscript assert(constructed.size() == 1); + assert(constructed[0]->ScriptSize() == script_size); if (in.size() > 0) return {}; const NodeRef tl_node = std::move(constructed.front()); tl_node->DuplicateKeyCheck(ctx); @@ -1779,6 +1831,8 @@ inline NodeRef FromString(const std::string& str, const Ctx& template inline NodeRef FromScript(const CScript& script, const Ctx& ctx) { using namespace internal; + // A too large Script is necessarily invalid, don't bother parsing it. + if (script.size() > MAX_STANDARD_P2WSH_SCRIPT_SIZE) return {}; auto decomposed = DecomposeScript(script); if (!decomposed) return {}; auto it = decomposed->begin();