20:03:33 #startmeeting 20:03:33 Meeting started Mon Aug 5 20:03:33 2019 UTC. The chair is bitconner. Information about MeetBot at http://wiki.debian.org/MeetBot. 20:03:33 Useful Commands: #action #agreed #help #info #idea #link #topic. 20:03:59 any thing people would like to prioritize first? 20:04:17 Maybe 557 since Rusty was able to make progress? 20:04:40 We also have a bunch of small ones with discussions pending 20:04:44 #topic extend channel range queries https://github.com/lightningnetwork/lightning-rfc/pull/557 20:04:51 Yeah, let's clear some brush... 20:05:39 So, it's actually a pretty simple change, TBH. There's one outstanding q: do we use the same encoding-byte-then-stream style for checksums or not? 20:06:06 The test vectors don't, but the spec was modified to do so. I think not, actually: it's a bit neater without it. 20:06:13 But I don't really mind. 20:06:42 do you think we'll ever need to compress checksums ? if not then let's just remove the encoding prefix 20:06:45 what's byte then stream mean? 20:07:40 roasbeef: like we do for short-channel-ids, one byte to say uncompressed/zlib, then the series of scids. 20:08:13 roasbeef: doesn't make as much sense for checksums, except we use 0 for "no channel update received". I think that's a bit unusual tho. 20:08:20 does seem unlikely that we would try to compress checksums 20:09:16 don't have a strong preference either way, but seems there's some general agreement to drop the encoding type? 20:09:26 It's unlikely that we would compress individual checksums but wouldn't it make sense to compress a whole array of checksums? 20:09:49 It's an open question, I'm a bit new to that part of the codebase 20:10:22 not unless we have a lot of 0s (ie channels for which we only have 1 channel update and not 2) 20:10:24 Spec is a bit clearer as '...*channel_update_checksums'. It's a line-ball, can sstone comment? 20:11:16 if possible, something like compress(array(type)) or compressed*array*type would be even clearer 20:11:58 but I don't know how hard that would be to parse (I guess parenthesis would make things harder) 20:12:00 (FWIW, I implemented both, so it's trivial for me). We've got -rc1 in 4 days, so I was planning on including this as an --enable-experimental-features. 20:12:27 yes ! :) 20:12:29 sstone: well, formatting can be cleaned up later I guess? 20:12:58 sstone: your call, I think. 20:12:58 agreed that formatting can be changed later 20:13:01 yes of course 20:14:02 * rusty imagines sstone flipping a coin... 20:14:03 so I'll change the description of the TLV to `...*` 20:14:07 so the rough consensus is to drop the encoding byte for checksums and go with the latest version of the PR and test compatibility between CL and eclair? 20:14:17 And clean-up formatting later if needed 20:14:20 sgtm 20:14:24 ack. 20:14:41 #action drop checksum encoding type and clean up formatting 20:14:43 imo the compression could prob be left off all together, impls are forced to bring in compression libs and impl defensive measures agaisnt them (and the additional surface area), just to save on some round trip times 20:14:44 sgtm 20:14:50 great, very cool if that's included in your rc1 rusty! 20:14:52 but we'll prob just impl decoding again, but not sending 20:15:10 good point roasbeef 20:15:20 The commit needs some minor attention: the test vectors are in a comment, should be in an appendix. But minor stuff. 20:15:41 yes I'll move the test vectors to a separate file 20:15:41 roasbeef: yes, we have that problem already though. 20:15:58 #topic merge final_expiry_too_soon into incorrect_or_unknown_payment https://github.com/lightningnetwork/lightning-rfc/pull/608 20:16:00 yeah, and imo the same argument applies there, with teh current graph size, we still do multiple RTTs 20:16:06 I do want to ask for a feature bit: it's unnecessary, but useful to flag it. 20:16:38 and I'll add a feature bit too 20:16:47 #action add feature bit 20:16:57 sstone: OK, take the next pair, please, since this is basically approved. 20:17:20 that would be 50/51 right ? 20:17:40 10/11? 20:17:49 yep 10/11 20:18:00 you get to steal it from a not-implemented-yet feature ;) 20:18:20 ah ok yes 20:18:53 cool, i think this is gtg then 20:18:54 wilkl it be a global? since can be helpful to seek out just these nodes 20:19:06 roasbeef: in my mind, yes 20:19:14 yes it makes sense 20:19:18 agreed that it make sense to have it be a global 20:20:02 just to be clear, do have an agreement to merge this PR (and lock the feature flags / format) once the small nits are addressed? 20:20:11 No, because all bits will be advertized, so everything is "global" in that sense? 20:20:19 https://github.com/lightningnetwork/lightning-rfc/issues/605#issuecomment-518383654 20:20:28 well tehre's connection lvl vs node lvl 20:20:32 t-bast: ack. 20:20:41 connection level is like ephemeral stuff, like initila_routing_table_dump (or w/e it is) 20:20:47 i think that's our only *true* connection level feature 20:20:53 roasbeef: but we're (probably, eventually, after bikeshedding!) going to mirror those in node_announcement. 20:20:57 roasbeef: sounds like we'll start discussing feature bit unification and naming now ;) 20:20:59 roasbeef: sure. 20:20:59 everything else could be used for preferential node selection 20:21:09 shall we move on to final_expiry_too_soon and circle back on minor details towards the end? 20:21:19 bitconner: sgtm 20:21:22 bitconner: ack 20:21:52 t-bast: last i looked the pr was in good shape, will give another pass but i'd say probably in good shape 20:22:02 joost: do you want to update us on 608? 20:22:05 bitconner: cool thanks 20:22:10 608 looks good to me, we're removing privacy leaks via error code one after the other ;) 20:22:25 nothing changed, just rebased on top of the new data types 20:22:26 we've implemented 608 on our end as well 20:22:40 it fixes a probe vector and allows us to get more details in the final expiry too soon case 20:22:44 great, iirc we all thought it was a good change prior 20:22:47 I think that at some point we will just have a "payment_failed" error to avoid leaking information 20:23:05 we reach that point after this pr is merged I think 20:23:15 Yeah, but for now having extra information is really useful for debugging ^^ 20:23:28 cdecker: totally agree 20:23:36 joost: oh already that's the last one? 20:23:37 wasn't that true for the previous fix (amt) too ? 20:23:54 yes definitely 20:23:56 it is debugging info vs privacy 20:24:10 privacy > debugging 20:24:18 and in this case also debugging info vs better payment reliability in a way 20:24:42 Ack 608, FWIW. 20:24:44 true that the added blockchain height info can be useful for smarter retries 20:24:45 (because we know whether the failure was caused by wrong details or delaying nodes on the forward path. in the former we can give up, while in the latter we can try another route) 20:24:54 ACK for me too 20:25:27 cool, approved 20:25:43 FWIW we still have the `error` messages that leaks info on which version of which impl you're using 20:25:55 we briefly touched on 571 (feature bit unification), shall we do that next? 20:26:04 Yep 20:26:10 mpp? 20:26:25 #topic base amp https://github.com/lightningnetwork/lightning-rfc/pull/643 20:26:33 floor is yours rusty :) 20:27:16 sstone: we had actual error codes before, but then they ended up falling off for some reason (original lnd had it), we need them again imo 20:27:18 OK, so roasbeef raised a good point on this PR. We have a current issue with invoices with no amount, that intermediaries can probe and steal funds. 20:27:24 but there's also a lot of other tagging vectors as well 20:27:44 t-bast: oops missed your message, switching to feature bits 20:27:50 I think roasbeef's suggested fix is simple and makes sense 20:27:57 bitconner: no worries, we can do that afterwards :) 20:28:02 sgtm 20:28:14 t-bast: referring to including a random id? 20:28:18 yes 20:28:31 yeh then you include it, and the reciver verifies 20:28:36 That's orthogonal to base amp, though. I'll open a separate issue. 20:28:46 for e2e security, the intermediate can't guess that value (is the asumption) 20:28:49 agreed. this will likely be shared with sss amp as well 20:29:10 and is useful for plain regular payments as well? 20:29:11 Yeah, a nonce. It's no longer needed once we're Scriptless Scripts but it's good for now. 20:29:26 also on this topic, we had this old PR for what's now known as "keysend", we considered having it be a distinct elemtn in the protocol, but then realized it's basically just AMP with a total payment shard size of 1 20:29:37 yep, a hold over till dlog stuff (on the nonce) 20:29:49 joost: yeah regular payments can also include it also 20:30:15 can you provide a link to that keysend thing? 20:30:15 roasbeef: this was noted in our original email :) 20:30:38 also fixes probe vectors where intermediaries start crafting routes on their own to figure things out 20:30:43 t-bast: https://github.com/lightningnetwork/lnd/pull/2455 20:30:45 Cool, I'll open a separate PR, picking some unassigned bolt11 field, and some new TLV field. I'll let invoicer choose length, roasbeef? 20:30:48 thx 20:31:00 t-bast: old version was just having the pre-image in the onion at the end, here's our PR: https://github.com/lightningnetwork/lnd/pull/2455 20:31:29 oh ok, got it 20:32:14 meanwhile, back at base amp... 20:32:19 #action rusty to make pr for payment nonces 20:32:20 so adding this nonce is orthogonal to 643 and should be done in a separate PR? 20:32:43 why exactly is it orthogonal? 20:33:01 that was a question :) 20:33:02 roasbeef: re keysend: have you seen the nice trick we did with the shared_secret from the onion-decoding for spontaneous payments? 20:33:20 That allows us not to include anything other than a signal in the onion itself :-) 20:33:45 cdecker: you're using the shared secret with the last hop to implicitly derive a preimage? 20:33:46 bitconner: it's orthogonal because it's a problem already, nothing to do with MPP. 20:33:59 (Any no-value-specified invoice has this issue) 20:34:07 t-bast: yes, that's the proposal we talked about at CoreDev 20:34:21 (might need to write it up...) 20:34:22 rusty: got it, thanks 20:34:59 cdecker: yeah, but we realized we can just compress that into a amp of shard one, so then it's just included, and less similar-ish stuff in the spec 20:35:22 Note that there's a "split the bill" mode of using MPP that I'm trying to preserve, where the parts are paid by different nodes. 20:35:30 and it's nice to be able to also drop off additional info, so like an account ID or something 20:35:48 rusty: woudl require some coordination, so you could give htem the nonce at that point 20:35:52 roasbeef: but the shared secret trick works with all payment mechanisms, no need for moon-math xD 20:36:03 xor is moon math? ;) 20:36:08 lol 20:36:09 roasbeef: I was going to include it in a new bolt11 field... 20:36:48 rusty: for the split the bill scenario, we can simply let every payer derive something randomly from the shared nonce (from the invoice) and send what randomness they used to derive 20:36:57 rusty: well could be like: proposer has a QR code, to coordinate w/ everyone at lunch, and that includes the invociec or something 20:37:00 lol, I might be mixing up which AMP you mean then. I'll write it up and we can discuss on the PR (will stop hijacking the discussion now) 20:37:12 and the recipient can verify all shares were using something generated from the invoice's nonce, but different for each payer 20:37:21 cdecker: so this is just preimage = rand1 xor rand2 xor realThing 20:38:10 split the bill already requires coordination to decide how much everyone pays, no? 20:38:15 i think i have a good grip on what yuo're proposing, in the end still useful to have additional meta data, and we can have a single method for spontanoues stuff rather than like 3 (hence the death of keysend) 20:38:29 OK, so there are some interesting things in the PR, I think it needs more commenting on issue, FWIW. 20:38:49 rusty: tbh i haven't had a chance to read it in depth :/ 20:39:07 bitconner: TBH I'm just reading joost's comments now... :) 20:39:09 roasbeef: totally agree that it's a good time to regroup and make sure we all do the same thing :) 20:39:50 #action continue discussion/review on pr 20:40:23 #topic feature bit unification and renaming https://github.com/lightningnetwork/lightning-rfc/pull/571 20:40:34 seems most of the discussion is still about naming :P 20:40:34 FWIW I implemented 643 for eclair, it's quite simple to integrate, if CL has it in an experimental-feature in the next release I would be interested in testing interoperability 20:41:45 t-bast: nice, we are also planning to start implementing in the neart term 20:42:01 t-bast: we never got the bikeshedding session we wanted with roasbeef on names local/global/node/channel feature naming ... 20:42:08 bitconner: cool, this is going to ship very quickly ;) 20:42:28 rusty: true, but we have many proposals on the PR right now don't we? 20:42:52 and we can still find new ones until everyone is satisfied 20:43:07 rusty: shall we defer naming discussion to the pr? 20:43:21 can make a todo for roasbeef :) 20:43:29 t-bast, bitconner: ack. 20:43:45 #action roasbeef to weight in on feature field naming 20:43:56 alright 20:44:00 let's do an easy one 20:44:14 #topic single-option large channel https://github.com/lightningnetwork/lightning-rfc/pull/596 20:45:22 needs a rebase, feature bits might be a little out of date since variable payload went in 20:45:47 also depends on feature bit unification? 20:46:09 yes this depends on feature bit unification, but I think other than that it only needs a small rebase and everyone agrees on the intent? 20:47:49 It tries to do too much. It adds enforcement of the channel limit in channel_update, which is a separate issue. 20:48:27 And just because I have a large channel, doesn't mean I'm prepared to offer it to others. 20:49:36 But yeah, basic idea is sound. Blocked on fb Grand Unification. 20:50:03 #action consider removing enforcement on channel_update 20:50:18 basic idea lgtm 20:50:52 tho would probably shoot for a more specific name, e.g. option_no_funding_limit 20:50:53 (Note: we don't currently enforce 2^32 limit on maximum_htlc in channel_update, this PR actually adds it, which IMHO is out of scope) 20:51:18 bitconner: if only.... there were some word for this... preferably cartoon oriented... 20:51:54 :D 20:52:10 rusty: in all of our searching, wumbo is still the most descriptive 20:52:24 bitconner: "wumbo"? Never heard of it... 20:52:38 #action wumbify? 20:53:09 okie doke, gonna move on if there are no objections 20:53:30 #topic simple tooling fixes https://github.com/lightningnetwork/lightning-rfc/pull/650 20:53:44 rusty: just a clean up? 20:53:53 bitconner: yeah. 20:54:01 I think for 650 the only thing I'm not a big fan of is the ...* notation 20:54:07 but that can be cleaned up later 20:54:20 cool, approved 20:54:35 t-bast: yeah it becomes less clear if there are multiple variable sized fields 20:55:00 to me n*type would allow us to express that different fields should have the same size 20:55:00 bitconner: you can't have more than one in a tlv tho... that's literally the notation for "the rest of the tlv". 20:55:28 rusty: i mean that two different tlv records should have the same number of items 20:56:00 do you really want to enforce that? 20:56:07 bitconner: oh... hmm, that would be nice, but hard to make a nice notation for. Still, we can paint that bikeshed later... 20:56:22 OK, so we accept this PR? 20:56:24 agreed that we can re-visit that later, ACK 20:56:25 rusty: agreed, can be something to consider down the road 20:56:42 even if we make n a keyword then it's no different from ... 20:57:06 would need to reserve a couple of single letter length variables, but something for the future :) 20:57:35 bitconner: shades of Fortran! 20:57:41 we have two small ones that we discussed many times, 550 and 601 20:57:47 #agreed merge 650 20:57:48 they are very close to getting merged ;) 20:58:05 t-bast: yep i have 550 next 20:58:27 #topic define my_current_per_commitment_point https://github.com/lightningnetwork/lightning-rfc/pull/550 20:59:09 bit of recent activity on the pr itself 20:59:34 major outstanding q is to decide whether we take the lowest unrevoked point or the highest there are multiple 20:59:53 seems like comment consensus is on taking the highest 20:59:55 from the comments, it seems lnd eclair and rust-lightning all do lowest 21:00:03 Yeah.... the point is that where there's ambiguity, you need to send the one you'd broadcast. That's the *point* of sending this information, after all. 21:00:35 eclair does highest atm 21:00:52 Sending one then broadcasting the other tx would violate the "be excellent to each other!" rule. 21:01:23 t-bast: pm47's comment says the switch happens atomically, which means eclair uses lowest iiuc 21:02:02 rusty: that's a good point 21:02:17 regarding you need to send for the one you'd broadcast 21:02:21 We're atomic too; we tell the master daemon we've got the commitment, and that msg also implies permission to send the revoke_and_ack. So if it's in the db, we have to assume we sent the revoke_and_ack. 21:02:37 bitconner: I'll get him to clarify, but I'm pretty sure we send state 1 - and we re-sent the revoke_and_ack if you didn't receive it (this is what I'm calling highest here) 21:02:55 I understood it exactly as rusty 21:03:01 got it. perhaps there's less ambiguity than i anticipated? 21:03:06 niftynei: there's no req to stash the "highest" though, since the other party can retransmit 21:03:18 stash as in commit to disk here 21:03:34 t-bast, bitconner: yes I'm pretty sure that's what we do 21:04:05 this is also an area where the spec has some wiggle room 21:04:15 Strictly, you only need to remember if you sent revoke on previous, though you'd be *allowed* to, in which case your options are open to profit maximize. 21:04:58 either way, transmitting lowest or highest shouldn't cause dlp to fail 21:05:20 Definitely not. And both are tested in my protocol test PR, BTW. 21:05:26 nice 21:05:38 agreed 21:05:40 wait where are you getting stashing from roasbeef? i'm not sure i follow 21:06:50 the requirement to retranmit is specified elsewhere i think 21:06:55 niftynei: you could save the received commitment signature to the db, and not send the revoke_and_ack. Then on restart, you can decide whether you want to have received that commitment signature or not. 21:06:58 i think he means that it's technically valid to receive a commitment and write it without actually revokeing 21:07:15 this is evil 21:07:38 don't hate the playa 21:07:41 :) 21:07:44 In practice, this is dumb, since it costs you two db writes (you need another when you do send revoke_and_ack) for a very slim advantage. 21:08:07 yep, and leads to a more complex state machine 21:08:16 i mean that if you recv, you don't need to write it to disk 21:08:19 since the other party will retransmit 21:08:41 but also if you _do_, the other party is actually able to send a _diff_ commit set (expired htlcs or w/e), so seems at that point y'all's state would diverge 21:09:13 roasbeef: heh... pause before sending revoke_and_ack and hope they die horribly in the meantime, then PROFIT. 21:09:13 or you'd just reject that even, since they're sending a diff state 5 (in this example), when you epxect 6 21:09:23 this seems like a problem with the commitment tx exchange rather than this particular PR 21:09:33 possibly? 21:09:39 it's due to flexibility that's in the protocol rn 21:09:48 there's 2 ways to handle this, so there's two diff points you can send 21:10:05 but also if/when we move to having multuiple unrevoked, the "lowest" one is more stable, since you can have others that extend beyond 21:11:47 It's fundamental, you might have lost connectivity before receiving commitment_signed, or after sending revoke_and_ack, you don't know. That's why we have these numbers on reestablish, though technically it could be a single bit if we wanted to optimize stupidly. 21:13:21 okay, general sentiment on 550? 21:13:32 i think that the wording on this proposed change is pretty good at encompassing either usecase "the commitment_point 21:13:32 corresponding to the commitment transaction the sender would use to unilaterally close" 21:13:36 ah would depend on if what they set there for the local/remote heights on the msg 21:13:52 but it can't be ambiguous 21:14:21 ppl shouldn't be able to chose, there needs to be a single correct answer, that some of us impl it differently shows that the current language is to weak also 21:14:32 well, i mean it kind of can. either then send the 'lower' one and you assume that they, for some reason, didn't get your last message 21:14:38 or they send the 'higher' one and you know they did 21:14:41 i agree that "both work", but that can lead to ppl not being able to reest their channels, if from one's PoV, the other sends the wrong point 21:14:55 either case should be covered though, no? 21:14:59 but with the simplified commitment stuff, the point doesn't matter, only the secret stuff 21:15:17 niftynei: if they send higher and I want lower, what do I do? 21:15:20 Yes, it needs to correspond to what they sent, since the receiver checks it. Hey, did I mention I test all these cases in #655 ? 21:16:06 if you send anything other than those two points than you're definitely breaking protocol; either of them should be handle-able 21:16:08 rusty: woah nice pr 21:16:24 yeah i'm really excited about these protocol tests 21:16:47 wrt to this PR: should we change the language, accept it as is, or reject it? 21:17:07 (also to step back, we thought this was the issue with CL and lnd, but then it was actually just timing) 21:17:08 i don't think it resolves the problem we've identified but it does make the spec a bit more readable imo 21:17:14 yes, correct 21:18:34 which is great! so maybe we don't need clarification and we can leave it ambiguous as is? 21:18:53 yeah i think i'm now convinced that the current language is sufficient :) 21:19:34 Me too. Ack! 21:19:43 sgtm then 21:19:47 great. 21:20:21 woo 21:20:37 #agreed merge using current language 21:21:02 we're a bit over, so probably won't get 601 21:21:08 rusty2: can we also get the original rusty to confirm as well? 21:21:09 unless anyone has a burning issue 21:21:14 very quickly on 627: do you feel this new error code is needed or should I just retract this PR and use the general PERM error? 21:21:39 I'm just curious what your implementations do (they have to somehow handle that code-path) 21:22:06 roasbeef: agreed, I don't know who that rusty2 guy is 21:22:23 it's more forward looking imo, we'll need it in the future as the payload get more flexible 21:22:32 but rn, not like yo ucan fail to decode an integer, since it's all fixed sized 21:22:37 roasbeef: done :) 21:22:50 btu when we have a buncha types in there, makes sense to be able to say "actually idk what this is" 21:23:20 t-bast: yeah i think it'll be useful for tlv payloads, i'd keep it open 21:23:36 ok great, let's discuss on the PR then since we're overtime 21:23:39 sgtm 21:23:42 #endmeeting