--- 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> 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