From 9c4cb91f9f6aa20711f35cf9959d855e6ded4f89 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Tue, 23 Sep 2025 21:11:09 -0500 Subject: [PATCH 1/6] Re-order ConstructedTransaction::new checks An upcoming commit will include the contributed inputs and outputs in an error whenever ConstructedTransaction::new fails. In order to DRY up that logic, this commit updates the constructor to create the resulting object prior to performing any checks. This way a conversion method can be added that extracts the necessary input and output data. --- lightning/src/ln/interactivetxs.rs | 54 +++++++++++++----------------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 3c683fcc5ee..00318e00ebb 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -248,26 +248,13 @@ impl_writeable_tlv_based!(ConstructedTransaction, { impl ConstructedTransaction { fn new(context: NegotiationContext) -> Result { - if let Some(shared_funding_input) = &context.shared_funding_input { - if !context.inputs.iter().any(|(_, input)| { - input.txin().previous_output == shared_funding_input.input.previous_output - }) { - return Err(AbortReason::MissingFundingInput); - } - } - if !context - .outputs - .iter() - .any(|(_, output)| *output.tx_out() == context.shared_funding_output.tx_out) - { - return Err(AbortReason::MissingFundingOutput); - } - let satisfaction_weight = Weight::from_wu(context.inputs.iter().fold(0u64, |value, (_, input)| { value.saturating_add(input.satisfaction_weight().to_wu()) })); + let lock_time = context.tx_locktime; + let mut inputs: Vec<(TxIn, TxInMetadata)> = context.inputs.into_values().map(|input| input.into_txin_and_metadata()).collect(); let mut outputs: Vec<(TxOut, TxOutMetadata)> = @@ -275,35 +262,42 @@ impl ConstructedTransaction { inputs.sort_unstable_by_key(|(_, input)| input.serial_id); outputs.sort_unstable_by_key(|(_, output)| output.serial_id); + let (input, input_metadata): (Vec, Vec) = inputs.into_iter().unzip(); + let (output, output_metadata): (Vec, Vec) = + outputs.into_iter().unzip(); + let shared_input_index = context.shared_funding_input.as_ref().and_then(|shared_funding_input| { - inputs + input .iter() - .position(|(txin, _)| { + .position(|txin| { txin.previous_output == shared_funding_input.input.previous_output }) .map(|position| position as u32) }); - let (input, input_metadata): (Vec, Vec) = inputs.into_iter().unzip(); - let (output, output_metadata): (Vec, Vec) = - outputs.into_iter().unzip(); + let tx = ConstructedTransaction { + holder_is_initiator: context.holder_is_initiator, + input_metadata, + output_metadata, + tx: Transaction { version: Version::TWO, lock_time, input, output }, + shared_input_index, + }; + + if context.shared_funding_input.is_some() && tx.shared_input_index.is_none() { + return Err(AbortReason::MissingFundingInput); + } - let tx = - Transaction { version: Version::TWO, lock_time: context.tx_locktime, input, output }; + if !tx.tx.output.iter().any(|txout| *txout == context.shared_funding_output.tx_out) { + return Err(AbortReason::MissingFundingOutput); + } - let tx_weight = tx.weight().checked_add(satisfaction_weight).unwrap_or(Weight::MAX); + let tx_weight = tx.tx.weight().checked_add(satisfaction_weight).unwrap_or(Weight::MAX); if tx_weight > Weight::from_wu(MAX_STANDARD_TX_WEIGHT as u64) { return Err(AbortReason::TransactionTooLarge); } - Ok(Self { - holder_is_initiator: context.holder_is_initiator, - input_metadata, - output_metadata, - tx, - shared_input_index, - }) + Ok(tx) } pub fn tx(&self) -> &Transaction { From 89e1f3371a6f8fb14ec58969d6d148d38e4534a4 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Tue, 23 Sep 2025 22:10:02 -0500 Subject: [PATCH 2/6] Move NegotiationContext checks into ConstructedTransaction Both NegotiationContext::validate_tx and ConstructedTransaction::new contain validity checks. Move the former into the latter in order to consolidate the checks in a single place. This will also allow for reusing error construction in an upcoming commit. --- lightning/src/ln/interactivetxs.rs | 87 ++++++++++++++---------------- 1 file changed, 40 insertions(+), 47 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 00318e00ebb..44511f5b2ae 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -248,6 +248,10 @@ impl_writeable_tlv_based!(ConstructedTransaction, { impl ConstructedTransaction { fn new(context: NegotiationContext) -> Result { + let remote_inputs_value = context.remote_inputs_value(); + let remote_outputs_value = context.remote_outputs_value(); + let remote_weight_contributed = context.remote_weight_contributed(); + let satisfaction_weight = Weight::from_wu(context.inputs.iter().fold(0u64, |value, (_, input)| { value.saturating_add(input.satisfaction_weight().to_wu()) @@ -284,6 +288,29 @@ impl ConstructedTransaction { shared_input_index, }; + // The receiving node: + // MUST fail the negotiation if: + // - the peer's total input satoshis is less than their outputs + if remote_inputs_value < remote_outputs_value { + return Err(AbortReason::OutputsValueExceedsInputsValue); + } + + // - the peer's paid feerate does not meet or exceed the agreed feerate (based on the minimum fee). + let remote_fees_contributed = remote_inputs_value.saturating_sub(remote_outputs_value); + let required_remote_contribution_fee = + fee_for_weight(context.feerate_sat_per_kw, remote_weight_contributed); + if remote_fees_contributed < required_remote_contribution_fee { + return Err(AbortReason::InsufficientFees); + } + + // - there are more than 252 inputs + // - there are more than 252 outputs + if tx.tx.input.len() > MAX_INPUTS_OUTPUTS_COUNT + || tx.tx.output.len() > MAX_INPUTS_OUTPUTS_COUNT + { + return Err(AbortReason::ExceededNumberOfInputsOrOutputs); + } + if context.shared_funding_input.is_some() && tx.shared_input_index.is_none() { return Err(AbortReason::MissingFundingInput); } @@ -886,6 +913,18 @@ impl NegotiationContext { ) } + fn remote_weight_contributed(&self) -> u64 { + self.remote_inputs_weight() + .to_wu() + .saturating_add(self.remote_outputs_weight().to_wu()) + // The receiving node: + // - MUST fail the negotiation if + // - if is the non-initiator: + // - the initiator's fees do not cover the common fields (version, segwit marker + flag, + // input count, output count, locktime) + .saturating_add(if !self.holder_is_initiator { TX_COMMON_FIELDS_WEIGHT } else { 0 }) + } + fn remote_outputs_weight(&self) -> Weight { Weight::from_wu( self.outputs @@ -1188,52 +1227,6 @@ impl NegotiationContext { self.outputs.remove(&msg.serial_id); Ok(()) } - - fn check_counterparty_fees( - &self, counterparty_fees_contributed: u64, - ) -> Result<(), AbortReason> { - let mut counterparty_weight_contributed = self - .remote_inputs_weight() - .to_wu() - .saturating_add(self.remote_outputs_weight().to_wu()); - if !self.holder_is_initiator { - // if is the non-initiator: - // - the initiator's fees do not cover the common fields (version, segwit marker + flag, - // input count, output count, locktime) - counterparty_weight_contributed += TX_COMMON_FIELDS_WEIGHT; - } - let required_counterparty_contribution_fee = - fee_for_weight(self.feerate_sat_per_kw, counterparty_weight_contributed); - if counterparty_fees_contributed < required_counterparty_contribution_fee { - return Err(AbortReason::InsufficientFees); - } - Ok(()) - } - - fn validate_tx(self) -> Result { - // The receiving node: - // MUST fail the negotiation if: - - // - the peer's total input satoshis is less than their outputs - let remote_inputs_value = self.remote_inputs_value(); - let remote_outputs_value = self.remote_outputs_value(); - if remote_inputs_value < remote_outputs_value { - return Err(AbortReason::OutputsValueExceedsInputsValue); - } - - // - there are more than 252 inputs - // - there are more than 252 outputs - if self.inputs.len() > MAX_INPUTS_OUTPUTS_COUNT - || self.outputs.len() > MAX_INPUTS_OUTPUTS_COUNT - { - return Err(AbortReason::ExceededNumberOfInputsOrOutputs); - } - - // - the peer's paid feerate does not meet or exceed the agreed feerate (based on the minimum fee). - self.check_counterparty_fees(remote_inputs_value.saturating_sub(remote_outputs_value))?; - - ConstructedTransaction::new(self) - } } // The interactive transaction construction protocol allows two peers to collaboratively build a @@ -1372,7 +1365,7 @@ macro_rules! define_state_transitions { let holder_node_id = context.holder_node_id; let counterparty_node_id = context.counterparty_node_id; - let tx = context.validate_tx()?; + let tx = ConstructedTransaction::new(context)?; // Strict ordering prevents deadlocks during tx_signatures exchange let local_contributed_input_value = tx.local_contributed_input_value(); From 8ca05a8602882f869913b3e48c7bf41650365b7f Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 24 Sep 2025 18:25:10 -0500 Subject: [PATCH 3/6] Keep InteractiveTxConstructor contributed inputs and outputs Instead of popping each input and output to contribute during an interactive tx session, clone the necessary parts and keep around the original inputs and outputs. This will let us reuse them later when constructing an error. The tradeoff is using additional memory to avoid more code complexity required to extract the sent input and outputs from NegotiationContext. --- lightning/src/ln/interactivetxs.rs | 51 ++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 44511f5b2ae..7063edd8948 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -1827,6 +1827,8 @@ pub(super) struct InteractiveTxConstructor { channel_id: ChannelId, inputs_to_contribute: Vec<(SerialId, InputOwned)>, outputs_to_contribute: Vec<(SerialId, OutputOwned)>, + next_input_index: Option, + next_output_index: Option, } #[allow(clippy::enum_variant_names)] // Clippy doesn't like the repeated `Tx` prefix here @@ -1979,12 +1981,17 @@ impl InteractiveTxConstructor { // In the same manner and for the same rationale as the inputs above, we'll shuffle the outputs. outputs_to_contribute.sort_unstable_by_key(|(serial_id, _)| *serial_id); + let next_input_index = (!inputs_to_contribute.is_empty()).then_some(0); + let next_output_index = (!outputs_to_contribute.is_empty()).then_some(0); + let mut constructor = Self { state_machine, initiator_first_message: None, channel_id, inputs_to_contribute, outputs_to_contribute, + next_input_index, + next_output_index, }; // We'll store the first message for the initiator. if is_initiator { @@ -1998,22 +2005,24 @@ impl InteractiveTxConstructor { } fn maybe_send_message(&mut self) -> Result { + let channel_id = self.channel_id; + // We first attempt to send inputs we want to add, then outputs. Once we are done sending // them both, then we always send tx_complete. - if let Some((serial_id, input)) = self.inputs_to_contribute.pop() { + if let Some((serial_id, input)) = self.next_input_to_contribute() { let satisfaction_weight = input.satisfaction_weight(); let msg = match input { InputOwned::Single(single) => msgs::TxAddInput { - channel_id: self.channel_id, - serial_id, - prevtx: Some(single.prev_tx), + channel_id, + serial_id: *serial_id, + prevtx: Some(single.prev_tx.clone()), prevtx_out: single.input.previous_output.vout, sequence: single.input.sequence.to_consensus_u32(), shared_input_txid: None, }, InputOwned::Shared(shared) => msgs::TxAddInput { - channel_id: self.channel_id, - serial_id, + channel_id, + serial_id: *serial_id, prevtx: None, prevtx_out: shared.input.previous_output.vout, sequence: shared.input.sequence.to_consensus_u32(), @@ -2022,22 +2031,44 @@ impl InteractiveTxConstructor { }; do_state_transition!(self, sent_tx_add_input, (&msg, satisfaction_weight))?; Ok(InteractiveTxMessageSend::TxAddInput(msg)) - } else if let Some((serial_id, output)) = self.outputs_to_contribute.pop() { + } else if let Some((serial_id, output)) = self.next_output_to_contribute() { let msg = msgs::TxAddOutput { - channel_id: self.channel_id, - serial_id, + channel_id, + serial_id: *serial_id, sats: output.tx_out().value.to_sat(), script: output.tx_out().script_pubkey.clone(), }; do_state_transition!(self, sent_tx_add_output, &msg)?; Ok(InteractiveTxMessageSend::TxAddOutput(msg)) } else { - let msg = msgs::TxComplete { channel_id: self.channel_id }; + let msg = msgs::TxComplete { channel_id }; do_state_transition!(self, sent_tx_complete, &msg)?; Ok(InteractiveTxMessageSend::TxComplete(msg)) } } + fn next_input_to_contribute(&mut self) -> Option<&(SerialId, InputOwned)> { + match self.next_input_index { + Some(index) => { + self.next_input_index = + index.checked_add(1).filter(|index| *index < self.inputs_to_contribute.len()); + self.inputs_to_contribute.get(index) + }, + None => None, + } + } + + fn next_output_to_contribute(&mut self) -> Option<&(SerialId, OutputOwned)> { + match self.next_output_index { + Some(index) => { + self.next_output_index = + index.checked_add(1).filter(|index| *index < self.outputs_to_contribute.len()); + self.outputs_to_contribute.get(index) + }, + None => None, + } + } + pub fn handle_tx_add_input( &mut self, msg: &msgs::TxAddInput, ) -> Result { From dc672f12ffb98870bbb1a0e5bb6a2557be88c7c7 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 24 Sep 2025 19:08:40 -0500 Subject: [PATCH 4/6] Store shared output index in ConstructedTransaction Currently, only the shared input index is stored in ConstructedTransaction. This will be used later to filter out the shared input when constructing an error during interactive tx negotiation. Store the shared output index as well so that the shared output can be filtered out as well. --- lightning/src/ln/interactivetxs.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 7063edd8948..ebb6a897c25 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -200,6 +200,7 @@ pub(crate) struct ConstructedTransaction { output_metadata: Vec, tx: Transaction, shared_input_index: Option, + shared_output_index: u16, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -244,6 +245,7 @@ impl_writeable_tlv_based!(ConstructedTransaction, { (5, output_metadata, required), (7, tx, required), (9, shared_input_index, option), + (11, shared_output_index, required), }); impl ConstructedTransaction { @@ -280,12 +282,19 @@ impl ConstructedTransaction { .map(|position| position as u32) }); + let shared_output_index = output + .iter() + .position(|txout| *txout == context.shared_funding_output.tx_out) + .map(|position| position as u16) + .unwrap_or(u16::MAX); + let tx = ConstructedTransaction { holder_is_initiator: context.holder_is_initiator, input_metadata, output_metadata, tx: Transaction { version: Version::TWO, lock_time, input, output }, shared_input_index, + shared_output_index, }; // The receiving node: @@ -315,7 +324,7 @@ impl ConstructedTransaction { return Err(AbortReason::MissingFundingInput); } - if !tx.tx.output.iter().any(|txout| *txout == context.shared_funding_output.tx_out) { + if tx.shared_output_index == u16::MAX { return Err(AbortReason::MissingFundingOutput); } @@ -3329,6 +3338,7 @@ mod tests { output_metadata: vec![], // N/A for test tx: transaction.clone(), shared_input_index: None, + shared_output_index: 0, }; let secp_ctx = Secp256k1::new(); From 0ecbe629eaf483b3407977e24887caee8b4df478 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 29 Sep 2025 16:43:05 +0200 Subject: [PATCH 5/6] Use u16 for ConstructedTransaction::shared_input_index The number of inputs allowed during an interactive-tx construction session is limited to 4096, so a u16 can be used instead of u32 when serializing ConstructedTransaction. --- lightning/src/ln/interactivetxs.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index ebb6a897c25..ea9994d89ab 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -199,7 +199,7 @@ pub(crate) struct ConstructedTransaction { input_metadata: Vec, output_metadata: Vec, tx: Transaction, - shared_input_index: Option, + shared_input_index: Option, shared_output_index: u16, } @@ -279,7 +279,7 @@ impl ConstructedTransaction { .position(|txin| { txin.previous_output == shared_funding_input.input.previous_output }) - .map(|position| position as u32) + .map(|position| position as u16) }); let shared_output_index = output @@ -455,7 +455,7 @@ impl ConstructedTransaction { self.holder_is_initiator } - pub fn shared_input_index(&self) -> Option { + pub fn shared_input_index(&self) -> Option { self.shared_input_index } } From 0956efdb485e7deea2783031ff76437ba99c4e86 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 24 Sep 2025 17:25:53 -0500 Subject: [PATCH 6/6] Consume InteractiveTxConstructor after error checks InteractiveTxConstructor contains the users contributed inputs. When an interactive tx sessions is aborted, the user will need to be notified with an event indicating which inputs and outputs were contributed. This allows them to re-use inputs that are no longer in use. This commit ensures the InteractiveTxConstructor is only consumed after all error checking. That way, in the case of a failure, we're able to produce an event from its input data. --- lightning/src/ln/channel.rs | 178 ++++++++++++++++------------- lightning/src/ln/interactivetxs.rs | 129 +++++++++++++++++---- 2 files changed, 207 insertions(+), 100 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 1ca067f43f4..585e1ac134e 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( - &mut self, logger: &L, + &mut self, funding_outpoint: OutPoint, logger: &L, ) -> Result 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( - &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 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( - self, context: &ChannelContext, funding: &FundingScope, signer_provider: &SP, + mut self, context: &ChannelContext, funding: &FundingScope, signer_provider: &SP, entropy_source: &ES, holder_node_id: PublicKey, - ) -> Result + ) -> Result 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 ea9994d89ab..d1cac891c11 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, + pub contributed_outputs: Vec, +} + #[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, + prevtx_outpoints: HashSet, /// The outputs added so far. outputs: HashMap, /// 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, 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(args: InteractiveTxConstructorArgs) -> Result + pub fn new(args: InteractiveTxConstructorArgs) -> Result 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 { 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) + }, }) }, }