--- Log opened Sat Dec 12 00:00:44 2020 03:07 -!- jonatack [~jon@213.152.161.244] has quit [Ping timeout: 256 seconds] 03:24 -!- jonatack [~jon@88.124.242.136] has joined ##miniscript 03:29 -!- jonatack [~jon@88.124.242.136] has quit [Ping timeout: 264 seconds] 03:30 -!- jonatack [~jon@213.152.161.40] has joined ##miniscript 04:03 < sanket1729> possibly too late to suggest this. But I think if we have descriptor a trait having methods address, witness_script, script_code, satisfy .. it would make extending descriptors easier in future. 04:03 < sanket1729> I don't think there is much value in the enum struct which we currently have. What could be a possible use-case where we might want to match on it? 06:35 < andytoshi> sanket1729: so, the value is (a) rust has better ergonomics for working with real objects than with trait objects 06:35 < andytoshi> (b) it means the user doesn't have to box so much (though miniscripts are already recursive types so it's not like you can avoid heap allocations :)) 06:36 < andytoshi> but i agree, there's no real use-case for matching and having a closed enum makes it a PITA for users to extend the library 06:36 < andytoshi> e.g. in liquid we have this special garbage descriptor which compiles to this legacy OP_DEPTH construction, and it's super ugly how we implemented that 06:37 < andytoshi> and going forward we'd like to add pegin descriptors, and since that'd be a user-facing descriptor maybe the ugliness is less oku 06:37 < andytoshi> sanket1729: so, one idea might be to move all the descriptor methods into a trait, and have all the descriptor variants be independent structs 06:37 < andytoshi> but *also* provide the enum, which would just contain every struct (and here we'd have a shitload of repetition but i think it'd be ok to macro it away) 06:38 < andytoshi> then users who wanted a vector of descriptors could just keep using the struct, instead of having to do their own Box and deal with the annoyance of that 06:38 < sanket1729> Yep. I like it 06:38 < andytoshi> and elements-miniscript could just recreate that enum entirely (maybe using the same macro..), plus the legacy-watchman descriptor plus pegin descriptors 06:39 < andytoshi> darosior: do you have any thoughts/reactions to this? 06:40 < sanket1729> just that it's going to be yet another major breaking change. 06:40 < andytoshi> that's fine :P 06:41 < sanket1729> One sad part I could not define the translate methods in the trait. 06:41 < sanket1729> Because rust won't allow returning a generic Self from impl Self

