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.

The value of `remote_balance_before_fee_msat` remains *exactly*
the same.

`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.

Finally, 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.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -4488,78 +4488,49 @@ 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 do_not_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, do_not_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, do_not_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 {
+			// Note: 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
+			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);
@@ -9520,7 +9491,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,
@@ -9536,7 +9507,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(_)) => {},