Use CoinSelection::change_output when splicing

Now that CoinSelection is used to fund a splice funding transaction, use
that for determining of a change output should be used. Previously, the
initiator could either provide a change script upfront or let LDK
generate one using SignerProvider::get_destination_script.

Since older versions may have serialized a SpliceInstruction without a
change script while waiting on quiescence, LDK must still generate a
change output in this case.

Author: jkczyz

fuzz/src/chanmon_consistency.rs

@@ -1824,7 +1824,7 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], underlying_out:
 			},
 
 			0xa0 => {
-				let contribution = SpliceContribution::splice_in(Amount::from_sat(10_000), None);
+				let contribution = SpliceContribution::splice_in(Amount::from_sat(10_000));
 				let feerate_sat_per_kw =
 					fee_estimators[0].ret_val.load(atomic::Ordering::Acquire);
 				let feerate = FeeRate::from_sat_per_kwu(feerate_sat_per_kw as u64);
@@ -1842,7 +1842,7 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], underlying_out:
 				}
 			},
 			0xa1 => {
-				let contribution = SpliceContribution::splice_in(Amount::from_sat(10_000), None);
+				let contribution = SpliceContribution::splice_in(Amount::from_sat(10_000));
 				let feerate_sat_per_kw =
 					fee_estimators[1].ret_val.load(atomic::Ordering::Acquire);
 				let feerate = FeeRate::from_sat_per_kwu(feerate_sat_per_kw as u64);
@@ -1860,7 +1860,7 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], underlying_out:
 				}
 			},
 			0xa2 => {
-				let contribution = SpliceContribution::splice_in(Amount::from_sat(10_000), None);
+				let contribution = SpliceContribution::splice_in(Amount::from_sat(10_000));
 				let feerate_sat_per_kw =
 					fee_estimators[1].ret_val.load(atomic::Ordering::Acquire);
 				let feerate = FeeRate::from_sat_per_kwu(feerate_sat_per_kw as u64);
@@ -1878,7 +1878,7 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], underlying_out:
 				}
 			},
 			0xa3 => {
-				let contribution = SpliceContribution::splice_in(Amount::from_sat(10_000), None);
+				let contribution = SpliceContribution::splice_in(Amount::from_sat(10_000));
 				let feerate_sat_per_kw =
 					fee_estimators[2].ret_val.load(atomic::Ordering::Acquire);
 				let feerate = FeeRate::from_sat_per_kwu(feerate_sat_per_kw as u64);

lightning-tests/src/upgrade_downgrade_tests.rs

@@ -455,7 +455,8 @@ fn do_test_0_1_htlc_forward_after_splice(fail_htlc: bool) {
 		value: Amount::from_sat(1_000),
 		script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(),
 	}]);
