ln/refactor: use get_initial_channel_type for channel downgrade

Rather than duplicating our channel type preference ordering in
downgrade logic, make a modified version of the remote peer's supported
features and remove our current channel type from it to get the next
preferred channel type.

Author: carlaKC

lightning/src/ln/channel.rs

@@ -1557,6 +1557,7 @@ impl<SP: Deref> Channel<SP> where
 
 	pub fn maybe_handle_error_without_close<F: Deref, L: Deref>(
 		&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
+		user_config: &UserConfig, their_features: &InitFeatures,
 	) -> Result<Option<OpenChannelMessage>, ()>
 	where
 		F::Target: FeeEstimator,
@@ -1567,13 +1568,17 @@ impl<SP: Deref> Channel<SP> where
 			ChannelPhase::Funded(_) => Ok(None),
 			ChannelPhase::UnfundedOutboundV1(chan) => {
 				let logger = WithChannelContext::from(logger, &chan.context, None);
-				chan.maybe_handle_error_without_close(chain_hash, fee_estimator, &&logger)
+				chan.maybe_handle_error_without_close(
+					chain_hash, fee_estimator, &&logger, user_config, their_features,
+				)
 					.map(|msg| Some(OpenChannelMessage::V1(msg)))
 			},
 			ChannelPhase::UnfundedInboundV1(_) => Ok(None),
 			ChannelPhase::UnfundedV2(chan) => {
 				if chan.funding.is_outbound() {
-					chan.maybe_handle_error_without_close(chain_hash, fee_estimator)
+					chan.maybe_handle_error_without_close(
+						chain_hash, fee_estimator, user_config, their_features,
+					)
 						.map(|msg| Some(OpenChannelMessage::V2(msg)))
 				} else {
 					Ok(None)
@@ -4868,7 +4873,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 	/// of the channel type we tried, not of our ability to open any channel at all. We can see if a
 	/// downgrade of channel features would be possible so that we can still open the channel.
 	pub(crate) fn maybe_downgrade_channel_features<F: Deref>(
-		&mut self, funding: &mut FundingScope, fee_estimator: &LowerBoundedFeeEstimator<F>
+		&mut self, funding: &mut FundingScope, fee_estimator: &LowerBoundedFeeEstimator<F>,
+		user_config: &UserConfig, their_features: &InitFeatures,
 	) -> Result<(), ()>
 	where
 		F::Target: FeeEstimator
@@ -4885,25 +4891,35 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			// We've exhausted our options
 			return Err(());
 		}
+
+		// We should never have negotiated `anchors_nonzero_fee_htlc_tx` because it can result in a
+		// loss of funds.
+		let channel_type = &funding.channel_transaction_parameters.channel_type_features;
+		assert!(!channel_type.supports_anchors_nonzero_fee_htlc_tx());
+
 		// We support opening a few different types of channels. Try removing our additional
 		// features one by one until we've either arrived at our default or the counterparty has
-		// accepted one.
-		//
-		// Due to the order below, we may not negotiate `option_anchors_zero_fee_htlc_tx` if the
-		// counterparty doesn't support `option_scid_privacy`. Since `get_initial_channel_type`
-		// checks whether the counterparty supports every feature, this would only happen if the
-		// counterparty is advertising the feature, but rejecting channels proposing the feature for
-		// whatever reason.
-		let channel_type = &mut funding.channel_transaction_parameters.channel_type_features;
+		// accepted one. Features are un-set for the current channel type or any that come before
+		// it in our order of preference, allowing us to negotiate the "next best" based on the
+		// counterparty's remaining features per our ranking in `get_initial_channel_type`.
+		let mut eligible_features = their_features.clone();
 		if channel_type.supports_anchors_zero_fee_htlc_tx() {
-			channel_type.clear_anchors_zero_fee_htlc_tx();
-			self.feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::NonAnchorChannelFee);
-			assert!(!channel_type.supports_anchors_nonzero_fee_htlc_tx());
+			eligible_features.clear_anchors_zero_fee_htlc_tx();
 		} else if channel_type.supports_scid_privacy() {
-			channel_type.clear_scid_privacy();
-		} else {
-			*channel_type = ChannelTypeFeatures::only_static_remote_key();
+			eligible_features.clear_scid_privacy();
+			eligible_features.clear_anchors_zero_fee_htlc_tx();
 		}
+
+		let next_channel_type = get_initial_channel_type(user_config, &eligible_features);
+
+		let conf_target = if next_channel_type.supports_anchors_zero_fee_htlc_tx() {
+			ConfirmationTarget::AnchorChannelFee
+		} else {
+			ConfirmationTarget::NonAnchorChannelFee
+		};
+		self.feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(conf_target);
+	 	funding.channel_transaction_parameters.channel_type_features = next_channel_type;
+
 		Ok(())
 	}
 
@@ -9893,13 +9909,16 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 	/// not of our ability to open any channel at all. Thus, on error, we should first call this
 	/// and see if we get a new `OpenChannel` message, otherwise the channel is failed.
 	pub(crate) fn maybe_handle_error_without_close<F: Deref, L: Deref>(
-		&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+		&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
+		user_config: &UserConfig, their_features: &InitFeatures,
 	) -> Result<msgs::OpenChannel, ()>
 	where
 		F::Target: FeeEstimator,
 		L::Target: Logger,
 	{
-		self.context.maybe_downgrade_channel_features(&mut self.funding, fee_estimator)?;
+		self.context.maybe_downgrade_channel_features(
+			&mut self.funding, fee_estimator, user_config, their_features,
+		)?;
 		self.get_open_channel(chain_hash, logger).ok_or(())
 	}
 
