Disallow net-negative contributions when adding value

When a user requests to add value via coin-selected inputs, we should
strive to fulfill their request. Allowing them to remove value from the
channel is undesired as it goes against their request. While we still
allow adding outputs to enabled mixed contributions, their funds must
now always come from the set of coin-selected inputs, and must never
draw from the channel balance resulting in a smaller added value.

Author: wpaulino

lightning/src/ln/channelmanager.rs

@@ -6638,20 +6638,32 @@ impl<
 	/// The splice initiator is responsible for paying fees for common fields, shared inputs, and
 	/// shared outputs along with any contributed inputs and outputs. When building a
 	/// [`FundingContribution`], fees are estimated at `min_feerate` assuming initiator
-	/// responsibility and must be covered by the supplied inputs for splice-in or the channel
-	/// balance for splice-out. If the counterparty also initiates a splice and wins the
-	/// tie-break, they become the initiator and choose the feerate. The fee is then
-	/// re-estimated at the counterparty's feerate for only our contributed inputs and outputs,
-	/// which may be higher or lower than the original estimate. The contribution is dropped and
-	/// the splice proceeds without it when:
+	/// responsibility. Contributions fall into two cases:
+	/// - **input-backed contributions**: when wallet inputs are selected, those inputs pay for both
+	///   the requested value added to the channel and any explicit withdrawal outputs. For
+	///   example, a 60,000 sat input might add 50,000 sat to the channel, pay a 2,000 sat fee,
+	///   and return 8,000 sat as change. A later RBF first tries to preserve that 50,000 sat
+	///   value added and cover any higher fee or newly requested withdrawal from the original
+	///   10,000 sat fee buffer (2,000 sat fee + 8,000 sat change). If that buffer is not enough,
+	///   the prior contribution cannot be reused without selecting new wallet inputs.
+	/// - **input-less contributions**: when no wallet inputs are selected, fees and explicit
+	///   withdrawal outputs are paid from the channel balance. For example, a pure splice-out that
+	///   withdraws 20,000 sat from a 100,000 sat holder balance leaves up to 80,000 sat available
+	///   for fees. A later RBF keeps the 20,000 sat withdrawal only while that remaining balance
+	///   can still cover the re-estimated fee.
+	///
+	/// If the counterparty also initiates a splice and wins the tie-break, they become the
+	/// initiator and choose the feerate. The fee is then re-estimated at the counterparty's
+	/// feerate for only our contributed inputs and outputs, which may be higher or lower than the
+	/// original estimate. The contribution is dropped and the splice proceeds without it when:
 	/// - the counterparty's feerate is below `min_feerate`
 	/// - the counterparty's feerate is above `max_feerate` and the re-estimated fee exceeds the
 	///   original fee estimate
 	/// - the re-estimated fee exceeds the *fee buffer* regardless of `max_feerate`
 	///
 	/// The fee buffer is the maximum fee that can be accommodated:
-	/// - **splice-in**: the selected inputs' value minus the contributed amount
-	/// - **splice-out**: the channel balance minus the withdrawal outputs
+	/// - **input-backed contributions**: the original fee plus any change output value
+	/// - **input-less contributions**: the channel balance minus the withdrawal outputs
 	///
 	/// # Events
 	///

lightning/src/ln/funding.rs

