Improve prediction of commitment stats in can_accept_incoming_htlc

`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.

This is an overestimate:

* Outbound HTLC removals which have been ACK'ed by the counterparty will
  certainly not be present in any *next* commitment, even though they
  remain in `pending_outbound_htlcs`.

* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
  the next *local* commitment.

* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
  they were yet received by the counterparty.

* Outbound `update_add_htlc`'s in the holding cell are certainly not
  known by the counterparty, and we will reevaluate their addition to
  the channel when freeing the holding cell.

* Inbound HTLCs in the `LocalRemoved` state will not be present in the
  next *remote* commitment.

This commit stops using `get_pending_htlc_stats` in favor of the newly
added `ChannelContext::get_next_{local, remote}_commitment_stats`
methods, and fixes the issues described above.

`ChannelContext::next_remote_commit_tx_fee_msat` counts inbound HTLCs in
the `LocalRemoved` state, as well as outbound HTLCs in the
`LocalAnnounced` state. We now do not count them for the same reasons
described above.

Inbound `LocalRemoved` HTLCs that were **not** successful are now
credited to `remote_balance_before_fee_msat` as they will certainly not
be on the next remote commitment. We previously debited these from the
remote balance to arrive at `remote_balance_before_fee_msat`.

We now always check holder dust exposure, whereas we previously would
only do it if the incoming HTLC was dust on our own commitment
transaction.

Furthermore, dust exposure calculations now take a buffer from the
currently committed feerate, and ignore any fee updates in
`ChannelContext.pending_update_fee`.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -4486,78 +4486,47 @@ where
 
 	#[rustfmt::skip]
 	fn can_accept_incoming_htlc<L: Deref>(
-		&self, funding: &FundingScope, msg: &msgs::UpdateAddHTLC,
+		&self, funding: &FundingScope,
 		dust_exposure_limiting_feerate: Option<u32>, logger: &L,
 	) -> Result<(), LocalHTLCFailureReason>
 	where
 		L::Target: Logger,
 	{
-		let htlc_stats = self.get_pending_htlc_stats(funding, None, dust_exposure_limiting_feerate);
+		// The fee spike buffer (an additional nondust HTLC) we keep for the remote if the channel
+		// is not zero fee. This deviates from the spec because the fee spike buffer requirement
+		// doesn't exist on the receiver's side, only on the sender's. Note that with anchor
+		// outputs we are no longer as sensitive to fee spikes, so we need to account for them.
+		let fee_spike_buffer_htlc = if funding.get_channel_type().supports_anchor_zero_fee_commitments() {
+			0
+		} else {
+			1
+		};
+		// Do not include outbound update_add_htlc's in the holding cell, or those which haven't yet been ACK'ed by the counterparty (ie. LocalAnnounced HTLCs)
+		let include_counterparty_unknown_htlcs = false;
+		// A `None` `HTLCCandidate` is used as in this case because we're already accounting for
+		// the incoming HTLC as it has been fully committed by both sides.
+		let next_local_commitment_stats = self.get_next_local_commitment_stats(funding, None, include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, self.feerate_per_kw, dust_exposure_limiting_feerate);
+		let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, None, include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, self.feerate_per_kw, dust_exposure_limiting_feerate);
+
 		let max_dust_htlc_exposure_msat = self.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate);
-		let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat;
-		if on_counterparty_tx_dust_htlc_exposure_msat > max_dust_htlc_exposure_msat {
+		if next_remote_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat {
 			// Note that the total dust exposure includes both the dust HTLCs and the excess mining fees of the counterparty commitment transaction
 			log_info!(logger, "Cannot accept value that would put our total dust exposure at {} over the limit {} on counterparty commitment tx",
-				on_counterparty_tx_dust_htlc_exposure_msat, max_dust_htlc_exposure_msat);
+			        next_remote_commitment_stats.dust_exposure_msat, max_dust_htlc_exposure_msat);
 			return Err(LocalHTLCFailureReason::DustLimitCounterparty)
 		}
-		let dust_buffer_feerate = self.get_dust_buffer_feerate(None);
-		let (htlc_success_tx_fee_sat, _) = second_stage_tx_fees_sat(
-			&funding.get_channel_type(), dust_buffer_feerate,
-		);
-		let exposure_dust_limit_success_sats = htlc_success_tx_fee_sat + self.holder_dust_limit_satoshis;
-		if msg.amount_msat / 1000 < exposure_dust_limit_success_sats {
-			let on_holder_tx_dust_htlc_exposure_msat = htlc_stats.on_holder_tx_dust_exposure_msat;
-			if on_holder_tx_dust_htlc_exposure_msat > max_dust_htlc_exposure_msat {
-				log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
-					on_holder_tx_dust_htlc_exposure_msat, max_dust_htlc_exposure_msat);
-				return Err(LocalHTLCFailureReason::DustLimitHolder)
-			}
+		if next_local_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat {
+			log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
+				next_local_commitment_stats.dust_exposure_msat, max_dust_htlc_exposure_msat);
+			return Err(LocalHTLCFailureReason::DustLimitHolder)
 		}
 
 		if !funding.is_outbound() {
-			let removed_outbound_total_msat: u64 = self.pending_outbound_htlcs
-				.iter()
-				.filter_map(|htlc| {
-					matches!(
-						htlc.state,
-						OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _))
-						| OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _))
-					)
-					.then_some(htlc.amount_msat)
-				})
-				.sum();
-			let pending_value_to_self_msat =
-				funding.value_to_self_msat + htlc_stats.pending_inbound_htlcs_value_msat - removed_outbound_total_msat;
-			let pending_remote_value_msat =
-				funding.get_value_satoshis() * 1000 - pending_value_to_self_msat;
-			// Subtract any non-HTLC outputs from the local and remote balances
-			let (_, remote_balance_before_fee_msat) = SpecTxBuilder {}.subtract_non_htlc_outputs(
-				funding.is_outbound(),
-				pending_value_to_self_msat,
-				pending_remote_value_msat,
-				funding.get_channel_type()
-			);
-
-			// `Some(())` is for the fee spike buffer we keep for the remote if the channel is
-			// not zero fee. This deviates from the spec because the fee spike buffer requirement
-			// doesn't exist on the receiver's side, only on the sender's. Note that with anchor
-			// outputs we are no longer as sensitive to fee spikes, so we need to account for them.
-			//
-			// A `None` `HTLCCandidate` is used as in this case because we're already accounting for
-			// the incoming HTLC as it has been fully committed by both sides.
-			let fee_spike_buffer_htlc = if funding.get_channel_type().supports_anchor_zero_fee_commitments() {
-				None
-			} else {
-				Some(())
-			};
-
-			let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(
-				funding, None, fee_spike_buffer_htlc,
-			);
+			let mut remote_fee_cost_incl_stuck_buffer_msat = next_remote_commitment_stats.commit_tx_fee_sat * 1000;
 			if !funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
 				remote_fee_cost_incl_stuck_buffer_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
 			}
+			let remote_balance_before_fee_msat = next_remote_commitment_stats.counterparty_balance_msat.unwrap_or(0);
 			if remote_balance_before_fee_msat.saturating_sub(funding.holder_selected_channel_reserve_satoshis * 1000) < remote_fee_cost_incl_stuck_buffer_msat {
 				log_info!(logger, "Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", &self.channel_id());
 				return Err(LocalHTLCFailureReason::FeeSpikeBuffer);
@@ -9525,7 +9494,7 @@ where
 	/// this function determines whether to fail the HTLC, or forward / claim it.
 	#[rustfmt::skip]
 	pub fn can_accept_incoming_htlc<F: Deref, L: Deref>(
-		&self, msg: &msgs::UpdateAddHTLC, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: L
+		&self, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: L
 	) -> Result<(), LocalHTLCFailureReason>
 	where
 		F::Target: FeeEstimator,
@@ -9541,7 +9510,7 @@ where
 
 		core::iter::once(&self.funding)
 			.chain(self.pending_funding.iter())
-			.try_for_each(|funding| self.context.can_accept_incoming_htlc(funding, msg, dust_exposure_limiting_feerate, &logger))
+			.try_for_each(|funding| self.context.can_accept_incoming_htlc(funding, dust_exposure_limiting_feerate, &logger))
 	}
 
 	pub fn get_cur_holder_commitment_transaction_number(&self) -> u64 {

lightning/src/ln/channelmanager.rs

@@ -6262,7 +6262,7 @@ where
 							&chan.context,
 							Some(update_add_htlc.payment_hash),
 						);
-						chan.can_accept_incoming_htlc(update_add_htlc, &self.fee_estimator, &logger)
+						chan.can_accept_incoming_htlc(&self.fee_estimator, &logger)
 					},
 				) {
 					Some(Ok(_)) => {},