When are sanity_checks supposed to be run?

[apoelstra]

On the Miniscript type, which users are not really supposed to use directly, we run sanity checks (e.g. all branches are "safe") on from_str. You have to use from_str_ext or from_str_insane to override this.

However, we don't do the same for Descriptor::from_str, which simply parses a tree then calls Miniscript::from_tree, bypassing all sanity checks. So this seems backward -- by default for the users-shouldn't-use-this type we have sanity checks, while for descriptors, you have to manually call the sanity_check method.

However^2, for Taproot descriptors we do run the sanity checks on parsing, because we have bizarre special-purpose parsing logic which actually calls Miniscript::from_str for individual tapbranches rather than calling Miniscript::from_tree.

You can see this with the following unit test

commit 966524264e948b3e68f68a10eba513692f2839e6
Author: Andrew Poelstra <[email protected]>
Date:   Sat Aug 31 20:36:29 2024 +0000

    f unit test

diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs
index 0b0bd02b..72f533f7 100644
--- a/src/descriptor/mod.rs
+++ b/src/descriptor/mod.rs
@@ -2003,6 +2003,12 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))";
         Descriptor::<DescriptorPublicKey>::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1;2;3>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/<0;1;2>/*)))").unwrap_err();
     }

+    #[test]
+    fn taproot_s_check() {
+        Descriptor::<DescriptorPublicKey>::from_str("sh(or_i(pk(0202baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a66a),1))").unwrap();
+        Descriptor::<DescriptorPublicKey>::from_str("tr(02baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a66a,1)").unwrap_err();
+    }
+
     #[test]
     fn test_context_pks() {
         let comp_key = bitcoin::PublicKey::from_str(

We have the identical policy expressed as a sh (can parse no problem) or as a tr (will not parse, complains about the sigless `1( branch).

[apoelstra]

Related #653 -- we should not have sanity_check methods that you have to know/remember to call.

Related #723 -- we should drop the Ctx parameter, unify our sanity checking, and carry these checks along with objects.

[apoelstra]

Curiously, in the bare segwitv0 and sh modules we call Ctx::top_level_checks to check that the top-level expression is B. The Tr context does not even define this method. Instead the B check is done explicitly in Miniscript::from_str_ext (and also explicitly in Miniscript::parse_ext which duplicates a bunch of logic). Madness.