--- Day changed Wed Oct 07 2020
01:56 < darosior> sanket1729: are you around ? I have some questions as i dig more in the intrinsics and would like to discuss the approach of https://github.com/rust-bitcoin/rust-miniscript/pull/148
01:56 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 240 seconds]     
01:56 < sanket1729> sure
01:57 < darosior> What do you think about making insert_elemen_*() return Results ?
01:58 < darosior> We could this way test both the new MaxStackElemExceeded and the already existing MaxOpCountExceeded
01:59 < sanket1729> thinking about it
02:00 < sanket1729> Recapping the current algorithm for completeness of discussion
02:01 < darosior> Sure, will cherry-pick my commit adding the stack_elem_size to the ExtraData struct. Why do we need the dissat size ? We don't check it currently for the OP count afaik
02:06 < sanket1729> For every fragment, we try all the possible wrappers to obtain a map of best typed node for the policy node. The insert_elem tries to insert one particular element into the map that is called by insert_elem_closure
02:08 < sanket1729> It is possible that insert_elem_closure succeeds for some types(i.e types in miniscripts), but fails(due to opcodes/stack size) reasons for others. 
02:11 < sanket1729> The types may in turn fail in future because they are malleable. 
02:12 < sanket1729> okay, this sounds confusing. let me try again
02:16 < sanket1729> * taking I said everything back. I think returning results in possible. But we would have to maintain the results along the way while taking in closures etc. 
02:17 < sanket1729> So, we need return a tuple, (compilations, Errors) instead of Results. 
02:20 < sanket1729> for example, a compilation for or(A,B) may have some compilations that exceed op_count(for ex. type `Bun`) and some valid compilations for type (`Vz`). 
02:24 < darosior> Hmm     
02:25 < darosior> Just to be clear could you explicit Bun and Vz
02:25 < darosior> I understand they are fragment types but still
02:29 < sanket1729> those were random types I suggested. There is nothing specific about Bun or Vz. 
02:31 -!- jonatack [~jon@213.152.162.94] has joined ##miniscript     
02:31 < sanket1729> The compiler does brute force at every node. It maybe possible that it discarded some combinations of children because those had high opcount. 
02:32 < sanket1729> But all the compilations the parent node has are say malleable, which are also discarded later. 
02:32 < sanket1729> So, we need information from children as to whether some of their compilations were discarded or not. 
02:34 < sanket1729> And I think that is difficult code-wise to maintain and propagate. 
02:37 < sanket1729> As a example, if all computations of type W were discarded in children, we cannot use the thresh Terminal because the typing rules require children to be W.
02:39 < sanket1729> The current compilation map stores types -> best_compilation. That would have to changed to type -> (best_compilation, error_type)
02:41 < sanket1729> type -> Result<best_compilation, Error> and maintain that all along. 
02:41 < darosior> I see. Thank you
02:42 < darosior> Think i'll go without Result for now, can still be implemented in a follow-up
02:42 < sanket1729> I think that change will leak everywhere.
02:42 < sanket1729> And probably complicate some logic. 
02:43 < darosior> Now, why do you think we need the dissat cost in stack elements for each fragment ?
02:43 < sanket1729> So, for satisfaction of some miniscript fragments it is also necessary to dissat some
02:43 < sanket1729> for example, thresh
02:43 < sanket1729> To satisfy, k , you need dissat n -k 
02:44 < sanket1729> or `or_d`
02:45 < sanket1729> That second satisfaction of `or_d` for example
02:45 < sanket1729> You need to put sat(Z) dsat(X) on stack
02:46 < sanket1729> or_d(X,Z) = 	[X] IFDUP NOTIF [Z] ENDIF
02:46 < sanket1729> So, you also need to calculate the max dissat stack size of x in order to calculate the max sat size of or_d
02:49 < darosior> Hmm so we should check the dissat cost in OP codes as well ?
02:50 < sanket1729> dissat cost of op_codes?
02:50 < sanket1729> in op_codes you mean
02:50 < sanket1729> I think we already do
02:51 < sanket1729> There is `ops_count_static`, `ops_count_nsat` and `ops_count_sat`
02:51 < sanket1729> in ExtData
02:53 < sanket1729> darosior: Here is sipa's c++ code for the same
02:53 < sanket1729> https://github.com/sipa/miniscript/blob/027b422495a8ee3ea3fa7787150389bb65d1b9de/bitcoin/script/miniscript.h#L529-L569
02:59 < darosior> Yeah but we don't use them
02:59 < darosior> static is completely unused
02:59 < darosior> grep -rI ops_count_static src/     => We set it but never branch on it
03:00 < darosior> And we don't use the nsat in the compiler
03:00 < darosior> Thanks i'll cross check my values with the C++ code
04:00 < sanket1729> darosior: It is used internally in the computation of sat values.
04:03 < sanket1729> ops_count_sat is used in compiler.rs line 660. 
04:03 < sanket1729> And ops_count_sat for parent depends on ops_count_nsat for children. And that is why we need to compute it for all children
04:03 < sanket1729> s/children/nodes
04:05 < sanket1729> As an slightly unrelated issue, I think we should also be doing the ops count check whenever we get from string, or parse it from code. 
04:19 -!- shesek [~shesek@164.90.217.137] has joined ##miniscript     
04:19 -!- shesek [~shesek@164.90.217.137] has quit [Changing host]     
04:19 -!- shesek [~shesek@unaffiliated/shesek] has joined ##miniscript     
04:21 < darosior> sanket1729: re check_resource_limits(..), that's kind of what check_frag_validity() is ?
04:21 < darosior> We could move the op count check here as well
04:21 < sanket1729> yeah, we should.
04:22 < darosior> And it'd be called at parsing if i'm not mistaking
04:23 < sanket1729> I think check_frag_validity is already called at all places where we create miniscripts
04:27 < sanket1729> darosior: I think you are assuming that fragments can be dissatisfied with only 1 stack size.
04:27 < darosior> In the PR ?
04:27 < darosior> Did not implement the dissat cost yet
04:27 < sanket1729> yeah
04:27 < sanket1729> Ah     
04:27 < darosior> On it rn
04:28 < darosior> Thinking that we'll need size in ""bytes"" as well..
04:28 < darosior> In addition to number of stack elements
04:29 < sanket1729> why? Is there some other resource constraint rule?
04:29 < darosior> For https://github.com/bitcoin/bitcoin/blob/283a73d7eaea2907a6f7f800f529a0d6db53d7a6/src/policy/policy.h#L43-L44
04:29 < darosior> (Witness script size)
04:30 < sanket1729> yes     
04:31 < sanket1729> fortunately, this will be easier as this has nothing to do with satisfaction. 
04:32 < darosior> Hmm how could the wit script size be exceed without blowing out the stack elements limit * max stack element size
04:34 < sanket1729> witness_script size has nothing to with stack elements size/number of stack elements?
04:34 < sanket1729> take a 1 of 120 multisig. There is only 1 witness
04:35 < sanket1729> sorry, I mean or (1 of 120)
04:35 < darosior> Yeah but 119 empty byte arrays ?
04:35 < sanket1729> there would log(n) ors.
04:35 < sanket1729> log(n) dissats
04:35 < darosior> "witness_script size has nothing to with stack elements size/number of stack elements?" => No but it influences it
04:36 < sanket1729> one can be pedantic and keep on wrapping the miniscript
04:36 < sanket1729> with wrappers to increase the size
04:37 < darosior> Ok got one (slightly nonsensical) => 99 * sha256(). This requires 99 stack elems to be satisfied (of 32 bytes each) so are valid, but the witness script size is (5 + 32) * 99 (since it's  "SIZE <32> EQUALVERIFY SHA256 <h> EQUAL")
04:37 < darosior> >>> (5 + 32) * 99
04:37 < darosior> 3663
04:38 < darosior> Maybe even (6 + 32) as i think the "32" element needs a PUSHDATA ?
04:39 < sanket1729> l (vt)*:M where M is a valid miniscript is also a miniscript
04:39 < sanket1729> t is basically and_v(M, 1)
04:40 < sanket1729> v is verify
04:40 < sanket1729> it's like adding and popping 1 again and again
04:41 < darosior> Ah yeah, nice one :)
04:44 -!- jonatack [~jon@213.152.162.94] has quit [Ping timeout: 246 seconds]     
04:59 < darosior> sanket1729: by the way didn't you and andytoshi planned to do an MSRV bump release ?
05:33 -!- martindale [ericfabric@gateway/shell/matrix.org/x-pjuyuxqsnaoarwte] has quit [Quit: killed]     
05:41 -!- martindale [ericfabric@gateway/shell/matrix.org/x-adwvcsrhggcunnme] has joined ##miniscript     
06:04 -!- Netsplit *.net <-> *.split quits: martindale, kallewoof
06:05 -!- martindale [ericfabric@gateway/shell/matrix.org/x-inzvrljutvskskgo] has joined ##miniscript     
06:09 -!- kallewoof [~quassel@240d:1a:759:6000:a7b1:451a:8874:e1ac] has joined ##miniscript     
06:38 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined ##miniscript     
07:04 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection]     
07:43 < andytoshi> darosior: yes, lol sorry, i wanted to do this so we could get https://github.com/rust-bitcoin/rust-bitcoin/pull/387 ... but actually we don't yet have a rust-bitcoin release with that PR in it)
07:44 < andytoshi> let me see about doing a rust-bitcoin minor release
09:10 < darosior> Thanks, ok i thought it was about bumping without any new feature like you did for rust-bitcoin
09:12 < darosior> FWIW sanket1729 i think i'm going to split the PR between a) checking the witness items limits + moving the op_count check to Segwitv0 context (which uncovered some panics on dissatisfaction cost computation) and b) moving the Script cost computations to be non-recursive
11:21 < jeremyrubin> andytoshi: yeah we should pass though any flags we can
11:21 < jeremyrubin> The issue is that AFAICT secp256k1 does not like being linked more than once
11:22 < jeremyrubin> Could have been some other issue?
11:22 < jeremyrubin> But in any case, in order to ensure all of the types are consisntent (e.g., a miniscript::bitcoin::publickey is a bitcoin::publickey)
11:23 < jeremyrubin> it's best not to have to import separately for different features
11:28 < andytoshi> darosior: the goal is to bump without any breaking changse
11:28 < andytoshi> aside from updating deps
11:28 < andytoshi> but these cleanups aren't breaking changes (they should be invisible)
11:28 < andytoshi> jeremyrubin: secp256k1 should be fine being linked more than onec
11:29 < andytoshi> but yes, you're right that it's super annoying to add an explicit secp dep to get features
11:29 < andytoshi> because if the versions don't match, it will indeed compile two versions and then you'll get type compatibility errors
11:29 < jeremyrubin> https://github.com/rust-bitcoin/rust-miniscript/issues/146
11:30 < jeremyrubin> could have been something else incorrect but at least for me I couldn't make it happy
11:34 < andytoshi> innteresting
11:35 < andytoshi> "undefined reference" does not sound like a "too many version of libsecp" issue
11:36 < andytoshi> will try to reproduce locally
11:38 < jeremyrubin> I don't recall what my config flags were, but it was something relatively basic
11:42 < andytoshi> yeah, there shouldn't be any combination of config flags that'll trigger that
11:42 < andytoshi> i'm worried that we've published a version of rust-secp that doesn't build, somehow
14:14 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection]     
14:14 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript     
16:07 -!- shesek [~shesek@164.90.217.137] has joined ##miniscript     
16:07 -!- shesek [~shesek@164.90.217.137] has quit [Changing host]     
16:07 -!- shesek [~shesek@unaffiliated/shesek] has joined ##miniscript     
23:35 -!- martindale [ericfabric@gateway/shell/matrix.org/x-inzvrljutvskskgo] has quit [Quit: killed]     
23:42 -!- martindale [ericfabric@gateway/shell/matrix.org/x-pbpuumytgpjwpbjr] has joined ##miniscript