Validate next_splice_out_maximum_sat on both commitments

Wilmer's fuzzing runs caught a case where an advertised splice out
maximum hit the debug assertions in `get_next_splice_out_maximum`.
These debug assertions ensure that any adverstised splice out maximum
passes the validation of splice contributions.

The core issue is that we only read the local commitment when
calculating the splice out maximum, but our splice validation requires
that any splice out maximum is covered by the minimum of the holder's
balances on the local and the remote commitments.

Therefore, if a HTLC is dust on the local commitment, but non-dust on
the remote commitment, and the holder is the funder of the channel, we
advertise a splice out maximum that is not covered by the holder's
balance on the remote commitment, and fails our validation of splice
contributions.

We now read both commitments when calculating the next splice out
maximum, which fixes this issue.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -2554,6 +2554,9 @@ pub(super) struct FundingScope {
 	value_to_self_msat: u64, // Excluding all pending_htlcs, fees, and anchor outputs
 
 	/// minimum channel reserve for self to maintain - set by them.
+	#[cfg(any(test, feature = "_externalize_tests"))]
+	pub(super) counterparty_selected_channel_reserve_satoshis: Option<u64>,
+	#[cfg(not(any(test, feature = "_externalize_tests")))]
 	counterparty_selected_channel_reserve_satoshis: Option<u64>,
 
 	#[cfg(any(test, feature = "_externalize_tests"))]

lightning/src/ln/splicing_tests.rs

@@ -9304,3 +9304,175 @@ fn do_test_splice_out_initiator_reserve_breach_zero_fee_commitments(
 		acceptor.logger.assert_log("lightning::ln::channelmanager", cannot_splice_out, 1);
 	}
 }
