Consume InteractiveTxConstructor after error checks

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 1ca067f..585e1ac 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -60,7 +60,8 @@ use crate::ln::funding::{FundingTxInput, SpliceContribution};
 use crate::ln::interactivetxs::{
 	calculate_change_output_value, get_output_weight, AbortReason, HandleTxCompleteValue,
 	InteractiveTxConstructor, InteractiveTxConstructorArgs, InteractiveTxMessageSend,
-	InteractiveTxSigningSession, SharedOwnedInput, SharedOwnedOutput, TX_COMMON_FIELDS_WEIGHT,
+	InteractiveTxSigningSession, NegotiationError, SharedOwnedInput, SharedOwnedOutput,
+	TX_COMMON_FIELDS_WEIGHT,
 };
 use crate::ln::msgs;
 use crate::ln::msgs::{ClosingSigned, ClosingSignedFeeRange, DecodeError, OnionErrorPacket};
@@ -1794,20 +1795,22 @@ where
 
 		let (interactive_tx_msg_send, negotiation_complete) = match tx_complete_action {
 			HandleTxCompleteValue::SendTxMessage(interactive_tx_msg_send) => {
-				(Some(interactive_tx_msg_send), false)
+				(Some(interactive_tx_msg_send), None)
 			},
-			HandleTxCompleteValue::SendTxComplete(
+			HandleTxCompleteValue::NegotiationComplete(
 				interactive_tx_msg_send,
-				negotiation_complete,
-			) => (Some(interactive_tx_msg_send), negotiation_complete),
-			HandleTxCompleteValue::NegotiationComplete => (None, true),
+				funding_outpoint,
+			) => (interactive_tx_msg_send, Some(funding_outpoint)),
 		};
-		if !negotiation_complete {
+
+		let funding_outpoint = if let Some(funding_outpoint) = negotiation_complete {
+			funding_outpoint
+		} else {
 			return Ok((interactive_tx_msg_send, None));
-		}
+		};
 
 		let commitment_signed = self
-			.funding_tx_constructed(logger)
+			.funding_tx_constructed(funding_outpoint, logger)
 			.map_err(|abort_reason| self.fail_interactive_tx_negotiation(abort_reason, logger))?;
 		Ok((interactive_tx_msg_send, Some(commitment_signed)))
 	}
