--- Day changed Thu Jun 18 2020
02:45 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has joined ##miniscript     
02:58 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]     
03:05 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has joined ##miniscript     
03:26 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]     
03:49 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has joined ##miniscript     
05:22 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 272 seconds]     
05:24 -!- jonatack [~jon@184.75.221.35] has joined ##miniscript     
07:56 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]     
08:02 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has joined ##miniscript     
11:17 -!- jonatack [~jon@184.75.221.35] has quit [Ping timeout: 258 seconds]     
11:19 -!- jonatack [~jon@37.170.249.11] has joined ##miniscript     
13:48 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Quit: ZNC 1.7.5 - https://znc.in]     
13:49 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined ##miniscript     
15:34 < sanket1729> andytoshi: #100, #99, #98 and #97 are up for review. #100 should fix all the fuzz crashes because of aliasing. 
15:38 < sanket1729> I am also looking at old issue by practicalswift about limiting recursion depth to Miniscript. Any ideas about fixing that? I am thinking we could apply some hueristic for limiting miniscript input size. 
15:38 < sipa> in the parser?
15:40 < sanket1729> Something like error out if the script is more than 10000 bytes when decoding(script->ms), and more than 100,000 len string(str->ms). and limit the policy string length(policy->ms by compiler). 
15:40 < sipa> https://github.com/sipa/miniscript/pull/5
15:40 < sipa> there is some discussion there
15:41 < sanket1729> Makes sense. We can implement a similar solution. 
15:46 < sanket1729> I don't like the explcitly counting the depth, but I guess there is no elegant solution to this.  
17:06 < andytoshi> sanket1729: re limiting recursion depth, i think we need to just replace every single recursive algorithm
17:06 < andytoshi> is there anywhere where you think that'd be too onerous/
17:07 < sipa> so write a real parser? :p
17:07 < andytoshi> well, we did that, the parser is not recursive
17:07 < andytoshi> but many many things are :P
17:07 < sipa> ah, nice
17:07 < sipa> yeah
17:08 < andytoshi> it's tempting to just limit the depth, everything in miniscript has a natural recursive solutino
17:08 < andytoshi> but if we're limited to 402 ... we can still blow our stack in a lot of cases
17:08 < sipa> yeah
17:08 < sipa> and with taproot no such natural recursion limit exists
17:08 < sipa> though we could define there to be one
17:08 < andytoshi> ah, kk, that's good to know
17:08 < andytoshi> nah, i think we should just fix rust-miniscript to not use recursion
17:08 < andytoshi> even though that's gonna be a giant PITA
17:09 < sipa> i expect that will be a pain :(
17:09 < andytoshi> yeah :/ in many cases we recurse multiple times, so you have to emulate an instruction pointer etc
17:09 < andytoshi> i had the same issue with simplicity actually recently
17:10 < andytoshi> i had a recursive parser then i ran a set of test vectors gmax came up with, and ofc one blew my stack :)
17:10 < andytoshi> recursive execution engine*
17:10 < sipa> yeah
17:11 < andytoshi> i wish there were a general solution to this. like a way to annotate functions "use the heap for local vars" or something
17:15 < sipa> just limit miniscript to 32 levels
17:15 < sipa> ;)     
17:15 < sipa> ought to be enough for everyone
17:16 < andytoshi> hahah
17:29 < andytoshi> sanket1729: waiting for 100 to finish, then maybe you need to kick the other jobs
17:29 < andytoshi> i'm hesitant to review anything with a red X on it :)
17:29 < andytoshi> s/kick the other jobs/rebase the other PRs/
17:36 < sanket1729> andytoshi, #100 actually passed
17:36 < sanket1729> I don't know why travis shows yellow
17:36 < sanket1729> The second build is not running
17:38 < andytoshi> sigh
17:38 < andytoshi> ok, i'll poke at it
17:38 < sanket1729> https://travis-ci.com/github/apoelstra/rust-miniscript/builds/172130948
17:38 < andytoshi> yep, i see
17:39 < andytoshi> eh whatever i'll just force merge it
17:39 < sanket1729> andytoshi, we only need change recursive algorithms for parsing from strings, right?
17:39 < sanket1729> I made an attempt in #101
17:39 < andytoshi> merged 100
17:40 < andytoshi> sanket1729: no, i think all of them (even stuff like estimating sizes)
17:40 < andytoshi> if you can get a deep enough miniscript i think most of the methods on Miniscript can overflow the stack
17:40 < sanket1729> There must be some way users puts in Miniscript into rust-miniscript
17:40 < sanket1729> If we guard the entry points, we should be safe?
17:41 < sanket1729> from string, from policy compiler and from decoding script
17:41 < andytoshi> sanket1729: ah, no, so i don't think we should be guarding the entry points like this
17:41 < andytoshi> firstly i don't think limiting to 402 is sufficient to prevent stack overflow in all cases
17:41 < andytoshi> secondly i don't want to limit to 402, because this won't work for taproot (and maybe won't work for simplicity or elements or whatever)
17:42 < sipa> i mean, right now it _is_ limited to 402 whether you make that explicit or not
17:42 < sipa> no miniscript with more levels can typecheck
17:42 < andytoshi> oh, that's a good observation
17:42 < sipa> (or pass other sanity checks, based on how it's implemented)
17:42 < sipa> that may not be the case for future extensions of the language
17:43 < andytoshi> i wonder what happens if you make a policy with nesting depth >402 and try to compile
17:43 < andytoshi> i guess it'll fail
17:44 < sipa> i think all policy elements have some non-zero bound on how many miniscript elements they translate to
17:44 < andytoshi> sipa: so, one thing sanket1729 and i are doing pretty soon is writing a simplicity target for the miniscript compiler, and a parallel "miniscript" which is actually simplicity rather than Script
17:45 < andytoshi> and importantly, this mini-simplicity should be able to lift back to the policy language (minus probabilities i guess)
17:45 < andytoshi> so users can do comparisons between Script and Simplicity things
17:45 < andytoshi> so i'm a bit uncomfortable with Script-specific limits
17:45 < sipa> right
17:45 < andytoshi> becuase already they won't apply to simplicity
17:46 < sipa> yeah that makes sense
17:46 < sipa> it's also nice future proofing
17:46 < andytoshi> though it is an interesting question, what happens if you make a simplicity rpogram "equivalent to" a script which exceeds consensus limits
17:46 < andytoshi> i wonder if there is a potential vulnerability in some protocol because of this
17:47 < sipa> but mini-simplicity shouldn't need to be identical to miniscript, i think?
17:47 < sipa> as in: it could do with far fewer fragments
17:47 < sipa> no point in having dozens of ands and ors
17:47 < andytoshi> yeah, i'm pretty sure it'll have fewer fragments
17:47 < andytoshi> though how many fewer, i honestly don't know
17:47 < sipa> so it's a different language
17:47 < andytoshi> it's plausible that it'd only need one and, one or, etc
17:47 < sipa> and a compiler can target that different language
17:47 < sipa> you'll probably want to share some code... but miniscript exists in order to be able to analyze bitcoin script
17:48 < andytoshi> yeah, all true ... but i'd like them both to "lift" to the same absrtact policy language
17:48 < sipa> that absolutely makes sense
17:48 < andytoshi> but i get what you're saying ... mini-simplicity and mini-script will share very little, so limits in miniscript won't affect mini-simplicity
17:48 < sipa> right
17:49 < sipa> and i like your desire to not have any hardcoded limits, especially if we expect them to change with tapscript
17:49 < sipa> but at the same time... i think that making all algorithms non-recursive will be huge pain
17:49 < andytoshi> yeah, agreed
17:50 < andytoshi> so maybe for now we should just do the 402-limit..
17:50 < andytoshi> i feel like making things non-recursive is a fairly low priority ... but it would be nice in the meantime if we didn't have crashes
17:51 < sanket1729> andytoshi: can you quickly merge #102 :P .I kindof messed up keeping track of different branches :P . 
17:51 < andytoshi> lol sure one sec
17:51 < andytoshi> sorry, i should've flagged this
17:51 < andytoshi> i have a policy of "whenever people try to commit artifacts from IDEs i don't use i'll just let them"
17:52 < andytoshi> but that's probably a bad policy to have
17:53 < sipa> in bitcoin core they're all rejected (and there is a .gitignore file listing the most common ones)
17:53 < sipa> so people can have their own locally
17:53 < andytoshi> yeah i think that's reasonable, to allow additions to .gitignore but not allow actual files
18:09 < sanket1729> sigh, there are more fuzz fixes.. In roundtrips we allow upper case and lower case hex, but while outputting we output lower case hex
18:09 < sanket1729> I am amazed by this fuzzer
18:10 < andytoshi> i'm amazed that we didn't hit that much earlier
18:10 < andytoshi> i had no idea we allowed upper case hex
18:12 < sanket1729> should we restrict to lower case hex? That seems to originate from bitcoin_hashes
18:12 < andytoshi> ah, bitcoin_hashes allows upper case hex?
18:12 < andytoshi> yeah i think we should restrict to lowercase. sipa thoughts?
18:15 < sipa> i think descriptors allow lower and upper case
18:15 < sipa> let me check
18:16 < andytoshi> ok. it's not a big deal to normalize in the fuzztests to lowercase
18:16 < andytoshi> much easier than removing all the 1@s :)
18:17 < sanket1729> Yeah, but it also runs into tricky corner cases for other string types or(pk(a),pk(A)) could would map to or(pk(a),pk(a))
18:18 < andytoshi> ah yeah, good point
18:18 < sanket1729> I guess we safely ignore those cases.
18:19 < andytoshi> i mean, it's probably ok for the purpose of fuzztesting
18:19 < andytoshi> i don't think we should change our actual logic (well, if pieter comes back and says "descriptors are lowercase only" then i think we should enforce that, it'll make our lives a bit easier)
18:26 < sipa> yeah, mixed case is supported
18:27 < sipa> in public keys at least
18:27 < sipa> in xpubs case is case-sensitive of course (because it's base58)
18:29 < andytoshi> how about hashes
18:29 < sipa> descriptors never contain hashes, afaik
18:30 < andytoshi> ah right, that's a miniscript thing
18:30 < andytoshi> well, for consistency i guess they ought to support mixed case, whenever they're introduced
18:31 < andytoshi> so, sanket1729 for roundtrip fuzzetst purposes i think we should just normalize everything to lowercase
18:32 < andytoshi> you are correct that this will hide case-changing bugs in theory, but i guess we'll just live with that
18:32 < sanket1729> andytoshi, yes I working on it meanwhile. raised #013. Let's see if fuzzer finds more  crashes
18:32 < sanket1729> #103
18:33 < sanket1729> are we updating the fuzzer crates automatically?
18:33 < sanket1729> I don't recall these many crashes last time. Maybe they improved some fuzzer logic
18:36 < andytoshi> what do you mean updating the fuzzer crates automatically?
18:37 < andytoshi> ah i see
18:37 < andytoshi> so, i actually think not ... i seem to recall every new version of honggfuzz requiring we update cargo.toml
18:37 < andytoshi> maybe this is only in liquid where we pin/vendor everything
18:37 < andytoshi> you may be right .. if there was some recent update that might explain all this
18:38 < andytoshi> ok, waiting on CI for 103
18:39 < sanket1729> we have version=0.5 in Cargo.toml, which means it probably does not 0.6 or above if my understanding of versioning is correct. 
18:39 < sanket1729> But it can go to 0.5.999 and so on
18:39 < andytoshi> yep, that's correct
18:41 < sanket1729> okay, this is a tricky situation to make both PRs pass. PkH PR fixes the previous bug which is causing #103 to crash
18:41 < sanket1729> and PKh Pr wont' pass without 103
18:41 < andytoshi> lol     
18:41 < andytoshi> can you just combine them into one?
18:41 < andytoshi> you can use `git rebase` or `git cherry-pick`
18:41 < sanket1729> Yes, I will do that
18:42 < andytoshi> kool
18:45 < andytoshi> ok, waiting on 97
18:45 < andytoshi> 98 i mean
18:53 < sanket1729> #98 is passing
18:53 < andytoshi> awesome :)
18:54 < andytoshi> let me do another once-over these
18:55 < andytoshi> looks good. i actually hadn't seen this one, but it's pretty similar to the c:pk alias
18:55 < andytoshi> thanks a ton for getting this done
19:04 < sanket1729> sure, happy CI should help in future faster development too. I have also 97 which has some non-trivial changes and may require iterations
19:34 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Quit: ZNC 1.7.5 - https://znc.in]     
19:36 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined ##miniscript     
23:14 -!- jonatack_ [~jon@37.166.220.215] has joined ##miniscript     
23:18 -!- jonatack [~jon@37.170.249.11] has quit [Ping timeout: 264 seconds]