Open apoelstra opened 2 weeks ago
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.
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.
On the
Miniscript
type, which users are not really supposed to use directly, we run sanity checks (e.g. all branches are "safe") onfrom_str
. You have to usefrom_str_ext
orfrom_str_insane
to override this.However, we don't do the same for
Descriptor::from_str
, which simply parses a tree then callsMiniscript::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 thesanity_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 callingMiniscript::from_tree
.You can see this with the following unit test
We have the identical policy expressed as a
sh
(can parse no problem) or as atr
(will not parse, complains about the sigless `1( branch).