--- Day changed Mon Aug 13 2018
00:52 < dongcarl> Was thinking that network::serialize::deserialize should error if the cursor is not consumed... Does that make sense?
12:11 < andytoshi> no
12:12 < andytoshi> how would you deserialize a block with that behaviour?
12:13 < BlueMatt> okkkkkkkk
12:13 < BlueMatt> good morning rust-bitcoin
12:13 < BlueMatt> lets get this review show on the road
12:13 < BlueMatt> andytoshi: and requests for where to start?
12:13 < andytoshi> lol
12:15 < andytoshi> so, sipa and i have been working on a subset of script which is designed to be easily analyzable for use in PSBT (specifically there is a simple deterministic way to figure out what pubkeys need to sign, and how to assemble the signatures, and it supports arbitrary and/or/thresholds/whatever and locktimes and hash preimages and checksigs and multisigs) ... so 95% of the talk here has been about
12:15 < andytoshi> that, sorry
12:15 < andytoshi> you can ignore all that
12:15 < BlueMatt> heh, nice
12:15 < BlueMatt> but, yea, wasnt planning on reading scrollback anwyway :p
12:15 < andytoshi> ok :P
12:16 < BlueMatt> there was lots and I wasnt tagged so I figured meh
12:16 < andytoshi> yep
12:16 < andytoshi> i think the best place to start is with the open PRs at https://github.com/rust-bitcoin/bitcoin_hashes
12:16 < BlueMatt> k
12:16 < andytoshi> these i basically just need a concept ACK for .. i'm just implementing hash functions, and i run fuzzers against rust-crypto's output, so it can't be wrong
12:17 < BlueMatt> yea, fair, so just review for "lol, are there any strange branches or overflow bugs"
12:18 < andytoshi> lol yep (and there should be no branches except the dumb HMAC "if the key is too long then hash it" one, and every +/- should be wrapping_add/wrapping_sub)
12:18 < andytoshi> then on the rust-bitcoin side there is https://github.com/rust-bitcoin/rust-bitcoin/pull/113 which updates rust-bitcoin to rust-secp 0.10
12:19 < BlueMatt> ok, cool, I'll try to get to all of that today
12:19 < BlueMatt> gotta get to a few rust-lightning things too, it looks like
12:19 < BlueMatt> plus some mining stuff
12:19 < andytoshi> cool, thanks. no rush, i'm at work today but tomorrow i'm flying to mountain view and will probably be too swamped with in-person work
12:20 < andytoshi> oh, finally there are a couple PRs on https://github.com/rust-bitcoin/rust-secp256k1/pulls .. but just the 64-bit compilation thing and a couple minor things .. and i guess i should release 0.10.1 after those are in
12:20 < BlueMatt> yea, was sad to see 1.0 plans blow up, but they all look api-compatible so no big deal, will take a look at those when I get a chance
12:21 < andytoshi> i think we're pretty much at 1.0 with rust-secp .. but i want to get the liquid functionary upgraded to it first to make sure it's not missing anything big
12:22 < andytoshi> but upgrading rust-secp forces me to upgrade serde, which forces me to upgrade rust-bitcoin and rust-jsonrpc and replace strason with serde_json, so it's a bit of an ordeal
12:22 < BlueMatt> yea, will take a look at updating rust-lightning this week if I can, but def no promises there
12:22 < andytoshi> nice
12:26 < andytoshi> oh, actually for rust-secp 1.0 i think i'd like to go through and make all the no-context things no longer require a context object (initially i'll just create a no-capabilities context, which is almost free, but costs one malloc/free of a couple hundred bytes ... later i can replace it with a static object if i can convince upstream to do that)
12:27 < BlueMatt> yea, that'd be nice...there's probably some tweaks worth making for 1.0
12:27 < BlueMatt> probably best to sit on it while we gather more experience I suppose
12:27 < BlueMatt> I was somehow thinking things would be quiet, but that was probably a dumb conclusion
12:28 < andytoshi> well, for rust-secp things nearly have been
12:28 < andytoshi> for rust-bitcoin we're a couple versions away from 1.0 unfortunately ... but i think i know everything that needs to be done
12:28 < BlueMatt> yea, hence my initial conclusion, but best to let things sit
12:28 < andytoshi> yep
12:28 < BlueMatt> yea, rust-bitcoin still needs some love
12:28 < BlueMatt> but rust-secp getting more experience using the new api and then whittling down any extra useless things woule be nice
12:29 < BlueMatt> one thing that is super annoying is handling all the potential errors in rust-lightning
12:29 < BlueMatt> some of those should go away with 0.10 but if they dont all I want to sit down and discuss which other ones can
12:31 < andytoshi> cool
12:31 < andytoshi> definitely
12:34 < andytoshi> oh, there is one API break actually: some folks opened https://github.com/rust-bitcoin/rust-secp256k1/issues/47 which complains that SecretKey::add_assign takes another secretkey, which (a) adds error paths when converting a hash or whatever your tweak actually is into a SecretKey, (b) prevents tweaking by 0 which is unergonemic
12:34 < BlueMatt> ah, fun
12:35 < BlueMatt> and yay fewer error paths
12:35 < BlueMatt> error paths is /definitely/ one of the most painful secp-0.9 issues
12:35 < andytoshi> agreed. in my trial-run of upgrading the functionary software i eliminated a lot of .unwrap()s
12:48 < BlueMatt> andytoshi: you have wrong comments in some of bitcoin_hashes
12:48 < BlueMatt> eg sha256.rs says "copied from rust-crypto ripemd.rs file"
12:48 < andytoshi> oh, sure, i'll delete that from everywhere except ripemd.rs
12:49 < BlueMatt> well I mean how much did you copy from rust-crypto?
12:49 < BlueMatt> vs rewrite blind?
12:49 < BlueMatt> cause licensing bitcoin_hashes as apache is easier than accidentally violating the license of rust-crypto...
12:49 < andytoshi> i copied ripemd.rs, from the version that i committed
12:49 < andytoshi> then i blindly fixed the macros to compile today (but later i checked and the 2-3 lines i changed were largely identical to later rust-crypto changes)
12:50 < andytoshi> then the sha2 stuff i copied from sipa
12:50 < andytoshi> his code is MIT, but i was betting that he wouldn't sue me..
12:50 < andytoshi> (and i did actually change it substantially from C to rust)
12:50 < BlueMatt> lol ugh...can you get him to say somewhere on a github issue or something that its ok?
12:50 < andytoshi> sure
12:56 < BlueMatt> ouch...io::Write? that kinda sucks, no?
12:56 < andytoshi> does it?
12:56 < BlueMatt> you will never fail but get an io::Result every time?
12:56 < andytoshi> yeah, that's very annoying
12:56 < andytoshi> but that's true of basically everything that you can write to
12:56 < andytoshi> rust just sucks in that respect
12:57 < BlueMatt> I mean we could just not use io::Write?
12:57 < BlueMatt> rust-bitcoin doesnt
12:57 < BlueMatt> errr, rust-crypto
12:57 < andytoshi> yes, and that's the most irritating thing about it
12:57 < BlueMatt> cause this isnt /really/ io
12:57 < andytoshi> i considered rewriting the lib entirely because of that
12:57 < BlueMatt> oh? when have you needed io::Write?
12:57 < andytoshi> whenever you serialize into a hash
12:57 < BlueMatt> ah, hmmm
12:57 < BlueMatt> ok...can implement both?
12:57 < andytoshi> you need special hash-specific code, which currently in rust-bitcoin is inefficient in a ton of places
12:57 < andytoshi> both what?
12:58 < BlueMatt> io::Write and add_data(&[u8])
12:58 < BlueMatt> or whatever
12:58 < andytoshi> oh sure
12:58 < BlueMatt> also, always using the buffer has to be mega ineffecient?
12:59 < andytoshi> i don't think so
12:59 < andytoshi> copying 64 bytes sometimes is basically free
12:59 < andytoshi> and i don't see any alternative
13:00 < BlueMatt> make process_block take a &[u8] that is always 64 bytes in len and do the "do I use the internal buffer" check in write()
13:00 < BlueMatt> thats the normal approach in hash libs, no?
13:00 < BlueMatt> at least that's how the Sha'er in bitcoin core does it, iirc
13:01 < BlueMatt> is it possible in rust to add a like "impl io::Write for HashFunctionTrait" that will just call the no-err-possible write form from HashFunctionTrait?
13:01 < BlueMatt> I mean I guess you can go the other way
13:02 < andytoshi> i'd need to check if i can do that .. the rules for blanket impls are weird
13:02 < BlueMatt> like trait HashWriter : io::Write, InternalNoFailDeclareHashTrait { fn write() { call io::Write } }
13:02 < andytoshi> and i don't understand what the difference between `process_block` taking a 64-byte buffer as an argument, and it using `self.buffer`, is
13:02 < BlueMatt> and then just impl InternalNoFailDeclareHashTrait for Hash {}
13:03 < BlueMatt> well the difference is you can skip the 64-byte-copy if the input is >= 64 bytes in length
13:03 < andytoshi> oh sure
13:03 < andytoshi> yeah i can do that
13:04 < BlueMatt> eg https://github.com/bitcoin/bitcoin/blob/master/src/crypto/ripemd160.cpp#L261
13:04 < BlueMatt> ie just call process_block directly on the input data when you can, and then fall back to self.buffer (and pass a ref to that in) when you cant
13:06 < andytoshi> so, i first need to check if the buffer is nonempty, and if so concatenate the new data to that .. then process 64-byte blocks .. then check if there's anything left to copy into the buffer
13:06 < andytoshi> i'll try this
13:06 < BlueMatt> yes
13:06 < andytoshi> but it may be too many branches and wind up being slower
13:08 < BlueMatt> hmm, yea, in the case of writing lots of little bits into the hasher it may be, but in the common-case hash-64-bytes it may not be
13:10 < BlueMatt> I dunno, and benchmarking it is hard cause for the 64-byte-copy version to be fast you're relying on cache effects that the copy is mostly the read cost and the write is who-cares cause its just read right back and then never read elsewhere
13:15 < andytoshi> current version is faster according to my bechmarks (but we are welll within margin of error)
13:25 < BlueMatt> lol, ok, leave it I guess
13:26 < BlueMatt> oh, wait, change the order, though...
13:26 < BlueMatt> length should be after buffer?
13:26 < BlueMatt> actually, order should be buffer, h, length, I think?
13:27 < andytoshi> i don't understand what you're saying
13:28 < BlueMatt> oh, nvm
13:28 < BlueMatt> well, shouldnt hurt
13:28 < BlueMatt> I'm saying change the order of things in the struct
13:28 < BlueMatt> generally put structs in decreasing-size order
13:29 < BlueMatt> in this case I dont actually know what alignment rust guarantees, but putting the buffer first means its more likely to not straddle a cache line
13:29 < BlueMatt> given the buffer is the length of a normal cache line
16:10 < andytoshi> ah gotcha
16:10 < andytoshi> (sorry, had to step out for a couple hours)
16:11 < andytoshi> IIRC rust makes no guarantees whatsoever about struct representation unless you specify #[repr(C)] or something like that
16:12 < andytoshi> fwiw that change made no perceptible difference either way in my benchmarks ... but ofc the benchmarks will have the sha2 engine in cache constantly anyway
20:18 < andytoshi> BlueMatt: what do you think about making cryptographically inaccessible error paths in rust-secp be panics? (i'm not sure how many of these there are, i suspect a couple)
20:20 < BlueMatt> andytoshi: I'd generally prefer that, I think...obviously I'd like such things get review from other libsecp256k1 folks, but handling such things in rust-lightning is like...really dumb, cause I'm just writing failure paths that cannot be tested...
20:20 < andytoshi> yeah, we have that problem upstream as well (and we do really elaborate things to test ... like we found a similar EC group with the same addition equation but which is super small, so we can exhaustively test every possibility)
20:21 < BlueMatt> yes, I'm aware
20:21 < BlueMatt> I presume upstream asserts?
20:21 < BlueMatt> or does it return failure?
20:21 < andytoshi> it returns failure
20:21 < BlueMatt> ugh
20:21 < andytoshi> yeah
20:21 < BlueMatt> what was the rationale for that?
20:21 < andytoshi> that it's mathematically possible to hit
20:21 < BlueMatt> well then it should assert, no?
20:21 < andytoshi> and typically these conditions eventually derive from untrusted inputs
20:22 < andytoshi> and we didn't want situations where some sha-cracker could kill every app using libsecp
20:22 < BlueMatt> you mean like an invalid key that wasnt tested?
20:22 < andytoshi> no, i mean like hashes that overflow the group order
20:22 < andytoshi> one sec, lemme see if i can find one that gets exposed at the library API level..
20:22 < BlueMatt> oh, is that the only case you're referencing?
20:23 < andytoshi> yeah
20:23 < BlueMatt> hmmmmmm
20:23 < BlueMatt> how many functions is that?
20:24 < andytoshi> checking now .. it might actually be zero at this point
20:24 < andytoshi> i forgot, i already catch errors that the rust API makes impossible
20:24 < BlueMatt> yea, the invalid-pubkey stuff should be safe (now)
20:24 < BlueMatt> but hash-crack should be separate?
20:25 < andytoshi> yeah, sorry, there are no hash-crack errors exposed by rust-secp. never mind
20:25 < BlueMatt> I assume thats cause they're already panics?
20:25 < andytoshi> i think in secp 0.10 every error condition can actually be tested
20:26 < andytoshi> no, i think there are just no impossible conditions exposed by the library. if a signature hash overflows or something we try a new nonce
20:27 < andytoshi> so that's a problem for upstream's test coverage, but users don't care
20:28 < BlueMatt> ah, and hash -> secretkey mappings are already handled by the ::from_slice() api?
20:28 < andytoshi> yep
20:28 < BlueMatt> yea, ok
20:28 < andytoshi> actually let me test this :P
20:31 < andytoshi> ok, we disallow invalid secret keys .. but we do allow the messagehash to be set to 0 or overflew
20:31 < BlueMatt> ah, right, cause thats from_slice
20:31 < BlueMatt> maybe that shouldnt be allowed?
20:32 < andytoshi> probably not. i'm writing a unit test to see what'll happen..
20:34 < andytoshi> cool, the library allows it and (apparently) successfully signs ... which means there are no exposed panics
20:34 < andytoshi> but also this is cryptographically super dangerous
20:34 < BlueMatt> yea, not ideal
20:34 < andytoshi> because signing the zero messagehash exposes your key IIRC
20:34 < andytoshi> ah, no, it doesn't
20:35 < andytoshi> but signatures on the zero messagehash can be forged without knowing the key
20:35 < BlueMatt> yea, shouldnt be allowed, then, imo
20:35 < andytoshi> agreed. i'll forbid zeroes
20:35 < andytoshi> but still allow overflow because that's kinda hard to check for, and harmless?
20:35 < BlueMatt> wait, does it not panic if the messagehash is past the order?
20:35 < andytoshi> apparently not
20:35 < BlueMatt> what does it sign...?
20:35 < andytoshi> good question, i'm checking upstream
20:36 < andytoshi> heh, overflow is just ignored -- it wraps around
20:36 < BlueMatt> ewww
20:36 < andytoshi> yeah.
20:36 < andytoshi> so maybe in rust-secp we should error on Message::from_slice whenever it overflows
20:37 < andytoshi> (which is not that hard, i just have to copy the checks from SecretKey::from_slice)
20:37 < andytoshi> though in practice this should be cryptographically unreachable
20:38 < andytoshi> an alternative is to have the lib depend on bitcoin_hashes, and do the hashing itself
20:38 < andytoshi> though that'll irritate people using blake or whatever
20:38 < BlueMatt> yea, I'd say just keep it as-is and return a Result<> like SecretKey does
20:38 < andytoshi> but like, it's really irritating right now that there's an error path converting a hash to a Message
20:39 < andytoshi> sounds good. best we can do
20:39 < andytoshi> we could also add an _unchecked() variant that panics
20:40 < BlueMatt> yea, it sucks, but documenting "if you're using something that is actually hashed you can unwrap(), if not you're an idiot"
20:40 < andytoshi> yeah, probably the best thing
20:44 < andytoshi> oh, now that we have our own hash lib we can expose the ability to unwrap hashes into [u8; 32]s
20:44 < andytoshi> and add a From<[u8; 32]> to Message which panics on overflow
20:44 < BlueMatt> errr wat
20:45 < andytoshi> erm, i mean, i can do
20:45 < andytoshi> impl<T: Into<[u8; 32]>> From<T> for Message { ... }
20:45 < andytoshi> in libsecp
20:45 < andytoshi> err rust-secp
20:45 < BlueMatt> but then you've re-added the panic?
20:45 < andytoshi> only in the From impl
20:45 < andytoshi> i'll leave the current function which returns an error
20:45 < BlueMatt> yea, but people are just gonna end up using that?
20:46 < andytoshi> if they have access to a [u8; 32] yes
20:46 < andytoshi> not if they're using slices
20:46 < BlueMatt> I mean you can do a From<hashlib::32ByteHash>
20:46 < BlueMatt> errrr....I cant say I'm a fan
20:46 < andytoshi> i can do that, but then i'd be locking in a hash lib
20:47 < BlueMatt> sure but if not its one well-documented unwrap() away so nbd
20:47 < andytoshi> with a From on [u8; 32] the panic would only be accessible by people doing cryptographically invalid things
20:47 < andytoshi> with from_slice() we already have an error return because of length issues, so it's nbd to add another "cryptographically invalid" error path
20:48 < andytoshi> but i'd like a way to convert hashes into messagehashes without unwrapping
20:48 < andytoshi> oh! i can define a trait *in rust-secp* which things can implement if they promise to be 32 byte hashes
20:48 < andytoshi> then i guess bitcoin_hashes can depend on rust-secp (bleh) and impl that..
20:53 < BlueMatt> I mean you can make things an optional dep
20:53 < BlueMatt> but I /really/ dont like taking a [u8; 32]
20:53 < BlueMatt> cause I certainly use a ton of [u8; 32]s for non-hash-results
20:53 < andytoshi> yeah, fair enough
20:53 < andytoshi> ok, sure, i'll take the optional dep route
21:03 < dongcarl> Cleaning up errors in rust-bitcoin... Is https://docs.rs/bitcoin/0.13.2/bitcoin/util/fn.propagate_err.html still needed? Seems like a hack to me and why not just backtrace?
21:03 < andytoshi> what do you mean "why not just backtrace"
21:04 < andytoshi> i do think we should drop that function because it's not very rustic, seems like people just live with the fact that their error displays give no hint of where they came from
21:05 < dongcarl> I'm seeing it used in socket.rs receive_message, and it seems like it just adds the string "receive_message" to the error
21:05 < dongcarl> andytoshi: do you mean information other than the ones given by RUST_BACKTRACE=1 ?
21:06 < andytoshi> yes
21:06 < andytoshi> rust_backtrace=1 is only useful when the library panics
21:07 < andytoshi> but yeah, if `propagate_err` isn't even used we might as well drop it
21:08 < andytoshi> the issue is that if you have a network error like "expected network magic" that winds up being returned through multiple function calls until a user tries to start an application and it fails with "expected network magic" and no other context
21:08 < dongcarl> andytoshi: I think in those cases, we should (wrap it in another error type/adding an error type onto it) instead of adding string information onto it?
21:09 < andytoshi> it is wrapped in another error type
21:09 < andytoshi> the issue is that every error type just passes through to its children when implementing Display etc
21:10 < dongcarl> Hmmm... okay I'll tinker around
21:12 < andytoshi> in liquid we have a `log_try` macro which serves a similar purpose
21:12 < andytoshi> because otherwise there's no way to determine the LOC that an error happened on
21:13 < dongcarl> Ahhhhhhh I see what you're saying!
21:14 < andytoshi> i don't think it's so important because rust-bitcoin no longer does network stuff (spawns long-running threads etc) that would require that kind of logging
21:14 < andytoshi> now it's a bunch of utility functions that, if they fail, the application doesn't really need to know the gory details
21:15 < dongcarl> andytoshi: Cool cool, yeah this Error stuff is kinda tangled into everything... taking a while lol
21:16 < dongcarl> I think `failure` provides nice Backtraces
21:16 < dongcarl> and Context
21:16 < dongcarl> Hopefully it'll get into Rust at some point
21:16 < andytoshi> yeah, that would be nice