Don't include below-dust inbound HTLCs in commit tx fee calculation

Also remove part of the holding cell channel reserve test that's newly failing but
a bit of a redundant test anyway.

Author: valentinewallace

lightning/src/ln/channel.rs

@@ -1729,10 +1729,23 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	// commitment tx. `addl_htcs` is an optional parameter allowing the caller
 	// to add a number of additional HTLCs to the calculation. Note that dust
 	// HTLCs are excluded.
-	fn next_local_commit_tx_fee_msat(&self, addl_htlcs: usize) -> u64 {
+	fn next_local_commit_tx_fee_msat(&self, new_htlc_amount: u64, fee_spike_buffer_htlc: Option<()>) -> u64 {
 		assert!(self.channel_outbound);
 
-		let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+		let mut addl_htlcs = 1;
+		if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
+		if new_htlc_amount / 1000 <= self.counterparty_dust_limit_satoshis {
+			addl_htlcs -= 1;
+		}
+
+		let mut their_acked_htlcs = 0;
+		for ref htlc in self.pending_inbound_htlcs.iter() {
+			if htlc.amount_msat / 1000 <= self.counterparty_dust_limit_satoshis {
+				continue
+			}
+			their_acked_htlcs += 1;
+		}
+
 		for ref htlc in self.pending_outbound_htlcs.iter() {
 			if htlc.amount_msat / 1000 <= self.holder_dust_limit_satoshis {
 				continue
@@ -1760,13 +1773,27 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	// next commitment tx. `addl_htcs` is an optional parameter allowing the caller
 	// to add a number of additional HTLCs to the calculation. Note that dust HTLCs
 	// are excluded.
-	fn next_remote_commit_tx_fee_msat(&self, addl_htlcs: usize) -> u64 {
+	fn next_remote_commit_tx_fee_msat(&self, new_htlc_amount: u64, fee_spike_buffer_htlc: Option<()>) -> u64 {
 		assert!(!self.channel_outbound);
 
-		// When calculating the set of HTLCs which will be included in their next
-		// commitment_signed, all inbound HTLCs are included (as all states imply it will be
-		// included) and only committed outbound HTLCs, see below.
-		let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+		let mut addl_htlcs = 1;
+		if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
+		if new_htlc_amount / 1000 <= self.holder_dust_limit_satoshis {
+			addl_htlcs -= 1;
+		}
+
+		// When calculating the set of HTLCs which will be included in their next commitment_signed, all
+		// non-dust inbound HTLCs are included (as all states imply it will be included) and only
+		// committed outbound HTLCs, see below.
+		// let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+		let mut their_acked_htlcs = 0;
+		for ref htlc in self.pending_inbound_htlcs.iter() {
+			if htlc.amount_msat / 1000 <= self.holder_dust_limit_satoshis {
+				continue
+			}
+			their_acked_htlcs += 1;
+		}
+
 		for ref htlc in self.pending_outbound_htlcs.iter() {
 			if htlc.amount_msat / 1000 <= self.counterparty_dust_limit_satoshis {
 				continue
@@ -1848,8 +1875,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		// Check that the remote can afford to pay for this HTLC on-chain at the current
 		// feerate_per_kw, while maintaining their channel reserve (as required by the spec).
 		let remote_commit_tx_fee_msat = if self.channel_outbound { 0 } else {
-			// +1 for this HTLC.
-			self.next_remote_commit_tx_fee_msat(1)
+			self.next_remote_commit_tx_fee_msat(msg.amount_msat, None) // Don't include the extra fee spike buffer HTLC in calculations
 		};
 		if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
 			return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
@@ -1862,14 +1888,15 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		}
 
 		if !self.channel_outbound {
-			// `+1` for this HTLC, `2 *` and `+1` fee spike buffer we keep for the remote. This deviates from the
-			// spec because in the spec, the fee spike buffer requirement doesn't exist on the receiver's side,
-			// only on the sender's.
-			// Note that when we eventually remove support for fee updates and switch to anchor output fees,
-			// we will drop the `2 *`, since we no longer be as sensitive to fee spikes. But, keep the extra +1
-			// as we should still be able to afford adding this HTLC plus one more future HTLC, regardless of
-			// being sensitive to fee spikes.
-			let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.next_remote_commit_tx_fee_msat(1 + 1);
+			// `2 *` and `Some(())` is for the fee spike buffer we keep for the remote. This deviates from
+			// the spec because in the spec, the fee spike buffer requirement doesn't exist on the
+			// receiver's side, only on the sender's.
+			// Note that when we eventually remove support for fee updates and switch to anchor output
+			// fees, we will drop the `2 *`, since we no longer be as sensitive to fee spikes. But, keep
+			// the extra htlc when calculating the next remote commitment transaction fee as we should
+			// still be able to afford adding this HTLC plus one more future HTLC, regardless of being
+			// sensitive to fee spikes.
+			let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.next_remote_commit_tx_fee_msat(msg.amount_msat, Some(()));
 			if pending_remote_value_msat - msg.amount_msat - chan_reserve_msat < remote_fee_cost_incl_stuck_buffer_msat {
 				// Note that if the pending_forward_status is not updated here, then it's because we're already failing
 				// the HTLC, i.e. its status is already set to failing.
@@ -1878,9 +1905,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		} else {
 			// Check that they won't violate our local required channel reserve by adding this HTLC.
-
-			// +1 for this HTLC.
-			let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(1);
+			let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(msg.amount_msat, None);
 			if self.value_to_self_msat < self.counterparty_selected_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
 				return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
 			}
@@ -3738,11 +3763,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		if !self.channel_outbound {
 			// Check that we won't violate the remote channel reserve by adding this HTLC.
-
 			let counterparty_balance_msat = self.channel_value_satoshis * 1000 - self.value_to_self_msat;
 			let holder_selected_chan_reserve_msat = Channel::<ChanSigner>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis);
-			// 1 additional HTLC corresponding to this HTLC.
-			let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(1);
+			let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(amount_msat, None);
 			if counterparty_balance_msat < holder_selected_chan_reserve_msat + counterparty_commit_tx_fee_msat {
 				return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned()));
 			}
@@ -3753,10 +3776,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			return Err(ChannelError::Ignore(format!("Cannot send value that would overdraw remaining funds. Amount: {}, pending value to self {}", amount_msat, pending_value_to_self_msat)));
 		}
 
-		// The `+1` is for the HTLC currently being added to the commitment tx and
-		// the `2 *` and `+1` are for the fee spike buffer.
+		// `2 *` and extra HTLC are for the fee spike buffer.
 		let commit_tx_fee_msat = if self.channel_outbound {
-			2 * self.next_local_commit_tx_fee_msat(1 + 1)
+			2 * self.next_local_commit_tx_fee_msat(amount_msat, Some(()))
 		} else { 0 };
 		if pending_value_to_self_msat - amount_msat < commit_tx_fee_msat {
 			return Err(ChannelError::Ignore(format!("Cannot send value that would not leave enough to pay for fees. Pending value to self: {}. local_commit_tx_fee {}", pending_value_to_self_msat, commit_tx_fee_msat)));

lightning/src/ln/functional_tests.rs

@@ -1764,7 +1764,7 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() {
 		}}
 	};
 
-	let (route, our_payment_hash, _) = get_route_and_payment_hash!(1000);
+	let (route, our_payment_hash, _) = get_route_and_payment_hash!(1001000);
 	unwrap_send_err!(nodes[1].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { ref err },
 		assert_eq!(err, "Cannot send value that would put counterparty balance under holder-announced channel reserve value"));
 	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
@@ -1822,6 +1822,36 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
 	check_added_monitors!(nodes[0], 1);
 }
 
+#[test]
+fn test_chan_reserve_dust_htlcs() {
+	// Test that if we receive many dust HTLCs over an inbound channel, they don't count when
+	// calculating our counterparty's commitment transaction fee (this was previously broken).
+	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, &[None, None, None]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 98000000, InitFeatures::known(), InitFeatures::known());
+
+	let payment_amt = 46000; // Dust amount
+	// In the previous code, these first four payments would succeed.
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+
+	// Then these next 5 would be interpreted by nodes[1] as violating the fee spike buffer.
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+
+	// And this last payment previously resulted in nodes[1] closing on its inbound-channel
+	// counterparty, because it counted all the previous dust HTLCs against nodes[0]'s commitment
+	// transaction fee and therefore perceived this next payment as a channel reserve violation.
+	let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+}
+
 #[test]
 fn test_chan_reserve_violation_inbound_htlc_inbound_chan() {
 	let chanmon_cfgs = create_chanmon_cfgs(3);
@@ -2133,23 +2163,6 @@ fn test_channel_reserve_holding_cell_htlcs() {
 
 	let commit_tx_fee_0_htlcs = 2*commit_tx_fee_msat(feerate, 1);
 	let recv_value_3 = commit_tx_fee_2_htlcs - commit_tx_fee_0_htlcs - total_fee_msat;
-	{
-		let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_3 + 1);
-		let err = nodes[0].node.send_payment(&route, our_payment_hash, &None).err().unwrap();
-		match err {
-			PaymentSendFailure::AllFailedRetrySafe(ref fails) => {
-				match &fails[0] {
-					&APIError::ChannelUnavailable{ref err} =>
-						assert!(regex::Regex::new(r"Cannot send value that would put our balance under counterparty-announced channel reserve value \(\d+\)").unwrap().is_match(err)),
-					_ => panic!("Unexpected error variant"),
-				}
-			},
-			_ => panic!("Unexpected error variant"),
-		}
-		assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-		nodes[0].logger.assert_log_contains("lightning::ln::channelmanager".to_string(), "Cannot send value that would put our balance under counterparty-announced channel reserve value".to_string(), 3);
-	}
-
 	send_payment(&nodes[0], &vec![&nodes[1], &nodes[2]][..], recv_value_3, recv_value_3);
 
 	let commit_tx_fee_1_htlc = 2*commit_tx_fee_msat(feerate, 1 + 1);