--- Log opened Sat Sep 25 00:00:17 2021
04:48 < darosior> Shouldn't DecomposeScript enforce minimal push?
04:54 < darosior> The Rust implem would not accept to parse a Script with a non minimal push, and it seems more correct as it would break roundtrip Script -> Miniscript -> Script
05:34 <@sipa> darosior: ooh, i had no idea we didn't do that
05:34 < darosior> https://github.com/sipa/miniscript/pull/69
05:34 <@sipa> how about OP_n vs <n>, in the OP_CHECKMULTISIG argument?
05:35 <@sipa> for n>16 <n> has to be used
05:35 < darosior> I know we do that in the Rust implem, checking the C++ one now
05:35 <@sipa> thanks
05:37 < darosior> Finally got back to Miniscript, working on the fuzz target i mentioned previously for https://github.com/darosior/bitcoin/pull/2 . Do you have any insight of what it could check? It's a miniscript-from-script only since we already have parse_descriptor for the string representation
05:37 < darosior> (it does the roundtrip already which uncovered this issue)
05:38 <@sipa> once integrated into bitcoin core, with signing code, it could test that signing produces valid signatures against consesus code
05:40 < darosior> Cool, i'll add that after i get to complete the Miniscript integration on descriptor wallets (second item of my todo list)
05:40 < darosior> Re CMS num, i think we already check it. ParseScriptNumber gives fRequireMinimal to CScriptNum
05:41 <@sipa> this is great
06:08 < darosior> Ok this is not enough, we should CheckMinimalPush also for CHECKMULTISIG. Will push a commit to the PR
07:18 < darosior> So `vn:pk_k(02d7924d4f7d43ea965a465ae3095ff41131e5946f3c85f79e44adbcf8e27e080e)` does not roundtrip because https://github.com/sipa/miniscript/blob/200f5ef6cc5208dc54f05ab528c9d3426e0f6319/bitcoin/script/miniscript.h#L366 does a check on the type, but we don't do any check at parsing time
07:19 < darosior> How about we do some basic tests at parsing time? Like only the types at least to outlaw any really insane Miniscript
07:20 < darosior> Hmm or we can say that we only ensure roundtrip for sane miniscripts?
07:21 < darosior> Scratch this, actually it just uncovered a bug: WRAP_N should be "x"
07:23 < darosior> Interesting Bitcoin fact: 0NOTEQUAL and NUMNOTEQUAL are the two only "EQUAL-family" opcodes which don't have a -VERIFY version :)
07:34 <@sipa> at the very least, the top level result from DecodeScript must be B
07:35 <@sipa> maybe we should have separate IsValid and IsSafe and IsTopLevelValid and IsTopLevelSafe ?
07:36 <@sipa> because even things like opcode counts don't necessarily make a miniscript invalid... just some (possibly all) branches unusual
07:42 < darosior> Yep i can do that, the error wasn't even that WRAP_N was not x (it was, i overlooked it). Having something at least remotely sane after parsing would be nice
07:44 < darosior> The Rust implem has a parse_insane() and parse() version. Both have type checks and the parse() have safety checks (no sigless branch, not malleable, not over standardness limits, etc..). I think we could have something similar
07:47 < darosior> (as in having type checks mandatory and safety checks opt-in)
10:54 < darosior> Hmm actually parse_insane checks for consensus+policy validity.. I'm not sure we should do that in the C++ implem
11:01 <@sipa> i think IsTopLevelValid should reject anything that's badly typed (not B, or other type error), or categorically unspendable (e.g. script over 10000)
11:01 <@sipa> and those things should be rejected by the parser/decoder
11:06 <@sipa> some of miniscript's rules depend on standardness, btw
11:06 <@sipa> as in, the fragments/typing rules assume standardness
11:06 <@sipa> presumably some badly typed things can be consensus valid, but those should imho not be supported at all
11:17 <@sipa> so i think that for everything we can apply standardness rule, both for validity and safety
--- Log closed Sun Sep 26 00:00:18 2021