From 63abd10a20ac062b4b145b22416fa5777e90c434 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 28 Aug 2025 01:43:47 +0000 Subject: [PATCH 1/7] Make `TxBuilder::get_next_commitment_stats` fallible Anytime we ask `TxBuilder` for stats on a commitment transaction, `TxBuilder` can now return an error to indicate that a balance not including the commitment transaction fee has been overdrawn. We then map this error to the appropriate action depending on where in the life cycle of the channel the error occurred. We now do not require that `channel_value_satoshis * 1000` is greater than or equal to `value_to_holder_msat`; we previously would panic in this case. --- lightning/src/ln/channel.rs | 307 ++++++++++++++++++------------- lightning/src/ln/onion_utils.rs | 11 +- lightning/src/sign/tx_builder.rs | 70 ++++--- 3 files changed, 213 insertions(+), 175 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 08ca1d34362..53da7b42ac5 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4478,7 +4478,7 @@ where &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 { + ) -> Result { let next_commitment_htlcs = self.get_next_commitment_htlcs( true, htlc_candidate, @@ -4497,7 +4497,7 @@ where dust_exposure_limiting_feerate, self.holder_dust_limit_satoshis, funding.get_channel_type(), - ); + )?; #[cfg(any(test, fuzzing))] { @@ -4508,18 +4508,20 @@ where 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(), - ); + 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(), + ) + .expect("Balance after HTLCs and anchors exhausted on local commitment"); *funding.next_local_fee.lock().unwrap() = PredictedNextFee { predicted_feerate: feerate_per_kw, predicted_nondust_htlc_count: predicted_stats.nondust_htlc_count, @@ -4528,14 +4530,14 @@ where } } - ret + Ok(ret) } 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 { + ) -> Result { let next_commitment_htlcs = self.get_next_commitment_htlcs( false, htlc_candidate, @@ -4554,7 +4556,7 @@ where dust_exposure_limiting_feerate, self.counterparty_dust_limit_satoshis, funding.get_channel_type(), - ); + )?; #[cfg(any(test, fuzzing))] { @@ -4565,18 +4567,20 @@ where 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(), - ); + 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(), + ) + .expect("Balance after HTLCs and anchors exhausted on remote commitment"); *funding.next_remote_fee.lock().unwrap() = PredictedNextFee { predicted_feerate: feerate_per_kw, predicted_nondust_htlc_count: predicted_stats.nondust_htlc_count, @@ -4585,7 +4589,7 @@ where } } - ret + Ok(ret) } fn validate_update_add_htlc( @@ -4608,14 +4612,18 @@ where 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, - ); + 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, + ) + .map_err(|()| { + ChannelError::close(String::from("Remote HTLC add would overdraw remaining funds")) + })?; if next_remote_commitment_stats.inbound_htlcs_count > self.holder_max_accepted_htlcs as usize @@ -4634,11 +4642,6 @@ where ))); } - let remote_balance_before_fee_msat = - next_remote_commitment_stats.counterparty_balance_before_fee_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). // @@ -4660,12 +4663,16 @@ where } else { next_remote_commitment_stats.commit_tx_fee_sat * 1000 }; - if remote_balance_before_fee_msat < remote_commit_tx_fee_msat { + if next_remote_commitment_stats.counterparty_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(remote_commit_tx_fee_msat) + if next_remote_commitment_stats + .counterparty_balance_before_fee_msat + .saturating_sub(remote_commit_tx_fee_msat) < funding.holder_selected_channel_reserve_satoshis * 1000 { return Err(ChannelError::close( @@ -4675,20 +4682,22 @@ where } if funding.is_outbound() { - 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, - fee_spike_buffer_htlc, - self.feerate_per_kw, - dust_exposure_limiting_feerate, - ); - let holder_balance_msat = - next_local_commitment_stats.holder_balance_before_fee_msat.expect( - "Adding an inbound HTLC should never exhaust the holder's balance before fees", - ); + 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, + fee_spike_buffer_htlc, + self.feerate_per_kw, + dust_exposure_limiting_feerate, + ) + .map_err(|()| { + ChannelError::close(String::from( + "Balance after HTLCs and anchors exhausted on local commitment", + )) + })?; // Check that they won't violate our local required channel reserve by adding this HTLC. - if holder_balance_msat + if next_local_commitment_stats.holder_balance_before_fee_msat < funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + next_local_commitment_stats.commit_tx_fee_sat * 1000 { @@ -4714,22 +4723,34 @@ where // 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 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, + ) + .map_err(|()| { + ChannelError::close(String::from( + "Balance after HTLCs and anchors exhausted on local commitment", + )) + })?; + 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, + ) + .map_err(|()| { + ChannelError::close(String::from( + "Balance after HTLCs and anchors exhausted on remote commitment", + )) + })?; let max_dust_htlc_exposure_msat = self.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); @@ -4926,20 +4947,25 @@ where // 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( + let next_remote_commitment_stats = if let Ok(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, - ); - let holder_balance_msat = next_remote_commitment_stats - .holder_balance_before_fee_msat - .expect("The holder's balance before fees should never underflow."); + ) { + stats + } else { + log_debug!( + logger, + "Cannot afford to send new feerate due to balance after HTLCs and anchors exhausted on remote commitment", + ); + return false; + }; // 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 + if next_remote_commitment_stats.holder_balance_before_fee_msat < next_remote_commitment_stats.commit_tx_fee_sat * 1000 + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { @@ -4961,14 +4987,22 @@ where return false; } - let next_local_commitment_stats = self.get_next_local_commitment_stats( + let next_local_commitment_stats = if let Ok(stats) = self.get_next_local_commitment_stats( funding, None, include_counterparty_unknown_htlcs, CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, feerate_per_kw, dust_exposure_limiting_feerate, - ); + ) { + stats + } else { + log_debug!( + logger, + "Cannot afford to send new feerate due to balance after HTLCs and anchors exhausted on local commitment", + ); + return false; + }; if next_local_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat { log_debug!( logger, @@ -4997,22 +5031,32 @@ where 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 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, + ) + .map_err(|()| { + log_trace!(logger, "Attempting to fail HTLC due to balance after HTLCs and anchors exhausted on local commitment"); + LocalHTLCFailureReason::ChannelBalanceOverdrawn + })?; + 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, + ) + .map_err(|()| { + log_trace!(logger, "Attempting to fail HTLC due to balance after HTLCs and anchors exhausted on remote commitment"); + LocalHTLCFailureReason::ChannelBalanceOverdrawn + })?; let max_dust_htlc_exposure_msat = self.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); @@ -5046,12 +5090,8 @@ where remote_fee_incl_fee_spike_buffer_htlc_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE; } - // We unwrap here; if the HTLC exhausts the counterparty's balance, we should have rejected it - // at `update_add_htlc`, here the HTLC is already irrevocably committed to the channel. - let remote_balance_before_fee_msat = next_remote_commitment_stats + if next_remote_commitment_stats .counterparty_balance_before_fee_msat - .expect("The counterparty's balance before fees should never underflow"); - if remote_balance_before_fee_msat .saturating_sub(funding.holder_selected_channel_reserve_satoshis * 1000) < remote_fee_incl_fee_spike_buffer_htlc_msat { @@ -11950,43 +11990,44 @@ where // nondust HTLC on the channel. let addl_nondust_htlc_count = 1; - let local_commitment_stats = self.context.get_next_local_commitment_stats( - funding, - None, // htlc_candidate - include_counterparty_unknown_htlcs, - addl_nondust_htlc_count, - self.context.feerate_per_kw, - dust_exposure_limiting_feerate, - ); + let local_commitment_stats = self + .context + .get_next_local_commitment_stats( + funding, + None, // htlc_candidate + include_counterparty_unknown_htlcs, + addl_nondust_htlc_count, + self.context.feerate_per_kw, + dust_exposure_limiting_feerate, + ) + .map_err(|()| "Balance after HTLCs and anchors exhausted on local commitment")?; let (holder_balance_on_local_msat, counterparty_balance_on_local_msat) = - local_commitment_stats.get_holder_counterparty_balances_incl_fee_msat(); + local_commitment_stats + .get_holder_counterparty_balances_incl_fee_msat() + .map_err(|()| "Channel funder cannot afford the fee on local commitment")?; - let remote_commitment_stats = self.context.get_next_remote_commitment_stats( - funding, - None, // htlc_candidate - include_counterparty_unknown_htlcs, - addl_nondust_htlc_count, - self.context.feerate_per_kw, - dust_exposure_limiting_feerate, - ); + let remote_commitment_stats = self + .context + .get_next_remote_commitment_stats( + funding, + None, // htlc_candidate + include_counterparty_unknown_htlcs, + addl_nondust_htlc_count, + self.context.feerate_per_kw, + dust_exposure_limiting_feerate, + ) + .map_err(|()| "Balance after HTLCs and anchors exhausted on remote commitment")?; let (holder_balance_on_remote_msat, counterparty_balance_on_remote_msat) = - remote_commitment_stats.get_holder_counterparty_balances_incl_fee_msat(); + remote_commitment_stats + .get_holder_counterparty_balances_incl_fee_msat() + .map_err(|()| "Channel funder cannot afford the fee on remote commitment")?; let holder_balance_floor = Amount::from_sat( - cmp::min( - holder_balance_on_local_msat - .ok_or("holder balance exhausted on local commitment")?, - holder_balance_on_remote_msat - .ok_or("holder balance exhausted on remote commitment")?, - ) / 1000, + cmp::min(holder_balance_on_local_msat, holder_balance_on_remote_msat) / 1000, ); let counterparty_balance_floor = Amount::from_sat( - cmp::min( - counterparty_balance_on_local_msat - .ok_or("counterparty balance exhausted on local commitment")?, - counterparty_balance_on_remote_msat - .ok_or("counterparty balance exhausted on remote commitment")?, - ) / 1000, + cmp::min(counterparty_balance_on_local_msat, counterparty_balance_on_remote_msat) + / 1000, ); Ok((holder_balance_floor, counterparty_balance_floor)) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 962bb9a4e25..6bba2b59e10 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -1678,6 +1678,8 @@ pub enum LocalHTLCFailureReason { HTLCMaximum, /// The HTLC was failed because our remote peer is offline. PeerOffline, + /// The HTLC was failed because the channel balance was overdrawn. + ChannelBalanceOverdrawn, } impl LocalHTLCFailureReason { @@ -1697,7 +1699,8 @@ impl LocalHTLCFailureReason { | Self::ZeroAmount | Self::HTLCMinimum | Self::HTLCMaximum - | Self::PeerOffline => UPDATE | 7, + | Self::PeerOffline + | Self::ChannelBalanceOverdrawn => UPDATE | 7, Self::PermanentChannelFailure | Self::ChannelClosed | Self::OnChainTimeout => PERM | 8, Self::RequiredChannelFeature => PERM | 9, Self::UnknownNextPeer @@ -1876,7 +1879,8 @@ ser_failure_reasons!( (38, ZeroAmount), (39, HTLCMinimum), (40, HTLCMaximum), - (41, PeerOffline) + (41, PeerOffline), + (42, ChannelBalanceOverdrawn) ); impl From<&HTLCFailReason> for HTLCHandlingFailureReason { @@ -1992,7 +1996,8 @@ impl HTLCFailReason { | LocalHTLCFailureReason::ZeroAmount | LocalHTLCFailureReason::HTLCMinimum | LocalHTLCFailureReason::HTLCMaximum - | LocalHTLCFailureReason::PeerOffline => { + | LocalHTLCFailureReason::PeerOffline + | LocalHTLCFailureReason::ChannelBalanceOverdrawn => { debug_assert_eq!( data.len() - 2, u16::from_be_bytes(data[0..2].try_into().unwrap()) as usize diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index cb5415cb3b3..a4bcdff0ffd 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -38,8 +38,8 @@ pub(crate) struct NextCommitmentStats { pub is_outbound_from_holder: bool, pub inbound_htlcs_count: usize, pub inbound_htlcs_value_msat: u64, - pub holder_balance_before_fee_msat: Option, - pub counterparty_balance_before_fee_msat: Option, + pub holder_balance_before_fee_msat: u64, + pub counterparty_balance_before_fee_msat: u64, pub nondust_htlc_count: usize, pub commit_tx_fee_sat: u64, pub dust_exposure_msat: u64, @@ -50,23 +50,21 @@ pub(crate) struct NextCommitmentStats { } impl NextCommitmentStats { - pub(crate) fn get_holder_counterparty_balances_incl_fee_msat( - &self, - ) -> (Option, Option) { + pub(crate) fn get_holder_counterparty_balances_incl_fee_msat(&self) -> Result<(u64, u64), ()> { if self.is_outbound_from_holder { - ( - self.holder_balance_before_fee_msat.and_then(|balance_msat| { - balance_msat.checked_sub(self.commit_tx_fee_sat * 1000) - }), + Ok(( + self.holder_balance_before_fee_msat + .checked_sub(self.commit_tx_fee_sat * 1000) + .ok_or(())?, self.counterparty_balance_before_fee_msat, - ) + )) } else { - ( + Ok(( self.holder_balance_before_fee_msat, - self.counterparty_balance_before_fee_msat.and_then(|balance_msat| { - balance_msat.checked_sub(self.commit_tx_fee_sat * 1000) - }), - ) + self.counterparty_balance_before_fee_msat + .checked_sub(self.commit_tx_fee_sat * 1000) + .ok_or(())?, + )) } } } @@ -133,9 +131,9 @@ fn excess_fees_on_counterparty_tx_dust_exposure_msat( } fn subtract_addl_outputs( - is_outbound_from_holder: bool, value_to_self_after_htlcs_msat: Option, - value_to_remote_after_htlcs_msat: Option, channel_type: &ChannelTypeFeatures, -) -> (Option, Option) { + is_outbound_from_holder: bool, value_to_self_after_htlcs_msat: u64, + value_to_remote_after_htlcs_msat: u64, channel_type: &ChannelTypeFeatures, +) -> Result<(u64, u64), ()> { let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { @@ -150,17 +148,15 @@ fn subtract_addl_outputs( // cover the total anchor sum. if is_outbound_from_holder { - ( - value_to_self_after_htlcs_msat - .and_then(|balance_msat| balance_msat.checked_sub(total_anchors_sat * 1000)), + Ok(( + value_to_self_after_htlcs_msat.checked_sub(total_anchors_sat * 1000).ok_or(())?, value_to_remote_after_htlcs_msat, - ) + )) } else { - ( + Ok(( value_to_self_after_htlcs_msat, - value_to_remote_after_htlcs_msat - .and_then(|balance_msat| balance_msat.checked_sub(total_anchors_sat * 1000)), - ) + value_to_remote_after_htlcs_msat.checked_sub(total_anchors_sat * 1000).ok_or(())?, + )) } } @@ -181,7 +177,7 @@ pub(crate) trait TxBuilder { addl_nondust_htlc_count: usize, feerate_per_kw: u32, dust_exposure_limiting_feerate: Option, broadcaster_dust_limit_satoshis: u64, channel_type: &ChannelTypeFeatures, - ) -> NextCommitmentStats; + ) -> Result; fn commit_tx_fee_sat( &self, feerate_per_kw: u32, nondust_htlc_count: usize, channel_type: &ChannelTypeFeatures, ) -> u64; @@ -208,7 +204,7 @@ impl TxBuilder for SpecTxBuilder { addl_nondust_htlc_count: usize, feerate_per_kw: u32, dust_exposure_limiting_feerate: Option, broadcaster_dust_limit_satoshis: u64, channel_type: &ChannelTypeFeatures, - ) -> NextCommitmentStats { + ) -> Result { 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. @@ -225,9 +221,8 @@ impl TxBuilder for SpecTxBuilder { 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 value_to_counterparty_msat = + (channel_value_satoshis * 1000).checked_sub(value_to_holder_msat).ok_or(())?; let outbound_htlcs_value_msat: u64 = next_commitment_htlcs .iter() .filter_map(|htlc| htlc.outbound.then_some(htlc.amount_msat)) @@ -236,13 +231,10 @@ impl TxBuilder for SpecTxBuilder { .iter() .filter_map(|htlc| (!htlc.outbound).then_some(htlc.amount_msat)) .sum(); - // 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_msat = - value_to_holder_msat.checked_sub(outbound_htlcs_value_msat); + value_to_holder_msat.checked_sub(outbound_htlcs_value_msat).ok_or(())?; let value_to_counterparty_after_htlcs_msat = - value_to_counterparty_msat.checked_sub(inbound_htlcs_value_msat); + value_to_counterparty_msat.checked_sub(inbound_htlcs_value_msat).ok_or(())?; // Subtract the anchors from the channel funder let (holder_balance_before_fee_msat, counterparty_balance_before_fee_msat) = @@ -251,7 +243,7 @@ impl TxBuilder for SpecTxBuilder { value_to_holder_after_htlcs_msat, value_to_counterparty_after_htlcs_msat, channel_type, - ); + )?; // Increment the feerate by a buffer to calculate dust exposure let dust_buffer_feerate = get_dust_buffer_feerate(feerate_per_kw); @@ -300,7 +292,7 @@ impl TxBuilder for SpecTxBuilder { (dust_exposure_msat, None) }; - NextCommitmentStats { + Ok(NextCommitmentStats { is_outbound_from_holder, inbound_htlcs_count, inbound_htlcs_value_msat, @@ -310,7 +302,7 @@ impl TxBuilder for SpecTxBuilder { 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, From 061a725e64d16eb9d9c54b3e1ccf5b36c565985c Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 28 Aug 2025 06:24:43 +0000 Subject: [PATCH 2/7] Include HTLCs unknown by remote in `can_accept_incoming_htlc` stats While these HTLCs may currently be unknown to our counterparty, they can end up in commitments soon. Moreover, we are considering failing a single HTLC here, not the entire channel, so we opt to be conservative in what we accept to forward. --- lightning/src/ln/channel.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 53da7b42ac5..61e402e2c44 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5026,9 +5026,11 @@ where // doesn't exist on the receiver's side, only on the sender's. 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; + // While these HTLCs may currently be unknown to our counterparty, they can + // end up in commitments soon. Moreover, we are considering failing a + // single HTLC here, not the entire channel, so we opt to be conservative + // in what we accept to forward. + let include_counterparty_unknown_htlcs = true; // 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 From f30fb62cd4de2459cb83f7247df73a69a5fb5c6a Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 28 Aug 2025 06:42:30 +0000 Subject: [PATCH 3/7] Include any pending fee updates in `can_accept_incoming_htlc` stats While these fee updates may currently be unknown to our counterparty, they can end up in commitments soon. Moreover, we are considering failing a single HTLC here, not the entire channel, so we opt to be conservative in what we accept to forward. --- lightning/src/ln/channel.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 61e402e2c44..4cb417fe01f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5031,6 +5031,9 @@ where // single HTLC here, not the entire channel, so we opt to be conservative // in what we accept to forward. let include_counterparty_unknown_htlcs = true; + // Similar reasoning as above + let feerate = + cmp::max(self.feerate_per_kw, self.pending_update_fee.map(|(fee, _)| fee).unwrap_or(0)); // 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 @@ -5039,7 +5042,7 @@ where None, include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, - self.feerate_per_kw, + feerate, dust_exposure_limiting_feerate, ) .map_err(|()| { @@ -5052,7 +5055,7 @@ where None, include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, - self.feerate_per_kw, + feerate, dust_exposure_limiting_feerate, ) .map_err(|()| { From ec13990df3572e5fa55b8ba0d56d809e4153e383 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 28 Aug 2025 07:29:38 +0000 Subject: [PATCH 4/7] Relax feerate requirements in `TxBuilder::get_next_commitment_stats` We sometimes do not have easy access to the `dust_exposure_limiting_feerate`, yet we are still interested in basic stats on commitments like balances and transaction fees. So we relax the requirement that the `dust_exposure_limiting_feerate` is always set when `feerate_per_kw` is not 0. --- lightning/src/ln/channel.rs | 13 ++----------- lightning/src/sign/tx_builder.rs | 7 ++----- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 4cb417fe01f..d30089f535c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -11979,21 +11979,12 @@ where fn get_holder_counterparty_balances_floor_incl_fee( &self, funding: &FundingScope, ) -> Result<(Amount, Amount), String> { - // We don't care about the exact value of `dust_exposure_limiting_feerate` here as - // we do not validate dust exposure below, but we want to avoid triggering a debug - // assert. - // - // TODO: clean this up here and elsewhere. - let dust_exposure_limiting_feerate = - if funding.get_channel_type().supports_anchor_zero_fee_commitments() { - None - } else { - Some(self.context.feerate_per_kw) - }; let include_counterparty_unknown_htlcs = true; // Make sure that that the funder of the channel can pay the transaction fees for an additional // nondust HTLC on the channel. let addl_nondust_htlc_count = 1; + // We are not interested in dust exposure + let dust_exposure_limiting_feerate = None; let local_commitment_stats = self .context diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index a4bcdff0ffd..8583bdef3b5 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -206,11 +206,8 @@ impl TxBuilder for SpecTxBuilder { channel_type: &ChannelTypeFeatures, ) -> Result { 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 { + feerate_per_kw.checked_sub(dust_exposure_limiting_feerate.unwrap_or(feerate_per_kw)); + if channel_type.supports_anchor_zero_fee_commitments() { debug_assert_eq!(feerate_per_kw, 0); debug_assert_eq!(excess_feerate_opt, Some(0)); debug_assert_eq!(addl_nondust_htlc_count, 0); From 6c6361de5d9406697c11881016058edec0db75d8 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 28 Aug 2025 08:19:48 +0000 Subject: [PATCH 5/7] Cleanup dust exposure due to excess fees in `get_next_commitment_stats` --- lightning/src/sign/tx_builder.rs | 108 ++++++++++++++----------------- 1 file changed, 47 insertions(+), 61 deletions(-) diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index 8583bdef3b5..74941ec8a87 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -43,10 +43,7 @@ pub(crate) struct NextCommitmentStats { 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, + pub extra_accepted_htlc_dust_exposure_msat: u64, } impl NextCommitmentStats { @@ -69,65 +66,52 @@ impl NextCommitmentStats { } } -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, dust_htlc_exposure_msat: u64, - channel_type: &ChannelTypeFeatures, +fn commit_plus_htlc_tx_fees_msat( + local: bool, next_commitment_htlcs: &[HTLCAmountDirection], dust_buffer_feerate: u32, + feerate: u32, broadcaster_dust_limit_satoshis: u64, channel_type: &ChannelTypeFeatures, ) -> (u64, u64) { - let on_counterparty_tx_accepted_nondust_htlcs = next_commitment_htlcs + let accepted_nondust_htlcs = next_commitment_htlcs .iter() .filter(|htlc| { - htlc.outbound + htlc.outbound != local && !htlc.is_dust( - false, + local, dust_buffer_feerate, - counterparty_dust_limit_satoshis, + broadcaster_dust_limit_satoshis, channel_type, ) }) .count(); - let on_counterparty_tx_offered_nondust_htlcs = next_commitment_htlcs + let offered_nondust_htlcs = next_commitment_htlcs .iter() .filter(|htlc| { - !htlc.outbound + htlc.outbound == local && !htlc.is_dust( - false, + local, dust_buffer_feerate, - counterparty_dust_limit_satoshis, + broadcaster_dust_limit_satoshis, channel_type, ) }) .count(); - 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 commitment_fee_sat = + commit_tx_fee_sat(feerate, accepted_nondust_htlcs + offered_nondust_htlcs, channel_type); + let second_stage_fees_sat = + htlc_tx_fees_sat(feerate, accepted_nondust_htlcs, offered_nondust_htlcs, channel_type); + let total_fees_msat = (commitment_fee_sat + second_stage_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, + let extra_accepted_htlc_commitment_fee_sat = commit_tx_fee_sat( + feerate, + accepted_nondust_htlcs + 1 + 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; + let extra_accepted_htlc_second_stage_fees_sat = + htlc_tx_fees_sat(feerate, accepted_nondust_htlcs + 1, offered_nondust_htlcs, channel_type); + let extra_accepted_htlc_total_fees_msat = + (extra_accepted_htlc_commitment_fee_sat + extra_accepted_htlc_second_stage_fees_sat) * 1000; - (on_counterparty_tx_dust_exposure_msat, extra_htlc_dust_exposure_msat) + (total_fees_msat, extra_accepted_htlc_total_fees_msat) } fn subtract_addl_outputs( @@ -205,11 +189,11 @@ impl TxBuilder for SpecTxBuilder { dust_exposure_limiting_feerate: Option, broadcaster_dust_limit_satoshis: u64, channel_type: &ChannelTypeFeatures, ) -> Result { - let excess_feerate_opt = - feerate_per_kw.checked_sub(dust_exposure_limiting_feerate.unwrap_or(feerate_per_kw)); + let excess_feerate = + feerate_per_kw.saturating_sub(dust_exposure_limiting_feerate.unwrap_or(feerate_per_kw)); if channel_type.supports_anchor_zero_fee_commitments() { debug_assert_eq!(feerate_per_kw, 0); - debug_assert_eq!(excess_feerate_opt, Some(0)); + debug_assert_eq!(excess_feerate, 0); debug_assert_eq!(addl_nondust_htlc_count, 0); } @@ -272,22 +256,24 @@ impl TxBuilder for SpecTxBuilder { }) .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) - }; + // Add any excess fees to dust exposure on counterparty transactions + let (dust_exposure_msat, extra_accepted_htlc_dust_exposure_msat) = if local { + (dust_exposure_msat, dust_exposure_msat) + } else { + let (excess_fees_msat, extra_accepted_htlc_excess_fees_msat) = + commit_plus_htlc_tx_fees_msat( + local, + &next_commitment_htlcs, + dust_buffer_feerate, + excess_feerate, + broadcaster_dust_limit_satoshis, + channel_type, + ); + ( + dust_exposure_msat + excess_fees_msat, + dust_exposure_msat + extra_accepted_htlc_excess_fees_msat, + ) + }; Ok(NextCommitmentStats { is_outbound_from_holder, @@ -298,7 +284,7 @@ impl TxBuilder for SpecTxBuilder { 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, + extra_accepted_htlc_dust_exposure_msat, }) } fn commit_tx_fee_sat( From 1ae5da1c9d3a293019bca64a2d657712e0a61b7d Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 28 Aug 2025 07:35:29 +0000 Subject: [PATCH 6/7] Note that we may want to apply HTLC deletes to the upcoming set of HTLCs --- lightning/src/ln/channel.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d30089f535c..3c7a825ed02 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4405,6 +4405,9 @@ where amount_msat, }); + // TODO: HTLC removals are released from the holding cell at the same time + // as HTLC additions, so if HTLC additions are applied here, so should HTLC removals. + // This would allow us to make better use of channel liquidity. 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 }) From 2356e05262f0cea438055a40490aa263c1fbc959 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Mon, 8 Sep 2025 16:18:06 +0000 Subject: [PATCH 7/7] Check dust exposure on receiving `commitment_signed`, not `update_fee` We allow a peer to send an `update_fee` message that pushes the dust exposure over our max as long as they send HTLC updates that bring the dust exposure back down under the limit before they send `commitment_signed`. --- lightning/src/ln/chanmon_update_fail_tests.rs | 10 +- lightning/src/ln/channel.rs | 240 +++++------------- lightning/src/ln/channelmanager.rs | 4 +- lightning/src/ln/functional_tests.rs | 18 +- 4 files changed, 87 insertions(+), 185 deletions(-) diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 469bab7f077..1bc1bfbc2ff 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -12,6 +12,7 @@ //! There are a bunch of these as their handling is relatively error-prone so they are split out //! here. See also the chanmon_fail_consistency fuzz test. +use crate::chain::chaininterface::LowerBoundedFeeEstimator; use crate::chain::chainmonitor::ChainMonitor; use crate::chain::channelmonitor::{ChannelMonitor, MonitorEvent, ANTI_REORG_DELAY}; use crate::chain::transaction::OutPoint; @@ -133,9 +134,12 @@ fn test_monitor_and_persister_update_fail() { let chan_opt = get_channel_ref!(nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan.2); if let Some(channel) = chan_opt.as_funded_mut() { assert_eq!(updates.commitment_signed.len(), 1); - if let Ok(Some(update)) = - channel.commitment_signed(&updates.commitment_signed[0], &node_cfgs[0].logger) - { + let feeest = LowerBoundedFeeEstimator::new(&chanmon_cfgs[0].fee_estimator); + if let Ok(Some(update)) = channel.commitment_signed( + &updates.commitment_signed[0], + &feeest, + &node_cfgs[0].logger, + ) { // Check that the persister returns InProgress (and will never actually complete) // as the monitor update errors. if let ChannelMonitorUpdateStatus::InProgress = diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 3c7a825ed02..ffa25b6fb62 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -303,24 +303,6 @@ struct InboundHTLCOutput { state: InboundHTLCState, } -impl InboundHTLCOutput { - fn is_dust( - &self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, - features: &ChannelTypeFeatures, - ) -> bool { - let (htlc_success_tx_fee_sat, htlc_timeout_tx_fee_sat) = - second_stage_tx_fees_sat(features, feerate_per_kw); - - let htlc_tx_fee_sat = if !local { - // This is an offered HTLC. - htlc_timeout_tx_fee_sat - } else { - htlc_success_tx_fee_sat - }; - self.amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat - } -} - #[cfg_attr(test, derive(Clone, Debug, PartialEq))] enum OutboundHTLCState { /// Added by us and included in a commitment_signed (if we were AwaitingRemoteRevoke when we @@ -447,24 +429,6 @@ struct OutboundHTLCOutput { hold_htlc: Option<()>, } -impl OutboundHTLCOutput { - fn is_dust( - &self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, - features: &ChannelTypeFeatures, - ) -> bool { - let (htlc_success_tx_fee_sat, htlc_timeout_tx_fee_sat) = - second_stage_tx_fees_sat(features, feerate_per_kw); - - let htlc_tx_fee_sat = if local { - // This is an offered HTLC. - htlc_timeout_tx_fee_sat - } else { - htlc_success_tx_fee_sat - }; - self.amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat - } -} - /// See AwaitingRemoteRevoke ChannelState for more info #[cfg_attr(test, derive(Clone, Debug, PartialEq))] enum HTLCUpdateAwaitingACK { @@ -1101,7 +1065,6 @@ struct HTLCStats { /// A struct gathering data on a commitment, either local or remote. struct CommitmentData<'a> { tx: CommitmentTransaction, - stats: CommitmentStats, htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction outbound_htlc_preimages: Vec, // preimages for successful offered HTLCs since last commitment inbound_htlc_preimages: Vec, // preimages for successful received HTLCs since last commitment @@ -2005,10 +1968,11 @@ where } #[rustfmt::skip] - pub fn commitment_signed( - &mut self, msg: &msgs::CommitmentSigned, best_block: BestBlock, signer_provider: &SP, logger: &L + pub fn commitment_signed( + &mut self, msg: &msgs::CommitmentSigned, best_block: BestBlock, signer_provider: &SP, fee_estimator: &LowerBoundedFeeEstimator, logger: &L ) -> Result<(Option::EcdsaSigner>>, Option), ChannelError> where + F::Target: FeeEstimator, L::Target: Logger { let phase = core::mem::replace(&mut self.phase, ChannelPhase::Undefined); @@ -2060,10 +2024,10 @@ where .unwrap_or(true); let res = if has_negotiated_pending_splice && !session_received_commitment_signed { funded_channel - .splice_initial_commitment_signed(msg, logger) + .splice_initial_commitment_signed(msg, fee_estimator, logger) .map(|monitor_update_opt| (None, monitor_update_opt)) } else { - funded_channel.commitment_signed(msg, logger) + funded_channel.commitment_signed(msg, fee_estimator, logger) .map(|monitor_update_opt| (None, monitor_update_opt)) }; @@ -4715,7 +4679,7 @@ where fn validate_update_fee( &self, funding: &FundingScope, fee_estimator: &LowerBoundedFeeEstimator, - msg: &msgs::UpdateFee, + new_feerate_per_kw: u32, ) -> Result<(), ChannelError> where F::Target: FeeEstimator, @@ -4732,7 +4696,7 @@ where None, include_counterparty_unknown_htlcs, 0, - msg.feerate_per_kw, + new_feerate_per_kw, dust_exposure_limiting_feerate, ) .map_err(|()| { @@ -4740,13 +4704,25 @@ where "Balance after HTLCs and anchors exhausted on local commitment", )) })?; + + next_local_commitment_stats + .get_holder_counterparty_balances_incl_fee_msat() + .and_then(|(_, counterparty_balance_incl_fee_msat)| { + counterparty_balance_incl_fee_msat + .checked_sub(funding.holder_selected_channel_reserve_satoshis * 1000) + .ok_or(()) + }) + .map_err(|()| { + ChannelError::close("Funding remote cannot afford proposed new fee".to_owned()) + })?; + let next_remote_commitment_stats = self .get_next_remote_commitment_stats( funding, None, include_counterparty_unknown_htlcs, 0, - msg.feerate_per_kw, + new_feerate_per_kw, dust_exposure_limiting_feerate, ) .map_err(|()| { @@ -4761,7 +4737,7 @@ where 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, + new_feerate_per_kw, next_local_commitment_stats.dust_exposure_msat, ) )); @@ -4770,7 +4746,7 @@ where 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, + new_feerate_per_kw, next_remote_commitment_stats.dust_exposure_msat, ) )); @@ -4779,14 +4755,15 @@ where Ok(()) } - fn validate_commitment_signed( + fn validate_commitment_signed( &self, funding: &FundingScope, transaction_number: u64, commitment_point: PublicKey, - msg: &msgs::CommitmentSigned, logger: &L, + msg: &msgs::CommitmentSigned, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Result< (HolderCommitmentTransaction, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>), ChannelError, > where + F::Target: FeeEstimator, L::Target: Logger, { let funding_script = funding.get_funding_redeemscript(); @@ -4825,36 +4802,10 @@ where // If our counterparty updated the channel fee in this commitment transaction, check that // they can actually afford the new fee now. - let update_fee = if let Some((_, update_state)) = self.pending_update_fee { - update_state == FeeUpdateState::RemoteAnnounced - } else { - false - }; - if update_fee { - debug_assert!(!funding.is_outbound()); - let counterparty_reserve_we_require_msat = - funding.holder_selected_channel_reserve_satoshis * 1000; - if commitment_data.stats.remote_balance_before_fee_msat - < commitment_data.stats.commit_tx_fee_sat * 1000 - + counterparty_reserve_we_require_msat - { - return Err(ChannelError::close( - "Funding remote cannot afford proposed new fee".to_owned(), - )); - } - } - #[cfg(any(test, fuzzing))] + if let Some((new_feerate_per_kw, FeeUpdateState::RemoteAnnounced)) = self.pending_update_fee { - let PredictedNextFee { - predicted_feerate, - predicted_nondust_htlc_count, - predicted_fee_sat, - } = *funding.next_local_fee.lock().unwrap(); - if predicted_feerate == commitment_data.tx.negotiated_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); - } + debug_assert!(!funding.is_outbound()); + self.validate_update_fee(funding, fee_estimator, new_feerate_per_kw)?; } if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() { @@ -5134,83 +5085,6 @@ where feerate_per_kw } - /// Builds stats on a potential commitment transaction build, without actually building the - /// commitment transaction. See `build_commitment_transaction` for further docs. - #[inline] - #[rustfmt::skip] - fn build_commitment_stats(&self, funding: &FundingScope, local: bool, generated_by_local: bool, feerate_per_kw: Option, fee_buffer_nondust_htlcs: Option) -> CommitmentStats { - let broadcaster_dust_limit_sat = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis }; - let mut nondust_htlc_count = 0; - let mut remote_htlc_total_msat = 0; - let mut local_htlc_total_msat = 0; - let mut value_to_self_claimed_msat = 0; - let mut value_to_remote_claimed_msat = 0; - - let feerate_per_kw = feerate_per_kw.unwrap_or_else(|| self.get_commitment_feerate(funding, generated_by_local)); - - for htlc in self.pending_inbound_htlcs.iter() { - if htlc.state.included_in_commitment(generated_by_local) { - if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { - nondust_htlc_count += 1; - } - remote_htlc_total_msat += htlc.amount_msat; - } else { - if htlc.state.preimage().is_some() { - value_to_self_claimed_msat += htlc.amount_msat; - } - } - }; - - for htlc in self.pending_outbound_htlcs.iter() { - if htlc.state.included_in_commitment(generated_by_local) { - if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { - nondust_htlc_count += 1; - } - local_htlc_total_msat += htlc.amount_msat; - } else { - if htlc.state.preimage().is_some() { - value_to_remote_claimed_msat += htlc.amount_msat; - } - } - }; - - // # Panics - // - // After all HTLC claims have been accounted for, the local balance MUST remain greater than or equal to 0. - - let mut value_to_self_msat = (funding.value_to_self_msat + value_to_self_claimed_msat).checked_sub(value_to_remote_claimed_msat).unwrap(); - - let mut value_to_remote_msat = (funding.get_value_satoshis() * 1000).checked_sub(value_to_self_msat).unwrap(); - value_to_self_msat = value_to_self_msat.checked_sub(local_htlc_total_msat).unwrap(); - value_to_remote_msat = value_to_remote_msat.checked_sub(remote_htlc_total_msat).unwrap(); - - #[cfg(debug_assertions)] - { - // Make sure that the to_self/to_remote is always either past the appropriate - // channel_reserve *or* it is making progress towards it. - let mut broadcaster_max_commitment_tx_output = if generated_by_local { - funding.holder_max_commitment_tx_output.lock().unwrap() - } else { - funding.counterparty_max_commitment_tx_output.lock().unwrap() - }; - debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap()); - broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat); - debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis); - broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat); - } - - let commit_tx_fee_sat = SpecTxBuilder {}.commit_tx_fee_sat(feerate_per_kw, nondust_htlc_count + fee_buffer_nondust_htlcs.unwrap_or(0), funding.get_channel_type()); - // Subtract any non-HTLC outputs from the local and remote balances - let (local_balance_before_fee_msat, remote_balance_before_fee_msat) = SpecTxBuilder {}.subtract_non_htlc_outputs( - funding.is_outbound(), - value_to_self_msat, - value_to_remote_msat, - funding.get_channel_type(), - ); - - CommitmentStats { commit_tx_fee_sat, local_balance_before_fee_msat, remote_balance_before_fee_msat } - } - /// Transaction nomenclature is somewhat confusing here as there are many different cases - a /// transaction is referred to as "a's transaction" implying that a will be able to broadcast /// the transaction. Thus, b will generally be sending a signature over such a transaction to @@ -5311,7 +5185,28 @@ where broadcaster_dust_limit_sat, logger, ); - debug_assert_eq!(stats, self.build_commitment_stats(funding, local, generated_by_local, None, None), "Caught an inconsistency between `TxBuilder::build_commitment_transaction` and the rest of the `TxBuilder` methods"); + #[cfg(any(test, fuzzing))] + { + let PredictedNextFee { predicted_feerate, predicted_nondust_htlc_count, predicted_fee_sat } = if local { *funding.next_local_fee.lock().unwrap() } else { *funding.next_remote_fee.lock().unwrap() }; + if predicted_feerate == tx.negotiated_feerate_per_kw() && predicted_nondust_htlc_count == tx.nondust_htlcs().len() { + assert_eq!(predicted_fee_sat, stats.commit_tx_fee_sat); + } + } + #[cfg(debug_assertions)] + { + // Make sure that the to_self/to_remote is always either past the appropriate + // channel_reserve *or* it is making progress towards it. + let mut broadcaster_max_commitment_tx_output = if generated_by_local { + funding.holder_max_commitment_tx_output.lock().unwrap() + } else { + funding.counterparty_max_commitment_tx_output.lock().unwrap() + }; + debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap()); + broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat); + debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis); + broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat); + } + // This populates the HTLC-source table with the indices from the HTLCs in the commitment // transaction. @@ -5346,7 +5241,6 @@ where CommitmentData { tx, - stats, htlcs_included, inbound_htlc_preimages, outbound_htlc_preimages, @@ -7641,10 +7535,12 @@ where /// Note that our `commitment_signed` send did not include a monitor update. This is due to: /// 1. Updates cannot be made since the state machine is paused until `tx_signatures`. /// 2. We're still able to abort negotiation until `tx_signatures`. - fn splice_initial_commitment_signed( - &mut self, msg: &msgs::CommitmentSigned, logger: &L, + fn splice_initial_commitment_signed( + &mut self, msg: &msgs::CommitmentSigned, fee_estimator: &LowerBoundedFeeEstimator, + logger: &L, ) -> Result, ChannelError> where + F::Target: FeeEstimator, L::Target: Logger, { debug_assert!(self @@ -7676,6 +7572,7 @@ where transaction_number, commitment_point, msg, + fee_estimator, logger, )?; // This corresponds to the same `commitment_signed` we sent earlier, which we know to be the @@ -7745,10 +7642,12 @@ where (nondust_htlc_sources, dust_htlcs) } - pub fn commitment_signed( - &mut self, msg: &msgs::CommitmentSigned, logger: &L, + pub fn commitment_signed( + &mut self, msg: &msgs::CommitmentSigned, fee_estimator: &LowerBoundedFeeEstimator, + logger: &L, ) -> Result, ChannelError> where + F::Target: FeeEstimator, L::Target: Logger, { self.commitment_signed_check_state()?; @@ -7768,6 +7667,7 @@ where transaction_number, commitment_point, msg, + fee_estimator, logger, ) .map(|(commitment_tx, htlcs_included)| { @@ -7786,10 +7686,12 @@ where self.commitment_signed_update_monitor(update, logger) } - pub fn commitment_signed_batch( - &mut self, batch: Vec, logger: &L, + pub fn commitment_signed_batch( + &mut self, batch: Vec, fee_estimator: &LowerBoundedFeeEstimator, + logger: &L, ) -> Result, ChannelError> where + F::Target: FeeEstimator, L::Target: Logger, { self.commitment_signed_check_state()?; @@ -7838,6 +7740,7 @@ where transaction_number, commitment_point, msg, + fee_estimator, logger, )?; commitment_txs.push(commitment_tx); @@ -9163,10 +9066,7 @@ where self.context.pending_update_fee = Some((msg.feerate_per_kw, FeeUpdateState::RemoteAnnounced)); self.context.update_time_counter += 1; - - core::iter::once(&self.funding) - .chain(self.pending_funding().iter()) - .try_for_each(|funding| self.context.validate_update_fee(funding, fee_estimator, msg)) + Ok(()) } /// Indicates that the signer may have some signatures for us, so we should retry if we're @@ -12378,14 +12278,6 @@ where ); let counterparty_commitment_tx = commitment_data.tx; - #[cfg(any(test, fuzzing))] - { - let PredictedNextFee { predicted_feerate, predicted_nondust_htlc_count, predicted_fee_sat } = *funding.next_remote_fee.lock().unwrap(); - if predicted_feerate == counterparty_commitment_tx.negotiated_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) } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0bdca77b366..c771f9ad663 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -10783,7 +10783,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let logger = WithChannelContext::from(&self.logger, &chan.context(), None); let funding_txo = chan.funding().get_funding_txo(); let (monitor_opt, monitor_update_opt) = try_channel_entry!( - self, peer_state, chan.commitment_signed(msg, best_block, &self.signer_provider, &&logger), + self, peer_state, chan.commitment_signed(msg, best_block, &self.signer_provider, &self.fee_estimator, &&logger), chan_entry); if let Some(chan) = chan.as_funded_mut() { @@ -10828,7 +10828,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let funding_txo = chan.funding().get_funding_txo(); if let Some(chan) = chan.as_funded_mut() { let monitor_update_opt = try_channel_entry!( - self, peer_state, chan.commitment_signed_batch(batch, &&logger), chan_entry + self, peer_state, chan.commitment_signed_batch(batch, &self.fee_estimator, &&logger), chan_entry ); if let Some(monitor_update) = monitor_update_opt { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index b961da202d4..aabce4a55a3 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -7281,9 +7281,12 @@ pub fn test_update_err_monitor_lockdown() { get_channel_ref!(nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan_1.2); if let Some(channel) = chan_ref.as_funded_mut() { assert_eq!(updates.commitment_signed.len(), 1); - if let Ok(Some(update)) = - channel.commitment_signed(&updates.commitment_signed[0], &node_cfgs[0].logger) - { + let feeest = LowerBoundedFeeEstimator::new(&chanmon_cfgs[0].fee_estimator); + if let Ok(Some(update)) = channel.commitment_signed( + &updates.commitment_signed[0], + &feeest, + &node_cfgs[0].logger, + ) { assert_eq!( watchtower.chain_monitor.update_channel(chan_1.2, &update), ChannelMonitorUpdateStatus::InProgress @@ -7434,9 +7437,12 @@ pub fn test_concurrent_monitor_claim() { get_channel_ref!(nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan_1.2); if let Some(channel) = chan_ref.as_funded_mut() { assert_eq!(updates.commitment_signed.len(), 1); - if let Ok(Some(update)) = - channel.commitment_signed(&updates.commitment_signed[0], &node_cfgs[0].logger) - { + let feeest = LowerBoundedFeeEstimator::new(&chanmon_cfgs[0].fee_estimator); + if let Ok(Some(update)) = channel.commitment_signed( + &updates.commitment_signed[0], + &feeest, + &node_cfgs[0].logger, + ) { // Watchtower Alice should already have seen the block and reject the update assert_eq!( watchtower_alice.chain_monitor.update_channel(chan_1.2, &update),