From c38c7c5817b7e73cf0f788855c4aba59c287b0ad Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 10 Dec 2021 14:50:02 +0100 Subject: [PATCH 1/5] miniscript: don't check for top level validity at parsing time Letting the caller perform the checks allows for finer-grained error reporting. --- src/script/miniscript.h | 10 ++++++---- src/test/miniscript_tests.cpp | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 2c239c26788..0137293180a 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -953,7 +953,11 @@ void BuildBack(const Ctx& ctx, Fragment nt, std::vector>& construct } } -//! Parse a miniscript from its textual descriptor form. +/** + * Parse a miniscript from its textual descriptor form. + * This does not check whether the script is valid, let alone sane. The caller is expected to use + * the `IsValidTopLevel()` and `IsSaneTopLevel()` to check for these properties on the node. + */ template inline NodeRef Parse(Span in, const Ctx& ctx) { @@ -1255,9 +1259,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) // Sanity checks on the produced miniscript assert(constructed.size() == 1); if (in.size() > 0) return {}; - const NodeRef tl_node = std::move(constructed.front()); - if (!tl_node->IsValidTopLevel()) return {}; - return tl_node; + return std::move(constructed.front()); } /** Decode a script into opcode/push pairs. diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index 3877fea907e..6a5bee1d1bd 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -276,7 +276,7 @@ BOOST_AUTO_TEST_CASE(fixed_tests) // (for now) have 'd:' be 'u'. This tests we can't use a 'd:' wrapper for a thresh, which requires // its subs to all be 'u' (taken from https://github.com/rust-bitcoin/rust-miniscript/discussions/341). const auto ms_minimalif = miniscript::FromString("thresh(3,c:pk_k(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),sc:pk_k(03fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),sc:pk_k(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798),sdv:older(32))", CONVERTER); - BOOST_CHECK(!ms_minimalif); + BOOST_CHECK(ms_minimalif && !ms_minimalif->IsValid()); // A Miniscript with duplicate keys is not sane const auto ms_dup1 = miniscript::FromString("and_v(v:pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65))", CONVERTER); BOOST_CHECK(ms_dup1); From d25d58bf5f301d3bb8683bd67c8847a4957d8e97 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 4 Oct 2021 00:08:00 +0200 Subject: [PATCH 2/5] miniscript: add a helper to find the first insane sub with no child This is helpful for finer grained descriptor parsing error: when there are multiple errors to report in a Miniscript descriptor start with the "smallest" fragments: the ones closer to be a leaf. Co-Authored-By: Pieter Wuille --- src/script/miniscript.h | 24 ++++++++++++++++++++++++ src/test/miniscript_tests.cpp | 13 +++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 0137293180a..f783d1dafcd 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -429,6 +429,21 @@ private: )); } + /** Like TreeEval, but without downfn or State type. + * upfn takes (const Node&, Span) and returns Result. */ + template + Result TreeEval(UpFn upfn) const + { + struct DummyState {}; + return std::move(*TreeEvalMaybe(DummyState{}, + [](DummyState, const Node&, size_t) { return DummyState{}; }, + [&upfn](DummyState, const Node& node, Span subs) { + Result res{upfn(node, subs)}; + return std::optional(std::move(res)); + } + )); + } + /** Compare two miniscript subtrees, using a non-recursive algorithm. */ friend int Compare(const Node& node1, const Node& node2) { @@ -818,6 +833,15 @@ public: //! Return the expression type. Type GetType() const { return typ; } + //! Find an insane subnode which has no insane children. Nullptr if there is none. + const Node* FindInsaneSub() const { + return TreeEval([](const Node& node, Span subs) -> const Node* { + for (auto& sub: subs) if (sub) return sub; + if (!node.IsSaneSubexpression()) return &node; + return nullptr; + }); + } + //! Check whether this node is valid at all. bool IsValid() const { return !(GetType() == ""_mst) && ScriptSize() <= MAX_STANDARD_P2WSH_SCRIPT_SIZE; } diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index 6a5bee1d1bd..bc5c49ef63f 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -111,6 +111,10 @@ struct KeyConverter { assert(it != g_testdata->pkmap.end()); return it->second; } + + std::optional ToString(const Key& key) const { + return HexStr(ToPKBytes(key)); + } }; //! Singleton instance of KeyConverter. @@ -290,6 +294,15 @@ 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()); + // 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 + // We'll report the timelock mix error, as it's "deeper" (closer to be a leaf node) than the "no 's' property" + // error is. + const auto ms_ins = miniscript::FromString("or_i(and_b(after(1),a:after(1000000000)),pk(03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204))", CONVERTER); + BOOST_CHECK(ms_ins && ms_ins->IsValid() && !ms_ins->IsSane()); + const auto insane_sub = ms_ins->FindInsaneSub(); + BOOST_CHECK(insane_sub && *insane_sub->ToString(CONVERTER) == "and_b(after(1),a:after(1000000000))"); // Timelock tests Test("after(100)", "?", TESTMODE_VALID | TESTMODE_NONMAL); // only heightlock From 4a082887bee76a96deada5dbd7f991c23b301c54 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 4 Oct 2021 00:18:08 +0200 Subject: [PATCH 3/5] qa: better error reporting on descriptor parsing error A nit, but was helpful when writing unit tests for Miniscript parsing --- src/test/descriptor_tests.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 63c86a896d5..f6b2a5bf360 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -141,14 +141,13 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string& } else { parse_priv = Parse(prv, keys_priv, error); } + BOOST_CHECK_MESSAGE(parse_priv, error); if (replace_apostrophe_with_h_in_pub) { parse_pub = Parse(UseHInsteadOfApostrophe(pub), keys_pub, error); } else { parse_pub = Parse(pub, keys_pub, error); } - - BOOST_CHECK(parse_priv); - BOOST_CHECK(parse_pub); + BOOST_CHECK_MESSAGE(parse_pub, error); // Check that the correct OutputType is inferred BOOST_CHECK(parse_priv->GetOutputType() == type); @@ -161,8 +160,8 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string& // Check that both versions serialize back to the public version. std::string pub1 = parse_priv->ToString(); std::string pub2 = parse_pub->ToString(); - BOOST_CHECK(EqualDescriptor(pub, pub1)); - BOOST_CHECK(EqualDescriptor(pub, pub2)); + BOOST_CHECK_MESSAGE(EqualDescriptor(pub, pub1), "Private ser: " + pub1 + " Public desc: " + pub); + BOOST_CHECK_MESSAGE(EqualDescriptor(pub, pub2), "Public ser: " + pub2 + " Public desc: " + pub); // Check that both can be serialized with private key back to the private version, but not without private key. if (!(flags & MISSING_PRIVKEYS)) { From bfb036756ad6e187fd6d3abfefe5804bb54a5c71 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 12 Apr 2022 21:18:37 +0200 Subject: [PATCH 4/5] Miniscript support in output descriptors Miniscript descriptors are defined under P2WSH context (either `wsh()` or `sh(wsh())`). Only sane Miniscripts are accepted, as insane ones (although valid by type) can have surprising behaviour with regard to malleability guarantees and resources limitations. As Miniscript descriptors are longer and more complex than "legacy" descriptors, care was taken in error reporting to help a user determine for what reason a provided Miniscript is insane. Co-authored-by: Pieter Wuille --- src/script/descriptor.cpp | 262 ++++++++++++++++++++++++++++++---- src/test/descriptor_tests.cpp | 23 +++ 2 files changed, 259 insertions(+), 26 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index ca0170c84bb..34a4da74f8b 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -6,6 +6,7 @@ #include #include +#include