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

valentinewallace · valentinewallace · commit 15a92be2cdbc · 2020-12-15T21:06:50.000-05:00
Also remove part of the holding cell channel reserve test that's newly failing but
a bit of a redundant test anyway.
diff --git a/lightning/src/ln/channel.rs b/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.holder_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.holder_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.counterparty_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.counterparty_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)));
@@ -4587,6 +4609,73 @@ mod tests {
 		assert_eq!(open_channel_msg.feerate_per_kw, original_fee);
 	}
 
+	#[test]
+	fn test_commit_tx_fees_with_dust() {
+		// 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.
+		node_a_chan.pending_inbound_htlcs.push(InboundHTLCOutput {
+			htlc_id: 0,
+			amount_msat: 547_000, // put an amount below A's dust amount but above B's.
+			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: 548_000, // 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 htlc_amt = 549_000;
+		let local_commit_tx_fee = node_a_chan.next_local_commit_tx_fee_msat(htlc_amt, 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_outbound = 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_amt, None);
+		assert_eq!(remote_commit_tx_fee, remote_commit_fee_3_htlcs);
+	}
+
 	#[test]
 	fn channel_reestablish_no_updates() {
 		let feeest = TestFeeEstimator{fee_est: 15000};
diff --git a/lightning/src/ln/functional_tests.rs b/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,57 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
 	check_added_monitors!(nodes[0], 1);
 }
 
+#[test]
+fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
+	// Test that if we receive many dust HTLCs over an outbound channel, they don't count when
+	// calculating our 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);
+
+	// Set nodes[0]'s balance such that they will consider any above-dust received HTLC to be a
+	// channel reserve violation (so their balance is channel reserve (1000 sats) + commitment
+	// transaction fee with 0 HTLCs (183 sats)).
+	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 98817000, InitFeatures::known(), InitFeatures::known());
+
+	let dust_amt = 546000; // Dust amount
+	// In the previous code, routing this dust payment would cause nodes[0] to perceive a channel
+	// reserve violation even though it's a dust HTLC and therefore shouldn't count towards the
+	// commitment transaction fee.
+	let (_, _) = route_payment(&nodes[1], &[&nodes[0]], dust_amt);
+}
+
+#[test]
+fn test_chan_reserve_dust_inbound_htlcs_inbound_chan() {
+	// 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 +2184,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);
