Skip to content

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+150 −84
Open

Make channel_type required #3896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 13 additions & 13 deletions fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1066,8 +1066,8 @@ fn two_peer_forwarding_seed() -> Vec<u8> {
ext_from_hex("0010 03000000000000000000000000000000", &mut test);
// 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);
// init message (type 16) with static_remotekey required, no anchors/taproot, and other bits optional and mac
ext_from_hex("0010 00021aaa 0008aaa210aa2a0a9aaa 03000000000000000000000000000000", &mut test);

// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
Expand Down Expand Up @@ -1167,8 +1167,8 @@ fn two_peer_forwarding_seed() -> Vec<u8> {
ext_from_hex("0010 01000000000000000000000000000000", &mut test);
// 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);
// init message (type 16) with static_remotekey required, no anchors/taproot, and other bits optional and mac
ext_from_hex("0010 00021aaa 0008aaa210aa2a0a9aaa 01000000000000000000000000000000", &mut test);

// create outbound channel to peer 1 for 50k sat
ext_from_hex(
Expand All @@ -1180,17 +1180,17 @@ 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);
// message header indicating message length 278
ext_from_hex("0116 01000000000000000000000000000000", &mut test);
Copy link
Contributor

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?

// inbound read from peer id 1 of len 255
ext_from_hex("0301ff", &mut test);
// beginning of accept_channel
ext_from_hex("0021 0000000000000000000000000000000000000000000000000000000000000e12 0000000000000162 00000000004c4b40 00000000000003e8 00000000000003e8 00000002 03f0 0005 030000000000000000000000000000000000000000000000000000000000000100 030000000000000000000000000000000000000000000000000000000000000200 030000000000000000000000000000000000000000000000000000000000000300 030000000000000000000000000000000000000000000000000000000000000400 030000000000000000000000000000000000000000000000000000000000000500 02660000000000000000000000000000", &mut test);
// inbound read from peer id 1 of len 35
ext_from_hex("030123", &mut test);
// inbound read from peer id 1 of len 39
ext_from_hex("030127", &mut test);
// rest of accept_channel and mac
ext_from_hex(
"0000000000000000000000000000000000 0000 01000000000000000000000000000000",
"0000000000000000000000000000000000 0000 01021000 01000000000000000000000000000000",
&mut test,
);

Expand Down Expand Up @@ -1582,8 +1582,8 @@ fn gossip_exchange_seed() -> Vec<u8> {
ext_from_hex("0010 03000000000000000000000000000000", &mut test);
// 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 0008aaa20aaa2a0a9aaa 03000000000000000000000000000000", &mut test);
// init message (type 16) with static_remotekey required, no anchors/taproot, and other bits optional and mac
ext_from_hex("0010 00021aaa 0008aaa210aa2a0a9aaa 03000000000000000000000000000000", &mut test);

// new inbound connection with id 1
ext_from_hex("01", &mut test);
Expand All @@ -1602,8 +1602,8 @@ fn gossip_exchange_seed() -> Vec<u8> {
ext_from_hex("0010 01000000000000000000000000000000", &mut test);
// 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 0008aaa20aaa2a0a9aaa 01000000000000000000000000000000", &mut test);
// init message (type 16) with static_remotekey required, no anchors/taproot, and other bits optional and mac
ext_from_hex("0010 00021aaa 0008aaa210aa2a0a9aaa 01000000000000000000000000000000", &mut test);

// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
Expand Down
76 changes: 28 additions & 48 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3845,18 +3845,10 @@ where
return Err(ChannelError::close("Got an accept_channel message at a strange time".to_owned()));
}

if let Some(ty) = &common_fields.channel_type {
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()));
}
} else if their_features.supports_channel_type() {
// Assume they've accepted the channel type as they said they understand it.
} else {
let channel_type = ChannelTypeFeatures::from_init(&their_features);
if channel_type != ChannelTypeFeatures::only_static_remote_key() {
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;
let channel_type = common_fields.channel_type.as_ref()
.ok_or_else(|| ChannelError::close("option_channel_type assumed to be supported".to_owned()))?;
if channel_type != funding.get_channel_type() {
return Err(ChannelError::close("Channel Type in accept_channel didn't match the one sent in open_channel.".to_owned()));
}

if common_fields.dust_limit_satoshis > 21000000 * 100000000 {
Expand Down Expand Up @@ -11541,37 +11533,31 @@ where
/// [`msgs::CommonOpenChannelFields`].
#[rustfmt::skip]
pub(super) fn channel_type_from_open_channel(
common_fields: &msgs::CommonOpenChannelFields, their_features: &InitFeatures,
our_supported_features: &ChannelTypeFeatures
common_fields: &msgs::CommonOpenChannelFields, our_supported_features: &ChannelTypeFeatures
) -> Result<ChannelTypeFeatures, ChannelError> {
if let Some(channel_type) = &common_fields.channel_type {
if channel_type.supports_any_optional_bits() {
return Err(ChannelError::close("Channel Type field contained optional bits - this is not allowed".to_owned()));
}
let channel_type = common_fields.channel_type.as_ref()
.ok_or_else(|| ChannelError::close("option_channel_type assumed to be supported".to_owned()))?;

// We only support the channel types defined by the `ChannelManager` in
// `provided_channel_type_features`. The channel type must always support
// `static_remote_key`, either implicitly with `option_zero_fee_commitments`
// or explicitly.
if !channel_type.requires_static_remote_key() && !channel_type.requires_anchor_zero_fee_commitments() {
return Err(ChannelError::close("Channel Type was not understood - we require static remote key".to_owned()));
}
// Make sure we support all of the features behind the channel type.
if channel_type.requires_unknown_bits_from(&our_supported_features) {
return Err(ChannelError::close("Channel Type contains unsupported features".to_owned()));
}
let announce_for_forwarding = if (common_fields.channel_flags & 1) == 1 { true } else { false };
if channel_type.requires_scid_privacy() && announce_for_forwarding {
return Err(ChannelError::close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned()));
}
Ok(channel_type.clone())
} else {
let channel_type = ChannelTypeFeatures::from_init(&their_features);
if channel_type != ChannelTypeFeatures::only_static_remote_key() {
return Err(ChannelError::close("Only static_remote_key is supported for non-negotiated channel types".to_owned()));
}
Ok(channel_type)
if channel_type.supports_any_optional_bits() {
return Err(ChannelError::close("Channel Type field contained optional bits - this is not allowed".to_owned()));
}

// We only support the channel types defined by the `ChannelManager` in
// `provided_channel_type_features`. The channel type must always support
// `static_remote_key`, either implicitly with `option_zero_fee_commitments`
// or explicitly.
if !channel_type.requires_static_remote_key() && !channel_type.requires_anchor_zero_fee_commitments() {
return Err(ChannelError::close("Channel Type was not understood - we require static remote key".to_owned()));
}
// Make sure we support all of the features behind the channel type.
if channel_type.requires_unknown_bits_from(&our_supported_features) {
return Err(ChannelError::close("Channel Type contains unsupported features".to_owned()));
}
let announce_for_forwarding = if (common_fields.channel_flags & 1) == 1 { true } else { false };
if channel_type.requires_scid_privacy() && announce_for_forwarding {
return Err(ChannelError::close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned()));
}
Ok(channel_type.clone())
}

impl<SP: Deref> InboundV1Channel<SP>
Expand All @@ -11595,7 +11581,7 @@ where

// First check the channel type is known, failing before we do anything else if we don't
// support this channel type.
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)?;

let holder_selected_channel_reserve_satoshis = get_holder_selected_channel_reserve_satoshis(msg.common_fields.funding_satoshis, config);
let counterparty_pubkeys = ChannelPublicKeys {
Expand Down Expand Up @@ -11992,13 +11978,7 @@ where
let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
channel_value_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS);

// First check the channel type is known, failing before we do anything else if we don't
// support this channel type.
if msg.common_fields.channel_type.is_none() {
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)?;
Copy link
Contributor

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?

Copy link
Contributor Author

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


let counterparty_pubkeys = ChannelPublicKeys {
funding_pubkey: msg.common_fields.funding_pubkey,
Expand Down
117 changes: 103 additions & 14 deletions lightning/src/ln/channel_type_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,9 @@ fn do_test_supports_channel_type(config: UserConfig, expected_channel_type: Chan
}

#[test]
fn test_rejects_implicit_simple_anchors() {
// Tests that if `option_anchors` is being negotiated implicitly through the intersection of
// each side's `InitFeatures`, it is rejected.
fn test_rejects_if_channel_type_not_set() {
// Tests that if `channel_type` is not set in `open_channel` and `accept_channel`, it is
// rejected.
let secp_ctx = Secp256k1::new();
let test_est = TestFeeEstimator::new(15000);
let fee_estimator = LowerBoundedFeeEstimator::new(&test_est);
Expand All @@ -312,13 +312,6 @@ fn test_rejects_implicit_simple_anchors() {

let config = UserConfig::default();

// See feature bit assignments: https://github.com/lightning/bolts/blob/master/09-features.md
let static_remote_key_required: u64 = 1 << 12;
let simple_anchors_required: u64 = 1 << 20;
let raw_init_features = static_remote_key_required | simple_anchors_required;
let init_features_with_simple_anchors =
InitFeatures::from_le_bytes(raw_init_features.to_le_bytes().to_vec());

let mut channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
&fee_estimator,
&&keys_provider,
Expand All @@ -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.
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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).

let mut open_channel_msg =
channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
open_channel_msg.common_fields.channel_type = None;

// Since A supports both `static_remote_key` and `option_anchors`, but B only accepts
// `static_remote_key`, it will fail the channel.
let channel_b = InboundV1Channel::<&TestKeysInterface>::new(
&fee_estimator,
&&keys_provider,
&&keys_provider,
node_id_a,
&channelmanager::provided_channel_type_features(&config),
&init_features_with_simple_anchors,
&channelmanager::provided_init_features(&config),
&open_channel_msg,
7,
&config,
Expand All @@ -358,6 +349,104 @@ fn test_rejects_implicit_simple_anchors() {
/*is_0conf=*/ false,
);
assert!(channel_b.is_err());

open_channel_msg.common_fields.channel_type =
Some(channel_a.funding.get_channel_type().clone());
let mut channel_b = InboundV1Channel::<&TestKeysInterface>::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking/for a follow up: these tests could benefit from being DRY-ed up a bit

&fee_estimator,
&&keys_provider,
&&keys_provider,
node_id_a,
&channelmanager::provided_channel_type_features(&config),
&channelmanager::provided_init_features(&config),
&open_channel_msg,
7,
&config,
0,
&&logger,
/*is_0conf=*/ false,
)
.unwrap();

// Set `channel_type` to `None` in `accept_channel` to cause failure.
let mut accept_channel_msg = channel_b.get_accept_channel_message(&&logger).unwrap();
accept_channel_msg.common_fields.channel_type = None;

let res = channel_a.accept_channel(
&accept_channel_msg,
&config.channel_handshake_limits,
&channelmanager::provided_init_features(&config),
);
assert!(res.is_err());
}

#[test]
fn test_rejects_if_channel_type_differ() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, thx for adding 👍

// Tests that if the `channel_type` in `accept_channel` does not match the one set in
// `open_channel` it rejects the channel.
let secp_ctx = Secp256k1::new();
let test_est = TestFeeEstimator::new(15000);
let fee_estimator = LowerBoundedFeeEstimator::new(&test_est);
let network = Network::Testnet;
let keys_provider = TestKeysInterface::new(&[42; 32], network);
let logger = TestLogger::new();

let node_id_a =
PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[1; 32]).unwrap());
let node_id_b =
PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[2; 32]).unwrap());

