--- Day changed Wed Nov 18 2020
00:19 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 240 seconds]     
02:42 -!- midnight [~midnight@unaffiliated/midnightmagic] has quit [Ping timeout: 244 seconds]     
02:46 -!- midnight [~midnight@unaffiliated/midnightmagic] has joined ##miniscript     
05:37 -!- shesek` is now known as shesek     
05:37 -!- shesek [~shesek@164.90.217.137] has quit [Changing host]     
05:37 -!- shesek [~shesek@unaffiliated/shesek] has joined ##miniscript     
06:08 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection]     
06:08 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript     
06:34 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection]     
06:34 -!- shesek [~shesek@164.90.217.137] has joined ##miniscript     
06:34 -!- shesek [~shesek@164.90.217.137] has quit [Changing host]     
06:34 -!- shesek [~shesek@unaffiliated/shesek] has joined ##miniscript     
07:04 < andytoshi> lol the PR is failing CI because of an unrelated fuzz failure
07:05 < andytoshi> which looks like we are parsing a pubkey in a hybrid mode (lol fucking openssl) and then serializing it as uncompressed
07:05 < andytoshi> which is actually a rust-bitcoin "bug" that i'm surprised we haven't run into in previous fuzztests. i'm not sure quite what to do
07:06 < andytoshi> it's legit in legacy bitcoin to use hybrid keys IIRC, and i really do not want to reflect in the rust-bitcoin API that this is a possibility ... but OTOH i would like these keys to round-trip
07:08 < andytoshi> the issue actually is that we call rust-secp (which calls libsecp) to parse the public key .. and then we decide "compressed" vs "uncompressed" based on the length of what we serialized ... and actually this is wrong, we should be looking at the first byte and checking all possibilites
07:08 < andytoshi> s/serialized/parsed/
07:08 < andytoshi> this is so annoying
07:09 < andytoshi> i wonder if we should forbid hybrid keys in rust-bitcoin. it shouldn't affect our consensus validity (these keys are only consensus-revelent when they appear in Script, and we only validate script through libbitcoinconsensus, which is a black-box for us)
07:10 < andytoshi> so it'd only forbid using them in addresses, which is already de-facto forbidden because there is no base58 encoding for the corresponding secret keys
07:10 < andytoshi> and realistically. probably nobody remembers these exist
07:13 < andytoshi> https://github.com/rust-bitcoin/rust-bitcoin/issues/520 should discuss there
07:16 -!- roconnor [~roconnor@host-104-157-230-3.dyn.295.ca] has quit [Ping timeout: 260 seconds]     
07:18 < andytoshi> sanket1729_: definitely, regardless of what rust-bitcoin does, we should refuse to accept hybrid keys in descriptor wallets :)
07:18 < andytoshi> sipa: what happens in your code if someone provides a hybrid key in a descriptor? does it work?)
07:19 < andytoshi> or are your descriptors compressed-only even
07:31 -!- roconnor [~roconnor@host-104-157-230-3.dyn.295.ca] has joined ##miniscript     
07:37 < sanket1729_> andytoshi: I should have read this earlier
07:37 < sanket1729_> I invested the last hour or so trying to figure out whats going on there
07:37 < sanket1729_> I was about to create a branch asking to help me investigate
07:37 < andytoshi> lol sorry! i should've looked at it last night
07:37 < sanket1729_> nah, it's my bad. what are hybrid keys?
07:38 < andytoshi> so....it turns out that secg actually defines four serializations for EC keys in secp256k1, not just two
07:38 < andytoshi> there are normal and uncompressed, which are 33/65 bytes and have prefixes 2 and 4
07:38 < andytoshi> err     
07:38 < andytoshi> prefixes 2/3 and 4/5 depending on parity
07:38 < andytoshi> err     
07:39 < andytoshi> prefixes 2/3 for compressed keys depending on parity. just 4 for uncompressed keys because you have the full y coordinate so why encode the parity
07:39 < sanket1729_> yep     
07:39 < andytoshi> but *also* you have "hybrid keys" where you get both the full y-coordinate **and** the parity. in case you really like error paths and think all code should have extras
07:40 < andytoshi> these are 65 bytes and have prefixes 6/7, IIRC
07:40 < sanket1729_> Ah     
07:40 < andytoshi> and because openssl will parse all of these, and satoshi treated openssl as a black box, all of these are legal in bitcoin
07:40 < sanket1729_> That's why we have the 07 going to 04 in the fuzz case
07:40 < andytoshi> bingo
07:41 < sanket1729_> so, I guess we should be discarding these in rust-secp?
07:41 < andytoshi> lol i'm morbidly curious how long it would've taken you to figure this out, it's so dumb and obscure
07:41 < sanket1729_> I was strting to think maybe it's a rust bug :O 
07:41 < andytoshi> sanket1729_: no, we definitely shouldn't do that, that would make it impossible to verify sigs with them
07:41 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has joined ##miniscript     
07:41 < andytoshi> and they are legal in bitcoin
07:42 < sanket1729_> descriptor specification says that uncompressed key must be 04
07:42 < andytoshi> nice
07:42 < sanket1729_> https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md
07:42 < sanket1729_> > Hex encoded public keys (either 66 characters starting with 02 or 03 for a compressed pubkey, or 130 characters starting with 04 for an uncompressed pubkey).
07:42 < andytoshi> so, i would be ok with forbidding them in the rust-bitcoin PublicKey type because its use is basically limited to producing addresses and descriptors
07:43 < andytoshi> but also ok with leaving rust-bitcoin alone and just forbidding them in the DescriptorPublicKey parser
07:43 < andytoshi> i think we should definitely do that, for now, so we don't have to wait on rust-bitcoin to move forward
07:44 < sanket1729_> cool. Is there a nice way to do this. Since, we are probably using bitcoin::PublicKey::from_str at multiple places
07:45 < andytoshi> yeah, before calling from_str we should check the first byte
07:45 < sanket1729_> and we can't do that for the generic Pk::from_str
07:45 < andytoshi> and check that it's 2/3/4
07:45 < andytoshi> that's ok
07:45 < andytoshi> i think we only want to do this for DescriptorPublicKey
07:46 < andytoshi> and if we have fuzztests that use bitcoin::PublicKey i guess we should change them to DescriptorPublicKey
07:46 < sanket1729_> technically, the miniscript rtt could fail as they use Pk::from_str internally. 
07:47 < sanket1729_> but we can't do anything about it for now. 
07:47 < andytoshi> which Pk::from_str
07:48 < andytoshi> ah! bitcoin::PublicKey, ofc
07:48 < andytoshi> because it's coming from Script
07:48 < sanket1729_> Yep. but can't know that at compile time
07:48 < andytoshi> lol. dammit.
07:49 < andytoshi> i'm pretty surprised/disappointed that the miniscript_rtt test never noticed this
07:49 < andytoshi> i should set up a box that does 1000s of cpu-hours of fuzztesting on this library, like gmax does to libsecp..
07:54 < andytoshi> sanket1729_: want to add a commit which changes DescriptorPublicKey::from_str for now
07:54 < sanket1729_> I am working on it
07:54 < sanket1729_> almost done
07:54 < andytoshi> perfect. i'll get back on reviewing this..
08:14 < sanket1729_> andytoshi: fixed the fuzz failure and CI is passing
08:14 < sanket1729_> restarted the other build
08:15 < andytoshi> dope. i am at tim's talk then have a 1-on-1 meeting with somebody, may need a couple hours to look at this
08:41 < michaelfolkson> Miniscript question on StackExchange https://bitcoin.stackexchange.com/questions/100073/miniscript-or-i-d-j-do-they-always-imply-an-1-or-0-witness
09:05 < andytoshi> thanks, answered
09:06 < sanket1729_> andytoshi: it has incomplete sentence at the end
09:06 < sanket1729_> the stackexchange answer
09:06 < andytoshi> lol SE
09:06 < andytoshi> one sec
09:07 < andytoshi> it didn't like me typing an answer, then joiyning bitcoin.SE, then submitting, i guess
09:07 < andytoshi> fixed
09:08 < sanket1729_> the empty signature failure still a standardness rule for schnorr? 
09:08 < sanket1729_> *is the
09:12 < sanket1729_> It is
09:14 < sipa> not just standardness
09:14 < sanket1729_> oops, I meant a consensus failure
09:53 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection]     
09:53 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript     
11:00 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Remote host closed the connection]     
11:00 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined ##miniscript     
12:22 -!- nothingmuch_ is now known as nothingmuch     
13:21 < sanket1729_> andytoshi: so I tried new Descriptor refactor and turns out it does not save that much duplication. 
13:21 < andytoshi> hmm. damn
13:23 < sanket1729_> The issue is because we always separate match arms for different ScriptContext
13:23 < sanket1729_> even if the match arm body is identical
13:24 < andytoshi> right, i see
13:24 < andytoshi> ok fair enough. it was worth a shot
13:24 < andytoshi> sorry to waste your time
13:25 < sanket1729_> it was definitely worth a shot. The upside was huge. Would you like to get a release out today if you have time?
13:25 < andytoshi> hmm, yeah, let's give that a shot.
13:26 < andytoshi> i'm done calls for the day and can look at the satisfier
13:26 < andytoshi> then i am thinking about making the satisfied_constraints iterator use its own special-purpose type instead of using Descriptor
13:26 < andytoshi> you're welcome to give that a shot if you want, i don't have a more specific plan than that. but i'd like to get rid of the unimplemented!s and maybe even get rid of the transmutes
13:28 < sanket1729_> Do you think that unimplemented!() in SatisfiedConstraints::from_descriptor() for srtedmultivec are worth tackling?
13:29 < andytoshi> yeah, mainly because i think the existing transmutation is also hacky and i don't like it
13:30 < sanket1729_> So, what is the new API that you are suggesting?
13:30 < andytoshi> i'm tempted to give SatisfiedConstraints its own Ctx type even and have it parse miniscripts into that (similar to the "nonstandard Ctx idea but less extreme")
13:30 < andytoshi> i don't know, i need to experiment more. i was hoping to do that this weekend
13:30 < andytoshi> but the elements rebase took too much time
13:39 < sanket1729_> It would be undoing of a lot of work, but it might be make sense to return a collection instead of iterator
13:40 < sanket1729_> But it saves allocation.
13:43 < andytoshi> i really don't want to return a collection
13:45 < andytoshi> lol this satsifaction code is really hard to review
13:51 < andytoshi> mostly it's because my original code was hard to follow
13:52 < andytoshi> eg having a "minimum" function that sometimes returns Unavailable even if neither input was Unavailable :P
13:52 < andytoshi> ok ack merged
13:56 < sanket1729_> I have a small PR to fix #179. 
13:56 < sanket1729_> I am working on one
14:27 -!- digi_james [sid281632@gateway/web/irccloud.com/x-luadapapmlcwedbg] has quit [Ping timeout: 260 seconds]     
14:27 -!- elichai2 [sid212594@gateway/web/irccloud.com/x-mbwcgsqcljczdygs] has quit [Ping timeout: 260 seconds]     
14:28 -!- digi_james [sid281632@gateway/web/irccloud.com/x-cgclkowkiyrokwek] has joined ##miniscript     
14:29 -!- elichai2 [sid212594@gateway/web/irccloud.com/x-kmxptbegweexrioe] has joined ##miniscript     
14:41 < sanket1729_> pushed a new benach
14:41 < sanket1729_> *branch
14:43 < sanket1729_> I think the only thing which we need to decide is what to do about the satisfied constraints
14:44 < andytoshi> yep     
14:44 < andytoshi> i need a bit more time
14:46 < andytoshi> i think i can simplify a lot of stuff here
14:46 < sanket1729_> I am writing a satisfy fuzz test
14:46 < andytoshi> awesome :)
15:12 -!- Ed0 [~edouard@2001:41d0:401:3100::4897] has quit [Ping timeout: 244 seconds]     
15:13 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Ping timeout: 240 seconds]     
15:15 -!- Ed0 [~edouard@137.ip-193-70-113.eu] has joined ##miniscript     
15:26 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript     
15:34 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Remote host closed the connection]     
15:44 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined ##miniscript     
16:55 < andytoshi> do we still not have the Script::p2pkh() etc consrtuctors in a published version of rust-bitcoin???
16:55 < andytoshi> oh, no, we do, it's just duckduckgo sending me to outdated docs
17:00 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 272 seconds]     
17:06 < sanket1729_> andytoshi: I see secp bindings are feature  guarded by fuzztarget 
17:07 < andytoshi> what do you mean?
17:07 < sanket1729_> I want to use Signature::from_der inside the fuzztarget 
17:07 < sanket1729_> feature
17:07 < andytoshi> oh interesting
17:07 < sanket1729_> for satisfier
17:07 < andytoshi> you can un-gate the bindings
17:07 < andytoshi> and even change the fuzztarget parser to use the ffi DER parser
17:08 < sanket1729_> Why did we have those bindings initially?
17:08 < andytoshi> fuzztarget should only disable the actual cryptography that would require the fuzzer to e.g. forge signatures to do anything interesting
17:08 < andytoshi> they were presumably gated because matt didn't use them at all
17:09 < andytoshi> in his fuzzing code
17:09 < andytoshi> i would like to revisit this... it's really scary that we have a build target that completely disables all the cryptography in that library
17:10 < andytoshi> but it's difficult otherwise to stub out crypto code in the rust-bitcoin fuzzer
17:11 < andytoshi> i think we should use a direct rustc --cfg argument at least instead of a cargo feature
17:11 < andytoshi> if i remember this tomorrow i'll do that
17:35 < andytoshi> sanket1729_: oh, we should figure out what to do about the compiler error in the main Error enum
17:35 < andytoshi> i think we should probably let the compiler error type be a top-level type. we can re-export it as CompilerError or something
17:36 < andytoshi> i'm going to do the same with the interpreter, because it has a pile of error conditions that don't really make sense as a part of the main error enum
17:40 < sanket1729_> andytoshi: I disabled all the all fuzztarget features but still the fuzzer seems to be using the fuzztarget flag
17:41 < sanket1729_> Is there anything going on travis-fuzz.sh that does some magic
17:41 < andytoshi> in what library?
17:41 < sanket1729_> Because I am not able to reproduce the crash locally
17:41 < sanket1729_> rust-miniscript
17:41 < andytoshi> which crash?
17:41 < sanket1729_> For my satisfaction code. It is still local. 
17:42 < sanket1729_> I ran the travis-fuzz.sh locally, it is crashing on some hex
17:42 < andytoshi> ok     
17:42 < andytoshi> in fuzz/Cargo.toml you removed the fuzztarget feature?
17:42 < sanket1729_> yeah
17:42 < andytoshi> that should be sufficient
17:42 < sanket1729_> maybe something is unclean. 
17:43 < andytoshi> maybe. i'm doubtful
17:43 < andytoshi> can you try running cargo -v and see if fuzztarget shows up in any rustc invocationss
17:50 < sanket1729_> So, the script run cargo hfuzz run ..
17:50 < sanket1729_> The strange part is it is working on the CI, maybe I have something wrong locallyt
18:29 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has joined ##miniscript     
18:41 < andytoshi> almost done with the interpreter btw, i think i'll finish tonight
18:42 < andytoshi> what's cool is that i had to touch zero of the actual interpreter code, other than renames
18:42 < andytoshi> but all the transmutes go away and the from_txin_with_witness_stack function (and the create_descriptor module) all go away
19:03 < andytoshi> sigh, lifetime issues
19:27 < andytoshi> oh, interesting, the PSBT code was using the existing weirdo interpreter API to extract a sighash for the transaction
19:27 < andytoshi> let me see if i can provide a more direct API for that, it's pretty useful
19:27 < sanket1729_> nice
20:55 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Ping timeout: 240 seconds]     
21:04 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript     
21:21 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection]     
21:21 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined ##miniscript     
21:23 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 256 seconds]