Lightning Specification Meeting 2021/08/30 #901

t-bast · 2021-08-27T08:25:49Z

t-bast · 2021-08-27T08:26:24Z

As usual, feel free to suggest/add topics if you want to see specific PRs or issues covered.

t-bast · 2021-08-31T07:11:02Z

The bot was on vacation, here are the meeting logs:

<t-bast> #startmeeting LN Dev Meeting
<niftynei> #link agenda https://github.com/lightningnetwork/lightning-rfc/issues/901
<t-bast> #ZeBotIsDed?
<ariard_> again
<BlueMatt> cdecker[m]: ?
<BlueMatt> well lets go anyway
<t-bast> let's do without, thanks niftynei for the link
<BlueMatt> already late
<niftynei> :thumbs_up:
<t-bast> #topic channel type
<t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/880
<t-bast> Are we good to go with that one?
<t-bast> roasbeef: on that one, regarding your comment for empty feature bits, don't forget the different between having the tlv present with empty feature bits and having the tlv missing
<t-bast> TLV present with empty feature bits = default channel (pre-static_remotekey)
<t-bast> TLV absent = old negotiation based on each node's init feature bits
<roasbeef> t-bast: what about a node that sets explicit to static remote key as a required bit?
<roasbeef> those nodes (like us) no longer support pre static remote key funding 
<rusty> Looks like it's acked, needs typo fix
<roasbeef> but yeh I think lets land it 
<t-bast> roasbeef: then just don't accept this channel type, people shouldn't try it anyway since you require static_remotekey
<roasbeef> t-bast: since otherwise, you'd force a downgrade?
<roasbeef> yeh sure
<t-bast> great then rusty I'll let you fixup the typos and we can merge!
<t-bast> #action rusty to fix the tiny typos
<t-bast> #action merge PR
<t-bast> #topic Peers should check remote dust limit
<t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/894
<BlueMatt> mostly need feedback from roasbeef on https://github.com/lightningnetwork/lightning-rfc/pull/894#discussion_r697111408
<BlueMatt> rusty: noted we could just do "mutual close must be segwit" in 847 to resolve it
<rusty> (Can be put 824 on the Agenda pls too... merging this would help kill the previous anchor, and light a fire under me to handle complex tx deps properly so we can properly CPFP)
<BlueMatt> and then we can just say dust limit below 354 is not allowed
<t-bast> What I'd like to discuss now is the comment BlueMatt mentioned, about shutdown and non-segwit outputs
<ariard_> yeah looking on 824 rn if we have forgot nothing
<roasbeef> rusty: +1, totally forgot about that one lol
* smartin has quit (Quit: smartin)
<t-bast> Let's do 824 right after #894?
<roasbeef> BlueMatt: is that linking to the right comment?
<BlueMatt> mostly, roasbeef are you ok with requiring segwit shutdown scripts?
<roasbeef> ah sure, makes sense 
<roasbeef> why'd you do anything else tbh
<BlueMatt> yea, basically
<roasbeef> would*
<BlueMatt> roasbeef: yes, I linked to the correct comment
<BlueMatt> roasbeef: its all interrelated cause the dust limit set in the channel impacts how cooperative-close outputs are included or not
<rusty> Yes, let's burn the boats... it's been years, anyway.
<t-bast> So just to be sure: shall we remove the option to do non-segwit in shutdown from the spec directly in #894?
<BlueMatt> so dust limit on the channel can interact with cooperative close
<BlueMatt> t-bast: I dont care how it lands
<BlueMatt> rusty: suggested in 847
<roasbeef> BlueMatt: gotcha, makes sense 
<BlueMatt> which we should also disucss
<BlueMatt> t-bast: I'd say put it in 894 cause 847 is already delayed enough
<BlueMatt> then we can Just Merge 894 with a "dust limit below 354 is not allowed"
<t-bast> Can you share after the meeting how you got to 354, in a gist or something? My calculations and bitcoind tests didn't get this value anywhere
<roasbeef> ahh the elusive dust derivation 
<BlueMatt> I'd rather you calculate it fresh and tell me if you agree :p
<roasbeef> lmao
<ariard_> t-bast: maybe we should precise if the scriptpubkey is unknown SegWit v1+, allow any dust limit
<t-bast> Ok I will push to #894 to remove the non-segwit option
<t-bast> BlueMatt: I did, I found 294 sats and 330 sats detailed in the PR ;)
* roasbeef summons crypt-iq
<BlueMatt> t-bast: oh, i missed that, I'll follow you. t-bast did you test for maximum-size segwit v-X
<BlueMatt> t-bast: which IIRC is like 42 byte sPKs or so
<ariard_> 330 sats for p2wsh match my calculations iirc
<BlueMatt> what about p2-witness-v2-length-40-bytes?
<t-bast> BlueMatt: I just took bitcoind's code for calculating a dust value, even though some choices there are arbitrary, they're the default rule of the network so I went with it
<BlueMatt> t-bast: yes, did you calculate it for max-length witness output which is a future witness version?
<BlueMatt> t-bast: cause those are standard and allowed today
<t-bast> ariard_: good point for segwit v1+, I'll think about that and amend the PR
<t-bast> BlueMatt: not yet, I indeed should add that
<BlueMatt> alright, moving on?
<t-bast> #action t-bast to remove non-segwit option in shutdown and clarify dust for segwit v1+
<t-bast> #topic anchor_output_htlc_zero_fees
<t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/824
<roasbeef> also wait remove non-segwit w/o addition of a feature bit (y'all know how much I love feature bits) 
<BlueMatt> roasbeef: yes, without a feature bit, but i think that's why rusty was suggesting it in 847
<ariard_> just one remark on 824, i would make it clearer HTLC-success/timeout must be zero fees for trimmed outputs computation
<BlueMatt> roasbeef: does anyone send non-segwit outputs for closing by default (i assume most will allow user-specified, but still)
<roasbeef> idk, we don't do it by default, but maybe another impl like BLW (I think that's still around?) does 
<roasbeef> in theory someone could set it using a manaul up front shutdown addr, and we'll accept it and store it 
<BlueMatt> sure, but you aren't re-negotiating the dust fee on that channel either
<BlueMatt> but, worst case you force-close while closing
<BlueMatt> which isn't exactly the worst thing in the world
<roasbeef> but an existing channel can advertise it and we'd either need to accept it or deny 
<t-bast> ariard_: yes the only thing I'm doubting where we may have incompatibilities is trimmed outputs computation for this PR
<roasbeef> there's potential to handle it more gracefully tho 
<rusty> Yeah, we'll have to go through a deprecation cycle where we stop people trying to close to non-segwit, and probably immediately stop them from making them upfront...
<roasbeef> force close may not be the worst thing to you, but ofc users hate it 
<roasbeef> rusty: so something something optional feature bit turning into required?
<BlueMatt> I mean you currently allow counterparties to create unbroadcastable shutdowns
<rusty> roasbeef: we could, but if nobody every notices, is it worth the hassle?
<BlueMatt> so you gotta force-close anyway :)
<t-bast> I'm mostly of the opinion here that this isn't worth the hassle indeed...at the implementation level you can choose to keep accepting non-segwit scripts for a while if you want
<roasbeef> idk, it's a weird move imo to make previously co-op closeable channels now uncloseable w/o a force close 
<roasbeef> analogous (but ofc not the same) to making prior spendable outputs now unspendable onchain 
<BlueMatt> but, indeed, you can/should accept them for some time yet
<BlueMatt> the feature bit doesn't really change anything
<BlueMatt> you accept them until you dont
<BlueMatt> and at that point you dont accept them
<roasbeef> tho as t-bast mentions that might be a side effect of some of the newly proposed dust smenatics? 
<BlueMatt> a feature bit doesn't change that process realy
<roasbeef> yes it does, it makes it explicit 
<BlueMatt> you can send an error message which is pretty explicit :)
<roasbeef> kek
<t-bast> roasbeef: not really because right now lnd doesn't validate the remote dust, so you let people close to dust which is an issue...
<t-bast> roasbeef: the issue there is that you accept a closing_tx that won't be able to relay, so you will have to force-close (with the current code in lnd/c-lightning at least)
<BlueMatt> but, seriously, I dont see how its any more or less explicit with a feature bit
<BlueMatt> roasbeef: ?
<roasbeef> BlueMatt: w/ a bit, you know not to send me non-segwit close, sending a non-segwit addr in a chan open is explcitily invalid 
<roasbeef> chan open as part of an upfront shutdown
<BlueMatt> presumably you'll not send them starting tomorrow
<BlueMatt> so it doesnt change your behavior at all
<BlueMatt> if someone sends you an old script, you know they are old
<BlueMatt> call it an implicit feature bit :)
<roasbeef> so from an API perspective, we need to start to reject chan open attempts that use a non-segwit upfront shutdown addr? 
<t-bast> roasbeef: that's an implementation detail, for channels created before you change this code that have a non-segwit upfront, you would keep allowing it (a feature bit wouldn't make it simpler, would it?)
<BlueMatt> you can even calculate an implied feature bit after you get a channel open :)
<roasbeef> vs allow it now, signal to users, add the bit, then depecrate it over time w/ a quired bit?
<BlueMatt> roasbeef: that's up to you. you either reject them or don't reject them as you see fit, the spec will say SHOULD, and you can do it on the timeline you like
<BlueMatt> you can deprecate it over time whether there's a bit or not
<roasbeef> t-bast: ok I see what you're saying, yeah you need to allow them to keep using it
<t-bast> roasbeef: exactly, we should in the next releases prevent people from closing to non-segwit scripts, and in the one after that (or N releases later) reject non-segwit in shutdown
<BlueMatt> in fact, I'd say, your code should probably be on the same schedule either way
<t-bast> roasbeef: with the exception of already existing upfront channels
<roasbeef> idk just feels like we're ignoring some tools here that make updates like this easier, and w/ the way the spec is someone could  have an older impl, boot it up and now need to pay a ton of fees to force close everything that they could've just co-op closed before w/ their peers 
<t-bast> roasbeef: on the contrary, it would be worse with a feature bit
<roasbeef> if it's just us tho, then yeah we can prob just do this, and let ppl decide how to handle it on the API perspective 
<BlueMatt> aren't you the one who hates adding bits on the wire cause it makes messages larger :p
<rusty> roasbeef: I wonder if anyone has upfront shutdown scripts which are non-segwit today?  They'll be the real losers.
<t-bast> roasbeef: an old node waking up and seeing a required feature bit it doesn't about will brick everything, whereas they're likely using segwit which means everything would be fine to just continue as it went before
<roasbeef> BlueMatt: not bits generally, just very high ones in the 10s of thousands, but we know how to solve that if we want to (sparse encoding) 
<roasbeef> since as is the "experimental" bit section, means a few KB of zeros at times 
<roasbeef> t-bast: true 
<roasbeef> rusty: good q, idk, but in theory there may be lnd nodes in the wild w/ that, guess they'll have to eat the fees or w/e, hopefully they aren't too angry about that lol
<roasbeef> ok I guess the old ppl will just have to deal w/ it 
<rusty> ... actually, none of my peers atm have set upfront_shutdown_script, so maybe it's not normal
<t-bast> since lnd implemented shutdown script recently, I'd be surprised if there were many of them using non-segwit
<t-bast> same for eclair, we implemented it recently, and I think none of our peers use it yet
<BlueMatt> we had been setting it aggressively, but bluewallet got mad and wanted it off
<BlueMatt> so I guess many users actively dislike it :p
<ariard_> well someone might have a weird use-case, closing to an OP_RETURN output flag
<t-bast> That's probably because it's not a very good protection against anything...
<t-bast> Because if your node gets hacked, upfront shutdown script will not save you
<BlueMatt> no, it doesn't do very much
<roasbeef> t-bast: recently? we've had it for a while now: https://github.com/lightningnetwork/lnd/pull/3655 (2 years ago) 
<BlueMatt> anyway, so it sounds like roasbeef agrees, should we move on?
<rusty> roasbeef: t-bast is showing his age :)
<roasbeef> our main use case in the servies we run is to avoid an extra sweep transaction into another wallet when somethign gets co-op closed 
<t-bast> roasbeef: it's 2 years ago already??? I remembered that carla implemented that, and I thought she joined...somewhat recently...damn time flies!
<roasbeef> yeah we're all old now....
<roasbeef> 824 now?
<BlueMatt> yea
<t-bast> I'll add a commit to #894 where I'll mention everything we discussed here and we can review that on github
<BlueMatt> thanks, t-bast sounds like we agreed on no feature bit
<t-bast> :+1:
<rusty> +1
<t-bast> Let's go for #824
<roasbeef> so 824 is pretty old, totally forgot about it, has an approval from rusty, an impl for a while in lnd, and is linked in the chan_type PR 
<t-bast> Does anyone have a working implementation? As ariard_ mentioned, I think the only place where we may have a misunderstanding is the trimmed to dust calculation
<roasbeef> johan doesn't really work on LN stuff anymore, but I can take over the PR (I see it has a fixup commit), tho maybe I can ping him to rebase it if needed 
<t-bast> roasbeef: can you do a quick check about this trimmed to dust case?
<ariard_> Not yet, on my todo once we're done with 0.0.101 on the LDK side
<roasbeef> sure, you have a link to the comment? 
<rusty> Yeah, while we don't have a second impl, it's deployed, trivial and helps kill the anchor-with-fees previous one.
<t-bast> roasbeef: or you can just use the `squash and merge` github option :)
<roasbeef> ahh I see 
<roasbeef> t-bast: ayee true
<roasbeef> I think we already to that in our impl, can double c heck sec 
<t-bast> roasbeef: https://github.com/lightningnetwork/lightning-rfc/pull/824#pullrequestreview-742028638
<t-bast> I'm ok with merging it if we're sure the trimming to dust works as we'd expect (not including HTLC 2nd-stage fees)
<ariard_> I think lnd is already implementing the 0-fees on trimming-or-not HTLC outputs
<roasbeef> ok yeh confirmed we do it as zero 
<ariard_> and I intend to implement it that way for LDK, though would be nice to clarify
<roasbeef> https://github.com/lightningnetwork/lnd/blob/master/lnwallet/commitment.go#L286
<t-bast> I intend to implement it that way as well for eclair, so LGTM
<roasbeef> ok, so just need to make that explicit in this PR?
* crypt-iq (~crypt-iq@2603-6080-8f06-6d01-c534-15e3-2e3d-d67a.res6.spectrum.com) has joined
<roasbeef> ok no that's already there: https://github.com/lightningnetwork/lightning-rfc/pull/824/files#diff-6bed824879b760d329ec379b16a1d0e78ffba034fdfa13b95cf0480e09fa7c4bR421 
<roasbeef> so we're g2g on this one?
<ariard_> yeah this section is just coming *after* the trimmed outputs ones and might confuse the reader
<ariard_> see also the point on "MAY combine the HTLC transaction with other transactions" which I would recommend not to do
<t-bast> There's a comment by rusty about the fact that it doesn't depend on static_remotekey...?
<t-bast> https://github.com/lightningnetwork/lightning-rfc/pull/824#discussion_r579892815
<rusty> Oh yeah, it's gotta depend on static_remotekey.  I mean, in practice ofc it will, but let's make it clear.
<roasbeef> t-bast: I think that's an artifact of the way we impl'd it, given that we always have a non-static key for anything anchor based, since felt natural to build on top of it 
<roasbeef> I can add that 
<rusty> non-static for anchors?  Is that a typo?
<roasbeef> err yeh static
<rusty> Phew!!
<roasbeef> anchors means static
<roasbeef> requires it
<t-bast> roasbeef: I see what you mean, that's something that kinda bothered me as well but since anchor_outputs depends on static_remotekey anchor_outputs_zero_fee should to
<roasbeef> I guess it's kinda transitive as written rn 
<roasbeef> since: https://github.com/lightningnetwork/lightning-rfc/pull/824/files#diff-c3e160eeb369e80b50a24ceee86928786ae3b67a0687a7aea80e8cefac0145daR46
<roasbeef> and above that, anchors mentions static key? 
<ariard_> ad that's already assumed in #880 i think
<roasbeef> yeh so that's ok if we like that added definition at the end there
<roasbeef> tho on second glance, not sure what it's trying to do there...
<rusty> It's just not in the table.  Used to be transient when option_anchors_zero_fee_htlc_tx depended on option_anchor_outputs, but we rightly killed that.
<rusty> s/transient/transitive/  damn more coffeee...
<t-bast> I think it would be clearer if the table of features in Bolt 9 clearly mentioned `option_static_remotekey` in the `Dependencies` of `option_anchors_zero_fee_htlc_tx`
<roasbeef> gotcha yeh, looking at the rendered version it's blank rn in that spot 
<roasbeef> t-bast: yeh def
<t-bast> Ok so I think we can just clear these comments on Github this week, re-ACK and merge?
<roasbeef> t-bast: sgtm
<ariard_> yeah sgtm
<t-bast> #action roasbeef to fix last comments, others to ACK and merge
<t-bast> #topic new closing_signed is awesome
<t-bast> #link https://github.com/lightningnetwork/lightning-rfc/pull/847
<roasbeef> ok merge now, then I made a new branch adding the table thing?
<t-bast> roasbeef: ACK for me if others are ok
<roasbeef> since the chan_type PR also mentions it 
<ariard_> yeah do a follow-up PR if you think it's easier
<t-bast> We can indeed submit a follow-up small PR to address the comments to unblock the early merge
<roasbeef> cool
<t-bast> #action merge 824 and address latest comments in a follow-up PR
<BlueMatt> ok, 847 is another one of these 6+month old prs, we need to land this.
<BlueMatt> we went ahead and shipped an 847 implementation cause it was an excuse to solve several existing issues and it had acks and cross-impl tests from > 2 implementations
* lnd-bot (~lnd-bot@165.227.7.29) has joined
<lnd-bot> [lightning-rfc] Roasbeef merged pull request #824: feature: define option_zero_htlc_tx_fee (feature 22/23) (master...zero-fee-second-stage) https://github.com/lightningnetwork/lightning-rfc/pull/824
* lnd-bot (~lnd-bot@165.227.7.29) has left
<BlueMatt> it seems the only thing holding it up is roasbeef 
* lnd-bot (~lnd-bot@165.227.7.29) has joined
<lnd-bot> [lightning-rfc] Roasbeef pushed 1 commit to master: https://github.com/lightningnetwork/lightning-rfc/compare/d892c318af3b...fdc078f845e1
<lnd-bot> lightning-rfc/master fdc078f Johan T. Halseth: feature: define option_zero_htlc_tx_fee (feature 22/23) (#824)
* lnd-bot (~lnd-bot@165.227.7.29) has left
<BlueMatt> roasbeef: can you explain your concern with no feature bit here?
<BlueMatt> and, specifically, what you will implement differently with or without a feature bit in lnd
<t-bast> roasbeef: IIUC regarding 847, your current objection is whether we should add a feature bit right? There's nothing else?
<t-bast> I can understand in theory the argument to allow nodes to explicitly seek it (I don't want to open channels to nodes that use only the old negotiation)
<t-bast> But I'm not sure it's actually useful in practice
<rusty> Yeah, I've been calling 847 quick close internally in the implementation BTW.
<BlueMatt> well, I think someone would have to speak up and say that they plan on implementing that behavior
* lnd-bot (~lnd-bot@165.227.7.29) has joined
<lnd-bot> [lightning-rfc] Roasbeef pushed 1 commit to master: https://github.com/lightningnetwork/lightning-rfc/compare/d892c318af3b...fdc078f845e1
<lnd-bot> lightning-rfc/master fdc078f Johan T. Halseth: feature: define option_zero_htlc_tx_fee (feature 22/23) (#824)
* lnd-bot (~lnd-bot@165.227.7.29) has left
<t-bast> If everyone ships that soon (because it's quite simple to implement), since nodes must upgrade lightning often at the moment (we're constantly fixing important bugs), I don't think this scenario is worth handling
<roasbeef> t-bast: is there a downside tho?
<BlueMatt> yes, complexity and more code and cases to check
<BlueMatt> the shutdown fee negotiation stuff is already a big if tree
<roasbeef> I have an impl that's like 80%of the way there, but then i realized that if the behavior wasn't gated it broke a buncha tests since we exercised the prior behavior to make sure it terminates, etc 
<BlueMatt> you can chose to implement, or not, the relaxed cases, and presumably you wont for some time
<t-bast> yes, I'd say the downside is more code and conditional branches, whereas without a feature bit it's really simple, you just look at whether a TLV is here or not
<crypt-iq> Any implementation needs to have a branch here already, a feature bit just makes it explicit up-front and is IMO nicer to have
<roasbeef> I don't understand how it's meant to behave if one side sends it and the other doesn't understand it, if one side had the feature bit and the other didn't, then the sender would know just _not_ to send it and fall back to the normal behavior right?
<roasbeef> yeh since you need to handle that fall back any way right?
<t-bast> crypt-iq: the issue is that you then have two different `if`, it doesn't replace the existing one
<BlueMatt> roasbeef: you can always calculate an implicit feature bit locally :p
<niftynei> i mean the diff is in one case you know not to send it, in the other you send it and then wait and see what  they respond, no?
<t-bast> it's much simpler to always send it (it's an odd tlv), less code and be done with it
<BlueMatt> if (tlv is present) { node.featurebits |= internal_flag }
<roasbeef> niftynei: yeh that's my understanding
<t-bast> with a feature bit you need to handle the non-compliant cases (the feature bit is set but not the tlv, or the other way around)
<niftynei> "where is the if" 
<roasbeef> but I don't understand what happens w/ the "wait and see" 
<t-bast> so with feature bit there's more `if`s than without it...
<roasbeef> at that point, you don't know if htey could parse it or not (right?), so you need to be able to handle them not using your guidelines? so it insta falls back to the existing behavior?
<crypt-iq> t-bast: Oh I see what you mean, then you'd just bail
<crypt-iq> I think it's easier to reason about changes with a feature bit, and they're cheap
<BlueMatt> roasbeef: you can still enforce the guidelines
<niftynei> roasbeef: what happens is you have an if stmt later, not earlier??
<BlueMatt> (we do, in fact)
<BlueMatt> crypt-iq: after implementing it I think it'd be much more complicated to have to check the feature bits :)
<roasbeef> niftynei: not sure if I understand the context here, but i'm just confused w.r.t what type of expectations both sides can have 
<BlueMatt> its rather easy to just implement the old negotiation as a function of the new negotiation parameters
<t-bast> crypt-iq: yes, it's just boring, additional code that can be avoided and would likely be implemented differently by each impl, leading to some inconsistencies and users opening issues...if I were to add a feature bit to the implementation in eclair, I'm pretty sure I'd keep all the current code and add more code, which I consider a loss
<roasbeef> seems if I know they support it, I know I can pay lower closing fees, if I don't then I go in blind 
<BlueMatt> roasbeef: the expectation is "always send new tlv, if old node doesn't use old negotiation". if you add a feature bit, then you have a 2x2 matrix to handle
<t-bast> I agree with BlueMatt, checking the feature bit is purely additive to the code required to make this work without it
<BlueMatt> like, if you wanted to add a feature bit, I'd take the existing code we have, and add a additional if statement and return a force-close if you did something dumb
<t-bast> exactly
<BlueMatt> its purely added complexity
<crypt-iq> Are feature bits then only used in the case something requires explicit negotiation like option_zero_htlc_tx_fee?
<roasbeef> from my pov it isn't about the force close case, it's about how well this new state machine is defined 
<roasbeef> crypt-iq: imo they should be used to gate new behavior, and also let peers seek out others that understand that behavior 
<rusty> roasbeef: I assumed you can lowball the close with anchors already, since only lnd currently supports it really?
<BlueMatt> roasbeef: do you have a specific question about the state machine?
<BlueMatt> I found it quite well-defined
<ariard_> I agree it's nice to be able to find new closing_signed peers if you expect to have lower closing fees in average
<ariard_> though it's more a deployment question, once all nodes are upgraded that's a behavior you expect from everyone
<roasbeef> rusty: yeah, then users needs to just CPFP or w/e if they really care about it 
<t-bast> ariard_: but everyone will offer this very soon, so you don't need to find them, all the network will support it before you actually really need it
<roasbeef> ariard_: yeh I can't think of any downsides....
<BlueMatt> ariard_: someone would have to want to actually *implement* that
<roasbeef> if that's the case t-bast why do we ever use feature bits at all?
<BlueMatt> ariard_: I'm not. and no one seems to actually want to do that
<ariard_> t-bast: that's the point i trying to do, feature bit could be reserved when the feature usage isn't expect to happen in every channel lifecycle
<t-bast> roasbeef: exactly like ariard_ says, when it's a feature that you wouldn't expect to see everywhere
<rusty> I originally wanted a feature bit too, FWIW.  But "try and see" also technically *works*, so I don't really mind.
<BlueMatt> roasbeef: we use feature bits to negotiate things where we need backwards compat and need additional negotiation.
<roasbeef> BlueMatt: what does the sender do if the receiver appears (since no bit?) to not understand it, force close or continue along? what if the channel responder includes it but then the initiator sends something outside the range (they don't understand the new values) 
<BlueMatt> negotiation here is explicit
<BlueMatt> roasbeef: you continue the negotiation?
<rusty> (We will eventually have a "modern shit" feature, which will eventually become compulsory and imply all the older ones, freeing up those feature bits for eventual reuse... one day...)
<roasbeef> so you just have no guarantee at all if the new stuff will be used BlueMatt ?
<BlueMatt> roasbeef: if the non-initiator sends something out of range then you force-close, it says that.
<t-bast> rusty: YES to lightning v1.1
<BlueMatt> roasbeef: what do you mean? you use it, opportunistically, it doesn't make a difference materially except that its simpler to implement
* fch (~myname87@mob-5-90-227-40.net.vodafone.it) has joined
<crypt-iq> roasbeef: I think then you fall-back to old negotiation since they didn't send a fee_range TLV
<BlueMatt> just really means the negotiation happens quicker
<t-bast> crypt-iq: exactly
<niftynei> not to curtail the discussion too much, but it feels like a lot of your questions roasbeef would be worked out in the process of implementing it? but i thought LND already had it?
* niftynei is out of the loop on this clearly
<roasbeef> niftynei: yeh I got to the point of implementing it, then stopped once these questions popped up
<t-bast> crypt-iq: and at one point, in X years, when everyone does that or we've shipped the "Modern shit" feature bit, you completely remove the old negotiation code and enjoy seeing your codebase shrink for once xD
<BlueMatt> I dont get which questions arent answered in the doc
<roasbeef> impl is here: https://github.com/lightningnetwork/lnd/pull/5644
<BlueMatt> like, all of your above questions are explicitly answered in the spec pr
<niftynei> ok if there's specific ques can you put them on the PR?
<BlueMatt> honestly I dunno how much longer we can delay this
<niftynei> are roasbeef's questions a blocker for merging the PR seems like the bigger ques here
<rusty> Implementation wise, it's actually nice to have (if option_quickclose) .... else {old crufty code}, TBH.  At the moment they're intermingled, and it's a bit meh.
<BlueMatt> its been six months, we have acks from multiple implementations, including cross-compat testing.
<rusty> But IMHO it's transient, so I can live with it.
<BlueMatt> rusty: but then you have a 2x2 matrix of possible states
<BlueMatt> instead of just one branch
<niftynei> +1 to what rusty says fwiw, i did the initial impl in CL and it isn't amazing 
<BlueMatt> We largely merged the impls, it makes them quite simple
<BlueMatt> use the min/max in both impls
<niftynei> yeah, they're intermingled
<BlueMatt> makes the old-style negotiation much simpler :)
<niftynei> ok so by the 'rules of the spec' it sounds like we've got consensus (3impl and cross-compat) testing for this
<BlueMatt> so are we merging or is this gonna sit forever?
<roasbeef> I think from my PoV, this would be easier to understand if it was just like a standalone thing describing the way new things happen vs all the inline if statements in the spec, like it says "SHOULD set `max_fee_satoshis` to at least the `max_fee_satoshis` received" but in the section about the sending node, what about when the responder initaites it, `max_fee_satoshis` isn't defined right?
<roasbeef> idk maybe I'm just over thinking it 
<roasbeef> in my mind it was simple till I sat down to impl it 
<BlueMatt> huh, can you provide specific questions?
<ariard_> roasbeef: yeah we all agree that the way we write the spec rn isn't great but that's different from open questions on this specific PR?
<BlueMatt> cause I found it simple while implementing :)
<roasbeef> ariard_: yeh that's totally independent, but this is just where I have a hard time understanding what the specified behavior _should_ be 
<t-bast> I think all the inline statements in the spec are mostly because I tried to "factorize" it with existing requirements, I could have separated them cleanly but would have needed some duplication on the requirements (but I agree it may have been easier to read)
<roasbeef> BlueMatt: the quesiton there is how is `max_fee_satoshis` defined there? in the case where the responder kicks things off 
<roasbeef> here: https://github.com/lightningnetwork/lightning-rfc/pull/847/files#diff-ed04ca2c673fd6aabde69389511fa9ee60cb44d6b2ef6c88b549ffaa753d6afeR589
<BlueMatt> what do you mean "responder kicks things off"?
<BlueMatt> the non-channel-initiator never starts the fee negotiation, never has
<roasbeef> the responder sends a closing_signed message before the initiator does (or it just didn't receive it from the initiator) 
<niftynei> (t-bast: fwiw i typically end up in favor of duplication, esp where documentation is concerned as it decouples processes a lil bit more)
<roasbeef> any side can send a shutdown message 
<BlueMatt> this is not a shutdown message, its a closing_signed message
<BlueMatt> the channel initiator always sends first closing_signed
<BlueMatt> that didng change in this pr
<t-bast> niftynei: that's what I'll do in next PRs, I really hesitated for that one...
<t-bast> what BlueMatt says, remember that the initiator starts
<roasbeef> ahh yeh that's right
<t-bast> that's a bit hard to remember when only reading the requirements because they're separated as "sender/receiver"
<niftynei> i think someone (t-bast.?) noticed me duping stuff in the v2 opens PR :P
<t-bast> but since there's an order, it actually simplifies things
<t-bast> niftynei: xD
<ariard_> i think we can merge the PR, and if the wording is found confusing, from proposed suggestions we can always amend, add footnote, extend the rational...?
<t-bast> that's actually something I think we should be able to greatly simplify in the spec whenever we dump the old negotiation, instead of "sender" and "receiver" requirements we'll be able to have "initiator" and "non-initiator" requirements which will make the flow simpler to read
<BlueMatt> alright, we've been over time for a while.
<rusty> ariard_: I think what actually happens is that in 12 months we remove the old scheme, and it's now "A: send shutdown  B: reply.  A: if reply != send, reply again.".
<t-bast> rusty: I can't wait to see that happen
<rusty> Everyone please note https://github.com/lightningnetwork/lightning-rfc/pull/902 where we need to remove channel_update (we missed this, got a bug report)
<rusty> Quick time to discuss LN summit?
<roasbeef> I g2g, but will revisit my impl to update it, feel free to merge this as is, tho I still think you want a feature bit there
<t-bast> roasbeef: in light of the fact that the initiator starts, can you have another look at the state machine and let us know on the PR if you're ok with no feature bits for that one?
<t-bast> rusty: yay LN summit!
<roasbeef> t-bast: I still think one should be there, but I think this just exposed that we don't really have a set of consistent rules to use feature bits vs not, when new behavior or functinality is added 
<t-bast> roasbeef: yep, I think we never will be able to have a completely consistent set of rules for that though, it will be on a case-by-case basis and depend on rough agreement :)
<t-bast> roasbeef: my simple rule of thumb is that if adding a feature bit is actually more complicated and doesn't solve any compelling use-case, we shouldn't add one
* fch has quit (Quit: Leaving)
<t-bast> But let's not re-do the conversation again!
<roasbeef> t-bast: I think one issue there is "compliacted" as I've seen it defined is pretty langauge/impl dependent: "an extra if statement, oh noes" 
<roasbeef> but yeh I think let's just go ahead w/ it, and we'll catch up eventually 
<t-bast> Great let's go with that version then, thanks!
<t-bast> #action t-bast to merge #847
<BlueMatt> rusty: do you want to chat about https://github.com/lightningnetwork/lightning-rfc/pull/834#discussion_r698140878
<BlueMatt> while we're here
<BlueMatt> post-meeting
<BlueMatt> cause I wanna see warnings merged :)
<t-bast> Good idea, I want to see warnings merged as well :)
<t-bast> But let's do a bit of LN Summit first?
<t-bast> And warnings can be discussed after that in IRC or Github outside of the spec meeting?
<BlueMatt> seemed like there was agreement on the email thread to have a meetup, and not a formal spec summit that can decide on prs in dependently
<BlueMatt> which seems like a great idea
<BlueMatt> there may also be people in el salvador who would do the same, presumably
<t-bast> #topic LN Summit / Meetup / Something
<rusty> Yeah, I can't make it but I will be pining from my desert island.
<BlueMatt> is there anything to discuss?
<roasbeef> is this the zurich thing or something else?
<BlueMatt> roasbeef: well, I guess both zurich and el salvador at the conference if that happens
<roasbeef> oh el salvador
<roasbeef> don't have any plans for el salvador myself atm
<BlueMatt> i doubt I'll go to either
<roasbeef> I think i'll be at the other one tho
* fch (~myname87@mob-5-90-227-40.net.vodafone.it) has joined
<BlueMatt> soooo....discussion over?
<BlueMatt> sounds like there was nothing to discuss
<t-bast> BlueMatt & rusty: if you can't attend either, we definitely need a third one, it would be great to meet soon(ish) it really does speed up agreement on some priorities
<roasbeef> t-bast: seems like y'alla re going to both?
<ariard_> well zurich on the coredev side, is moving well, location for the 3 days has been booked
<t-bast> roasbeef: yeah I think we'll be going to both
<t-bast> roasbeef: you gotta come to at least one of these, it's been too long
<BlueMatt> t-bast: I mean you can have high bandwidth movement on lots of things without actually hitting merge in person :)
<BlueMatt> t-bast: but, anyway, we'll have a more formal one once rusty and folks can/want to travel a ton
<roasbeef> yeh I'll think I'll be at the europe one t-bast 
<t-bast> BlueMatt: sure, but there are a lot of things I'd like to discuss in a higher-bandwidth environment with most of you
<roasbeef> the el salvaor thing is also around thanksgiving for the americans
<t-bast> roasbeef: oh good point, I don't know if they took that into account
<roasbeef> maybe better not to lol 
<crypt-iq> BlueMatt: I agree with dust_limit that t-bast calculated and not 354 or which was mentioned before.  It may be that < 354 is not economical to spend, but Core and btcd will still accept it if >= 330 for P2WSH, >= 294 for P2WPKH
* fch has quit (Quit: Leaving)
<roasbeef> but yeh some LL ppl will be at the zurich one, looking forward to seeing those in meatspace again that'll be there 
<BlueMatt> crypt-iq: the point was segwit-v2-40-bytes, not existing script forms
* fch (~myname87@mob-5-90-227-40.net.vodafone.it) has joined
<BlueMatt> crypt-iq: cause with shutdown_anysegwit nodes will accept things that aren't the standard types
<t-bast> crypt-iq: yes, I suspect BlueMatt has calculated what's economical to spend, where you and I have just checked what bitcoind allows :)
* fch has quit (Client Quit)
<BlueMatt> t-bast: no, I did not. I checked what bitcoind allows
<BlueMatt> max size segwit output is bigger than p2wsh :p
<t-bast> BlueMatt: interesting, I'll need to dive back into that rabbit hole then xD
<crypt-iq> segwit v2, not taproot right?
<BlueMatt> well, any segwit that is bigger than p2wsh
<crypt-iq> oh got it
<BlueMatt> which could be v1, but not taproot
<crypt-iq> there's this pr for those who haven't seen: https://github.com/bitcoin/bitcoin/pull/22779
<BlueMatt> meeting over
<BlueMatt> rusty: you around?
<BlueMatt> still
<t-bast> #endmeeting

t-bast pinned this issue Aug 27, 2021

t-bast unpinned this issue Sep 23, 2021

t-bast closed this as completed Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lightning Specification Meeting 2021/08/30 #901

Lightning Specification Meeting 2021/08/30 #901

t-bast commented Aug 27, 2021 •

edited

t-bast commented Aug 27, 2021

t-bast commented Aug 31, 2021

Lightning Specification Meeting 2021/08/30 #901

Lightning Specification Meeting 2021/08/30 #901

Comments

t-bast commented Aug 27, 2021 • edited

Pull Request Review

Long Term Updates

Backlog

t-bast commented Aug 27, 2021

t-bast commented Aug 31, 2021

t-bast commented Aug 27, 2021 •

edited