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 <pieter@wuille.net>

2 files changed, 37 insertions, 0 deletions

diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 0137293180..f783d1dafc 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<Result>) and returns Result. */
+    template<typename Result, typename UpFn>
+    Result TreeEval(UpFn upfn) const
+    {
+        struct DummyState {};
+        return std::move(*TreeEvalMaybe<Result>(DummyState{},
+            [](DummyState, const Node&, size_t) { return DummyState{}; },
+            [&upfn](DummyState, const Node& node, Span<Result> subs) {
+                Result res{upfn(node, subs)};
+                return std::optional<Result>(std::move(res));
+            }
+        ));
+    }
+
     /** Compare two miniscript subtrees, using a non-recursive algorithm. */
     friend int Compare(const Node<Key>& node1, const Node<Key>& 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*>([](const Node& node, Span<const Node*> 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 6a5bee1d1b..bc5c49ef63 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<std::string> 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