@@ -59,8 +59,8 @@ pub(super) enum FeeRateAdjustmentError {
 	FeeBufferOverflow,
 	/// The re-estimated fee exceeds the available fee buffer regardless of `max_feerate`. The fee
 	/// buffer is the maximum fee that can be accommodated:
-	/// - **splice-in**: the selected inputs' value minus the contributed amount
-	/// - **splice-out**: the channel balance minus the withdrawal outputs
+	/// - **input-backed contributions**: the original fee plus any change output value
+	/// - **input-less contributions**: the channel balance minus the withdrawal outputs
 	FeeBufferInsufficient { source: &'static str, available: Amount, required: Amount },
 }
 
@@ -288,7 +288,7 @@ macro_rules! build_funding_contribution {
 		let max_feerate: FeeRate = $max_feerate;
 		let force_coin_selection: bool = $force_coin_selection;
 
-		let value_removed = validate_funding_contribution_params(
+		let _value_removed = validate_funding_contribution_params(
 			value_added,
 			&outputs,
 			min_rbf_feerate,
@@ -312,8 +312,6 @@ macro_rules! build_funding_contribution {
 					.map(|shared_input| shared_input.previous_utxo.value)
 					.unwrap_or(Amount::ZERO)
 					.checked_add(value_added)
-					.ok_or(FundingContributionError::InvalidSpliceValue)?
-					.checked_sub(value_removed)
 					.ok_or(FundingContributionError::InvalidSpliceValue)?,
 				script_pubkey: make_funding_redeemscript(&dummy_pubkey, &dummy_pubkey).to_p2wsh(),
 			};
@@ -469,7 +467,8 @@ impl FundingTemplate {
 	/// `value_added` and `outputs` are the complete parameters for this contribution, not
 	/// increments on top of a prior contribution. When replacing a prior contribution via RBF,
 	/// use [`FundingTemplate::prior_contribution`] to inspect the prior parameters and combine
-	/// them as needed.
+	/// them as needed. The withdrawal `outputs` are funded by the selected wallet inputs and do
+	/// not reduce the requested `value_added` to the channel.
 	pub async fn splice_in_and_out<W: CoinSelectionSource + MaybeSend>(
 		self, value_added: Amount, outputs: Vec<TxOut>, min_feerate: FeeRate, max_feerate: FeeRate,
 		wallet: W,
@@ -528,9 +527,9 @@ impl FundingTemplate {
 	///   the fee difference. For splice-out (no wallet inputs), the holder's channel balance
 	///   covers the higher fees.
 	/// - If adjustment fails, coin selection is re-run using the prior contribution's
-	///   parameters and the caller's `max_feerate`. For splice-out contributions, this changes
-	///   the fee source: wallet inputs are selected to cover fees instead of deducting them
-	///   from the channel balance.
+	///   parameters and the caller's `max_feerate`. For prior contributions without inputs,
+	///   this changes the funding source: wallet inputs are selected to cover the outputs and
+	///   fees instead of deducting them from the channel balance.
 	/// - If no prior contribution exists, coin selection is run for a fee-bump-only contribution
 	///   (`value_added = 0`), covering fees for the common fields and shared input/output via
 	///   a newly selected input. Check [`FundingTemplate::prior_contribution`] to see if this
@@ -712,8 +711,10 @@ pub struct FundingContribution {
 	/// excess amount will be sent to a change output.
 	inputs: Vec<FundingTxInput>,
 
-	/// The outputs to include in the funding transaction. The total value of all outputs plus fees
-	/// will be the amount that is removed.
+	/// The outputs to include in the funding transaction.
+	///
+	/// When no wallet inputs are contributed, these outputs are paid from the channel balance.
+	/// Otherwise, they are paid by the contributed inputs.
 	outputs: Vec<TxOut>,
 
 	/// The output where any change will be sent.
@@ -912,82 +913,63 @@ impl FundingContribution {
 			}
 		}
 
+		let target_fee = estimate_transaction_fee(
+			&self.inputs,
+			&self.outputs,
+			self.change_output.as_ref(),
+			is_initiator,
+			self.is_splice,
+			target_feerate,
+		);
+
 		if !self.inputs.is_empty() {
-			if let Some(ref change_output) = self.change_output {
-				let old_change_value = change_output.value;
-				let dust_limit = change_output.script_pubkey.minimal_non_dust();
+			let fee_buffer = self
+				.estimated_fee
+				.checked_add(
+					self.change_output.as_ref().map_or(Amount::ZERO, |output| output.value),
+				)
+				.ok_or(FeeRateAdjustmentError::FeeBufferOverflow)?;
 
-				// Target fee including the change output's weight.
-				let target_fee = estimate_transaction_fee(
-					&self.inputs,
-					&self.outputs,
-					self.change_output.as_ref(),
-					is_initiator,
-					self.is_splice,
-					target_feerate,
-				);
+			if let Some(change_output) = self.change_output.as_ref() {
+				let dust_limit = change_output.script_pubkey.minimal_non_dust();
+				if let Some(new_change_value) = fee_buffer.checked_sub(target_fee) {
+					if new_change_value >= dust_limit {
+						return Ok((target_fee, Some(new_change_value)));
+					}
 
-				let fee_buffer = self
-					.estimated_fee
-					.checked_add(old_change_value)
-					.ok_or(FeeRateAdjustmentError::FeeBufferOverflow)?;
-
-				match fee_buffer.checked_sub(target_fee) {
-					Some(new_change_value) if new_change_value >= dust_limit => {
-						Ok((target_fee, Some(new_change_value)))
-					},
-					_ => {
-						// Change would be below dust or negative. Try without change.
-						let target_fee_no_change = estimate_transaction_fee(
-							&self.inputs,
-							&self.outputs,
-							None,
-							is_initiator,
-							self.is_splice,
-							target_feerate,
-						);
-						if target_fee_no_change > fee_buffer {
-							Err(FeeRateAdjustmentError::FeeBufferInsufficient {
-								source: "estimated fee + change value",
-								available: fee_buffer,
-								required: target_fee_no_change,
-							})
-						} else {
-							Ok((target_fee_no_change, None))
-						}
-					},
+					// Our remaining change was not enough to be a valid output, fallthrough to the
+					// no remaining change case.
 				}
-			} else {
-				// No change output.
-				let target_fee = estimate_transaction_fee(
+
+				let target_fee_no_change = estimate_transaction_fee(
 					&self.inputs,
 					&self.outputs,
 					None,
 					is_initiator,
 					self.is_splice,
 					target_feerate,
 				);
-				if target_fee > self.estimated_fee {
-					return Err(FeeRateAdjustmentError::FeeBufferInsufficient {
-						source: "estimated fee",
-						available: self.estimated_fee,
-						required: target_fee,
-					});
+				if target_fee_no_change > fee_buffer {
+					Err(FeeRateAdjustmentError::FeeBufferInsufficient {
+						source: "estimated fee + change value",
+						available: fee_buffer,
+						required: target_fee_no_change,
+					})
+				} else {
+					Ok((target_fee_no_change, None))
 				}
+			} else if let Some(_surplus) = fee_buffer.checked_sub(target_fee) {
 				Ok((target_fee, None))
+			} else {
+				Err(FeeRateAdjustmentError::FeeBufferInsufficient {
+					source: "estimated fee",
+					available: fee_buffer,
+					required: target_fee,
+				})
 			}
 		} else {
-			// No inputs (splice-out): fees paid from channel balance.
-			let target_fee = estimate_transaction_fee(
-				&[],
-				&self.outputs,
-				None,
-				is_initiator,
-				self.is_splice,
-				target_feerate,
-			);
-
-			// Check that the channel balance can cover the withdrawal outputs plus fees.
+			// Without coin-selected inputs, both the withdrawals and the fee come from the channel
+			// balance.
 			let value_removed: Amount = self.outputs.iter().map(|o| o.value).sum();
 			let total_cost = target_fee
 				.checked_add(value_removed)
@@ -999,7 +981,6 @@ impl FundingContribution {
 					required: target_fee,
 				});
 			}
-			// Surplus goes back to the channel balance.
 			Ok((target_fee, None))
 		}
 	}

lightning/src/ln/splicing_tests.rs

@@ -1177,7 +1177,7 @@ fn test_splice_out() {
 }
 
 #[test]
-fn test_splice_in_and_out() {
+fn test_splice_in_and_out_funds_outputs_from_inputs() {
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let mut config = test_default_channel_config();
@@ -1190,118 +1190,40 @@ fn test_splice_in_and_out() {
 	let (_, _, channel_id, _) =
 		create_announced_chan_between_nodes_with_value(&nodes, 0, 1, initial_channel_value_sat, 0);
 
-	let _ = send_payment(&nodes[0], &[&nodes[1]], 100_000);
-
-	// Contribute a net negative value, with fees taken from the contributed inputs and the
-	// remaining value sent to change
-	let htlc_limit_msat = nodes[0].node.list_channels()[0].next_outbound_htlc_limit_msat;
-	let added_value = Amount::from_sat(htlc_limit_msat / 1000);
-	let removed_value = added_value * 2;
-	let utxo_value = added_value * 3 / 4;
-	let fees = if cfg!(feature = "grind_signatures") {
-		Amount::from_sat(385)
-	} else {
-		Amount::from_sat(385)
-	};
-
-	assert!(htlc_limit_msat > initial_channel_value_sat / 2 * 1000);
-
-	provide_utxo_reserves(&nodes, 2, utxo_value);
-
+	let value_added = Amount::from_sat(20_000);
+	let utxo_value = Amount::from_sat(50_000);
 	let outputs = vec![
 		TxOut {
-			value: removed_value / 2,
+			value: Amount::from_sat(20_000),
 			script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(),
 		},
 		TxOut {
-			value: removed_value / 2,
+			value: Amount::from_sat(20_000),
 			script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(),
 		},
 	];
-	let funding_contribution =
-		do_initiate_splice_in_and_out(&nodes[0], &nodes[1], channel_id, added_value, outputs);
-
-	let (splice_tx, new_funding_script) =
-		splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution);
-	let expected_change = utxo_value * 2 - added_value - fees;
-	assert_eq!(
-		splice_tx
-			.output
-			.iter()
-			.filter(|txout| txout.value != removed_value / 2)
-			.find(|txout| txout.script_pubkey != new_funding_script)
-			.unwrap()
-			.value,
-		expected_change,
-	);
-
-	mine_transaction(&nodes[0], &splice_tx);
-	mine_transaction(&nodes[1], &splice_tx);
-
-	let htlc_limit_msat = nodes[0].node.list_channels()[0].next_outbound_htlc_limit_msat;
-	assert!(htlc_limit_msat < added_value.to_sat() * 1000);
-	let _ = send_payment(&nodes[0], &[&nodes[1]], htlc_limit_msat);
-
-	lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1);
-
-	let htlc_limit_msat = nodes[0].node.list_channels()[0].next_outbound_htlc_limit_msat;
-	assert!(htlc_limit_msat < added_value.to_sat() * 1000);
-	let _ = send_payment(&nodes[0], &[&nodes[1]], htlc_limit_msat);
-
-	// Contribute a net positive value, with fees taken from the contributed inputs and the
-	// remaining value sent to change
-	let added_value = Amount::from_sat(initial_channel_value_sat * 2);
-	let removed_value = added_value / 2;
-	let utxo_value = added_value * 3 / 4;
-	let fees = if cfg!(feature = "grind_signatures") {
-		Amount::from_sat(385)
-	} else {
-		Amount::from_sat(385)
-	};
-
-	// Clear UTXOs so that the change output from the previous splice isn't considered
-	nodes[0].wallet_source.clear_utxos();
-
 	provide_utxo_reserves(&nodes, 2, utxo_value);
 
-	let outputs = vec![
-		TxOut {
-			value: removed_value / 2,
-			script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(),
-		},
-		TxOut {
-			value: removed_value / 2,
-			script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(),
-		},
-	];
 	let funding_contribution =
-		do_initiate_splice_in_and_out(&nodes[0], &nodes[1], channel_id, added_value, outputs);
-
-	let (splice_tx, new_funding_script) =
-		splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution);
-	let expected_change = utxo_value * 2 - added_value - fees;
-	assert_eq!(
-		splice_tx
-			.output
-			.iter()
-			.filter(|txout| txout.value != removed_value / 2)
-			.find(|txout| txout.script_pubkey != new_funding_script)
-			.unwrap()
-			.value,
-		expected_change,
-	);
+		initiate_splice_in_and_out(&nodes[0], &nodes[1], channel_id, value_added, outputs);
+	let fees = Amount::from_sat(385);
+	let total_output_value: Amount =
+		funding_contribution.outputs().iter().map(|output| output.value).sum();
+	let expected_change = utxo_value * 2 - value_added - total_output_value - fees;
+	assert_eq!(funding_contribution.change_output().unwrap().value, expected_change);
+	assert!(funding_contribution.net_value() >= value_added.to_signed().unwrap());
 
+	let (splice_tx, _) =
+		splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution.clone());
 	mine_transaction(&nodes[0], &splice_tx);
 	mine_transaction(&nodes[1], &splice_tx);
-
-	let htlc_limit_msat = nodes[0].node.list_channels()[0].next_outbound_htlc_limit_msat;
-	assert_eq!(htlc_limit_msat, 0);
-
 	lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1);
 
-	let htlc_limit_msat = nodes[0].node.list_channels()[0].next_outbound_htlc_limit_msat;
-	assert!(htlc_limit_msat > initial_channel_value_sat / 2 * 1000);
-	let _ = send_payment(&nodes[0], &[&nodes[1]], htlc_limit_msat);
+	let channel = &nodes[0].node.list_channels()[0];
+	assert_eq!(
+		channel.channel_value_satoshis,
+		initial_channel_value_sat + funding_contribution.net_value().to_sat() as u64,
+	);
 }
 
 #[test]