@@ -1890,13 +1893,13 @@ where
 	}
 
 	fn funding_tx_constructed<L: Deref>(
-		&mut self, logger: &L,
+		&mut self, funding_outpoint: OutPoint, logger: &L,
 	) -> Result<msgs::CommitmentSigned, AbortReason>
 	where
 		L::Target: Logger,
 	{
 		let logger = WithChannelContext::from(logger, self.context(), None);
-		match &mut self.phase {
+		let (interactive_tx_constructor, commitment_signed) = match &mut self.phase {
 			ChannelPhase::UnfundedV2(chan) => {
 				debug_assert_eq!(
 					chan.context.channel_state,
@@ -1906,52 +1909,73 @@ where
 					),
 				);
 
-				let signing_session = chan
+				let interactive_tx_constructor = chan
 					.interactive_tx_constructor
 					.take()
-					.expect("PendingV2Channel::interactive_tx_constructor should be set")
-					.into_signing_session();
+					.expect("PendingV2Channel::interactive_tx_constructor should be set");
 				let commitment_signed = chan.context.funding_tx_constructed(
 					&mut chan.funding,
-					signing_session,
+					funding_outpoint,
 					false,
 					chan.unfunded_context.transaction_number(),
 					&&logger,
 				)?;
 
-				return Ok(commitment_signed);
+				(interactive_tx_constructor, commitment_signed)
 			},
 			ChannelPhase::Funded(chan) => {
 				if let Some(pending_splice) = chan.pending_splice.as_mut() {
-					if let Some(funding_negotiation) = pending_splice.funding_negotiation.take() {
-						if let FundingNegotiation::ConstructingTransaction {
-							mut funding,
-							interactive_tx_constructor,
-						} = funding_negotiation
-						{
-							let signing_session = interactive_tx_constructor.into_signing_session();
-							let commitment_signed = chan.context.funding_tx_constructed(
+					pending_splice
+						.funding_negotiation
+						.take()
+						.and_then(|funding_negotiation| {
+							if let FundingNegotiation::ConstructingTransaction {
+								funding,
+								interactive_tx_constructor,
+							} = funding_negotiation
+							{
+								Some((funding, interactive_tx_constructor))
+							} else {
+								// Replace the taken state for later error handling
+								pending_splice.funding_negotiation = Some(funding_negotiation);
+								None
+							}
+						})
+						.ok_or_else(|| {
+							AbortReason::InternalError(
+								"Got a tx_complete message in an invalid state",
+							)
+						})
+						.and_then(|(mut funding, interactive_tx_constructor)| {
+							match chan.context.funding_tx_constructed(
 								&mut funding,
-								signing_session,
+								funding_outpoint,
 								true,
 								chan.holder_commitment_point.next_transaction_number(),
 								&&logger,
-							)?;
-
-							pending_splice.funding_negotiation =
-								Some(FundingNegotiation::AwaitingSignatures { funding });
-
-							return Ok(commitment_signed);
-						} else {
-							// Replace the taken state
-							pending_splice.funding_negotiation = Some(funding_negotiation);
-						}
-					}
+							) {
+								Ok(commitment_signed) => {
+									// Advance the state
+									pending_splice.funding_negotiation =
+										Some(FundingNegotiation::AwaitingSignatures { funding });
+									Ok((interactive_tx_constructor, commitment_signed))
+								},
+								Err(e) => {
+									// Restore the taken state for later error handling
+									pending_splice.funding_negotiation =
+										Some(FundingNegotiation::ConstructingTransaction {
+											funding,
+											interactive_tx_constructor,
+										});
+									Err(e)
+								},
+							}
+						})?
+				} else {
+					return Err(AbortReason::InternalError(
+						"Got a tx_complete message in an invalid state",
+					));
 				}
-
-				return Err(AbortReason::InternalError(
-					"Got a tx_complete message in an invalid state",
-				));
 			},
 			_ => {
 				debug_assert!(false);
@@ -1959,7 +1983,11 @@ where
 					"Got a tx_complete message in an invalid phase",
 				));
 			},
