Merge pull request #4417 from wpaulino/fix-splice-funding-scope-max-commitment-output

Account for missing balance in splice max commitment output tracking

Author: tankyleo

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,15 +2847,21 @@ impl FundingScope {
 			counterparty_selected_channel_reserve_satoshis,
 			holder_selected_channel_reserve_satoshis,
 			#[cfg(debug_assertions)]
-			holder_max_commitment_tx_output: Mutex::new((
-				post_value_to_self_msat,
-				(post_channel_value * 1000).saturating_sub(post_value_to_self_msat),
-			)),
+			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: Mutex::new((
-				post_value_to_self_msat,
-				(post_channel_value * 1000).saturating_sub(post_value_to_self_msat),
-			)),
+			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),
+				))
+			},
 			#[cfg(any(test, fuzzing))]
 			next_local_fee: Mutex::new(PredictedNextFee::default()),
 			#[cfg(any(test, fuzzing))]
@@ -3799,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()),
@@ -4037,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()),
@@ -5588,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.
@@ -15977,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()),
@@ -18542,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

@@ -2745,3 +2745,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);
+}