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

@@ -1684,63 +1684,108 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		(COMMITMENT_TX_BASE_WEIGHT + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * self.feerate_per_kw as u64 / 1000 * 1000
 	}
 
-	// Get the commitment tx fee for the local (i.e our) next commitment transaction
-	// based on the number of pending HTLCs that are on track to be in our 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_local_commit_tx_fee_msat(&self, addl_htlcs: usize) -> u64 {
+	// Get the commitment tx fee for the local's (i.e. our) next commitment transaction based on the
+	// number of pending HTLCs that are on track to be in our next commitment tx, plus an additional
+	// HTLC if `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs
+	// are excluded.
+	fn next_local_commit_tx_fee_msat(&self, new_htlc_amount: u64, locally_offered: bool, fee_spike_buffer_htlc: Option<()>) -> u64 {
 		assert!(self.is_outbound());
 
-		let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+		let real_dust_limit_success_sat = (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
+		let real_dust_limit_timeout_sat = (self.feerate_per_kw as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
+
+		let mut addl_htlcs = 0;
+		if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
+		if locally_offered && new_htlc_amount / 1000 >= real_dust_limit_timeout_sat {
+			addl_htlcs += 1;
+		} else if !locally_offered && new_htlc_amount / 1000 >= real_dust_limit_success_sat {
+			addl_htlcs += 1;
+		}
+
+		let mut included_htlcs = 0;
+		for ref htlc in self.pending_inbound_htlcs.iter() {
+			if htlc.amount_msat / 1000 < real_dust_limit_success_sat {
+				continue
+			}
+			// We include LocalRemoved HTLCs here because we may still need to broadcast a commitment
+			// transaction including this HTLC if it times out before they RAA.
+			included_htlcs += 1;
+		}
+
 		for ref htlc in self.pending_outbound_htlcs.iter() {
-			if htlc.amount_msat / 1000 <= self.holder_dust_limit_satoshis {
+			if htlc.amount_msat / 1000 < real_dust_limit_timeout_sat {
 				continue
 			}
 			match htlc.state {
-				OutboundHTLCState::Committed => their_acked_htlcs += 1,
-				OutboundHTLCState::RemoteRemoved {..} => their_acked_htlcs += 1,
-				OutboundHTLCState::LocalAnnounced {..} => their_acked_htlcs += 1,
+				OutboundHTLCState::LocalAnnounced {..} => included_htlcs += 1,
+				OutboundHTLCState::Committed => included_htlcs += 1,
+				OutboundHTLCState::RemoteRemoved {..} => included_htlcs += 1,
+				// We don't include AwaitingRemoteRevokeToRemove HTLCs because our next commitment
+				// transaction won't be generated until they send us their next RAA, which will mean
+				// dropping any HTLCs in this state.
 				_ => {},
 			}
 		}
 
 		for htlc in self.holding_cell_htlc_updates.iter() {
 			match htlc {
-				&HTLCUpdateAwaitingACK::AddHTLC { .. } => their_acked_htlcs += 1,
-				_ => {},
+				&HTLCUpdateAwaitingACK::AddHTLC { amount_msat, .. } => {
+					if amount_msat / 1000 < real_dust_limit_timeout_sat {
+						continue
+					}
+					included_htlcs += 1
+				},
+				_ => {}, // Don't include claims/fails that are awaiting ack, because once we get the
+				         // ack we're guaranteed to never include them in commitment txs anymore.
 			}
 		}
 
-		self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
+		self.commit_tx_fee_msat(included_htlcs + addl_htlcs)
 	}
 
-	// Get the commitment tx fee for the remote's next commitment transaction
-	// based on the number of pending HTLCs that are on track to be in their
-	// 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 {
+	// Get the commitment tx fee for the remote's next commitment transaction based on the number of
+	// pending HTLCs that are on track to be in their next commitment tx, plus an additional HTLC if
+	// `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs are
+	// excluded.
+	fn next_remote_commit_tx_fee_msat(&self, new_htlc_amount: u64, locally_offered: bool, fee_spike_buffer_htlc: Option<()>) -> u64 {
 		assert!(!self.is_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 real_dust_limit_success_sat = (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
+		let real_dust_limit_timeout_sat = (self.feerate_per_kw as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
+
+		let mut addl_htlcs = 0;
+		if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
+		if locally_offered && new_htlc_amount / 1000 >= real_dust_limit_success_sat {
+			addl_htlcs += 1;
+		} else if !locally_offered && new_htlc_amount / 1000 >= real_dust_limit_timeout_sat {
+			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 included_htlcs = 0;
+		for ref htlc in self.pending_inbound_htlcs.iter() {
+			if htlc.amount_msat / 1000 <= real_dust_limit_timeout_sat {
+				continue
+			}
+			included_htlcs += 1;
+		}
+
 		for ref htlc in self.pending_outbound_htlcs.iter() {
-			if htlc.amount_msat / 1000 <= self.counterparty_dust_limit_satoshis {
+			if htlc.amount_msat / 1000 <= real_dust_limit_success_sat {
 				continue
 			}
-			// We only include outbound HTLCs if it will not be included in their next
-			// commitment_signed, i.e. if they've responded to us with an RAA after announcement.
+			// We only include outbound HTLCs if it will not be included in their next commitment_signed,
+			// i.e. if they've responded to us with an RAA after announcement.
 			match htlc.state {
-				OutboundHTLCState::Committed => their_acked_htlcs += 1,
-				OutboundHTLCState::RemoteRemoved {..} => their_acked_htlcs += 1,
+				OutboundHTLCState::Committed => included_htlcs += 1,
+				OutboundHTLCState::RemoteRemoved {..} => included_htlcs += 1,
 				_ => {},
 			}
 		}
 
-		self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
+		self.commit_tx_fee_msat(included_htlcs + addl_htlcs)
 	}
 
 	pub fn update_add_htlc<F, L: Deref>(&mut self, msg: &msgs::UpdateAddHTLC, mut pending_forward_status: PendingHTLCStatus, create_pending_htlc_status: F, logger: &L) -> Result<(), ChannelError>
@@ -1808,8 +1853,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.is_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, false, 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()));
@@ -1822,14 +1866,15 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		}
 
 		if !self.is_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, false, 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.
@@ -1838,9 +1883,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, false, 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()));
 			}
@@ -3720,11 +3763,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		if !self.is_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, true, 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()));
 			}