-	let splice_tx = splice_channel(&nodes[0], &nodes[1], ChannelId(chan_id_bytes_a), contribution);
+	let (splice_tx, _) =
+		splice_channel(&nodes[0], &nodes[1], ChannelId(chan_id_bytes_a), contribution);
 	for node in nodes.iter() {
 		mine_transaction(node, &splice_tx);
 		connect_blocks(node, ANTI_REORG_DELAY - 1);

lightning/src/ln/channel.rs

@@ -2715,6 +2715,7 @@ impl_writeable_tlv_based!(PendingFunding, {
 enum FundingNegotiation {
 	AwaitingAck {
 		context: FundingNegotiationContext,
+		change_strategy: ChangeStrategy,
 		new_holder_funding_key: PublicKey,
 	},
 	ConstructingTransaction {
@@ -6547,18 +6548,25 @@ pub(super) struct FundingNegotiationContext {
 	/// The funding outputs we will be contributing to the channel.
 	#[allow(dead_code)] // TODO(dual_funding): Remove once contribution to V2 channels is enabled.
 	pub our_funding_outputs: Vec<TxOut>,
+}
+
+/// How the funding transaction's change is determined.
+#[derive(Debug)]
+pub(super) enum ChangeStrategy {
+	/// The change output, if any, is included in the FundingContribution's outputs.
+	FromCoinSelection,
+
 	/// The change output script. This will be used if needed or -- if not set -- generated using
 	/// `SignerProvider::get_destination_script`.
-	#[allow(dead_code)] // TODO(splicing): Remove once splicing is enabled.
-	pub change_script: Option<ScriptBuf>,
+	LegacyUserProvided(Option<ScriptBuf>),
 }
 
 impl FundingNegotiationContext {
 	/// Prepare and start interactive transaction negotiation.
 	/// If error occurs, it is caused by our side, not the counterparty.
 	fn into_interactive_tx_constructor<SP: Deref, ES: Deref>(
 		mut self, context: &ChannelContext<SP>, funding: &FundingScope, signer_provider: &SP,
-		entropy_source: &ES, holder_node_id: PublicKey,
+		entropy_source: &ES, holder_node_id: PublicKey, change_strategy: ChangeStrategy,
 	) -> Result<InteractiveTxConstructor, NegotiationError>
 	where
 		SP::Target: SignerProvider,
@@ -6583,46 +6591,15 @@ impl FundingNegotiationContext {
 			script_pubkey: funding.get_funding_redeemscript().to_p2wsh(),
 		};
 
-		// Optionally add change output
-		let change_value_opt = if !self.our_funding_inputs.is_empty() {
-			match calculate_change_output_value(
-				&self,
-				self.shared_funding_input.is_some(),
-				&shared_funding_output.script_pubkey,
-				context.holder_dust_limit_satoshis,
-			) {
-				Ok(change_value_opt) => change_value_opt,
-				Err(reason) => {
-					return Err(self.into_negotiation_error(reason));
-				},
-			}
-		} else {
-			None
-		};
-
-		if let Some(change_value) = change_value_opt {
-			let change_script = if let Some(script) = self.change_script {
-				script
-			} else {
-				match signer_provider.get_destination_script(context.channel_keys_id) {
-					Ok(script) => script,
-					Err(_) => {
-						let reason = AbortReason::InternalError("Error getting change script");
-						return Err(self.into_negotiation_error(reason));
-					},
-				}
-			};
-			let mut change_output = TxOut { value: change_value, script_pubkey: change_script };
-			let change_output_weight = get_output_weight(&change_output.script_pubkey).to_wu();
-			let change_output_fee =
-				fee_for_weight(self.funding_feerate_sat_per_1000_weight, change_output_weight);
-			let change_value_decreased_with_fee =
-				change_value.to_sat().saturating_sub(change_output_fee);
-			// Check dust limit again
-			if change_value_decreased_with_fee > context.holder_dust_limit_satoshis {
-				change_output.value = Amount::from_sat(change_value_decreased_with_fee);
-				self.our_funding_outputs.push(change_output);
-			}
+		match self.calculate_change_output(
+			context,
+			signer_provider,
+			&shared_funding_output,
+			change_strategy,
+		) {
+			Ok(Some(change_output)) => self.our_funding_outputs.push(change_output),
+			Ok(None) => {},
+			Err(reason) => return Err(self.into_negotiation_error(reason)),
 		}
 
 		let constructor_args = InteractiveTxConstructorArgs {
@@ -6644,6 +6621,55 @@ impl FundingNegotiationContext {
 		InteractiveTxConstructor::new(constructor_args)
 	}
 
+	fn calculate_change_output<SP: Deref>(
+		&self, context: &ChannelContext<SP>, signer_provider: &SP, shared_funding_output: &TxOut,
+		change_strategy: ChangeStrategy,
+	) -> Result<Option<TxOut>, AbortReason>
+	where
+		SP::Target: SignerProvider,
+	{
+		if self.our_funding_inputs.is_empty() {
+			return Ok(None);
+		}
+
+		let change_script = match change_strategy {
+			ChangeStrategy::FromCoinSelection => return Ok(None),
+			ChangeStrategy::LegacyUserProvided(change_script) => change_script,
+		};
+
+		let change_value = calculate_change_output_value(
+			&self,
+			self.shared_funding_input.is_some(),
+			&shared_funding_output.script_pubkey,
+			context.holder_dust_limit_satoshis,
+		)?;
+
+		if let Some(change_value) = change_value {
+			let change_script = match change_script {
+				Some(script) => script,
+				None => match signer_provider.get_destination_script(context.channel_keys_id) {
+					Ok(script) => script,
+					Err(_) => {
+						return Err(AbortReason::InternalError("Error getting change script"))
+					},
+				},
+			};
+			let mut change_output = TxOut { value: change_value, script_pubkey: change_script };
+			let change_output_weight = get_output_weight(&change_output.script_pubkey).to_wu();
+			let change_output_fee =
+				fee_for_weight(self.funding_feerate_sat_per_1000_weight, change_output_weight);
+			let change_value_decreased_with_fee =
+				change_value.to_sat().saturating_sub(change_output_fee);
+			// Check dust limit again
+			if change_value_decreased_with_fee > context.holder_dust_limit_satoshis {
+				change_output.value = Amount::from_sat(change_value_decreased_with_fee);
+				return Ok(Some(change_output));
+			}
+		}
+
+		Ok(None)
+	}
+
 	fn into_negotiation_error(self, reason: AbortReason) -> NegotiationError {
 		let (contributed_inputs, contributed_outputs) = self.into_contributed_inputs_and_outputs();
 		NegotiationError { reason, contributed_inputs, contributed_outputs }
@@ -12128,14 +12154,13 @@ where
 			shared_funding_input: Some(prev_funding_input),
 			our_funding_inputs,
 			our_funding_outputs,
-			change_script,
 		};
 
-		self.send_splice_init_internal(context)
+		self.send_splice_init_internal(context, ChangeStrategy::LegacyUserProvided(change_script))
 	}
 
 	fn send_splice_init_internal(
-		&mut self, context: FundingNegotiationContext,
+		&mut self, context: FundingNegotiationContext, change_strategy: ChangeStrategy,
 	) -> msgs::SpliceInit {
 		debug_assert!(self.pending_splice.is_none());
 		// Rotate the funding pubkey using the prev_funding_txid as a tweak
@@ -12156,8 +12181,11 @@ where
 		let funding_contribution_satoshis = context.our_funding_contribution.to_sat();
 		let locktime = context.funding_tx_locktime.to_consensus_u32();
 
-		let funding_negotiation =
-			FundingNegotiation::AwaitingAck { context, new_holder_funding_key: funding_pubkey };
+		let funding_negotiation = FundingNegotiation::AwaitingAck {
+			context,
+			change_strategy,
+			new_holder_funding_key: funding_pubkey,
+		};
 		self.pending_splice = Some(PendingFunding {
 			funding_negotiation: Some(funding_negotiation),
 			negotiated_candidates: vec![],
@@ -12387,7 +12415,6 @@ where
 			shared_funding_input: Some(prev_funding_input),
 			our_funding_inputs: Vec::new(),
 			our_funding_outputs: Vec::new(),
-			change_script: None,
 		};
 
 		let mut interactive_tx_constructor = funding_negotiation_context
@@ -12397,6 +12424,8 @@ where
 				signer_provider,
 				entropy_source,
 				holder_node_id.clone(),
+				// ChangeStrategy doesn't matter when no inputs are contributed
+				ChangeStrategy::FromCoinSelection,
 			)
 			.map_err(|err| {
 				ChannelError::WarnAndDisconnect(format!(
@@ -12451,11 +12480,11 @@ where
 		let pending_splice =
 			self.pending_splice.as_mut().expect("We should have returned an error earlier!");
 		// TODO: Good candidate for a let else statement once MSRV >= 1.65
-		let funding_negotiation_context =
-			if let Some(FundingNegotiation::AwaitingAck { context, .. }) =
+		let (funding_negotiation_context, change_strategy) =
+			if let Some(FundingNegotiation::AwaitingAck { context, change_strategy, .. }) =
 				pending_splice.funding_negotiation.take()
 			{
-				context
+				(context, change_strategy)
 			} else {
 				panic!("We should have returned an error earlier!");
 			};
@@ -12467,6 +12496,7 @@ where
 				signer_provider,
 				entropy_source,
 				holder_node_id.clone(),
+				change_strategy,
 			)
 			.map_err(|err| {
 				ChannelError::WarnAndDisconnect(format!(
@@ -12497,7 +12527,7 @@ where
 		let (funding_negotiation_context, new_holder_funding_key) = match &pending_splice
 			.funding_negotiation
 		{
-			Some(FundingNegotiation::AwaitingAck { context, new_holder_funding_key }) => {
+			Some(FundingNegotiation::AwaitingAck { context, new_holder_funding_key, .. }) => {
 				(context, new_holder_funding_key)
 			},
 			Some(FundingNegotiation::ConstructingTransaction { .. })
@@ -13460,7 +13490,7 @@ where
 						},
 					};
 					let funding_feerate_per_kw = contribution.feerate().to_sat_per_kwu() as u32;
-					let (our_funding_inputs, our_funding_outputs, change_script) = contribution.into_tx_parts();
+					let (our_funding_inputs, our_funding_outputs) = contribution.into_tx_parts();
 
 					let context = FundingNegotiationContext {
 						is_initiator,
@@ -13470,10 +13500,9 @@ where
 						shared_funding_input: Some(prev_funding_input),
 						our_funding_inputs,
 						our_funding_outputs,
-						change_script,
 					};
 
-					let splice_init = self.send_splice_init_internal(context);
+					let splice_init = self.send_splice_init_internal(context, ChangeStrategy::FromCoinSelection);
 					return Ok(Some(StfuResponse::SpliceInit(splice_init)));
 				},
 				#[cfg(any(test, fuzzing))]
@@ -14310,7 +14339,6 @@ where
 			shared_funding_input: None,
 			our_funding_inputs: funding_inputs,
 			our_funding_outputs: Vec::new(),
-			change_script: None,
 		};
 		let chan = Self {
 			funding,
@@ -14464,7 +14492,6 @@ where
 			shared_funding_input: None,
 			our_funding_inputs: our_funding_inputs.clone(),
 			our_funding_outputs: Vec::new(),
-			change_script: None,
 		};
 		let shared_funding_output = TxOut {
 			value: Amount::from_sat(funding.get_value_satoshis()),