Don't trim HTLCs when calculating the fee spike commit tx fee

We previously accounted for HTLC trims at the spiked feerate when
calculating the commitment transaction fee including the fee spike
multiple.

This only ensured that the funder of the channel could afford the
commitment transaction fee for an exact 2x increase in the feerate.

Now, we check that the funder can cover any increase in the feerate
between 1x to 2x.

Author: tankyleo

lightning/src/ln/htlc_reserve_unit_tests.rs

@@ -3,8 +3,8 @@
 use crate::events::{ClosureReason, Event, HTLCHandlingFailureType, PaymentPurpose};
 use crate::ln::chan_utils::{
 	self, commit_tx_fee_sat, commitment_tx_base_weight, second_stage_tx_fees_sat,
-	shared_anchor_script_pubkey, CommitmentTransaction, COMMITMENT_TX_WEIGHT_PER_HTLC,
-	TRUC_CHILD_MAX_WEIGHT,
+	shared_anchor_script_pubkey, CommitmentTransaction, HTLCOutputInCommitment,
+	COMMITMENT_TX_WEIGHT_PER_HTLC, TRUC_CHILD_MAX_WEIGHT,
 };
 use crate::ln::channel::{
 	get_holder_selected_channel_reserve_satoshis, Channel, ANCHOR_OUTPUT_VALUE_SATOSHI,
@@ -888,7 +888,7 @@ pub fn do_test_fee_spike_buffer(cfg: Option<UserConfig>, htlc_fails: bool) {
 
 	// Build the remote commitment transaction so we can sign it, and then later use the
 	// signature for the commitment_signed message.
-	let accepted_htlc_info = chan_utils::HTLCOutputInCommitment {
+	let accepted_htlc_info = HTLCOutputInCommitment {
 		offered: false,
 		amount_msat: payment_amt_msat,
 		cltv_expiry: htlc_cltv,
@@ -2143,7 +2143,7 @@ pub fn do_test_dust_limit_fee_accounting(can_afford: bool) {
 		let (_payment_preimage, payment_hash, ..) =
 			route_payment(&nodes[0], &[&nodes[1]], HTLC_AMT_SAT * 1000);
 		// Grab a snapshot of these HTLCs to manually build the commitment transaction later...
-		let accepted_htlc = chan_utils::HTLCOutputInCommitment {
+		let accepted_htlc = HTLCOutputInCommitment {
 			offered: false,
 			amount_msat: HTLC_AMT_SAT * 1000,
 			// Hard-coded to match the expected value
@@ -2257,7 +2257,7 @@ pub fn do_test_dust_limit_fee_accounting(can_afford: bool) {
 				&channel_type,
 			);
 
-		let accepted_htlc_info = chan_utils::HTLCOutputInCommitment {
+		let accepted_htlc_info = HTLCOutputInCommitment {
 			offered: false,
 			amount_msat: HTLC_AMT_SAT * 1000,
 			cltv_expiry,
@@ -2838,21 +2838,30 @@ fn do_test_0reserve_no_outputs_legacy(no_outputs_case: LegacyChannelsNoOutputs)
 			return;
 		}
 
+		let htlcs_in_commitment = vec![HTLCOutputInCommitment {
+			offered: false,
+			amount_msat: receiver_amount_msat,
+			cltv_expiry: htlc_cltv,
+			payment_hash,
+			transaction_output_index: Some(1),
+		}];
+
 		manually_trigger_update_fail_htlc(
 			&nodes,
 			channel_id,
-			channel_value_sat,
+			channel_value_sat * 1000,
 			dust_limit_satoshis,
-			receiver_amount_msat,
-			htlc_cltv,
 			payment_hash,
+			htlcs_in_commitment,
+			false,
 		);
 	}
 }
 
 fn manually_trigger_update_fail_htlc<'a, 'b, 'c, 'd>(
-	nodes: &'a Vec<Node<'b, 'c, 'd>>, channel_id: ChannelId, channel_value_sat: u64,
-	dust_limit_satoshis: u64, receiver_amount_msat: u64, htlc_cltv: u32, payment_hash: PaymentHash,
+	nodes: &'a Vec<Node<'b, 'c, 'd>>, channel_id: ChannelId, value_to_self_msat: u64,
+	dust_limit_satoshis: u64, payment_hash: PaymentHash,
+	htlcs_in_commitment: Vec<HTLCOutputInCommitment>, can_afford_but_reserve_is_breached: bool,
 ) {
 	let node_a_id = nodes[0].node.get_our_node_id();
 	let node_b_id = nodes[1].node.get_our_node_id();
@@ -2864,45 +2873,36 @@ fn manually_trigger_update_fail_htlc<'a, 'b, 'c, 'd>(
 
 	let feerate_per_kw = get_feerate!(nodes[0], nodes[1], channel_id);
 
-	const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
-
 	let (local_secret, next_local_point) = {
 		let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
 		let chan_lock = per_peer_state.get(&node_b_id).unwrap().lock().unwrap();
 		let local_chan =
 			chan_lock.channel_by_id.get(&channel_id).and_then(Channel::as_funded).unwrap();
 		let chan_signer = local_chan.get_signer();
 		// Make the signer believe we validated another commitment, so we can release the secret
+		let commit_number = chan_signer.get_enforcement_state().last_holder_commitment;
 		chan_signer.get_enforcement_state().last_holder_commitment -= 1;
 
 		(
-			chan_signer.release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(),
-			chan_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(),
+			chan_signer.release_commitment_secret(commit_number).unwrap(),
+			chan_signer.get_per_commitment_point(commit_number - 2, &secp_ctx).unwrap(),
 		)
 	};
-	let remote_point = {
+	let (remote_commit_number, remote_point) = {
 		let per_peer_lock;
 		let mut peer_state_lock;
 
 		let channel =
 			get_channel_ref!(nodes[1], nodes[0], per_peer_lock, peer_state_lock, channel_id);
 		let chan_signer = channel.as_funded().unwrap().get_signer();
-		chan_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap()
+		let commit_number = chan_signer.get_enforcement_state().last_holder_commitment;
+		let remote_point =
+			chan_signer.get_per_commitment_point(commit_number - 1, &secp_ctx).unwrap();
+		(commit_number - 1, remote_point)
 	};
 
 	// Build the remote commitment transaction so we can sign it, and then later use the
 	// signature for the commitment_signed message.
-	let accepted_htlc_info = chan_utils::HTLCOutputInCommitment {
-		offered: false,
-		amount_msat: receiver_amount_msat,
-		cltv_expiry: htlc_cltv,
-		payment_hash,
-		transaction_output_index: Some(1),
-	};
-
-	let local_chan_balance_msat = channel_value_sat * 1000;
-	let commitment_number = INITIAL_COMMITMENT_NUMBER - 1;
-
 	let res = {
 		let per_peer_lock;
 		let mut peer_state_lock;
@@ -2913,12 +2913,12 @@ fn manually_trigger_update_fail_htlc<'a, 'b, 'c, 'd>(
 
 		let (commitment_tx, _stats) = SpecTxBuilder {}.build_commitment_transaction(
 			false,
-			commitment_number,
+			remote_commit_number,
 			&remote_point,
 			&channel.funding().channel_transaction_parameters,
 			&secp_ctx,
-			local_chan_balance_msat,
-			vec![accepted_htlc_info],
+			value_to_self_msat,
+			htlcs_in_commitment,
 			feerate_per_kw,
 			dust_limit_satoshis,
 			&nodes[0].logger,
@@ -2969,11 +2969,13 @@ fn manually_trigger_update_fail_htlc<'a, 'b, 'c, 'd>(
 		},
 		_ => panic!("Unexpected event"),
 	};
-	nodes[1].logger.assert_log(
-		"lightning::ln::channel",
-		"Attempting to fail HTLC due to balance exhausted on remote commitment".to_string(),
-		1,
-	);
+	let log_string =
+		if can_afford_but_reserve_is_breached {
+			String::from("Attempting to fail HTLC due to fee spike buffer violation. Rebalancing is required.")
+		} else {
+			String::from("Attempting to fail HTLC due to balance exhausted on remote commitment")
+		};
+	nodes[1].logger.assert_log("lightning::ln::channel", log_string, 1);
 
 	check_added_monitors(&nodes[1], 3);
 }
@@ -3549,3 +3551,199 @@ fn test_outbound_vs_available_capacity_outbound_htlc_limit_spiked_feerate() {
 		);
 	}
 }
+
+/// Make sure that we do not account for HTLCs going from non-dust to dust at the spiked feerate
+/// when checking the fee spike buffer in `can_accept_incoming_htlc`. This is required to make sure
+/// that we can afford *any* increase in the feerate between 1x to 2x, instead of checking whether
+/// we can afford only the 2x increase in the feerate.
+#[xtest(feature = "_externalize_tests")]
+fn test_fail_cannot_afford_dust_htlcs_at_spike_multiple_if_nondust_at_base_feerate() {
+	let mut config = test_default_channel_config();
+	config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = false;
+	config.channel_handshake_config.negotiate_anchor_zero_fee_commitments = false;
+
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	config.channel_handshake_config.announced_channel_max_inbound_htlc_value_in_flight_percentage =
+		100;
+
+	let channel_type = ChannelTypeFeatures::only_static_remote_key();
+
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config.clone()), Some(config)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let node_a_id = nodes[0].node.get_our_node_id();
+	let _node_b_id = nodes[1].node.get_our_node_id();
+
+	const FEERATE: u32 = 253;
+	const MULTIPLE: u32 = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE as u32;
+	const SPIKED_FEERATE: u32 = FEERATE * MULTIPLE;
+	const DUST_LIMIT_MSAT: u64 = 354 * 1000;
+	const CHANNEL_VALUE_MSAT: u64 = 10_000 * 1000;
+	const NODE_0_VALUE_TO_SELF_MSAT: u64 = 5_000 * 1000;
+	const NODE_1_VALUE_TO_SELF_MSAT: u64 = 5_000 * 1000;
+	const CHANNEL_RESERVE_MSAT: u64 = 1_000 * 1_000;
+
+	let channel_id = create_announced_chan_between_nodes_with_value(
+		&nodes,
+		0,
+		1,
+		CHANNEL_VALUE_MSAT / 1000,
+		NODE_1_VALUE_TO_SELF_MSAT,
+	)
+	.2;
+	assert_eq!(nodes[0].node.list_channels()[0].channel_type.as_ref().unwrap(), &channel_type);
+
+	// Find the HTLC amount that will be non-dust at the current feerate,
+	// but dust at the spiked feerate.
+	const SPIKED_DUST_HTLC_MSAT: u64 = 688 * 1000;
+	const HTLC_SPIKE_DUST_LIMIT_MSAT: u64 = 689 * 1000;
+	// When checking the fee spike buffer in `can_accept_incoming_htlc`, we check the remote
+	// commitment, hence inbound HTLCs will be offered HTLCs, and use the timeout dust limit.
+	let htlc_timeout_spike_tx_fee_msat =
+		second_stage_tx_fees_sat(&channel_type, SPIKED_FEERATE).1 * 1000;
+	assert_eq!(HTLC_SPIKE_DUST_LIMIT_MSAT, DUST_LIMIT_MSAT + htlc_timeout_spike_tx_fee_msat);
+
+	// Calculate here the dust limit at the current feerate so we know when node 0 cannot send
+	// any further non-dust HTLCs at the current feerate.
+	let htlc_timeout_tx_fee_msat = second_stage_tx_fees_sat(&channel_type, FEERATE).1 * 1000;
+	let htlc_dust_limit_msat = DUST_LIMIT_MSAT + htlc_timeout_tx_fee_msat;
+	// Make sure the HTLC will be non-dust at the current feerate
+	assert!(SPIKED_DUST_HTLC_MSAT > htlc_dust_limit_msat);
+
+	// Place a few non-dust HTLCs on the commitment, these HTLCs would get trimmed upon a 2x
+	// increase in the feerate.
+	let mut sent_htlcs_count: usize = 0;
+	let mut payment_hashes = Vec::new();
+	while nodes[0].node.list_channels()[0].next_outbound_htlc_limit_msat >= htlc_dust_limit_msat {
+		let (_preimage, hash, _secret, _id) =
+			route_payment(&nodes[0], &[&nodes[1]], SPIKED_DUST_HTLC_MSAT);
+		payment_hashes.push(hash);
+		sent_htlcs_count += 1;
+	}
+	assert_eq!(sent_htlcs_count, 4);
+
+	// Check the outbound and available capacities
+	let node_0_outbound_capacity_msat = NODE_0_VALUE_TO_SELF_MSAT
+		- sent_htlcs_count as u64 * SPIKED_DUST_HTLC_MSAT
+		- CHANNEL_RESERVE_MSAT;
+	let node_0_details = &nodes[0].node.list_channels()[0];
+	assert_eq!(node_0_details.outbound_capacity_msat, node_0_outbound_capacity_msat);
+	// Node 0 can now only send dust HTLCs, so we reserve the fees for a single additional
+	// inbound non-dust HTLC.
+	let min_reserved_fee_msat =
+		commit_tx_fee_sat(SPIKED_FEERATE, sent_htlcs_count + 1, &channel_type) * 1000;
+	let node_0_available_capacity_msat = node_0_outbound_capacity_msat - min_reserved_fee_msat;
+	assert_eq!(node_0_details.next_outbound_htlc_limit_msat, node_0_available_capacity_msat);
+
+	// Then send an identical, 5th non-dust HTLC, bypass the validation from the holder, and
+	// check that the counterparty fails it due to a fee spike buffer violation.
+
+	// First check the maths
+
+	// Node 0 can afford an exact 2x increase in the feerate
+	let spiked_commit_tx_fee_msat = commit_tx_fee_sat(SPIKED_FEERATE, 0, &channel_type) * 1000;
+	assert!((node_0_outbound_capacity_msat - SPIKED_DUST_HTLC_MSAT)
+		.checked_sub(spiked_commit_tx_fee_msat)
+		.is_some());
+	// Node 0 can afford a 5th non-dust HTLC at the current feerate, so `update_add_htlc`
+	// validation will pass.
+	let real_commit_tx_fee_msat = commit_tx_fee_sat(FEERATE, 5, &channel_type) * 1000;
+	assert!((node_0_outbound_capacity_msat - SPIKED_DUST_HTLC_MSAT)
+		.checked_sub(real_commit_tx_fee_msat)
+		.is_some());
+	// But we don't account for the HTLC trimming effect of the spike multiple feerate increase,
+	//  so the 5th HTLC should be rejected at `can_accept_incoming_htlc`!
+	let expected_commit_tx_fee_msat = commit_tx_fee_sat(SPIKED_FEERATE, 5, &channel_type) * 1000;
+	assert!((node_0_outbound_capacity_msat - SPIKED_DUST_HTLC_MSAT)
+		.checked_sub(expected_commit_tx_fee_msat)
+		.is_none());
+
+	// Then run the experiment
+
+	let sender_amount_msat = node_0_available_capacity_msat;
+	let receiver_amount_msat = SPIKED_DUST_HTLC_MSAT;
+	let (route, payment_hash, _, payment_secret) =
+		get_route_and_payment_hash!(nodes[0], nodes[1], sender_amount_msat);
+	let secp_ctx = Secp256k1::new();
+	let session_priv = SecretKey::from_slice(&[42; 32]).unwrap();
+	let cur_height = nodes[0].node.best_block.read().unwrap().height + 1;
+	let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route.paths[0], &session_priv);
+	let recipient_onion_fields =
+		RecipientOnionFields::secret_only(payment_secret, sender_amount_msat);
+	let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::test_build_onion_payloads(
+		&route.paths[0],
+		&recipient_onion_fields,
+		cur_height,
+		&None,
+		None,
+		None,
+	)
+	.unwrap();
+	assert_eq!(htlc_msat, sender_amount_msat);
+	let onion_packet =
+		onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash)
+			.unwrap();
+	let msg = msgs::UpdateAddHTLC {
+		channel_id,
+		htlc_id: sent_htlcs_count as u64,
+		amount_msat: receiver_amount_msat,
+		payment_hash,
+		cltv_expiry: htlc_cltv,
+		onion_routing_packet: onion_packet,
+		skimmed_fee_msat: None,
+		blinding_point: None,
+		hold_htlc: None,
+		accountable: None,
+	};
+
+	nodes[1].node.handle_update_add_htlc(node_a_id, &msg);
+
+	let htlcs_in_tx = vec![
+		HTLCOutputInCommitment {
+			offered: false,
+			cltv_expiry: 81,
+			payment_hash: payment_hashes.iter().find(|hash| hash.0[0] == 0x75).unwrap().clone(),
+			amount_msat: 688_000,
+			transaction_output_index: Some(0),
+		},
+		HTLCOutputInCommitment {
+			offered: false,
+			cltv_expiry: 81,
+			payment_hash: payment_hashes.iter().find(|hash| hash.0[0] == 0x64).unwrap().clone(),
+			amount_msat: 688_000,
+			transaction_output_index: Some(1),
+		},
+		HTLCOutputInCommitment {
+			offered: false,
+			cltv_expiry: 81,
+			payment_hash,
+			amount_msat: 688_000,
+			transaction_output_index: Some(2),
+		},
+		HTLCOutputInCommitment {
+			offered: false,
+			cltv_expiry: 81,
+			payment_hash: payment_hashes.iter().find(|hash| hash.0[0] == 0x72).unwrap().clone(),
+			amount_msat: 688_000,
+			transaction_output_index: Some(3),
+		},
+		HTLCOutputInCommitment {
+			offered: false,
+			cltv_expiry: 81,
+			payment_hash: payment_hashes.iter().find(|hash| hash.0[0] == 0x66).unwrap().clone(),
+			amount_msat: 688_000,
+			transaction_output_index: Some(4),
+		},
+	];
+
+	manually_trigger_update_fail_htlc(
+		&nodes,
+		channel_id,
+		NODE_0_VALUE_TO_SELF_MSAT,
+		DUST_LIMIT_MSAT / 1000,
+		payment_hash,
+		htlcs_in_tx,
+		true,
+	);
+}

lightning/src/sign/tx_builder.rs

@@ -300,9 +300,18 @@ fn get_next_commitment_stats(
 	// 2) Now including any additional non-dust HTLCs (usually the fee spike buffer HTLC), does the funder cover
 	// this bigger transaction fee ? The funder can dip below their dust limit to cover this case, as the
 	// commitment will have at least one output: the non-dust fee spike buffer HTLC offered by the counterparty.
+	let nondust_htlc_count = next_commitment_htlcs
+		.iter()
+		.filter(|htlc| {
+			!htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_satoshis, channel_type)
+		})
+		.count();
+	// Note here we use the htlc count at the current feerate together with the spiked feerate;
+	// this makes sure that the holder can afford any fee bump between 1x to 2x from the current
+	// feerate if the fee spike multiple is included.
 	let commit_tx_fee_sat = commit_tx_fee_sat(
 		spiked_feerate,
-		spiked_nondust_htlc_count + addl_nondust_htlc_count,
+		nondust_htlc_count + addl_nondust_htlc_count,
 		channel_type,
 	);
 	let (holder_balance_msat, counterparty_balance_msat) = checked_sub_from_funder(
@@ -317,7 +326,7 @@ fn get_next_commitment_stats(
 		counterparty_balance_msat,
 		dust_exposure_msat,
 		#[cfg(any(test, fuzzing))]
-		nondust_htlc_count: spiked_nondust_htlc_count + addl_nondust_htlc_count,
+		nondust_htlc_count: nondust_htlc_count + addl_nondust_htlc_count,
 		#[cfg(any(test, fuzzing))]
 		commit_tx_fee_sat,
 	})