+
+#[test]
+fn test_splice_out_maximum_on_both_commitments_dust_on_fundee_commitment() {
+	use crate::ln::htlc_reserve_unit_tests::setup_0reserve_no_outputs_channels;
+
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let mut config = test_default_channel_config();
+	config.channel_handshake_config.announced_channel_max_inbound_htlc_value_in_flight_percentage =
+		100;
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config.clone()), Some(config)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	const CHANNEL_VALUE_SAT: u64 = 100_000;
+	const FEERATE: u32 = 253;
+	const TOTAL_ANCHORS_SAT: u64 = 2 * 330;
+	const NODE_0_DUST_LIMIT_SAT: u64 = 354;
+	const NODE_1_DUST_LIMIT_SAT: u64 = 10_000;
+
+	let (channel_id, _transaction) =
+		setup_0reserve_no_outputs_channels(&nodes, CHANNEL_VALUE_SAT, NODE_0_DUST_LIMIT_SAT);
+
+	{
+		let per_peer_state_lock;
+		let mut peer_state_lock;
+		let chan =
+			get_channel_ref!(nodes[0], nodes[1], per_peer_state_lock, peer_state_lock, channel_id);
+		chan.context_mut().counterparty_dust_limit_satoshis = NODE_1_DUST_LIMIT_SAT;
+		assert_eq!(chan.context().holder_dust_limit_satoshis, NODE_0_DUST_LIMIT_SAT);
+		assert_eq!(chan.funding().holder_selected_channel_reserve_satoshis, 0);
+		assert_eq!(chan.funding().counterparty_selected_channel_reserve_satoshis, Some(0));
+	}
+
+	{
+		let per_peer_state_lock;
+		let mut peer_state_lock;
+		let chan =
+			get_channel_ref!(nodes[1], nodes[0], per_peer_state_lock, peer_state_lock, channel_id);
+		chan.context_mut().holder_dust_limit_satoshis = NODE_1_DUST_LIMIT_SAT;
+		assert_eq!(chan.context().counterparty_dust_limit_satoshis, NODE_0_DUST_LIMIT_SAT);
+		assert_eq!(chan.funding().holder_selected_channel_reserve_satoshis, 0);
+		assert_eq!(chan.funding().counterparty_selected_channel_reserve_satoshis, Some(0));
+	}
+
+	let details = &nodes[0].node.list_channels()[0];
+	let channel_type = details.channel_type.clone().unwrap();
+	assert_eq!(channel_type, ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies());
+
+	// This HTLC is only present on node 0's commitment
+	const SNEAKY_HTLC_SAT: u64 = 5_000;
+
+	let (_, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], SNEAKY_HTLC_SAT * 1000);
+
+	let node_0_details = &nodes[0].node.list_channels()[0];
+	let reserved_fee_sat = chan_utils::commit_tx_fee_sat(FEERATE, 0, &channel_type);
+	let expected_next_splice_out_maximum_sat = CHANNEL_VALUE_SAT
+		- SNEAKY_HTLC_SAT
+		- TOTAL_ANCHORS_SAT
+		- reserved_fee_sat
+		- NODE_1_DUST_LIMIT_SAT;
+	assert_eq!(node_0_details.next_splice_out_maximum_sat, expected_next_splice_out_maximum_sat);
+	let node_1_details = &nodes[1].node.list_channels()[0];
+	assert_eq!(node_1_details.next_splice_out_maximum_sat, 0);
+
+	fail_payment(&nodes[0], &[&nodes[1]], payment_hash);
+
+	let details = &nodes[0].node.list_channels()[0];
+	let reserved_fee_sat = chan_utils::commit_tx_fee_sat(FEERATE, 2, &channel_type);
+	let expected_available_capacity_sat = CHANNEL_VALUE_SAT - TOTAL_ANCHORS_SAT - reserved_fee_sat;
+	assert_eq!(details.next_outbound_htlc_limit_msat, expected_available_capacity_sat * 1000);
+	let node_0_payment_sat = expected_available_capacity_sat;
+	send_payment(&nodes[0], &[&nodes[1]], node_0_payment_sat * 1000);
+
+	// Make sure the local output is now gone from node 1's commitment
+	assert!(TOTAL_ANCHORS_SAT + reserved_fee_sat < NODE_1_DUST_LIMIT_SAT);
+
+	let details = &nodes[1].node.list_channels()[0];
+	let expected_next_splice_out_maximum_sat = node_0_payment_sat - NODE_1_DUST_LIMIT_SAT;
+	assert_eq!(details.next_splice_out_maximum_sat, expected_next_splice_out_maximum_sat);
+
+	let details = &nodes[0].node.list_channels()[0];
+	let reserved_fee_sat = chan_utils::commit_tx_fee_sat(FEERATE, 1, &channel_type);
+	let expected_next_splice_out_maximum_sat =
+		CHANNEL_VALUE_SAT - node_0_payment_sat - TOTAL_ANCHORS_SAT - reserved_fee_sat;
+	assert_eq!(details.next_splice_out_maximum_sat, expected_next_splice_out_maximum_sat);
+}
+
+#[test]
+fn test_splice_out_maximum_on_both_commitments_dust_on_funder_commitment() {
+	use crate::ln::htlc_reserve_unit_tests::setup_0reserve_no_outputs_channels;
+
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let mut config = test_default_channel_config();
+	config.channel_handshake_config.announced_channel_max_inbound_htlc_value_in_flight_percentage =
+		100;
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config.clone()), Some(config)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	const CHANNEL_VALUE_SAT: u64 = 100_000;
+	const FEERATE: u32 = 253;
+	const TOTAL_ANCHORS_SAT: u64 = 2 * 330;
+	const NODE_0_DUST_LIMIT_SAT: u64 = 10_000;
+	const NODE_1_DUST_LIMIT_SAT: u64 = 354;
+
+	let (channel_id, _transaction) =
+		setup_0reserve_no_outputs_channels(&nodes, CHANNEL_VALUE_SAT, NODE_1_DUST_LIMIT_SAT);
+
+	{
+		let per_peer_state_lock;
+		let mut peer_state_lock;
+		let chan =
+			get_channel_ref!(nodes[0], nodes[1], per_peer_state_lock, peer_state_lock, channel_id);
+		chan.context_mut().holder_dust_limit_satoshis = NODE_0_DUST_LIMIT_SAT;
+		assert_eq!(chan.context().counterparty_dust_limit_satoshis, NODE_1_DUST_LIMIT_SAT);
+		assert_eq!(chan.funding().holder_selected_channel_reserve_satoshis, 0);
+		assert_eq!(chan.funding().counterparty_selected_channel_reserve_satoshis, Some(0));
+	}
+
+	{
+		let per_peer_state_lock;
+		let mut peer_state_lock;
+		let chan =
+			get_channel_ref!(nodes[1], nodes[0], per_peer_state_lock, peer_state_lock, channel_id);
+		chan.context_mut().counterparty_dust_limit_satoshis = NODE_0_DUST_LIMIT_SAT;
+		assert_eq!(chan.context().holder_dust_limit_satoshis, NODE_1_DUST_LIMIT_SAT);
+		assert_eq!(chan.funding().holder_selected_channel_reserve_satoshis, 0);
+		assert_eq!(chan.funding().counterparty_selected_channel_reserve_satoshis, Some(0));
+	}
+
+	let details = &nodes[0].node.list_channels()[0];
+	let channel_type = details.channel_type.clone().unwrap();
+	assert_eq!(channel_type, ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies());
+
+	// This HTLC is only present on node 1's commitment
+	const SNEAKY_HTLC_SAT: u64 = 5_000;
+
+	let (_, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], SNEAKY_HTLC_SAT * 1000);
+
+	let node_0_details = &nodes[0].node.list_channels()[0];
+	let reserved_fee_sat = chan_utils::commit_tx_fee_sat(FEERATE, 0, &channel_type);
+	let expected_next_splice_out_maximum_sat = CHANNEL_VALUE_SAT
+		- SNEAKY_HTLC_SAT
+		- TOTAL_ANCHORS_SAT
+		- reserved_fee_sat
+		- NODE_0_DUST_LIMIT_SAT;
+	assert_eq!(node_0_details.next_splice_out_maximum_sat, expected_next_splice_out_maximum_sat);
+	let node_1_details = &nodes[1].node.list_channels()[0];
+	assert_eq!(node_1_details.next_splice_out_maximum_sat, 0);
+
+	fail_payment(&nodes[0], &[&nodes[1]], payment_hash);
+
+	let details = &nodes[0].node.list_channels()[0];
+	let reserved_fee_sat = chan_utils::commit_tx_fee_sat(FEERATE, 2, &channel_type);
+	let expected_available_capacity_sat = CHANNEL_VALUE_SAT - TOTAL_ANCHORS_SAT - reserved_fee_sat;
+	assert_eq!(details.next_outbound_htlc_limit_msat, expected_available_capacity_sat * 1000);
+	let node_0_payment_sat = expected_available_capacity_sat;
+	send_payment(&nodes[0], &[&nodes[1]], node_0_payment_sat * 1000);
+
+	// Make sure the local output is now gone from node 0's commitment
+	assert!(TOTAL_ANCHORS_SAT + reserved_fee_sat < NODE_0_DUST_LIMIT_SAT);
+
+	let details = &nodes[1].node.list_channels()[0];
+	let expected_next_splice_out_maximum_sat = node_0_payment_sat - NODE_0_DUST_LIMIT_SAT;
+	assert_eq!(details.next_splice_out_maximum_sat, expected_next_splice_out_maximum_sat);
+
+	let details = &nodes[0].node.list_channels()[0];
+	let reserved_fee_sat = chan_utils::commit_tx_fee_sat(FEERATE, 1, &channel_type);
+	let expected_next_splice_out_maximum_sat =
+		CHANNEL_VALUE_SAT - node_0_payment_sat - TOTAL_ANCHORS_SAT - reserved_fee_sat;
+	assert_eq!(details.next_splice_out_maximum_sat, expected_next_splice_out_maximum_sat);
+}