let config = UserConfig::default();

let mut channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
&fee_estimator,
&&keys_provider,
&&keys_provider,
node_id_b,
&channelmanager::provided_init_features(&config),
10000000,
100000,
42,
&config,
0,
42,
None,
&logger,
)
.unwrap();

let open_channel_msg =
channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();

let mut channel_b = InboundV1Channel::<&TestKeysInterface>::new(
&fee_estimator,
&&keys_provider,
&&keys_provider,
node_id_a,
&channelmanager::provided_channel_type_features(&config),
&channelmanager::provided_init_features(&config),
&open_channel_msg,
7,
&config,
0,
&&logger,
/*is_0conf=*/ false,
)
.unwrap();

// Change the `channel_type` in `accept_channel` msg to make it different from the one set in
// `open_channel` to cause failure.
let mut accept_channel_msg = channel_b.get_accept_channel_message(&&logger).unwrap();
let mut channel_type = channelmanager::provided_channel_type_features(&config);
channel_type.set_zero_conf_required();
accept_channel_msg.common_fields.channel_type = Some(channel_type.clone());

let res = channel_a.accept_channel(
&accept_channel_msg,
&config.channel_handshake_limits,
&channelmanager::provided_init_features(&config),
);
assert!(res.is_err());
}

#[test]
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8593,7 +8593,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
// We can get the channel type at this point already as we'll need it immediately in both the
// manual and the automatic acceptance cases.
let channel_type = channel::channel_type_from_open_channel(
common_fields, &peer_state.latest_features, &self.channel_type_features()
common_fields, &self.channel_type_features()
).map_err(|e| MsgHandleErrInternal::from_chan_no_close(e, common_fields.temporary_channel_id))?;

