Unwrap get_available_balances with no counterparty unknown HTLCs

We can unwrap `get_available_balances` when not counting counterparty
unknown HTLCs because all balance changes in channel are validated with
the same `get_next_remote_commitment_stats` call before getting applied
to the channel's state.

Now calls to `get_available_balances` external to `ChannelContext` will
only be updated to include HTLCs in the holding cell, and
`LocalAnnounced` HTLCs once we receive the ack from the counterparty for
these updates.

`FundedChannel::send_htlc` includes these HTLCs when calculating
available balances, so it will always have the latest view of the
channel's state and can fail graciously in case some balance has been
overdrawn.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -2446,24 +2446,56 @@ where
 	/// Doesn't bother handling the
 	/// if-we-removed-it-already-but-haven't-fully-resolved-they-can-still-send-an-inbound-HTLC
 	/// corner case properly.
-	pub fn get_available_balances<F: FeeEstimator>(
+	fn get_available_balances_internal<F: FeeEstimator>(
 		&self, fee_estimator: &LowerBoundedFeeEstimator<F>,
-	) -> AvailableBalances {
+		include_counterparty_unknown_htlcs: bool,
+	) -> Result<AvailableBalances, ()> {
 		match &self.phase {
 			ChannelPhase::Undefined => unreachable!(),
-			ChannelPhase::Funded(chan) => chan.get_available_balances(fee_estimator),
+			ChannelPhase::Funded(chan) => chan
+				.get_available_balances_internal(fee_estimator, include_counterparty_unknown_htlcs),
 			ChannelPhase::UnfundedOutboundV1(chan) => {
-				chan.context.get_available_balances_for_scope(&chan.funding, fee_estimator)
-			},
-			ChannelPhase::UnfundedInboundV1(chan) => {
-				chan.context.get_available_balances_for_scope(&chan.funding, fee_estimator)
-			},
-			ChannelPhase::UnfundedV2(chan) => {
-				chan.context.get_available_balances_for_scope(&chan.funding, fee_estimator)
+				chan.context.get_available_balances_for_scope(
+					&chan.funding,
+					fee_estimator,
+					include_counterparty_unknown_htlcs,
+				)
 			},
+			ChannelPhase::UnfundedInboundV1(chan) => chan.context.get_available_balances_for_scope(
+				&chan.funding,
+				fee_estimator,
+				include_counterparty_unknown_htlcs,
+			),
+			ChannelPhase::UnfundedV2(chan) => chan.context.get_available_balances_for_scope(
+				&chan.funding,
+				fee_estimator,
+				include_counterparty_unknown_htlcs,
+			),
 		}
 	}
 
+	#[cfg(any(test, feature = "_externalize_tests"))]
+	pub fn get_available_balances_with_counterparty_unknown_htlcs<F: FeeEstimator>(
+		&self, fee_estimator: &LowerBoundedFeeEstimator<F>,
+	) -> Result<AvailableBalances, ()> {
+		self.get_available_balances_internal(fee_estimator, true)
+	}
+
+	pub fn get_available_balances<F: FeeEstimator>(
+		&self, fee_estimator: &LowerBoundedFeeEstimator<F>,
+	) -> AvailableBalances {
+		let balances_result = self.get_available_balances_internal(fee_estimator, false);
+		balances_result.unwrap_or_else(|()| {
+			debug_assert!(false, "some channel balance has been overdrawn");
+			AvailableBalances {
+				inbound_capacity_msat: 0,
+				outbound_capacity_msat: 0,
+				next_outbound_htlc_limit_msat: 0,
+				next_outbound_htlc_minimum_msat: u64::MAX,
+			}
+		})
+	}
+
 	pub fn minimum_depth(&self) -> Option<u32> {
 		self.context().minimum_depth(self.funding())
 	}
@@ -5783,8 +5815,8 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 	#[rustfmt::skip]
 	fn get_available_balances_for_scope<F: FeeEstimator>(
 		&self, funding: &FundingScope, fee_estimator: &LowerBoundedFeeEstimator<F>,
-	) -> AvailableBalances {
-		let include_counterparty_unknown_htlcs = true;
+		include_counterparty_unknown_htlcs: bool,
+	) -> Result<AvailableBalances, ()> {
 		let dust_exposure_limiting_feerate = self.get_dust_exposure_limiting_feerate(
 			&fee_estimator, funding.get_channel_type(),
 		);
@@ -5796,7 +5828,7 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 			0,
 			self.feerate_per_kw,
 			dust_exposure_limiting_feerate
-		).map(|(remote_stats, _)| remote_stats.available_balances).unwrap();
+		).map(|(remote_stats, _)| remote_stats.available_balances)?;
 
 		#[cfg(debug_assertions)]
 		if balances.next_outbound_htlc_limit_msat >= balances.next_outbound_htlc_minimum_msat
