Merge pull request #1009 from ariard/2021-07-add-forward-dust-limit

Add new config setting `max_balance_dust_htlc_msat`

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -247,6 +247,7 @@ fn check_api_err(api_err: APIError) {
 				_ if err.starts_with("Cannot send value that would put counterparty balance under holder-announced channel reserve value") => {},
 				_ if err.starts_with("Cannot send value that would overdraw remaining funds.") => {},
 				_ if err.starts_with("Cannot send value that would not leave enough to pay for fees.") => {},
+				_ if err.starts_with("Cannot send value that would put our exposure to dust HTLCs at") => {},
 				_ => panic!("{}", err),
 			}
 		},

lightning/src/ln/channel.rs

@@ -275,6 +275,14 @@ enum HTLCInitiator {
 	RemoteOffered,
 }
 
+/// An enum gathering stats on pending HTLCs, either inbound or outbound side.
+struct HTLCStats {
+	pending_htlcs: u32,
+	pending_htlcs_value_msat: u64,
+	on_counterparty_tx_dust_exposure_msat: u64,
+	on_holder_tx_dust_exposure_msat: u64,
+}
+
 /// Used when calculating whether we or the remote can afford an additional HTLC.
 struct HTLCCandidate {
 	amount_msat: u64,
@@ -1842,32 +1850,63 @@ impl<Signer: Sign> Channel<Signer> {
 		Ok(())
 	}
 
-	/// Returns (inbound_htlc_count, htlc_inbound_value_msat)
-	fn get_inbound_pending_htlc_stats(&self) -> (u32, u64) {
-		let mut htlc_inbound_value_msat = 0;
+	/// Returns a HTLCStats about inbound pending htlcs
+	fn get_inbound_pending_htlc_stats(&self) -> HTLCStats {
+		let mut stats = HTLCStats {
+			pending_htlcs: self.pending_inbound_htlcs.len() as u32,
+			pending_htlcs_value_msat: 0,
+			on_counterparty_tx_dust_exposure_msat: 0,
+			on_holder_tx_dust_exposure_msat: 0,
+		};
+
+		let counterparty_dust_limit_timeout_sat = (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
+		let holder_dust_limit_success_sat = (self.get_dust_buffer_feerate() as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
 		for ref htlc in self.pending_inbound_htlcs.iter() {
-			htlc_inbound_value_msat += htlc.amount_msat;
+			stats.pending_htlcs_value_msat += htlc.amount_msat;
+			if htlc.amount_msat / 1000 < counterparty_dust_limit_timeout_sat {
+				stats.on_counterparty_tx_dust_exposure_msat += htlc.amount_msat;
+			}
+			if htlc.amount_msat / 1000 < holder_dust_limit_success_sat {
+				stats.on_holder_tx_dust_exposure_msat += htlc.amount_msat;
+			}
 		}
-		(self.pending_inbound_htlcs.len() as u32, htlc_inbound_value_msat)
+		stats
 	}
 
-	/// Returns (outbound_htlc_count, htlc_outbound_value_msat) *including* pending adds in our
-	/// holding cell.
-	fn get_outbound_pending_htlc_stats(&self) -> (u32, u64) {
-		let mut htlc_outbound_value_msat = 0;
+	/// Returns a HTLCStats about pending outbound htlcs, *including* pending adds in our holding cell.
+	fn get_outbound_pending_htlc_stats(&self) -> HTLCStats {
+		let mut stats = HTLCStats {
+			pending_htlcs: self.pending_outbound_htlcs.len() as u32,
+			pending_htlcs_value_msat: 0,
+			on_counterparty_tx_dust_exposure_msat: 0,
+			on_holder_tx_dust_exposure_msat: 0,
+		};
+
+		let counterparty_dust_limit_success_sat = (self.get_dust_buffer_feerate() as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
+		let holder_dust_limit_timeout_sat = (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
 		for ref htlc in self.pending_outbound_htlcs.iter() {
-			htlc_outbound_value_msat += htlc.amount_msat;
+			stats.pending_htlcs_value_msat += htlc.amount_msat;
+			if htlc.amount_msat / 1000 < counterparty_dust_limit_success_sat {
+				stats.on_counterparty_tx_dust_exposure_msat += htlc.amount_msat;
+			}
+			if htlc.amount_msat / 1000 < holder_dust_limit_timeout_sat {
+				stats.on_holder_tx_dust_exposure_msat += htlc.amount_msat;
+			}
 		}
 
-		let mut htlc_outbound_count = self.pending_outbound_htlcs.len();
 		for update in self.holding_cell_htlc_updates.iter() {
 			if let &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } = update {
-				htlc_outbound_count += 1;
-				htlc_outbound_value_msat += amount_msat;
+				stats.pending_htlcs += 1;
+				stats.pending_htlcs_value_msat += amount_msat;
+				if *amount_msat / 1000 < counterparty_dust_limit_success_sat {
+					stats.on_counterparty_tx_dust_exposure_msat += amount_msat;
+				}
+				if *amount_msat / 1000 < holder_dust_limit_timeout_sat {
+					stats.on_holder_tx_dust_exposure_msat += amount_msat;
+				}
 			}
 		}
-
-		(htlc_outbound_count as u32, htlc_outbound_value_msat)
+		stats
 	}
 
 	/// Get the available (ie not including pending HTLCs) inbound and outbound balance in msat.
@@ -1879,11 +1918,11 @@ impl<Signer: Sign> Channel<Signer> {
 		(
 			cmp::max(self.channel_value_satoshis as i64 * 1000
 				- self.value_to_self_msat as i64
-				- self.get_inbound_pending_htlc_stats().1 as i64
+				- self.get_inbound_pending_htlc_stats().pending_htlcs_value_msat as i64
 				- Self::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis) as i64 * 1000,
 			0) as u64,
 			cmp::max(self.value_to_self_msat as i64
-				- self.get_outbound_pending_htlc_stats().1 as i64
+				- self.get_outbound_pending_htlc_stats().pending_htlcs_value_msat as i64
 				- self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000,
 			0) as u64
 		)
@@ -2095,12 +2134,13 @@ impl<Signer: Sign> Channel<Signer> {
 			return Err(ChannelError::Close(format!("Remote side tried to send less than our minimum HTLC value. Lower limit: ({}). Actual: ({})", self.holder_htlc_minimum_msat, msg.amount_msat)));
 		}
 
-		let (inbound_htlc_count, htlc_inbound_value_msat) = self.get_inbound_pending_htlc_stats();
-		if inbound_htlc_count + 1 > OUR_MAX_HTLCS as u32 {
+		let inbound_stats = self.get_inbound_pending_htlc_stats();
+		let outbound_stats = self.get_outbound_pending_htlc_stats();
+		if inbound_stats.pending_htlcs + 1 > OUR_MAX_HTLCS as u32 {
 			return Err(ChannelError::Close(format!("Remote tried to push more than our max accepted HTLCs ({})", OUR_MAX_HTLCS)));
 		}
 		let holder_max_htlc_value_in_flight_msat = Channel::<Signer>::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis);
-		if htlc_inbound_value_msat + msg.amount_msat > holder_max_htlc_value_in_flight_msat {
+		if inbound_stats.pending_htlcs_value_msat + msg.amount_msat > holder_max_htlc_value_in_flight_msat {
 			return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", holder_max_htlc_value_in_flight_msat)));
 		}
 		// Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet
@@ -2124,8 +2164,28 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 		}
 
+		let exposure_dust_limit_timeout_sats = (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
+		if msg.amount_msat / 1000 < exposure_dust_limit_timeout_sats {
+			let on_counterparty_tx_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat + msg.amount_msat;
+			if on_counterparty_tx_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() {
+				log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx",
+					on_counterparty_tx_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat());
+				pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x1000|7);
+			}
+		}
+
+		let exposure_dust_limit_success_sats = (self.get_dust_buffer_feerate() as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
+		if msg.amount_msat / 1000 < exposure_dust_limit_success_sats {
+			let on_holder_tx_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat + msg.amount_msat;
+			if on_holder_tx_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() {
+				log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
+					on_holder_tx_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat());
+				pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x1000|7);
+			}
+		}
+
 		let pending_value_to_self_msat =
-			self.value_to_self_msat + htlc_inbound_value_msat - removed_outbound_total_msat;
+			self.value_to_self_msat + inbound_stats.pending_htlcs_value_msat - removed_outbound_total_msat;
 		let pending_remote_value_msat =
 			self.channel_value_satoshis * 1000 - pending_value_to_self_msat;
 		if pending_remote_value_msat < msg.amount_msat {
@@ -3558,11 +3618,24 @@ impl<Signer: Sign> Channel<Signer> {
 		cmp::max(self.config.cltv_expiry_delta, MIN_CLTV_EXPIRY_DELTA)
 	}
 
+	pub fn get_max_dust_htlc_exposure_msat(&self) -> u64 {
+		self.config.max_dust_htlc_exposure_msat
+	}
+
 	#[cfg(test)]
 	pub fn get_feerate(&self) -> u32 {
 		self.feerate_per_kw
 	}
 
+	pub fn get_dust_buffer_feerate(&self) -> u32 {
+		// When calculating our exposure to dust HTLCs, we assume that the channel feerate
+		// may, at any point, increase by at least 10 sat/vB (i.e 2530 sat/kWU) or 25%,
+		// whichever is higher. This ensures that we aren't suddenly exposed to significantly
+		// more dust balance if the feerate increases when we have several HTLCs pending
+		// which are near the dust limit.
+		cmp::max(2530, self.feerate_per_kw * 1250 / 1000)
+	}
+
 	pub fn get_cur_holder_commitment_transaction_number(&self) -> u64 {
 		self.cur_holder_commitment_transaction_number + 1
 	}
@@ -4207,12 +4280,13 @@ impl<Signer: Sign> Channel<Signer> {
 			return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
 		}
 
-		let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
-		if outbound_htlc_count + 1 > self.counterparty_max_accepted_htlcs as u32 {
+		let inbound_stats = self.get_inbound_pending_htlc_stats();
+		let outbound_stats = self.get_outbound_pending_htlc_stats();
+		if outbound_stats.pending_htlcs + 1 > self.counterparty_max_accepted_htlcs as u32 {
 			return Err(ChannelError::Ignore(format!("Cannot push more than their max accepted HTLCs ({})", self.counterparty_max_accepted_htlcs)));
 		}
 		// Check their_max_htlc_value_in_flight_msat
-		if htlc_outbound_value_msat + amount_msat > self.counterparty_max_htlc_value_in_flight_msat {
+		if outbound_stats.pending_htlcs_value_msat + amount_msat > self.counterparty_max_htlc_value_in_flight_msat {
 			return Err(ChannelError::Ignore(format!("Cannot send value that would put us over the max HTLC value in flight our peer will accept ({})", self.counterparty_max_htlc_value_in_flight_msat)));
 		}
 
@@ -4227,7 +4301,25 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 		}
 
-		let pending_value_to_self_msat = self.value_to_self_msat - htlc_outbound_value_msat;
+		let exposure_dust_limit_success_sats = (self.get_dust_buffer_feerate() as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
+		if amount_msat / 1000 < exposure_dust_limit_success_sats {
+			let on_counterparty_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat + amount_msat;
+			if on_counterparty_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() {
+				return Err(ChannelError::Ignore(format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx",
+					on_counterparty_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat())));
+			}
+		}
+
+		let exposure_dust_limit_timeout_sats = (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
+		if amount_msat / 1000 <  exposure_dust_limit_timeout_sats {
+			let on_holder_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat + amount_msat;
+			if on_holder_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() {
+				return Err(ChannelError::Ignore(format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
+					on_holder_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat())));
+			}
+		}
+
+		let pending_value_to_self_msat = self.value_to_self_msat - outbound_stats.pending_htlcs_value_msat;
 		if pending_value_to_self_msat < amount_msat {
 			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)));
 		}

lightning/src/ln/functional_test_utils.rs

@@ -1394,6 +1394,9 @@ pub fn test_default_channel_config() -> UserConfig {
 	// When most of our tests were written, the default HTLC minimum was fixed at 1000.
 	// It now defaults to 1, so we simply set it to the expected value here.
 	default_config.own_channel_config.our_htlc_minimum_msat = 1000;
+	// When most of our tests were written, we didn't have the notion of a `max_dust_htlc_exposure_msat`,
+	// It now defaults to 5_000_000 msat; to avoid interfering with tests we bump it to 50_000_000 msat.
+	default_config.channel_options.max_dust_htlc_exposure_msat = 50_000_000;
 	default_config
 }
 

lightning/src/ln/functional_tests.rs

@@ -9764,3 +9764,116 @@ fn test_keysend_payments_to_private_node() {
 	pass_along_path(&nodes[0], &path, 10000, payment_hash, None, event, true, Some(test_preimage));
 	claim_payment(&nodes[0], &path, test_preimage);
 }
+
+fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, at_forward: bool, on_holder_tx: bool) {
+	// Test that we properly reject dust HTLC violating our `max_dust_htlc_exposure_msat` policy.
+	//
+	// At HTLC forward (`send_payment()`), if the sum of the trimmed-to-dust HTLC inbound and
+	// trimmed-to-dust HTLC outbound balance and this new payment as included on next counterparty
+	// commitment are above our `max_dust_htlc_exposure_msat`, we'll reject the update.
+	// At HTLC reception (`update_add_htlc()`), if the sum of the trimmed-to-dust HTLC inbound
+	// and trimmed-to-dust HTLC outbound balance and this new received HTLC as included on next
+	// counterparty commitment are above our `max_dust_htlc_exposure_msat`, we'll fail the update.
+	// Note, we return a `temporary_channel_failure` (0x1000 | 7), as the channel might be
+	// available again for HTLC processing once the dust bandwidth has cleared up.
+
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let mut config = test_default_channel_config();
+	config.channel_options.max_dust_htlc_exposure_msat = 5_000_000; // default setting value
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(config)]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 1_000_000, 500_000_000, 42, None).unwrap();
+	let mut open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	open_channel.max_htlc_value_in_flight_msat = 50_000_000;
+	open_channel.max_accepted_htlcs = 60;
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel);
+	let mut accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
+	if on_holder_tx {
+		accept_channel.dust_limit_satoshis = 660;
+	}
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_channel);
+
+	let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], 1_000_000, 42);
+
+	if on_holder_tx {
+		if let Some(mut chan) = nodes[1].node.channel_state.lock().unwrap().by_id.get_mut(&temporary_channel_id) {
+			chan.holder_dust_limit_satoshis = 660;
+		}
+	}
+
+	nodes[0].node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap();
+	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()));
+	check_added_monitors!(nodes[1], 1);
+
+	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
+	check_added_monitors!(nodes[0], 1);
+
+	let (funding_locked, _) = create_chan_between_nodes_with_value_confirm(&nodes[0], &nodes[1], &tx);
+	let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked);
+	update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update);
+
+	if on_holder_tx {
+		if dust_outbound_balance {
+			for i in 0..2 {
+				let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 2_300_000);
+				if let Err(_) = nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)) { panic!("Unexpected event at dust HTLC {}", i); }
+			}
+		} else {
+			for _ in 0..2 {
+				route_payment(&nodes[0], &[&nodes[1]], 2_300_000);
+			}
+		}
+	} else {
+		if dust_outbound_balance {
+			for i in 0..25 {
+				let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 200_000); // + 177_000 msat of HTLC-success tx at 253 sats/kWU
+				if let Err(_) = nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)) { panic!("Unexpected event at dust HTLC {}", i); }
+			}
+		} else {
+			for _ in 0..25 {
+				route_payment(&nodes[0], &[&nodes[1]], 200_000); // + 167_000 msat of HTLC-timeout tx at 253 sats/kWU
+			}
+		}
+	}
+
+	if at_forward {
+		let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], if on_holder_tx { 2_300_000 } else { 200_000 });
+		let mut config = UserConfig::default();
+		if on_holder_tx {
+			unwrap_send_err!(nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)), true, APIError::ChannelUnavailable { ref err }, assert_eq!(err, &format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx", 6_900_000, config.channel_options.max_dust_htlc_exposure_msat)));
+		} else {
+			unwrap_send_err!(nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)), true, APIError::ChannelUnavailable { ref err }, assert_eq!(err, &format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", 5_200_000, config.channel_options.max_dust_htlc_exposure_msat)));
+		}
+	} else {
+		let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1 ], if on_holder_tx { 2_300_000 } else { 200_000 });
+		nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
+		check_added_monitors!(nodes[0], 1);
+		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		let payment_event = SendEvent::from_event(events.remove(0));
+		nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
+		if on_holder_tx {
+			nodes[1].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx", 6_900_000, config.channel_options.max_dust_htlc_exposure_msat), 1);
+		} else {
+			nodes[1].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", 5_200_000, config.channel_options.max_dust_htlc_exposure_msat), 1);
+		}
+	}
+
+	let _ = nodes[1].node.get_and_clear_pending_msg_events();
+	let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();
+	added_monitors.clear();
+}
+
+#[test]
+fn test_max_dust_htlc_exposure() {
+	do_test_max_dust_htlc_exposure(true, true, true);
+	do_test_max_dust_htlc_exposure(false, true, true);
+	do_test_max_dust_htlc_exposure(false, false, true);
+	do_test_max_dust_htlc_exposure(false, false, false);
+	do_test_max_dust_htlc_exposure(true, true, false);
+	do_test_max_dust_htlc_exposure(true, false, false);
+	do_test_max_dust_htlc_exposure(true, false, true);
+	do_test_max_dust_htlc_exposure(false, true, false);
+}