@@ -10405,12 +10424,15 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 	/// not of our ability to open any channel at all. Thus, on error, we should first call this
 	/// and see if we get a new `OpenChannelV2` message, otherwise the channel is failed.
 	pub(crate) fn maybe_handle_error_without_close<F: Deref>(
-		&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>
+		&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>,
+		user_config: &UserConfig, their_features: &InitFeatures,
 	) -> Result<msgs::OpenChannelV2, ()>
 	where
 		F::Target: FeeEstimator
 	{
-		self.context.maybe_downgrade_channel_features(&mut self.funding, fee_estimator)?;
+		self.context.maybe_downgrade_channel_features(
+			&mut self.funding, fee_estimator, user_config, their_features,
+		)?;
 		Ok(self.get_open_channel_v2(chain_hash))
 	}
 

lightning/src/ln/channelmanager.rs

@@ -12472,6 +12472,7 @@ where
 				match peer_state.channel_by_id.get_mut(&msg.channel_id) {
 					Some(chan) => match chan.maybe_handle_error_without_close(
 						self.chain_hash, &self.fee_estimator, &self.logger,
+						&self.default_configuration, &peer_state.latest_features,
 					) {
 						Ok(Some(OpenChannelMessage::V1(msg))) => {
 							peer_state.pending_msg_events.push(MessageSendEvent::SendOpenChannel {
@@ -16368,6 +16369,31 @@ mod tests {
 		do_test_channel_type_downgrade(initiator_cfg, receiver_cfg, start_type, vec![end_type]);
 	}
 
+	#[test]
+	fn test_scid_privacy_downgrade() {
+		// Tests downgrade from `anchors_zero_fee_htlc_tx` with `option_scid_alias` when the
+		// remote node advertises the features but does not accept the channel, asserting that
+		// `option_scid_alias` is the last feature to be downgraded.
+		let mut initiator_cfg = test_default_channel_config();
+		initiator_cfg.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+		initiator_cfg.channel_handshake_config.negotiate_scid_privacy = true;
+		initiator_cfg.channel_handshake_config.announce_for_forwarding = false;
+
+		let mut receiver_cfg = test_default_channel_config();
+		receiver_cfg.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+		receiver_cfg.channel_handshake_config.negotiate_scid_privacy = true;
+		receiver_cfg.manually_accept_inbound_channels = true;
+
+		let mut start_type = ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies();
+		start_type.set_scid_privacy_required();
+		let mut with_scid_privacy = ChannelTypeFeatures::only_static_remote_key();
+		with_scid_privacy.set_scid_privacy_required();
+		let static_remote = ChannelTypeFeatures::only_static_remote_key();
+		let downgrade_types = vec![with_scid_privacy, static_remote];
+
+		do_test_channel_type_downgrade(initiator_cfg, receiver_cfg, start_type, downgrade_types);
+	}
+
 	#[rustfmt::skip]
 	fn do_test_channel_type_downgrade(initiator_cfg: UserConfig, acceptor_cfg: UserConfig,
 		start_type: ChannelTypeFeatures, downgrade_types: Vec<ChannelTypeFeatures>) {
@@ -16404,6 +16430,45 @@ mod tests {
 		}
 	}
 
+	#[test]
+	#[rustfmt::skip]
+	fn test_no_channel_downgrade() {
+		// Tests that the local node will not retry when a `option_static_remote` channel is
+		// rejected by a peer that advertises support for the feature.
+		let initiator_cfg = test_default_channel_config();
+		let mut receiver_cfg = test_default_channel_config();
+		receiver_cfg.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+		receiver_cfg.manually_accept_inbound_channels = true;
+
+		let chanmon_cfgs = create_chanmon_cfgs(2);
+		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(initiator_cfg), Some(receiver_cfg)]);
+		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+		let error_message = "Channel force-closed";
+
+		nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 0, None, None).unwrap();
+		let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+		let start_type = ChannelTypeFeatures::only_static_remote_key();
+		assert_eq!(open_channel_msg.common_fields.channel_type.as_ref().unwrap(), &start_type);
+
+		nodes[1].node.handle_open_channel(nodes[0].node.get_our_node_id(), &open_channel_msg);
+		let events = nodes[1].node.get_and_clear_pending_events();
+		match events[0] {
+			Event::OpenChannelRequest { temporary_channel_id, .. } => {
+				nodes[1].node.force_close_broadcasting_latest_txn(&temporary_channel_id, &nodes[0].node.get_our_node_id(), error_message.to_string()).unwrap();
+			}
+			_ => panic!("Unexpected event"),
+		}
+
+		let error_msg = get_err_msg(&nodes[1], &nodes[0].node.get_our_node_id());
+		nodes[0].node.handle_error(nodes[1].node.get_our_node_id(), &error_msg);
+
+		// Since nodes[0] could not retry the channel with a different type, it should close it.
+		let chan_closed_events = nodes[0].node.get_and_clear_pending_events();
+		assert_eq!(chan_closed_events.len(), 1);
+		if let Event::ChannelClosed { .. } = chan_closed_events[0] { } else { panic!(); }
+	}
+
 	#[test]
 	#[rustfmt::skip]
 	fn test_update_channel_config() {