Skip to content

Update channel-type implementation to upstream spec as merged #1314

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 18, 2022
Merged

Update channel-type implementation to upstream spec as merged #1314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92556c8
Drop spurious whitespace in channel.rs
TheBlueMatt Feb 16, 2022
6d7ae6e
Update channel-type implementation to upstream spec as merged
TheBlueMatt Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
} else if their_features.supports_channel_type() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Assume they've accepted the channel type as they said they understand it.
} else {
self.channel_type = ChannelTypeFeatures::from_counterparty_init(&their_features)
}

let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
match &msg.shutdown_scriptpubkey {
&OptionalField::Present(ref script) => {
Expand Down Expand Up @@ -2174,11 +2184,11 @@ impl<Signer: Sign> Channel<Signer> {

/// Returns transaction if there is pending funding transaction that is yet to broadcast
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "to be broadcast"

pub fn unbroadcasted_funding(&self) -> Option<Transaction> {
if self.channel_state & (ChannelState::FundingCreated as u32) != 0 {
self.funding_transaction.clone()
} else {
None
}
if self.channel_state & (ChannelState::FundingCreated as u32) != 0 {
self.funding_transaction.clone()
} else {
None
}
}

/// Returns a HTLCStats about inbound pending htlcs
Expand Down Expand Up @@ -4720,6 +4730,7 @@ impl<Signer: Sign> Channel<Signer> {
Some(script) => script.clone().into_inner(),
None => Builder::new().into_script(),
}),
channel_type: Some(self.channel_type.clone()),
}
}

Expand Down
13 changes: 11 additions & 2 deletions lightning/src/ln/msgs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,12 @@ pub struct AcceptChannel {
pub first_per_commitment_point: PublicKey,
/// Optionally, a request to pre-set the to-sender output's scriptPubkey for when we collaboratively close
pub shutdown_scriptpubkey: OptionalField<Script>,
/// The channel type that this channel will represent. If none is set, we derive the channel
/// type from the intersection of our feature bits with our counterparty's feature bits from
/// the Init message.
///
/// This is required to match the equivalent field in [`OpenChannel::channel_type`].
pub channel_type: Option<ChannelTypeFeatures>,
}

/// A funding_created message to be sent or received from a peer
Expand Down Expand Up @@ -1064,7 +1070,9 @@ impl_writeable_msg!(AcceptChannel, {
htlc_basepoint,
first_per_commitment_point,
shutdown_scriptpubkey
}, {});
}, {
(1, channel_type, option),
});

impl_writeable_msg!(AnnouncementSignatures, {
channel_id,
Expand Down Expand Up @@ -2191,7 +2199,8 @@ mod tests {
delayed_payment_basepoint: pubkey_4,
htlc_basepoint: pubkey_5,
first_per_commitment_point: pubkey_6,
shutdown_scriptpubkey: if shutdown { OptionalField::Present(Address::p2pkh(&::bitcoin::PublicKey{compressed: true, key: pubkey_1}, Network::Testnet).script_pubkey()) } else { OptionalField::Absent }
shutdown_scriptpubkey: if shutdown { OptionalField::Present(Address::p2pkh(&::bitcoin::PublicKey{compressed: true, key: pubkey_1}, Network::Testnet).script_pubkey()) } else { OptionalField::Absent },
channel_type: None,
};
let encoded_value = accept_channel.encode();
let mut target_value = hex::decode("020202020202020202020202020202020202020202020202020202020202020212345678901234562334032891223698321446687011447600083a840000034d000c89d4c0bcc0bc031b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f024d4b6cd1361032ca9bd2aeb9d900aa4d45d9ead80ac9423374c451a7254d076602531fe6068134503d2723133227c867ac8fa6c83c537e9a44c3c5bdbdcb1fe33703462779ad4aad39514614751a71085f2f10e1c7a593e4e030efb5b8721ce55b0b0362c0a046dacce86ddd0343c6d3c7c79c2208ba0d9c9cf24a6d046d21d21f90f703f006a18d5653c4edf5391ff23a61f03ff83d237e880ee61187fa9f379a028e0a").unwrap();
Expand Down