Merge pull request #3896 from elnosh/channel-type-required

Make channel_type required

Author: TheBlueMatt

fuzz/src/full_stack.rs

@@ -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);
@@ -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(
@@ -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);
 	// 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,
 	);
 
@@ -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);
@@ -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);

lightning/src/ln/channel.rs

@@ -3827,18 +3827,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 {
@@ -11526,37 +11518,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>
@@ -11580,7 +11566,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 {
@@ -11977,13 +11963,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)?;
 
 		let counterparty_pubkeys = ChannelPublicKeys {
 			funding_pubkey: msg.common_fields.funding_pubkey,

lightning/src/ln/channel_type_tests.rs

@@ -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);
@@ -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,
@@ -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.
 	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,
@@ -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(
+		&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() {
+	// 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]

lightning/src/ln/channelmanager.rs

@@ -8860,7 +8860,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.
@@ -14106,7 +14106,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();

lightning/src/ln/msgs.rs

@@ -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>,
 }
 
@@ -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`].