From 2937000051e9a047702f89de7268f88c2503106e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 27 Mar 2024 02:14:50 +0000 Subject: [PATCH 1/6] Unify `get_{inbound,outbound}_pending_htlc_stats` In most cases we already call both in a pair, and in fact always consolidate some of the returned values across both accessors, so there's not much reason to have them be separate methods. Here we merge them. --- lightning/src/ln/channel.rs | 165 +++++++++++++++++------------------- 1 file changed, 76 insertions(+), 89 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 66301c95bd6..20bb15ce7e0 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -998,14 +998,16 @@ enum HTLCInitiator { RemoteOffered, } -/// An enum gathering stats on pending HTLCs, either inbound or outbound side. +/// Current counts of various HTLCs, useful for calculating current balances available exactly. struct HTLCStats { - pending_htlcs: u32, - pending_htlcs_value_msat: u64, + pending_inbound_htlcs: usize, + pending_outbound_htlcs: usize, + pending_inbound_htlcs_value_msat: u64, + pending_outbound_htlcs_value_msat: u64, on_counterparty_tx_dust_exposure_msat: u64, on_holder_tx_dust_exposure_msat: u64, - holding_cell_msat: u64, - on_holder_tx_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included + outbound_holding_cell_msat: u64, + on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included } /// An enum gathering stats on commitment transaction, either local or remote. @@ -2738,86 +2740,81 @@ impl ChannelContext where SP::Target: SignerProvider { self.counterparty_forwarding_info.clone() } - /// Returns a HTLCStats about inbound pending htlcs - fn get_inbound_pending_htlc_stats(&self, outbound_feerate_update: Option) -> HTLCStats { + /// Returns a HTLCStats about pending htlcs + fn get_pending_htlc_stats(&self, outbound_feerate_update: Option) -> HTLCStats { let context = self; - let mut stats = HTLCStats { - pending_htlcs: context.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, - holding_cell_msat: 0, - on_holder_tx_holding_cell_htlcs_count: 0, - }; + let uses_0_htlc_fee_anchors = self.get_channel_type().supports_anchors_zero_fee_htlc_tx(); - let (htlc_timeout_dust_limit, htlc_success_dust_limit) = if context.get_channel_type().supports_anchors_zero_fee_htlc_tx() { + let (htlc_timeout_dust_limit, htlc_success_dust_limit) = if uses_0_htlc_fee_anchors { (0, 0) } else { let dust_buffer_feerate = context.get_dust_buffer_feerate(outbound_feerate_update) as u64; (dust_buffer_feerate * htlc_timeout_tx_weight(context.get_channel_type()) / 1000, dust_buffer_feerate * htlc_success_tx_weight(context.get_channel_type()) / 1000) }; + + let mut on_holder_tx_dust_exposure_msat = 0; + let mut on_counterparty_tx_dust_exposure_msat = 0; + + let mut pending_inbound_htlcs_value_msat = 0; + { let counterparty_dust_limit_timeout_sat = htlc_timeout_dust_limit + context.counterparty_dust_limit_satoshis; let holder_dust_limit_success_sat = htlc_success_dust_limit + context.holder_dust_limit_satoshis; for ref htlc in context.pending_inbound_htlcs.iter() { - stats.pending_htlcs_value_msat += htlc.amount_msat; + pending_inbound_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; + 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; + on_holder_tx_dust_exposure_msat += htlc.amount_msat; } } - stats - } - - /// Returns a HTLCStats about pending outbound htlcs, *including* pending adds in our holding cell. - fn get_outbound_pending_htlc_stats(&self, outbound_feerate_update: Option) -> HTLCStats { - let context = self; - let mut stats = HTLCStats { - pending_htlcs: context.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, - holding_cell_msat: 0, - on_holder_tx_holding_cell_htlcs_count: 0, - }; + } - let (htlc_timeout_dust_limit, htlc_success_dust_limit) = if context.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - (0, 0) - } else { - let dust_buffer_feerate = context.get_dust_buffer_feerate(outbound_feerate_update) as u64; - (dust_buffer_feerate * htlc_timeout_tx_weight(context.get_channel_type()) / 1000, - dust_buffer_feerate * htlc_success_tx_weight(context.get_channel_type()) / 1000) - }; + let mut pending_outbound_htlcs_value_msat = 0; + let mut outbound_holding_cell_msat = 0; + let mut on_holder_tx_outbound_holding_cell_htlcs_count = 0; + let mut pending_outbound_htlcs = self.pending_outbound_htlcs.len(); + { let counterparty_dust_limit_success_sat = htlc_success_dust_limit + context.counterparty_dust_limit_satoshis; let holder_dust_limit_timeout_sat = htlc_timeout_dust_limit + context.holder_dust_limit_satoshis; for ref htlc in context.pending_outbound_htlcs.iter() { - stats.pending_htlcs_value_msat += htlc.amount_msat; + pending_outbound_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; + 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; + on_holder_tx_dust_exposure_msat += htlc.amount_msat; } } for update in context.holding_cell_htlc_updates.iter() { if let &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } = update { - stats.pending_htlcs += 1; - stats.pending_htlcs_value_msat += amount_msat; - stats.holding_cell_msat += amount_msat; + pending_outbound_htlcs += 1; + pending_outbound_htlcs_value_msat += amount_msat; + outbound_holding_cell_msat += amount_msat; if *amount_msat / 1000 < counterparty_dust_limit_success_sat { - stats.on_counterparty_tx_dust_exposure_msat += amount_msat; + 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; + on_holder_tx_dust_exposure_msat += amount_msat; } else { - stats.on_holder_tx_holding_cell_htlcs_count += 1; + on_holder_tx_outbound_holding_cell_htlcs_count += 1; } } } - stats + } + + HTLCStats { + pending_inbound_htlcs: self.pending_inbound_htlcs.len(), + pending_outbound_htlcs, + pending_inbound_htlcs_value_msat, + pending_outbound_htlcs_value_msat, + on_counterparty_tx_dust_exposure_msat, + on_holder_tx_dust_exposure_msat, + outbound_holding_cell_msat, + on_holder_tx_outbound_holding_cell_htlcs_count, + } } /// Returns information on all pending inbound HTLCs. @@ -2923,8 +2920,7 @@ impl ChannelContext where SP::Target: SignerProvider { { let context = &self; // Note that we have to handle overflow due to the above case. - let inbound_stats = context.get_inbound_pending_htlc_stats(None); - let outbound_stats = context.get_outbound_pending_htlc_stats(None); + let htlc_stats = context.get_pending_htlc_stats(None); let mut balance_msat = context.value_to_self_msat; for ref htlc in context.pending_inbound_htlcs.iter() { @@ -2932,10 +2928,10 @@ impl ChannelContext where SP::Target: SignerProvider { balance_msat += htlc.amount_msat; } } - balance_msat -= outbound_stats.pending_htlcs_value_msat; + balance_msat -= htlc_stats.pending_outbound_htlcs_value_msat; let outbound_capacity_msat = context.value_to_self_msat - .saturating_sub(outbound_stats.pending_htlcs_value_msat) + .saturating_sub(htlc_stats.pending_outbound_htlcs_value_msat) .saturating_sub( context.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) * 1000); @@ -2995,7 +2991,7 @@ impl ChannelContext where SP::Target: SignerProvider { let holder_selected_chan_reserve_msat = context.holder_selected_channel_reserve_satoshis * 1000; let remote_balance_msat = (context.channel_value_satoshis * 1000 - context.value_to_self_msat) - .saturating_sub(inbound_stats.pending_htlcs_value_msat); + .saturating_sub(htlc_stats.pending_inbound_htlcs_value_msat); if remote_balance_msat < max_reserved_commit_tx_fee_msat + holder_selected_chan_reserve_msat + anchor_outputs_value_msat { // If another HTLC's fee would reduce the remote's balance below the reserve limit @@ -3021,18 +3017,16 @@ impl ChannelContext where SP::Target: SignerProvider { (context.counterparty_dust_limit_satoshis + dust_buffer_feerate * htlc_success_tx_weight(context.get_channel_type()) / 1000, context.holder_dust_limit_satoshis + dust_buffer_feerate * htlc_timeout_tx_weight(context.get_channel_type()) / 1000) }; - let on_counterparty_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat; - if on_counterparty_dust_htlc_exposure_msat as i64 + htlc_success_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat.try_into().unwrap_or(i64::max_value()) { + if htlc_stats.on_counterparty_tx_dust_exposure_msat as i64 + htlc_success_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat.try_into().unwrap_or(i64::max_value()) { remaining_msat_below_dust_exposure_limit = - Some(max_dust_htlc_exposure_msat.saturating_sub(on_counterparty_dust_htlc_exposure_msat)); + Some(max_dust_htlc_exposure_msat.saturating_sub(htlc_stats.on_counterparty_tx_dust_exposure_msat)); dust_exposure_dust_limit_msat = cmp::max(dust_exposure_dust_limit_msat, htlc_success_dust_limit * 1000); } - let on_holder_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat; - if on_holder_dust_htlc_exposure_msat as i64 + htlc_timeout_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat.try_into().unwrap_or(i64::max_value()) { + if htlc_stats.on_holder_tx_dust_exposure_msat as i64 + htlc_timeout_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat.try_into().unwrap_or(i64::max_value()) { remaining_msat_below_dust_exposure_limit = Some(cmp::min( remaining_msat_below_dust_exposure_limit.unwrap_or(u64::max_value()), - max_dust_htlc_exposure_msat.saturating_sub(on_holder_dust_htlc_exposure_msat))); + max_dust_htlc_exposure_msat.saturating_sub(htlc_stats.on_holder_tx_dust_exposure_msat))); dust_exposure_dust_limit_msat = cmp::max(dust_exposure_dust_limit_msat, htlc_timeout_dust_limit * 1000); } @@ -3045,16 +3039,16 @@ impl ChannelContext where SP::Target: SignerProvider { } available_capacity_msat = cmp::min(available_capacity_msat, - context.counterparty_max_htlc_value_in_flight_msat - outbound_stats.pending_htlcs_value_msat); + context.counterparty_max_htlc_value_in_flight_msat - htlc_stats.pending_outbound_htlcs_value_msat); - if outbound_stats.pending_htlcs + 1 > context.counterparty_max_accepted_htlcs as u32 { + if htlc_stats.pending_outbound_htlcs + 1 > context.counterparty_max_accepted_htlcs as usize { available_capacity_msat = 0; } AvailableBalances { inbound_capacity_msat: cmp::max(context.channel_value_satoshis as i64 * 1000 - context.value_to_self_msat as i64 - - context.get_inbound_pending_htlc_stats(None).pending_htlcs_value_msat as i64 + - htlc_stats.pending_inbound_htlcs_value_msat as i64 - context.holder_selected_channel_reserve_satoshis as i64 * 1000, 0) as u64, outbound_capacity_msat, @@ -4143,11 +4137,11 @@ impl Channel where return Err(ChannelError::Close(format!("Remote side tried to send less than our minimum HTLC value. Lower limit: ({}). Actual: ({})", self.context.holder_htlc_minimum_msat, msg.amount_msat))); } - let inbound_stats = self.context.get_inbound_pending_htlc_stats(None); - if inbound_stats.pending_htlcs + 1 > self.context.holder_max_accepted_htlcs as u32 { + let htlc_stats = self.context.get_pending_htlc_stats(None); + if htlc_stats.pending_inbound_htlcs + 1 > self.context.holder_max_accepted_htlcs as usize { return Err(ChannelError::Close(format!("Remote tried to push more than our max accepted HTLCs ({})", self.context.holder_max_accepted_htlcs))); } - if inbound_stats.pending_htlcs_value_msat + msg.amount_msat > self.context.holder_max_htlc_value_in_flight_msat { + if htlc_stats.pending_inbound_htlcs_value_msat + msg.amount_msat > self.context.holder_max_htlc_value_in_flight_msat { return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.context.holder_max_htlc_value_in_flight_msat))); } @@ -4173,7 +4167,7 @@ impl Channel where } let pending_value_to_self_msat = - self.context.value_to_self_msat + inbound_stats.pending_htlcs_value_msat - removed_outbound_total_msat; + self.context.value_to_self_msat + htlc_stats.pending_inbound_htlcs_value_msat - removed_outbound_total_msat; let pending_remote_value_msat = self.context.channel_value_satoshis * 1000 - pending_value_to_self_msat; if pending_remote_value_msat < msg.amount_msat { @@ -4995,12 +4989,11 @@ impl Channel where } // Before proposing a feerate update, check that we can actually afford the new fee. - let inbound_stats = self.context.get_inbound_pending_htlc_stats(Some(feerate_per_kw)); - let outbound_stats = self.context.get_outbound_pending_htlc_stats(Some(feerate_per_kw)); + let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw)); let keys = self.context.build_holder_transaction_keys(self.context.cur_holder_commitment_transaction_number); let commitment_stats = self.context.build_commitment_transaction(self.context.cur_holder_commitment_transaction_number, &keys, true, true, logger); - let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + outbound_stats.on_holder_tx_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; - let holder_balance_msat = commitment_stats.local_balance_msat - outbound_stats.holding_cell_msat; + let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; + let holder_balance_msat = commitment_stats.local_balance_msat - htlc_stats.outbound_holding_cell_msat; if holder_balance_msat < buffer_fee_msat + self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { //TODO: auto-close after a number of failures? log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw); @@ -5008,14 +5001,12 @@ impl Channel where } // Note, we evaluate pending htlc "preemptive" trimmed-to-dust threshold at the proposed `feerate_per_kw`. - let holder_tx_dust_exposure = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat; - let counterparty_tx_dust_exposure = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat; let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(fee_estimator); - if holder_tx_dust_exposure > max_dust_htlc_exposure_msat { + if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { log_debug!(logger, "Cannot afford to send new feerate at {} without infringing max dust htlc exposure", feerate_per_kw); return None; } - if counterparty_tx_dust_exposure > max_dust_htlc_exposure_msat { + if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { log_debug!(logger, "Cannot afford to send new feerate at {} without infringing max dust htlc exposure", feerate_per_kw); return None; } @@ -5249,18 +5240,15 @@ impl Channel where self.context.update_time_counter += 1; // Check that we won't be pushed over our dust exposure limit by the feerate increase. if !self.context.channel_type.supports_anchors_zero_fee_htlc_tx() { - let inbound_stats = self.context.get_inbound_pending_htlc_stats(None); - let outbound_stats = self.context.get_outbound_pending_htlc_stats(None); - let holder_tx_dust_exposure = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat; - let counterparty_tx_dust_exposure = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat; + let htlc_stats = self.context.get_pending_htlc_stats(None); let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(fee_estimator); - if holder_tx_dust_exposure > max_dust_htlc_exposure_msat { + if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our own transactions (totaling {} msat)", - msg.feerate_per_kw, holder_tx_dust_exposure))); + msg.feerate_per_kw, htlc_stats.on_holder_tx_dust_exposure_msat))); } - if counterparty_tx_dust_exposure > max_dust_htlc_exposure_msat { + if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our counterparty's transactions (totaling {} msat)", - msg.feerate_per_kw, counterparty_tx_dust_exposure))); + msg.feerate_per_kw, htlc_stats.on_counterparty_tx_dust_exposure_msat))); } } Ok(()) @@ -6105,8 +6093,7 @@ impl Channel where return Err(("Shutdown was already sent", 0x4000|8)) } - let inbound_stats = self.context.get_inbound_pending_htlc_stats(None); - let outbound_stats = self.context.get_outbound_pending_htlc_stats(None); + let htlc_stats = self.context.get_pending_htlc_stats(None); let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(fee_estimator); let (htlc_timeout_dust_limit, htlc_success_dust_limit) = if self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() { (0, 0) @@ -6117,7 +6104,7 @@ impl Channel where }; let exposure_dust_limit_timeout_sats = htlc_timeout_dust_limit + self.context.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; + let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat + msg.amount_msat; if on_counterparty_tx_dust_htlc_exposure_msat > 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, max_dust_htlc_exposure_msat); @@ -6127,7 +6114,7 @@ impl Channel where let exposure_dust_limit_success_sats = htlc_success_dust_limit + self.context.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; + let on_holder_tx_dust_htlc_exposure_msat = htlc_stats.on_holder_tx_dust_exposure_msat + msg.amount_msat; if on_holder_tx_dust_htlc_exposure_msat > 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, max_dust_htlc_exposure_msat); @@ -6151,7 +6138,7 @@ impl Channel where } let pending_value_to_self_msat = - self.context.value_to_self_msat + inbound_stats.pending_htlcs_value_msat - removed_outbound_total_msat; + self.context.value_to_self_msat + htlc_stats.pending_inbound_htlcs_value_msat - removed_outbound_total_msat; let pending_remote_value_msat = self.context.channel_value_satoshis * 1000 - pending_value_to_self_msat; From 0e831b4bd0615a980e024fd0c63c5a3752fc718c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 27 Mar 2024 02:21:40 +0000 Subject: [PATCH 2/6] Correct indentation in `get_pending_htlc_stats` The previous commit left indentation in `get_pending_htlc_stats` deliberately wrong, to ease reviewability. This commit fixes the indentation. --- lightning/src/ln/channel.rs | 66 ++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 20bb15ce7e0..a56bb3dc9e6 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2758,52 +2758,52 @@ impl ChannelContext where SP::Target: SignerProvider { let mut pending_inbound_htlcs_value_msat = 0; { - let counterparty_dust_limit_timeout_sat = htlc_timeout_dust_limit + context.counterparty_dust_limit_satoshis; - let holder_dust_limit_success_sat = htlc_success_dust_limit + context.holder_dust_limit_satoshis; - for ref htlc in context.pending_inbound_htlcs.iter() { - pending_inbound_htlcs_value_msat += htlc.amount_msat; - if htlc.amount_msat / 1000 < counterparty_dust_limit_timeout_sat { - on_counterparty_tx_dust_exposure_msat += htlc.amount_msat; - } - if htlc.amount_msat / 1000 < holder_dust_limit_success_sat { - on_holder_tx_dust_exposure_msat += htlc.amount_msat; + let counterparty_dust_limit_timeout_sat = htlc_timeout_dust_limit + context.counterparty_dust_limit_satoshis; + let holder_dust_limit_success_sat = htlc_success_dust_limit + context.holder_dust_limit_satoshis; + for ref htlc in context.pending_inbound_htlcs.iter() { + pending_inbound_htlcs_value_msat += htlc.amount_msat; + if htlc.amount_msat / 1000 < counterparty_dust_limit_timeout_sat { + on_counterparty_tx_dust_exposure_msat += htlc.amount_msat; + } + if htlc.amount_msat / 1000 < holder_dust_limit_success_sat { + on_holder_tx_dust_exposure_msat += htlc.amount_msat; + } } } - } let mut pending_outbound_htlcs_value_msat = 0; let mut outbound_holding_cell_msat = 0; let mut on_holder_tx_outbound_holding_cell_htlcs_count = 0; let mut pending_outbound_htlcs = self.pending_outbound_htlcs.len(); { - let counterparty_dust_limit_success_sat = htlc_success_dust_limit + context.counterparty_dust_limit_satoshis; - let holder_dust_limit_timeout_sat = htlc_timeout_dust_limit + context.holder_dust_limit_satoshis; - for ref htlc in context.pending_outbound_htlcs.iter() { - pending_outbound_htlcs_value_msat += htlc.amount_msat; - if htlc.amount_msat / 1000 < counterparty_dust_limit_success_sat { - on_counterparty_tx_dust_exposure_msat += htlc.amount_msat; - } - if htlc.amount_msat / 1000 < holder_dust_limit_timeout_sat { - on_holder_tx_dust_exposure_msat += htlc.amount_msat; + let counterparty_dust_limit_success_sat = htlc_success_dust_limit + context.counterparty_dust_limit_satoshis; + let holder_dust_limit_timeout_sat = htlc_timeout_dust_limit + context.holder_dust_limit_satoshis; + for ref htlc in context.pending_outbound_htlcs.iter() { + pending_outbound_htlcs_value_msat += htlc.amount_msat; + if htlc.amount_msat / 1000 < counterparty_dust_limit_success_sat { + on_counterparty_tx_dust_exposure_msat += htlc.amount_msat; + } + if htlc.amount_msat / 1000 < holder_dust_limit_timeout_sat { + on_holder_tx_dust_exposure_msat += htlc.amount_msat; + } } - } - for update in context.holding_cell_htlc_updates.iter() { - if let &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } = update { - pending_outbound_htlcs += 1; - pending_outbound_htlcs_value_msat += amount_msat; - outbound_holding_cell_msat += amount_msat; - if *amount_msat / 1000 < counterparty_dust_limit_success_sat { - on_counterparty_tx_dust_exposure_msat += amount_msat; - } - if *amount_msat / 1000 < holder_dust_limit_timeout_sat { - on_holder_tx_dust_exposure_msat += amount_msat; - } else { - on_holder_tx_outbound_holding_cell_htlcs_count += 1; + for update in context.holding_cell_htlc_updates.iter() { + if let &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } = update { + pending_outbound_htlcs += 1; + pending_outbound_htlcs_value_msat += amount_msat; + outbound_holding_cell_msat += amount_msat; + if *amount_msat / 1000 < counterparty_dust_limit_success_sat { + on_counterparty_tx_dust_exposure_msat += amount_msat; + } + if *amount_msat / 1000 < holder_dust_limit_timeout_sat { + on_holder_tx_dust_exposure_msat += amount_msat; + } else { + on_holder_tx_outbound_holding_cell_htlcs_count += 1; + } } } } - } HTLCStats { pending_inbound_htlcs: self.pending_inbound_htlcs.len(), From 11c3d7001ba54e1c8641566ca05dd58571f29c55 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 1 May 2024 21:56:04 +0000 Subject: [PATCH 3/6] Swap `UserConfig::default()` for `test_default_channel_config` As LDK changes, `UserConfig::default()` may imply marginally different behavior, whereas `test_default_channel_config` is intended to tweak defaults to provide a stable behavior for test contexts. This commit changes a few uses of `UserConfig::default()` to `test_default_channel_config` in cases that will fail over the coming commits due to dust changes. --- lightning/src/ln/functional_tests.rs | 8 ++++---- lightning/src/ln/monitor_tests.rs | 5 ++--- lightning/src/ln/onion_route_tests.rs | 3 ++- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 465d6288d9d..34ca9a7101f 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2433,11 +2433,11 @@ fn channel_monitor_network_test() { #[test] fn test_justice_tx_htlc_timeout() { // Test justice txn built on revoked HTLC-Timeout tx, against both sides - let mut alice_config = UserConfig::default(); + let mut alice_config = test_default_channel_config(); alice_config.channel_handshake_config.announced_channel = true; alice_config.channel_handshake_limits.force_announced_channel_preference = false; alice_config.channel_handshake_config.our_to_self_delay = 6 * 24 * 5; - let mut bob_config = UserConfig::default(); + let mut bob_config = test_default_channel_config(); bob_config.channel_handshake_config.announced_channel = true; bob_config.channel_handshake_limits.force_announced_channel_preference = false; bob_config.channel_handshake_config.our_to_self_delay = 6 * 24 * 3; @@ -2496,11 +2496,11 @@ fn test_justice_tx_htlc_timeout() { #[test] fn test_justice_tx_htlc_success() { // Test justice txn built on revoked HTLC-Success tx, against both sides - let mut alice_config = UserConfig::default(); + let mut alice_config = test_default_channel_config(); alice_config.channel_handshake_config.announced_channel = true; alice_config.channel_handshake_limits.force_announced_channel_preference = false; alice_config.channel_handshake_config.our_to_self_delay = 6 * 24 * 5; - let mut bob_config = UserConfig::default(); + let mut bob_config = test_default_channel_config(); bob_config.channel_handshake_config.announced_channel = true; bob_config.channel_handshake_limits.force_announced_channel_preference = false; bob_config.channel_handshake_config.our_to_self_delay = 6 * 24 * 3; diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index d5f28c23b4e..83d22c4deea 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -18,7 +18,6 @@ use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureR use crate::ln::{channel, ChannelId}; use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, PaymentId, RecipientOnionFields}; use crate::ln::msgs::ChannelMessageHandler; -use crate::util::config::UserConfig; use crate::crypto::utils::sign; use crate::util::ser::Writeable; use crate::util::scid_utils::block_from_scid; @@ -2249,7 +2248,7 @@ fn test_yield_anchors_events() { // emitted by LDK, such that the consumer can attach fees to the zero fee HTLC transactions. let mut chanmon_cfgs = create_chanmon_cfgs(2); let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); - let mut anchors_config = UserConfig::default(); + let mut anchors_config = test_default_channel_config(); anchors_config.channel_handshake_config.announced_channel = true; anchors_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; anchors_config.manually_accept_inbound_channels = true; @@ -2400,7 +2399,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { let bob_persister; let bob_chain_monitor; - let mut anchors_config = UserConfig::default(); + let mut anchors_config = test_default_channel_config(); anchors_config.channel_handshake_config.announced_channel = true; anchors_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; anchors_config.manually_accept_inbound_channels = true; diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index aeb175bc626..8b21139667d 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -21,6 +21,7 @@ use crate::ln::onion_utils; use crate::routing::gossip::{NetworkUpdate, RoutingFees}; use crate::routing::router::{get_route, PaymentParameters, Route, RouteParameters, RouteHint, RouteHintHop}; use crate::ln::features::{InitFeatures, Bolt11InvoiceFeatures}; +use crate::ln::functional_test_utils::test_default_channel_config; use crate::ln::msgs; use crate::ln::msgs::{ChannelMessageHandler, ChannelUpdate, OutboundTrampolinePayload}; use crate::ln::wire::Encode; @@ -328,7 +329,7 @@ fn test_onion_failure() { // to 2000, which is above the default value of 1000 set in create_node_chanmgrs. // This exposed a previous bug because we were using the wrong value all the way down in // Channel::get_counterparty_htlc_minimum_msat(). - let mut node_2_cfg: UserConfig = Default::default(); + let mut node_2_cfg: UserConfig = test_default_channel_config(); node_2_cfg.channel_handshake_config.our_htlc_minimum_msat = 2000; node_2_cfg.channel_handshake_config.announced_channel = true; node_2_cfg.channel_handshake_limits.force_announced_channel_preference = false; From 51bf78d604b28fe189171e1b976fe87cbb2614b7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 30 Apr 2024 20:42:36 +0000 Subject: [PATCH 4/6] Include excess commitment transaction fees in dust exposure Transaction fees on counterparty commitment transactions are ultimately not our money and thus are really "dust" from our PoV - they're funds that may be ours during off-chain updates but are not ours once we go on-chain. Thus, here, we count any such fees in excess of our own fee estimates towards dust exposure. We don't bother to make an inbound/outbound channel distinction here as in most cases users will use `MaxDustExposure::FeeRateMultiplier` which will scale with the fee we set on outbound channels anyway. Note that this also enables the dust exposure checks on anchor channels during feerate updates. We'd previously elided these as increases in the channel feerates do not change the HTLC dust exposure, but now do for the fee dust exposure. --- lightning/src/ln/channel.rs | 140 ++++++++++++++++++++------- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/functional_tests.rs | 84 +++++++++++----- 3 files changed, 170 insertions(+), 56 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index a56bb3dc9e6..e82c66dc458 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2339,15 +2339,16 @@ impl ChannelContext where SP::Target: SignerProvider { cmp::max(self.config.options.cltv_expiry_delta, MIN_CLTV_EXPIRY_DELTA) } - pub fn get_max_dust_htlc_exposure_msat(&self, - fee_estimator: &LowerBoundedFeeEstimator) -> u64 - where F::Target: FeeEstimator - { + fn get_dust_exposure_limiting_feerate(&self, + fee_estimator: &LowerBoundedFeeEstimator, + ) -> u32 where F::Target: FeeEstimator { + fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::OnChainSweep) + } + + pub fn get_max_dust_htlc_exposure_msat(&self, limiting_feerate_sat_per_kw: u32) -> u64 { match self.config.options.max_dust_htlc_exposure { MaxDustHTLCExposure::FeeRateMultiplier(multiplier) => { - let feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight( - ConfirmationTarget::OnChainSweep) as u64; - feerate_per_kw.saturating_mul(multiplier) + (limiting_feerate_sat_per_kw as u64).saturating_mul(multiplier) }, MaxDustHTLCExposure::FixedLimitMsat(limit) => limit, } @@ -2741,22 +2742,26 @@ impl ChannelContext where SP::Target: SignerProvider { } /// Returns a HTLCStats about pending htlcs - fn get_pending_htlc_stats(&self, outbound_feerate_update: Option) -> HTLCStats { + fn get_pending_htlc_stats(&self, outbound_feerate_update: Option, dust_exposure_limiting_feerate: u32) -> HTLCStats { let context = self; let uses_0_htlc_fee_anchors = self.get_channel_type().supports_anchors_zero_fee_htlc_tx(); + let dust_buffer_feerate = context.get_dust_buffer_feerate(outbound_feerate_update); let (htlc_timeout_dust_limit, htlc_success_dust_limit) = if uses_0_htlc_fee_anchors { (0, 0) } else { - let dust_buffer_feerate = context.get_dust_buffer_feerate(outbound_feerate_update) as u64; - (dust_buffer_feerate * htlc_timeout_tx_weight(context.get_channel_type()) / 1000, - dust_buffer_feerate * htlc_success_tx_weight(context.get_channel_type()) / 1000) + (dust_buffer_feerate as u64 * htlc_timeout_tx_weight(context.get_channel_type()) / 1000, + dust_buffer_feerate as u64 * htlc_success_tx_weight(context.get_channel_type()) / 1000) }; let mut on_holder_tx_dust_exposure_msat = 0; let mut on_counterparty_tx_dust_exposure_msat = 0; + let mut on_counterparty_tx_offered_nondust_htlcs = 0; + let mut on_counterparty_tx_accepted_nondust_htlcs = 0; + let mut pending_inbound_htlcs_value_msat = 0; + { let counterparty_dust_limit_timeout_sat = htlc_timeout_dust_limit + context.counterparty_dust_limit_satoshis; let holder_dust_limit_success_sat = htlc_success_dust_limit + context.holder_dust_limit_satoshis; @@ -2764,6 +2769,8 @@ impl ChannelContext where SP::Target: SignerProvider { pending_inbound_htlcs_value_msat += htlc.amount_msat; if htlc.amount_msat / 1000 < counterparty_dust_limit_timeout_sat { on_counterparty_tx_dust_exposure_msat += htlc.amount_msat; + } else { + on_counterparty_tx_offered_nondust_htlcs += 1; } if htlc.amount_msat / 1000 < holder_dust_limit_success_sat { on_holder_tx_dust_exposure_msat += htlc.amount_msat; @@ -2782,6 +2789,8 @@ impl ChannelContext where SP::Target: SignerProvider { pending_outbound_htlcs_value_msat += htlc.amount_msat; if htlc.amount_msat / 1000 < counterparty_dust_limit_success_sat { on_counterparty_tx_dust_exposure_msat += htlc.amount_msat; + } else { + on_counterparty_tx_accepted_nondust_htlcs += 1; } if htlc.amount_msat / 1000 < holder_dust_limit_timeout_sat { on_holder_tx_dust_exposure_msat += htlc.amount_msat; @@ -2795,6 +2804,8 @@ impl ChannelContext where SP::Target: SignerProvider { outbound_holding_cell_msat += amount_msat; if *amount_msat / 1000 < counterparty_dust_limit_success_sat { on_counterparty_tx_dust_exposure_msat += amount_msat; + } else { + on_counterparty_tx_accepted_nondust_htlcs += 1; } if *amount_msat / 1000 < holder_dust_limit_timeout_sat { on_holder_tx_dust_exposure_msat += amount_msat; @@ -2805,6 +2816,26 @@ impl ChannelContext where SP::Target: SignerProvider { } } + // Include any mining "excess" fees in the dust calculation + let excess_feerate_opt = outbound_feerate_update + .or(self.pending_update_fee.map(|(fee, _)| fee)) + .unwrap_or(self.feerate_per_kw) + .checked_sub(dust_exposure_limiting_feerate); + if let Some(excess_feerate) = excess_feerate_opt { + let on_counterparty_tx_nondust_htlcs = + on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs; + on_counterparty_tx_dust_exposure_msat += + commit_tx_fee_msat(excess_feerate, on_counterparty_tx_nondust_htlcs, &self.channel_type); + if !self.channel_type.supports_anchors_zero_fee_htlc_tx() { + on_counterparty_tx_dust_exposure_msat += + on_counterparty_tx_accepted_nondust_htlcs as u64 * htlc_success_tx_weight(&self.channel_type) + * excess_feerate as u64 / 1000; + on_counterparty_tx_dust_exposure_msat += + on_counterparty_tx_offered_nondust_htlcs as u64 * htlc_timeout_tx_weight(&self.channel_type) + * excess_feerate as u64 / 1000; + } + } + HTLCStats { pending_inbound_htlcs: self.pending_inbound_htlcs.len(), pending_outbound_htlcs, @@ -2919,8 +2950,11 @@ impl ChannelContext where SP::Target: SignerProvider { where F::Target: FeeEstimator { let context = &self; - // Note that we have to handle overflow due to the above case. - let htlc_stats = context.get_pending_htlc_stats(None); + // Note that we have to handle overflow due to the case mentioned in the docs in general + // here. + + let dust_exposure_limiting_feerate = self.get_dust_exposure_limiting_feerate(&fee_estimator); + let htlc_stats = context.get_pending_htlc_stats(None, dust_exposure_limiting_feerate); let mut balance_msat = context.value_to_self_msat; for ref htlc in context.pending_inbound_htlcs.iter() { @@ -3008,7 +3042,7 @@ impl ChannelContext where SP::Target: SignerProvider { // send above the dust limit (as the router can always overpay to meet the dust limit). let mut remaining_msat_below_dust_exposure_limit = None; let mut dust_exposure_dust_limit_msat = 0; - let max_dust_htlc_exposure_msat = context.get_max_dust_htlc_exposure_msat(fee_estimator); + let max_dust_htlc_exposure_msat = context.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); let (htlc_success_dust_limit, htlc_timeout_dust_limit) = if context.get_channel_type().supports_anchors_zero_fee_htlc_tx() { (context.counterparty_dust_limit_satoshis, context.holder_dust_limit_satoshis) @@ -3017,7 +3051,23 @@ impl ChannelContext where SP::Target: SignerProvider { (context.counterparty_dust_limit_satoshis + dust_buffer_feerate * htlc_success_tx_weight(context.get_channel_type()) / 1000, context.holder_dust_limit_satoshis + dust_buffer_feerate * htlc_timeout_tx_weight(context.get_channel_type()) / 1000) }; - if htlc_stats.on_counterparty_tx_dust_exposure_msat as i64 + htlc_success_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat.try_into().unwrap_or(i64::max_value()) { + + let excess_feerate_opt = self.feerate_per_kw.checked_sub(dust_exposure_limiting_feerate); + if let Some(excess_feerate) = excess_feerate_opt { + let htlc_dust_exposure_msat = + per_outbound_htlc_counterparty_commit_tx_fee_msat(excess_feerate, &context.channel_type); + let nondust_htlc_counterparty_tx_dust_exposure = + htlc_stats.on_counterparty_tx_dust_exposure_msat.saturating_add(htlc_dust_exposure_msat); + if nondust_htlc_counterparty_tx_dust_exposure > max_dust_htlc_exposure_msat { + // If adding an extra HTLC would put us over the dust limit in total fees, we cannot + // send any non-dust HTLCs. + available_capacity_msat = cmp::min(available_capacity_msat, htlc_success_dust_limit * 1000); + } + } + + if htlc_stats.on_counterparty_tx_dust_exposure_msat.saturating_add(htlc_success_dust_limit * 1000) > max_dust_htlc_exposure_msat.saturating_add(1) { + // Note that we don't use the `counterparty_tx_dust_exposure` (with + // `htlc_dust_exposure_msat`) here as it only applies to non-dust HTLCs. remaining_msat_below_dust_exposure_limit = Some(max_dust_htlc_exposure_msat.saturating_sub(htlc_stats.on_counterparty_tx_dust_exposure_msat)); dust_exposure_dust_limit_msat = cmp::max(dust_exposure_dust_limit_msat, htlc_success_dust_limit * 1000); @@ -3517,6 +3567,17 @@ pub(crate) fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize, channel_ (commitment_tx_base_weight(channel_type_features) + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate_per_kw as u64 / 1000 * 1000 } +pub(crate) fn per_outbound_htlc_counterparty_commit_tx_fee_msat(feerate_per_kw: u32, channel_type_features: &ChannelTypeFeatures) -> u64 { + // Note that we need to divide before multiplying to round properly, + // since the lowest denomination of bitcoin on-chain is the satoshi. + let commitment_tx_fee = COMMITMENT_TX_WEIGHT_PER_HTLC * feerate_per_kw as u64 / 1000 * 1000; + if channel_type_features.supports_anchors_zero_fee_htlc_tx() { + commitment_tx_fee + htlc_success_tx_weight(channel_type_features) * feerate_per_kw as u64 / 1000 + } else { + commitment_tx_fee + } +} + /// Context for dual-funded channels. #[cfg(any(dual_funding, splicing))] pub(super) struct DualFundingChannelContext { @@ -4114,9 +4175,10 @@ impl Channel where Ok(self.get_announcement_sigs(node_signer, chain_hash, user_config, best_block.height, logger)) } - pub fn update_add_htlc( + pub fn update_add_htlc( &mut self, msg: &msgs::UpdateAddHTLC, pending_forward_status: PendingHTLCStatus, - ) -> Result<(), ChannelError> { + fee_estimator: &LowerBoundedFeeEstimator, + ) -> Result<(), ChannelError> where F::Target: FeeEstimator { if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { return Err(ChannelError::Close("Got add HTLC message when channel was not in an operational state".to_owned())); } @@ -4137,7 +4199,8 @@ impl Channel where return Err(ChannelError::Close(format!("Remote side tried to send less than our minimum HTLC value. Lower limit: ({}). Actual: ({})", self.context.holder_htlc_minimum_msat, msg.amount_msat))); } - let htlc_stats = self.context.get_pending_htlc_stats(None); + let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); + let htlc_stats = self.context.get_pending_htlc_stats(None, dust_exposure_limiting_feerate); if htlc_stats.pending_inbound_htlcs + 1 > self.context.holder_max_accepted_htlcs as usize { return Err(ChannelError::Close(format!("Remote tried to push more than our max accepted HTLCs ({})", self.context.holder_max_accepted_htlcs))); } @@ -4989,7 +5052,8 @@ impl Channel where } // Before proposing a feerate update, check that we can actually afford the new fee. - let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw)); + let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); + let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate); let keys = self.context.build_holder_transaction_keys(self.context.cur_holder_commitment_transaction_number); let commitment_stats = self.context.build_commitment_transaction(self.context.cur_holder_commitment_transaction_number, &keys, true, true, logger); let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; @@ -5001,7 +5065,7 @@ impl Channel where } // Note, we evaluate pending htlc "preemptive" trimmed-to-dust threshold at the proposed `feerate_per_kw`. - let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(fee_estimator); + let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { log_debug!(logger, "Cannot afford to send new feerate at {} without infringing max dust htlc exposure", feerate_per_kw); return None; @@ -5239,17 +5303,16 @@ impl Channel where self.context.pending_update_fee = Some((msg.feerate_per_kw, FeeUpdateState::RemoteAnnounced)); self.context.update_time_counter += 1; // Check that we won't be pushed over our dust exposure limit by the feerate increase. - if !self.context.channel_type.supports_anchors_zero_fee_htlc_tx() { - let htlc_stats = self.context.get_pending_htlc_stats(None); - let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(fee_estimator); - if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { - return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our own transactions (totaling {} msat)", - msg.feerate_per_kw, htlc_stats.on_holder_tx_dust_exposure_msat))); - } - if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { - return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our counterparty's transactions (totaling {} msat)", - msg.feerate_per_kw, htlc_stats.on_counterparty_tx_dust_exposure_msat))); - } + let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); + let htlc_stats = self.context.get_pending_htlc_stats(None, dust_exposure_limiting_feerate); + let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); + if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { + return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our own transactions (totaling {} msat)", + msg.feerate_per_kw, htlc_stats.on_holder_tx_dust_exposure_msat))); + } + if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { + return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our counterparty's transactions (totaling {} msat)", + msg.feerate_per_kw, htlc_stats.on_counterparty_tx_dust_exposure_msat))); } Ok(()) } @@ -6093,8 +6156,9 @@ impl Channel where return Err(("Shutdown was already sent", 0x4000|8)) } - let htlc_stats = self.context.get_pending_htlc_stats(None); - let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(fee_estimator); + let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); + let htlc_stats = self.context.get_pending_htlc_stats(None, dust_exposure_limiting_feerate); + let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); let (htlc_timeout_dust_limit, htlc_success_dust_limit) = if self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() { (0, 0) } else { @@ -6110,6 +6174,16 @@ impl Channel where on_counterparty_tx_dust_htlc_exposure_msat, max_dust_htlc_exposure_msat); return Err(("Exceeded our dust exposure limit on counterparty commitment tx", 0x1000|7)) } + } else { + let htlc_dust_exposure_msat = + per_outbound_htlc_counterparty_commit_tx_fee_msat(self.context.feerate_per_kw, &self.context.channel_type); + let counterparty_tx_dust_exposure = + htlc_stats.on_counterparty_tx_dust_exposure_msat.saturating_add(htlc_dust_exposure_msat); + if counterparty_tx_dust_exposure > max_dust_htlc_exposure_msat { + log_info!(logger, "Cannot accept value that would put our exposure to tx fee dust at {} over the limit {} on counterparty commitment tx", + counterparty_tx_dust_exposure, max_dust_htlc_exposure_msat); + return Err(("Exceeded our tx fee dust exposure limit on counterparty commitment tx", 0x1000|7)) + } } let exposure_dust_limit_success_sats = htlc_success_dust_limit + self.context.holder_dust_limit_satoshis; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 05d4351509e..08b223a9994 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7673,7 +7673,7 @@ where } } } - try_chan_phase_entry!(self, chan.update_add_htlc(&msg, pending_forward_info), chan_phase_entry); + try_chan_phase_entry!(self, chan.update_add_htlc(&msg, pending_forward_info, &self.fee_estimator), chan_phase_entry); } else { return try_chan_phase_entry!(self, Err(ChannelError::Close( "Got an update_add_htlc message for an unfunded channel!".into())), chan_phase_entry); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 34ca9a7101f..cba5f2bab87 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -9872,7 +9872,7 @@ enum ExposureEvent { AtUpdateFeeOutbound, } -fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_event: ExposureEvent, on_holder_tx: bool, multiplier_dust_limit: bool) { +fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_event: ExposureEvent, on_holder_tx: bool, multiplier_dust_limit: bool, apply_excess_fee: bool) { // Test that we properly reject dust HTLC violating our `max_dust_htlc_exposure_msat` // policy. // @@ -9887,12 +9887,33 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e let chanmon_cfgs = create_chanmon_cfgs(2); let mut config = test_default_channel_config(); + + // We hard-code the feerate values here but they're re-calculated furter down and asserted. + // If the values ever change below these constants should simply be updated. + const AT_FEE_OUTBOUND_HTLCS: u64 = 20; + let nondust_htlc_count_in_limit = + if exposure_breach_event == ExposureEvent::AtUpdateFeeOutbound { + AT_FEE_OUTBOUND_HTLCS + } else { 0 }; + let initial_feerate = if apply_excess_fee { 253 * 2 } else { 253 }; + let expected_dust_buffer_feerate = initial_feerate + 2530; + let mut commitment_tx_cost = commit_tx_fee_msat(initial_feerate - 253, nondust_htlc_count_in_limit, &ChannelTypeFeatures::empty()); + commitment_tx_cost += + if on_holder_tx { + htlc_success_tx_weight(&ChannelTypeFeatures::empty()) + } else { + htlc_timeout_tx_weight(&ChannelTypeFeatures::empty()) + } * (initial_feerate as u64 - 253) / 1000 * nondust_htlc_count_in_limit; + { + let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap(); + *feerate_lock = initial_feerate; + } config.channel_config.max_dust_htlc_exposure = if multiplier_dust_limit { // Default test fee estimator rate is 253 sat/kw, so we set the multiplier to 5_000_000 / 253 // to get roughly the same initial value as the default setting when this test was // originally written. - MaxDustHTLCExposure::FeeRateMultiplier(5_000_000 / 253) - } else { MaxDustHTLCExposure::FixedLimitMsat(5_000_000) }; // initial default setting value + MaxDustHTLCExposure::FeeRateMultiplier((5_000_000 + commitment_tx_cost) / 253) + } else { MaxDustHTLCExposure::FixedLimitMsat(5_000_000 + commitment_tx_cost) }; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config), None]); let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -9936,6 +9957,11 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &channel_ready); update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update); + { + let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap(); + *feerate_lock = 253; + } + // Fetch a route in advance as we will be unable to once we're unable to send. let (mut route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000); @@ -9945,8 +9971,9 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let chan = chan_lock.channel_by_id.get(&channel_id).unwrap(); (chan.context().get_dust_buffer_feerate(None) as u64, - chan.context().get_max_dust_htlc_exposure_msat(&LowerBoundedFeeEstimator(nodes[0].fee_estimator))) + chan.context().get_max_dust_htlc_exposure_msat(253)) }; + assert_eq!(dust_buffer_feerate, expected_dust_buffer_feerate as u64); let dust_outbound_htlc_on_holder_tx_msat: u64 = (dust_buffer_feerate * htlc_timeout_tx_weight(&channel_type_features) / 1000 + open_channel.common_fields.dust_limit_satoshis - 1) * 1000; let dust_outbound_htlc_on_holder_tx: u64 = max_dust_htlc_exposure_msat / dust_outbound_htlc_on_holder_tx_msat; @@ -9956,8 +9983,13 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e let dust_inbound_htlc_on_holder_tx_msat: u64 = (dust_buffer_feerate * htlc_success_tx_weight(&channel_type_features) / 1000 + open_channel.common_fields.dust_limit_satoshis - if multiplier_dust_limit { 3 } else { 2 }) * 1000; let dust_inbound_htlc_on_holder_tx: u64 = max_dust_htlc_exposure_msat / dust_inbound_htlc_on_holder_tx_msat; + // This test was written with a fixed dust value here, which we retain, but assert that it is, + // indeed, dust on both transactions. let dust_htlc_on_counterparty_tx: u64 = 4; - let dust_htlc_on_counterparty_tx_msat: u64 = max_dust_htlc_exposure_msat / dust_htlc_on_counterparty_tx; + let dust_htlc_on_counterparty_tx_msat: u64 = 1_250_000; + let calcd_dust_htlc_on_counterparty_tx_msat: u64 = (dust_buffer_feerate * htlc_timeout_tx_weight(&channel_type_features) / 1000 + open_channel.common_fields.dust_limit_satoshis - if multiplier_dust_limit { 3 } else { 2 }) * 1000; + assert!(dust_htlc_on_counterparty_tx_msat < dust_inbound_htlc_on_holder_tx_msat); + assert!(dust_htlc_on_counterparty_tx_msat < calcd_dust_htlc_on_counterparty_tx_msat); if on_holder_tx { if dust_outbound_balance { @@ -10027,7 +10059,7 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e // Outbound dust balance: 5200 sats nodes[0].logger.assert_log("lightning::ln::channel", format!("Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", - dust_htlc_on_counterparty_tx_msat * (dust_htlc_on_counterparty_tx - 1) + dust_htlc_on_counterparty_tx_msat + 4, + dust_htlc_on_counterparty_tx_msat * dust_htlc_on_counterparty_tx + commitment_tx_cost + 4, max_dust_htlc_exposure_msat), 1); } } else if exposure_breach_event == ExposureEvent::AtUpdateFeeOutbound { @@ -10035,7 +10067,7 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e // For the multiplier dust exposure limit, since it scales with feerate, // we need to add a lot of HTLCs that will become dust at the new feerate // to cross the threshold. - for _ in 0..20 { + for _ in 0..AT_FEE_OUTBOUND_HTLCS { let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(1_000), None); nodes[0].node.send_payment_with_route(&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)).unwrap(); @@ -10054,25 +10086,33 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e added_monitors.clear(); } -fn do_test_max_dust_htlc_exposure_by_threshold_type(multiplier_dust_limit: bool) { - do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCForward, true, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCForward, true, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCReception, true, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCReception, false, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCForward, false, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCReception, false, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCReception, true, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCForward, false, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(true, ExposureEvent::AtUpdateFeeOutbound, true, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(true, ExposureEvent::AtUpdateFeeOutbound, false, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(false, ExposureEvent::AtUpdateFeeOutbound, false, multiplier_dust_limit); - do_test_max_dust_htlc_exposure(false, ExposureEvent::AtUpdateFeeOutbound, true, multiplier_dust_limit); +fn do_test_max_dust_htlc_exposure_by_threshold_type(multiplier_dust_limit: bool, apply_excess_fee: bool) { + do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCForward, true, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCForward, true, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCReception, true, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCReception, false, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCForward, false, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCReception, false, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCReception, true, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCForward, false, multiplier_dust_limit, apply_excess_fee); + if !multiplier_dust_limit && !apply_excess_fee { + // Because non-dust HTLC transaction fees are included in the dust exposure, trying to + // increase the fee to hit a higher dust exposure with a + // `MaxDustHTLCExposure::FeeRateMultiplier` is no longer super practical, so we skip these + // in the `multiplier_dust_limit` case. + do_test_max_dust_htlc_exposure(true, ExposureEvent::AtUpdateFeeOutbound, true, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(true, ExposureEvent::AtUpdateFeeOutbound, false, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(false, ExposureEvent::AtUpdateFeeOutbound, false, multiplier_dust_limit, apply_excess_fee); + do_test_max_dust_htlc_exposure(false, ExposureEvent::AtUpdateFeeOutbound, true, multiplier_dust_limit, apply_excess_fee); + } } #[test] fn test_max_dust_htlc_exposure() { - do_test_max_dust_htlc_exposure_by_threshold_type(false); - do_test_max_dust_htlc_exposure_by_threshold_type(true); + do_test_max_dust_htlc_exposure_by_threshold_type(false, false); + do_test_max_dust_htlc_exposure_by_threshold_type(false, true); + do_test_max_dust_htlc_exposure_by_threshold_type(true, false); + do_test_max_dust_htlc_exposure_by_threshold_type(true, true); } #[test] From f0b3708c209bc378e006678dffc6707740f37deb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 5 May 2024 00:44:07 +0000 Subject: [PATCH 5/6] Update default dust exposure limit and the documentation thereof Now that we're including excess counterparty commitment transaction fees in our dust calculation, we need to update the docs accordingly. We do so here, describing some of the considerations and risks that come with the new changes. We also take this opportunity to double the default value, as users have regularly complained that non-anchor channels fail to send HTLCs with the default settings with some feerates. Fixes #2922 --- lightning/src/util/config.rs | 67 ++++++++++++++++++++++++++++-------- 1 file changed, 52 insertions(+), 15 deletions(-) diff --git a/lightning/src/util/config.rs b/lightning/src/util/config.rs index 2c8f03b93c8..2473eea2627 100644 --- a/lightning/src/util/config.rs +++ b/lightning/src/util/config.rs @@ -360,7 +360,7 @@ impl Readable for ChannelHandshakeLimits { } } -/// Options for how to set the max dust HTLC exposure allowed on a channel. See +/// Options for how to set the max dust exposure allowed on a channel. See /// [`ChannelConfig::max_dust_htlc_exposure`] for details. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum MaxDustHTLCExposure { @@ -374,19 +374,17 @@ pub enum MaxDustHTLCExposure { /// to this maximum the channel may be unable to send/receive HTLCs between the maximum dust /// exposure and the new minimum value for HTLCs to be economically viable to claim. FixedLimitMsat(u64), - /// This sets a multiplier on the estimated high priority feerate (sats/KW, as obtained from - /// [`FeeEstimator`]) to determine the maximum allowed dust exposure. If this variant is used - /// then the maximum dust exposure in millisatoshis is calculated as: - /// `high_priority_feerate_per_kw * value`. For example, with our default value - /// `FeeRateMultiplier(5000)`: + /// This sets a multiplier on the [`ConfirmationTarget::OnChainSweep`] feerate (in sats/KW) to + /// determine the maximum allowed dust exposure. If this variant is used then the maximum dust + /// exposure in millisatoshis is calculated as: + /// `feerate_per_kw * value`. For example, with our default value + /// `FeeRateMultiplier(10_000)`: /// /// - For the minimum fee rate of 1 sat/vByte (250 sat/KW, although the minimum /// defaults to 253 sats/KW for rounding, see [`FeeEstimator`]), the max dust exposure would - /// be 253 * 5000 = 1,265,000 msats. + /// be 253 * 10_000 = 2,530,000 msats. /// - For a fee rate of 30 sat/vByte (7500 sat/KW), the max dust exposure would be - /// 7500 * 5000 = 37,500,000 msats. - /// - /// This allows the maximum dust exposure to automatically scale with fee rate changes. + /// 7500 * 50_000 = 75,000,000 msats (0.00075 BTC). /// /// Note, if you're using a third-party fee estimator, this may leave you more exposed to a /// fee griefing attack, where your fee estimator may purposely overestimate the fee rate, @@ -401,6 +399,7 @@ pub enum MaxDustHTLCExposure { /// by default this will be set to a [`Self::FixedLimitMsat`] of 5,000,000 msat. /// /// [`FeeEstimator`]: crate::chain::chaininterface::FeeEstimator + /// [`ConfirmationTarget::OnChainSweep`]: crate::chain::chaininterface::ConfirmationTarget::OnChainSweep FeeRateMultiplier(u64), } @@ -453,13 +452,16 @@ pub struct ChannelConfig { /// /// [`MIN_CLTV_EXPIRY_DELTA`]: crate::ln::channelmanager::MIN_CLTV_EXPIRY_DELTA pub cltv_expiry_delta: u16, - /// Limit our total exposure to in-flight HTLCs which are burned to fees as they are too - /// small to claim on-chain. + /// Limit our total exposure to potential loss to on-chain fees on close, including in-flight + /// HTLCs which are burned to fees as they are too small to claim on-chain and fees on + /// commitment transaction(s) broadcasted by our counterparty in excess of our own fee estimate. + /// + /// # HTLC-based Dust Exposure /// /// When an HTLC present in one of our channels is below a "dust" threshold, the HTLC will /// not be claimable on-chain, instead being turned into additional miner fees if either /// party force-closes the channel. Because the threshold is per-HTLC, our total exposure - /// to such payments may be sustantial if there are many dust HTLCs present when the + /// to such payments may be substantial if there are many dust HTLCs present when the /// channel is force-closed. /// /// The dust threshold for each HTLC is based on the `dust_limit_satoshis` for each party in a @@ -473,7 +475,42 @@ pub struct ChannelConfig { /// The selected limit is applied for sent, forwarded, and received HTLCs and limits the total /// exposure across all three types per-channel. /// - /// Default value: [`MaxDustHTLCExposure::FeeRateMultiplier`] with a multiplier of 5000. + /// # Transaction Fee Dust Exposure + /// + /// Further, counterparties broadcasting a commitment transaction in a force-close may result + /// in other balance being burned to fees, and thus all fees on commitment and HTLC + /// transactions in excess of our local fee estimates are included in the dust calculation. + /// + /// Because of this, another way to look at this limit is to divide it by 43,000 (or 218,750 + /// for non-anchor channels) and see it as the maximum feerate disagreement (in sats/vB) per + /// non-dust HTLC we're allowed to have with our peers before risking a force-closure for + /// inbound channels. + // This works because, for anchor channels the on-chain cost is 172 weight (172+703 for + // non-anchors with an HTLC-Success transaction), i.e. + // dust_exposure_limit_msat / 1000 = 172 * feerate_in_sat_per_vb / 4 * HTLC count + // dust_exposure_limit_msat = 43,000 * feerate_in_sat_per_vb * HTLC count + // dust_exposure_limit_msat / HTLC count / 43,000 = feerate_in_sat_per_vb + /// + /// Thus, for the default value of 10_000 * a current feerate estimate of 10 sat/vB (or 2,500 + /// sat/KW), we risk force-closure if we disagree with our peer by: + /// * `10_000 * 2_500 / 43_000 / (483*2)` = 0.6 sat/vB for anchor channels with 483 HTLCs in + /// both directions (the maximum), + /// * `10_000 * 2_500 / 43_000 / (50*2)` = 5.8 sat/vB for anchor channels with 50 HTLCs in both + /// directions (the LDK default max from [`ChannelHandshakeConfig::our_max_accepted_htlcs`]) + /// * `10_000 * 2_500 / 218_750 / (483*2)` = 0.1 sat/vB for non-anchor channels with 483 HTLCs + /// in both directions (the maximum), + /// * `10_000 * 2_500 / 218_750 / (50*2)` = 1.1 sat/vB for non-anchor channels with 50 HTLCs + /// in both (the LDK default maximum from [`ChannelHandshakeConfig::our_max_accepted_htlcs`]) + /// + /// Note that when using [`MaxDustHTLCExposure::FeeRateMultiplier`] this maximum disagreement + /// will scale linearly with increases (or decreases) in the our feerate estimates. Further, + /// for anchor channels we expect our counterparty to use a relatively low feerate estimate + /// while we use [`ConfirmationTarget::OnChainSweep`] (which should be relatively high) and + /// feerate disagreement force-closures should only occur when theirs is higher than ours. + /// + /// Default value: [`MaxDustHTLCExposure::FeeRateMultiplier`] with a multiplier of 10_000. + /// + /// [`ConfirmationTarget::OnChainSweep`]: crate::chain::chaininterface::ConfirmationTarget::OnChainSweep pub max_dust_htlc_exposure: MaxDustHTLCExposure, /// The additional fee we're willing to pay to avoid waiting for the counterparty's /// `to_self_delay` to reclaim funds. @@ -561,7 +598,7 @@ impl Default for ChannelConfig { forwarding_fee_proportional_millionths: 0, forwarding_fee_base_msat: 1000, cltv_expiry_delta: 6 * 12, // 6 blocks/hour * 12 hours - max_dust_htlc_exposure: MaxDustHTLCExposure::FeeRateMultiplier(5000), + max_dust_htlc_exposure: MaxDustHTLCExposure::FeeRateMultiplier(10000), force_close_avoidance_max_fee_satoshis: 1000, accept_underpaying_htlcs: false, } From 5091c1fd68e0a72fb18b7f1bed6c6f3998bf4ef1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 5 May 2024 02:00:42 +0000 Subject: [PATCH 6/6] Add a simple test of the new excess-fees-are-dust behavior --- lightning/src/ln/functional_tests.rs | 88 ++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index cba5f2bab87..fdd610bf476 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -10115,6 +10115,94 @@ fn test_max_dust_htlc_exposure() { do_test_max_dust_htlc_exposure_by_threshold_type(true, true); } +#[test] +fn test_nondust_htlc_fees_are_dust() { + // Test that the transaction fees paid in nondust HTLCs count towards our dust limit + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + + let mut config = test_default_channel_config(); + // Set the dust limit to the default value + config.channel_config.max_dust_htlc_exposure = + MaxDustHTLCExposure::FeeRateMultiplier(10_000); + // Make sure the HTLC limits don't get in the way + config.channel_handshake_limits.min_max_accepted_htlcs = 400; + config.channel_handshake_config.our_max_accepted_htlcs = 400; + config.channel_handshake_config.our_htlc_minimum_msat = 1; + + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config), Some(config), Some(config)]); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + // Create a channel from 1 -> 0 but immediately push all of the funds towards 0 + let chan_id_1 = create_announced_chan_between_nodes(&nodes, 1, 0).2; + while nodes[1].node.list_channels()[0].next_outbound_htlc_limit_msat > 0 { + send_payment(&nodes[1], &[&nodes[0]], nodes[1].node.list_channels()[0].next_outbound_htlc_limit_msat); + } + + // First get the channel one HTLC_VALUE HTLC away from the dust limit by sending dust HTLCs + // repeatedly until we run out of space. + const HTLC_VALUE: u64 = 1_000_000; // Doesn't matter, tune until the test passes + let payment_preimage = route_payment(&nodes[0], &[&nodes[1]], HTLC_VALUE).0; + + while nodes[0].node.list_channels()[0].next_outbound_htlc_minimum_msat == 0 { + route_payment(&nodes[0], &[&nodes[1]], HTLC_VALUE); + } + assert_ne!(nodes[0].node.list_channels()[0].next_outbound_htlc_limit_msat, 0, + "We don't want to run out of ability to send because of some non-dust limit"); + assert!(nodes[0].node.list_channels()[0].pending_outbound_htlcs.len() < 10, + "We should be able to fill our dust limit without too many HTLCs"); + + let dust_limit = nodes[0].node.list_channels()[0].next_outbound_htlc_minimum_msat; + claim_payment(&nodes[0], &[&nodes[1]], payment_preimage); + assert_ne!(nodes[0].node.list_channels()[0].next_outbound_htlc_minimum_msat, 0, + "Make sure we are able to send once we clear one HTLC"); + + // At this point we have somewhere between dust_limit and dust_limit * 2 left in our dust + // exposure limit, and we want to max that out using non-dust HTLCs. + let commitment_tx_per_htlc_cost = + htlc_success_tx_weight(&ChannelTypeFeatures::empty()) * 253; + let max_htlcs_remaining = dust_limit * 2 / commitment_tx_per_htlc_cost; + assert!(max_htlcs_remaining < 30, + "We should be able to fill our dust limit without too many HTLCs"); + for i in 0..max_htlcs_remaining + 1 { + assert_ne!(i, max_htlcs_remaining); + if nodes[0].node.list_channels()[0].next_outbound_htlc_limit_msat < dust_limit { + // We found our limit, and it was less than max_htlcs_remaining! + // At this point we can only send dust HTLCs as any non-dust HTLCs will overuse our + // remaining dust exposure. + break; + } + route_payment(&nodes[0], &[&nodes[1]], dust_limit * 2); + } + + // At this point non-dust HTLCs are no longer accepted from node 0 -> 1, we also check that + // such HTLCs can't be routed over the same channel either. + create_announced_chan_between_nodes(&nodes, 2, 0); + let (route, payment_hash, _, payment_secret) = + get_route_and_payment_hash!(nodes[2], nodes[1], dust_limit * 2); + let onion = RecipientOnionFields::secret_only(payment_secret); + nodes[2].node.send_payment_with_route(&route, payment_hash, onion, PaymentId([0; 32])).unwrap(); + check_added_monitors(&nodes[2], 1); + let send = SendEvent::from_node(&nodes[2]); + + nodes[0].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &send.msgs[0]); + commitment_signed_dance!(nodes[0], nodes[2], send.commitment_msg, false, true); + + expect_pending_htlcs_forwardable!(nodes[0]); + check_added_monitors(&nodes[0], 1); + let node_id_1 = nodes[1].node.get_our_node_id(); + expect_htlc_handling_failed_destinations!( + nodes[0].node.get_and_clear_pending_events(), + &[HTLCDestination::NextHopChannel { node_id: Some(node_id_1), channel_id: chan_id_1 }] + ); + + let fail = get_htlc_update_msgs(&nodes[0], &nodes[2].node.get_our_node_id()); + nodes[2].node.handle_update_fail_htlc(&nodes[0].node.get_our_node_id(), &fail.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[2], nodes[0], fail.commitment_signed, false); + expect_payment_failed_conditions(&nodes[2], payment_hash, false, PaymentFailedConditions::new()); +} + + #[test] fn test_non_final_funding_tx() { let chanmon_cfgs = create_chanmon_cfgs(2);