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 08ca1d34362..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)) }; @@ -4405,6 +4369,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 }) @@ -4478,7 +4445,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 +4464,7 @@ where dust_exposure_limiting_feerate, self.holder_dust_limit_satoshis, funding.get_channel_type(), - ); + )?; #[cfg(any(test, fuzzing))] { @@ -4508,18 +4475,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 +4497,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 +4523,7 @@ where dust_exposure_limiting_feerate, self.counterparty_dust_limit_satoshis, funding.get_channel_type(), - ); + )?; #[cfg(any(test, fuzzing))] { @@ -4565,18 +4534,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 +4556,7 @@ where } } - ret + Ok(ret) } fn validate_update_add_htlc( @@ -4608,14 +4579,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 +4609,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 +4630,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 +4649,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 { @@ -4703,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, @@ -4714,22 +4690,46 @@ 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, + new_feerate_per_kw, + dust_exposure_limiting_feerate, + ) + .map_err(|()| { + ChannelError::close(String::from( + "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, + new_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); @@ -4737,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, ) )); @@ -4746,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, ) )); @@ -4755,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(); @@ -4801,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() { @@ -4926,20 +4901,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 +4941,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, @@ -4992,27 +4980,42 @@ 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; + // 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.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, + feerate, + 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, + feerate, + 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 +5049,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 { @@ -5086,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 @@ -5263,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. @@ -5298,7 +5241,6 @@ where CommitmentData { tx, - stats, htlcs_included, inbound_htlc_preimages, outbound_htlc_preimages, @@ -7593,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 @@ -7628,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 @@ -7697,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()?; @@ -7720,6 +7667,7 @@ where transaction_number, commitment_point, msg, + fee_estimator, logger, ) .map(|(commitment_tx, htlcs_included)| { @@ -7738,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()?; @@ -7790,6 +7740,7 @@ where transaction_number, commitment_point, msg, + fee_estimator, logger, )?; commitment_txs.push(commitment_tx); @@ -9115,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 @@ -11934,59 +11882,51 @@ 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.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)) @@ -12338,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), 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..74941ec8a87 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -38,104 +38,86 @@ 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, - // 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 { - 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(())?, + )) } } } -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( - 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 +132,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 +161,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,15 +188,12 @@ 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 { - 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 { + ) -> Result { + 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); } @@ -225,9 +202,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 +212,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 +224,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); @@ -283,24 +256,26 @@ 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, + ) + }; - NextCommitmentStats { + Ok(NextCommitmentStats { is_outbound_from_holder, inbound_htlcs_count, inbound_htlcs_value_msat, @@ -309,8 +284,8 @@ 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( &self, feerate_per_kw: u32, nondust_htlc_count: usize, channel_type: &ChannelTypeFeatures,