--- Day changed Wed Sep 26 2018
01:34 < dongcarl> andytoshi: woke up, looking at your comments
01:34 < dongcarl> I agree the the multiple allocations are less than ideal
01:36 < andytoshi> i don't think they'd be too hard to get rid of
01:37 < dongcarl> It's a little weird because PSBT doesn't enforce ordering of pairs, so there's a distinct possibility that it's the last pair in the map
01:37 < dongcarl> if there are a bunch of unknown global pairs
01:38 < andytoshi> i don't see why the last pair would be treated differently from any other
01:38 < dongcarl> Yeah, found a solution, will push
01:38 < andytoshi> cool, thanks
01:56 < dongcarl> andytoshi: It seems that I would at least have to allocate one `Vec<raw::Pair>` so that I can keep the unknown pairs until we see if the unsigned transaction pair exists for this Global map
02:56 < andytoshi> just assume the unsigned transaction will show up
02:56 < andytoshi> and fail if it doesn't
03:01 < dongcarl> Okay
03:08 < dongcarl> andytoshi: All concerns have been addressed, I've linked the relevant lines as replies to your reviews
05:24 -!- gribble [~gribble@unaffiliated/nanotube/bot/gribble] has quit [Remote host closed the connection]
05:33 -!- gribble [~gribble@unaffiliated/nanotube/bot/gribble] has joined #rust-bitcoin
05:39 -!- warren_ is now known as warren
09:46 < stevenroose> How does one test a custom deserialize method???
09:47 < stevenroose> When you use the #[serde(deserialize_with = "custom")] thing, you have to make a method that takes a Deserializer. I've been trying to manually unit test these methods but I don't get it :D
09:49 < stevenroose> the trait `serde::Deserializer<'_>` is not implemented for `serde_json::Deserializer<serde_json::de::StrRead<'_>>`
09:51 < stevenroose> Arghhh, it was a stupid &mut missing
09:52 < dongcarl> RIP
12:38 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-mwggotwszylbocsc] has joined #rust-bitcoin
14:57 < dongcarl> andytoshi: Thanks for the thoughtful reviews, I'm a little confused about https://github.com/rust-bitcoin/rust-bitcoin/pull/103/files/d5bf5b53a16506605dbcf88b47009793631b61c4#r220569861
15:00 < dongcarl> I'm guessing the rationale would be if it is not one of the cases we match on for `from_u32`, it wouldn't roundtrip and therefore that's unexpected
15:00 < andytoshi> dongcarl: correct
15:01 < andytoshi> having said that, it would technically violate the spec if we rejected that ... but we're already violating the spec by coercing unknown things to SIGHASH_ALL
15:01 < andytoshi> i'd prefer we just reject them outright (and propose an extension to bip174 to say we're doing the right thing)
15:01 < dongcarl> Perhaps we should start documenting where we violate the spec?
15:02 < andytoshi> yeah definitely. i think this is the first instance.
15:02 < andytoshi> probably it should go in the doccomment at the top of psbt/mod.rs
15:03 < dongcarl> Roger, do we need to PR to original BIP 174 to see if they'd like to rebel as well?
15:03 < andytoshi> well, sipa and i have been talking about proposing an extension anyway to add pay-to-contract and sign-to-contract support
15:04 < andytoshi> so let's just try to remember this whenever we get around to that
15:04 < dongcarl> Perfect
15:06 < dongcarl> What should I name the error? `NonStandardSigHashType`?
15:11 < andytoshi> oh, yeah, sure
15:11 < andytoshi> i hate adding error type boilerplate, i'd just use whatever we use for other parse errors, but if you're willing to add an extra variant that'd be much better
15:18 < treyzania> does rust-bitcoin use error_chain or failure?
15:19 < treyzania> oh looks like it doesn't
15:19 < dongcarl> I think we discussed `failure` but found it to be too unstable
15:19 < treyzania> yeah it's still pretty beta
15:19 < dongcarl> I would very much like to use it though
15:20 < dongcarl> Once it's stable
15:20 < dongcarl> How is `error_chain`?
15:21 < andytoshi> i think i looked at it a year or two ago and it seemed like too much work
15:21 < treyzania> pretty cool, but supposedly you're "supposed" to use failure in new projects
15:21 < dongcarl> what do you mean "supposed" to?
15:22 < treyzania> I read somewhere error_chain is being superseded by failure
15:22 < andytoshi> treyzania: lol, we are not using the 2018-09 version of some crate that changes every other week and doesn't even compile with our target compiler
15:22 < andytoshi> ah, yeah
15:22 < andytoshi> i suspect error_chain is deprecated by it though
15:22 < treyzania> very true
15:22 < andytoshi> i think we should just keep with what we're doing and wait for things to settle out
15:23 < treyzania> yeah they're convenient if you're exceedingly lazy
15:23 < dongcarl> Well... seemingly convenient until it changes on you...
15:23 < treyzania> which you probably shouldn't be
15:23 < andytoshi> well, eventually they'll also be standard/idiomatic, which has a lot of value
15:23 < andytoshi> but not now :P
15:29 < dongcarl> andytoshi: pushed fixes to all reviews
15:49 < andytoshi> have more issues :P
15:50 < dongcarl> oh it
15:50 < dongcarl> on it*
15:51 < dongcarl> For https://github.com/rust-bitcoin/rust-bitcoin/pull/103#discussion_r220620908, we take a `Vec<u8>` because most of these deserializations are done on fields of `psbt::raw` types, which are `Vec<u8>`...
15:52 < dongcarl> But I think in Rust casting from `Vec<u8>` to `&[u8]` doesn't cost anything... right?
15:53 < andytoshi> correct
15:53 < andytoshi> you should basically never take &Vec<u8>
15:53 < andytoshi> unless you are doing something really weird and specific
15:53 < dongcarl> Yeah... looked weird to me as well...
15:58 < stevenroose> dongcarl: you're probably too far in to make random nit changes anymore, but just found this to sound weird and non-explicative
15:58 < stevenroose> > A key-value map for an input of the same index in the unsigned transaction.
15:59 < stevenroose> does that mean s/the same index/a certain index/ ?
15:59 < andytoshi> it means the same index in the psbt as it has in the transaction
15:59 < stevenroose> I promised I'd go over it, I'm a bit late and don't expect much
15:59 < dongcarl> yeah thats what I meant...
16:00 < dongcarl> haha stevenroose don't sweat it
16:00 < stevenroose> ok yeah that makes sense
16:01 < stevenroose> dongcarl: well I have to use it soon for, so it's useful!
16:01 < dongcarl> s/the same index/the corresponding index/
16:01 < dongcarl> andytoshi: ^
16:01 < dongcarl> stevenroose: ah! happy to hear that
16:01 < andytoshi> sure, that's good
16:02 < stevenroose> Oh ok, I was confused because I was reading on github (their ordering) and I saw Global first, not realising the actual total psbt is global + ins + outs
16:03 < dongcarl> no problem
16:19 < dongcarl> andytoshi: pushed fixes
16:28 < andytoshi> almost there
16:29 < andytoshi> one issue is that public keys don't roundtrip
16:29 < andytoshi> which is hard to deal with
16:30 < dongcarl> Hmmm... not sure what best to do for that...
16:30 < andytoshi> the spec actually says nothing about compressed or uncompressed public keys
16:30 < andytoshi> but it seems like all the test vectors have only compressed ones
16:30 < dongcarl> smh...
16:31 < andytoshi> i'll bug andy on #bitcoin-core-dev
16:31 < andytoshi> if we can simply reject uncompressed keys that'd be easiest, but i'm pretty sure we don't want to do that, because the whole point of this spec is interop
16:31 < dongcarl> true
16:31 < andytoshi> meanwhile i did add another comment about your parsing of ChildNumber..
16:31 < dongcarl> yeah lemme fix that
16:38 < andytoshi> also dongcarl and ariard -- i've added you with `Write` permission to the bitcoin_hashes library so that you can approve pull requests. if either of you are comfortable clearing out the review queue there that'd be awesome
16:39 < andytoshi> the algorithmic stuff is easy because there are test vectors and you can't fuck up hash algorithms without breaking every single test vector
16:39 < andytoshi> it's more about API sanity
16:39 < dongcarl> andytoshi: I actually wanted to ask you if you had a process of reviewing PRs
16:39 < dongcarl> I'm guessing you go commit by commit
16:39 < andytoshi> dongcarl: like, how am i spamming you with nits like i have been?
16:39 < andytoshi> yes, commit by commit
16:39 < dongcarl> But are there any tricks?
16:40 < dongcarl> Haha yeah
16:40 < dongcarl> I feel like I'd just gloss over them
16:40 < dongcarl> So I'd like to learn
16:40 < andytoshi> a few ... in this case i'm being a dick, i wrote a much more involved fuzzer than the one in your code and i've been running it while i've been reviewing the schnorr code
16:41 < andytoshi> but more generally, if a single commit is too big to fit in my head and there isn't a good reason for that, i'll ask for it to be split up
16:41 < andytoshi> i read the definitions of all the functions and check that they make sense, have only as much &mut or ownership as they need, that they match whatever docs exist
16:41 < dongcarl> andytoshi: sidebar: review fixed
16:41 < andytoshi> cool, thanks
16:42 < dongcarl> Those are good tips
16:42 < andytoshi> then in rust, i read individual functions and kinda mentally mutation-test them. why is this if-branch here? what if i inverted it?
16:42 < andytoshi> in C i do this too but it's sometimes harder because C is so weakly typed
16:42 < andytoshi> and there are no guarantees about aliasing or mutability
16:43 < dongcarl> Seems like Rust is much easier to review
16:43 < andytoshi> yes, much much easier
16:43 < andytoshi> anyway that's the closest thing to a process .. read individual commits, check APIs, specifically check mut/ownership/pub, check that any types/traits make sense and seem to do One Job
16:44 < andytoshi> then i cargo test every commit, try to write fuzzers for stuff, in C i run things in valgrind, etc
16:46 < dongcarl> Cool cool... when you get the chance could you show me what a "more involved fuzzer" looks like?
16:48 < andytoshi> https://gist.github.com/apoelstra/3acef1288963cf9c8dbc71142032f51a .. ignore the `println!`s, they're just there so i can tell what went wrong
16:48 < andytoshi> but that doesn't work right now because it triggers on uncompressed pubkeys being turned into compressed ones
16:50 < dongcarl> Ah... that roundtrip must've been pretty helpful in finding stuff
16:50 < andytoshi> yep :P
16:50 < andytoshi> the pubkey thing is even a bug in the original spec :)
16:51 < dongcarl> niiiice
16:52 < dongcarl> urgh achow must be in class or sth
16:52 < andytoshi> heh, yeah
16:53 < andytoshi> and he's gonna be super annoyed to come home to this
16:53 < andytoshi> "why the hell are you using uncompressed keys?"
16:54 < dongcarl> Oh I can imagine him saying it in my head lol
17:05 < andytoshi> so, this is a bit annoying .. it's possible though frustrating to deal with uncompressed keys
17:05 < andytoshi> but we have literally no way to deal with hybrid keys
17:05 < andytoshi> so i think we should reject those and list in our "deviations from the spec" that we do
17:06 < dongcarl> yeah I'm happy with that
17:09 < andytoshi> kk. so, keys that start with 0x02 or 0x03 are compressed; 0x04 are uncompressed
17:10 < andytoshi> i think the hybrid prefixes are 5 and 6, but i don't remember and i don't care
17:11 < dongcarl> So I'll just change `impl Deserialize for PublicKey` to only accept `bytes` that start with `0x02` or `0x03`?
17:11 < dongcarl> Oh... we're taking uncompressed too?
17:11 < andytoshi> hmmm
17:12 < andytoshi> i would prefer to take uncompressed keys
17:12 < dongcarl> Sounds like we need to write a wrapper type around PublicKey
17:13 < andytoshi> yeah, i think so
17:13 < andytoshi> we should have a Pubkey to go along with Privkey I guess
17:13 < andytoshi> that'll need to be a separate PR
17:13 < dongcarl> an enum of `Compressed`, `Uncompressed`, `Hybrid` (if we wanna support that)
17:14 < andytoshi> i think not, because there's no WIF format for hybrid keys
17:14 < andytoshi> they're supported in script interpreter but not really in any other part of the ecosystem
17:15 < dongcarl> well I'm glad my PR has made many much-needed changes clearer
17:16  * dongcarl cries for psbt PR
18:14 < stevenroose> andytoshi: there is no method on Sha256dHash to create one from a hex string, right?
18:14 < stevenroose> :|
18:15 < stevenroose> Oh there is https://github.com/rust-bitcoin/rust-bitcoin/pull/168
18:17 < stevenroose> nvm
18:24 < andytoshi> stevenroose: yeah.. though i wish that weren't there, i should've implemented FromStr originally
21:11 -!- HFRadical [~none@185.156.175.43] has joined #rust-bitcoin
21:11 -!- HFRadical [~none@185.156.175.43] has quit [Remote host closed the connection]
21:11 -!- Tralfaz [~none@185.156.175.43] has quit [Ping timeout: 260 seconds]
22:14 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-mwggotwszylbocsc] has quit [Quit: Connection closed for inactivity]
23:02 < ariard> andytoshi: hey seen for bitcoin_hashes, I'm focus on finishing a lenghty PR on channels closing tonight but will allocate times to review it after!
23:12 < andytoshi> ariard: awesome, thanks :) no rush