--- Log opened Wed Aug 26 00:00:59 2020
01:04 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Quit: jonatack]
01:08 -!- midnight [~midnight@unaffiliated/midnightmagic] has quit [Ping timeout: 272 seconds]
01:31 -!- Dean_Guss [~dean@gateway/tor-sasl/deanguss] has quit [Remote host closed the connection]
01:32 -!- Dean_Guss [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin
01:32 -!- sgeisler [sid356034@gateway/web/irccloud.com/x-hztdwhfrhxkhiibj] has quit [Read error: Connection reset by peer]
01:36 -!- fjahr [sid374480@gateway/web/irccloud.com/x-ljdthgnwgzteqqfh] has quit [Ping timeout: 260 seconds]
01:39 -!- belcher [~belcher@unaffiliated/belcher] has joined #rust-bitcoin
01:39 -!- fjahr [sid374480@gateway/web/irccloud.com/x-fkopmipbioxkvxko] has joined #rust-bitcoin
01:42 < elichai2> andytoshi: we already "broke" the MSRV of secp256k1-sys https://github.com/rust-bitcoin/rust-secp256k1/commit/fd8b3ff572354ecb30cf1c783d393fbb404a5a9c and you can't bump secp256k1 without secp256k1-sys because of https://github.com/rust-bitcoin/rust-secp256k1/pull/208/files#diff-e0934bb6f70a4f8c3baf22a773c14ec7R110 and maybe other changes. and if we already do a major for secp256k1-sys then we should get #216 in?
01:42 < elichai2> although right now there's a lot going on in libsecp so maybe wait more? idk
01:45 -!- sgeisler [sid356034@gateway/web/irccloud.com/x-xovlpgilwcahhtkw] has joined #rust-bitcoin
02:03 -!- jonatack [~jon@37.173.221.225] has joined #rust-bitcoin
03:22 -!- jonatack [~jon@37.173.221.225] has quit [Read error: Connection reset by peer]
05:18 -!- Marjory65Bechtel [~Marjory65@static.57.1.216.95.clients.your-server.de] has joined #rust-bitcoin
06:05 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection]
06:36 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #rust-bitcoin
06:38 -!- midnight [~midnight@unaffiliated/midnightmagic] has joined #rust-bitcoin
06:58 -!- DeanWeen [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin
07:01 -!- Dean_Guss [~dean@gateway/tor-sasl/deanguss] has quit [Ping timeout: 240 seconds]
07:44 -!- DeanWeen [~dean@gateway/tor-sasl/deanguss] has quit [Remote host closed the connection]
07:44 -!- DeanWeen [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin
07:53 < andytoshi> elichai2: we didn't really break the MSRV
07:53 < andytoshi> we provided explicit instructions for compiling with 1.22
07:56 < andytoshi> agree thuogh that we could hold off on updating rust-secp. it doesn't look like anything urgent has been merged there?
08:02 -!- shesek [~shesek@unaffiliated/shesek] has joined #rust-bitcoin
08:09 < BlueMatt> andytoshi: I'd like to see it merged - I'm close to being blocked on wanting the new as_ref() things I added
08:10 < BlueMatt> I admit its probably cause I'm lazy and dont want to find an alternate approach, but still
08:10 < BlueMatt> (though thats totally a minor version that doesnt need an msrv bump)
08:10 < BlueMatt> ariard: looks like https://github.com/rust-bitcoin/rust-lightning/pull/633 doesnt compile?
08:17 < BlueMatt> andytoshi: also, if its possible, would be nice to land https://github.com/rust-bitcoin/rust-bitcoin/pull/461 before shipping a new rust-bitcoin, to expand the API of SigHashCache before we commit to it.
08:37 -!- fiatjaf1 [~fiatjaf@2804:7f2:2a84:b2d3:ea40:f2ff:fe85:d2dc] has quit [Quit: WeeChat 2.9]
08:38 -!- fiatjaf [~fiatjaf@2804:7f2:2a84:b2d3:ea40:f2ff:fe85:d2dc] has joined #rust-bitcoin
08:40 < andytoshi> ah yep i saw that
08:40 < andytoshi> ok sure i can do a new rust-secp then
09:03 -!- Dean_Guss [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin
09:04 -!- DeanWeen [~dean@gateway/tor-sasl/deanguss] has quit [Remote host closed the connection]
09:43 < andytoshi> hmm looks like we need to do a major bump of secp-sys
09:43 < andytoshi> i wonder if we should also update to latest upstream then
09:47 < BlueMatt> probably? anything new or exciting upstream?
09:48 < andytoshi> lots of comment, test, QA improvements
09:48 < andytoshi> improved const-time stuff, new SECURITY.md, benchmarks
09:50 < andytoshi> i *think* nothing API-visible. though it clarifies that the ECDH callback function needs to return 0 or 1 (not "zero or nonzero") which is a bug that we reported
09:50 < BlueMatt> yea, i mean if its mostly qa and constant-timeness, probably good to bump it
09:50 < andytoshi> +1
09:50 < BlueMatt> especially if its not very api-visible, which should make a bump easy
09:50 < andytoshi> tried bumping, it's not building now, hard to tell why
09:51 < andytoshi> oh i see
09:51 < andytoshi> we have to set `ECMULT_GEN_PREC_BITS` now
09:51 < BlueMatt> wasnt there an issue/pr somewhere already with a bump?
09:51 < andytoshi> oh nice
09:51 < andytoshi> yeah but from may :P
09:52 < BlueMatt> heh, right, from some time ago
09:52 < andytoshi> also it bumps secp-sys to 0.1.3 which i think we can't do now, we've merged some PRs which change its API
09:52 < andytoshi> #195 and #199 at least are breaking changes i think
09:53 < andytoshi> elichai2: are you around? can you take 20 minutes to update https://github.com/rust-bitcoin/rust-secp256k1/pull/216 to latest secp, change version bump to 0.2.0 instead of 0.1.3, maybe add a changelog to secp-sys
09:53 < andytoshi> i can ack and merge
09:54 < andytoshi> lol what am i looking at here ... this ECMULT_GEN_PREC_BITS code was added by djb?? in 2015??
09:54 < BlueMatt> whut
09:54 < BlueMatt> uhhh ok
09:55 < andytoshi> i think this #define used to be set automatically and something changed to make it no longer automatic
09:56 < BlueMatt> ah
09:57 < andytoshi> hmm, maybe not, it looks like the automatic setting code is still there and has been untouched since 2015
09:57 < andytoshi> so i wonder what changed for us
09:57 < BlueMatt> well, maybe we're importing in a different order or so?
09:58 < andytoshi> we're setting all these #defines manually rather than using autotools
09:58 < andytoshi> so who knows what weird cascading logic might have changed
09:58 < BlueMatt> right.
10:07 -!- vindard [~vindard@190.83.165.233] has joined #rust-bitcoin
10:16 < elichai2> In on my phone, I think pieter added it after he changed one of the precomputed things
10:16 < elichai2> *I'm
10:18 < andytoshi> heh kk i'll keep loking
10:30 < andytoshi> oh https://github.com/bitcoin-core/secp256k1/pull/793 is cool, it lets us drop our 64-bit detection code
10:30 -!- belcher_ [~belcher@unaffiliated/belcher] has joined #rust-bitcoin
10:32 < andytoshi> lol it was djb in 2015, but just merged https://github.com/bitcoin-core/secp256k1/pull/337
10:32 < andytoshi> that's what i get for using `git blame`, it uses the commit date not the date that the commit was merged
10:34 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 265 seconds]
10:40 < andytoshi> ok https://github.com/rust-bitcoin/rust-secp256k1/pull/229
10:41 < andytoshi> oh i should get 220 in first
10:44 < andytoshi> oh lol https://github.com/rust-bitcoin/rust-secp256k1/issues/214 talks about the ECMULT_GEN_PREC_BITS thing
10:44 < andytoshi> i really gotta stop ignoring rust-secp github notifications, they're not exactly high-volume..
10:47 < BlueMatt> lol, even i see them and read the title
10:47 < andytoshi> heh i'm sure i read it
10:47 < andytoshi> but immediately forgot it
11:04 -!- vindard [~vindard@190.83.165.233] has quit [Remote host closed the connection]
11:07 -!- vindard [~vindard@190.83.165.233] has joined #rust-bitcoin
11:07 < andytoshi> ok updated PR
11:54 < BlueMatt> ariard: sorry, 633 now also needs rebase :/
11:58 < ariard> BlueMatt: I'm not I'm on it
11:58 < BlueMatt> ?
11:58 < ariard> sorry spent a bit of time, catching log for testing isn't friendly
11:58 < BlueMatt> E_NO_PARSE
11:58 < ariard> local_set_to_counterparty_delay ? not sure if it's better
11:58 < BlueMatt> its mean its more verbose
11:59 < BlueMatt> but, like, when i read it, yet again I managed to confuse myself
11:59 < BlueMatt> counterparty_to_self_delay sounds like the delay to counterparty, not the delay counterparty set for to_us outputs
11:59 < ariard> just pushed a test-passing branch
11:59 < ariard> why not pick up broadcaster/countersignatory here too ?
12:00 < BlueMatt> you mean like local_broadcast_to_self_delay?
12:00 < BlueMatt> wait, no, because this isnt per-broadcaster
12:00 < BlueMatt> oh, yes it is
12:00 < BlueMatt> damn it
12:00 < ariard> it is
12:01 < ariard> ah no doesn't work
12:01 -!- Marjory65Bechtel [~Marjory65@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 260 seconds]
12:01 < ariard> because we store the as picked up by counterparty self_delay
12:01 < BlueMatt> it could be like locally_broadcast_to_self_delay
12:01 < ariard> pfffffff
12:01 < BlueMatt> lol
12:01 < ariard> I don't know
12:02 < BlueMatt> i think we need to give up and just make all of our variables full sentences
12:02 < ariard> ahahahaah
12:02 < ariard> let me open a pr against bolt3
12:02 < BlueMatt> i_think_we_need_to_give_up_and_just_make_all_of_our_variables_full_sentences
12:02 < ariard> this_is_the_self_delay_picked_up_either_by_us_the_local_or_them_the_counterparty_but_not_always
12:02 < BlueMatt> anyway, lets get our own nomenclature squared away, bolts we can update to matc
12:03 < ariard> I don't care about the bolt3 at this point, I tried once ago
12:03 < BlueMatt> yea, i think folks are busy with things other than tweaking wording lol
12:03 < ariard> but people didn't see the issue they are enumerating keys never generated
12:03 < ariard> sadly wording as been source of bugs :/
12:04 < BlueMatt> yes.
12:04 < ariard> otherwise that would be useless to spend that much time on it
12:04 < BlueMatt> anyway, I'm open to whatever you want, as long as you add at least one more word to the variable name :p
12:04 < ariard> matt's_counterparty_self_delay
12:04 < BlueMatt> because I dont think we'll come up with anything sane until then, and by "sane" I mean "I'd catch it in review if we screwed it up again"
12:05 < ariard> counerparty_selected_delay/local_selected_delay ?
12:05 < ariard> yes kinda my same standard of sanity, can we catchup this in review
12:06 < ariard> I'm going forward with *_selected_delay
12:06 < BlueMatt> I'm ok with selected
12:06 < BlueMatt> I'll still confuse myself, but at least slightly less so
12:11 < BlueMatt> ariard: ping me when you get it done and hopefully we can merge this damn thing today.
12:31 < elichai2> andytoshi: back at the computer, still need anything?
12:31 < BlueMatt> elichai2: probably review?
12:32 < ariard> BlueMatt: done, add the diff in its own commits, and push the renaming in Channel, as it's interfacing with keysinterface
12:33 < BlueMatt> elichai2: isn't there something where rust says "all memory allocations are min aligned to X, even though std::mem::align_of() says 1?
12:33 < BlueMatt> i thought i saw that somewhere
12:33 < BlueMatt> like, Box/heap allocations.
12:33 < elichai2> I've never heard of it. the rust allocator accepts a alignment as input
12:33 < BlueMatt> hmm, damn
12:33 < BlueMatt> also, wtf secp256k1::Signature is align-1
12:34 < elichai2> why is that wtf?
12:34 < elichai2> you do have things like https://doc.rust-lang.org/std/primitive.slice.html#method.align_to
12:34 < BlueMatt> cause its a 64 byte array...
12:34 < BlueMatt> i mean, sure, its a byte array, but, like, align the damn thing
12:35 < elichai2> hehe yeah, well arrays are aligned the same as their members and u8 is 1 align
12:35 < BlueMatt> (also, I presume libsecp will be faster if its aligned, but maybe only if the compiler choses to check for it)
12:36 < elichai2> libsecp starts by memcpying it anyway
12:36 < BlueMatt> oof'd
12:36 < elichai2> arguably a const size memcpy of aligned might be faster
12:36 < BlueMatt> but, hey, then its *def* faster to memcpy :)
12:37 < BlueMatt> i think most memcpy things are compiled as a) check if we can simd cause aligned, b) copy with simd, c) fallback
12:37 < BlueMatt> but maybe it depends on size
12:38 < elichai2> well if the size is known at compile time it will almost never compile into a memcpy call anyway
12:38 < elichai2> unless the size is ~>200bytes or maybe even 500, don't remember
12:38 < andytoshi> hi elichai2 are you a rust-secp maintainer?
12:38 < BlueMatt> right, but my point stands
12:38 < andytoshi> can you ack https://github.com/rust-bitcoin/rust-secp256k1/pull/229
12:39 < andytoshi> i'll follow up with a PR bumping rust-secp itself
12:39 < elichai2> andytoshi: I don't think so, only rust-bitcoin. but I'm running the same now and comparing
12:39 < BlueMatt> well lets fix that!
12:39 < andytoshi> lol yeah, one sec
12:39 < elichai2> also I had some commits deprecating privkey for seckey if you want to cherry-pick those (altough the deprecated from value might be out of date)
12:40 < andytoshi> oh right, lemme take a look
12:40 < BlueMatt> ariard: oh, wait, you want to drop the "to_self" part of the name?
12:41 < ariard> yes it's confusing
12:41 < BlueMatt> I'd ideally like to leave it there, cause just "counterparty_selected_delay" could apply to several things
12:41 < ariard> _csv_ ?
12:41 < BlueMatt> but its only for payment
12:41 < BlueMatt> csv could also imply htlc or some shit
12:41 < ariard> right and htlc outputs
12:41 < ariard> ah how much LN knowldege do we assume on the reader
12:41 < BlueMatt> oh, right, maybe contest_delay?
12:42 < ariard> it's well known csv timelocks are the justice ones
12:42 < BlueMatt> assume me - knowledgeable, but with a terrible memory :)
12:42 < ariard> I like it, counterparty_selected_contest_delay
12:42 < BlueMatt> sure, go with that
12:42 < BlueMatt> sorry lol
12:43 < andytoshi> elichai2: i don't think there's a need to deprecate if these are just renames
12:43 < ariard> lol naming is hard they told so
12:43 < elichai2> andytoshi: it says they're deprecated https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L601
12:43 < andytoshi> sure but that's because upstream has no versioning
12:44 < andytoshi> in rust as long as we roll this into a major revision i think it's ok
12:44 < andytoshi> i mean, up to you
12:44 < andytoshi> i guess there's no harm in deprecating and it's a bit kind to uses
12:44 < andytoshi> users
12:45 < elichai2> yeah it's mostly to warn users that this might get removed in the future
12:46 < elichai2> I run `./vendor-libsecp.sh depend/ 0_2_0 670cdd3f8be25f81472b2d16dcd228b0d24a5c45` and I get a few discrepancies with your PR for some reason
12:46 < andytoshi> kk one sec
12:47 < andytoshi> ok i cherry-picked and pushed your deprecation commit
12:47 < andytoshi> when i run that `vendor-libsecp.sh` line i get no diff
12:47 < elichai2> https://pastebin.com/raw/gTVB3CDK
12:47 < elichai2> weird
12:48 < andytoshi> :(
12:48 < elichai2> where did these files come from? https://github.com/rust-bitcoin/rust-secp256k1/blob/1a9aece312f180f14287f08751b55eb82169292a/secp256k1-sys/depend/secp256k1/src/secp256k1.c.orig
12:48 < elichai2> the *.orig
12:48 < ariard> BlueMatt: done
12:49 < elichai2> andytoshi: wait, maybe my master is old
12:49 < elichai2> and the vendor script has changed
12:49 < andytoshi> elichai2: oh, those files may have been spare files that were laying around in my repo
12:49 < andytoshi> and when i did `git add .` on the depend/ directory they got caught up in it
12:50 < andytoshi> ok hmm, when i first delete depend/* and then revendor i get a different result
12:50 < andytoshi> which is really weird, since the revendor script has a rm -rf in it
12:51 < elichai2> that's weird
12:53 < andytoshi> oh, lol, i get differetn results just because depend/ has patchfiles in it that the revendor script needs
12:53 < andytoshi> i think my code is corret
12:53 < andytoshi> and you need to update to the latest revendoring script
12:57 < elichai2> andytoshi: It looks like i'm latest, where does the `.orig` files come from in your PR?
12:58 < elichai2> I can't see that in the vendor script
12:58 < elichai2> *find
12:59 < elichai2> also in my cherry-picked commit changed the `since = "0.1.3"`` to `since = "0.2.2"` 😅
13:00 < elichai2> there's also https://github.com/rust-bitcoin/rust-secp256k1/pull/216/commits/89f08e1ef11ebb45a9187566e481d94497b62470 to prevent warnings in the project after we deprecate
13:01 < andytoshi> elichai2: yes, i'll cherry-pick the other commit in a followup PR that bumps rust-secp
13:01 < elichai2> +1
13:02 < andytoshi> also lol, i'll fix 0.2.2 to be 0.1.2
13:02 < andytoshi> or 0.2.0
13:03 < andytoshi> as for the .orig files, i don't know, they just appear
13:03 < andytoshi> i think my copy of `patch` has different default behaviour than yours
13:05 < andytoshi> trying again with --posix added to the patch lines
13:06 < andytoshi> hmm still no diff
13:06 < elichai2> oh your think the actual `patch` binary is different?
13:06 < andytoshi> yeah
13:07 < elichai2> waitt
13:07 < andytoshi> tried with `-V never` .. still the .orig files are appearing
13:07 < elichai2> fuck
13:08 < elichai2> damn it
13:08 < elichai2> I used `git diff origin/pr/229 --stat`
13:08 < elichai2> which only checks tracked files
13:08 < ariard> BlueMatt: updated with last nits
13:08 < andytoshi> oh lol
13:08 < elichai2> all the new files in libsecp / the vendored script weren't tracked
13:09 < elichai2> approved
13:09 < elichai2> (I think I still don't have permissions though 😅 )
13:10 < andytoshi> elichai2: sent you a invite
13:10 < andytoshi> can you accept and re-ack
13:11 < elichai2> done. Thanks :D
13:12 < andytoshi> dope. let's see how travis does
13:16 < elichai2> if there's anything else tag me
13:19 < andytoshi> will do. probably in 15-20 mins i'll have another rust-secp PR
13:20 < andytoshi> lol and then another, bumping the MSRV to 1.29 for rust-secp
13:31 < ariard> BlueMatt: hmmmm #667 broke a bit the distributed watchtower infra for which we don't have coverage : https://github.com/rust-bitcoin/rust-lightning/pull/667#discussion_r477570428
13:31 < BlueMatt> wait, what exactly does that break?
13:32 < BlueMatt> if we decide to broadcast the state there, we should refuse to accept future updates.
13:32 < BlueMatt> which, I thought, is what local_tx_signed effectively did.
13:32 < ariard> if you have concurrent watchtower, one of them can accept the upgrade
13:32 < ariard> and you not due to having broadcast
13:33 < BlueMatt> sure, but if a majority of your concurrent watchtowers accept an update after one watchtower has braodcast, you're screwed.
13:33 < ariard> but you want this instance to claim against latest state in case of confirmation of this one
13:33 < ariard> depends which one confirms
13:34 < BlueMatt> sure, but you're screwed if you accept the update and the one that is currently in mempools confirms
13:34 < BlueMatt> which...is obviously not ok
13:34 < ariard> you can have after the fact CPFP
13:35 < BlueMatt> thats still not ok...you've now revoked a commitment transaction currently in mempools
13:35 < BlueMatt> whether you can maybe race and make it not confirm is beside the point
13:35 < ariard> yes so why do we have this big comment there ?
13:35 < BlueMatt> which big comment?
13:36  * BlueMatt -> brb
13:36 < ariard> "We re NOT robust again this scenario right now but we should consider it"
13:36 < ariard> in provide_latest_local_commitment_tx_info
13:39 < ariard> I remember the exact scenario, watchtower_A sees block 100, broadcast state X, reject update Y
13:39 < ariard> watchtower_B accept update Y, sees block 100, broadcast state Y
13:40 < ariard> state Y confirms but you want watchtower_A to access state Y to claim outputs
13:41 < ariard> And update Y should be rejected by your watchtower coordinator, not releasing secret X offchain
13:41 < ariard> so yes this makes sense, and I think #667 break this
13:42 < andytoshi> elichai2: i need a re-ack on #229
13:42 < andytoshi> i had to exdit the last commit because there was a conflict between the "add negate functions" PR and your "rename privkey funcitons to seckey" commit
13:42 < BlueMatt> ariard: I'll look in a bit and get back to you...gotta help make dinner atm
13:43 < ariard> this should be documentted in #604
13:43 < ariard> ssure, will fix your nits in the meanwhile :)
13:44 -!- Dean_Guss [~dean@gateway/tor-sasl/deanguss] has quit [Remote host closed the connection]
13:44 < elichai2> andytoshi: done
13:45 < andytoshi> thanks
13:55 < andytoshi> loll elichai2 again, i made a typo
13:55 < andytoshi> secp2561k instead of secp256k1
13:55 < andytoshi> you might wanna wait for travis before acking this one :P
13:55 < elichai2> lol
14:07 < BlueMatt> ariard: right, there's a bug there, but I dont think having watchtower_B reject state X makes it worse
14:09 < BlueMatt> if anything it makes it more correct because watchtower_B should absolutely in no case ever accept state Y after broadcasting X
14:10 < BlueMatt> i think, ultimately, the only solution is that you need some kind of either consensus around when to connect a block across your watchtowers or you need consensus before broadcasting - eg if B decides to broadcast, dont broadcast until you have a majority....if the majority then later agrees to an update, you'd need to reset the state.
14:10 < BlueMatt> but in any case B still shouldnt accept the update
14:16 < andytoshi> elichai2: awesome, merged
14:17 < andytoshi> now we have this one https://github.com/rust-bitcoin/rust-secp256k1/pull/230
14:17 < elichai2> I think we can remove a bunch of stuff from the build.rs now (endianess etc) because real_or_random removed them from autotools
14:18 < andytoshi> oh, nice
14:18 < andytoshi> i did remove the 32/64 bit stuff
14:18 < ariard> BlueMatt: what do you mean by watchtower reject state X ? that's the state they both have already accepted before block reception phase
14:19 < andytoshi> elichai2: other than endianness is there anything we can remove ?
14:19 < BlueMatt> ariard: errr, right, Y
14:20 < ariard> BlueMatt: yes but the feature is about watchtower_A _storing_ Y to claim it in case of confirmation
14:21 < BlueMatt> if your watchtowers reject a state, you will force-close the channel, so it cannot appear on-chain.
14:21 < ariard> I don't think that's a great model to assume interactivity between your watchtowers for security broadcast, you should assume they have to operate independently
14:21 < ariard> what state cannot appear on-chain ?
14:21 < BlueMatt> you cant? if you cant get a confirmation that your watchtowers have the latest state you should never, ever accept the state
14:21 < BlueMatt> ie send the new commitment_signed to your peer.
14:22 < ariard> I think there is a difference between accepting a state, and actually storing-for-claim-in-case-of-concurrent-broadcast
14:22 < ariard> I'm talking about watchtower accceptance which is different from offchain acceptance
14:23 < ariard> obviously offchain should reject it
14:23 < BlueMatt> sure, but I think its the only model that really makes sense - when we get an update, we have to store it, so wait until we have confirmation that it has been stored before moving forward. that has to happen either way.
14:23 < ariard> but between not-coordinated watchtowers you can't be sure of updates/block reception order
14:23 < BlueMatt> sure, but you can wait for them :)
14:24 < BlueMatt> still, what about in the revocation case?
14:24 < ariard> and with these constraints we should provide the highest availability we can
14:24 < BlueMatt> you can never send an RAA message if there's a chance one of your watchtowers broadcasted a state already
14:24 < ariard> can you detail the revocation case ?
14:24 < BlueMatt> so sending the RAA message has to be blocked until we know the watchtowers are ready for it.
14:25 < ariard> yes we already agree on this months ago in this chan and that's the reason the state is rejected by monitor
14:25 < ariard> in provided_last_commitment_update thing
14:26 < BlueMatt> right, maybe I'm not understanding the disagreement here, but thats exactly the case in 667 that you pointed out
14:26 < BlueMatt> we have to set local_tx_signed to true because we broadcasted a commitment tx
14:26 < ariard> I agree, never release secret before consensus of update acceptance by your watchtower
14:26 < BlueMatt> local_tx_signed is the thing that prevents us from accepting a new update
14:26 < ariard> right we were not before because of this storage feature
14:27 < BlueMatt> wait, so we agree that that change in 667 is good?
14:27 < ariard> yes there is difference between accepting the update (sending back true to offchain) and storing the update (sending back false to offchain)
14:27 < ariard> no it's breaking the "storing the update to claim" thing
14:27 < BlueMatt> but why do we need to store it?
14:27 < BlueMatt> we're rejecting it
14:27 < BlueMatt> we wont send the commitment_signed message
14:28 < BlueMatt> are you proposing a model where we dont wait for channel monitors before sending a commitment_signed (but we do for revoke_and_acks?)
14:28 < ariard> but it has been accepted by your watchtower Bob, and Bob may receive a block triggering the broadcast
14:28 < BlueMatt> right, but the fix for that is having consensus between watchtowers before broadcasting
14:28 < ariard> so both state X and Y are not-revoked and can be broadcast by different watchtowrers
14:28 < BlueMatt> I'm not sure how you avoid that.
14:29 < ariard> why do you need consensus between watchtowers if they can just _store_ the update (but never sign it)
14:29 < BlueMatt> I mean....they could store it, but why bother storing it if you're never going to sign it :)
14:30 < BlueMatt> its not useful to store it?
14:30 < BlueMatt> <BlueMatt> are you proposing a model where we dont wait for channel monitors before sending a commitment_signed (but we do for revoke_and_acks?)
14:30 < BlueMatt> we clearly have different models *somewhere*, question is where :p
14:30 < ariard> you have state X broadcasted by watchtowers A and state Y broadcasted by watchtowers B, you want A and B ablet o claim X and Y, whichever confirms
14:30 < ariard> it's not for signing but claiming the HTLC/balance outputs
14:31 < ariard> ahahah I think we agree, we haven't just yet write down the model somewhere to remember it everytime we touch this code
14:31 < ariard> kinda the endgoal of 604
14:31 < BlueMatt> so you're saying that instead of getting majority-of-watchtowers to sign+broadcast, you get all-watchtowers to agree that they havent braodcast for a revoke_and_ack, and then you can send commitment_signed without waiting for channel monitors?
14:32 < BlueMatt> I agree thats a possible model, though I dont think we currently expose enough information for a user to implement that
14:32 < ariard> Before to expose, I need to write down and add documentation comment
14:33 < BlueMatt> I mean we still disagree, I think? I still think we *should* set local_tx_signed there :p
14:33 < ariard> No we need to wait for consensus of watchtowers for both accepting the update offchain and sending the commitment_signed
14:33 < andytoshi> lol the rust-secp version bump PR failed because the wasm code ICE'd the rust compiler
14:33 < BlueMatt> andytoshi: whut? how...is it a different rustc?
14:33 < ariard> at least for commitment_signed you can only have a majority, that's not insecure per se
14:34 < BlueMatt> ariard: I was thinking of a model that is majority-of-watchtowers for anything...in that case, we want to reject new udpates after we broadcast the latest state
14:34 < andytoshi> BlueMatt: my guess is yeah, we just merged your "fix wasm" PR which was a bit old
14:34 < andytoshi> trying to reproduce locally
14:34 < BlueMatt> because then if you have a majority of monitors which have local_tx_signed set there, then they'll reject the commitment_signed and then we wont need to store anything
14:34 < ariard> BlueMatt: majority-of-watchtowers sounds insecure for offchain update acceptance
14:35 < BlueMatt> on the other hand, if you have less than majority, then the non-majority will accept the new update and the non-majority will not broadcast anyway
14:35 < BlueMatt> ariard: I dont believe so, as long as you require a majority for broadcasting as well.
14:35 < ariard> I think I understand your point local_signed_tx is the right name but I think the check isn't at the rigt place
14:36 < ariard> ah yes pre-667 name should have been local_signed_tx_as_required_by_offchain
14:36 < ariard> now it's local_signed_tx_as_triggeted_by_monitor
14:36 < BlueMatt> you're saying we should instead reject revoke_and_ack, not new_local_commitment?
14:36 < BlueMatt> ie reject latest_remote_commitment_tx_info not latest_local?
14:37 < ariard> so it was making sense to reject up directly update from offchain, they were inconsistent
14:37 < ariard> I think so
14:37 < elichai2> andytoshi: the int128 but I think you did that already?
14:38 < BlueMatt> before 667, if a single monitor decided to broadcast, and channel hadn't seen the block yet, then channel could revoke the now-broadcast state
14:38 < BlueMatt> I think before 667 we had an actually serious bug because we didnt have that set there
14:38 < BlueMatt> i mean, highly unlikely race, but still
14:38 < ariard> I think you right that's scary
14:39 < BlueMatt> (and, in fact, its not an issue because channel and channelmonitor would have decided to go on chain at the same time)
14:39 < BlueMatt> but, like, its not exactly a robust design
14:39 < BlueMatt> and, of course, the whole point of 667 is that only channelmonitor decides to go on chain, not channel anymore.
14:39 < ariard> no it would have been rejected by onchain_tx_handler.provide_latest_local_tx
14:40 < elichai2> wtf, the compiler just ICEd https://travis-ci.org/github/rust-bitcoin/rust-secp256k1/jobs/721488293#L2039
14:40 < ariard> so 667 didn't solve the bug
14:40 < BlueMatt> ariard: right, but of course only because channel *also* decided to go on chain :)
14:41 < BlueMatt> oh, you mean that we could still revoke it....hmm, I dont think so? We can only revoke a previous_local_state, not current_local_state
14:41 < ariard> BlueMatt: are you sure, we have another big comment in OnchainTxHandler::provide_latest_local_tx
14:41 < andytoshi> elichai2: lol, yeah, i'm trying to repro locally
14:41 < andytoshi> but having some trouble, i have clang-10 but not clang-9
14:42 < ariard> channel monitor doesn't revoke it only accept/reject revoked based on the knowledge of past local state signature
14:42 < ariard> s/revoked/RAA update/g
14:42 < BlueMatt> ariard: well I wouldnt object to *also* rejecting new remote commitment tx :p
14:43 < BlueMatt> oh, wait, no, i have it wrong, no thats right
14:43 < ariard> right, this make sense you shouldn't have something like a new remote commitment if you previously fail to RAA
14:43 < BlueMatt> right, so we reject new local commitment tx (ie things where we'd revoke the previous one)
14:43 < andytoshi> lol now i'm getting "permission denied", wtf is wasm trying to do
14:43 < ariard> like don't send sigs if you local update fail
14:44 < ariard> yes we reject new local commitment but we store it
14:44 < BlueMatt> andytoshi: lol what the fuck kinda ICE.....open upstream bug?
14:44 < ariard> and we _never_ sign it
14:44 < andytoshi> BlueMatt: not sure yet https://travis-ci.org/github/rust-bitcoin/rust-secp256k1/jobs/721488293 still trying to reproduce
14:44 < BlueMatt> andytoshi: also maybe upstream is using more from libc than previously?
14:44 < andytoshi> maaybe. i'm more inclined to think they're using less
14:45 < andytoshi> though there was a pile of valgrind instrumentation stuff added (but that should all be disabled at compile time)
14:45 < BlueMatt> ariard: I'm super confused why you keep talking about storing new local commitment txn - we reject them wholesale and dont store them, but thats ok - if we were ready to revoke a local tx, it would be previous_local_commitment_tx, and we only ever sign current_local_commitment_tx
14:46 < BlueMatt> andytoshi: is it reproducible on travis? Add a RUST_BACKTRACE=1 and file a bug report, at least?
14:47 < ariard> you store new local commitment txn in case of one your buddy watchtower did accept it due not being on tip
14:47 < elichai2> andytoshi: try to restart
14:47 < andytoshi> kk restratng
14:47 < BlueMatt> ariard: right, I dont think we should :p
14:48 < elichai2> this might be a one off CMB flipping bits kind of thing https://github.com/rust-lang/rust/issues/68132
14:48 < BlueMatt> andytoshi: maybe also add a RUST_BACKTRACE to the travis script so that we have a backtrace in the future if we ever hit it again
14:48 < ariard> Note, AFAICT, that's a super unlikely scenario you need a) distributed watchtowers on different block infra b) a HTLC-timeout c) a bad block propogation event
14:48 < andytoshi> BlueMatt: i don't know that we can since it's a cargo-mediated rustc call
14:48 < andytoshi> but i'll try
14:48 < andytoshi> wtf where is my travis "restart" button
14:48 < BlueMatt> andytoshi: export? I dunno...
14:48 < ariard> why you think we shouldn't ? You reduce your "claiming coverage"
14:49 < elichai2> andytoshi: I restarted for you now :)
14:49 < BlueMatt> ariard: hmm? I disagree, its a race, and across remote infrastructure so I figure its totally plausible to hit it.
14:49 < andytoshi> lol thank you el	
14:49 < elichai2> it looks like it failed while installing wasm-pack btw
14:49 < andytoshi> but idk why i can't do it..
14:49 < BlueMatt> ariard: yes, agreed, see my note above there's an alternative model where you require all watchtowers ack for an update
14:49 < elichai2> here: https://travis-ci.org/github/rust-bitcoin/rust-secp256k1/jobs/721488293 you don't have "Cancel Job" at the top right? (right under "Move options")
14:50 < BlueMatt> ariard: but even in that model I think its right to set that bool - any single watchtower can broadcast, but then in that case every watchtower must agree they did *not* broadcast before any revoke_and_ack/local_commitment_tx_update
14:50 < ariard> BlueMatt: yes, agreed maybe those kind of checks should be at the higher level to let either implement consensus or quorum style of update
14:51 < BlueMatt> ariard: hmm, can you describe a concrete model where the code on master isn't correct?
14:51 < BlueMatt> ariard: I think there's a few potential models, but I think the code is correct for any of them (though they all need test/fuzz, really coverage)
14:51 < ariard> BlueMatt: https://github.com/rust-bitcoin/rust-lightning/issues/604#issuecomment-681116016
14:51 < elichai2> andytoshi: also idk if you followed the whole safegcd PR but we'll soon get rid of all the libgmp comments and bullshit https://github.com/rust-bitcoin/rust-secp256k1/blob/master/secp256k1-sys/build.rs#L43
14:52 < BlueMatt> ariard: right, I dont think the model works there, I think you either need majority-for-everything, or all-for-update
14:52 < ariard> BlueMatt: i see your model but you need 2 phases : 1) we accept update 2) you need can process block based on this update
14:52 < BlueMatt> ariard: ie can you describe the quorum model for that comment
14:53 < ariard> it's less a diffenrence between consensus-vs-quorum but more about round-trips between watchtowers and coordinator
14:54 < ariard> in the quorum model, watchtower A sees block 100, broadcast state X, rejects state Y
14:54 < ariard> and send back "I rejected state Y" to coordinator
14:54 < ariard> watchtowr B, receive state Y, stage state Y, send back "I stage state Y" to coordinator
14:54 < andytoshi> elichai2: yeah i've been following safegcd from a distance
14:55 < andytoshi> i.e. talking to tim and pieter on the phone about it from time to time :P
14:55 < ariard> coordinator seeing absence of consensus (or quorum) thus send back to watchtower B disregards state Y
14:56 < ariard> BlueMatt: it's not consensus-vs-quorum but more if you want a 2-phase commit protocol between coordinator and watchtowers
14:57 < elichai2> andytoshi: lol :D
14:57 < ariard> and I don't think 2-phase commit protocol is great for routing nodes, you bear more round-trips with your watchtowers before to relay HTLC forward
14:58 < elichai2> it ICEd again :(
14:58 < BlueMatt> ariard: I'm not sure how you avoid a race condition where you broadcast two revoked txn in that model, though? like, you have to get *something* more than async to update to a new local commitment tx because you may revoke the previous one, so it cannot have been broadcast
14:59 < BlueMatt> ariard: right, for a routing node you probably cant have a watchtower, or at least not have one that can auto-broadcast (which is also doable) and then just update it async and hope its up to date when its needed.
15:00 < ariard> BlueMatt: yes for a quorum you need a 2-phase commit protocols where watchtowers are not allowed to broadcast before receiving a ACK from coordinator
15:00 < ariard> coordinator --> receive_update
15:02 < ariard> BlueMatt: hmmmm I think quorum is just unsafe
15:03 < ariard> you need explicit acknowledgement of every watchtower before accepting state forward offchain
15:03 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Quit: Leaving]
15:04 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #rust-bitcoin
15:04 < ariard> but you can have 2-phase commit protocol to solve https://github.com/rust-bitcoin/rust-lightning/issues/604#issuecomment-681116016
15:05 < andytoshi> elichai2: lol ok, i'll try to add a rust_backtrace to it
15:08 < ariard> BlueMatt: with regards to revocation you need consensus, with regards to broadcast due to block reception, a majority is secure
15:09 < BlueMatt> ariard: sure, you need two-phase commit for consensus, but I don't really get why thats important here
15:10 < elichai2> andytoshi: another option would be clearing the cache, we cache the install artifacts of wasm-pack
15:10 < BlueMatt> ariard: my understanding of a two-phase commit here is that you call all the monitors with a new update, then you all either see consensus and store the newly-updated monitor or you dont and you load the previous one from disk
15:11 < ariard> BlueMatt: to avoid either state X or Y being concurrent broadcast
15:12 < ariard> BlueMatt: are monitors allowed to use the new update, before the coordinator sees the consensus ?
15:12 < BlueMatt> ariard: no, and, specifically, the channel is not allowed to send a confirmation of the new update to its counterparty until then.
15:12 < BlueMatt> (consensus could mean either majority or all, depending on the model, though)
15:12 < ariard> BlueMatt: and what if we do a blockchain between watchtowers and they have to commit inside every broadcast they do :P ?
15:13 < BlueMatt> lol
15:13 < BlueMatt> anyway, I still dont fully understand the model you're thinking of where the code on master is broken
15:13 -!- Florine21Barton [~Florine21@static.57.1.216.95.clients.your-server.de] has joined #rust-bitcoin
15:13 < BlueMatt> I still think its right for any of the models that work :/
15:14 < ariard> BlueMatt: okay but you still have the case where one wachtower recieve the authorization and the other not and they receive blocks ?
15:17 < ariard> BlueMatt: let me write a test case to illusrate it, that's the only way forward I guess :)
15:18 -!- Florine21Barton [~Florine21@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 240 seconds]
15:21 < andytoshi> elichai2: lol now what are the errors we're getting
15:22 < andytoshi> oh woops
15:22 < andytoshi> i committed crate-type = ["cdylib", "rlib"]
15:22 < andytoshi> 
15:23 < elichai2> I opened a Zulip thread on this, hopefully some compiler dev will swoop in soon
15:24 < ariard> BlueMatt: update naming nomenclature, but don't re-added the local_ prefix in Channel as I think we agree with Jeff that counterparty prefix on the other guy data was clear enough
15:24 < BlueMatt> ariard: yea, a test-case would be good. I still dont understand what your exact model is here :(
15:24 < BlueMatt> ariard: hmm, I really hate having no-prefix - it just seems really really really ripe for missing it in review
15:25 < ariard> yeah but I want to get ride off of local_
15:25 < BlueMatt> its so easy to say "oh, yea, thats right, it says the right thing" in channel or otherwise
15:25 < BlueMatt> i mean feel free to replace it....i dont love local_, but it needs *something*
15:25  * BlueMatt -> dinner.
15:25 < ariard> me too dinner
15:27 < elichai2> ok, I need to go to sleep now (1:30AM 😴 )
15:33 < elichai2> FWIW https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Compiler.20ICE.20in.20travis.20that.20I.20can't.20reproduce.20locally/near/208154810
15:56 < BlueMatt> you have to log in to read the rust chats now?!
15:56 < BlueMatt> yeesh
15:58 < BlueMatt> lol, welp, time to bump bitcoin_hashes instead andytoshi?
15:58 < elichai2> BlueMatt: nothing interesting there yet
15:59 < BlueMatt> elichai2: heh, I am gonna be blocked trying to merge the RL language bindings on a release with the stupid as_ref or whatever that utility method was that I added
16:06 < BlueMatt> ariard: lmk when i should take another look at the role-renaming stuff.
17:36 < andytoshi> BlueMatt: oh, sure, lemme take a look
17:40 < andytoshi> BlueMatt: https://github.com/rust-bitcoin/bitcoin_hashes/pull/84
17:41 < andytoshi> gonna follow up with a 0.9.0 with no changes except a MSRV bump
17:41 < andytoshi> assuming this passes..
17:46 < BlueMatt> andytoshi: cool, thanks.
18:46 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection]
20:19 -!- shesek [~shesek@164.90.217.137] has joined #rust-bitcoin
20:19 -!- shesek [~shesek@164.90.217.137] has quit [Changing host]
20:19 -!- shesek [~shesek@unaffiliated/shesek] has joined #rust-bitcoin
--- Log closed Thu Aug 27 00:00:00 2020