--- Log opened Thu Dec 10 00:00:45 2020 01:03 -!- jonatack [~jon@109.232.227.138] has quit [Remote host closed the connection] 01:04 -!- jonatack [~jon@109.232.227.138] has joined ##miniscript 01:55 < darosior> Agree that it's weird: for example if it's set for HTLC transactions in Lightning (signed with ACP | SINGLE by your peer) you would not be able to attach your ALL sig with a spec-compliant PSBT finalizer 02:35 < darosior> We had a regression with the bounds checks.... I could just compile succesfully a 170-of-170 multisig.. 02:36 < darosior> So that's a bug in the max_witness_elements computation, it's "73" for 170 pubkeys .. 02:36 < darosior> Oh and 3 for 100 lol 02:37 < darosior> At 99 it comes back to 100 03:02 < darosior> Bisected to 60edd01374651c54e973db072c8436cc8a51271b 03:56 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 03:56 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined ##miniscript 03:56 < darosior> sanket1729: do you have an input on the above ? Been debugging for some time but can't make sense out of it. Starting from https://github.com/rust-bitcoin/rust-miniscript/commit/60edd01374651c54e973db072c8436cc8a51271b satisfaction cost tests are voided as it seems to restart at 0 once it reaches the limit.... 03:56 < darosior> Can provide a testcase 03:57 < darosior> Actually i should upstream all my limitations testing to avoid this.. 03:58 < sanket1729> > So that's a bug in the max_witness_elements computation, it's "73" for 170 pubkeys .. 03:59 < sanket1729> huh interesting 04:03 < sanket1729> Which ScriptContext are you are operating in? 04:06 < darosior> Segwit 04:06 < darosior> Working on a test case for you to reproduce 04:07 < darosior> I don't get how this commit broke it. First thought it was because of check_frag_validity() removal, but it's not 04:08 < darosior> afaict 04:08 < sanket1729> this commit does rearrange some thing 04:19 < sanket1729> I am confused, how could you compile a 170 of 170. Won't you need a initial stack of atleast 170 elements? 04:19 < sanket1729> Ah. you were not able to compile that before and now you are able to 04:20 < sanket1729> darosior, I can reproduce it locally 04:20 < sanket1729> looking into debuggin it 04:22 < darosior> Yeah, my "bisect good" is eb5e04b025008da0844f96741a0b16e1fd60d93d 04:22 < darosior> I'm doing a new bisect with my new testcase.. 04:28 < darosior> Ok, still bisected to 60edd01374651c54e973db072c8436cc8a51271b 04:28 < sanket1729> figured it out. This is indeed a bug from that commit 04:28 < darosior> https://github.com/darosior/rust-miniscript/commit/f817405a92cd8b388fd0d437d108beec41b2542b 04:28 < darosior> Oh what was it ? 04:28 < sanket1729> check_local_policy_validity() returns Ok() when max_satisfaction_witness_elements is None 04:28 < sanket1729> It should return Err 04:28 < darosior> Arrrr 04:28 < darosior> It slipped through my eyes :'( 04:30 < sanket1729> I will need to re-review some of other things. 04:30 < sanket1729> We changed our APIs to return None and documented it 04:30 < sanket1729> But we did not update our internal code to reflect it 04:30 < darosior> Ok. Do you want me to make a commit or are you on it? Otherwise you can cherry-pick my test but i'll add more later 04:31 < sanket1729> Do you more tests that test resource limits? 04:33 < darosior> Yeah, i have a bunch in my lib: https://github.com/re-vault/revault_tx/blob/ef87fe9182ebda0047001e839beec011cbde1aa3/src/scripts.rs#L453-L631 04:33 < darosior> Will upstream them :) 04:33 < sanket1729> Awesome :) 04:34 < sanket1729> So, all the APIs have that bug :O 04:34 < sanket1729> if let Some(x) = ms.func() 04:34 < sanket1729> if func() reutrns None, our check succeeds! 04:35 < sanket1729> I will add a commit fixing all of those 04:39 < sanket1729> note to self: This would not have happened if we used Result of Option. 04:40 < darosior> Right 04:40 * darosior grabs the note for himself too 05:06 < sanket1729> reviewed the codebase for all "if let" short circuits :) and pushed a new commit fixing all incorrect ones(there were 4!) 05:13 < darosior> sanket1729: Thanks, still working on the tests and unfortunately.. Your commit does not fix all the bugs 05:15 < sanket1729> woah! 05:17 < darosior> https://github.com/darosior/rust-miniscript/commit/2bc9af1d0dc39f65c62ed8bc16ae9d6c6ec9caa3 05:17 < sanket1729> right 05:17 < darosior> Oh my comment is wrong 05:18 < sanket1729> looking into the test 05:18 < darosior> Actually the number of elements computation was even wrong in https://github.com/darosior/rust-miniscript/commit/eb5e04b025008da0844f96741a0b16e1fd60d93d 05:19 < darosior> Compilation succeeded with a witscript size of 'Ok(3508)' and 'Ok(Some(3))' elements', src/policy/compiler.rs:1485:9 05:20 < sanket1729> yikes! I thought the previous commit fixed this. 05:20 < darosior> It fixed the max size though 05:23 < sanket1729> it is using and_n for k=100 05:23 < sanket1729> probably some bug in and_or 05:51 < sanket1729> fixed it. Some bug in andor where we swapped out b and c 05:51 < sanket1729> it's meant to (a and b) or c 05:53 < darosior> Nice :D 05:53 < darosior> So we have the test case for this one, but i'm still struggling to produce a Miniscript where the witscript size > 3600 but number of sat wit elements is < 100 06:00 < darosior> Got one :tada: thresh(49, vec![pubkey; 98]) 06:01 < sanket1729> it may exceed opcodes :P 06:01 < darosior> 😭 06:04 < sanket1729> I think it is possible to it with thresh(2, thresh(75, vec![p;, 75]), thresh(75, vec![pk, 75])) 06:04 < sanket1729> or(thresh(75, vec![p;, 75]), thresh(75, vec![pk, 75])) 06:05 < sanket1729> The idea is thresh(75, vec![pk; 75]) to be chain of and_v 06:06 < darosior> Yeah because i was breaking the number of opcodes .. But it was compiling lolg 06:06 < darosior> Ttying your suggestion now 06:07 -!- roconnor [~roconnor@host-45-58-200-239.dyn.295.ca] has joined ##miniscript 06:07 < darosior> Hmmm but thresh(2, [75 keys], [75 keys]) breaks n of wit elements 06:07 < darosior> Oh you proposed "Or" 06:13 < darosior> Oh nice, that was quite a nice suggestion: 5252 bytes for 77 stack elems 06:15 < sanket1729> perfect. If andytoshi is up we can do yet another patch release :P 06:23 < andytoshi> lol i'm up 06:23 < andytoshi> lemme read the scrollback here 06:25 < andytoshi> lol gross 06:26 < andytoshi> +1 on "we should've used Result" :P oh well 06:26 < andytoshi> sanket1729: is this actually a patch release? 06:27 < andytoshi> does it touch public apis? 06:27 < darosior> Rebasing my branch on your last fixes, let's see :) 06:28 < sanket1729> it does not touch any APIs. changes the locality rules which were incorrect previously. 06:29 < sanket1729> also andytoshi: maybe you are a better person to answer this question https://github.com/ElementsProject/elements/pull/949#issuecomment-742550864 06:32 < andytoshi> yeah, i'll write a reply 06:38 < andytoshi> i'll write that, then review this PR, then do a patch release 06:44 < darosior> sanket1729: wait actually now i can't compile an 8-of-9 anymore 😭 06:46 < darosior> Oh my bad.. 06:50 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 240 seconds] 07:20 < andytoshi> replied to dmitry 07:20 < andytoshi> darosior: lol is this PR good or not? 07:21 < darosior> andytoshi: Yes it is :) 07:21 < darosior> See https://github.com/rust-bitcoin/rust-miniscript/pull/207 that tests it 07:22 < andytoshi> alright, reviewing both now 07:29 < andytoshi> lolol that our checks had duplicated lines in them 07:29 < andytoshi> idk how i didn't see that 07:30 < andytoshi> good catch on the andor thing. ugh this is hard to verify with a human brain 07:31 < andytoshi> musing on the non-canonical andor dissat though 07:32 < andytoshi> it may be cheaper to sat a and dissat b, rather than dissat a and c 07:32 < andytoshi> but i think we have a design rule in miniscrpit that dissats are always non-hassig 07:35 < andytoshi> oh!, so i guess this code change (estimating the max dissat size) this is fine 07:36 < andytoshi> there is definitely no reason to look at the max of these two dissatisfactions. we know the "dissat a dissat c" one is always available and will be taken if it's cheapest 07:36 < andytoshi> and "sat a dissat b" may not be available, so we can't reduce our dissat cost estimation based on it 08:09 < andytoshi> lol holy shit i cannot keep up with dmitry, just seeing his alloy-generated test vectors now 08:09 < andytoshi> and i still haven't looked at his alloy ML post 08:17 < andytoshi> darosior: can you rebase 207 08:17 < darosior> yup 08:17 < andytoshi> oh thanks 08:17 < andytoshi> not a big deal, github is just showing sanket's commits as well which is a bit annoying 08:18 < andytoshi> and i thought for a moment i'd have to verify they were the same ... but they literally have the same commit IDs. so idk why github is being dumb 08:21 < andytoshi> cool til that assert_eq! can take a parameterized failure message 08:21 < darosior> I think you can do a clean merge since the tree is the same ? 08:22 < darosior> Or a you using the Github UI to merge ? 08:22 < darosior> Anyways, done 08:22 < andytoshi> yeah sorry, i was just being dumb 08:22 < andytoshi> it was a clean merge, my worry was that i'd have to do a tiny bit of extra work to make sure the version of sanket's PR i merged was the same as what you built on 08:23 < andytoshi> which was lazy of me 08:24 < darosior> Are you going to do a release today ? 08:24 < andytoshi> yeah 08:24 < andytoshi> actually if you could add a commit which bumps the patch version 08:24 < darosior> Oh i could add a version bump btw.. 08:24 < andytoshi> yeah :) 08:24 < darosior> :) 08:25 < darosior> Done 08:26 < andytoshi> hopefully this weekend i'll be able to write a smarter merge script BTW (or rather, take the one bitcoin core uses) 08:26 < andytoshi> it's blocked on me generating a new GPG key 08:26 < andytoshi> which is blocked on me finishing my deterministic rsa key generator to do self-signing 08:29 < andytoshi> ok, just waiting on CI (on github and my local check-every-commit script) 08:40 < andytoshi> publishing 08:41 < andytoshi> done 08:42 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has joined ##miniscript 08:43 < darosior> Thanks! 08:44 * darosior is back to finding new bugs downstream 08:53 < andytoshi> hehe 10:46 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript 15:08 -!- jonatack [~jon@109.232.227.138] has quit [Ping timeout: 246 seconds] 15:09 -!- jonatack [~jon@213.152.186.24] has joined ##miniscript 15:30 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Remote host closed the connection] 15:30 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined ##miniscript 15:30 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Remote host closed the connection] 15:34 -!- jonatack [~jon@213.152.186.24] has quit [Quit: jonatack] 15:40 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined ##miniscript 15:49 < darosior> SortedMulti is bugged too.. 15:50 < darosior> Wanted to migrate (i still don't really get the rationale, but hey compatibility) so i quickly tested it 15:50 < darosior> Here is the patch https://0bin.net/paste/gbhNn1nc#JieFsLE0hJ3Uyaxds3WeENBjXlURWk5Olc443TgkjUz 15:51 < darosior> Here is the output https://0bin.net/paste/0jm+uPpR#Zf063-CHP6hum7XMEJNvTvaoW5mTX+toqDEAb1KQFvz 😭 15:52 < darosior> It's broken in (at least) two ways: 15:52 < darosior> - For 98 keys i get a CHECKMULTISIG of 62 (wtf) 15:52 < darosior> - It compiles to a non-standard (>15) CHECKMULTISIG 15:53 < darosior> andytoshi, sanket1729: ^ 15:53 < darosior> Looking into it now... 15:54 -!- jonatack [~jon@88.124.242.136] has joined ##miniscript 15:59 < darosior> Sorry, it's just the second point actually. 15:59 * darosior feels bad for not having checked hex 15:59 -!- jonatack [~jon@88.124.242.136] has quit [Ping timeout: 260 seconds] 16:00 -!- jonatack [~jon@213.152.186.40] has joined ##miniscript 16:01 < darosior> And s/>15/>20/ too, but still 16:03 < darosior> Oh, i got it: https://github.com/rust-bitcoin/rust-miniscript/blob/e9732e466c830fde02a127aad331ac54ceebbdfb/src/descriptor/mod.rs#L1500-L1514 should return a thresh / a combination of and()s. Path soon ™️ 16:03 < darosior> (if keys.len() > 20) 16:09 < darosior> Though this case seems completely unspecified in the BIP.. 16:15 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Ping timeout: 240 seconds] 16:15 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Ping timeout: 240 seconds] 16:16 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Ping timeout: 240 seconds] 16:24 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript 16:24 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined ##miniscript 16:27 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined ##miniscript 16:44 < sanket1729> darosior: sortedmulti should only work for upto 20 keys. I wonder if we should take the effort for creating something adhoc for > 20 keys 16:44 < darosior> Hmm 16:44 < darosior> I'd like to use it actually but not only for <20 keys 16:45 < darosior> I wrote the patch anyways, but can make it an error if >20 keys if we decide so.. 16:46 < sanket1729> Because a sortedmulti >20 keys descriptor won't work in core. 16:46 < sanket1729> We would also have to suggest to update to sortedmulti along with other miniscript descriptors 16:47 < sanket1729> Which is an option 16:47 < sanket1729> nothing wrong with it. cc andytoshi 16:52 < darosior> Conceptually, it looks like everyone shoudl tend to use sortedmulti() because multi() would be annoying for HW manufacturers. But if it is limited to 20 keys it's annoying for applications. 16:54 < sanket1729> Yeah, I agree. I have come to appreciate the value of sorting. 16:54 < sipa> please don't tell me we'll need a sortedand_b etc 16:54 < sanket1729> lol 16:55 < sipa> or a sortedthresh 16:56 < sipa> sanket1729: what does that have to do with bitcoin core? sortedmulti translates to OP_CHECKMULTISIG(VERIFY), and that opcode is restricted to 20 keys 16:56 < darosior> I mean i don't really get the reason of sorting if we have a descriptor in the first place, but if we use sortedmulti we should not limit it to 20 keys because of some legacy BIP ? 16:56 < sipa> darosior: i don't understand what you're suggesting 16:57 < sipa> or what BIP you're referring to 16:57 < darosior> Doing as for Multi for SortedMulti 16:57 < darosior> BIP67 iirc 16:57 < sanket1729> darosior was suggesting extending sortedmulti is limited to 20 in bitcoin core and that we support more than 20 keys by getting a thresh type construction with keys sorted 16:57 < sipa> WHAT DOES THIS HAVE TO DO WITH BITCOIN CORE 16:58 < sipa> the checkmultisig opcode is restricted to 20 keys in the bitcoin Script language 16:59 < sanket1729> Well, if we implement something adhoc for sortedmtuli(50, ). We would want all the ecosystem to do that one day. Currently the ecosystem only supports descriptor upto 20.(because of CMS limit) 16:59 < sanket1729> I meant this disuccsion is more general than rust-miniscript. 16:59 < sipa> that adhoc isn't needed; miniscript has thresh 16:59 < sipa> which works for more than 20 keys 17:00 < sipa> descriptor fragments refer to specific scripts 17:00 < sanket1729> but darosior was asking for a sorted one. 17:00 < sanket1729> with more than 20 keys. ' 17:00 < sipa> you can't redefine sortedmulti to mean something other than the OP_CHECKMULTISIG based construction; if you need that, give it another name 17:00 < sipa> ok, so you do want sortedthresh 17:00 < sipa> sigh 17:01 < sipa> that's my hesitation around sorting: you can't really make it work generically without breaking other nice properties 17:01 < sipa> because in a thresh() the first argument has a different type than the other ones 17:02 < sipa> so you can't just reorder them in general 17:02 < sanket1729> yep. I get it. 17:03 < sipa> sorry for yelling earlier, i have a short fuse today 17:04 < sanket1729> I am not arguing for it :) . Just raising a discussion because a lot of people are asking about sorting 17:04 < darosior> I am not either 17:04 < sipa> i understand the concern, but i don't think it really works... and i'd generally prefer if people would stop arguing for more sorting :p 17:04 < sipa> it works for the simplest of cases, but generalizations of it just aren't nice anymore 17:12 < sanket1729> We expect HW wallets to have a generic miniscript finalizer instead of a adhoc thing where they sort and place signatures in order. 17:13 < sipa> HW wallets don't need to finalize, that can be done by SW 17:13 < sipa> but yes, i think whatever they support, we should see key sorting to something that's done at the descriptor level, if at all 17:14 < sipa> then it shouldn't matter if sorting was done or not, generic infrastructure will work regardless 17:15 < sanket1729> Agreed. 17:16 < sipa> but it does have limitations; imagine you have a 40-of-40 multisig policy 17:16 < sipa> ideally you convert that to a miniscript descriptor that's an and of two thresh_m's 17:17 < sipa> if you want the same kind of privacy as sortedmulti has you'd want to sort *across* the two 17:19 < sanket1729> oh, I did not consider the privacy side of sortedmulti, only the convenience. 17:20 < sipa> in a descriptor world there is no convenience 17:20 < sipa> except a slightly longer descriptor :p 17:34 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 260 seconds] 17:39 < andytoshi> oops i missed this 17:39 < andytoshi> yeah, what pieter says 17:40 < andytoshi> our sortedmulti should check the 20 key limit, if it doesn't now 17:40 < andytoshi> though i thought that it did 17:55 < sanket1729> bitcoin core has the limit set to 16 instead of 20 which I think comes from p2sh 520 byte limit 17:56 < sanket1729> but looking at the it seems like 17 thresh is also invalid for wsh(sortedmulti) 17:56 < sanket1729> https://github.com/bitcoin/bitcoin/blob/736eb4d8083862a6c3dd79e65afca6217cf7939d/src/script/descriptor.cpp#L900-L901 17:57 < sipa> ah that could be a bug 17:57 -!- dr-orlovsky [~dr-orlovs@31.14.40.19] has quit [Ping timeout: 256 seconds] 17:59 < sipa> or an unnecessary limitation at least 18:00 < sipa> sanket1729: oh this may be due to the fact that the pattern matching signer can't deal with numbers above 16 18:00 < sipa> (as they're encoded as pushes rather than OP_n) 18:00 < sanket1729> ah 18:00 < sipa> that's easy to fix, if that's the case 18:54 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has joined ##miniscript 20:35 < sanket1729> it is indeed the case. The pattern signer parses numbers(>16) correctly, but has explicit checks for rejecting it. 20:36 < sanket1729> raised an issue incase anyone wants to do it. Busy with some elements work now. 20:40 < sipa> we could have have a descriptor fragment sortkeys(subexpression) that sorts all the keys inside subexpression if we really wanted 20:41 < sipa> so sortkeys(and_b(thresh(3,pk(A),pk(B),pk(C)),pk(D))) would be equivalent to and_b(thresh(3,pk(A),pk(B),pk(C)),pk(D)), but with A,B,C,D rearranged to be in lexicographical ordering 20:41 < sipa> dragons be upon you if the keys aren't interchangeable... 20:42 < sipa> not really worth it i think, but if we'd really want something in descriptors to get the privacy benefits of sorting, that's one way to do it generically 20:43 < sanket1729> I don't think privacy point for sorting is that strong that we should go out of the way to do this 20:43 < sanket1729> yeah 20:43 < sipa> agree 20:44 < sanket1729> The privacy loss only occurs if you reuse one of the keys across the many multisigs you participate in, then all your keys from the multisig can be linked. 20:44 < sipa> hmm, no 20:46 < sipa> if you have an application that uses descriptor thresh(3,userkey,usermobilekey,escrowservice), and someone is able to distinguish these transactions on the network by unrelated means, they can tell who is signing, just by observing the position of the signing key 20:46 < sipa> it doesn't need any key reuse 20:46 < sipa> you can argue though that most is lost already in case this distinguishing occurs 20:47 < sanket1729> Ah, I was assuming that we don't know which positions are whose. But that is a wrong assumption. 20:48 < sanket1729> reading BIP67, it does not mention privacy 20:50 < sipa> well, that's the only good reason i can come up with sorting :) 20:50 < sipa> (in a descriptor setting where you have a string that unambiguously encodes the scriptPubKey anyway, and you're not trying to reconstruct it from a bag of pubkeys) 20:51 < sanket1729> everything is clean in the descriptor world 21:35 -!- harding [quassel@2600:3c03::f03c:91ff:fe7b:78d1] has quit [Remote host closed the connection] 21:39 -!- harding [~quassel@newmail.dtrt.org] has joined ##miniscript 22:00 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection] 22:00 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript 23:30 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 260 seconds] --- Log closed Fri Dec 11 00:00:45 2020