-		}
+		};
+
+		let signing_session = interactive_tx_constructor.into_signing_session();
+		self.context_mut().interactive_tx_signing_session = Some(signing_session);
+		Ok(commitment_signed)
 	}
 
 	pub fn force_shutdown(&mut self, closure_reason: ClosureReason) -> ShutdownResult {
@@ -6051,30 +6079,13 @@ where
 
 	#[rustfmt::skip]
 	fn funding_tx_constructed<L: Deref>(
-		&mut self, funding: &mut FundingScope, signing_session: InteractiveTxSigningSession,
-		is_splice: bool, holder_commitment_transaction_number: u64, logger: &L
+		&mut self, funding: &mut FundingScope, funding_outpoint: OutPoint, is_splice: bool,
+		holder_commitment_transaction_number: u64, logger: &L,
 	) -> Result<msgs::CommitmentSigned, AbortReason>
 	where
 		L::Target: Logger
 	{
-		let mut output_index = None;
-		let expected_spk = funding.get_funding_redeemscript().to_p2wsh();
-		for (idx, outp) in signing_session.unsigned_tx().tx().output.iter().enumerate() {
-			if outp.script_pubkey == expected_spk && outp.value.to_sat() == funding.get_value_satoshis() {
-				if output_index.is_some() {
-					return Err(AbortReason::DuplicateFundingOutput);
-				}
-				output_index = Some(idx as u16);
-			}
-		}
-		let outpoint = if let Some(output_index) = output_index {
-			OutPoint { txid: signing_session.unsigned_tx().compute_txid(), index: output_index }
-		} else {
-			return Err(AbortReason::MissingFundingOutput);
-		};
-		funding
-			.channel_transaction_parameters.funding_outpoint = Some(outpoint);
-		self.interactive_tx_signing_session = Some(signing_session);
+		funding.channel_transaction_parameters.funding_outpoint = Some(funding_outpoint);
 
 		if is_splice {
 			debug_assert_eq!(
@@ -6091,7 +6102,6 @@ where
 			Some(commitment_signed) => commitment_signed,
 			// TODO(splicing): Support async signing
 			None => {
-				funding.channel_transaction_parameters.funding_outpoint = None;
 				return Err(AbortReason::InternalError("Failed to compute commitment_signed signatures"));
 			},
 		};
@@ -6167,8 +6177,6 @@ where
 		SP::Target: SignerProvider,
 		L::Target: Logger,
 	{
-		debug_assert!(self.interactive_tx_signing_session.is_some());
-
 		let signatures = self.get_initial_counterparty_commitment_signatures(funding, logger);
 		if let Some((signature, htlc_signatures)) = signatures {
 			log_info!(
@@ -6508,9 +6516,9 @@ 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>(
-		self, context: &ChannelContext<SP>, funding: &FundingScope, signer_provider: &SP,
+		mut self, context: &ChannelContext<SP>, funding: &FundingScope, signer_provider: &SP,
 		entropy_source: &ES, holder_node_id: PublicKey,
-	) -> Result<InteractiveTxConstructor, AbortReason>
+	) -> Result<InteractiveTxConstructor, NegotiationError>
 	where
 		SP::Target: SignerProvider,
 		ES::Target: EntropySource,
@@ -6536,25 +6544,32 @@ impl FundingNegotiationContext {
 
 		// Optionally add change output
 		let change_value_opt = if self.our_funding_contribution > SignedAmount::ZERO {
-			calculate_change_output_value(
+			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
 		};
 
-		let mut funding_outputs = self.our_funding_outputs;
-
 		if let Some(change_value) = change_value_opt {
 			let change_script = if let Some(script) = self.change_script {
 				script
 			} else {
-				signer_provider
-					.get_destination_script(context.channel_keys_id)
-					.map_err(|_err| AbortReason::InternalError("Error getting change script"))?
+				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: Amount::from_sat(change_value), script_pubkey: change_script };
@@ -6565,7 +6580,7 @@ impl FundingNegotiationContext {
 			// 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);
-				funding_outputs.push(change_output);
+				self.our_funding_outputs.push(change_output);
 			}
 		}
 
@@ -6583,10 +6598,19 @@ impl FundingNegotiationContext {
 				shared_funding_output,
 				funding.value_to_self_msat / 1000,
 			),
-			outputs_to_contribute: funding_outputs,
+			outputs_to_contribute: self.our_funding_outputs,
 		};
 		InteractiveTxConstructor::new(constructor_args)
 	}
+
+	fn into_negotiation_error(self, reason: AbortReason) -> NegotiationError {
+		let contributed_inputs =
+			self.our_funding_inputs.into_iter().map(|input| input.utxo.outpoint).collect();
+
+		let contributed_outputs = self.our_funding_outputs;
+
+		NegotiationError { reason, contributed_inputs, contributed_outputs }
+	}
 }
 
 // Holder designates channel data owned for the benefit of the user client.
@@ -13660,8 +13684,8 @@ where
 				outputs_to_contribute: funding_negotiation_context.our_funding_outputs.clone(),
 			}
 		).map_err(|err| {
-			let reason = ClosureReason::ProcessingError { err: err.to_string() };
-			ChannelError::Close((err.to_string(), reason))
+			let reason = ClosureReason::ProcessingError { err: err.reason.to_string() };
+			ChannelError::Close((err.reason.to_string(), reason))
 		})?);
 
 		let unfunded_context = UnfundedChannelContext {
diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs
index ea9994d..d1cac89 100644
--- a/lightning/src/ln/interactivetxs.rs
+++ b/lightning/src/ln/interactivetxs.rs
@@ -7,6 +7,7 @@
 // You may not use this file except in accordance with one or both of these
 // licenses.
 
+use crate::chain::transaction::OutPoint;
 use crate::io_extras::sink;
 use crate::prelude::*;
 
@@ -21,8 +22,8 @@ use bitcoin::secp256k1::{Message, PublicKey};
 use bitcoin::sighash::SighashCache;
 use bitcoin::transaction::Version;
 use bitcoin::{
-	sighash, EcdsaSighashType, OutPoint, ScriptBuf, Sequence, TapSighashType, Transaction, TxIn,
-	TxOut, Txid, Weight, Witness, XOnlyPublicKey,
+	sighash, EcdsaSighashType, OutPoint as BitcoinOutPoint, ScriptBuf, Sequence, TapSighashType,
+	Transaction, TxIn, TxOut, Txid, Weight, Witness, XOnlyPublicKey,
 };
 
 use crate::chain::chaininterface::fee_for_weight;
@@ -88,6 +89,13 @@ impl SerialIdExt for SerialId {
 	}
 }
 
+#[derive(Clone, Debug)]
+pub(crate) struct NegotiationError {
+	pub reason: AbortReason,
+	pub contributed_inputs: Vec<BitcoinOutPoint>,
+	pub contributed_outputs: Vec<TxOut>,
+}
+
 #[derive(Debug, Clone, Copy, PartialEq)]
 pub(crate) enum AbortReason {
 	InvalidStateTransition,
@@ -336,6 +344,36 @@ impl ConstructedTransaction {
 		Ok(tx)
 	}
 
+	fn into_negotiation_error(self, reason: AbortReason) -> NegotiationError {
+		let contributed_inputs = self
+			.tx
+			.input
+			.into_iter()
+			.zip(self.input_metadata.iter())
+			.enumerate()
+			.filter(|(_, (_, input))| input.is_local(self.holder_is_initiator))
+			.filter(|(index, _)| {
+				self.shared_input_index
+					.map(|shared_index| *index != shared_index as usize)
+					.unwrap_or(true)
+			})
+			.map(|(_, (txin, _))| txin.previous_output)
+			.collect();
+
+		let contributed_outputs = self
+			.tx
+			.output
+			.into_iter()
+			.zip(self.output_metadata.iter())
+			.enumerate()
+			.filter(|(_, (_, output))| output.is_local(self.holder_is_initiator))
+			.filter(|(index, _)| *index != self.shared_output_index as usize)
+			.map(|(_, (txout, _))| txout)
+			.collect();
+
+		NegotiationError { reason, contributed_inputs, contributed_outputs }
+	}
+
 	pub fn tx(&self) -> &Transaction {
 		&self.tx
 	}
@@ -348,6 +386,10 @@ impl ConstructedTransaction {
 		self.tx().compute_txid()
 	}
 
+	fn funding_outpoint(&self) -> OutPoint {
+		OutPoint { txid: self.compute_txid(), index: self.shared_output_index }
+	}
+
 	/// Returns the total input value from all local contributions, including the entire shared
 	/// input value if applicable.
 	fn local_contributed_input_value(&self) -> Amount {
@@ -806,6 +848,10 @@ impl InteractiveTxSigningSession {
 
 		Ok(())
 	}
+
+	pub(crate) fn into_negotiation_error(self, reason: AbortReason) -> NegotiationError {
+		self.unsigned_tx.into_negotiation_error(reason)
+	}
 }
 
 impl_writeable_tlv_based!(InteractiveTxSigningSession, {
@@ -840,7 +886,7 @@ struct NegotiationContext {
 	/// - For the acceptor:
 	/// The output expected as new funding output. It should be added by the initiator node.
 	shared_funding_output: SharedOwnedOutput,
-	prevtx_outpoints: HashSet<OutPoint>,
+	prevtx_outpoints: HashSet<BitcoinOutPoint>,
 	/// The outputs added so far.
 	outputs: HashMap<SerialId, InteractiveTxOutput>,
 	/// The locktime of the funding transaction.
@@ -990,7 +1036,7 @@ impl NegotiationContext {
 					return Err(AbortReason::DuplicateFundingInput);
 				}
 
-				let previous_output = OutPoint { txid: *shared_txid, vout: msg.prevtx_out };
+				let previous_output = BitcoinOutPoint { txid: *shared_txid, vout: msg.prevtx_out };
 				if previous_output != shared_funding_input.input.previous_output {
 					return Err(AbortReason::UnexpectedFundingInput);
 				}
@@ -1010,7 +1056,7 @@ impl NegotiationContext {
 					return Err(AbortReason::PrevTxOutInvalid);
 				}
 
-				let prev_outpoint = OutPoint { txid, vout: msg.prevtx_out };
+				let prev_outpoint = BitcoinOutPoint { txid, vout: msg.prevtx_out };
 				let txin = TxIn {
 					previous_output: prev_outpoint,
 					sequence: Sequence(msg.sequence),
@@ -1179,7 +1225,7 @@ impl NegotiationContext {
 	) -> Result<(), AbortReason> {
 		let vout = msg.prevtx_out as usize;
 		let (prev_outpoint, input) = if let Some(shared_input_txid) = msg.shared_input_txid {
-			let prev_outpoint = OutPoint { txid: shared_input_txid, vout: msg.prevtx_out };
+			let prev_outpoint = BitcoinOutPoint { txid: shared_input_txid, vout: msg.prevtx_out };
 			if let Some(shared_funding_input) = &self.shared_funding_input {
 				(prev_outpoint, InputOwned::Shared(shared_funding_input.clone()))
 			} else {
@@ -1187,7 +1233,7 @@ impl NegotiationContext {
 			}
 		} else if let Some(prevtx) = &msg.prevtx {
 			let prev_txid = prevtx.compute_txid();
-			let prev_outpoint = OutPoint { txid: prev_txid, vout: msg.prevtx_out };
+			let prev_outpoint = BitcoinOutPoint { txid: prev_txid, vout: msg.prevtx_out };
 			let prev_output = prevtx.output.get(vout).ok_or(AbortReason::PrevTxOutInvalid)?.clone();
 			let txin = TxIn {
 				previous_output: prev_outpoint,
@@ -1610,6 +1656,13 @@ impl InputOwned {
 		}
 	}
 
+	fn into_tx_in(self) -> TxIn {
+		match self {
+			InputOwned::Single(single) => single.input,
+			InputOwned::Shared(shared) => shared.input,
+		}
+	}
+
 	pub fn value(&self) -> u64 {
 		match self {
 			InputOwned::Single(single) => single.prev_output.value.to_sat(),
@@ -1891,8 +1944,7 @@ where
 
 pub(super) enum HandleTxCompleteValue {
 	SendTxMessage(InteractiveTxMessageSend),
-	SendTxComplete(InteractiveTxMessageSend, bool),
-	NegotiationComplete,
+	NegotiationComplete(Option<InteractiveTxMessageSend>, OutPoint),
 }
 
 pub(super) struct InteractiveTxConstructorArgs<'a, ES: Deref>
@@ -1917,7 +1969,7 @@ impl InteractiveTxConstructor {
 	///
 	/// If the holder is the initiator, they need to send the first message which is a `TxAddInput`
 	/// message.
-	pub fn new<ES: Deref>(args: InteractiveTxConstructorArgs<ES>) -> Result<Self, AbortReason>
+	pub fn new<ES: Deref>(args: InteractiveTxConstructorArgs<ES>) -> Result<Self, NegotiationError>
 	where
 		ES::Target: EntropySource,
 	{
@@ -2004,11 +2056,36 @@ impl InteractiveTxConstructor {
 		};
 		// We'll store the first message for the initiator.
 		if is_initiator {
-			constructor.initiator_first_message = Some(constructor.maybe_send_message()?);
+			match constructor.maybe_send_message() {
+				Ok(message) => {
+					constructor.initiator_first_message = Some(message);
+				},
+				Err(reason) => {
+					return Err(constructor.into_negotiation_error(reason));
+				},
+			}
 		}
 		Ok(constructor)
 	}
 
+	fn into_negotiation_error(self, reason: AbortReason) -> NegotiationError {
+		NegotiationError {
+			reason,
+			contributed_inputs: self
+				.inputs_to_contribute
+				.into_iter()
+				.filter(|(_, input)| !input.is_shared())
+				.map(|(_, input)| input.into_tx_in().previous_output)
+				.collect(),
+			contributed_outputs: self
+				.outputs_to_contribute
+				.into_iter()
+				.filter(|(_, output)| !output.is_shared())
+				.map(|(_, output)| output.into_tx_out())
+				.collect(),
+		}
+	}
+
 	pub fn take_initiator_first_message(&mut self) -> Option<InteractiveTxMessageSend> {
 		self.initiator_first_message.take()
 	}
@@ -2114,8 +2191,13 @@ impl InteractiveTxConstructor {
 			StateMachine::ReceivedTxComplete(_) => {
 				let msg_send = self.maybe_send_message()?;
 				match &self.state_machine {
-					StateMachine::NegotiationComplete(_) => {
-						Ok(HandleTxCompleteValue::SendTxComplete(msg_send, true))
+					StateMachine::NegotiationComplete(NegotiationComplete(signing_session)) => {
+						let funding_outpoint = signing_session.unsigned_tx.funding_outpoint();
+						debug_assert!(matches!(msg_send, InteractiveTxMessageSend::TxComplete(_)));
+						Ok(HandleTxCompleteValue::NegotiationComplete(
+							Some(msg_send),
+							funding_outpoint,
+						))
 					},
 					StateMachine::SentChangeMsg(_) => {
 						Ok(HandleTxCompleteValue::SendTxMessage(msg_send))
@@ -2126,7 +2208,10 @@ impl InteractiveTxConstructor {
 					},
 				}
 			},
-			StateMachine::NegotiationComplete(_) => Ok(HandleTxCompleteValue::NegotiationComplete),
+			StateMachine::NegotiationComplete(NegotiationComplete(signing_session)) => {
+				let funding_outpoint = signing_session.unsigned_tx.funding_outpoint();
+				Ok(HandleTxCompleteValue::NegotiationComplete(None, funding_outpoint))
+			},
 			_ => {
 				debug_assert!(
 					false,
@@ -2367,9 +2452,9 @@ mod tests {
 			outputs_to_contribute: session.outputs_a,
 		}) {
 			Ok(r) => Some(r),
-			Err(abort_reason) => {
+			Err(e) => {
 				assert_eq!(
-					Some((abort_reason, ErrorCulprit::NodeA)),
+					Some((e.reason, ErrorCulprit::NodeA)),
 					session.expect_error,
 					"Test: {}",
 					session.description
@@ -2406,9 +2491,9 @@ mod tests {
 			outputs_to_contribute: session.outputs_b,
 		}) {
 			Ok(r) => Some(r),
-			Err(abort_reason) => {
+			Err(e) => {
 				assert_eq!(
-					Some((abort_reason, ErrorCulprit::NodeB)),
+					Some((e.reason, ErrorCulprit::NodeB)),
 					session.expect_error,
 					"Test: {}",
 					session.description
@@ -2431,11 +2516,9 @@ mod tests {
 							HandleTxCompleteValue::SendTxMessage(msg_send) => {
 								(Some(msg_send), false)
 							},
-							HandleTxCompleteValue::SendTxComplete(
-								msg_send,
-								negotiation_complete,
-							) => (Some(msg_send), negotiation_complete),
-							HandleTxCompleteValue::NegotiationComplete => (None, true),
+							HandleTxCompleteValue::NegotiationComplete(msg_send, _) => {
+								(msg_send, true)
+							},
 						})
 					},
 				}