06:42 < sanket1729> inside a trait. 06:42 < andytoshi> oh frustrating 06:42 < andytoshi> lemme go bug ##rust about this 06:43 < sanket1729> someone also tried this: https://stackoverflow.com/questions/57701914/trait-method-which-returns-self-type-with-a-different-type-and-or-lifetime-par 06:44 < andytoshi> thanks 06:46 < andytoshi> i mean, worst-case we can impl translate_pk on the giant enum 06:47 < andytoshi> and that's another thing elements-miniscript would need to copy 06:47 < andytoshi> but i'm sure we can somehow do better 06:48 < andytoshi> ah ok, the SE post has an answer for how to use traits... you need a second trait for it 06:48 < andytoshi> but i think that's ok, it might even simplify things because we currently have a zillion objects that you can convert pks on 06:48 < andytoshi> and we're already not generic over those 06:49 < andytoshi> that is, you define `trait PkConvert { type Output; fn convert_pk(self) -> Self::Output; }` 06:50 < andytoshi> then `impl PkConvert for Descriptor { type Output = Descriptor; ... }` 06:50 < sanket1729> yup, makes sense 06:50 < andytoshi> lol i am starting to feel matt's "why won't rust just fucking pretend to be C++ just for this case" 07:14 < sanket1729> if we are going forward with this, we should first merge all existing PRs first to avoid a rebase for all of them. 07:18 < andytoshi> sure 07:18 < andytoshi> sureiew the checksum thing 07:18 < andytoshi> let me review* 07:23 < andytoshi> lol dammit i gotta completely rebuild core, i updated libboost so i need to re-link, and i moved my source directory so i need to do a full rebuild since autotools has injected full paths all over the place 07:23 < andytoshi> just to check the stupid checksum test vectors :P 07:25 < andytoshi> is it true that # can't appear anywhere else in a descriptor? 07:26 < andytoshi> or do we need to use rsplit rather than split? 07:28 < sanket1729> only one symbol 07:29 < sanket1729> https://github.com/bitcoin/bitcoin/blob/b18978066d875707af8e15edf225d3e52b5ade05/src/script/descriptor.cpp#L1058-L1086 07:29 -!- darosior [~darosior@194.36.189.246] has quit [Remote host closed the connection] 07:30 -!- darosior [~darosior@194.36.189.246] has joined ##miniscript 07:38 < andytoshi> ncie, cool 07:39 < andytoshi> ok, reviewed the code, i'm happy with it except that i wish they'd added a real checksum error variant rather than BadDescriptor 07:39 < andytoshi> need to compile every commit, and also sync my bitcoind and check the checksum test vectors 07:41 < andytoshi> second-to-last commit does not compile with 1.29 07:46 < andytoshi> lol TIL that you can parse miniscripts without -any- checks by calling expression::Tree::from_str and then Miniscript::from_tree, basically just unrolling miniscript::from_str 07:47 < andytoshi> and apparently i showed people how to do this at an Advancing Bitcoin workshop in feb 07:48 < sanket1729> should not expose FromTree at all? 07:48 < andytoshi> i think we should, it's plausible somebody would want to do this for some reasn 07:48 < andytoshi> and it's undiscoverable and crazy enough that nobody would do it by accident 07:49 < sanket1729> yeah 07:50 < andytoshi> acked #197. think i should merge? 07:50 < andytoshi> strictly speaking it breaks bisection with rustc 1.29 07:50 < andytoshi> but i'm having trouble imagining why we would ever actually need to do that.. 07:51 < sanket1729> yeah, agreed. I had a minor bit about chaning ? to unwrap which can be ignored 07:51 < andytoshi> in general i'm ok with using ? even when errors can't actually happen 07:51 < andytoshi> in rust 1.1234678 or whhatever, when we have the ! type, we can audit this 07:52 < sanket1729> I prefer unwrap because it does increase your confidence in your code. Maybe's its scary because it may panic 07:53 < sanket1729> do final applications have hook to catch unwraps? 07:53 < andytoshi> what do you mean 07:54 < sanket1729> Say, I am using some third party party which I must use but it can panic for some inputs 07:54 < sanket1729> But I don't want to crash my application 07:54 < andytoshi> typically nobody does this 07:54 < sanket1729> or rather, if it happens get to start/initial state and try again or something 07:55 < andytoshi> but if you were really stuck using an lib that had a crash bug they wouldn't fix 07:55 < andytoshi> and you couldn't sanitize your own inputs 07:55 < andytoshi> then you might do this 07:55 < sanket1729> you can never be sure, right? 07:55 < andytoshi> i mean, you can never be sure that your libraries won't delete your home director 07:56 < andytoshi> but if a library had a crash bug, i'd rather have that result in a crash 07:56 < andytoshi> than catching it 07:56 < andytoshi> because what do i do, as an application, after catching it? 07:56 < sanket1729> Go to initial state. 07:57 < sanket1729> or some pre-defined state 07:57 < andytoshi> we do this in liquid with systemd 07:57 < andytoshi> but putting that logic into an actual application does not really make sense to me 07:58 < sanket1729> yeah, I guess you need an external problem to monitor this 07:58 < andytoshi> if an .unwrap() triggers you have a logic bug, there is no way in principle to recover other than by restarting your daemon 07:58 < andytoshi> and alerting some operator 07:59 < andytoshi> or not even restarting the daemon ... eg if a web request handler does this you just send an error to the client 07:59 < sanket1729> it maybe be other secondary feature bug which triggers the unwrap. 08:00 < sanket1729> But the user still wants to use the primary app. But I get it this is best left to some other program observing the application rather the application itself 08:01 < andytoshi> if the user wants to use the primary app, and the app has a logic bug, then the user is out of luck :) 08:01 < andytoshi> though i see what you're saying, if a spellchecker panics then the word processor shouldn't shut down 08:02 < andytoshi> and in massive applications like that, you do separate out stuff like that (indeed you'd do the spell check in another thread, and do something more intelligent if the thread crashed than shutting down the whole app) 08:03 < andytoshi> but again, this is not typical 08:04 -!- jonatack [~jon@213.152.161.40] has quit [Ping timeout: 240 seconds] 08:04 < andytoshi> ok, trying to understand the point of #209 08:05 < sanket1729> about 208, you think we should keep it 20 right? 08:05 < andytoshi> yeah, i think keeping 20 is fine 08:05 < andytoshi> sipa indicated that core would like to change it to 20 08:05 < andytoshi> and realistically, i think everybody using this has n = 3 08:06 < andytoshi> so it's not a very important incompatibility 08:06 -!- jonatack [~jon@109.232.227.138] has joined ##miniscript 08:06 < sanket1729> 209 is basically using is_uncompressed() function for size calculation APIs which should not require conversion to publickey 08:07 < andytoshi> ok, that i can get behind 08:07 < andytoshi> but why also change the aPI 08:07 < sanket1729> because the parameter ToPkCtx is no longer needed 08:07 < sanket1729> for size calculation APIs. Earlier, we converted to public keys and then checked the size 08:08 < andytoshi> for our implementations, yes, it's not needed 08:08 < sanket1729> But, we have the hack is_uncompressed() in MiniscriptKey 08:08 < andytoshi> heh i don't think .is_uncompressed is a hack, any more than bitcoin having uncompressed keys is a hack 08:09 < andytoshi> but i see, ToPkCtx we actually don't want in APIs if we can avoid it 08:09 < sanket1729> We will also need isXonly in future :P 08:09 < andytoshi> i was confusing this with some other stuff 08:09 < andytoshi> will we need isXonly? 08:09 < andytoshi> i think taproot is going to need all-new types everywhere 08:09 < sanket1729> because while parsing in Taproot context, we don't want to parse other keys 08:09 < andytoshi> hmm oh, yeah, i guess we will need is_x_only 08:10 < sanket1729> and vice-versa. While parsing segwitb0, we don't want xonly 08:10 < andytoshi> right .. but like, when serializing a taproot script we also don't want to produce a Script, we have to produce a TapScript 08:11 < andytoshi> which has different opcodes 08:11 < andytoshi> and actually we want to produce a TapTree, and sometimes only part of one 08:11 < sanket1729> Ah, right. Some of these problems are because we share bitcoin::Script for both Segwit and Legacy 08:11 < sanket1729> I mean bitcoin shares 08:12 < andytoshi> bitcoin shares because it's written in a language with a much more dynamic type system 08:13 < sanket1729> Do you want to have a new Script for TapScript with separate types for all the existing opcodes? 08:13 < andytoshi> heh, realistically no 08:13 < andytoshi> but we will need a new TapTree type 08:13 < andytoshi> and the output of .witness_script for a taproot descriptor should not be a Script 08:13 < andytoshi> tbh i'm not sure what it should be.. 08:14 < sanket1729> did we merge something in wrong order 08:14 < sanket1729> because pk() is failing roundtrip descriptor 08:15 < andytoshi> lol dammit, i even ran all the tests on the merged commit, i was all proud of myself 08:15 < andytoshi> but apparently i have to fuzz too 08:15 < andytoshi> oh no, i'll bet that the checksum PR broke the fuzztests on its own 08:16 < andytoshi> surprising that CI didn't catch that 08:16 < andytoshi> merged #209 08:16 < andytoshi> where do you see the fuzz failure? 08:16 < sanket1729> on the merge commit CI 08:17 < andytoshi> ah let's take a look 08:17 < sanket1729> actions tab 08:17 < andytoshi> yeah found the same thing locally 08:18 < sanket1729> left: `"pk()"`, 08:18 < sanket1729> right: `"pk(dummykey 08:18 < andytoshi> weeird, i don't think that has anything to do with the PRs we just merged 08:20 < sanket1729> we do 08:21 < sanket1729> we changed {:?} instead of desc.to_string 08:21 < sanket1729> to fix the roundtrip, but dummykey and dummykeyhash don't roundtrip on debug 08:22 < andytoshi> oh lol derp 08:22 < andytoshi> yeah i should have nacked that 08:23 < andytoshi> i basically looked for "did he change the fuzz test to not break" and didn't really look at what he did 08:24 < andytoshi> we should change the fuzztest to break apart the descriptor 08:24 < andytoshi> and check that the input either matches the non-checksummed part or the whole thing 08:26 < sanket1729> will add a fixup commit. 08:26 < andytoshi> i can do it 08:26 < sanket1729> I think we can modify the test to also recheck the checksum 08:27 < andytoshi> what do you mean recheck 08:27 < sanket1729> I guess check the checksum is correct. But it's meaningless I guess 08:27 < sanket1729> Because we run the same code on it again 08:28 < andytoshi> hmm 08:28 < sanket1729> I meant parse the output into 2 parts. And check the checksum is correct. But now that I think I don;t think this adds any value 08:29 < andytoshi> agreed 08:29 < andytoshi> it would be nice if we could check that the checksum would round-trip 08:29 < andytoshi> but we know that it doesn't 08:30 < andytoshi> i have a fixup commit but it'll eventually fail, if the fuzzer can ever find a weirdly-encoded descriptor with a valid checksum 08:31 < andytoshi> (in fairness, i highly doubt that the fuzzer -will- find such a thing) 08:33 < sanket1729> yep, really hard to find it. 1/2^40 to get a right one. 08:33 < andytoshi> the fuzzer could probably do better than 2^40 iterations, but probably not a lot better 08:34 < andytoshi> anyway this is my proposed fix https://github.com/rust-bitcoin/rust-miniscript/pull/210 08:34 < andytoshi> if you had something better in mind that's fine too 08:34 < andytoshi> meanwhile i will modify my local CI scripts to run the fuzzer on every commit :P 08:35 < andytoshi> or, rather, on the final commit 08:35 < andytoshi> again, i don't see a lot of value in being able to bisect the fuzzer.. 08:42 -!- jonatack [~jon@109.232.227.138] has quit [Quit: jonatack] 08:45 < sanket1729> fix looks great. CI passed. merging 08:47 < andytoshi> dope thanks 08:47 < sanket1729> also this is interesting 08:47 < sanket1729> https://github.com/ElementsProject/rust-elements/issues/66 08:54 < andytoshi> wow yeah 08:58 < andytoshi> have you looked at all at their rust-secp-zkp? 08:58 < andytoshi> idk if you know thomas, but he's been around for years and made some great contributions to rust-secp. The `C: Signing` `C: Verification` stuff was entirely his idea and implementation 09:05 -!- jonatack [~jon@88.124.242.136] has joined ##miniscript 09:10 -!- jonatack [~jon@88.124.242.136] has quit [Ping timeout: 240 seconds] 09:10 -!- jonatack [~jon@213.152.162.84] has joined ##miniscript --- Log closed Sat Dec 12 12:37:25 2020 --- Log opened Sat Dec 12 12:37:25 2020 13:07 -!- jonatack [~jon@213.152.162.84] has quit [Ping timeout: 256 seconds] 15:56 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has joined ##miniscript 23:16 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 272 seconds] --- Log closed Sun Dec 13 00:00:45 2020