@@ -5820,7 +5852,7 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 				>= funding.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) * 1000);
 		}
 
-		balances
+		Ok(balances)
 	}
 
 	#[rustfmt::skip]
@@ -12487,7 +12519,13 @@ where
 			return Err((LocalHTLCFailureReason::ZeroAmount, "Cannot send 0-msat HTLC".to_owned()));
 		}
 
-		let available_balances = self.get_available_balances(fee_estimator);
+		let available_balances =
+			self.get_available_balances_internal(fee_estimator, true).map_err(|()| {
+				(
+					LocalHTLCFailureReason::ChannelBalanceOverdrawn,
+					"Channel balance overdrawn".to_owned(),
+				)
+			})?;
 		if amount_msat < available_balances.next_outbound_htlc_minimum_msat {
 			return Err((
 				LocalHTLCFailureReason::HTLCMinimum,
@@ -12582,21 +12620,37 @@ where
 	}
 
 	#[rustfmt::skip]
-	pub(super) fn get_available_balances<F: FeeEstimator>(
-		&self, fee_estimator: &LowerBoundedFeeEstimator<F>,
-	) -> AvailableBalances {
-		core::iter::once(&self.funding)
-			.chain(self.pending_funding().iter())
-			.map(|funding| self.context.get_available_balances_for_scope(funding, fee_estimator))
-			.reduce(|acc, e| {
-				AvailableBalances {
+	pub(super) fn get_available_balances_internal<F: FeeEstimator>(
+		&self, fee_estimator: &LowerBoundedFeeEstimator<F>, include_counterparty_unknown_htlcs: bool,
+	) -> Result<AvailableBalances, ()> {
+		let init = self.context.get_available_balances_for_scope(&self.funding, fee_estimator, include_counterparty_unknown_htlcs)?;
+		self.pending_funding().iter().try_fold(
+			init,
+			|acc, funding| {
+				let e = self.context.get_available_balances_for_scope(funding, fee_estimator, include_counterparty_unknown_htlcs)?;
+				Ok(AvailableBalances {
 					inbound_capacity_msat: acc.inbound_capacity_msat.min(e.inbound_capacity_msat),
 					outbound_capacity_msat: acc.outbound_capacity_msat.min(e.outbound_capacity_msat),
 					next_outbound_htlc_limit_msat: acc.next_outbound_htlc_limit_msat.min(e.next_outbound_htlc_limit_msat),
 					next_outbound_htlc_minimum_msat: acc.next_outbound_htlc_minimum_msat.max(e.next_outbound_htlc_minimum_msat),
-				}
+				})
 			})
-			.expect("At least one FundingScope is always provided")
+	}
+
+	#[rustfmt::skip]
+	pub(super) fn get_available_balances<F: FeeEstimator>(
+		&self, fee_estimator: &LowerBoundedFeeEstimator<F>,
+	) -> AvailableBalances {
+		let balances_result = self.get_available_balances_internal(fee_estimator, false);
+		balances_result.unwrap_or_else(|()| {
+			debug_assert!(false, "some channel balance has been overdrawn");
+			AvailableBalances {
+				inbound_capacity_msat: 0,
+				outbound_capacity_msat: 0,
+				next_outbound_htlc_limit_msat: 0,
+				next_outbound_htlc_minimum_msat: u64::MAX,
+			}
+		})
 	}
 
 	fn build_commitment_no_status_check<L: Logger>(&mut self, logger: &L) -> ChannelMonitorUpdate {

lightning/src/ln/functional_tests.rs

@@ -10106,9 +10106,19 @@ pub fn test_dust_exposure_holding_cell_assertion() {
 	expect_and_process_pending_htlcs(&nodes[1], false);
 	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 
-	let bs_chans = nodes[1].node.list_channels();
-	let bc_chan = bs_chans.iter().find(|chan| chan.counterparty.node_id == node_c_id).unwrap();
-	assert!(bc_chan.next_outbound_htlc_minimum_msat > DUST_HTLC_VALUE_MSAT);
+	{
+		let per_peer_state_lock;
+		let mut peer_state_lock;
+		let chan =
+			get_channel_ref!(nodes[1], nodes[2], per_peer_state_lock, peer_state_lock, bc_chan_id);
+		// This how `chan.send_htlc` determines whether it can send another HTLC
+		let balances = chan
+			.get_available_balances_with_counterparty_unknown_htlcs(&LowerBoundedFeeEstimator::new(
+				nodes[1].fee_estimator,
+			))
+			.unwrap();
+		assert!(balances.next_outbound_htlc_minimum_msat > DUST_HTLC_VALUE_MSAT);
+	}
 
 	// Send an additional HTLC from C to B. This will make B unable to forward the HTLC already in
 	// its holding cell as it would be over-exposed to dust.