Echo channel_type in accept_channel
#960
Conversation
|
Spec meeting comment, waiting for someone from CL to confirm: cc @niftynei @rustyrussell -- can merge at will once confirmed. |
02-peer-protocol.md
Outdated
| @@ -348,6 +348,8 @@ The sender: | |||
| avoid double-spending of the funding transaction. | |||
| - MUST set `channel_reserve_satoshis` greater than or equal to `dust_limit_satoshis` from the `open_channel` message. | |||
| - MUST set `dust_limit_satoshis` less than or equal to `channel_reserve_satoshis` from the `open_channel` message. | |||
| - if `option_channel_type` was negotiated: | |||
| - MUST set `channel_type` | |||
| - if it sets `channel_type`: | |||
There was a problem hiding this comment.
is this not the same as the requirement the two lines below? is option_channel_type being negotiated different from 'setting channel_type'?
- if it sets `channel_type`:
is the same as
- if `option_channel_type` was negotiated
There was a problem hiding this comment.
anyway, clightnng enforces the draft, which is "if the opener sends a channel_type, we reply with a channel type.
There was a problem hiding this comment.
I think it's not quite the same: we never explicitly said here that having negotiated option_channel_type means we must set it. But if I understand you correctly, instead of adding two new lines I could just enrich the two lines below to more explicitly say that you MUST set channel type to what was in the open_channel, is that right?
There was a problem hiding this comment.
Let me know if 584b03e is better, I modified the existing requirement to clarify it instead of adding a new one on top
There was a problem hiding this comment.
yep that's great -- thanks!
|
concept ACK, but change NACK: is already expressed in the spec (but maybe needs rewording) |
One argument for adding a feature bit for channel types was to make things more explicit and remove ambiguity. When sending `open_channel`, we require funders to include a `channel_type`, so it would make sense to require fundees to echo that `channel_type` back to explicitly confirm that they're ok with this `channel_type`.
t-bast
force-pushed
the
accept-channel-type
branch
from
9bc97ff to
584b03e
Compare
|
ACK 584b03e |
| - if it sets `channel_type`: | ||
| - MUST set it to the `channel_type` from `open_channel` | ||
| - if `option_channel_type` was negotiated: | ||
| - MUST set `channel_type` to the `channel_type` from `open_channel` |
There was a problem hiding this comment.
Hmmm, this isn't quite right, anymore. Recall that the feature was added after the channel_type stuff was, thus, the "authoritative" data is the channel_type field, not the feature. It feels strange to remove the mention of setting the type here if you don't negotiate the feature.
There was a problem hiding this comment.
Recall that the feature was added after the channel_type stuff was, thus, the "authoritative" data is the channel_type field, not the feature.
That's true, but since now all implementations do use the feature bit, this is a historical artifact that can be removed from the spec, isn't it? Now that there is a feature bit, it does simplify requirements to always use it
There was a problem hiding this comment.
Not really? There are releases of node software that does not set the feature bit and expects the channel_type flags as sent to determine the channel type. I'd rather keep the channel_type so that we're clear on how things work, including with some nodes on the network.
There was a problem hiding this comment.
Ok, that sounds reasonable (and matches my first version of this PR). @niftynei does that work for you, since you requested changes in the initial version?
There was a problem hiding this comment.
it feels like the mention of the feature flag is unnecessary? is there ever a case where the feature flag will be set but there won't be a channel_type to mirror back in the accept?
maybe i'm unclear on what case we're solving for here, apologies.
There was a problem hiding this comment.
That's the whole point of this PR, previously we didn't require the channel_type be set even if the feature flag is, but, as you note its somewhat nonsensical for that to happen. This PR simply fixes that oversight and says "yea, if you negotiate the feature that says you support this, you MUST support this, yo"
There was a problem hiding this comment.
Then the change belongs on the open channel message, not the accept message.
The accept message behavior is unchanged and the existing spec should be sufficient.
There was a problem hiding this comment.
No? The change here is about the channel_type field in the accept message. The current spec only says that if you set channel_type in accept then it has to be a specific value, it never says anything about when you have to include the channel_type to begin with.
|
ACK 584b03e |
One argument for adding a feature bit for channel types was to make things more explicit and remove ambiguity (the other main argument was to be able to filter out peers that don't support this feature).
When sending
open_channel, we require funders to include achannel_type, so it would make sense to require fundees to echo thatchannel_typeback to explicitly confirm that they're ok with this particularchannel_type.This is the current behavior of eclair, please let me know if other implementations are doing it differently, and if you think this additional requirement shouldn't be added.