Rework max commitment transaction balance debug assertions

These assertions made sure that our balance would never dip below the
reserve, and if they ever were, that the balance must only move towards
meeting the reserve. With splicing, this doesn't always work, as a node
that is not interested in contributing could end up below the reserve of
the post-splice channel. Therefore, we rework these assertions such that
we only keep track of the previous commitment transaction balance, and
compare against the current, ensuring that our balance only increases
when below the reserve.

Author: wpaulino

lightning/src/ln/channel.rs

@@ -2583,10 +2583,10 @@ pub(super) struct FundingScope {
 
 	#[cfg(debug_assertions)]
 	/// Max to_local and to_remote outputs in a locally-generated commitment transaction
-	holder_max_commitment_tx_output: Mutex<(u64, u64)>,
+	holder_prev_commitment_tx_balance: Mutex<(u64, u64)>,
 	#[cfg(debug_assertions)]
 	/// Max to_local and to_remote outputs in a remote-generated commitment transaction
-	counterparty_max_commitment_tx_output: Mutex<(u64, u64)>,
+	counterparty_prev_commitment_tx_balance: Mutex<(u64, u64)>,
 
 	// We save these values so we can make sure validation of channel updates properly predicts
 	// what the next commitment transaction fee will be, by comparing the cached values to the
@@ -2658,9 +2658,9 @@ impl Readable for FundingScope {
 			counterparty_selected_channel_reserve_satoshis,
 			holder_selected_channel_reserve_satoshis: holder_selected_channel_reserve_satoshis.0.unwrap(),
 			#[cfg(debug_assertions)]
-			holder_max_commitment_tx_output: Mutex::new((0, 0)),
+			holder_prev_commitment_tx_balance: Mutex::new((0, 0)),
 			#[cfg(debug_assertions)]
-			counterparty_max_commitment_tx_output: Mutex::new((0, 0)),
+			counterparty_prev_commitment_tx_balance: Mutex::new((0, 0)),
 			channel_transaction_parameters: channel_transaction_parameters.0.unwrap(),
 			funding_transaction,
 			funding_tx_confirmed_in,
@@ -2847,16 +2847,16 @@ impl FundingScope {
 			counterparty_selected_channel_reserve_satoshis,
 			holder_selected_channel_reserve_satoshis,
 			#[cfg(debug_assertions)]
-			holder_max_commitment_tx_output: {
-				let prev = *prev_funding.holder_max_commitment_tx_output.lock().unwrap();
+			holder_prev_commitment_tx_balance: {
+				let prev = *prev_funding.holder_prev_commitment_tx_balance.lock().unwrap();
 				Mutex::new((
 					prev.0.saturating_add_signed(our_funding_contribution.to_sat() * 1000),
 					prev.1.saturating_add_signed(their_funding_contribution.to_sat() * 1000),
 				))
 			},
 			#[cfg(debug_assertions)]
-			counterparty_max_commitment_tx_output: {
-				let prev = *prev_funding.counterparty_max_commitment_tx_output.lock().unwrap();
+			counterparty_prev_commitment_tx_balance: {
+				let prev = *prev_funding.counterparty_prev_commitment_tx_balance.lock().unwrap();
 				Mutex::new((
 					prev.0.saturating_add_signed(our_funding_contribution.to_sat() * 1000),
 					prev.1.saturating_add_signed(their_funding_contribution.to_sat() * 1000),
@@ -3805,9 +3805,9 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 			holder_selected_channel_reserve_satoshis,
 
 			#[cfg(debug_assertions)]
-			holder_max_commitment_tx_output: Mutex::new((value_to_self_msat, (channel_value_satoshis * 1000 - msg_push_msat).saturating_sub(value_to_self_msat))),
+			holder_prev_commitment_tx_balance: Mutex::new((value_to_self_msat, (channel_value_satoshis * 1000 - msg_push_msat).saturating_sub(value_to_self_msat))),
 			#[cfg(debug_assertions)]
-			counterparty_max_commitment_tx_output: Mutex::new((value_to_self_msat, (channel_value_satoshis * 1000 - msg_push_msat).saturating_sub(value_to_self_msat))),
+			counterparty_prev_commitment_tx_balance: Mutex::new((value_to_self_msat, (channel_value_satoshis * 1000 - msg_push_msat).saturating_sub(value_to_self_msat))),
 
 			#[cfg(any(test, fuzzing))]
 			next_local_fee: Mutex::new(PredictedNextFee::default()),
@@ -4043,9 +4043,9 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 			// We'll add our counterparty's `funding_satoshis` to these max commitment output assertions
 			// when we receive `accept_channel2`.
 			#[cfg(debug_assertions)]
-			holder_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
+			holder_prev_commitment_tx_balance: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
 			#[cfg(debug_assertions)]
-			counterparty_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
+			counterparty_prev_commitment_tx_balance: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
 
 			#[cfg(any(test, fuzzing))]
 			next_local_fee: Mutex::new(PredictedNextFee::default()),
@@ -5594,17 +5594,26 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 		{
 			// Make sure that the to_self/to_remote is always either past the appropriate
 			// channel_reserve *or* it is making progress towards it.
-			let mut broadcaster_max_commitment_tx_output = if generated_by_local {
-				funding.holder_max_commitment_tx_output.lock().unwrap()
+			let mut broadcaster_prev_commitment_balance = if generated_by_local {
+				funding.holder_prev_commitment_tx_balance.lock().unwrap()
 			} else {
-				funding.counterparty_max_commitment_tx_output.lock().unwrap()
+				funding.counterparty_prev_commitment_tx_balance.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 the local balance is below the reserve on this new commitment, it MUST be
+				// greater than or equal to the one on the previous commitment.
+				debug_assert!(broadcaster_prev_commitment_balance.0 <= stats.local_balance_before_fee_msat);
+			}
+			broadcaster_prev_commitment_balance.0 = stats.local_balance_before_fee_msat;
+
+			if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {
+				// If the remote balance is below the reserve on this new commitment, it MUST be
+				// greater than or equal to the one on the previous commitment.
+				debug_assert!(broadcaster_prev_commitment_balance.1 <= stats.remote_balance_before_fee_msat);
+			}
+			broadcaster_prev_commitment_balance.1 = stats.remote_balance_before_fee_msat;
+		}
 
 		// This populates the HTLC-source table with the indices from the HTLCs in the commitment
 		// transaction.
@@ -15983,9 +15992,9 @@ impl<'a, 'b, 'c, ES: EntropySource, SP: SignerProvider>
 					.unwrap(),
 
 				#[cfg(debug_assertions)]
-				holder_max_commitment_tx_output: Mutex::new((0, 0)),
+				holder_prev_commitment_tx_balance: Mutex::new((0, 0)),
 				#[cfg(debug_assertions)]
-				counterparty_max_commitment_tx_output: Mutex::new((0, 0)),
+				counterparty_prev_commitment_tx_balance: Mutex::new((0, 0)),
 
 				#[cfg(any(test, fuzzing))]
 				next_local_fee: Mutex::new(PredictedNextFee::default()),
@@ -18548,9 +18557,9 @@ mod tests {
 			holder_selected_channel_reserve_satoshis: 0,
 
 			#[cfg(debug_assertions)]
-			holder_max_commitment_tx_output: Mutex::new((0, 0)),
+			holder_prev_commitment_tx_balance: Mutex::new((0, 0)),
 			#[cfg(debug_assertions)]
-			counterparty_max_commitment_tx_output: Mutex::new((0, 0)),
+			counterparty_prev_commitment_tx_balance: Mutex::new((0, 0)),
 
 			#[cfg(any(test, fuzzing))]
 			next_local_fee: Mutex::new(PredictedNextFee::default()),

lightning/src/ln/splicing_tests.rs

@@ -2730,3 +2730,54 @@ fn test_splice_buffer_invalid_commitment_signed_closes_channel() {
 	);
 	check_added_monitors(&nodes[0], 1);
 }
+
+#[test]
+fn test_splice_balance_falls_below_reserve() {
+	// Test that we're able to proceed with a splice where the acceptor does not contribute
+	// anything, but the initiator does, resulting in an increased channel reserve that the
+	// counterparty does not meet but is still valid.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let mut config = test_default_channel_config();
+	config.channel_handshake_config.max_inbound_htlc_value_in_flight_percent_of_channel = 100;
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config.clone()), Some(config)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let initial_channel_value_sat = 100_000;
+	// Push 10k sat to node 1 so it has balance to send HTLCs back.
+	let push_msat = 10_000_000;
+	let (_, _, channel_id, _) = create_announced_chan_between_nodes_with_value(
+		&nodes,
+		0,
+		1,
+		initial_channel_value_sat,
+		push_msat,
+	);
+
+	let _ = provide_anchor_reserves(&nodes);
+
+	// Create bidirectional pending HTLCs (routed but not claimed).
+	// Outbound HTLC from node 0 to node 1.
+	let (preimage_0_to_1, _hash_0_to_1, ..) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
+	// Large inbound HTLC from node 1 to node 0, bringing node 1's remaining balance down to
+	// 2000 sat. The old reserve (1% of 100k) is 1000 sat so this is still above reserve.
+	let (preimage_1_to_0, _hash_1_to_0, ..) = route_payment(&nodes[1], &[&nodes[0]], 8_000_000);
+
+	// Splice-in 200k sat. The new channel value becomes 300k sat, raising the reserve to 3000
+	// sat. Node 1's remaining 2000 sat is now below the new reserve.
+	let initiator_contribution =
+		initiate_splice_in(&nodes[0], &nodes[1], channel_id, Amount::from_sat(200_000));
+	let (splice_tx, _) = splice_channel(&nodes[0], &nodes[1], channel_id, initiator_contribution);
+
+	// Confirm and lock the splice.
+	mine_transaction(&nodes[0], &splice_tx);
+	mine_transaction(&nodes[1], &splice_tx);
+	lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1);
+
+	// Claim both pending HTLCs to verify the channel is fully functional after the splice.
+	claim_payment(&nodes[0], &[&nodes[1]], preimage_0_to_1);
+	claim_payment(&nodes[1], &[&nodes[0]], preimage_1_to_0);
+
+	// Final sanity check: send a payment using the new spliced capacity.
+	let _ = send_payment(&nodes[0], &[&nodes[1]], 1_000_000);
+}