@@ -3735,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.is_outbound() {
-			2 * self.next_local_commit_tx_fee_msat(1 + 1)
+			2 * self.next_local_commit_tx_fee_msat(amount_msat, true, 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)));
@@ -4492,7 +4532,7 @@ mod tests {
 	use ln::features::InitFeatures;
 	use ln::msgs::{OptionalField, DataLossProtect, DecodeError};
 	use ln::chan_utils;
-	use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
+	use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT};
 	use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
 	use chain::keysinterface::{InMemoryChannelKeys, KeysInterface};
 	use chain::transaction::OutPoint;
@@ -4575,6 +4615,116 @@ mod tests {
 		assert_eq!(open_channel_msg.feerate_per_kw, original_fee);
 	}
 
+	#[test]
+	fn test_holder_vs_counterparty_dust_limit() {
+		// Test that when calculating the local and remote commitment transaction fees, the correct
+		// dust limits are used.
+		let feeest = TestFeeEstimator{fee_est: 15000};
+		let secp_ctx = Secp256k1::new();
+		let seed = [42; 32];
+		let network = Network::Testnet;
+		let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
+
+		// Go through the flow of opening a channel between two nodes, making sure
+		// they have different dust limits.
+
+		// Create Node A's channel pointing to Node B's pubkey
+		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
+		let config = UserConfig::default();
+		let mut node_a_chan = Channel::<EnforcingChannelKeys>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap();
+
+		// Create Node B's channel by receiving Node A's open_channel message
+		// Make sure A's dust limit is as we expect.
+		let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
+		assert_eq!(open_channel_msg.dust_limit_satoshis, 1560);
+		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
+		let node_b_chan = Channel::<EnforcingChannelKeys>::new_from_req(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), &open_channel_msg, 7, &config).unwrap();
+
+		// Node B --> Node A: accept channel, explicitly setting B's dust limit.
+		let mut accept_channel_msg = node_b_chan.get_accept_channel();
+		accept_channel_msg.dust_limit_satoshis = 546;
+		node_a_chan.accept_channel(&accept_channel_msg, &config, InitFeatures::known()).unwrap();
+
+		// Put some inbound and outbound HTLCs in A's channel.
+		let htlc_amount_msat = 11_092_000; // put an amount below A's effective dust amount but above B's.
+		node_a_chan.pending_inbound_htlcs.push(InboundHTLCOutput {
+			htlc_id: 0,
+			amount_msat: htlc_amount_msat,
+			payment_hash: PaymentHash(Sha256::hash(&[42; 32]).into_inner()),
+			cltv_expiry: 300000000,
+			state: InboundHTLCState::Committed,
+		});
+
+		node_a_chan.pending_outbound_htlcs.push(OutboundHTLCOutput {
+			htlc_id: 1,
+			amount_msat: htlc_amount_msat, // put an amount below A's dust amount but above B's.
+			payment_hash: PaymentHash(Sha256::hash(&[43; 32]).into_inner()),
+			cltv_expiry: 200000000,
+			state: OutboundHTLCState::Committed,
+			source: HTLCSource::OutboundRoute {
+				path: Vec::new(),
+				session_priv: SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(),
+				first_hop_htlc_msat: 548,
+			}
+		});
+
+		// Make sure when Node A calculates their local commitment transaction, none of the HTLCs pass
+		// the dust limit check.
+		let local_commit_tx_fee = node_a_chan.next_local_commit_tx_fee_msat(htlc_amount_msat, true, None);
+		let local_commit_fee_0_htlcs = node_a_chan.commit_tx_fee_msat(0);
+		assert_eq!(local_commit_tx_fee, local_commit_fee_0_htlcs);
+
+		// Finally, make sure that when Node A calculates the remote's commitment transaction fees, all
+		// of the HTLCs are seen to be above the dust limit.
+		node_a_chan.channel_transaction_parameters.is_outbound_from_holder = false;
+		let remote_commit_fee_3_htlcs = node_a_chan.commit_tx_fee_msat(3);
+		let remote_commit_tx_fee = node_a_chan.next_remote_commit_tx_fee_msat(htlc_amount_msat, true, None);
+		assert_eq!(remote_commit_tx_fee, remote_commit_fee_3_htlcs);
+	}
+
+	#[test]
+	fn test_timeout_vs_success_htlc_dust_limit() {
+		// Make sure that when `next_remote_commit_tx_fee_msat` and `next_local_commit_tx_fee_msat`
+		// calculate the real dust limits for HTLCs (i.e. the dust limit given by the counterparty
+		// *plus* the fees paid for the HTLC) they don't swap `HTLC_SUCCESS_TX_WEIGHT` for
+		// `HTLC_TIMEOUT_TX_WEIGHT`, and vice versa.
+		let fee_est = TestFeeEstimator{fee_est: 253 };
+		let secp_ctx = Secp256k1::new();
+		let seed = [42; 32];
+		let network = Network::Testnet;
+		let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
+
+		let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
+		let config = UserConfig::default();
+		let mut chan = Channel::<EnforcingChannelKeys>::new_outbound(&&fee_est, &&keys_provider, node_id, 10000000, 100000, 42, &config).unwrap();
+
+		let commitment_tx_fee_0_htlcs = chan.commit_tx_fee_msat(0);
+		let commitment_tx_fee_1_htlc = chan.commit_tx_fee_msat(1);
+
+		// If HTLC_SUCCESS_TX_WEIGHT and HTLC_TIMEOUT_TX_WEIGHT were swapped: then this HTLC would be
+		// counted as dust when it shouldn't be.
+		let htlc_amt_above_timeout = ((253 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + chan.holder_dust_limit_satoshis + 1) * 1000;
+		let commitment_tx_fee = chan.next_local_commit_tx_fee_msat(htlc_amt_above_timeout, true, None);
+		assert_eq!(commitment_tx_fee, commitment_tx_fee_1_htlc);
+
+		// If swapped: this HTLC would be counted as non-dust when it shouldn't be.
+		let dust_htlc_amt_below_success = ((253 * HTLC_SUCCESS_TX_WEIGHT / 1000) + chan.holder_dust_limit_satoshis - 1) * 1000;
+		let commitment_tx_fee = chan.next_local_commit_tx_fee_msat(dust_htlc_amt_below_success, false, None);
+		assert_eq!(commitment_tx_fee, commitment_tx_fee_0_htlcs);
+
+		chan.channel_transaction_parameters.is_outbound_from_holder = false;
+
+		// If swapped: this HTLC would be counted as non-dust when it shouldn't be.
+		let dust_htlc_amt_above_timeout = ((253 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + chan.counterparty_dust_limit_satoshis + 1) * 1000;
+		let commitment_tx_fee = chan.next_remote_commit_tx_fee_msat(dust_htlc_amt_above_timeout, true, None);
+		assert_eq!(commitment_tx_fee, commitment_tx_fee_0_htlcs);
+
+		// If swapped: this HTLC would be counted as dust when it shouldn't be.
+		let htlc_amt_below_success = ((253 * HTLC_SUCCESS_TX_WEIGHT / 1000) + chan.counterparty_dust_limit_satoshis - 1) * 1000;
+		let commitment_tx_fee = chan.next_remote_commit_tx_fee_msat(htlc_amt_below_success, false, None);
+		assert_eq!(commitment_tx_fee, commitment_tx_fee_1_htlc);
+	}
+
 	#[test]
 	fn channel_reestablish_no_updates() {
 		let feeest = TestFeeEstimator{fee_est: 15000};