Improve prediction of commitment stats in validate_update_add_htlc

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

This is an overestimate:

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

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

* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
  they were received by the counterparty before she sent the
  `update_fee`.

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

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

`ChannelContext::next_local_commit_tx_fee_msat` over-counts outbound
HTLCs in the `LocalAnnounced` and `RemoteRemoved` states, as well as
outbound `update_add_htlc`'s in the holding cell.

`ChannelContext::next_remote_commit_tx_fee_msat` over-counts inbound
HTLCs in the `LocalRemoved` state, as well as outbound HTLCs in the
`LocalAnnounced` state.

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

As a side-effect, this commit makes consistent the set of HTLCs used to
calculate dust exposure, transaction fees, and balances in
`validate_update_add_htlc`.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -1097,12 +1097,12 @@ pub enum AnnouncementSigsState {
 /// An enum indicating whether the local or remote side offered a given HTLC.
 enum HTLCInitiator {
 	LocalOffered,
+	#[allow(dead_code)]
 	RemoteOffered,
 }
 
 /// Current counts of various HTLCs, useful for calculating current balances available exactly.
 struct HTLCStats {
-	pending_inbound_htlcs: usize,
 	pending_outbound_htlcs: usize,
 	pending_inbound_htlcs_value_msat: u64,
 	pending_outbound_htlcs_value_msat: u64,
@@ -4111,7 +4111,6 @@ where
 	///
 	/// We take the conservative approach and only assume that a HTLC will
 	/// not be in the next commitment when it is guaranteed that it won't be.
-	#[allow(dead_code)]
 	#[rustfmt::skip]
 	fn get_next_commitment_htlcs(
 		&self, local: bool, htlc_candidate: Option<HTLCAmountDirection>, include_counterparty_unknown_htlcs: bool,
@@ -4181,7 +4180,6 @@ where
 	/// will *not* be present on the next commitment from `next_commitment_htlcs`, and
 	/// check if their outcome is successful. If it is, we add the value of this claimed
 	/// HTLC to the balance of the claimer.
-	#[allow(dead_code)]
 	#[rustfmt::skip]
 	fn get_next_commitment_value_to_self_msat(&self, local: bool, funding: &FundingScope) -> u64 {
 		let inbound_claimed_htlc_msat: u64 =
@@ -4213,7 +4211,6 @@ where
 			.saturating_add(inbound_claimed_htlc_msat)
 	}
 
-	#[allow(dead_code)]
 	fn get_next_local_commitment_stats(
 		&self, funding: &FundingScope, htlc_candidate: Option<HTLCAmountDirection>,
 		include_counterparty_unknown_htlcs: bool, addl_nondust_htlc_count: usize,
@@ -4239,7 +4236,6 @@ where
 		)
 	}
 
-	#[allow(dead_code)]
 	fn get_next_remote_commitment_stats(
 		&self, funding: &FundingScope, htlc_candidate: Option<HTLCAmountDirection>,
 		include_counterparty_unknown_htlcs: bool, addl_nondust_htlc_count: usize,
@@ -4280,15 +4276,23 @@ where
 		let dust_exposure_limiting_feerate = self.get_dust_exposure_limiting_feerate(
 			&fee_estimator, funding.get_channel_type(),
 		);
-		let htlc_stats = self.get_pending_htlc_stats(funding, None, dust_exposure_limiting_feerate);
-		if htlc_stats.pending_inbound_htlcs + 1 > self.holder_max_accepted_htlcs as usize {
+		// 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;
+		let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, Some(HTLCAmountDirection { outbound: false, amount_msat: msg.amount_msat }), do_not_include_counterparty_unknown_htlcs, 0, self.feerate_per_kw, dust_exposure_limiting_feerate);
+
+		if next_remote_commitment_stats.inbound_htlcs_count > self.holder_max_accepted_htlcs as usize {
 			return Err(ChannelError::close(format!("Remote tried to push more than our max accepted HTLCs ({})", self.holder_max_accepted_htlcs)));
 		}
-		if htlc_stats.pending_inbound_htlcs_value_msat + msg.amount_msat > self.holder_max_htlc_value_in_flight_msat {
+		if next_remote_commitment_stats.inbound_htlcs_value_msat > self.holder_max_htlc_value_in_flight_msat {
 			return Err(ChannelError::close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.holder_max_htlc_value_in_flight_msat)));
 		}
 
-		// Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet
+		let remote_balance_before_fee_msat = next_remote_commitment_stats.counterparty_balance_msat.ok_or(ChannelError::close("Remote HTLC add would overdraw remaining funds".to_owned()))?;
+
+		// Check that the remote can afford to pay for this HTLC on-chain at the current
+		// feerate_per_kw, while maintaining their channel reserve (as required by the spec).
+		//
+		// We check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet
 		// the reserve_satoshis we told them to always have as direct payment so that they lose
 		// something if we punish them for broadcasting an old state).
 		// Note that we don't really care about having a small/no to_remote output in our local
@@ -4300,50 +4304,22 @@ where
 		// violate the reserve value if we do not do this (as we forget inbound HTLCs from the
 		// Channel state once they will not be present in the next received commitment
 		// transaction).
-		let (local_balance_before_fee_msat, remote_balance_before_fee_msat) = {
-			let removed_outbound_total_msat: u64 = self.pending_outbound_htlcs
-				.iter()
-				.filter_map(|htlc| {
-					matches!(
-						htlc.state,
-						OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _))
-						| OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _))
-					)
-					.then_some(htlc.amount_msat)
-				})
-				.sum();
-			let pending_value_to_self_msat =
-				funding.value_to_self_msat + htlc_stats.pending_inbound_htlcs_value_msat - removed_outbound_total_msat;
-			let pending_remote_value_msat =
-				funding.get_value_satoshis() * 1000 - pending_value_to_self_msat;
-
-			// Subtract any non-HTLC outputs from the local and remote balances
-			SpecTxBuilder {}.subtract_non_htlc_outputs(funding.is_outbound(), funding.value_to_self_msat, pending_remote_value_msat, funding.get_channel_type())
-		};
-		if remote_balance_before_fee_msat < msg.amount_msat {
-			return Err(ChannelError::close("Remote HTLC add would overdraw remaining funds".to_owned()));
-		}
-
-		// Check that the remote can afford to pay for this HTLC on-chain at the current
-		// feerate_per_kw, while maintaining their channel reserve (as required by the spec).
 		{
 			let remote_commit_tx_fee_msat = if funding.is_outbound() { 0 } else {
-				let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
-				self.next_remote_commit_tx_fee_msat(funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations
+				next_remote_commitment_stats.commit_tx_fee_sat * 1000
 			};
-			if remote_balance_before_fee_msat.saturating_sub(msg.amount_msat) < remote_commit_tx_fee_msat {
+			if remote_balance_before_fee_msat < remote_commit_tx_fee_msat {
 				return Err(ChannelError::close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
 			};
-			if remote_balance_before_fee_msat.saturating_sub(msg.amount_msat).saturating_sub(remote_commit_tx_fee_msat) < funding.holder_selected_channel_reserve_satoshis * 1000 {
+			if remote_balance_before_fee_msat.saturating_sub(remote_commit_tx_fee_msat) < funding.holder_selected_channel_reserve_satoshis * 1000 {
 				return Err(ChannelError::close("Remote HTLC add would put them under remote reserve value".to_owned()));
 			}
 		}
 
+		let next_local_commitment_stats = self.get_next_local_commitment_stats(funding, Some(HTLCAmountDirection { outbound: false, amount_msat: msg.amount_msat }), do_not_include_counterparty_unknown_htlcs, 0, self.feerate_per_kw, dust_exposure_limiting_feerate);
 		if funding.is_outbound() {
 			// Check that they won't violate our local required channel reserve by adding this HTLC.
-			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
-			let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(funding, htlc_candidate, None);
-			if local_balance_before_fee_msat < funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat {
+			if next_local_commitment_stats.holder_balance_msat.unwrap_or(0) < funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + next_local_commitment_stats.commit_tx_fee_sat * 1000 {
 				return Err(ChannelError::close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
 			}
 		}
@@ -4956,7 +4932,6 @@ where
 		});
 
 		HTLCStats {
-			pending_inbound_htlcs: self.pending_inbound_htlcs.len(),
 			pending_outbound_htlcs,
 			pending_inbound_htlcs_value_msat,
 			pending_outbound_htlcs_value_msat,