Apply review changes

- Remove splice-specific input (not yet relevant)
- Extract helper method in DualFundingChannelContext to extract prev outs
- Second dust check on change output (after subtracting fee)
- Add error case when inputs are insufficient
- Add optional change destination parameter

Author: optout21

lightning/src/ln/channel.rs

@@ -31,7 +31,7 @@ use crate::ln::types::ChannelId;
 use crate::types::payment::{PaymentPreimage, PaymentHash};
 use crate::types::features::{ChannelTypeFeatures, InitFeatures};
 use crate::ln::interactivetxs::{
-	get_output_weight, need_to_add_funding_change_output, HandleTxCompleteValue, HandleTxCompleteResult, InteractiveTxConstructor,
+	get_output_weight, calculate_change_output_value, HandleTxCompleteValue, HandleTxCompleteResult, InteractiveTxConstructor,
 	InteractiveTxConstructorArgs, InteractiveTxMessageSend, InteractiveTxSigningSession, InteractiveTxMessageSendResult,
 	OutputOwned, SharedOwnedOutput, TX_COMMON_FIELDS_WEIGHT,
 };
@@ -2213,34 +2213,22 @@ impl<SP: Deref> InitialRemoteCommitmentReceiver<SP> for FundedChannel<SP> where
 }
 
 impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