// If we're doing manual acceptance checks on the channel, then defer creation until we're sure we want to accept.
Expand Down Expand Up @@ -13695,7 +13695,7 @@ pub fn provided_init_features(config: &UserConfig) -> InitFeatures {
features.set_basic_mpp_optional();
features.set_wumbo_optional();
features.set_shutdown_any_segwit_optional();
features.set_channel_type_optional();
features.set_channel_type_required();
features.set_scid_privacy_optional();
features.set_zero_conf_optional();
features.set_route_blinding_optional();
Expand Down
11 changes: 4 additions & 7 deletions lightning/src/ln/msgs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,8 @@ pub struct CommonOpenChannelFields {
/// Optionally, a request to pre-set the to-channel-initiator output's scriptPubkey for when we
/// collaboratively close
pub shutdown_scriptpubkey: Option<ScriptBuf>,
/// The channel type that this channel will represent
///
/// If this is `None`, we derive the channel type from the intersection of our
/// feature bits with our counterparty's feature bits from the [`Init`] message.
/// The channel type that this channel will represent. As defined in the latest
/// specification, this field is required. However, it is an `Option` for legacy reasons.
pub channel_type: Option<ChannelTypeFeatures>,
}

Expand Down Expand Up @@ -356,9 +354,8 @@ pub struct CommonAcceptChannelFields {
/// Optionally, a request to pre-set the to-channel-acceptor output's scriptPubkey for when we
/// collaboratively close
pub shutdown_scriptpubkey: Option<ScriptBuf>,
/// 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.
/// The channel type that this channel will represent. As defined in the latest
/// specification, this field is required. However, it is an `Option` for legacy reasons.
///
/// This is required to match the equivalent field in [`OpenChannel`] or [`OpenChannelV2`]'s
/// [`CommonOpenChannelFields::channel_type`].
Expand Down
Loading