Account for missing balance in splice max commitment output tracking

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.07%. Comparing base (0b45bfd) to head (27bf59a).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4417      +/-   ##
==========================================
- Coverage   86.07%   86.07%   -0.01%     
==========================================
  Files         156      156              
  Lines      103442   103463      +21     
  Branches   103442   103463      +21     
==========================================
+ Hits        89033    89051      +18     
- Misses      11894    11898       +4     
+ Partials     2515     2514       -1     

Flag	Coverage Δ
tests	86.07% <96.29%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -2772,55 +2772,18 @@ impl FundingScope {
 			context.counterparty_dust_limit_satoshis,
 		);

-		// Account for in-flight HTLCs and anchor outputs when initializing the max
-		// commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),
-		// a splice may have pending HTLCs whose amounts are subtracted from the balances
-		// in the commitment transaction. Without this adjustment, the monotonicity debug
-		// assertion in `build_commitment_transaction` would fire on the first commitment.
-		#[cfg(debug_assertions)]
-		let (local_balance_msat, remote_balance_msat) = {
-			let pending_outbound_htlcs_value_msat: u64 =
-				context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();
-			let pending_inbound_htlcs_value_msat: u64 =
-				context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();
-			let channel_type = &post_channel_transaction_parameters.channel_type_features;
-			let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {
-				ANCHOR_OUTPUT_VALUE_SATOSHI * 2
-			} else {
-				0
-			};
-			let post_value_to_remote_msat =
-				(post_channel_value * 1000).saturating_sub(post_value_to_self_msat);
-			if post_channel_transaction_parameters.is_outbound_from_holder {
-				(
-					post_value_to_self_msat
-						.saturating_sub(pending_outbound_htlcs_value_msat)
-						.saturating_sub(total_anchors_sat * 1000),
-					post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),
-				)
-			} else {
-				(
-					post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),
-					post_value_to_remote_msat
-						.saturating_sub(pending_inbound_htlcs_value_msat)
-						.saturating_sub(total_anchors_sat * 1000),
-				)
-			}
-		};
-
 		Self {
 			channel_transaction_parameters: post_channel_transaction_parameters,
 			value_to_self_msat: post_value_to_self_msat,
 			funding_transaction: None,
 			counterparty_selected_channel_reserve_satoshis,
 			holder_selected_channel_reserve_satoshis,
+			// Here we copy over these values; if the party is below the reserve under the new funding
+			// scope, their balance MUST NOT decrease.
 			#[cfg(debug_assertions)]
-			holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),
+			holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
 			#[cfg(debug_assertions)]
-			counterparty_max_commitment_tx_output: Mutex::new((
-				local_balance_msat,
-				remote_balance_msat,
-			)),
+			counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
 			#[cfg(any(test, fuzzing))]
 			next_local_fee: Mutex::new(PredictedNextFee::default()),
 			#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 			} 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);
-		}

+			if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {
+				// If local is below the reserve on this new commitment, local balance MUST be greater than
+				// or equal to local balance on the previous commitment, even across a splice.
+				debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);
+			}
+			broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;
+
+			if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {
+				// If remote is below the reserve on this new commitment, remote balance MUST be greater than
+				// or equal to remote balance on the previous commitment, even across a splice.
+				debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);
+			}
+			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.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.