From 013967b2a0d90a111dc7c9a016c1b618fd15d8d9 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 31 Jul 2025 02:17:01 +0000 Subject: [PATCH 01/11] Add `TxBuilder::get_next_commitment_stats` Given a snapshot of the lightning state machine, `TxBuilder::get_next_commitment_stats` calculates the transaction fees, the dust exposure, and the holder and counterparty balances (the balances themselves do *not* account for the transaction fee). --- lightning/src/ln/chan_utils.rs | 2 +- lightning/src/sign/tx_builder.rs | 232 +++++++++++++++++++++++++++++-- 2 files changed, 225 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 557a988cc92..535649f7895 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -236,7 +236,7 @@ pub(crate) fn commit_tx_fee_sat(feerate_per_kw: u32, num_htlcs: usize, channel_t } /// Returns the fees for success and timeout second stage HTLC transactions. -pub(super) fn second_stage_tx_fees_sat( +pub(crate) fn second_stage_tx_fees_sat( channel_type: &ChannelTypeFeatures, feerate_sat_per_1000_weight: u32, ) -> (u64, u64) { if channel_type.supports_anchors_zero_fee_htlc_tx() diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index 6e623d1a7db..b7bd7c531ca 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -1,19 +1,128 @@ //! Defines the `TxBuilder` trait, and the `SpecTxBuilder` type +#![allow(dead_code)] +use core::cmp; use core::ops::Deref; use bitcoin::secp256k1::{self, PublicKey, Secp256k1}; use crate::ln::chan_utils::{ - commit_tx_fee_sat, htlc_success_tx_weight, htlc_timeout_tx_weight, - ChannelTransactionParameters, CommitmentTransaction, HTLCOutputInCommitment, + commit_tx_fee_sat, htlc_success_tx_weight, htlc_timeout_tx_weight, htlc_tx_fees_sat, + second_stage_tx_fees_sat, ChannelTransactionParameters, CommitmentTransaction, + HTLCOutputInCommitment, }; use crate::ln::channel::{CommitmentStats, ANCHOR_OUTPUT_VALUE_SATOSHI}; use crate::prelude::*; use crate::types::features::ChannelTypeFeatures; use crate::util::logger::Logger; +pub(crate) struct HTLCAmountDirection { + pub outbound: bool, + pub amount_msat: u64, +} + +impl HTLCAmountDirection { + fn is_dust( + &self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_satoshis: u64, + channel_type: &ChannelTypeFeatures, + ) -> bool { + let (success_tx_fee_sat, timeout_tx_fee_sat) = + second_stage_tx_fees_sat(channel_type, feerate_per_kw); + let htlc_tx_fee_sat = + if self.outbound == local { timeout_tx_fee_sat } else { success_tx_fee_sat }; + self.amount_msat / 1000 < broadcaster_dust_limit_satoshis + htlc_tx_fee_sat + } +} + +pub(crate) struct NextCommitmentStats { + pub inbound_htlcs_count: usize, + pub inbound_htlcs_value_msat: u64, + pub holder_balance_msat: Option, + pub counterparty_balance_msat: Option, + pub nondust_htlc_count: usize, + pub commit_tx_fee_sat: u64, + pub dust_exposure_msat: u64, + // If the counterparty sets a feerate on the channel in excess of our dust_exposure_limiting_feerate, + // this should be set to the dust exposure that would result from us adding an additional nondust outbound + // htlc on the counterparty's commitment transaction. + pub extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat: Option, +} + +#[rustfmt::skip] +fn excess_fees_on_counterparty_tx_dust_exposure_msat( + next_commitment_htlcs: &[HTLCAmountDirection], dust_buffer_feerate: u32, + excess_feerate: u32, counterparty_dust_limit_satoshis: u64, mut on_counterparty_tx_dust_exposure_msat: u64, + channel_type: &ChannelTypeFeatures, +) -> (u64, u64) { + + let on_counterparty_tx_accepted_nondust_htlcs = next_commitment_htlcs.iter().filter(|htlc| htlc.outbound && !htlc.is_dust(false, dust_buffer_feerate, counterparty_dust_limit_satoshis, channel_type)).count(); + let on_counterparty_tx_offered_nondust_htlcs = next_commitment_htlcs.iter().filter(|htlc| !htlc.outbound && !htlc.is_dust(false, dust_buffer_feerate, counterparty_dust_limit_satoshis, channel_type)).count(); + + let extra_htlc_commit_tx_fee_sat = commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, channel_type); + let extra_htlc_htlc_tx_fees_sat = htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1, on_counterparty_tx_offered_nondust_htlcs, channel_type); + + let commit_tx_fee_sat = commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs, channel_type); + let htlc_tx_fees_sat = htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs, on_counterparty_tx_offered_nondust_htlcs, channel_type); + + let extra_htlc_dust_exposure_msat = on_counterparty_tx_dust_exposure_msat + (extra_htlc_commit_tx_fee_sat + extra_htlc_htlc_tx_fees_sat) * 1000; + on_counterparty_tx_dust_exposure_msat += (commit_tx_fee_sat + htlc_tx_fees_sat) * 1000; + + ( + on_counterparty_tx_dust_exposure_msat, + extra_htlc_dust_exposure_msat, + ) +} + +fn subtract_addl_outputs( + is_outbound_from_holder: bool, value_to_self_after_htlcs: u64, + value_to_remote_after_htlcs: u64, channel_type: &ChannelTypeFeatures, +) -> (Option, Option) { + let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() { + ANCHOR_OUTPUT_VALUE_SATOSHI * 2 + } else { + 0 + }; + + // We MUST use checked subs here, as the funder's balance is not guaranteed to be greater + // than or equal to `total_anchors_sat`. + // + // This is because when the remote party sends an `update_fee` message, we build the new + // commitment transaction *before* checking whether the remote party's balance is enough to + // cover the total anchor sum. + + let local_balance_before_fee_msat = if is_outbound_from_holder { + value_to_self_after_htlcs.checked_sub(total_anchors_sat * 1000) + } else { + Some(value_to_self_after_htlcs) + }; + + let remote_balance_before_fee_msat = if !is_outbound_from_holder { + value_to_remote_after_htlcs.checked_sub(total_anchors_sat * 1000) + } else { + Some(value_to_remote_after_htlcs) + }; + + (local_balance_before_fee_msat, remote_balance_before_fee_msat) +} + +fn get_dust_buffer_feerate(feerate_per_kw: u32) -> 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. + let feerate_plus_quarter = feerate_per_kw.checked_mul(1250).map(|v| v / 1000); + cmp::max(feerate_per_kw.saturating_add(2530), feerate_plus_quarter.unwrap_or(u32::MAX)) +} + pub(crate) trait TxBuilder { + fn get_next_commitment_stats( + &self, local: bool, is_outbound_from_holder: bool, channel_value_satoshis: u64, + value_to_holder_msat: u64, next_commitment_htlcs: &[HTLCAmountDirection], + addl_nondust_htlc_count: usize, feerate_per_kw: u32, + dust_exposure_limiting_feerate: Option, broadcaster_dust_limit_satoshis: u64, + channel_type: &ChannelTypeFeatures, + ) -> NextCommitmentStats; fn commit_tx_fee_sat( &self, feerate_per_kw: u32, nondust_htlc_count: usize, channel_type: &ChannelTypeFeatures, ) -> u64; @@ -25,7 +134,7 @@ pub(crate) trait TxBuilder { &self, local: bool, commitment_number: u64, per_commitment_point: &PublicKey, channel_parameters: &ChannelTransactionParameters, secp_ctx: &Secp256k1, value_to_self_msat: u64, htlcs_in_tx: Vec, feerate_per_kw: u32, - broadcaster_dust_limit_sat: u64, logger: &L, + broadcaster_dust_limit_satoshis: u64, logger: &L, ) -> (CommitmentTransaction, CommitmentStats) where L::Target: Logger; @@ -34,6 +143,113 @@ pub(crate) trait TxBuilder { pub(crate) struct SpecTxBuilder {} impl TxBuilder for SpecTxBuilder { + fn get_next_commitment_stats( + &self, local: bool, is_outbound_from_holder: bool, channel_value_satoshis: u64, + value_to_holder_msat: u64, next_commitment_htlcs: &[HTLCAmountDirection], + addl_nondust_htlc_count: usize, feerate_per_kw: u32, + dust_exposure_limiting_feerate: Option, broadcaster_dust_limit_satoshis: u64, + channel_type: &ChannelTypeFeatures, + ) -> NextCommitmentStats { + let excess_feerate_opt = + feerate_per_kw.checked_sub(dust_exposure_limiting_feerate.unwrap_or(0)); + // Dust exposure is only decoupled from feerate for zero fee commitment channels. + let is_zero_fee_comm = channel_type.supports_anchor_zero_fee_commitments(); + debug_assert_eq!(is_zero_fee_comm, dust_exposure_limiting_feerate.is_none()); + if is_zero_fee_comm { + debug_assert_eq!(feerate_per_kw, 0); + debug_assert_eq!(excess_feerate_opt, Some(0)); + debug_assert_eq!(addl_nondust_htlc_count, 0); + } + + // Calculate inbound htlc count + let inbound_htlcs_count = + next_commitment_htlcs.iter().filter(|htlc| !htlc.outbound).count(); + + // Calculate balances after htlcs + let value_to_counterparty_msat = (channel_value_satoshis * 1000) + .checked_sub(value_to_holder_msat) + .expect("value_to_holder_msat outgrew the value of the channel!"); + let outbound_htlcs_value_msat: u64 = next_commitment_htlcs + .iter() + .filter_map(|htlc| htlc.outbound.then_some(htlc.amount_msat)) + .sum(); + let inbound_htlcs_value_msat: u64 = next_commitment_htlcs + .iter() + .filter_map(|htlc| (!htlc.outbound).then_some(htlc.amount_msat)) + .sum(); + let value_to_holder_after_htlcs = value_to_holder_msat + .checked_sub(outbound_htlcs_value_msat) + .expect("outbound_htlcs_value_msat is greater than value_to_holder_msat"); + let value_to_counterparty_after_htlcs = value_to_counterparty_msat + .checked_sub(inbound_htlcs_value_msat) + .expect("inbound_htlcs_value_msat is greater than value_to_counterparty_msat"); + + // Subtract the anchors from the channel funder + let (holder_balance_msat, counterparty_balance_msat) = subtract_addl_outputs( + is_outbound_from_holder, + value_to_holder_after_htlcs, + value_to_counterparty_after_htlcs, + channel_type, + ); + + // Increment the feerate by a buffer to calculate dust exposure + let dust_buffer_feerate = get_dust_buffer_feerate(feerate_per_kw); + + // Calculate fees on commitment transaction + let nondust_htlc_count = next_commitment_htlcs + .iter() + .filter(|htlc| { + !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_satoshis, channel_type) + }) + .count(); + let commit_tx_fee_sat = commit_tx_fee_sat( + feerate_per_kw, + nondust_htlc_count + addl_nondust_htlc_count, + channel_type, + ); + + // Calculate dust exposure on commitment transaction + let dust_exposure_msat = next_commitment_htlcs + .iter() + .filter_map(|htlc| { + htlc.is_dust( + local, + dust_buffer_feerate, + broadcaster_dust_limit_satoshis, + channel_type, + ) + .then_some(htlc.amount_msat) + }) + .sum(); + + // Count the excess fees on the counterparty's transaction as dust + let (dust_exposure_msat, extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat) = + if let (Some(excess_feerate), false) = (excess_feerate_opt, local) { + let (dust_exposure_msat, extra_nondust_htlc_exposure_msat) = + excess_fees_on_counterparty_tx_dust_exposure_msat( + &next_commitment_htlcs, + dust_buffer_feerate, + excess_feerate, + broadcaster_dust_limit_satoshis, + dust_exposure_msat, + channel_type, + ); + (dust_exposure_msat, Some(extra_nondust_htlc_exposure_msat)) + } else { + (dust_exposure_msat, None) + }; + + NextCommitmentStats { + inbound_htlcs_count, + inbound_htlcs_value_msat, + holder_balance_msat, + counterparty_balance_msat, + nondust_htlc_count, + commit_tx_fee_sat, + dust_exposure_msat, + extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat, + } + } fn commit_tx_fee_sat( &self, feerate_per_kw: u32, nondust_htlc_count: usize, channel_type: &ChannelTypeFeatures, ) -> u64 { @@ -74,7 +290,7 @@ impl TxBuilder for SpecTxBuilder { &self, local: bool, commitment_number: u64, per_commitment_point: &PublicKey, channel_parameters: &ChannelTransactionParameters, secp_ctx: &Secp256k1, value_to_self_msat: u64, mut htlcs_in_tx: Vec, feerate_per_kw: u32, - broadcaster_dust_limit_sat: u64, logger: &L, + broadcaster_dust_limit_satoshis: u64, logger: &L, ) -> (CommitmentTransaction, CommitmentStats) where L::Target: Logger, @@ -95,7 +311,7 @@ impl TxBuilder for SpecTxBuilder { // As required by the spec, round down feerate_per_kw as u64 * htlc_tx_weight / 1000 }; - amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat + amount_msat / 1000 < broadcaster_dust_limit_satoshis + htlc_tx_fee_sat }; // Trim dust htlcs @@ -107,7 +323,7 @@ impl TxBuilder for SpecTxBuilder { remote_htlc_total_msat += htlc.amount_msat; } if is_dust(htlc.offered, htlc.amount_msat) { - log_trace!(logger, " ...trimming {} HTLC with value {}sat, hash {}, due to dust limit {}", if htlc.offered == local { "outbound" } else { "inbound" }, htlc.amount_msat / 1000, htlc.payment_hash, broadcaster_dust_limit_sat); + log_trace!(logger, " ...trimming {} HTLC with value {}sat, hash {}, due to dust limit {}", if htlc.offered == local { "outbound" } else { "inbound" }, htlc.amount_msat / 1000, htlc.payment_hash, broadcaster_dust_limit_satoshis); false } else { true @@ -142,13 +358,13 @@ impl TxBuilder for SpecTxBuilder { let mut to_broadcaster_value_sat = if local { value_to_self } else { value_to_remote }; let mut to_countersignatory_value_sat = if local { value_to_remote } else { value_to_self }; - if to_broadcaster_value_sat >= broadcaster_dust_limit_sat { + if to_broadcaster_value_sat >= broadcaster_dust_limit_satoshis { log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, to_broadcaster_value_sat); } else { to_broadcaster_value_sat = 0; } - if to_countersignatory_value_sat >= broadcaster_dust_limit_sat { + if to_countersignatory_value_sat >= broadcaster_dust_limit_satoshis { log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, to_countersignatory_value_sat); } else { to_countersignatory_value_sat = 0; From 2db5d2a4b68a45ed8547bf4b58cd932008ce1425 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 6 Aug 2025 17:33:41 +0000 Subject: [PATCH 02/11] Adjust dust exposure due to excess fees for clarity --- lightning/src/sign/tx_builder.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index b7bd7c531ca..6617633e1bb 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -51,21 +51,20 @@ pub(crate) struct NextCommitmentStats { #[rustfmt::skip] fn excess_fees_on_counterparty_tx_dust_exposure_msat( next_commitment_htlcs: &[HTLCAmountDirection], dust_buffer_feerate: u32, - excess_feerate: u32, counterparty_dust_limit_satoshis: u64, mut on_counterparty_tx_dust_exposure_msat: u64, + excess_feerate: u32, counterparty_dust_limit_satoshis: u64, dust_htlc_exposure_msat: u64, channel_type: &ChannelTypeFeatures, ) -> (u64, u64) { let on_counterparty_tx_accepted_nondust_htlcs = next_commitment_htlcs.iter().filter(|htlc| htlc.outbound && !htlc.is_dust(false, dust_buffer_feerate, counterparty_dust_limit_satoshis, channel_type)).count(); let on_counterparty_tx_offered_nondust_htlcs = next_commitment_htlcs.iter().filter(|htlc| !htlc.outbound && !htlc.is_dust(false, dust_buffer_feerate, counterparty_dust_limit_satoshis, channel_type)).count(); - let extra_htlc_commit_tx_fee_sat = commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, channel_type); - let extra_htlc_htlc_tx_fees_sat = htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1, on_counterparty_tx_offered_nondust_htlcs, channel_type); + let commitment_fee_sat = commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs, channel_type); + let second_stage_fees_sat = htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs, on_counterparty_tx_offered_nondust_htlcs, channel_type); + let on_counterparty_tx_dust_exposure_msat = dust_htlc_exposure_msat + (commitment_fee_sat + second_stage_fees_sat) * 1000; - let commit_tx_fee_sat = commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs, channel_type); - let htlc_tx_fees_sat = htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs, on_counterparty_tx_offered_nondust_htlcs, channel_type); - - let extra_htlc_dust_exposure_msat = on_counterparty_tx_dust_exposure_msat + (extra_htlc_commit_tx_fee_sat + extra_htlc_htlc_tx_fees_sat) * 1000; - on_counterparty_tx_dust_exposure_msat += (commit_tx_fee_sat + htlc_tx_fees_sat) * 1000; + let extra_htlc_commitment_fee_sat = commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, channel_type); + let extra_htlc_second_stage_fees_sat = htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1, on_counterparty_tx_offered_nondust_htlcs, channel_type); + let extra_htlc_dust_exposure_msat = dust_htlc_exposure_msat + (extra_htlc_commitment_fee_sat + extra_htlc_second_stage_fees_sat) * 1000; ( on_counterparty_tx_dust_exposure_msat, From b5923c771f3c87f72590ce5c0693d7ceaac26605 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 31 Jul 2025 02:30:47 +0000 Subject: [PATCH 03/11] Add `ChannelContext::get_next_{local, remote}_commitment_stats` In upcoming commits, these methods will serve as proxies to `SpecTxBuilder::get_next_commitment_stats` in all validation of channel updates in `ChannelContext`. Eventually, these methods will completely replace `get_pending_htlc_stats`, and `get_next_{local, remote}_commit_tx_fee_msat`. When predicting the HTLCs on next commitment, we take the conservative approach and only assume that a HTLC will not be in the next commitment when it is guaranteed that it won't be. --- lightning/src/ln/channel.rs | 163 +++++++++++++++++++++++++++++++++++- 1 file changed, 162 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b5b77972a6c..0858bbe6cf5 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -70,7 +70,7 @@ use crate::ln::script::{self, ShutdownScript}; use crate::ln::types::ChannelId; use crate::routing::gossip::NodeId; use crate::sign::ecdsa::EcdsaChannelSigner; -use crate::sign::tx_builder::{SpecTxBuilder, TxBuilder}; +use crate::sign::tx_builder::{HTLCAmountDirection, NextCommitmentStats, SpecTxBuilder, TxBuilder}; use crate::sign::{ChannelSigner, EntropySource, NodeSigner, Recipient, SignerProvider}; use crate::types::features::{ChannelTypeFeatures, InitFeatures}; use crate::types::payment::{PaymentHash, PaymentPreimage}; @@ -4104,6 +4104,167 @@ where ); } + /// Returns a best-effort guess of the set of HTLCs that will be present + /// on the next local or remote commitment. We cannot be certain as the + /// actual set of HTLCs present on the next commitment depends on the + /// ordering of commitment_signed and revoke_and_ack messages. + /// + /// We take the conservative approach and only assume that a HTLC will + /// not be in the next commitment when it is guaranteed that it won't be. + #[allow(dead_code)] + #[rustfmt::skip] + fn get_next_commitment_htlcs( + &self, local: bool, htlc_candidate: Option, include_counterparty_unknown_htlcs: bool, + ) -> Vec { + let mut commitment_htlcs = Vec::with_capacity( + 1 + self.pending_inbound_htlcs.len() + + self.pending_outbound_htlcs.len() + + self.holding_cell_htlc_updates.len(), + ); + // `LocalRemoved` HTLCs will certainly not be present on any future remote + // commitments, but they could be in a future local commitment as the remote has + // not yet acknowledged the removal. + let pending_inbound_htlcs = self + .pending_inbound_htlcs + .iter() + .filter(|InboundHTLCOutput { state, .. }| match (state, local) { + (InboundHTLCState::RemoteAnnounced(..), _) => true, + (InboundHTLCState::AwaitingRemoteRevokeToAnnounce(..), _) => true, + (InboundHTLCState::AwaitingAnnouncedRemoteRevoke(..), _) => true, + (InboundHTLCState::Committed, _) => true, + (InboundHTLCState::LocalRemoved(..), true) => true, + (InboundHTLCState::LocalRemoved(..), false) => false, + }) + .map(|&InboundHTLCOutput { amount_msat, .. }| HTLCAmountDirection { outbound: false, amount_msat }); + // `RemoteRemoved` HTLCs can still be present on the next remote commitment if + // local produces a commitment before acknowledging the update. These HTLCs + // will for sure not be present on the next local commitment. + let pending_outbound_htlcs = self + .pending_outbound_htlcs + .iter() + .filter(|OutboundHTLCOutput { state, .. }| match (state, local) { + (OutboundHTLCState::LocalAnnounced(..), _) => include_counterparty_unknown_htlcs, + (OutboundHTLCState::Committed, _) => true, + (OutboundHTLCState::RemoteRemoved(..), true) => false, + (OutboundHTLCState::RemoteRemoved(..), false) => true, + (OutboundHTLCState::AwaitingRemoteRevokeToRemove(..), _) => false, + (OutboundHTLCState::AwaitingRemovedRemoteRevoke(..), _) => false, + }) + .map(|&OutboundHTLCOutput { amount_msat, .. }| HTLCAmountDirection { outbound: true, amount_msat }); + + let holding_cell_htlcs = self.holding_cell_htlc_updates.iter().filter_map(|htlc| { + if let &HTLCUpdateAwaitingACK::AddHTLC { amount_msat, .. } = htlc { + Some(HTLCAmountDirection { outbound: true, amount_msat }) + } else { + None + } + }); + + if include_counterparty_unknown_htlcs { + commitment_htlcs.extend( + htlc_candidate.into_iter().chain(pending_inbound_htlcs).chain(pending_outbound_htlcs).chain(holding_cell_htlcs) + ); + } else { + commitment_htlcs.extend( + htlc_candidate.into_iter().chain(pending_inbound_htlcs).chain(pending_outbound_htlcs) + ); + } + + commitment_htlcs + } + + /// This returns the value of `value_to_self_msat` after accounting for all the + /// successful inbound and outbound HTLCs that won't be present on the next + /// commitment. + /// + /// To determine which HTLC claims to account for, we take the cases where a HTLC + /// will *not* be present on the next commitment from `next_commitment_htlcs`, and + /// check if their outcome is successful. If it is, we add the value of this claimed + /// HTLC to the balance of the claimer. + #[allow(dead_code)] + #[rustfmt::skip] + fn get_next_commitment_value_to_self_msat(&self, local: bool, funding: &FundingScope) -> u64 { + let inbound_claimed_htlc_msat: u64 = + self.pending_inbound_htlcs + .iter() + .filter(|InboundHTLCOutput { state, .. }| match (state, local) { + (InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_, _)), true) => false, + (InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_, _)), false) => true, + _ => false, + }) + .map(|InboundHTLCOutput { amount_msat, .. }| amount_msat) + .sum(); + let outbound_claimed_htlc_msat: u64 = + self.pending_outbound_htlcs + .iter() + .filter(|OutboundHTLCOutput { state, .. }| match (state, local) { + (OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_, _)), true) => true, + (OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_, _)), false) => false, + (OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _)), _) => true, + (OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _)), _) => true, + _ => false, + }) + .map(|OutboundHTLCOutput { amount_msat, .. }| amount_msat) + .sum(); + + funding + .value_to_self_msat + .saturating_sub(outbound_claimed_htlc_msat) + .saturating_add(inbound_claimed_htlc_msat) + } + + #[allow(dead_code)] + fn get_next_local_commitment_stats( + &self, funding: &FundingScope, htlc_candidate: Option, + include_counterparty_unknown_htlcs: bool, addl_nondust_htlc_count: usize, + feerate_per_kw: u32, dust_exposure_limiting_feerate: Option, + ) -> NextCommitmentStats { + let next_commitment_htlcs = self.get_next_commitment_htlcs( + true, + htlc_candidate, + include_counterparty_unknown_htlcs, + ); + let next_value_to_self_msat = self.get_next_commitment_value_to_self_msat(true, funding); + SpecTxBuilder {}.get_next_commitment_stats( + true, + funding.is_outbound(), + funding.get_value_satoshis(), + next_value_to_self_msat, + &next_commitment_htlcs, + addl_nondust_htlc_count, + feerate_per_kw, + dust_exposure_limiting_feerate, + self.holder_dust_limit_satoshis, + funding.get_channel_type(), + ) + } + + #[allow(dead_code)] + fn get_next_remote_commitment_stats( + &self, funding: &FundingScope, htlc_candidate: Option, + include_counterparty_unknown_htlcs: bool, addl_nondust_htlc_count: usize, + feerate_per_kw: u32, dust_exposure_limiting_feerate: Option, + ) -> NextCommitmentStats { + let next_commitment_htlcs = self.get_next_commitment_htlcs( + false, + htlc_candidate, + include_counterparty_unknown_htlcs, + ); + let next_value_to_self_msat = self.get_next_commitment_value_to_self_msat(false, funding); + SpecTxBuilder {}.get_next_commitment_stats( + false, + funding.is_outbound(), + funding.get_value_satoshis(), + next_value_to_self_msat, + &next_commitment_htlcs, + addl_nondust_htlc_count, + feerate_per_kw, + dust_exposure_limiting_feerate, + self.counterparty_dust_limit_satoshis, + funding.get_channel_type(), + ) + } + #[rustfmt::skip] fn validate_update_add_htlc( &self, funding: &FundingScope, msg: &msgs::UpdateAddHTLC, From 4bfdb35f0b32e8934a633b3838c902348be69797 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 31 Jul 2025 02:22:22 +0000 Subject: [PATCH 04/11] Improve prediction of commitment stats in `validate_update_add_htlc` `ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs on the next commitment will be all the HTLCs in `ChannelContext.pending_inbound_htlcs`, and `ChannelContext.pending_outbound_htlcs`, as well as all the outbound HTLC adds in the holding cell. This is an overestimate: * Outbound HTLC removals which have been ACK'ed by the counterparty will certainly not be present in any *next* commitment, even though they remain in `pending_outbound_htlcs`. * Outbound HTLCs in the `RemoteRemoved` state, will not be present in the next *local* commitment. * Outbound HTLCs in the `LocalAnnounced` state have no guarantee that they were received by the counterparty before she sent the `update_fee`. * Outbound `update_add_htlc`'s in the holding cell are certainly not known by the counterparty, and we will reevaluate their addition to the channel when freeing the holding cell. * Inbound HTLCs in the `LocalRemoved` state will not be present in the next *remote* commitment. `ChannelContext::next_local_commit_tx_fee_msat` over-counts outbound HTLCs in the `LocalAnnounced` and `RemoteRemoved` states, as well as outbound `update_add_htlc`'s in the holding cell. `ChannelContext::next_remote_commit_tx_fee_msat` over-counts inbound HTLCs in the `LocalRemoved` state, as well as outbound HTLCs in the `LocalAnnounced` state. This commit stops using these functions in favor of the newly added `ChannelContext::get_next_{local, remote}_commitment_stats` methods, and fixes the issues described above. If we are the funder, we also check that adding this inbound HTLC doesn't increase the commitment transaction fee to the point of exhausting our balance on the local commitment. Previously, we would only subtract the anchors from `funding.value_to_self_msat`; we now also subtract the outbound HTLCs on the next local commitment from `funding.value_to_self_msat` before checking if we can afford the additional transaction fees. Inbound `LocalRemoved` HTLCs that were **not** successful are now credited to `remote_balance_before_fee_msat` as they will certainly not be on the next remote commitment. We previously debited these from the remote balance to arrive at `remote_balance_before_fee_msat`. When calculating dust exposure, we now take a buffer from the currently committed feerate, and ignore any fee updates in `ChannelContext.pending_update_fee`. --- lightning/src/ln/channel.rs | 63 ++++++++++++------------------------- 1 file changed, 20 insertions(+), 43 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 0858bbe6cf5..09af6dc366c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1097,12 +1097,12 @@ pub enum AnnouncementSigsState { /// An enum indicating whether the local or remote side offered a given HTLC. enum HTLCInitiator { LocalOffered, + #[allow(dead_code)] RemoteOffered, } /// Current counts of various HTLCs, useful for calculating current balances available exactly. struct HTLCStats { - pending_inbound_htlcs: usize, pending_outbound_htlcs: usize, pending_inbound_htlcs_value_msat: u64, pending_outbound_htlcs_value_msat: u64, @@ -4111,7 +4111,6 @@ where /// /// We take the conservative approach and only assume that a HTLC will /// not be in the next commitment when it is guaranteed that it won't be. - #[allow(dead_code)] #[rustfmt::skip] fn get_next_commitment_htlcs( &self, local: bool, htlc_candidate: Option, include_counterparty_unknown_htlcs: bool, @@ -4181,7 +4180,6 @@ where /// will *not* be present on the next commitment from `next_commitment_htlcs`, and /// check if their outcome is successful. If it is, we add the value of this claimed /// HTLC to the balance of the claimer. - #[allow(dead_code)] #[rustfmt::skip] fn get_next_commitment_value_to_self_msat(&self, local: bool, funding: &FundingScope) -> u64 { let inbound_claimed_htlc_msat: u64 = @@ -4213,7 +4211,6 @@ where .saturating_add(inbound_claimed_htlc_msat) } - #[allow(dead_code)] fn get_next_local_commitment_stats( &self, funding: &FundingScope, htlc_candidate: Option, include_counterparty_unknown_htlcs: bool, addl_nondust_htlc_count: usize, @@ -4239,7 +4236,6 @@ where ) } - #[allow(dead_code)] fn get_next_remote_commitment_stats( &self, funding: &FundingScope, htlc_candidate: Option, include_counterparty_unknown_htlcs: bool, addl_nondust_htlc_count: usize, @@ -4280,15 +4276,25 @@ where let dust_exposure_limiting_feerate = self.get_dust_exposure_limiting_feerate( &fee_estimator, funding.get_channel_type(), ); - let htlc_stats = self.get_pending_htlc_stats(funding, None, dust_exposure_limiting_feerate); - if htlc_stats.pending_inbound_htlcs + 1 > self.holder_max_accepted_htlcs as usize { + // Don't include outbound update_add_htlc's in the holding cell, or those which haven't yet been ACK'ed by the counterparty (ie. LocalAnnounced HTLCs) + let include_counterparty_unknown_htlcs = false; + // Don't include the extra fee spike buffer HTLC in calculations + let fee_spike_buffer_htlc = 0; + let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, Some(HTLCAmountDirection { outbound: false, amount_msat: msg.amount_msat }), include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, self.feerate_per_kw, dust_exposure_limiting_feerate); + + if next_remote_commitment_stats.inbound_htlcs_count > self.holder_max_accepted_htlcs as usize { return Err(ChannelError::close(format!("Remote tried to push more than our max accepted HTLCs ({})", self.holder_max_accepted_htlcs))); } - if htlc_stats.pending_inbound_htlcs_value_msat + msg.amount_msat > self.holder_max_htlc_value_in_flight_msat { + if next_remote_commitment_stats.inbound_htlcs_value_msat > self.holder_max_htlc_value_in_flight_msat { return Err(ChannelError::close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.holder_max_htlc_value_in_flight_msat))); } - // Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet + let remote_balance_before_fee_msat = next_remote_commitment_stats.counterparty_balance_msat.ok_or(ChannelError::close("Remote HTLC add would overdraw remaining funds".to_owned()))?; + + // 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). + // + // We check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet // the reserve_satoshis we told them to always have as direct payment so that they lose // something if we punish them for broadcasting an old state). // Note that we don't really care about having a small/no to_remote output in our local @@ -4300,50 +4306,22 @@ where // violate the reserve value if we do not do this (as we forget inbound HTLCs from the // Channel state once they will not be present in the next received commitment // transaction). - let (local_balance_before_fee_msat, remote_balance_before_fee_msat) = { - let removed_outbound_total_msat: u64 = self.pending_outbound_htlcs - .iter() - .filter_map(|htlc| { - matches!( - htlc.state, - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _)) - | OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _)) - ) - .then_some(htlc.amount_msat) - }) - .sum(); - let pending_value_to_self_msat = - funding.value_to_self_msat + htlc_stats.pending_inbound_htlcs_value_msat - removed_outbound_total_msat; - let pending_remote_value_msat = - funding.get_value_satoshis() * 1000 - pending_value_to_self_msat; - - // Subtract any non-HTLC outputs from the local and remote balances - SpecTxBuilder {}.subtract_non_htlc_outputs(funding.is_outbound(), funding.value_to_self_msat, pending_remote_value_msat, funding.get_channel_type()) - }; - if remote_balance_before_fee_msat < msg.amount_msat { - return Err(ChannelError::close("Remote HTLC add would overdraw remaining funds".to_owned())); - } - - // 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 funding.is_outbound() { 0 } else { - let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered); - self.next_remote_commit_tx_fee_msat(funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations + next_remote_commitment_stats.commit_tx_fee_sat * 1000 }; - if remote_balance_before_fee_msat.saturating_sub(msg.amount_msat) < remote_commit_tx_fee_msat { + if remote_balance_before_fee_msat < remote_commit_tx_fee_msat { return Err(ChannelError::close("Remote HTLC add would not leave enough to pay for fees".to_owned())); }; - if remote_balance_before_fee_msat.saturating_sub(msg.amount_msat).saturating_sub(remote_commit_tx_fee_msat) < funding.holder_selected_channel_reserve_satoshis * 1000 { + if remote_balance_before_fee_msat.saturating_sub(remote_commit_tx_fee_msat) < funding.holder_selected_channel_reserve_satoshis * 1000 { return Err(ChannelError::close("Remote HTLC add would put them under remote reserve value".to_owned())); } } + let next_local_commitment_stats = self.get_next_local_commitment_stats(funding, Some(HTLCAmountDirection { outbound: false, amount_msat: msg.amount_msat }), include_counterparty_unknown_htlcs, 0, self.feerate_per_kw, dust_exposure_limiting_feerate); if funding.is_outbound() { // Check that they won't violate our local required channel reserve by adding this HTLC. - let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered); - let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(funding, htlc_candidate, None); - if local_balance_before_fee_msat < funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat { + if next_local_commitment_stats.holder_balance_msat.unwrap_or(0) < funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + next_local_commitment_stats.commit_tx_fee_sat * 1000 { return Err(ChannelError::close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned())); } } @@ -4956,7 +4934,6 @@ where }); HTLCStats { - pending_inbound_htlcs: self.pending_inbound_htlcs.len(), pending_outbound_htlcs, pending_inbound_htlcs_value_msat, pending_outbound_htlcs_value_msat, From db6dd94d958e356eecb4c03e76c6cecc812c7bab Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 31 Jul 2025 02:29:37 +0000 Subject: [PATCH 05/11] Improve prediction of commitment stats in `can_accept_incoming_htlc` `ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs on the next commitment will be all the HTLCs in `ChannelContext.pending_inbound_htlcs`, and `ChannelContext.pending_outbound_htlcs`, as well as all the outbound HTLC adds in the holding cell. This is an overestimate: * Outbound HTLC removals which have been ACK'ed by the counterparty will certainly not be present in any *next* commitment, even though they remain in `pending_outbound_htlcs`. * Outbound HTLCs in the `RemoteRemoved` state, will not be present in the next *local* commitment. * Outbound HTLCs in the `LocalAnnounced` state have no guarantee that they were yet received by the counterparty. * Outbound `update_add_htlc`'s in the holding cell are certainly not known by the counterparty, and we will reevaluate their addition to the channel when freeing the holding cell. * Inbound HTLCs in the `LocalRemoved` state will not be present in the next *remote* commitment. This commit stops using `get_pending_htlc_stats` in favor of the newly added `ChannelContext::get_next_{local, remote}_commitment_stats` methods, and fixes the issues described above. `ChannelContext::next_remote_commit_tx_fee_msat` counts inbound HTLCs in the `LocalRemoved` state, as well as outbound HTLCs in the `LocalAnnounced` state. We now do not count them for the same reasons described above. Inbound `LocalRemoved` HTLCs that were **not** successful are now credited to `remote_balance_before_fee_msat` as they will certainly not be on the next remote commitment. We previously debited these from the remote balance to arrive at `remote_balance_before_fee_msat`. We now always check holder dust exposure, whereas we previously would only do it if the incoming HTLC was dust on our own commitment transaction. Furthermore, dust exposure calculations now take a buffer from the currently committed feerate, and ignore any fee updates in `ChannelContext.pending_update_fee`. --- lightning/src/ln/channel.rs | 85 ++++++++++-------------------- lightning/src/ln/channelmanager.rs | 2 +- 2 files changed, 28 insertions(+), 59 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 09af6dc366c..8826818e867 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4486,78 +4486,47 @@ where #[rustfmt::skip] fn can_accept_incoming_htlc( - &self, funding: &FundingScope, msg: &msgs::UpdateAddHTLC, + &self, funding: &FundingScope, dust_exposure_limiting_feerate: Option, logger: &L, ) -> Result<(), LocalHTLCFailureReason> where L::Target: Logger, { - let htlc_stats = self.get_pending_htlc_stats(funding, None, dust_exposure_limiting_feerate); + // The fee spike buffer (an additional nondust HTLC) we keep for the remote if the channel + // is not zero fee. This deviates from the spec because the fee spike buffer requirement + // doesn't exist on the receiver's side, only on the sender's. Note that with anchor + // outputs we are no longer as sensitive to fee spikes, so we need to account for them. + let fee_spike_buffer_htlc = if funding.get_channel_type().supports_anchor_zero_fee_commitments() { + 0 + } else { + 1 + }; + // Do not include outbound update_add_htlc's in the holding cell, or those which haven't yet been ACK'ed by the counterparty (ie. LocalAnnounced HTLCs) + let include_counterparty_unknown_htlcs = false; + // A `None` `HTLCCandidate` is used as in this case because we're already accounting for + // the incoming HTLC as it has been fully committed by both sides. + let next_local_commitment_stats = self.get_next_local_commitment_stats(funding, None, include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, self.feerate_per_kw, dust_exposure_limiting_feerate); + let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, None, include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, self.feerate_per_kw, dust_exposure_limiting_feerate); + let max_dust_htlc_exposure_msat = self.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); - let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat; - if on_counterparty_tx_dust_htlc_exposure_msat > max_dust_htlc_exposure_msat { + if next_remote_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat { // Note that the total dust exposure includes both the dust HTLCs and the excess mining fees of the counterparty commitment transaction log_info!(logger, "Cannot accept value that would put our total dust exposure at {} over the limit {} on counterparty commitment tx", - on_counterparty_tx_dust_htlc_exposure_msat, max_dust_htlc_exposure_msat); + next_remote_commitment_stats.dust_exposure_msat, max_dust_htlc_exposure_msat); return Err(LocalHTLCFailureReason::DustLimitCounterparty) } - let dust_buffer_feerate = self.get_dust_buffer_feerate(None); - let (htlc_success_tx_fee_sat, _) = second_stage_tx_fees_sat( - &funding.get_channel_type(), dust_buffer_feerate, - ); - let exposure_dust_limit_success_sats = htlc_success_tx_fee_sat + self.holder_dust_limit_satoshis; - if msg.amount_msat / 1000 < exposure_dust_limit_success_sats { - let on_holder_tx_dust_htlc_exposure_msat = htlc_stats.on_holder_tx_dust_exposure_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); - return Err(LocalHTLCFailureReason::DustLimitHolder) - } + if next_local_commitment_stats.dust_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", + next_local_commitment_stats.dust_exposure_msat, max_dust_htlc_exposure_msat); + return Err(LocalHTLCFailureReason::DustLimitHolder) } if !funding.is_outbound() { - let removed_outbound_total_msat: u64 = self.pending_outbound_htlcs - .iter() - .filter_map(|htlc| { - matches!( - htlc.state, - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _)) - | OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _)) - ) - .then_some(htlc.amount_msat) - }) - .sum(); - let pending_value_to_self_msat = - funding.value_to_self_msat + htlc_stats.pending_inbound_htlcs_value_msat - removed_outbound_total_msat; - let pending_remote_value_msat = - funding.get_value_satoshis() * 1000 - pending_value_to_self_msat; - // Subtract any non-HTLC outputs from the local and remote balances - let (_, remote_balance_before_fee_msat) = SpecTxBuilder {}.subtract_non_htlc_outputs( - funding.is_outbound(), - pending_value_to_self_msat, - pending_remote_value_msat, - funding.get_channel_type() - ); - - // `Some(())` is for the fee spike buffer we keep for the remote if the channel is - // not zero fee. This deviates from the spec because the fee spike buffer requirement - // doesn't exist on the receiver's side, only on the sender's. Note that with anchor - // outputs we are no longer as sensitive to fee spikes, so we need to account for them. - // - // A `None` `HTLCCandidate` is used as in this case because we're already accounting for - // the incoming HTLC as it has been fully committed by both sides. - let fee_spike_buffer_htlc = if funding.get_channel_type().supports_anchor_zero_fee_commitments() { - None - } else { - Some(()) - }; - - let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat( - funding, None, fee_spike_buffer_htlc, - ); + let mut remote_fee_cost_incl_stuck_buffer_msat = next_remote_commitment_stats.commit_tx_fee_sat * 1000; if !funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { remote_fee_cost_incl_stuck_buffer_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE; } + let remote_balance_before_fee_msat = next_remote_commitment_stats.counterparty_balance_msat.unwrap_or(0); if remote_balance_before_fee_msat.saturating_sub(funding.holder_selected_channel_reserve_satoshis * 1000) < remote_fee_cost_incl_stuck_buffer_msat { log_info!(logger, "Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", &self.channel_id()); return Err(LocalHTLCFailureReason::FeeSpikeBuffer); @@ -9525,7 +9494,7 @@ where /// this function determines whether to fail the HTLC, or forward / claim it. #[rustfmt::skip] pub fn can_accept_incoming_htlc( - &self, msg: &msgs::UpdateAddHTLC, fee_estimator: &LowerBoundedFeeEstimator, logger: L + &self, fee_estimator: &LowerBoundedFeeEstimator, logger: L ) -> Result<(), LocalHTLCFailureReason> where F::Target: FeeEstimator, @@ -9541,7 +9510,7 @@ where core::iter::once(&self.funding) .chain(self.pending_funding.iter()) - .try_for_each(|funding| self.context.can_accept_incoming_htlc(funding, msg, dust_exposure_limiting_feerate, &logger)) + .try_for_each(|funding| self.context.can_accept_incoming_htlc(funding, dust_exposure_limiting_feerate, &logger)) } pub fn get_cur_holder_commitment_transaction_number(&self) -> u64 { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 8bac6c2fa3a..64ef1b531d1 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6262,7 +6262,7 @@ where &chan.context, Some(update_add_htlc.payment_hash), ); - chan.can_accept_incoming_htlc(update_add_htlc, &self.fee_estimator, &logger) + chan.can_accept_incoming_htlc(&self.fee_estimator, &logger) }, ) { Some(Ok(_)) => {}, From 797cf5b68688f27e2f5caecdb776d7bc64904ef2 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 31 Jul 2025 02:23:19 +0000 Subject: [PATCH 06/11] Improve prediction of commitment stats in `validate_update_fee` `ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs on the next commitment will be all the HTLCs in `ChannelContext.pending_inbound_htlcs`, and `ChannelContext.pending_outbound_htlcs`, as well as all the outbound HTLC adds in the holding cell. This is an overestimate: * Outbound HTLC removals which have been ACK'ed by the counterparty will certainly not be present in any *next* commitment, even though they remain in `pending_outbound_htlcs`. * Outbound HTLCs in the `RemoteRemoved` state, will not be present in the next *local* commitment. * Outbound HTLCs in the `LocalAnnounced` state have no guarantee that they were received by the counterparty before she sent the `update_fee`. * Outbound `update_add_htlc`'s in the holding cell are certainly not known by the counterparty, and we will reevaluate their addition to the channel when freeing the holding cell. * Inbound HTLCs in the `LocalRemoved` state will not be present in the next *remote* commitment. This commit stops using `get_pending_htlc_stats` in favor of the newly added `ChannelContext::get_next_{local, remote}_commitment_stats` methods, and fixes the issues described above. We now always calculate dust exposure using a buffer from `msg.feerate_per_kw`, and not from `max(self.feerate_per_kw, msg.feerate_per_kw)`. --- lightning/src/ln/channel.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8826818e867..64b453853d8 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4341,16 +4341,21 @@ where let dust_exposure_limiting_feerate = self.get_dust_exposure_limiting_feerate( &fee_estimator, funding.get_channel_type(), ); - let htlc_stats = self.get_pending_htlc_stats(funding, None, dust_exposure_limiting_feerate); + // Do not include outbound update_add_htlc's in the holding cell, or those which haven't yet been ACK'ed by the counterparty (ie. LocalAnnounced HTLCs) + let include_counterparty_unknown_htlcs = false; + let next_local_commitment_stats = self.get_next_local_commitment_stats(funding, None, include_counterparty_unknown_htlcs, 0, msg.feerate_per_kw, dust_exposure_limiting_feerate); + let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, None, include_counterparty_unknown_htlcs, 0, msg.feerate_per_kw, dust_exposure_limiting_feerate); + let max_dust_htlc_exposure_msat = self.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); - if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { + if next_local_commitment_stats.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))); + msg.feerate_per_kw, next_local_commitment_stats.dust_exposure_msat))); } - if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { + if next_remote_commitment_stats.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))); + msg.feerate_per_kw, next_remote_commitment_stats.dust_exposure_msat))); } + Ok(()) } From 991f24861e60e22325862c7c74d9cb70e87f322d Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 31 Jul 2025 02:24:54 +0000 Subject: [PATCH 07/11] Improve prediction of commitment stats in `can_send_send_update_fee` `ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs on the next commitment will be all the HTLCs in `ChannelContext.pending_inbound_htlcs`, and `ChannelContext.pending_outbound_htlcs`, as well as all the outbound HTLC adds in the holding cell. This is an overestimate: * Outbound HTLC removals which have been ACK'ed by the counterparty will certainly not be present in any *next* commitment, even though they remain in `pending_outbound_htlcs` (I refer to states `AwaitingRemoteRevokeToRemove` and `AwaitingRemovedRemoteRevoke`). * Outbound HTLCs in the `RemoteRemoved` state, will not be present in the next *local* commitment. * Inbound HTLCs in the `LocalRemoved` state will not be present in the next *remote* commitment. `ChannelContext::build_commitment_stats(funding, true, true, ..)` makes these errors when predicting the HTLC count on the remote commitment: * Inbound HTLCs in the state `RemoteAnnounced` are not included, but they will be in the next remote commitment transaction if the local ACK's the addition before producing the next remote commitment. * Inbound HTLCs in the state `AwaitingRemoteRevokeToAnnounce` are not included, even though the local has ACK'ed the addition. * Outbound HTLCs in the state `AwaitingRemoteRevokeToRemove` are counted, even though the local party has ACK'ed the removal. This commit replaces these functions in favor of the newly added `ChannelContext::get_next_{local, remote}_commitment_stats` methods, and fixes the issues described above. We now always calculate dust exposure using a buffer from `msg.feerate_per_kw`, and not from `max(feerate_per_kw, self.feerate_per_kw, self.pending_update_fee)`. --- lightning/src/ln/channel.rs | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 64b453853d8..3dcd5774ae4 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1112,8 +1112,6 @@ struct HTLCStats { // htlc on the counterparty's commitment transaction. extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat: Option, on_holder_tx_dust_exposure_msat: u64, - outbound_holding_cell_msat: u64, - on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included } /// A struct gathering data on a commitment, either local or remote. @@ -4465,11 +4463,11 @@ where let dust_exposure_limiting_feerate = self.get_dust_exposure_limiting_feerate( &fee_estimator, funding.get_channel_type(), ); - let htlc_stats = self.get_pending_htlc_stats(funding, Some(feerate_per_kw), dust_exposure_limiting_feerate); - let stats = self.build_commitment_stats(funding, true, true, Some(feerate_per_kw), Some(htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize)); - let holder_balance_msat = stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat; + // Include outbound update_add_htlc's in the holding cell, and those which haven't yet been ACK'ed by the counterparty (ie. LocalAnnounced HTLCs) + let include_counterparty_unknown_htlcs = true; + let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, None, include_counterparty_unknown_htlcs, CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, feerate_per_kw, dust_exposure_limiting_feerate); // Note that `stats.commit_tx_fee_sat` accounts for any HTLCs that transition from non-dust to dust under a higher feerate (in the case where HTLC-transactions pay endogenous fees). - if holder_balance_msat < stats.commit_tx_fee_sat * 1000 + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { + if next_remote_commitment_stats.holder_balance_msat.unwrap_or(0) < next_remote_commitment_stats.commit_tx_fee_sat * 1000 + funding.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); return false; @@ -4477,11 +4475,12 @@ where // Note, we evaluate pending htlc "preemptive" trimmed-to-dust threshold at the proposed `feerate_per_kw`. let max_dust_htlc_exposure_msat = self.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); - if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { + if next_remote_commitment_stats.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 false; } - if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { + + if next_remote_commitment_stats.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 false; } @@ -4845,8 +4844,6 @@ where } 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_tx_fee_sat + context.counterparty_dust_limit_satoshis; @@ -4867,7 +4864,6 @@ where 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; } else { @@ -4875,8 +4871,6 @@ where } 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; } } } @@ -4914,8 +4908,6 @@ where on_counterparty_tx_dust_exposure_msat, extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat, on_holder_tx_dust_exposure_msat, - outbound_holding_cell_msat, - on_holder_tx_outbound_holding_cell_htlcs_count, } } From 205c4fc88c27c6c579310480d8f13f2c362c2946 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 31 Jul 2025 02:32:08 +0000 Subject: [PATCH 08/11] Add validation of the fees predicted by `next_commitment_stats` Anytime we build a (feerate, nondust-htlc-count, fee) pair, cache it, and check that the fee matches if the feerate and nondust-htlc-count match when building a commitment transaction. --- lightning/src/ln/channel.rs | 108 ++++++++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 3dcd5774ae4..20a5483b225 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1984,6 +1984,10 @@ pub(super) struct FundingScope { next_local_commitment_tx_fee_info_cached: Mutex>, #[cfg(any(test, fuzzing))] next_remote_commitment_tx_fee_info_cached: Mutex>, + #[cfg(any(test, fuzzing))] + next_local_fee: Mutex, + #[cfg(any(test, fuzzing))] + next_remote_fee: Mutex, pub(super) channel_transaction_parameters: ChannelTransactionParameters, @@ -2060,6 +2064,10 @@ impl Readable for FundingScope { next_local_commitment_tx_fee_info_cached: Mutex::new(None), #[cfg(any(test, fuzzing))] next_remote_commitment_tx_fee_info_cached: Mutex::new(None), + #[cfg(any(test, fuzzing))] + next_local_fee: Mutex::new(PredictedNextFee::default()), + #[cfg(any(test, fuzzing))] + next_remote_fee: Mutex::new(PredictedNextFee::default()), }) } } @@ -3206,6 +3214,10 @@ where next_local_commitment_tx_fee_info_cached: Mutex::new(None), #[cfg(any(test, fuzzing))] next_remote_commitment_tx_fee_info_cached: Mutex::new(None), + #[cfg(any(test, fuzzing))] + next_local_fee: Mutex::new(PredictedNextFee::default()), + #[cfg(any(test, fuzzing))] + next_remote_fee: Mutex::new(PredictedNextFee::default()), channel_transaction_parameters: ChannelTransactionParameters { holder_pubkeys: pubkeys, @@ -3449,6 +3461,10 @@ where next_local_commitment_tx_fee_info_cached: Mutex::new(None), #[cfg(any(test, fuzzing))] next_remote_commitment_tx_fee_info_cached: Mutex::new(None), + #[cfg(any(test, fuzzing))] + next_local_fee: Mutex::new(PredictedNextFee::default()), + #[cfg(any(test, fuzzing))] + next_remote_fee: Mutex::new(PredictedNextFee::default()), channel_transaction_parameters: ChannelTransactionParameters { holder_pubkeys: pubkeys, @@ -4220,7 +4236,8 @@ where include_counterparty_unknown_htlcs, ); let next_value_to_self_msat = self.get_next_commitment_value_to_self_msat(true, funding); - SpecTxBuilder {}.get_next_commitment_stats( + + let ret = SpecTxBuilder {}.get_next_commitment_stats( true, funding.is_outbound(), funding.get_value_satoshis(), @@ -4231,7 +4248,38 @@ where dust_exposure_limiting_feerate, self.holder_dust_limit_satoshis, funding.get_channel_type(), - ) + ); + + #[cfg(any(test, fuzzing))] + { + if addl_nondust_htlc_count == 0 { + *funding.next_local_fee.lock().unwrap() = PredictedNextFee { + predicted_feerate: feerate_per_kw, + predicted_nondust_htlc_count: ret.nondust_htlc_count, + predicted_fee_sat: ret.commit_tx_fee_sat, + }; + } else { + let predicted_stats = SpecTxBuilder {}.get_next_commitment_stats( + true, + funding.is_outbound(), + funding.get_value_satoshis(), + next_value_to_self_msat, + &next_commitment_htlcs, + 0, + feerate_per_kw, + dust_exposure_limiting_feerate, + self.holder_dust_limit_satoshis, + funding.get_channel_type(), + ); + *funding.next_local_fee.lock().unwrap() = PredictedNextFee { + predicted_feerate: feerate_per_kw, + predicted_nondust_htlc_count: predicted_stats.nondust_htlc_count, + predicted_fee_sat: predicted_stats.commit_tx_fee_sat, + }; + } + } + + ret } fn get_next_remote_commitment_stats( @@ -4245,7 +4293,8 @@ where include_counterparty_unknown_htlcs, ); let next_value_to_self_msat = self.get_next_commitment_value_to_self_msat(false, funding); - SpecTxBuilder {}.get_next_commitment_stats( + + let ret = SpecTxBuilder {}.get_next_commitment_stats( false, funding.is_outbound(), funding.get_value_satoshis(), @@ -4256,7 +4305,38 @@ where dust_exposure_limiting_feerate, self.counterparty_dust_limit_satoshis, funding.get_channel_type(), - ) + ); + + #[cfg(any(test, fuzzing))] + { + if addl_nondust_htlc_count == 0 { + *funding.next_remote_fee.lock().unwrap() = PredictedNextFee { + predicted_feerate: feerate_per_kw, + predicted_nondust_htlc_count: ret.nondust_htlc_count, + predicted_fee_sat: ret.commit_tx_fee_sat, + }; + } else { + let predicted_stats = SpecTxBuilder {}.get_next_commitment_stats( + false, + funding.is_outbound(), + funding.get_value_satoshis(), + next_value_to_self_msat, + &next_commitment_htlcs, + 0, + feerate_per_kw, + dust_exposure_limiting_feerate, + self.counterparty_dust_limit_satoshis, + funding.get_channel_type(), + ); + *funding.next_remote_fee.lock().unwrap() = PredictedNextFee { + predicted_feerate: feerate_per_kw, + predicted_nondust_htlc_count: predicted_stats.nondust_htlc_count, + predicted_fee_sat: predicted_stats.commit_tx_fee_sat, + }; + } + } + + ret } #[rustfmt::skip] @@ -4413,6 +4493,10 @@ where } } } + let PredictedNextFee { predicted_feerate, predicted_nondust_htlc_count, predicted_fee_sat } = *funding.next_local_fee.lock().unwrap(); + if predicted_feerate == commitment_data.tx.feerate_per_kw() && predicted_nondust_htlc_count == commitment_data.tx.nondust_htlcs().len() { + assert_eq!(predicted_fee_sat, commitment_data.stats.commit_tx_fee_sat); + } } if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() { @@ -6026,6 +6110,14 @@ struct CommitmentTxInfoCached { feerate: u32, } +#[cfg(any(test, fuzzing))] +#[derive(Clone, Copy, Default)] +struct PredictedNextFee { + predicted_feerate: u32, + predicted_nondust_htlc_count: usize, + predicted_fee_sat: u64, +} + /// Contents of a wire message that fails an HTLC backwards. Useful for [`FundedChannel::fail_htlc`] to /// fail with either [`msgs::UpdateFailMalformedHTLC`] or [`msgs::UpdateFailHTLC`] as needed. trait FailHTLCContents { @@ -10990,6 +11082,10 @@ where } } } + let PredictedNextFee { predicted_feerate, predicted_nondust_htlc_count, predicted_fee_sat } = *funding.next_remote_fee.lock().unwrap(); + if predicted_feerate == counterparty_commitment_tx.feerate_per_kw() && predicted_nondust_htlc_count == counterparty_commitment_tx.nondust_htlcs().len() { + assert_eq!(predicted_fee_sat, commitment_data.stats.commit_tx_fee_sat); + } } (commitment_data.htlcs_included, counterparty_commitment_tx) @@ -13615,6 +13711,10 @@ where next_local_commitment_tx_fee_info_cached: Mutex::new(None), #[cfg(any(test, fuzzing))] next_remote_commitment_tx_fee_info_cached: Mutex::new(None), + #[cfg(any(test, fuzzing))] + next_local_fee: Mutex::new(PredictedNextFee::default()), + #[cfg(any(test, fuzzing))] + next_remote_fee: Mutex::new(PredictedNextFee::default()), channel_transaction_parameters: channel_parameters, funding_transaction, From 83ff8986107a6fcf988c3580e9ca6e3e74fc2fb0 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 31 Jul 2025 03:24:17 +0000 Subject: [PATCH 09/11] Delete dead `next_{local, remote}_commitment_tx_fee_info_cached` The cached fee is never checked in the current test suite. --- lightning/src/ln/channel.rs | 124 ++---------------------------------- 1 file changed, 5 insertions(+), 119 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 20a5483b225..c949ff2985a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1976,14 +1976,9 @@ pub(super) struct FundingScope { /// Max to_local and to_remote outputs in a remote-generated commitment transaction counterparty_max_commitment_tx_output: Mutex<(u64, u64)>, - // We save these values so we can make sure `next_local_commit_tx_fee_msat` and - // `next_remote_commit_tx_fee_msat` properly predict what the next commitment transaction fee will - // be, by comparing the cached values to the fee of the transaction generated by - // `build_commitment_transaction`. - #[cfg(any(test, fuzzing))] - next_local_commitment_tx_fee_info_cached: Mutex>, - #[cfg(any(test, fuzzing))] - next_remote_commitment_tx_fee_info_cached: Mutex>, + // We save these values so we can make sure validation of channel updates properly predicts + // what the next commitment transaction fee will be, by comparing the cached values to the + // fee of the transaction generated by `build_commitment_transaction`. #[cfg(any(test, fuzzing))] next_local_fee: Mutex, #[cfg(any(test, fuzzing))] @@ -2061,10 +2056,6 @@ impl Readable for FundingScope { short_channel_id, minimum_depth_override, #[cfg(any(test, fuzzing))] - next_local_commitment_tx_fee_info_cached: Mutex::new(None), - #[cfg(any(test, fuzzing))] - next_remote_commitment_tx_fee_info_cached: Mutex::new(None), - #[cfg(any(test, fuzzing))] next_local_fee: Mutex::new(PredictedNextFee::default()), #[cfg(any(test, fuzzing))] next_remote_fee: Mutex::new(PredictedNextFee::default()), @@ -3210,10 +3201,6 @@ where #[cfg(debug_assertions)] counterparty_max_commitment_tx_output: Mutex::new((value_to_self_msat, (channel_value_satoshis * 1000 - msg_push_msat).saturating_sub(value_to_self_msat))), - #[cfg(any(test, fuzzing))] - next_local_commitment_tx_fee_info_cached: Mutex::new(None), - #[cfg(any(test, fuzzing))] - next_remote_commitment_tx_fee_info_cached: Mutex::new(None), #[cfg(any(test, fuzzing))] next_local_fee: Mutex::new(PredictedNextFee::default()), #[cfg(any(test, fuzzing))] @@ -3457,10 +3444,6 @@ where #[cfg(debug_assertions)] counterparty_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)), - #[cfg(any(test, fuzzing))] - next_local_commitment_tx_fee_info_cached: Mutex::new(None), - #[cfg(any(test, fuzzing))] - next_remote_commitment_tx_fee_info_cached: Mutex::new(None), #[cfg(any(test, fuzzing))] next_local_fee: Mutex::new(PredictedNextFee::default()), #[cfg(any(test, fuzzing))] @@ -4479,20 +4462,6 @@ where } #[cfg(any(test, fuzzing))] { - if funding.is_outbound() { - let projected_commit_tx_info = funding.next_local_commitment_tx_fee_info_cached.lock().unwrap().take(); - *funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None; - if let Some(info) = projected_commit_tx_info { - let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len() - + self.holding_cell_htlc_updates.len(); - if info.total_pending_htlcs == total_pending_htlcs - && info.next_holder_htlc_id == self.next_holder_htlc_id - && info.next_counterparty_htlc_id == self.next_counterparty_htlc_id - && info.feerate == self.feerate_per_kw { - assert_eq!(commitment_data.stats.commit_tx_fee_sat, info.fee / 1000); - } - } - } let PredictedNextFee { predicted_feerate, predicted_nondust_htlc_count, predicted_fee_sat } = *funding.next_local_fee.lock().unwrap(); if predicted_feerate == commitment_data.tx.feerate_per_kw() && predicted_nondust_htlc_count == commitment_data.tx.nondust_htlcs().len() { assert_eq!(predicted_fee_sat, commitment_data.stats.commit_tx_fee_sat); @@ -5322,31 +5291,7 @@ where } let num_htlcs = included_htlcs + addl_htlcs; - let commit_tx_fee_msat = SpecTxBuilder {}.commit_tx_fee_sat(context.feerate_per_kw, num_htlcs, funding.get_channel_type()) * 1000; - #[cfg(any(test, fuzzing))] - { - let mut fee = commit_tx_fee_msat; - if fee_spike_buffer_htlc.is_some() { - fee = SpecTxBuilder {}.commit_tx_fee_sat(context.feerate_per_kw, num_htlcs - 1, funding.get_channel_type()) * 1000; - } - let total_pending_htlcs = context.pending_inbound_htlcs.len() + context.pending_outbound_htlcs.len() - + context.holding_cell_htlc_updates.len(); - let commitment_tx_info = CommitmentTxInfoCached { - fee, - total_pending_htlcs, - next_holder_htlc_id: match htlc.origin { - HTLCInitiator::LocalOffered => context.next_holder_htlc_id + 1, - HTLCInitiator::RemoteOffered => context.next_holder_htlc_id, - }, - next_counterparty_htlc_id: match htlc.origin { - HTLCInitiator::LocalOffered => context.next_counterparty_htlc_id, - HTLCInitiator::RemoteOffered => context.next_counterparty_htlc_id + 1, - }, - feerate: context.feerate_per_kw, - }; - *funding.next_local_commitment_tx_fee_info_cached.lock().unwrap() = Some(commitment_tx_info); - } - commit_tx_fee_msat + SpecTxBuilder {}.commit_tx_fee_sat(context.feerate_per_kw, num_htlcs, funding.get_channel_type()) * 1000 } /// Get the commitment tx fee for the remote's next commitment transaction based on the number of @@ -5423,30 +5368,7 @@ where } let num_htlcs = included_htlcs + addl_htlcs; - let commit_tx_fee_msat = SpecTxBuilder {}.commit_tx_fee_sat(context.feerate_per_kw, num_htlcs, funding.get_channel_type()) * 1000; - #[cfg(any(test, fuzzing))] - if let Some(htlc) = &htlc { - let mut fee = commit_tx_fee_msat; - if fee_spike_buffer_htlc.is_some() { - fee = SpecTxBuilder {}.commit_tx_fee_sat(context.feerate_per_kw, num_htlcs - 1, funding.get_channel_type()) * 1000; - } - let total_pending_htlcs = context.pending_inbound_htlcs.len() + context.pending_outbound_htlcs.len(); - let commitment_tx_info = CommitmentTxInfoCached { - fee, - total_pending_htlcs, - next_holder_htlc_id: match htlc.origin { - HTLCInitiator::LocalOffered => context.next_holder_htlc_id + 1, - HTLCInitiator::RemoteOffered => context.next_holder_htlc_id, - }, - next_counterparty_htlc_id: match htlc.origin { - HTLCInitiator::LocalOffered => context.next_counterparty_htlc_id, - HTLCInitiator::RemoteOffered => context.next_counterparty_htlc_id + 1, - }, - feerate: context.feerate_per_kw, - }; - *funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = Some(commitment_tx_info); - } - commit_tx_fee_msat + SpecTxBuilder {}.commit_tx_fee_sat(context.feerate_per_kw, num_htlcs, funding.get_channel_type()) * 1000 } #[rustfmt::skip] @@ -6101,15 +6023,6 @@ macro_rules! promote_splice_funding { }; } -#[cfg(any(test, fuzzing))] -struct CommitmentTxInfoCached { - fee: u64, - total_pending_htlcs: usize, - next_holder_htlc_id: u64, - next_counterparty_htlc_id: u64, - feerate: u32, -} - #[cfg(any(test, fuzzing))] #[derive(Clone, Copy, Default)] struct PredictedNextFee { @@ -7618,16 +7531,6 @@ where return Err(ChannelError::close("Received an unexpected revoke_and_ack".to_owned())); } - #[cfg(any(test, fuzzing))] - { - for funding in - core::iter::once(&mut self.funding).chain(self.pending_funding.iter_mut()) - { - *funding.next_local_commitment_tx_fee_info_cached.lock().unwrap() = None; - *funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None; - } - } - match &self.context.holder_signer { ChannelSignerType::Ecdsa(ecdsa) => { ecdsa @@ -11069,19 +10972,6 @@ where #[cfg(any(test, fuzzing))] { - if !funding.is_outbound() { - let projected_commit_tx_info = funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap().take(); - *funding.next_local_commitment_tx_fee_info_cached.lock().unwrap() = None; - if let Some(info) = projected_commit_tx_info { - let total_pending_htlcs = self.context.pending_inbound_htlcs.len() + self.context.pending_outbound_htlcs.len(); - if info.total_pending_htlcs == total_pending_htlcs - && info.next_holder_htlc_id == self.context.next_holder_htlc_id - && info.next_counterparty_htlc_id == self.context.next_counterparty_htlc_id - && info.feerate == self.context.feerate_per_kw { - assert_eq!(commitment_data.stats.commit_tx_fee_sat, info.fee); - } - } - } let PredictedNextFee { predicted_feerate, predicted_nondust_htlc_count, predicted_fee_sat } = *funding.next_remote_fee.lock().unwrap(); if predicted_feerate == counterparty_commitment_tx.feerate_per_kw() && predicted_nondust_htlc_count == counterparty_commitment_tx.nondust_htlcs().len() { assert_eq!(predicted_fee_sat, commitment_data.stats.commit_tx_fee_sat); @@ -13707,10 +13597,6 @@ where #[cfg(debug_assertions)] counterparty_max_commitment_tx_output: Mutex::new((0, 0)), - #[cfg(any(test, fuzzing))] - next_local_commitment_tx_fee_info_cached: Mutex::new(None), - #[cfg(any(test, fuzzing))] - next_remote_commitment_tx_fee_info_cached: Mutex::new(None), #[cfg(any(test, fuzzing))] next_local_fee: Mutex::new(PredictedNextFee::default()), #[cfg(any(test, fuzzing))] From 049e3cb7cf20a599dcfe332c258b1edab5504571 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 13 Aug 2025 16:08:04 +0000 Subject: [PATCH 10/11] fixup: Add `TxBuilder::get_next_commitment_stats` Do not panic if the counterparty adds a HTLC with a value greater than its remaining balance! --- lightning/src/sign/tx_builder.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index 6617633e1bb..faaad14af18 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -73,8 +73,8 @@ fn excess_fees_on_counterparty_tx_dust_exposure_msat( } fn subtract_addl_outputs( - is_outbound_from_holder: bool, value_to_self_after_htlcs: u64, - value_to_remote_after_htlcs: u64, channel_type: &ChannelTypeFeatures, + is_outbound_from_holder: bool, value_to_self_after_htlcs: Option, + value_to_remote_after_htlcs: Option, channel_type: &ChannelTypeFeatures, ) -> (Option, Option) { let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 @@ -90,15 +90,16 @@ fn subtract_addl_outputs( // cover the total anchor sum. let local_balance_before_fee_msat = if is_outbound_from_holder { - value_to_self_after_htlcs.checked_sub(total_anchors_sat * 1000) + value_to_self_after_htlcs.and_then(|balance| balance.checked_sub(total_anchors_sat * 1000)) } else { - Some(value_to_self_after_htlcs) + value_to_self_after_htlcs }; let remote_balance_before_fee_msat = if !is_outbound_from_holder { - value_to_remote_after_htlcs.checked_sub(total_anchors_sat * 1000) + value_to_remote_after_htlcs + .and_then(|balance| balance.checked_sub(total_anchors_sat * 1000)) } else { - Some(value_to_remote_after_htlcs) + value_to_remote_after_htlcs }; (local_balance_before_fee_msat, remote_balance_before_fee_msat) @@ -176,12 +177,13 @@ impl TxBuilder for SpecTxBuilder { .iter() .filter_map(|htlc| (!htlc.outbound).then_some(htlc.amount_msat)) .sum(); - let value_to_holder_after_htlcs = value_to_holder_msat - .checked_sub(outbound_htlcs_value_msat) - .expect("outbound_htlcs_value_msat is greater than value_to_holder_msat"); - let value_to_counterparty_after_htlcs = value_to_counterparty_msat - .checked_sub(inbound_htlcs_value_msat) - .expect("inbound_htlcs_value_msat is greater than value_to_counterparty_msat"); + // Note there is no guarantee that the subtractions of the HTLC amounts don't + // overflow, so we do not panic. Instead, we return `None` to signal an overflow + // to channel, and let channel take the appropriate action. + let value_to_holder_after_htlcs = + value_to_holder_msat.checked_sub(outbound_htlcs_value_msat); + let value_to_counterparty_after_htlcs = + value_to_counterparty_msat.checked_sub(inbound_htlcs_value_msat); // Subtract the anchors from the channel funder let (holder_balance_msat, counterparty_balance_msat) = subtract_addl_outputs( From b4759445697f8ab205bf2cae432190896f7a1a66 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Tue, 12 Aug 2025 21:21:10 +0000 Subject: [PATCH 11/11] fixup: Add `TxBuilder::get_next_commitment_stats` For consistency, include the number of additional nondust HTLCs in the total nondust HTLC count; commit_tx_fee_sat also includes these. --- lightning/src/sign/tx_builder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index faaad14af18..7644f55e363 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -245,7 +245,7 @@ impl TxBuilder for SpecTxBuilder { inbound_htlcs_value_msat, holder_balance_msat, counterparty_balance_msat, - nondust_htlc_count, + nondust_htlc_count: nondust_htlc_count + addl_nondust_htlc_count, commit_tx_fee_sat, dust_exposure_msat, extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat,