18:59:45 #startmeeting 18:59:45 Meeting started Mon Jan 21 18:59:45 2019 UTC. The chair is cdecker. Information about MeetBot at http://wiki.debian.org/MeetBot. 18:59:45 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:10 ok 19:00:17 #info Meeting agenda can be found here: http://bit.ly/ln-agenda-201902 19:01:04 Ping AmikoPay_CJP bitconner BlueMatt cdecker Chris_Stewart_5 kanzure niftynei ott0disk roasbeef rusty sstone 19:01:13 hi. 19:01:45 Am I missing someone that ought to be pinged? 19:01:53 hi all 19:02:31 Hi Johanth :-) 19:02:32 #lightning-dev roasbeef aj BlueMatt cdecker jimpo niftynei rpickhardt I'm sure I missed others... 19:02:40 hi everyone :) 19:04:45 Heia, shall we start with #491 and by association #539? 19:04:56 #topic PR #491: BOLT #2: order htlc_signatures by BIP69 + increasing CLTV. 19:06:02 looks like it needs a rebase, rusty (who is mia today) indicated its waiting on a second implementation, and also we should inline BIP 69 19:06:16 not sure there's anything to talk about except "go implement, author should rebase" 19:06:28 Right, rebase is trivial, I just checked, but didn't push just yet 19:06:45 It's sort of the last 1.0 PR it seems, after this we can tag 19:07:13 we also implemented it and also added test vectors in #539 19:07:43 cdecker: can we please inline bip69 before tagging? bip69 is so massively underspecified.... 19:07:53 Excellent, so not only do we have a second implementation for #491, we also have the a test vector. 19:08:15 Ok, BlueMatt do you volunteer to inline BIP69 before we tag then? 19:08:17 also it just says "lexicographical ordering" 19:08:20 sure, I can do that 19:08:29 "lexicographical" is undefined as well... 19:08:40 #action BlueMatt to inline BIP69 into the BOLT to remove ambiguities 19:09:06 And I assume everybody is happy merging #491 given the second implementation? 19:09:44 yea, its straightforward enough 19:09:51 even if it doesnt resolve all ambiguities 19:10:06 you mean after test vectors have been verified ? 19:10:12 Any objections from other implementations? 19:10:39 sstone: I was going to propose that #539 needs to be verified and then merged, but sure we can bundle the two together 19:10:53 yep i'm happy with it, also nice to have the test vector! 19:10:53 lgtm, @bitconner you looked into 491? 19:11:02 no I agree with you verify first then merge 19:11:02 nice 19:11:17 we should make moves and implementing that in lnd 19:11:24 sstone: so merge #491, then verify #539 and then merge #539? 19:11:44 but in having discussed the changes at length prior, i think we agree 491 is good 19:11:50 yes 19:12:24 #action cdecker to merge #491 19:12:50 Good so we will keep #539 open for verification and then discuss that during the next meeting 19:13:25 excellent, sgtm 19:13:27 #action The test vectors in #539 need to be verified independently. Decision is to be deferred until the next meeting 19:13:52 #topic PR #500: BOLT 7: Add note for 'htlc_minimum_msat' being static 19:14:20 PR #500 opened the pandoras box about some value in the channel updates being static or not. 19:14:45 the name of the message is channel_update, it lets one update attributes of the channel useful when routing 19:15:08 there're link level values which are static, but hte routing level values can be modified as long as they're within the constraints of the link level values 19:15:15 We were mostly happy with moving that discussion to a separate issue (#540) since the changes were not introducing the "static" wording, just change it slightly 19:17:03 Shall we move the discussion to #540 and merge #500? I'm also happy if someone just opens a PR to remove the static wording btw 19:17:15 roasbeef: was the one with the objection 19:17:28 my self would rather just fix the wording and not take two hops to get there 19:17:44 i can make a PR outlining things a bit more 19:18:23 Ok, do you want to amend the PR (add another commit) that fixes that wording? 19:18:25 roasbeef: There will almost certainly be some disagreement about allowing nodes to set an htlc_minimum_msat below the remote peer's defined level 19:18:47 (It'll end up being the same in the commit history, whether we open a new PR or amend the existing one) 19:19:50 Bluematt: I believe that can be done today? 19:20:03 Ok, I'll let roasbeef decide whether to amend the PR or open a new one 19:20:38 #action roasbeef to amend PR #500 to remove the static definition or open a new PR to do that 19:20:46 roasbeef: is that ok with you? 19:21:00 sgtm 19:21:08 Excellent, onwards then 19:21:16 #topic PR #519: BOLT 7: add extended channel queries 19:21:53 We had some back and forth about this, and I think I'm the only one complaining about this. 19:22:42 To give some color to my objection: I absolutely loathe the hacked up gossip protocol that we have right now, and I don't want it to have yet another incremental change that may make things worse. 19:22:47 I think that with a few changes it can be made compatible with current impls without adding new messages (as is the case currently) 19:22:48 totally 19:23:28 i don't think client sshould be trying to "sync all the channel updates" it's super wasteful, and someone can always spam you if you want to fetch each update, instead you can prioritize the channels you're likely to take given past payment history, and upon failure you get the channel update anyway 19:23:29 yea, I'm still kinda hoping all the current gossip query stuff dies and we add a new feature and all move on 19:23:32 After discussing this with sstone, I think we found a middle ground by amending the existing short_id message with an optional field that adds the extra information, in a backwards compatible way 19:23:47 which message? 19:24:04 Basically all the existing channel queries 19:24:27 Will this still be relevant after the move to INV? 19:24:48 query_short_channel_ids_extended can be translated into query_short_channel_ids with the extra info appended as optional field, that was my proposal 19:24:52 this is for cartch up, taht would be once synced 19:25:04 That way we don't need a feature flag, we don't have new messages and all just keeps working 19:25:06 ah rather than adding a new message, that seems nice 19:25:18 If the encoding type is interpreted as a bit field then yes it becomes easy to add optional data 19:25:45 For the long term we might want to look into minisketch and IBLT very soon, that way we can throw the existing protocol out of the window 19:26:08 specifically, which additional data from the proposal would be carried over? 19:26:26 sstone: we can also just add a new flag field after the ids which tells us which other optional fields are there 19:27:00 The proposal proposes timestamp and checksum for each channel id that is being transferred iirc 19:27:38 yes but this works is everyone keeps the extra data that they're received (I think it's fairly easy for c-lightning) 19:27:54 gotcha, my onee concern in using a checksum is that it seems to be overfitting to the existing message fields 19:28:18 which is how optional fields work in generael atm sstone, but then again should we tack this on or wait for our overloads from the TLV clan 19:28:35 sstone: how so? I think it is trivial to reconstruct checksums and timestamps from the local view, isn't it? 19:29:19 roasbeef: good point, we also need to discuss TLV, since that's blocking some more things in the queue, but let's defer that to after the PRs 19:30:24 cdecker: sorry I meant keep the extra data at the end of the message you've received (what roasbeef said) 19:30:57 presumably, we will need to add more fields in the future that are dynamic. when that happens, do we define a new checksum field for every possible optional field combination? 19:31:18 Wait, isn't this just query_short_channel_ids which isn't getting gossiped anyway? 19:31:31 My view is that for now it's a simple addition to a feature that no-one has implemented but us, and which yields massive gains 19:31:33 just seems a little brittle for forwards compatibility 19:32:01 bitconner: my proposal was to just blank signature and timestamp and otherwise checksum everything that is covered by the signature, since that's an atomic transfer unit anyway 19:32:32 I think we all have implemented the gossip query mechanism 19:32:42 yeh 19:32:52 i think he means this addition w/ timestamps 19:33:02 Ah, ok, sorry for misreading it 19:33:13 sstone: eclair nodes in the wild will serve and signal this? 19:33:23 yes I don't think anyone has toyed with it so changing it does not impact others 19:34:03 roasbeef: no just ours for now 19:34:15 as in the main aqinc node? 19:34:17 acinq* 19:34:20 Ok, I misunderstood you in that case, I thought you were making a case for the current draft to be kept, instead you are saying it's ok to change that since it's eclair only? 19:34:43 roasbeef: yes on mainnet/testnet 19:35:20 sstone: would you be ok with going through one more iteration on the design with us then? 19:35:57 cdecker: yes, and I was planning to change it to use optional fields instead of new messages to make it more acceptable 19:36:30 Excellent, thanks ^^ 19:36:55 #action sstone to rework #519 with the feedback on the PR 19:37:03 Making good progress :-) 19:37:05 I also think that the underlying problem will also be an issue for INV based solutions and minisketch/IBLT 19:37:11 #topic PR #538: BOLT 4: Report outgoing HTLC values when appropriate 19:38:14 sstone: not if you give up on trying to sync every update from every channel on the network... 19:38:36 mo bandwidth, mo queries, mo problems 19:39:50 for #538, perhaps we should send both? 19:41:26 roasbeef: why both ? 19:42:20 to maintain backwards compat 19:42:44 seems like the second change is just a clarification, while the first is breaking wrt how routers interpret failures? 19:43:12 no, look at the line above the second change 19:43:13 either way, you get the latest channel_update, if it was a msitake on the sender (logic wise), then this doesn't really help, but if it was just a statel channel_update, then you get the latest one 19:43:21 cltv_expiry clearly implies the incoming 19:43:43 ah you're right, thanks BlueMatt 19:43:47 bitconner: I'm not following you sorry how does it break routers ? 19:44:17 roasbeef: we cant just "send both" cause the channel_update goes second. I mean we could append it *after* the channel_update 19:44:43 yeh append, old nodes just discard the rest, new nodes know to keep reading 19:45:09 The phrasing used above the second change is in contradiction to the sentence above (cltv_expiry is applicable on the incoming side, the outgoing side is referred to as outgoing_cltv_value) 19:45:12 but then it's implicit that way rather than explicit 19:45:33 sstone, existing routers will be expecting the incoming value, returning the outgoing means now means they will apply this to the wrong channel from their PoV 19:46:22 indeed appending seems like a good option 19:46:36 Re: routers not understanding the changed payload: this does not apply since the payload is onion wrapped and opaque to the routing nodes 19:47:01 cdecker, i'm referring to the sender's router in particular 19:47:09 This is only an issue if the sender tries to be smart and do something based on the value 19:47:09 well depends on which channel_update they inclose I guess (respond to myself above), the incoming or outgoing 19:47:34 Ok, wasn't aware you were calling that one router as well :-) 19:47:57 cdecker, haha i can see why that's confusing, sorry :) 19:48:40 the channel_update does not change, just the value that is reported as a hint. Are you also using it to recompute a new route ? 19:48:49 No prob, thanks for the clarification bitconner 19:49:06 sstone: use the channel_update? yeh 19:49:12 the values 19:49:41 atm no, are y'all? 19:49:56 if not, then this is just for better logs? 19:49:59 does the "hint" actually give you anything the channel update doesn't? 19:50:07 no so I don't see what it breaks ? 19:50:09 it gives you the context 19:50:13 the spec ;) 19:51:04 Ok, it seems more discussion is needed on this one? 19:51:07 appending lets ppl do clever stuff with that value and keeps things the same for existing nodes, q there is how to signal it or if it shuold just be appended 19:51:34 yes but since noone is using the value it's cleaner to change now 19:51:57 Shall we defer to the tracker again? 19:53:08 Ok, I take it we need to do some iterations on this on the list/tracker then 19:53:44 lol checked and we actually send the outgoing atm :p 19:53:50 Let's move on to #544, since #539 already has the check-and-commit decision above 19:54:13 roasbeef: so we can merge #538 ? 19:54:20 :) 19:54:24 lol 19:54:53 roasbeef: I'll let you have the last word before switching topics :-) 19:55:09 yeh since we do it already no contest on my end lol 19:55:32 Ok, any objections to merging #538? 19:55:37 nice discovery lol, merge away! 19:55:55 yes 19:56:02 * cdecker looks into BlueMatt's direction 19:56:26 nah, if its confused already might as well clarify 19:56:43 So your verdict is merge or !merge? 19:56:49 merge is fine with me 19:57:00 #action sstone to merge #538 19:57:10 #topic PR #544: BOLT 4: remove incorrect_payment_amount altogether. 19:57:54 Fewer error message = less information leakage I guess :-) 19:58:16 well we already "removed it" this just removes it 19:58:30 and, yea, the second hunk there was just missed in the first removal 19:58:32 Ok, so any objections to merging it PR #544? 19:58:40 no 19:58:50 #action cdecker to merge #544 19:59:19 lgtm 19:59:30 We're almost at time, are we at risk of losing quorum, or does everybody agree to stick around a few more minutes? 19:59:37 hey-o, there's a rusty! 19:59:51 * BlueMatt is hungry, but if we move quick I can delay :p 20:00:05 I can stick around 20:00:08 same 20:00:27 can stick around for a few minutes 20:00:29 Excellent 20:00:32 Moving on 20:00:34 #topic PR #545: BOLT 4: include `channel_update`'s message type 20:01:44 We discussed this one before, and decided to drop the message type since there were 2 impls already doing it that way, but it seems we had a misunderstnading there? 20:02:12 imo we already know it's a channel_update so no need to add the type, length alone can be enough to distinguish that there're extra fields there (optional channel_update stuff, or append stuff to the error message like was discussed above) 20:02:19 c-lightning currently supports both ways 20:02:25 yeh we parse both ways 20:02:27 but write w/o the type 20:03:06 Would including the type leave room for another message at a later time? 20:03:17 another message? 20:03:23 If something better than channel update comes along :p 20:03:27 * BlueMatt prefers without, only reason to define it *with* imo would be if most folks already serialize with 20:03:29 Same with us, I kind of like the type with type prefix since it allows us to reuse the parsing without any changes, cropping means we have to reallocate and copy it over 20:04:12 we still re-use the parsing logic, as it's separated into the brontide unpacking then the inner message, inner message knows what it is, so it doesn't need a type hint 20:04:43 it's much cleaner to include the type, and saving one byte here is not really useful (see I don't always obsess over bandwidth :)) 20:05:02 then we need to also specify what to do if the type is wrong 20:05:18 Let's say we define channel update v2 at some point, how would you signal that without the type? 20:05:25 It's 2 bytes, but that shouldn't matter 20:05:28 currently it says you have to send a channel_update, but we need to clarify that it means you have to send a channel update *right now*, and should ignore other type messages 20:05:46 johanth's point sways me enough to not care 20:06:01 but would need clarification on the recv end 20:06:35 if we want to swap out what's ecnlosed therein the future, we'd need a diff error msg 20:06:48 not if we include the type... 20:07:10 Ok, am I right in counting it this way: opposed=[roasbeef], abstained=[BlueMatt], infavor=[cdecker,sstone,johanth]? 20:07:34 i'm in favor of having the type byte, gives us more flexibility 20:07:36 I'm opposed to patch as-is 20:07:50 I'm fine with adding type, just need to clarify that we have to ignore other messages right now 20:07:58 so that we can swap it out in the future if we want 20:08:18 we can update the PR to reflect that 20:08:20 Ok, makes sense 20:08:56 i dont see any cons in adding it.. other than that we have to rewrite our serialization slightly... :P 20:08:56 roasbeef: would you like to have a more in-depth discussion about this on the tracker? 20:09:17 sure 20:09:24 johanth: I'll write the patch for you :-) 20:09:26 yea, gotta rewrite my shit for it too 20:09:38 cdecker: ewww. go. 20:09:41 Ok, everybody happy with amending PR and moving the discussion on the tracker 20:10:05 sounds like it, lets move forward 20:10:20 #action sstone to rephrase the PR to include what to do when type is unknown, everybody to discuss lively on the PR 20:10:31 #topic PR #546: Clarify behavior of id field in update_add_htlc 20:10:49 This one should be easy, I had no idea one could misread it, but here we are 20:10:50 cdecker: I'll remember 20:11:20 * cdecker starts frantically reading go code 20:11:48 yea, I'm not actually sure that the async nature of the protocol works out if you misread the id field, but, either way, obvious ACK 20:11:54 I think the last few are mostly spelling mistakes so that should go quick 20:12:23 Any opposition to merging it, other than making the document very verbose? 20:12:29 above says it shuld icnrease after each offer, but can't hurt to specify i guess 20:12:45 #action cdecker to merge PR #546 20:12:47 Done :-) 20:12:48 ack, can't hurt to be more specific :) 20:12:54 #topic PR #547: Keep HMAC case consistent 20:13:27 I think this should be deferred since as sstone points out it makes it even less consistent :-) 20:13:31 547 ack, shed's not my favorite color, but thats ok, I can live with eyesores :p 20:13:39 should be upper 20:13:40 or that, whatever 20:13:52 hash based message authentication code 20:13:59 yea, my favorite color is upper case 20:14:16 Mine too, we have soooooooooo much in common :-) 20:14:20 https://tools.ietf.org/html/rfc2104 20:14:24 yes but all other message fields are lowercase ? (very weak objection) 20:14:37 Ok, let's note that on the PR and move on then 20:14:56 #action BlueMatt to comment about the shed color not being uppercase 20:15:07 #topic PR #548: Align offered and received HTLC scripts similarly 20:15:25 Is this just a spelling change? IMHO it is, so single ACK is sufficient :-) 20:15:31 easy yeh, it' strivial 20:15:35 548 ack 20:15:56 next 20:16:12 #action cdecker to merge #548 20:16:12 strivial it is :) 20:16:19 #topic PR #550: dpl: concretely define `my_current_per_commitment_point` 20:16:41 Can you guys confirm that this is the correct interpretation? 20:17:59 The sending node: 20:18:04 - MUST set `my_current_per_commitment_point` to its commitment point for 20:18:17 the last signed commitment it received from its channel peer. 20:18:20  20:18:36 it shuld be the latest point used for the last commitment for the other peer, so the one that's unrevoked 20:18:51 as you need that to sweep your non-delay funds on chain if the other party broadcasts 20:19:27 roasbeef: yes, it should correspond to the commitment tx you're going to broadcast. 20:20:17 Do we need more time to verify the correctness here? 20:20:27 i think wording has right intent, but can be specified further 20:20:49 Ok, would you volunteer to improve the wording? That'd be a great help 20:20:55 "point used in last commitment signed for the peer" 20:20:56 ok 20:21:19 I think everyone assumes we immediately revoke previous so there's no case where you have two possible commitment txs. 20:21:27 #action roasbeef to help with the wording of the PR to remove any remaining uncertainty 20:21:35 #topic PR #552: Demand revoke_and_ack before more updates 20:21:44 there is before you revoke the previous ;) 20:22:02 another one that I didn't know you could misread, but, yea, nodes that dont wait for an RAA and get a CS are busted 20:22:19 This seems to have gotten quite a bit of feedback during the last few hours so I propose we should defer it, especially since this tries to address #526 which was opened by nayuta-co which is not currently present 20:22:32 they can't do that even, you can't sign again before you get a revoke 20:22:48 yea, I mean you *could* but dont, cause you'll break everything 20:22:56 they're leaning towards the prior multiple unrvoked 20:23:11 but you can't do that w/o making a "revocation window" so pre-send 20:23:45 * BlueMatt -> food 20:23:45 yeh i think defer, they seem to be musing about extending the design in the linked issue 20:23:54 * cdecker -> soon food 20:24:10 #action Defer #552 to the tracker 20:24:18 Ok, so that's the PRs done 20:24:33 I'd just like to talk 2 minutes about the TLV proposal for 1.1 20:25:05 * BlueMatt ventures out into -10 C weather to get food, see y'all 20:25:10 roasbeef: you guys are doing the spontaneous payment thing already right? That was/is blocked by the TLV proposal, so could you share what your current method is? 20:25:24 See you BlueMatt, and thanks for sticking with us for overtiem 20:25:32 method is that the TLV is a diff packet type, as there're cases where yuo can't fit what you want into 1 or two hops due to teh overhead of the tlv 20:25:49 as that also pushes the interpretation of what the tlv is above 20:26:02 so the upper layer sees "ok i'll try to parse kv's from this blob" 20:26:11 Right, but that means you have a working multi-onion-layer implementation and at least an idea on how TLV should look like right? 20:26:29 cdecker: no,, they fit into one per_hop IIUC. 20:26:31 other diff thign than ML is realm is left in place, instead there's a "more bit + length" byte and a type byte 20:26:49 this ignores tlv and leaves it to the upper layer to define that it looks like 20:26:54 cdecker, i've started working on a tlv impl/proposal separately 20:26:57 it just says "boss says this is a tlv, figure it out" 20:26:58 Oh, ok, was hoping you had a prototype that you could share 20:27:06 conner has some tlv code 20:27:11 i do have a tlv prototype 20:27:18 here's the onion PR https://github.com/lightningnetwork/lightning-onion/pull/31 20:27:18 it's really not bad at all 20:27:24 bitconner: awesome, would love to see that so I can start working on rendez-vous as well 20:27:26 Yeah, let's do it properly... lower 4 bits == extra hops. upper 4 bits = WTF (like now). 20:27:32 most of it is packing the bytes into diff hops, then unwrapping multiple hops on the receiver 20:27:41 rusty: bits of what? 20:27:48 roasbeef: realm byte. 20:27:56 Ok, I'll review lightning-onion#31 then 20:27:56 "byte formally known as the realm byte" 20:28:00 that PR does: first padding bytes is (1 bit more, 7 bits length), second padding byte is type 20:28:09 cdecker, will do! i can send out a link with the WIP branch later today 20:28:18 Awesome thanks 20:28:21 that's only for the "pivot hop" (the first hop no extra hops), full hops just have the (1 bit more, 7 bits length) 20:28:29 That's my mind about TLV put at ease :-) 20:28:47 #topic TLV format and multi-hop onion 20:28:55 the tlv format might be diff of wire vs onion 20:29:03 #action cdecker to review lightningnetwork/lightning-onion#31 20:29:07 as in # of bytes for type and value 20:29:21 you can pack 700 ish bytes, so you need a 2 byte length 20:29:25 err type and length soz 20:29:34 while 1 byte type may be enough 20:29:43 the wrie protocol on the otherhand may want diff values there 20:29:47 roasbeef, indeed. i think it'd be possible to reuse most of the logic and change some constants for different use cases if necessary 20:29:55 Right, I'll need to catch up there and then we can discuss in depth :-) 20:30:06 Any other topics that we need to address before we end the meeting? 20:30:49 nope, planning to reply to the existing eob thread to outline how waht i implemented works and to brdige the gap of the diff 20:31:20 Excellent, thank you very much everybody, and sorry for the overtime :-) 20:31:22 #endmeeting