-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BOLT 2: send channel_type in open/accept #880
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together, this will be really useful.
It looks good to me, I'll implement in eclair this week.
02-peer-protocol.md
Outdated
@@ -197,6 +206,13 @@ know this node will accept `funding_satoshis` greater than or equal to 2^24. | |||
Since it's broadcast in the `node_announcement` message other nodes can use it to identify peers | |||
willing to accept large channel even before exchanging the `init` message with them. | |||
|
|||
Channel features are explicitly enumerated as `channel_type` | |||
bitfields, using odd features bits. The currently defined types are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe that's just me, but even feature bits sound better here, because once you chose a channel type you must apply these features, they're not optional anymore. Thoughts?
Or to go even further, why do we even need to put feature bits here? We could define a new enum for channel_type
, for example:
- 0 -> no features, basic channel
- 1 -> static remote key
- 2 -> anchor outputs
- 3 -> anchor outputs with 0-fee htlcs
It would be a simple bigsize
or u8
(instead of a byte array), and implementations internally map that to a set of feature bits.
This way we're sure we won't have weird inconsistencies if one implementation decides to put harmless unrelated feature bits inside channel_type
, which is undefined behavior with the current proposal (what do I do if I receive a byte array contains bits 7, 13, and 21? Or if I receive bits 13, 21 and 37? The first one is harmless, the second one should probably be rejected).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we already have one mechanism to coordinate, let's not create another! For example, I already have variants which add option_simplified_update to all of these....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we already have one mechanism to coordinate
If we don't want to have to assign a new set of bits, we can just use the current bit position number for the actual feature bits themselves. Since it's just an integer, we don't waste any space by not assigning non-contiguous values. To simplify things a bit the optional bit can be used, so static key is 13, anchor zero htlc is 23, etc, etc.
The other issue with using the raw feature bit vector here is the size of the message. This likely isn't the last TLV we'll add here, and given it's an embedded feature vector it can swell to several KBs which means the message may run up against the current max message size.
Using a set integer based off the feature bit position addresses all these issues and doesn't attempt to send a redundant set of bytes to communicate things we already know. If we've already sent our feature bits in the node ann, and in innit upon connection, why do we need to send them again here? I already know what you support!
what do I do if I receive a byte array contains bits 7, 13, and 21? Or if I receive bits 13, 21 and 37? The first one is harmless, the second one should probably be rejected
This example shows how sending the feature bits again doesn't actually make the negotiation explicit and just adds more feature bit compatibility issues at a second site (first in the node ann, and now here again). Having the acceptor "pick" amongst an advertised set just adds more moving parts, and also makes weird downgrade cases (that we've grappled w/ in the past) exist at another site (what does "most preferred" even mean?).
If I send type 13
(static key), then it's 100% unambiguous what type of channel I want to open, no extra mapping (set of feature bits to a type) or implicit negotiation needed. It's dead simple.
For example, I already have variants which add option_simplified_update to all of these....
This is an argument for just using a single type imo. Lets say CL implement that new commitment update type, when newer more distinct commitment types (witness asymmetric commitments, eltoo, all the taproot variants, hybrid DLC etc) arise, that protocol will likely need to be revamped to support the new HTLC/commitment variants. Rather than needing to interpret that "modification" bit, a new commitment type should be added instead to enumerate all the possible supported update+commitment variants. This makes things explicit and gets rid of the weird (and error prone) implicit negotiation we have atm. This enumeration doesn't add any in-protocol overhead as a bigsize
integer can be used.
We've already implemented it and just using that type directly is a lot simpler than needing to map the set of feature bits (and determine incompatibilities or ambiguities, etc) to a type that ultimately needs to exist anyway for any implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought about this more, and the issue with a channel_type
enum as done by lnd (and by myself at first) is the explosion of enum values when you combine orthogonal features (the channel_type
isn't only about the commitment format, it's really the channel's feature set).
Right now we have:
- "basic" channels
- static remotekey channels
- anchor output channels
- anchor output with 0-fee htlc channels
If we add two features that are orthogonal to the commitment format, for example option_simplified_update
and option_zero_reserve
(these are just examples, imagine two channel features that are completely orthogonal to the commitment format), the enum becomes:
- "basic" channels
- static remotekey channels
- anchor output channels
- anchor output with 0-fee htlc channels
- "basic" channels with simplified update
- static remotekey channels with simplified update
- anchor output channels with simplified update
- anchor output with 0-fee htlc channels with simplified update
- "basic" channels with zero reserve
- static remotekey channels with zero reserve
- anchor output channels with zero reserve
- anchor output with 0-fee htlc channels with zero reserve
- "basic" channels with zero reserve and simplified update
- static remotekey channels with zero reserve and simplified update
- anchor output channels with zero reserve and simplified update
- anchor output with 0-fee htlc channels with zero reserve and simplified update
That simply doesn't scale...and is correctly handled by rusty's proposal IMHO. In the channel DB we'd just store the feature bytes array corresponding to channel features (features that affect the channel parameters/format). As discussed during yesterday's spec meeting, each channel_type
is uniquely identified by the value of its features
field so it's not ambiguous.
I don't think the size of the message is a real issue. Even if we have 150 feature bits (which will take decades), it means each channel_type
is ~20 bytes. If your implementation supports 50 distinct channel_type
s (which is probably unreasonable from a security point of view) that's only 1kB. It can be an issue if you want to use features that aren't in the spec though, and that need to use a high feature bit. In that case it can be a bit higher, but if you use a feature bit in the 150-300 range it's still not an issue imo.
Each implementation would probably internally define the list of channel_type
s they want to support, ordered by preference (e.g. I'd like to only support static_remotekey and anchor outputs, with or without option_simplified_update, and I'd prefer using anchor outputs and option_simplified_update) and somehow let the node operator configure what she wants to allow/disallow, but how to present that to the node operator is orthogonal to how we internally negotiate it in the channel open protocol and store it in our channel database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the explosion of enum values when you combine orthogonal features
Yeah this is the tradeoff in terms of the "enum space": everything needs to be unrolled. However there isn't any added overhead on the wire given that you can pack just about every combination you can think of into a bigsize
integer.
That simply doesn't scale..
I guess it depends where you think the scale issues arise. It's simply taking up space already allocated (bit wise) in the integer specified. In the end, if you send all the individual feature bits, you'll end up needing to wrangle with the same "parameter space" IMO, as you need to map those down to an actual channel type (concrete implementation).
One alternative would be to split things into a base channel type, and a modifier. In your example, the modifiers would be the zero reserve and the simplified update channel type.
I think in the end though, all implementations simply won't support every combination of the feature bit expansion (we shouldn't underestimate the complexity here). As an example, lnd doesn't support making a "basic" channel any longer, though we need to continue to signal the optional bit, as otherwise with the way feature bits are set up right now, things break down on the connection level. As more and more channel types are added (taproot variants, eltoo, DLC, etc, etc) IMO it'll become more and more difficult for an implementation to support all of them. Having an "enum" for the channel type simply makes that expansion explicit.
I don't think the size of the message is a real issue.
If your implementation supports 50 distinct channel_types (which is probably unreasonable from a security point of view) that's only 1kB
IMO the issue here is that as the TLV expands in this message, it may become impossible to honestly advertise the set of parameters you want, as you risk running into the max message limit. Why send kilobytes when a 4 bytes will do? It's unnecessarily inefficient and made worse by the fact that we don't use a sparse encoding.
The other issue I see here as mentioned above is that we're just sending the exact same feature bit information that's already present and has been exchanged potentially twice at this point (in the init and node_ann message). On top of this, by letting the responder ultimately select the channel type, the initiator is unable to actually pin down the channel type they want to send, other than sending a single feature bit (or set of bits), which reduces down to simply sending the channel type.
On top of that, if I want to open an explicit channel type (I only want the "most secure" option), why would I ever send an array of options and allow the receiver to choose amongst them? It's unclear how to advise implementations to create a safe generalized "channel type satisficement" engine (accepting the "best" set of bits). I don't think we should easily dismiss the complexity added here w.r.t preference and selection.
In short I see future potential for a series of inadvertent footguns introduced by adding a loose "negotiation layer". An implementation needs to wrangle with parsing all the different combinations, and choosing which one is "best" for it (to advertise and accept) vs just sending what they want (based on the feature bits already advertised at the othe layers) and the other side rejecting or accepting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One alternative would be to split things into a base channel type, and a modifier. In your example, the modifiers would be the zero reserve and the simplified update channel type.
I agree, but these modifiers are exactly the same thing as feature bits, aren't they? So why introduce a different mechanism?
IMO the issue here is that as the TLV expands in this message, it may become impossible to honestly advertise the set of parameters you want, as you risk running into the max message limit. Why send kilobytes when a 4 bytes will do? It's unnecessarily inefficient and made worse by the fact that we don't use a sparse encoding.
In my example, 1kB is really the most extreme case, I'm pretty sure this will never use more than ~100 bytes in the next couple of years. IMHO the number of bytes used in the messages isn't a strong enough argument to weigh in favor of one proposal or the other.
I think in the end though, all implementations simply won't support every combination of the feature bit expansion
I completely agree, but I can really see a future where there a few distinct orthogonal feature bits that an implementation may support, and where this mechanism is useful (an implementation could very well support two distinct update protocols, two distinct commitment formats and a few minor "modifiers").
On top of that, if I want to open an explicit channel type (I only want the "most secure" option), why would I ever send an array of options
You don't have to. If you want only one channel_type
, you should send just that one in your message. But if other implementations want to let the fundee choose among a range of options, we should give them the freedom to do that, shouldn't we? Especially once we have some channel features that are orthogonal to the commitment format, the choice of which one to use will be a choice of what trade-offs you'd rather make, so I expect nodes to have different preferences here, hence I think giving them a choice would make sense.
Concept ACK. |
A new proposal. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK, I've prototyped this and I'm now convinced it's more future-proof to use a features field instead of defining a new enum.
I'm wondering whether we should use even or odd feature bits, it feels to me that even feature bits make more sense here (these features must be applied to the channel) but I don't feel strongly about that.
Note that there is a small implementation subtlety with regards to commit feerate. If when using anchor outputs you use a lower commit feerate than you would use for standard channels, and you want to offer channel_types=[standard, anchor_outputs]
, you must use the commit feerate of a standard channel in your open_channel
message. If your peer chooses to use anchor outputs, then you will be able to send an update_fee
to lower the feerate. But you shouldn't offer the anchor output feerate before knowing what channel type your peer will select, otherwise you may end up with a standard channel with a feerate that's too low to confirm on its own, and if your peer plays dumb and ignores your future update_fee
you may waste time getting your funds back.
Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880).
I see this as just one of the many future edge cases that need to be wrangled with by introducing this loose negotiation layer. If the type I want to use is just sent out right, I can just set the params in the Thinking about this out a bit further, I just don't see how it's possible to craft a generalized |
AFAICT, in order for this to actually work (this loose negotiation layer vs explicit channel types), things need to happen in two phases (kinda like the client+server hello in TLS before the actual encrypted session creation): first both sides agree on the channel type to be used, then the initial If channel funding is actually explicit (funder sends over type they want to used, recveiver takes it or leaves it), this additional negotiation layer isn't needed. Unlike TLS, before starting the funding workflow, we already know the channel types supported by both sides (feature bits in innit and node_ann message). |
I think this is a very good idea. Negotiating the |
So this is what I've been (seemingly unsuccessfully) trying to get at: by having the initiator send multiple options with the receiver picking one of them, an actual negotiation layer is required. If instead the initiator sends just what they want to use (either a single enum, two enums (one for channel type and one for modifiers), or an eum and a set of bits (enum is chan type, bits are modifiers)), then this negotiation layer isn't needed at all. The initiator knows what the receiver deems as being possible by the feature bits they've already advertised (in |
Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880).
Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880).
One other point to consider, you might decide to accept your channel type in function of the level of trust you're pouring in the counterparty, which ultimaltely is a node policy decision. For e.g, I expect we'll have far more experimentation on the fee-bumping side, like high-feerate pre-signed commitment transaction if you have a low-scoring of this peer reliability and brace yourself to go onchain with high odds. Or even w.r.t fancier taproot types, one of the scripts structure might force you to commit plainly your watchtower policy, like one tapleaf per watchtower. That's okay if you trust the counterparty but don't do this if you expect adversarial settings. So we might not even have something like a static list of preferred channels, and we should keep the feature flags for what they're, a signal of what the mechanisms are supported by your implementation, not the actual channel features authorized by your node policy on an authenticated channel opening. You might not even communicate your channel policy as a receiver ("security by obscurity"), or at least only to already whitelisted peers. For those reasons, I agree with roasbeef it's better to have the sender locking the type during a newer initial channel type announcement phase. The fact that this type is a result of previous interactivity between funder/fundee should be deferred to a negotiation layer, of which the participation is optional. We could even have a new gossip message for announcing channel policy, for e.g "i'm open to do channel (bits XYZ) with anyone", where XYZ denominate the regular, safest channel type supported by every implementation. |
Signed-off-by: Rusty Russell <[email protected]>
Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880).
Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880).
Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880).
* Separate internal channel config from features Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880). * Add shutdown script to channel remote params This will be necessary when we implement `upfront_shutdown_script`. This field can be set to `None` for all current channels as we don't support the feature yet.
Add support for lightning/bolts#880 This lets node operators open a channel with different features than what the implicit choice based on activated features would use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 745b4d0
Add support for lightning/bolts#880 This lets node operators open a channel with different features than what the implicit choice based on activated features would use.
@@ -252,6 +272,7 @@ are not valid secp256k1 pubkeys in compressed format. | |||
- the funder's amount for the initial commitment transaction is not sufficient for full [fee payment](03-transactions.md#fee-payment). | |||
- both `to_local` and `to_remote` amounts for the initial commitment transaction are less than or equal to `channel_reserve_satoshis` (see [BOLT 3](03-transactions.md#commitment-transaction-outputs)). | |||
- `funding_satoshis` is greater than or equal to 2^24 and the receiver does not support `option_support_large_channel`. | |||
- It supports `channel_type`, `channel_type` was set, and the `type` is not suitable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" - channel type
is set to a type containing a feature which was not negotiated" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to do additional checks: having "MUST" here (above) implies you should be testing features.
It's weird, but you should only be policing your own options, not theirs. So if they offer you something you understand, you can take it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related comment: lightningnetwork/lnd#5373 (comment)
If we added a feature bit for something like upfront shutdown (that modified the chan negotiation messages to add new behavior), why wouldn't we add one here for something that revamps the channel type selection negotiation entirely?
This simply allows downgrade from "all the features we negotiated". If we want to make this required for some new feature we add, that's cool (I think it should be for stuff like option_simplified_commitment, which you might want to support, but not prefer), but we can add that when we add the feature.
I think it's gonna be moot since everyone wants this and in 12 months it'll be compulsory anyway.... (hope)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎊
@t-bast cool sgtm. Was rebasing our PR and realized that we had selected an interim feature bit (2020/2021) in our PR, but we don't have one here. This gets back to the meta conversation we were having on IRC earlier today during the spec meeting: when should new feature bits be added? I think this falls under the "new behavior that can be helpful in selecting potential channel peers for a node". If a peer I'm connected to doesn't signal this feature bit, then I have no way of knowing if they'll be able to handle the new explicit chan type negotiation. |
Related comment: lightningnetwork/lnd#5373 (comment) If we added a feature bit for something like upfront shutdown (that modified the chan negotiation messages to add new behavior), why wouldn't we add one here for something that revamps the channel type selection negotiation entirely? |
This is extracted from channel_upgrade (lightning#868), but used for opening negotiation as suggested by @Roasbeef on the last spec meeting. It's a trivial change, fully backwards compatible, but now each channel has a channel_type, which defines its behavior, rather than an ad-hoc set of "sticky" feature bits. It also means both peers can *support* a feature without endorsing it. Signed-off-by: Rusty Russell <[email protected]>
8186f59
to
27de157
Compare
Squashed into a single commit, trivial rebase on master. Applying now as per meeting 2021-08-30. |
Add support for lightning/bolts#880 This lets node operators open a channel with different features than what the implicit choice based on activated features would use.
I don't understand this new direction of "we don't actually need feature bits since everyone is going to implement it". This completely misses the purpose of feature bits which is to signal understanding of new behavior and also let nodes discover other nodes that also understand that behavior. I think we also thought that "everyone" would have implemented full anchors by now, but that's not the reality, and thankfully due to the feature bit, peers that understand it are able to seek out other peers that don't.
Sure and we can carve out that path today by adding the optional feature bit. Once again, there's literally zero cost to doing this, instead things are just more explicit and well defined. Without a feature bit it's essentially guess and check: you send out a new field and then attempt to see if they understood it or not. With a feature bit, you know up front if they support some new behavior or not. |
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
This feature was introduced by lightning/bolts#880 We also clean up the channel state machine to leverage the feature bits added in #237
This feature was introduced by lightning/bolts#880 We also clean up the channel state machine to leverage the feature bits added in #237
This feature was introduced by lightning/bolts#880 We also clean up the channel state machine to leverage the feature bits added in #237
This feature was introduced by lightning/bolts#880 We also clean up the channel state machine to leverage the feature bits added in #237
This feature was introduced by lightning/bolts#880 and the feature bit was added in lightning/bolts#906 We also clean up the channel state machine to leverage the feature bits added in #237
This feature was introduced by lightning/bolts#880 and the feature bit was added in lightning/bolts#906 We also clean up the channel state machine to leverage the feature bits added in #237
* Rename ChannelTypes -> ChannelData And move commands to a new ChannelCommands file. This commit doesn't contain any logic changes. * Require explicit channel type negotiation This feature was introduced by lightning/bolts#880 and the feature bit was added in lightning/bolts#906 We also clean up the channel state machine to leverage the feature bits added in #237
A new proposal. Signed-off-by: Rusty Russell <[email protected]>
This is extracted from channel_upgrade (#868), but used for opening
negotiation as suggested by @Roasbeef on the last spec meeting.
It's a trivial change, fully backwards compatible, but now each channel
has a channel_type, which defines its behavior, rather than an ad-hoc
set of "sticky" feature bits. It also means both peers can support a
feature without endorsing it.
Note:
channel_type
reuses feature bits so it's trivial to know how to extend in future (e.g. option_simplified_update, option_taproot, etc). It's not an arbitrary set of feature bits, there really are only 4 values currently defined!(I'm tempted to make another commit which changes the rest of the text to refer explicitly to channel_type now).