lightning/src/sign/tx_builder.rs

@@ -358,10 +358,18 @@ fn get_next_commitment_stats(
 // 3) s < (100h + 100 - 100d - c) / 99
 fn get_next_splice_out_maximum_sat(
 	is_outbound_from_holder: bool, channel_value_satoshis: u64, local_balance_before_fee_msat: u64,
-	remote_balance_before_fee_msat: u64, feerate_per_kw: u32, nondust_htlc_count: usize,
-	post_splice_delta_above_reserve_sat: u64, channel_constraints: &ChannelConstraints,
-	channel_type: &ChannelTypeFeatures,
+	remote_balance_before_fee_msat: u64, local_nondust_htlc_count: usize,
+	remote_nondust_htlc_count: usize, feerate_per_kw: u32, spiked_feerate: u32,
+	channel_constraints: &ChannelConstraints, channel_type: &ChannelTypeFeatures,
 ) -> u64 {
+	let post_splice_delta_above_reserve_sat = if is_outbound_from_holder {
+		let nondust_htlc_count = cmp::max(local_nondust_htlc_count, remote_nondust_htlc_count);
+		let commit_tx_fee_sat =
+			commit_tx_fee_sat(spiked_feerate, nondust_htlc_count + 1, channel_type);
+		commit_tx_fee_sat
+	} else {
+		0
+	};
 	let local_balance_before_fee_sat = local_balance_before_fee_msat / 1000;
 	let mut next_splice_out_maximum_sat = if channel_constraints
 		.counterparty_selected_channel_reserve_satoshis
@@ -424,37 +432,47 @@ fn get_next_splice_out_maximum_sat(
 		}
 		max_splice_out_sat
 	} else {
-		// In a zero-reserve channel, the holder is free to withdraw up to its `post_splice_delta_above_reserve_sat`
+		// In a zero-reserve channel, the holder is free to withdraw up to its `post_splice_delta_above_reserve_sat`.
 		local_balance_before_fee_sat.saturating_sub(post_splice_delta_above_reserve_sat)
 	};
 
-	// We only bother to check the local commitment here, the counterparty will check its own commitment.
-	//
 	// If the current `next_splice_out_maximum_sat` would produce a local commitment with no
 	// outputs, bump this maximum such that, after the splice, the holder's balance covers at
 	// least `dust_limit_satoshis` and, if they are the funder, `current_tx_fee_sat`.
 	// We don't include an additional non-dust inbound HTLC in the `current_tx_fee_sat`,
 	// because we don't mind if the holder dips below their dust limit to cover the fee for that
 	// inbound non-dust HTLC.
-	if !has_output(
-		is_outbound_from_holder,
-		local_balance_before_fee_msat.saturating_sub(next_splice_out_maximum_sat * 1000),
-		remote_balance_before_fee_msat,
-		feerate_per_kw,
-		nondust_htlc_count,
+	//
+	// We use the regular feerate instead of the spiked feerate here as zero-reserve is not
+	// allowed on legacy channels.
+	let current_tx_fee_sat = commit_tx_fee_sat(feerate_per_kw, 0, channel_type);
+	let mut trim_splice_out_max_if_no_outputs = |nondust_htlc_count, dust_limit_satoshis| {
+		if !has_output(
+			is_outbound_from_holder,
+			local_balance_before_fee_msat.saturating_sub(next_splice_out_maximum_sat * 1000),
+			remote_balance_before_fee_msat,
+			feerate_per_kw,
+			nondust_htlc_count,
+			dust_limit_satoshis,
+			channel_type,
+		) {
+			let min_balance_sat = if is_outbound_from_holder {
+				dust_limit_satoshis.saturating_add(current_tx_fee_sat)
+			} else {
+				dust_limit_satoshis
+			};
+			next_splice_out_maximum_sat =
+				(local_balance_before_fee_msat / 1000).saturating_sub(min_balance_sat);
+		}
+	};
+	trim_splice_out_max_if_no_outputs(
+		local_nondust_htlc_count,
 		channel_constraints.holder_dust_limit_satoshis,
-		channel_type,
-	) {
-		let dust_limit_satoshis = channel_constraints.holder_dust_limit_satoshis;
-		let current_tx_fee_sat = commit_tx_fee_sat(feerate_per_kw, 0, channel_type);
-		let min_balance_sat = if is_outbound_from_holder {
-			dust_limit_satoshis.saturating_add(current_tx_fee_sat)
-		} else {
-			dust_limit_satoshis
-		};
-		next_splice_out_maximum_sat =
-			(local_balance_before_fee_msat / 1000).saturating_sub(min_balance_sat);
-	}
+	);
+	trim_splice_out_max_if_no_outputs(
+		remote_nondust_htlc_count,
+		channel_constraints.counterparty_dust_limit_satoshis,
+	);
 
 	if channel_value_satoshis < next_splice_out_maximum_sat + MIN_CHANNEL_VALUE_SATOSHIS {
 		next_splice_out_maximum_sat =
@@ -568,11 +586,10 @@ fn get_available_balances(
 		channel_value_satoshis,
 		local_balance_before_fee_msat,
 		remote_balance_before_fee_msat,
-		feerate_per_kw,
-		// The number of non-dust HTLCs on the local commitment at the current feerate
 		local_nondust_htlc_count,
-		// The post-splice minimum balance of the holder
-		if is_outbound_from_holder { local_min_commit_tx_fee_sat } else { 0 },
+		remote_nondust_htlc_count,
+		feerate_per_kw,
+		spiked_feerate,
 		&channel_constraints,
 		channel_type,
 	);