20:06:48 <t-bast> #startmeeting
20:06:48 <lightningbot> Meeting started Mon Sep  2 20:06:48 2019 UTC.  The chair is t-bast. Information about MeetBot at http://wiki.debian.org/MeetBot.
20:06:48 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
20:06:50 <rusty> cdecker: it's when you take time off of typing out new code to think about spec issues :)
20:07:07 <cdecker> Ah, I see, maybe I should try some
20:07:18 <t-bast> let's warm up with a small PR that was almost accepted last time
20:07:28 <t-bast> #topic 657
20:07:36 <t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/657
20:08:14 <t-bast> a small clarification on the ordering of messages around funding_locked (we have a few PRs open on those subjects)
20:08:49 <t-bast> we ack-ed it last time asking for a small change that has been done
20:09:27 <rusty> t-bast: Ack, apply.  Though "turbo channels" would require this, I think.
20:09:40 <t-bast> Allright, let's merge this.
20:10:12 <cdecker> rusty: well turbo channels just send locked-in immediately
20:10:20 <cdecker> So there should not be any issue here
20:10:23 <t-bast> a few small things around onion encryption now (should be fast too)
20:10:37 <t-bast> #topic 663
20:10:39 <cdecker> They just consider 0-conf as depth reached
20:10:40 <t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/663
20:11:00 <t-bast> This PR proposed changing the short channel id tlv to save a few bytes, mentioning the last hop
20:11:16 <t-bast> as Rusty points out, the last hop will completely omit this tlv field, so we probably don't need this PR
20:12:16 <roasbeef> should be called "zero conf chans" ratehr than "turbo chans", the turbo part over sells a bit, ppl should think twice about the implications of accepting a zero conf channel
20:12:40 <roasbeef> iirc it optionally can, we still put it tehre since the main termination signal is the hmac
20:13:08 <t-bast> yeah but if it can be removed and it's useless, just remove it to save bytes :)
20:13:09 <cdecker> roasbeef: totally agreed, it's a hack
20:13:13 <rusty> roasbeef: agreed, zero-conf.
20:13:36 <t-bast> switching to tu64 would save a few bytes for intermediate hops though
20:13:51 <cdecker> t-bast: re 663 we only save anything if the short_channel_id fits into 32 bits, right? So nowhere :-)
20:14:22 <t-bast> hum no, tu64 can be any size smaller than 8
20:14:23 <roasbeef> yeh originally I thought since we always utilize the full 64-bits, we decided to not also truncate this one
20:14:24 <cdecker> With the current height any short_channel_id requires 59 bits to represent, i.e., 64
20:14:32 <roasbeef> or nearly the full
20:14:46 <t-bast> ah ok, if we're already using 59 bits then it's useless indeed :D
20:15:17 <cdecker> At least if my math doesn't abandon me (log2(590000 << 40) >= 59 bits)
20:15:51 <cdecker> Shall we add a quick comment to that effect and close then?
20:16:04 <roasbeef> well would want to give conner a chance to chime in
20:16:18 <cdecker> Right, I'll comment and leave it open ^^
20:16:19 <roasbeef> for those that _do_ include it would still be a savings
20:16:23 <t-bast> I agree, let's summarize this as a comment in the PR and bitconner will close
20:16:40 <t-bast> roasbeef: but wouldn't you just stop including it instead?
20:16:47 <cdecker> roasbeef: where would you include it and it'd require fewer bytes?
20:17:13 <roasbeef> t-bast: it's more uniform on our end code wise if it's always included
20:17:20 <roasbeef> the last hop
20:17:22 <rusty> No, you can't include it.  It's even, and it's a protocol violation.
20:18:31 <cdecker> roasbeef: if we switch to tu64 we suddenly encode the short_channel_id as 9 bytes (length byte + 8 payload bytes), so an overall loss
20:19:09 <rusty> The writer: ...  - MUST NOT include `short_channel_id` for the final node.
20:19:21 <rusty> It's right there in the spec.
20:19:23 <t-bast> cdecker: but the length byte is already there since it's a TLV field
20:19:35 <cdecker> Oh right, sorry my confusion ^^
20:20:10 <t-bast> Sounds like the spec has spoken: the protocol already says this field shouldn't be included (even though our implementation currently just ignores it if it's included).
20:20:11 <roasbeef> ok, soudns like mis comms somehere there, iirc there was talk of adding a tlv bool to indicate the final hop, but then we just decided that omitting it amounted to the same
20:20:42 <t-bast> roasbeef: we instead kept the empty HMAC to signal termination
20:20:47 <roasbeef> but i think we can move on, and allow conner to chie in once he's back
20:20:53 <cdecker> rusty: where is that quote from so I can add it to the comment on the PR?
20:21:02 <rusty> cdecker: the same PR...
20:21:03 <t-bast> roasbeef: totally agree, let's move on
20:21:14 <roasbeef> tbh things get lost at time when there's like a 30 comment deep thread on a PR
20:21:22 <t-bast> #action comment why we think it's not useful, let bitconner close the PR and chime in
20:21:41 <t-bast> #topic 627
20:21:45 <t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/627
20:22:17 <t-bast> This is the "generic" error code for invalid onion payloads (decryption was fine, but we're missing content or receive an invalid tlv stream)
20:22:45 <cdecker> roasbeef: same here :-) I tend to get cross-eyed the longer the PR thread
20:22:58 <t-bast> I added two fields to help debug what we dislike in the tlv stream: joost proposed using a tlv format which could be interesting
20:24:54 <t-bast> I think the onion failure code and the NODE, PERM, BADONION and UPDATE bits are useful so I wouldn't migrate all errors to using just TLV. But for the content of failures, should we use TLV instead of a fixed structure?
20:25:36 <lndbot> <joost.jager> hi all. yes, i see advantages of converting the failure message to tlv. it would also allow us to have a custom reply, just as we have can have a custom record in the hop payload.
20:25:44 <roasbeef> we don't have much space on the errors to being with, so do we really want the extra signalling over head/
20:25:47 <roasbeef> ?
20:25:49 <lndbot> <joost.jager> can't those bits be reconstructed using a simple mapping?
20:26:08 <roasbeef> how do you know if the sender understands this new format/
20:26:09 <roasbeef> ?
20:26:22 <cdecker> Hm, I get browser user-agent flashbacks with this sort of thing tbh
20:26:26 <t-bast> roasbeef: I agree that this is where we're the most space-constrained, so maybe we want to keep the current format
20:26:30 <lndbot> <joost.jager> introduce a new failure code now that will contain a tlv stream with everything that is defined from now on
20:26:31 <roasbeef> (if talking of replacing the current bits w/ new types)
20:26:31 <rusty> I think this is useful for diag, but it's a "shouldn't happen" kinda case, so it's hard to get too upset about the format detals.
20:26:53 <cdecker> I think errors ought to be short and concise, and hints at best, not a fully fledged debugging facility
20:27:07 <t-bast> I agree with cdecker
20:27:20 <t-bast> And the parsing should always be very simple, so maybe we shouldn't mix TLV into it?
20:27:49 <cdecker> If a node errors out we just drop it from future communication (would also incentivize people to build robust networks and update their nodes in time)
20:27:59 <lndbot> <joost.jager> we now already have variable length failure messages because of older versions. checking EOF whether to keep on reading. that could disappear with tlv
20:28:35 <cdecker> By EOF you mean the framing of the message?
20:28:55 <lndbot> <joost.jager> some errors like invalid_payment_details, we have been adding new fields over time
20:29:11 <lndbot> <joost.jager> in reading those errors, we need to be aware that there are multiple versions of it
20:29:33 <roasbeef> cdecker: EOF as in if there's any bytes left in the steam
20:29:35 <t-bast> But we're still constrained by the fixed size of the failure message, which adds some complexity when putting TLV inside it
20:29:40 <lndbot> <joost.jager> v0 had no params, v1 had an amt and v2 an amt and height
20:29:49 <lndbot> <joost.jager> eof, no bytes left
20:29:59 <lndbot> <joost.jager> and we cannot get rid of params that we don't use anymore
20:30:01 <t-bast> I think that using tlv here will make less obvious that messages don't overflow the failure message size
20:30:14 <lndbot> <joost.jager> like amt for invalid_pay_details, which is redundant really
20:30:44 <cdecker> Hm, I see your point
20:30:53 <lndbot> <joost.jager> overflow check could be done at any point
20:32:04 <t-bast> I like the fact that when you read the spec, you easily see that all the failure messages are well below the 256 bytes limit. If we use TLV, it's going to be less obvious...
20:32:12 <lndbot> <joost.jager> and the idea of a custom failure message also has some merit i think. suppose you send an htlc with a custom 'exchange deposit address'. the recipient could add a custom app-specific failure to the tlv stream like 'deposit limit exceeded'
20:32:44 <lndbot> <joost.jager> (defined in the 2^16+ range)
20:33:45 <t-bast> I honestly don't have a strong position for either choice
20:33:54 <roasbeef> so sounds like you're talking aobut an entirely new custom failure message now joost?
20:34:08 <lndbot> <joost.jager> imo flexbility in the format, saving bytes by not being forced to encode legacy fields and the ability to add custom data outweighs an easy length verification in the spec
20:34:14 <cdecker> Ok, that'd be a more involved change though
20:34:17 <lndbot> <joost.jager> no, not entirely new. just a new failure code within the current msg format
20:34:22 <cdecker> Let's focus on the specific case at hand here
20:34:24 <roasbeef> we can't remove any of those legacy fields at this point
20:34:40 <roasbeef> ok a new failure code for custom messages, but seems out of the scoep of this new error for invalid tlv payloads
20:34:41 <lndbot> <joost.jager> not those, but any new fields that we'd add to the tlv stream that may become legacy later on can be removed
20:34:51 <cdecker> roasbeef: we could try, and then start crying when we see how many nodes crash xD
20:35:19 <lndbot> <joost.jager> strictly speaking out of scope, but when is a good time to do this otherwise? i think it is a nice opportunity now that a new failure code with the current drawbacks is about to be added
20:35:36 <lndbot> <joost.jager> especially if people want to add some custom debugging tlv data
20:36:23 <t-bast> Ok I think there's a will to start using tlv in the future in onion errors as Joost points out, which could make sense. So I'm proposing to update my PR to use a tlv stream in the error code I'm adding to go in this direction, and another PR can introduce a more generic TLV error code for the use-cases Joost mentions. What do you think?
20:36:55 <t-bast> This way we solve the current issue (which is a specific error code for onion invalid payloads) and we open the door for more changes later.
20:36:58 <cdecker> t-bast: how about we merge your PR, without the hint fields and then we can bikeshed the payload to add?
20:37:11 <cdecker> Adding fields is always allowed :-)
20:37:23 <cdecker> And we'd just be adding a single TLV-stream field
20:37:29 <lndbot> <joost.jager> ending up with two failure codes that have tlv stream associated may be less desirable
20:37:31 <cdecker> (once we nail down the specifics)
20:37:34 <roasbeef> changing the error code fro this would be a bigger divergence, as all the existing error messages use this bitfield format, this can be considered a "core set"
20:37:39 <t-bast> cdecker: I like that proposal
20:37:45 <roasbeef> and we also don't know how to know if the sender supports the new error codes
20:37:59 <t-bast> joost: I disagree, I don't think the tlv field should replace the current error code
20:38:32 <t-bast> the current error code (first two bytes) is really useful to aggregate common failure scenario (like just probing for the PERM bit to be set)
20:38:52 <lndbot> <joost.jager> t-bast, you want to add a tlv stream to every future failure code that has additional data?
20:39:06 <t-bast> exactly instead of fixed encoding
20:39:11 <cdecker> I agree with t-bast, we can talk about making the payload after the type a TLV, not replace the entire error with just a TLV bag
20:39:44 <lndbot> <joost.jager> it is a nice intermediate step. doesn't allow for custom failure message though. or at least not in the same way
20:40:11 <cdecker> joost: not sure we _want_ custom failures at all tbh
20:40:14 <rusty> The only purpose for these error codes is to print in a debugger.
20:40:45 <cdecker> I want to avoid at all costs that LN becomes a general purpose communication network
20:41:09 <roasbeef> if that's all y'all use them for, then you're not integrating some useful information into your path finding attempts
20:41:09 <t-bast> rusty: what do you think of Joost's comment to use onion error to forward an application-level error to the sender? I think it could make sense in the future for lightning apps.
20:41:23 <roasbeef> cdecker: pretty long distance from a custom error to general purpose communication
20:41:25 <lndbot> <joost.jager> haven't brainstormed much about use cases, but it is the complementary in a way of a custom record in the updateadd payload.
20:41:51 <rusty> roasbeef: the broad error codes, yes, the sub erorr codes, like which TLV field failed to parse?
20:42:21 <t-bast> I think we may be spending a bit too much time on this, I'll update the PR to go in the direction cdecker proposed, and we can re-visit a broader change in another PR. Sounds good?
20:42:27 <cdecker> rusty: we also use the channel_updates in the errors :-)
20:42:29 <rusty> t-bast:  see cdecker's point about creating a general comms mechanism...
20:42:46 <rusty> Meanwhile, I
20:43:17 <roasbeef> not comms, just additional erroc context, comms would be adding an opauqe payload to each htlc or error
20:43:35 <rusty> I think I've decided that our feature bits are too complex for any human, and am trying to simplify them into one set of feature bits, re 557.
20:43:36 <roasbeef> but i'm for just proceeding with that error as is, then later on examining allowing axtra daata at the end of errors
20:43:42 <cdecker> Ok, shall we go with the minimal viable PR (error code, without payload) now and bikeshed the payload in a new PR?
20:43:44 <roasbeef> for the ones that have chan upddates, there isn't much room anyway fwiw
20:43:48 <rusty> roasbeef: "t-bast: rusty: what do you think of Joost's comment to use onion error to forward an application-level error to the sender? I think it could make sense in the future for lightning apps."
20:44:08 <rusty> cdecker: ack
20:44:11 <t-bast> cdecker: ack
20:44:13 <roasbeef> ?
20:44:25 <t-bast> #action t-bast to simplify the PR - broader change deferred
20:44:41 <roasbeef> what simplification?
20:44:56 <roasbeef> looks fine as is to me
20:45:04 <t-bast> only add the error code but no content yet
20:45:16 <roasbeef> the content is useful for debugging
20:45:25 <roasbeef> otherwise you get no feedback
20:45:36 <t-bast> yes but if we want to switch it to use TLV later it's a breaking change and has to use another error code...
20:45:42 <cdecker> roasbeef: but we have a competing proposal for a different encoding, hence we postpone decision on the format of the content
20:45:57 <roasbeef> but then it woudl go in, defined as having no payload?
20:46:12 <cdecker> Exactly, but adding fields is allowed
20:46:15 <rusty> I somewhat agree with roasbeef here.  If we're not enclosing any info, why bother merging it
20:46:32 <roasbeef> yeh we just use invalid onion versino atm in place of this
20:46:34 <t-bast> But then do you want that content to be TLV or fixed-encoding as currently proposed?
20:47:18 <lndbot> <joost.jager> the problem with invalid onion version is that it isn't clear which node to blame. this error can be trigger by node x+1 sending a malformed htlc error to node x, who passes it back to the origin
20:47:39 <rusty> t-bast: fixed encoding is fine, TBH.  I mean, TLV is nice in theory, but it's overkill AFAICT.
20:48:15 <t-bast> allright, so should we then even merge it as-is? Or do you see something that should change roasbeef / rusty ?
20:48:39 <roasbeef> i'm good w/ it as is, and also think that custom error is an interesting direciton that warrants it's own PR
20:48:59 <rusty> Yes, I would have voted to merge as-is.  Frankly, if we decide TLV is the ONE TRUE WAY later, I think we'll find that no implementation is relying on this format for anything meaningful.
20:49:25 <rusty> I mean, you're not supposed to be using it to probe what TLVs the node understands...
20:49:25 <t-bast> Good! So let's merge this and work on another proposal to integrate TLV in there.
20:49:46 <t-bast> #action merge 627, re-visit TLV in onion failures later
20:49:51 <t-bast> #topic 557
20:49:55 <cdecker> ack
20:49:55 <t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/557
20:50:10 <t-bast> Rusty you started mentioning something about 557?
20:50:14 <roasbeef> as far as prbing, nodes already signal what they understand in feature bits, unless they're trying to be sneaky...
20:50:37 <rusty> t-bast: yes.  I was reworking it with the new naming, and it got horribly confusing.
20:50:53 <t-bast> By new naming you mean feature bit unification?
20:51:07 <rusty> t-bast: yes.
20:51:24 <t-bast> Can you share why it becomes confusing?
20:51:35 <rusty> t-bast: we have three places where feature bits are used.  In the init msg there are two distinct sets.  In the channel_announce there is one, in the node_announce there is one.
20:51:56 <t-bast> And those don't map cleanly to one of the two feature types?
20:52:03 <rusty> t-bast: yeah.
20:52:26 <rusty> t-bast: I think we should simply have one set of feature bits.  And each one specifies whether it's places in the channel_ann and/or node_ann.
20:52:57 <rusty> t-bast: we leave the "global feature set" in initmsg as an empty set.
20:53:16 <t-bast> If we have only one set of feature bits, then shouldn't we always include them in both channel_ann and node_ann?
20:53:21 <rusty> This solves the naming problem: we jsut have "feature bits"/
20:53:45 <t-bast> Heh, nice way of avoiding picking different names xD
20:53:53 <cdecker> Simplification you say? Count me in ^^
20:53:56 <roasbeef> how's this related to 557?
20:54:17 <rusty> roasbeef: sorry, #571  oops
20:54:25 <t-bast> ah ok, switching the topic
20:54:30 <t-bast> #topic 571
20:54:35 <t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/571
20:54:54 <t-bast> I thought channel queries feature bits made you realize that :)
20:56:19 <rusty> So anyway, I'll create a replacement for 571 after the meeting with this simplification, and some examples of how various proposed features would work.  But generally a feature wants to be in node_ann (so people can find you), and *may* want to be in channel_ann if it effects how you use that channel.
20:56:39 <roasbeef> yep, like tlv stuff for example
20:56:47 <roasbeef> as rn we don't have very good ways of syncing node anns either
20:56:58 <roasbeef> tlv onion stuff*
20:57:01 <rusty> roasbeef: exactly.
20:57:14 <t-bast> Ok, curious to see that new PR then
20:57:22 <rusty> Yeah, sorry I didn't get it done in time.
20:57:28 <rusty> OK, let's move on?
20:57:34 <t-bast> #action rusty to create a new PR to supercede 571
20:57:49 <t-bast> #topic 557
20:57:54 <t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/557
20:58:01 <niftynei> need to hop off :wave:
20:58:10 * t-bast waves at niftynei
20:58:20 <t-bast> sstone has tested CL and Eclair compatibility for 557
20:58:31 <t-bast> With the latest changes applied, it seems everything is working fine
20:58:42 <t-bast> Rusty, did you have time to test it yourself?
20:59:31 <rusty> Yes, I have tested it against our updated protocol tests, and it's all good.
20:59:35 <rusty> It's quite a simple change really.
20:59:49 <t-bast> Ok, so shall we merge this spec PR and LL can implement later?
20:59:52 <rusty> It's running right now on my mainnet and testnet test nodes.
20:59:54 <t-bast> roasbeef does that sound good to you?
20:59:56 <rusty> t-bast: ack.
20:59:58 <t-bast> rusty: great!
21:00:56 <roasbeef> sgtm, it isn't very high on our prio list, would want to give conner a chance to sign off since he's been reviewing it though (haven't been following the PR too closely myself)
21:01:39 <t-bast> I think he signed off last time. I'll double-check the logs from last IRC meeting. If he did sign-off on it, let's merge it, otherwise let's wait for him to ACK.
21:01:44 <rusty> roasbeef: it's nice because it gives a way of querying node_ann as a side-effect.
21:01:59 <t-bast> exactly ;)
21:02:01 <rusty> t-bast: agreed.  merge pending confirmation from bitconner
21:02:24 <t-bast> #action get conner's ACK and then merge
21:02:33 <t-bast> #topic 656
21:02:38 <t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/656
21:02:49 <t-bast> This is the feature bits in the payment invoice
21:03:06 <t-bast> It seems to me that we all agree on it and reproduced the test vectors, so should be ready to be merged?
21:03:28 <t-bast> There are small PR comments on wording though.
21:03:54 <rusty> t-bast: ack.  The wording review is separate, since that "fail the payment" wording is used throughout.
21:04:28 <t-bast> ok, it all looks fine to me so I would merge it. roasbeef did you have a look at it? conner signed off last week.
21:04:32 <rusty> t-bast: I'd like to get it merged, since until it's merged we can't have compulsory bolt11 upgrades.
21:04:39 <t-bast> rusty: agreed
21:05:01 <t-bast> but roasbeef is conner's boss so maybe he doesn't trust conner's sign-off :D
21:05:48 <roasbeef> nope haven't looked at it, but looks like johan has some comments on it
21:06:04 <roasbeef> I think our PR is up to date tho
21:06:09 <roasbeef> the one implementing in lnd
21:06:23 <t-bast> yeah sounds like conner reproduced the test vectors last time I checked
21:06:24 <roasbeef> oh it's merged :o
21:06:29 <t-bast> oh really?
21:06:35 <roasbeef> so looks to be g2g
21:06:37 <t-bast> so we should definitely merge the PR spec then ;)
21:06:44 <roasbeef> lehdoit
21:06:46 <t-bast> #action rusty to merge 656
21:06:55 <rusty> Yay!
21:07:02 <t-bast> what do you want to discuss next? static_remote or AMP?
21:07:30 <rusty> I think we'll only have time for one, we're already over.  static_remote?
21:07:42 <t-bast> static_remote is probably faster
21:07:47 <t-bast> roasbeef that ok for you?
21:08:04 <roasbeef> static sgtm
21:08:04 <rusty> I have it implemented, with protocol tests.  I think we're waiting on interoperation tests?
21:08:07 <t-bast> #topic 642
21:08:11 <roasbeef> final on static is if to remove or not
21:08:13 <t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/642
21:08:21 <t-bast> we had one comment on static_remote
21:08:23 <roasbeef> not remove the field imo as it makes parsing more context dependent
21:08:37 <roasbeef> as you now need to know the feature bits advertised to parse it, as we still need to support both version s
21:08:39 <t-bast> Pierre-Marie mentioned on the mailing list that we could leverage this to do symmetric CSV delays
21:09:13 <t-bast> and brute-force csv values (but drawback is no way to use this in a bitcoin wallet directly, needs a dedicated tool)
21:09:34 <roasbeef> final thing on mpp/amp seems to be a partial unification a la conner's last commnet: https://github.com/lightningnetwork/lightning-rfc/pull/658#issuecomment-525564887
21:09:45 <roasbeef> leverage what?
21:09:59 <t-bast> leverage this opportunity to change the scripts
21:10:09 <t-bast> since we're changing the key derivation slightly
21:10:30 <rusty> t-bast: but there's going to be another chance to do that with dual-funding anyway...
21:10:47 <t-bast> rusty: is that coming soon?
21:10:53 <t-bast> soon-ish?
21:11:17 <rusty> t-bast: yes, niftynei is implementing and testing now.  Lots of variables there to bikeshed though.
21:11:21 <t-bast> It felt like a good opportunity to fix this incentive mis-alignment
21:11:33 <roasbeef> t-bast: it's been segmented, many weeks ago
21:12:02 <roasbeef> doing csv here also makes the recoveyr case that this PR strengthens less compelling
21:12:08 <t-bast> that's great to hear, I'm curious to see the dual-funding proposal
21:12:35 <roasbeef> would imagine many wouldn't enable dual-funding until the privacy leaks are patched, or mitigated somewhat
21:12:41 <roasbeef> but stopic is static right now...
21:12:59 <roasbeef> or we're ending/
21:13:00 <roasbeef> ?
21:13:17 <t-bast> So it sounds like both LL and CL would like to fix symmetric delays in another change right? And have static_remote not bother with that?
21:13:23 <rusty> roasbeef: shall we assign feature 12/13 to static?  That's next one I think.
21:13:35 <rusty> t-bast: indeed, defer.  BUt maybe indefinitely.
21:13:41 <t-bast> heh
21:13:55 <t-bast> ok, apart from that the current proposal for static_remote sounded good to us
21:14:15 <rusty> t-bast: OK, shall we assign proper feature bit so we can move to compat testing?
21:14:15 <roasbeef> also, I think i'm gonna start having a call concurrently w/ this meeting that ppl can optionally jump on, let's no kid ourselves IRC is a horrble discussion medium for something like this and has slowed down progress since communication is always so ambgiuous and requires many clarifications
21:14:32 <roasbeef> the change to channel reest is still unresovled rusty
21:14:37 <roasbeef> see my last comment there?
21:14:46 <roasbeef> 12/13 sgtm
21:14:51 <t-bast> rusty: sgtm
21:15:25 <rusty> roasbeef: you mean reconnect without the feature bit?
21:15:48 <t-bast> roasbeef: totally agree, and that brings me to another general point: do we want to have a spec meeting IRL during the LNConf? I think this would be invaluable.
21:15:58 <rusty> t-bast: ack!
21:16:55 <rusty> roasbeef: I'm confused as to what the unaddressed comment is?  I hit reload....
21:17:27 <roasbeef> rusty: https://github.com/lightningnetwork/lightning-rfc/pull/642#discussion_r319622705
21:17:40 <roasbeef> removing the field or not
21:18:30 <rusty> roasbeef: no way, that field was a huge underspecified mistake and must go.
21:18:51 <roasbeef> it adds additional parsing complexity
21:19:00 <roasbeef> as we we can't remove these legacy fields in a clean way
21:19:14 <roasbeef> you'd just continue to write those 32 bytes, and everythign else follows and is uniform
21:19:32 <roasbeef> vs needing to conditionally know the current set of feature bits, to know when to start reading/writing w/e future tlv extensions
21:20:22 <rusty> roasbeef: but this is true of the current parsing, since you need to know feature bits as to whether those fields are valid.
21:20:56 <roasbeef> no today you just read them all
21:21:00 <roasbeef> since we don't have optional fields
21:21:14 <roasbeef> our parsing doesn't have any feature bit awareness as is
21:21:18 <rusty> And in the long run, it's neater.  We have a *lot* of confusing logic to try to get that field correct, and deleting it is the entire point of this change.
21:21:33 <roasbeef> deleting it adds _more_ code
21:21:45 <rusty> roasbeef: for now, sure.
21:21:47 <roasbeef> yeh you don't need to udnerstand it, jsut to be able to read it out
21:21:59 <roasbeef> understand as in the entire dlp protocol, this is just about making parsing uniform
21:23:01 <rusty> roasbeef: you already need to know the features so you know what to do if field is missing.  You need to allow missing fields, UNLESS option_dataloss is negotiated.
21:23:28 <roasbeef> if it sin't there, you know becuase the stream just terminates, if it is, you continue
21:23:39 <roasbeef> if tlv data is added you need additional context to parse
21:23:45 <roasbeef> we've never revmoed a field yet for a reason
21:23:49 <rusty> roasbeef: if it isn't there, you need to abort the channel, since they're cheating.
21:24:38 <rusty> In our case, we use different parse routines for dataloss_protect and for non-dataloss_protect.  So this simply adds another.
21:25:26 <rusty> TBH I'd rather replace it with all zeroes than leave it intact.
21:25:30 <roasbeef> yep which adds more code vs keeping it unfirm
21:25:35 <roasbeef> yeh you can write all zeros even
21:26:11 <rusty> IN a few years' time that's gonna be really ugly.  I guess we all get scars though, and I can blame you :)
21:27:13 <rusty> OK, so shall we say "reader MUST ignore my_current_per_commitment_point iff option_static_remotekey is negotiated"?
21:27:28 <rusty> That lets you put anything in there, wahtever is easiest.
21:28:14 <t-bast> Sounds like a good compromise
21:28:21 <roasbeef> sgtm
21:28:30 <roasbeef> should be ready to do interop testing this week
21:28:46 <t-bast> great!
21:29:10 <cdecker> ack
21:29:10 <rusty> #action rusty to update #642 with new feature bit, restore (and ignore) my_current_per_commitment_point
21:29:22 <t-bast> rusty: do we need to connect with someone to reserve a time slot / meeting space during the LN Conf for a spec meeting?
21:29:37 <rusty> roasbeef: cool, I'll do that PR today, then update node.
21:29:41 <rusty> t-bast: I'll ask...
21:29:48 <t-bast> rusty: thx
21:30:09 <t-bast> Ok we've done a lot tonight, thanks all! Is there something else you'd like to discuss?
21:30:13 <cdecker> We can just subvert one of the panels ^^
21:30:35 <t-bast> cdecker: or hide in a dark room and do secret stuff
21:32:39 <t-bast> ok sounds like we're good to go then
21:32:42 <roasbeef> heading off now to smoke some meats, but would be nice to continue to make progress on the mpp/amp stuff in their respective PRs
21:32:43 <cdecker> Wait, panels aren't in dark secretive rooms?
21:32:55 <t-bast> I agree with roasbeef
21:32:58 <cdecker> roasbeef++
21:33:12 <t-bast> #action make progress on the AMP and MPP PRs
21:33:20 <t-bast> #endmeeting