+	/// Prepare and start interactive transaction negotiation.
+	/// `change_destination_opt` - Optional destination for optional change; if None, default destination address is used.
 	#[allow(dead_code)] // TODO(dual_funding): Remove once contribution to V2 channels is enabled
 	fn begin_interactive_funding_tx_construction<ES: Deref>(
 		&mut self, signer_provider: &SP, entropy_source: &ES, holder_node_id: PublicKey,
-		extra_input: Option<(TxIn, TransactionU16LenLimited)>,
+		change_destination_opt: Option<ScriptBuf>,
 	) -> Result<Option<InteractiveTxMessageSend>, APIError>
 	where ES::Target: EntropySource
 	{
-		let mut funding_inputs_with_extra = self.dual_funding_context.our_funding_inputs.take().unwrap_or_else(|| vec![]);
+		let mut funding_inputs = Vec::new();
+		mem::swap(&mut self.dual_funding_context.our_funding_inputs, &mut funding_inputs);
 
-		if let Some(extra_input) = extra_input {
-			funding_inputs_with_extra.push(extra_input);
-		}
-
-		let mut funding_inputs_prev_outputs: Vec<TxOut> = Vec::with_capacity(funding_inputs_with_extra.len());
-		// Check that vouts exist for each TxIn in provided transactions.
-		for (idx, input) in funding_inputs_with_extra.iter().enumerate() {
-			if let Some(output) = input.1.as_transaction().output.get(input.0.previous_output.vout as usize) {
-				funding_inputs_prev_outputs.push(output.clone());
-			} else {
-				return Err(APIError::APIMisuseError {
-					err: format!("Transaction with txid {} does not have an output with vout of {} corresponding to TxIn at funding_inputs_with_extra[{}]",
-						input.1.as_transaction().compute_txid(), input.0.previous_output.vout, idx) });
-			}
-		}
+		let funding_inputs_prev_outputs = DualFundingChannelContext::txouts_from_input_prev_txs(&funding_inputs)
+			.map_err(|err| APIError::APIMisuseError { err: err.to_string() })?;
 
-		let total_input_satoshis: u64 = funding_inputs_with_extra.iter().map(
-			|input| input.1.as_transaction().output.get(input.0.previous_output.vout as usize).map(|out| out.value.to_sat()).unwrap_or(0)
-		).sum();
+		let total_input_satoshis: u64 = funding_inputs_prev_outputs.iter().map(|txout| txout.value.to_sat()).sum();
 		if total_input_satoshis < self.dual_funding_context.our_funding_satoshis {
 			return Err(APIError::APIMisuseError {
 				err: format!("Total value of funding inputs must be at least funding amount. It was {} sats",
@@ -2271,18 +2259,38 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 		};
 
 		// Optionally add change output
-		if let Some(change_value) = need_to_add_funding_change_output(
+		let change_value_opt = calculate_change_output_value(
 			self.funding.is_outbound(), self.dual_funding_context.our_funding_satoshis,
 			&funding_inputs_prev_outputs, &funding_outputs,
 			self.dual_funding_context.funding_feerate_sat_per_1000_weight,
 			self.context.holder_dust_limit_satoshis,
-		) {
-			let change_script = signer_provider.get_destination_script(self.context.channel_keys_id).map_err(
-				|err| APIError::APIMisuseError {
-					err: format!("Failed to get change script as new destination script, {:?}", err),
-				})?;
-			let _res = add_funding_change_output(
-				change_value, change_script, &mut funding_outputs, self.dual_funding_context.funding_feerate_sat_per_1000_weight);
+		).map_err(|err| APIError::APIMisuseError {
+			err: format!("Insufficient inputs, cannot cover intended contribution of {} and fees; {}",
+				self.dual_funding_context.our_funding_satoshis, err
+			),
+		})?;
+		if let Some(change_value) = change_value_opt {
+			let change_script = match change_destination_opt {
+				Some(script) => script,
+				None => {
+					signer_provider.get_destination_script(self.context.channel_keys_id).map_err(
+						|err| APIError::APIMisuseError {
+							err: format!("Failed to get change script as new destination script, {:?}", err),
+						})?
+				}
+			};
+			let mut change_output = TxOut {
+				value: Amount::from_sat(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.dual_funding_context.funding_feerate_sat_per_1000_weight, change_output_weight);
+			let change_value_decreased_with_fee = change_value.saturating_sub(change_output_fee);
+			// Check dust limit again
+			if change_value_decreased_with_fee > self.context.holder_dust_limit_satoshis {
+				change_output.value = Amount::from_sat(change_value_decreased_with_fee);
+				funding_outputs.push(OutputOwned::Single(change_output));
+			}
 		}
 
 		let constructor_args = InteractiveTxConstructorArgs {
@@ -2293,15 +2301,15 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 			feerate_sat_per_kw: self.dual_funding_context.funding_feerate_sat_per_1000_weight,
 			is_initiator: self.funding.is_outbound(),
 			funding_tx_locktime: self.dual_funding_context.funding_tx_locktime,
-			inputs_to_contribute: funding_inputs_with_extra,
+			inputs_to_contribute: funding_inputs,
 			outputs_to_contribute: funding_outputs,
 			expected_remote_shared_funding_output,
 		};
 		let mut tx_constructor = InteractiveTxConstructor::new(constructor_args)
 			.map_err(|_| APIError::APIMisuseError { err: "Incorrect shared output provided".into() })?;
 		let msg = tx_constructor.take_initiator_first_message();
 
-		self.interactive_tx_constructor.replace(tx_constructor);
+		self.interactive_tx_constructor = Some(tx_constructor);
 
 		Ok(msg)
 	}
@@ -4764,7 +4772,7 @@ fn estimate_v2_funding_transaction_fee(
 fn add_funding_change_output(
 	change_value: u64, change_script: ScriptBuf,
 	funding_outputs: &mut Vec<OutputOwned>, funding_feerate_sat_per_1000_weight: u32,
-) -> TxOut {
+) {
 	let mut change_output = TxOut {
 		value: Amount::from_sat(change_value),
 		script_pubkey: change_script,
@@ -4773,7 +4781,6 @@ fn add_funding_change_output(
 	let change_output_fee = fee_for_weight(funding_feerate_sat_per_1000_weight, change_output_weight);
 	change_output.value = Amount::from_sat(change_value.saturating_sub(change_output_fee));
 	funding_outputs.push(OutputOwned::Single(change_output.clone()));
-	change_output
 }
 
 /// Context for dual-funded channels.
@@ -4794,8 +4801,37 @@ pub(super) struct DualFundingChannelContext {
 	/// Note that the `our_funding_satoshis` field is equal to the total value of `our_funding_inputs`
 	/// minus any fees paid for our contributed weight. This means that change will never be generated
 	/// and the maximum value possible will go towards funding the channel.
+	///
+	/// Note that this field may be emptied once the interactive negotiation has been started.
 	#[allow(dead_code)] // TODO(dual_funding): Remove once contribution to V2 channels is enabled.
-	pub our_funding_inputs: Option<Vec<(TxIn, TransactionU16LenLimited)>>,
+	pub our_funding_inputs: Vec<(TxIn, TransactionU16LenLimited)>,
+}
+
+impl DualFundingChannelContext {
+	/// Obtain prev outputs for each supplied input and matching transaction.
+	/// Will error when a prev tx does not have an output for the specified vout.
+	/// Also checks for matching of transaction IDs.
+	fn txouts_from_input_prev_txs(inputs: &Vec<(TxIn, TransactionU16LenLimited)>) -> Result<Vec<&TxOut>, ChannelError> {
+		let mut prev_outputs: Vec<&TxOut> = Vec::with_capacity(inputs.len());
+		// Check that vouts exist for each TxIn in provided transactions.
+		for (idx, (txin, tx)) in inputs.iter().enumerate() {
+			let txid = tx.as_transaction().compute_txid();
+			if txin.previous_output.txid != txid {
+				return Err(ChannelError::Warn(
+					format!("Transaction input txid mismatch, {} vs. {}, at index {}", txin.previous_output.txid, txid, idx)
+				));
+			}
+			if let Some(output) = tx.as_transaction().output.get(txin.previous_output.vout as usize) {
+				prev_outputs.push(output);
+			} else {
+				return Err(ChannelError::Warn(
+					format!("Transaction with txid {} does not have an output with vout of {} corresponding to TxIn, at index {}",
+						txid, txin.previous_output.vout, idx)
+				));
+			}
+		}
+		Ok(prev_outputs)
+	}
 }
 
 // Holder designates channel data owned for the benefit of the user client.
@@ -9714,17 +9750,18 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 			unfunded_channel_age_ticks: 0,
 			holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx),
 		};
+		let dual_funding_context = DualFundingChannelContext {
+			our_funding_satoshis: funding_satoshis,
+			their_funding_satoshis: None,
+			funding_tx_locktime,
+			funding_feerate_sat_per_1000_weight,
+			our_funding_inputs: funding_inputs,
+		};
 		let chan = Self {
 			funding,
 			context,
 			unfunded_context,
-			dual_funding_context: DualFundingChannelContext {
-				our_funding_satoshis: funding_satoshis,
-				their_funding_satoshis: None,
-				funding_tx_locktime,
-				funding_feerate_sat_per_1000_weight,
-				our_funding_inputs: Some(funding_inputs),
-			},
+			dual_funding_context,
 			interactive_tx_constructor: None,
 			interactive_tx_signing_session: None,
 		};
@@ -9869,7 +9906,7 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 			their_funding_satoshis: Some(msg.common_fields.funding_satoshis),
 			funding_tx_locktime: LockTime::from_consensus(msg.locktime),
 			funding_feerate_sat_per_1000_weight: msg.funding_feerate_sat_per_1000_weight,
-			our_funding_inputs: Some(our_funding_inputs.clone()),
+			our_funding_inputs: our_funding_inputs.clone(),
 		};
 
 		let interactive_tx_constructor = Some(InteractiveTxConstructor::new(