-
Notifications
You must be signed in to change notification settings - Fork 415
Make channel_type required #3896
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
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @carlaKC as a reviewer! |
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
@@ -1181,16 +1181,16 @@ fn two_peer_forwarding_seed() -> Vec<u8> { | |||
// inbound read from peer id 1 of len 18 | |||
ext_from_hex("030112", &mut test); | |||
// message header indicating message length 274 | |||
ext_from_hex("0112 01000000000000000000000000000000", &mut test); | |||
ext_from_hex("0116 01000000000000000000000000000000", &mut test); |
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.
Some of these comments need an update?
fuzz/src/full_stack.rs
Outdated
@@ -1067,7 +1067,7 @@ fn two_peer_forwarding_seed() -> Vec<u8> { | |||
// inbound read from peer id 0 of len 32 | |||
ext_from_hex("030020", &mut test); | |||
// init message (type 16) with static_remotekey required, no channel_type/anchors/taproot, and other bits optional and mac | |||
ext_from_hex("0010 00021aaa 0008aaa208aa2a0a9aaa 03000000000000000000000000000000", &mut test); | |||
ext_from_hex("0010 00021aaa 0008aaa218aa2a0a9aaa 03000000000000000000000000000000", &mut test); |
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 assume these changes are correct, because test_no_existing_test_breakage
still passes.
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 in large part to chatgpt:
The change is in the feature bits field: aaa208aa2a0a9aaa → aaa218aa2a0a9aaa
- Each two chars are the hex representation of a byte of feature info
- Each byte contains 8 bits used to mark our features
- This is just a vector, and the LSB is zero, next most significant is
one etc.
aa = bits 56-63
a2 = bits 48-55
18 = bits 40-47
aa = bits 32-39
2a = bits 24-31
0a = bits 16-23
9a = bits 8-15
aa = bits 0-7
Bits 40-47 changed from 08 to 18: 00001000 → 00011000, turning on required bit 44
I think that we can change the hex to 10
(00010000) so that we're only setting the required feature and not the optional one anymore? Admittedly have not read the full fuzz test though, so may be wrong here.
Update: testing suggested diff here - if this passes I think we should only set required, as that's what we're doing irl.
Update: fuzz tests pass with just the required bit, so IMO we should change them!
lightning/src/ln/channel.rs
Outdated
return Err(ChannelError::close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); | ||
} | ||
funding.channel_transaction_parameters.channel_type_features = channel_type; | ||
return Err(ChannelError::close("channel_type assumed to be supported".to_owned())); |
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.
Perhaps this is slightly better readable, accentuating the happy flow? Just a suggestion, in doubt whether it is better.
let ty = common_fields.channel_type.as_ref()
.ok_or_else(|| ChannelError::close("channel_type assumed to be supported".to_owned()))?;
if ty != funding.get_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left as is but don't feel strongly about it. I can change it if you prefer.
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.
Also have a slight preference for Joost's suggestion.
Interesting that we previously weren't actually enforcing that the channel type matches open_channel
when supports_channel_type
was true - more permissive than the spec allows.
lightning/src/ln/channel.rs
Outdated
@@ -3842,14 +3842,8 @@ where | |||
if ty != funding.get_channel_type() { |
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.
The comment on common_fields.channel_type
needs updating.
lightning/src/ln/channel.rs
Outdated
return Err(ChannelError::close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); | ||
} | ||
funding.channel_transaction_parameters.channel_type_features = channel_type; | ||
return Err(ChannelError::close("channel_type assumed to be supported".to_owned())); |
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.
Refer to it as option_channel_type
in the message, also below?
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 used channel_type
since that is the name of the field in the message?
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.
Agree with option_channel_type
- this error may go to a non-ldk node so we should adhere to spec terminology.
@@ -11999,7 +11988,7 @@ where | |||
return Err(ChannelError::close(format!("Rejecting V2 channel {} missing channel_type", | |||
msg.common_fields.temporary_channel_id))) | |||
} | |||
let channel_type = channel_type_from_open_channel(&msg.common_fields, their_features, our_supported_features)?; | |||
let channel_type = channel_type_from_open_channel(&msg.common_fields, our_supported_features)?; |
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.
Directly above there is already a check for non channel type. Is that redundant now?
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.
ah, yeah seems that can be removed, thanks
@@ -336,20 +329,18 @@ fn test_rejects_implicit_simple_anchors() { | |||
) | |||
.unwrap(); | |||
|
|||
// Set `channel_type` to `None` to force the implicit feature negotiation. | |||
// Set `channel_type` to `None` to cause failure. |
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.
Shouldn't this coverage be kept?
// Since A supports both `static_remote_key` and `option_anchors`, but B only accepts
// `static_remote_key`, it will fail the channel.
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 thought no since the code doing that implicit negotiation will be removed
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 agree this can be removed - we have test_rejects_simple_anchors_channel_type
that ensures we don't accept old anchors, now that we're assuming the feature there's no possibility that somebody can try to make us infer it (because we require channel_type
).
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
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.
few minor comments, looking good!
lightning/src/ln/channel.rs
Outdated
return Err(ChannelError::close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); | ||
} | ||
funding.channel_transaction_parameters.channel_type_features = channel_type; | ||
return Err(ChannelError::close("channel_type assumed to be supported".to_owned())); |
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.
Also have a slight preference for Joost's suggestion.
Interesting that we previously weren't actually enforcing that the channel type matches open_channel
when supports_channel_type
was true - more permissive than the spec allows.
lightning/src/ln/channel.rs
Outdated
return Err(ChannelError::close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); | ||
} | ||
funding.channel_transaction_parameters.channel_type_features = channel_type; | ||
return Err(ChannelError::close("channel_type assumed to be supported".to_owned())); |
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.
Agree with option_channel_type
- this error may go to a non-ldk node so we should adhere to spec terminology.
@@ -336,20 +329,18 @@ fn test_rejects_implicit_simple_anchors() { | |||
) | |||
.unwrap(); | |||
|
|||
// Set `channel_type` to `None` to force the implicit feature negotiation. | |||
// Set `channel_type` to `None` to cause failure. |
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 agree this can be removed - we have test_rejects_simple_anchors_channel_type
that ensures we don't accept old anchors, now that we're assuming the feature there's no possibility that somebody can try to make us infer it (because we require channel_type
).
fuzz/src/full_stack.rs
Outdated
@@ -1067,7 +1067,7 @@ fn two_peer_forwarding_seed() -> Vec<u8> { | |||
// inbound read from peer id 0 of len 32 | |||
ext_from_hex("030020", &mut test); | |||
// init message (type 16) with static_remotekey required, no channel_type/anchors/taproot, and other bits optional and mac | |||
ext_from_hex("0010 00021aaa 0008aaa208aa2a0a9aaa 03000000000000000000000000000000", &mut test); | |||
ext_from_hex("0010 00021aaa 0008aaa218aa2a0a9aaa 03000000000000000000000000000000", &mut test); |
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 in large part to chatgpt:
The change is in the feature bits field: aaa208aa2a0a9aaa → aaa218aa2a0a9aaa
- Each two chars are the hex representation of a byte of feature info
- Each byte contains 8 bits used to mark our features
- This is just a vector, and the LSB is zero, next most significant is
one etc.
aa = bits 56-63
a2 = bits 48-55
18 = bits 40-47
aa = bits 32-39
2a = bits 24-31
0a = bits 16-23
9a = bits 8-15
aa = bits 0-7
Bits 40-47 changed from 08 to 18: 00001000 → 00011000, turning on required bit 44
I think that we can change the hex to 10
(00010000) so that we're only setting the required feature and not the optional one anymore? Admittedly have not read the full fuzz test though, so may be wrong here.
Update: testing suggested diff here - if this passes I think we should only set required, as that's what we're doing irl.
Update: fuzz tests pass with just the required bit, so IMO we should change them!
fuzz/src/full_stack.rs
Outdated
@@ -1168,7 +1168,7 @@ fn two_peer_forwarding_seed() -> Vec<u8> { | |||
// inbound read from peer id 1 of len 32 | |||
ext_from_hex("030120", &mut test); | |||
// init message (type 16) with static_remotekey required, no channel_type/anchors/taproot, and other bits optional and mac | |||
ext_from_hex("0010 00021aaa 0008aaa208aa2a0a9aaa 01000000000000000000000000000000", &mut test); | |||
ext_from_hex("0010 00021aaa 0008aaa218aa2a0a9aaa 01000000000000000000000000000000", &mut test); |
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.
nitnit: comments here say that init has no channel_type
?
(pre-existingly wrong, but let's update here)
pushed changes addressing comments diff |
Diff lgtm, only remaining items for me:
Outdated: |
Set the option_channel_type feature as required and fail the channel if channel_type is not included during channel negotiation in open_channel or accept_channel.
8f9289d
to
b525e38
Compare
Closes #3872
Changes:
option_channel_type
feature as required.channel_type
is not included during channel negotiation.