miniscript: don't check for top level validity at parsing time

darosior · darosior · commit c38c7c5817b7 · 2022-07-14T12:03:49.000+02:00
Letting the caller perform the checks allows for finer-grained error
reporting.
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
@@ -953,7 +953,11 @@ void BuildBack(const Ctx& ctx, Fragment nt, std::vector<NodeRef<Key>>& 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<typename Key, typename Ctx>
 inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
 {
@@ -1255,9 +1259,7 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
     // Sanity checks on the produced miniscript
     assert(constructed.size() == 1);
     if (in.size() > 0) return {};
-    const NodeRef<Key> 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
@@ -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);

Original file line number	Diff line number	Diff line change
`@@ -953,7 +953,11 @@ void BuildBack(const Ctx& ctx, Fragment nt, std::vector<NodeRef<Key>>& construct`
`953`	`953`	`}`
`954`	`954`	`}`
`955`	`955`
`956`		`-//! Parse a miniscript from its textual descriptor form.`
	`956`	`+/**`
	`957`	`+ * Parse a miniscript from its textual descriptor form.`
	`958`	`+ * This does not check whether the script is valid, let alone sane. The caller is expected to use`
	`959`	+ * the `IsValidTopLevel()` and `IsSaneTopLevel()` to check for these properties on the node.
	`960`	`+ */`
`957`	`961`	`template<typename Key, typename Ctx>`
`958`	`962`	`inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)`
`959`	`963`	`{`
`@@ -1255,9 +1259,7 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)`
`1255`	`1259`	`// Sanity checks on the produced miniscript`
`1256`	`1260`	`assert(constructed.size() == 1);`
`1257`	`1261`	`if (in.size() > 0) return {};`
`1258`		`- const NodeRef<Key> tl_node = std::move(constructed.front());`
`1259`		`- if (!tl_node->IsValidTopLevel()) return {};`
`1260`		`- return tl_node;`
	`1262`	`+ return std::move(constructed.front());`
`1261`	`1263`	`}`
`1262`	`1264`
`1263`	`1265`	`/** Decode a script into opcode/push pairs.`