Update channel-type implementation to upstream spec as merged #1314
Conversation
Somehow, our channel type implementation doesn't echo back the channel type as we believe it was negotiated, as we should. Though the spec doesn't explicitly require this, some implementations may require it and it appears to have been in the BOLTs from the start of the channel type logic.
| @@ -1895,6 +1895,16 @@ impl<Signer: Sign> Channel<Signer> { | |||
| return Err(ChannelError::Close("Minimum confirmation depth must be at least 1".to_owned())); | |||
| } | |||
|
|
|||
| if let Some(ty) = &msg.channel_type { | |||
| if *ty != self.channel_type { | |||
| return Err(ChannelError::Close("Channel Type in accept_channel didn't match the one sent in open_channel.".to_owned())); | |||
There was a problem hiding this comment.
I think the spec says in accept_channel subsection:
"The sender if it sets the channel_type MUST set it to the channel_type from open_channel".
I agree the "it" isn't clear though sounds the spec requirement to echo is already there.
There was a problem hiding this comment.
I'd think it is pretty clearly (a) the sender, and (b) the channel_type, so technically I don't think we're in violation today, though we maybe should be.
There was a problem hiding this comment.
My point was the spec already mentions it. Contrary to what the commit message sounds to suggest "Though the spec doesn't explicitly require this" :p ?
There was a problem hiding this comment.
No, the spec very clearly does not explicitly require this - it says "IF [the sender] sets the channel_type"...then it must set it to a copy. The spec does not require the accept_channel sender to send a copy.
There was a problem hiding this comment.
I'll add my unsolicited 2 cents, but my reading of this is exactly that: if accept_channel sets the channel type, clearly implying optionality, it must match the value from open_channel, but it definitely needn't be set.
| if *ty != self.channel_type { | ||
| return Err(ChannelError::Close("Channel Type in accept_channel didn't match the one sent in open_channel.".to_owned())); | ||
| } | ||
| } else if their_features.supports_channel_type() { |
There was a problem hiding this comment.
IIUC we might have a peer supporting channel type (feature bits 44/45) but not announcing any channel_type because they don't support any channel feature. What's the state of mainstream implementations, they all support static_key at least ?
Maybe we can implement the spec suggestion : "The receiving node MAY fail the channel if option_channel_type was negotiated but the message doesn't include a channel_type". That would be a first step deprecating channel type detection from Init message (and I think we can deprecate such logic once channel_type is well-deployed, as it's doesn't constrain to force-close alive channel such as when deprecating commitment format). Maybe too early.
There was a problem hiding this comment.
IIUC we might have a peer supporting channel type (feature bits 44/45) but not announcing any channel_type because they don't support any channel feature. What's the state of mainstream implementations, they all support static_key at least ?
No, the point of the feature bit is they have to announce a channel type field/TLV, though it can be empty.
The receiving node MAY fail the channel if option_channel_type was negotiated but the message doesn't include a channel_type".
Two things: a) this is about open_channel not accept_channel, and (b) that's how I had this PR initially, but that means new-LDK won't be able to accept a channel from current-LDK, which is not ideal.
Note you need to do an official "github approve" to have it count now :/. It seems the linting CI check is unrelated and because of a tokio MSRV bump (ugh why do people do this in minor versions...I hate the rust ecosystem):
|
| @@ -2174,11 +2184,11 @@ impl<Signer: Sign> Channel<Signer> { | |||
|
|
|||
| /// Returns transaction if there is pending funding transaction that is yet to broadcast | |||
Somehow, our channel type implementation doesn't echo back the
channel type as we believe it was negotiated, as we should. Though
the spec doesn't explicitly require this, some implementations may
and it appears to have been in the BOLTs from the start of the
channel type logic.
Somewhat of a bugfix so I'd like to land it ASAP.