From 91845e48095eac2e876001cd12a2a2cf146e4dac Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 18 Sep 2025 21:31:44 -0500 Subject: [PATCH 01/10] Use FundingTxInput in InteractiveTxConstructorArgs InteractiveTxConstructor::new converts inputs given in channel.rs to InputOwned. Instead of converting FundingTxInput in channel.rs to the type needed by that constructor, pass in FundingTxInput and have it converted directly to InputOwned there. This saves one conversion and Vec allocation. It also lets us use the satisfaction_weight in an upcoming refactor. --- lightning/src/ln/channel.rs | 18 +---- lightning/src/ln/funding.rs | 5 ++ lightning/src/ln/interactivetxs.rs | 106 +++++++++++------------------ 3 files changed, 45 insertions(+), 84 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index de894de8088..b967c023c00 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -6617,14 +6617,6 @@ impl FundingNegotiationContext { } } - let funding_inputs = self - .our_funding_inputs - .into_iter() - .map(|FundingTxInput { utxo, sequence, prevtx }| { - (TxIn { previous_output: utxo.outpoint, sequence, ..Default::default() }, prevtx) - }) - .collect(); - let constructor_args = InteractiveTxConstructorArgs { entropy_source, holder_node_id, @@ -6633,7 +6625,7 @@ impl FundingNegotiationContext { feerate_sat_per_kw: self.funding_feerate_sat_per_1000_weight, is_initiator: self.is_initiator, funding_tx_locktime: self.funding_tx_locktime, - inputs_to_contribute: funding_inputs, + inputs_to_contribute: self.our_funding_inputs, shared_funding_input: self.shared_funding_input, shared_funding_output: SharedOwnedOutput::new( shared_funding_output, @@ -13669,12 +13661,6 @@ where value: Amount::from_sat(funding.get_value_satoshis()), script_pubkey: funding.get_funding_redeemscript().to_p2wsh(), }; - let inputs_to_contribute = our_funding_inputs - .into_iter() - .map(|FundingTxInput { utxo, sequence, prevtx }| { - (TxIn { previous_output: utxo.outpoint, sequence, ..Default::default() }, prevtx) - }) - .collect(); let interactive_tx_constructor = Some(InteractiveTxConstructor::new( InteractiveTxConstructorArgs { @@ -13685,7 +13671,7 @@ where feerate_sat_per_kw: funding_negotiation_context.funding_feerate_sat_per_1000_weight, funding_tx_locktime: funding_negotiation_context.funding_tx_locktime, is_initiator: false, - inputs_to_contribute, + inputs_to_contribute: our_funding_inputs, shared_funding_input: None, shared_funding_output: SharedOwnedOutput::new(shared_funding_output, our_funding_contribution_sats), outputs_to_contribute: funding_negotiation_context.our_funding_outputs.clone(), diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs index b0b8cd4aa1c..df9b111f40e 100644 --- a/lightning/src/ln/funding.rs +++ b/lightning/src/ln/funding.rs @@ -198,6 +198,11 @@ impl FundingTxInput { FundingTxInput::new(prevtx, vout, witness_weight, Script::is_p2tr) } + #[cfg(test)] + pub(crate) fn new_p2pkh(prevtx: Transaction, vout: u32) -> Result { + FundingTxInput::new(prevtx, vout, Weight::ZERO, Script::is_p2pkh) + } + /// The sequence number to use in the [`TxIn`]. /// /// [`TxIn`]: bitcoin::TxIn diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 3d65996df88..bb8b9df10de 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -1920,7 +1920,7 @@ where pub feerate_sat_per_kw: u32, pub is_initiator: bool, pub funding_tx_locktime: AbsoluteLockTime, - pub inputs_to_contribute: Vec<(TxIn, Transaction)>, + pub inputs_to_contribute: Vec, pub shared_funding_input: Option, pub shared_funding_output: SharedOwnedOutput, pub outputs_to_contribute: Vec, @@ -1959,21 +1959,14 @@ impl InteractiveTxConstructor { shared_funding_output.clone(), ); - // Check for the existence of prevouts' - for (txin, tx) in inputs_to_contribute.iter() { - let vout = txin.previous_output.vout as usize; - if tx.output.get(vout).is_none() { - return Err(AbortReason::PrevTxOutInvalid); - } - } let mut inputs_to_contribute: Vec<(SerialId, InputOwned)> = inputs_to_contribute .into_iter() - .map(|(txin, tx)| { + .map(|FundingTxInput { utxo, sequence, prevtx: prev_tx }| { let serial_id = generate_holder_serial_id(entropy_source, is_initiator); - let vout = txin.previous_output.vout as usize; - let prev_output = tx.output.get(vout).unwrap().clone(); // checked above + let txin = TxIn { previous_output: utxo.outpoint, sequence, ..Default::default() }; + let prev_output = utxo.output; let input = - InputOwned::Single(SingleOwnedInput { input: txin, prev_tx: tx, prev_output }); + InputOwned::Single(SingleOwnedInput { input: txin, prev_tx, prev_output }); (serial_id, input) }) .collect(); @@ -2283,12 +2276,12 @@ mod tests { struct TestSession { description: &'static str, - inputs_a: Vec<(TxIn, Transaction)>, + inputs_a: Vec, a_shared_input: Option<(OutPoint, TxOut, u64)>, /// The funding output, with the value contributed shared_output_a: (TxOut, u64), outputs_a: Vec, - inputs_b: Vec<(TxIn, Transaction)>, + inputs_b: Vec, b_shared_input: Option<(OutPoint, TxOut, u64)>, /// The funding output, with the value contributed shared_output_b: (TxOut, u64), @@ -2558,20 +2551,22 @@ mod tests { } } - fn generate_inputs(outputs: &[TestOutput]) -> Vec<(TxIn, Transaction)> { + fn generate_inputs(outputs: &[TestOutput]) -> Vec { let tx = generate_tx(outputs); - let txid = tx.compute_txid(); - tx.output + outputs .iter() .enumerate() - .map(|(idx, _)| { - let txin = TxIn { - previous_output: OutPoint { txid, vout: idx as u32 }, - script_sig: Default::default(), - sequence: Sequence::ENABLE_RBF_NO_LOCKTIME, - witness: Default::default(), - }; - (txin, tx.clone()) + .map(|(idx, output)| match output { + TestOutput::P2WPKH(_) => { + FundingTxInput::new_p2wpkh(tx.clone(), idx as u32).unwrap() + }, + TestOutput::P2WSH(_) => { + FundingTxInput::new_p2wsh(tx.clone(), idx as u32, Weight::from_wu(42)).unwrap() + }, + TestOutput::P2TR(_) => { + FundingTxInput::new_p2tr_key_spend(tx.clone(), idx as u32).unwrap() + }, + TestOutput::P2PKH(_) => FundingTxInput::new_p2pkh(tx.clone(), idx as u32).unwrap(), }) .collect() } @@ -2619,37 +2614,26 @@ mod tests { (generate_txout(&TestOutput::P2WSH(value)), local_value) } - fn generate_fixed_number_of_inputs(count: u16) -> Vec<(TxIn, Transaction)> { + fn generate_fixed_number_of_inputs(count: u16) -> Vec { // Generate transactions with a total `count` number of outputs such that no transaction has a // serialized length greater than u16::MAX. let max_outputs_per_prevtx = 1_500; let mut remaining = count; - let mut inputs: Vec<(TxIn, Transaction)> = Vec::with_capacity(count as usize); + let mut inputs: Vec = Vec::with_capacity(count as usize); while remaining > 0 { let tx_output_count = remaining.min(max_outputs_per_prevtx); remaining -= tx_output_count; + let outputs = vec![TestOutput::P2WPKH(1_000_000); tx_output_count as usize]; + // Use unique locktime for each tx so outpoints are different across transactions - let tx = generate_tx_with_locktime( - &vec![TestOutput::P2WPKH(1_000_000); tx_output_count as usize], - (1337 + remaining).into(), - ); - let txid = tx.compute_txid(); + let tx = generate_tx_with_locktime(&outputs, (1337 + remaining).into()); - let mut temp: Vec<(TxIn, Transaction)> = tx - .output + let mut temp: Vec = outputs .iter() .enumerate() - .map(|(idx, _)| { - let input = TxIn { - previous_output: OutPoint { txid, vout: idx as u32 }, - script_sig: Default::default(), - sequence: Sequence::ENABLE_RBF_NO_LOCKTIME, - witness: Default::default(), - }; - (input, tx.clone()) - }) + .map(|(idx, _)| FundingTxInput::new_p2wpkh(tx.clone(), idx as u32).unwrap()) .collect(); inputs.append(&mut temp); @@ -2860,13 +2844,11 @@ mod tests { }); let tx = generate_tx(&[TestOutput::P2WPKH(1_000_000)]); - let invalid_sequence_input = TxIn { - previous_output: OutPoint { txid: tx.compute_txid(), vout: 0 }, - ..Default::default() - }; + let mut invalid_sequence_input = FundingTxInput::new_p2wpkh(tx.clone(), 0).unwrap(); + invalid_sequence_input.set_sequence(Default::default()); do_test_interactive_tx_constructor(TestSession { description: "Invalid input sequence from initiator", - inputs_a: vec![(invalid_sequence_input, tx.clone())], + inputs_a: vec![invalid_sequence_input], a_shared_input: None, shared_output_a: generate_funding_txout(1_000_000, 1_000_000), outputs_a: vec![], @@ -2876,14 +2858,10 @@ mod tests { outputs_b: vec![], expect_error: Some((AbortReason::IncorrectInputSequenceValue, ErrorCulprit::NodeA)), }); - let duplicate_input = TxIn { - previous_output: OutPoint { txid: tx.compute_txid(), vout: 0 }, - sequence: Sequence::ENABLE_RBF_NO_LOCKTIME, - ..Default::default() - }; + let duplicate_input = FundingTxInput::new_p2wpkh(tx.clone(), 0).unwrap(); do_test_interactive_tx_constructor(TestSession { description: "Duplicate prevout from initiator", - inputs_a: vec![(duplicate_input.clone(), tx.clone()), (duplicate_input, tx.clone())], + inputs_a: vec![duplicate_input.clone(), duplicate_input], a_shared_input: None, shared_output_a: generate_funding_txout(1_000_000, 1_000_000), outputs_a: vec![], @@ -2894,35 +2872,27 @@ mod tests { expect_error: Some((AbortReason::PrevTxOutInvalid, ErrorCulprit::NodeB)), }); // Non-initiator uses same prevout as initiator. - let duplicate_input = TxIn { - previous_output: OutPoint { txid: tx.compute_txid(), vout: 0 }, - sequence: Sequence::ENABLE_RBF_NO_LOCKTIME, - ..Default::default() - }; + let duplicate_input = FundingTxInput::new_p2wpkh(tx.clone(), 0).unwrap(); do_test_interactive_tx_constructor(TestSession { description: "Non-initiator uses same prevout as initiator", - inputs_a: vec![(duplicate_input.clone(), tx.clone())], + inputs_a: vec![duplicate_input.clone()], a_shared_input: None, shared_output_a: generate_funding_txout(1_000_000, 905_000), outputs_a: vec![], - inputs_b: vec![(duplicate_input.clone(), tx.clone())], + inputs_b: vec![duplicate_input], b_shared_input: None, shared_output_b: generate_funding_txout(1_000_000, 95_000), outputs_b: vec![], expect_error: Some((AbortReason::PrevTxOutInvalid, ErrorCulprit::NodeA)), }); - let duplicate_input = TxIn { - previous_output: OutPoint { txid: tx.compute_txid(), vout: 0 }, - sequence: Sequence::ENABLE_RBF_NO_LOCKTIME, - ..Default::default() - }; + let duplicate_input = FundingTxInput::new_p2wpkh(tx.clone(), 0).unwrap(); do_test_interactive_tx_constructor(TestSession { description: "Non-initiator uses same prevout as initiator", - inputs_a: vec![(duplicate_input.clone(), tx.clone())], + inputs_a: vec![duplicate_input.clone()], a_shared_input: None, shared_output_a: generate_funding_txout(1_000_000, 1_000_000), outputs_a: vec![], - inputs_b: vec![(duplicate_input.clone(), tx.clone())], + inputs_b: vec![duplicate_input], b_shared_input: None, shared_output_b: generate_funding_txout(1_000_000, 0), outputs_b: vec![], From 1e510cc05074a4c445811cdab7bee8d21b0e1c9a Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 19 Sep 2025 09:48:31 -0500 Subject: [PATCH 02/10] Track satisfaction_weight in InteractiveTxInput Instead of estimating the input weight, track the satisfaction_weight in InteractiveTxInput. This then can be used compute the transaction weight without storing individual weights in NegotiatedTxInput in an upcoming commit. --- lightning/src/ln/interactivetxs.rs | 71 ++++++++++++++++++------------ 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index bb8b9df10de..7336c0c70fa 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -842,16 +842,18 @@ struct NegotiationContext { feerate_sat_per_kw: u32, } -pub(crate) fn estimate_input_weight(prev_output: &TxOut) -> Weight { - Weight::from_wu(if prev_output.script_pubkey.is_p2wpkh() { - P2WPKH_INPUT_WEIGHT_LOWER_BOUND - } else if prev_output.script_pubkey.is_p2wsh() { - P2WSH_INPUT_WEIGHT_LOWER_BOUND - } else if prev_output.script_pubkey.is_p2tr() { - P2TR_INPUT_WEIGHT_LOWER_BOUND - } else { - UNKNOWN_SEGWIT_VERSION_INPUT_WEIGHT_LOWER_BOUND - }) +fn estimate_input_satisfaction_weight(prev_output: &TxOut) -> Weight { + Weight::from_wu( + if prev_output.script_pubkey.is_p2wpkh() { + P2WPKH_INPUT_WEIGHT_LOWER_BOUND + } else if prev_output.script_pubkey.is_p2wsh() { + P2WSH_INPUT_WEIGHT_LOWER_BOUND + } else if prev_output.script_pubkey.is_p2tr() { + P2TR_INPUT_WEIGHT_LOWER_BOUND + } else { + UNKNOWN_SEGWIT_VERSION_INPUT_WEIGHT_LOWER_BOUND + } - BASE_INPUT_WEIGHT, + ) } pub(crate) fn get_output_weight(script_pubkey: &ScriptBuf) -> Weight { @@ -906,7 +908,9 @@ impl NegotiationContext { .iter() .filter(|(serial_id, _)| self.is_serial_id_valid_for_counterparty(serial_id)) .fold(0u64, |weight, (_, input)| { - weight.saturating_add(input.estimate_input_weight().to_wu()) + weight + .saturating_add(BASE_INPUT_WEIGHT) + .saturating_add(input.satisfaction_weight().to_wu()) }), ) } @@ -998,6 +1002,7 @@ impl NegotiationContext { input: txin, prev_tx: prevtx.clone(), prev_output: tx_out.clone(), + satisfaction_weight: estimate_input_satisfaction_weight(&tx_out), }), prev_outpoint, ) @@ -1150,7 +1155,9 @@ impl NegotiationContext { } } - fn sent_tx_add_input(&mut self, msg: &msgs::TxAddInput) -> Result<(), AbortReason> { + fn sent_tx_add_input( + &mut self, (msg, satisfaction_weight): (&msgs::TxAddInput, Weight), + ) -> 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 }; @@ -1168,8 +1175,12 @@ impl NegotiationContext { sequence: Sequence(msg.sequence), ..Default::default() }; - let single_input = - SingleOwnedInput { input: txin, prev_tx: prevtx.clone(), prev_output }; + let single_input = SingleOwnedInput { + input: txin, + prev_tx: prevtx.clone(), + prev_output, + satisfaction_weight, + }; (prev_outpoint, InputOwned::Single(single_input)) } else { return Err(AbortReason::MissingPrevTx); @@ -1432,7 +1443,7 @@ define_state_transitions!(SENT_MSG_STATE, [ // State transitions when we have received some messages from our counterparty and we should // respond. define_state_transitions!(RECEIVED_MSG_STATE, [ - DATA &msgs::TxAddInput, TRANSITION sent_tx_add_input, + DATA (&msgs::TxAddInput, Weight), TRANSITION sent_tx_add_input, DATA &msgs::TxRemoveInput, TRANSITION sent_tx_remove_input, DATA &msgs::TxAddOutput, TRANSITION sent_tx_add_output, DATA &msgs::TxRemoveOutput, TRANSITION sent_tx_remove_output @@ -1499,7 +1510,7 @@ impl StateMachine { } // TxAddInput - define_state_machine_transitions!(sent_tx_add_input, &msgs::TxAddInput, [ + define_state_machine_transitions!(sent_tx_add_input, (&msgs::TxAddInput, Weight), [ FROM ReceivedChangeMsg, TO SentChangeMsg, FROM ReceivedTxComplete, TO SentChangeMsg ]); @@ -1566,6 +1577,7 @@ struct SingleOwnedInput { input: TxIn, prev_tx: Transaction, prev_output: TxOut, + satisfaction_weight: Weight, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -1658,13 +1670,13 @@ impl InputOwned { } } - fn estimate_input_weight(&self) -> Weight { + fn satisfaction_weight(&self) -> Weight { match self { - InputOwned::Single(single) => estimate_input_weight(&single.prev_output), + InputOwned::Single(single) => single.satisfaction_weight, // TODO(taproot): Needs to consider different weights based on channel type - InputOwned::Shared(_) => Weight::from_wu( - BASE_INPUT_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT + FUNDING_TRANSACTION_WITNESS_WEIGHT, - ), + InputOwned::Shared(_) => { + Weight::from_wu(EMPTY_SCRIPT_SIG_WEIGHT + FUNDING_TRANSACTION_WITNESS_WEIGHT) + }, } } @@ -1835,12 +1847,12 @@ impl InteractiveTxInput { self.input.remote_value(self.added_by) } - pub fn estimate_input_weight(&self) -> Weight { - self.input.estimate_input_weight() + pub fn satisfaction_weight(&self) -> Weight { + self.input.satisfaction_weight() } fn into_negotiated_input(self) -> NegotiatedTxInput { - let weight = self.input.estimate_input_weight(); + let weight = Weight::from_wu(BASE_INPUT_WEIGHT) + self.input.satisfaction_weight(); let (txin, prev_output) = self.input.into_tx_in_with_prev_output(); NegotiatedTxInput { serial_id: self.serial_id, txin, weight, prev_output } } @@ -1965,8 +1977,12 @@ impl InteractiveTxConstructor { let serial_id = generate_holder_serial_id(entropy_source, is_initiator); let txin = TxIn { previous_output: utxo.outpoint, sequence, ..Default::default() }; let prev_output = utxo.output; - let input = - InputOwned::Single(SingleOwnedInput { input: txin, prev_tx, prev_output }); + let input = InputOwned::Single(SingleOwnedInput { + input: txin, + prev_tx, + prev_output, + satisfaction_weight: Weight::from_wu(utxo.satisfaction_weight), + }); (serial_id, input) }) .collect(); @@ -2022,6 +2038,7 @@ impl InteractiveTxConstructor { // 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() { + let satisfaction_weight = input.satisfaction_weight(); let msg = match input { InputOwned::Single(single) => msgs::TxAddInput { channel_id: self.channel_id, @@ -2040,7 +2057,7 @@ impl InteractiveTxConstructor { shared_input_txid: Some(shared.input.previous_output.txid), }, }; - do_state_transition!(self, sent_tx_add_input, &msg)?; + 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() { let msg = msgs::TxAddOutput { From 0a1d445ebe6d5bdd08435d0ce4cd25c2e11738e4 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 19 Sep 2025 09:54:12 -0500 Subject: [PATCH 03/10] Remove NegotiatedTxInput::weight The weight is no longer needed once a ConstructedTransaction is created, so it doesn't need to be persisted. --- lightning/src/ln/interactivetxs.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 7336c0c70fa..b2a7ad8c6ce 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -215,8 +215,6 @@ pub(crate) struct ConstructedTransaction { pub(crate) struct NegotiatedTxInput { serial_id: SerialId, txin: TxIn, - // The weight of the input including an estimate of its witness weight. - weight: Weight, prev_output: TxOut, } @@ -233,8 +231,7 @@ impl NegotiatedTxInput { impl_writeable_tlv_based!(NegotiatedTxInput, { (1, serial_id, required), (3, txin, required), - (5, weight, required), - (7, prev_output, required), + (5, prev_output, required), }); impl_writeable_tlv_based!(ConstructedTransaction, { @@ -278,6 +275,12 @@ impl ConstructedTransaction { let remote_inputs_value_satoshis = context.remote_inputs_value(); let remote_outputs_value_satoshis = context.remote_outputs_value(); + + let satisfaction_weight = + Weight::from_wu(context.inputs.iter().fold(0u64, |value, (_, input)| { + value.saturating_add(input.satisfaction_weight().to_wu()) + })); + let mut inputs: Vec = context.inputs.into_values().map(|tx_input| tx_input.into_negotiated_input()).collect(); let mut outputs: Vec = context.outputs.into_values().collect(); @@ -310,17 +313,18 @@ impl ConstructedTransaction { shared_input_index, }; - if constructed_tx.weight().to_wu() > MAX_STANDARD_TX_WEIGHT as u64 { + let tx_weight = constructed_tx.weight(satisfaction_weight); + if tx_weight > Weight::from_wu(MAX_STANDARD_TX_WEIGHT as u64) { return Err(AbortReason::TransactionTooLarge); } Ok(constructed_tx) } - pub fn weight(&self) -> Weight { - let inputs_weight = self.inputs.iter().fold(Weight::from_wu(0), |weight, input| { - weight.checked_add(input.weight).unwrap_or(Weight::MAX) - }); + fn weight(&self, satisfaction_weight: Weight) -> Weight { + let inputs_weight = Weight::from_wu(self.inputs.len() as u64 * BASE_INPUT_WEIGHT) + .checked_add(satisfaction_weight) + .unwrap_or(Weight::MAX); let outputs_weight = self.outputs.iter().fold(Weight::from_wu(0), |weight, output| { weight.checked_add(get_output_weight(output.script_pubkey())).unwrap_or(Weight::MAX) }); @@ -1852,9 +1856,8 @@ impl InteractiveTxInput { } fn into_negotiated_input(self) -> NegotiatedTxInput { - let weight = Weight::from_wu(BASE_INPUT_WEIGHT) + self.input.satisfaction_weight(); let (txin, prev_output) = self.input.into_tx_in_with_prev_output(); - NegotiatedTxInput { serial_id: self.serial_id, txin, weight, prev_output } + NegotiatedTxInput { serial_id: self.serial_id, txin, prev_output } } } @@ -3328,7 +3331,6 @@ mod tests { NegotiatedTxInput { serial_id: idx as u64, // even values will be holder (initiator in this test) txin, - weight: Weight::from_wu(0), // N/A for test prev_output, } }) From e2623263ba47cd9c4f4e59a1fd2b60e2b80230cb Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 19 Sep 2025 12:56:26 -0500 Subject: [PATCH 04/10] Compute ConstructedTransaction weight from Transaction Instead of defining a custom function for calculating a transaction's weight, have ConstructedTransaction hold a Transaction so that its weight method can be re-used. As a result NegotiatedTxInput no longer needs to store the transaction inputs. --- lightning/src/ln/channel.rs | 4 +- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/interactivetxs.rs | 130 ++++++++++++----------------- 3 files changed, 56 insertions(+), 80 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b967c023c00..a488b502eda 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -8615,7 +8615,7 @@ where return Err(APIError::APIMisuseError { err }); }; - let tx = signing_session.unsigned_tx().build_unsigned_tx(); + let tx = signing_session.unsigned_tx().tx(); if funding_txid_signed != tx.compute_txid() { return Err(APIError::APIMisuseError { err: "Transaction was malleated prior to signing".to_owned(), @@ -8627,7 +8627,7 @@ where let sig = match &self.context.holder_signer { ChannelSignerType::Ecdsa(signer) => signer.sign_splice_shared_input( &self.funding.channel_transaction_parameters, - &tx, + tx, splice_input_index as usize, &self.context.secp_ctx, ), diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ba02faf04f2..c17d999af85 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -9137,7 +9137,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ { if signing_session.has_local_contribution() { let mut pending_events = self.pending_events.lock().unwrap(); - let unsigned_transaction = signing_session.unsigned_tx().build_unsigned_tx(); + let unsigned_transaction = signing_session.unsigned_tx().tx().clone(); let event_action = ( Event::FundingTransactionReadyForSigning { unsigned_transaction, diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index b2a7ad8c6ce..d3df003e93f 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -200,6 +200,7 @@ pub(crate) struct ConstructedTransaction { inputs: Vec, outputs: Vec, + tx: Transaction, local_inputs_value_satoshis: u64, local_outputs_value_satoshis: u64, @@ -207,14 +208,12 @@ pub(crate) struct ConstructedTransaction { remote_inputs_value_satoshis: u64, remote_outputs_value_satoshis: u64, - lock_time: AbsoluteLockTime, shared_input_index: Option, } #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) struct NegotiatedTxInput { serial_id: SerialId, - txin: TxIn, prev_output: TxOut, } @@ -230,19 +229,18 @@ impl NegotiatedTxInput { impl_writeable_tlv_based!(NegotiatedTxInput, { (1, serial_id, required), - (3, txin, required), - (5, prev_output, required), + (3, prev_output, required), }); impl_writeable_tlv_based!(ConstructedTransaction, { (1, holder_is_initiator, required), (3, inputs, required), (5, outputs, required), - (7, local_inputs_value_satoshis, required), - (9, local_outputs_value_satoshis, required), - (11, remote_inputs_value_satoshis, required), - (13, remote_outputs_value_satoshis, required), - (15, lock_time, required), + (7, tx, required), + (9, local_inputs_value_satoshis, required), + (11, local_outputs_value_satoshis, required), + (13, remote_inputs_value_satoshis, required), + (15, remote_outputs_value_satoshis, required), (17, shared_input_index, option), }); @@ -281,23 +279,37 @@ impl ConstructedTransaction { value.saturating_add(input.satisfaction_weight().to_wu()) })); - let mut inputs: Vec = - context.inputs.into_values().map(|tx_input| tx_input.into_negotiated_input()).collect(); + let mut inputs: Vec<(TxIn, NegotiatedTxInput)> = context + .inputs + .into_values() + .map(|input| input.into_txin_and_negotiated_input()) + .collect(); let mut outputs: Vec = context.outputs.into_values().collect(); - inputs.sort_unstable_by_key(|input| input.serial_id); + inputs.sort_unstable_by_key(|(_, input)| input.serial_id); outputs.sort_unstable_by_key(|output| output.serial_id); let shared_input_index = context.shared_funding_input.as_ref().and_then(|shared_funding_input| { inputs .iter() - .position(|input| { - input.txin.previous_output == shared_funding_input.input.previous_output + .position(|(txin, _)| { + txin.previous_output == shared_funding_input.input.previous_output }) .map(|position| position as u32) }); - let constructed_tx = Self { + let (input, inputs): (Vec, Vec) = inputs.into_iter().unzip(); + let output = outputs.iter().map(|output| output.tx_out().clone()).collect(); + + let tx = + Transaction { version: Version::TWO, lock_time: context.tx_locktime, input, output }; + + let tx_weight = 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, local_inputs_value_satoshis, @@ -308,39 +320,14 @@ impl ConstructedTransaction { inputs, outputs, + tx, - lock_time: context.tx_locktime, shared_input_index, - }; - - let tx_weight = constructed_tx.weight(satisfaction_weight); - if tx_weight > Weight::from_wu(MAX_STANDARD_TX_WEIGHT as u64) { - return Err(AbortReason::TransactionTooLarge); - } - - Ok(constructed_tx) + }) } - fn weight(&self, satisfaction_weight: Weight) -> Weight { - let inputs_weight = Weight::from_wu(self.inputs.len() as u64 * BASE_INPUT_WEIGHT) - .checked_add(satisfaction_weight) - .unwrap_or(Weight::MAX); - let outputs_weight = self.outputs.iter().fold(Weight::from_wu(0), |weight, output| { - weight.checked_add(get_output_weight(output.script_pubkey())).unwrap_or(Weight::MAX) - }); - Weight::from_wu(TX_COMMON_FIELDS_WEIGHT) - .checked_add(inputs_weight) - .and_then(|weight| weight.checked_add(outputs_weight)) - .unwrap_or(Weight::MAX) - } - - pub fn build_unsigned_tx(&self) -> Transaction { - let ConstructedTransaction { inputs, outputs, .. } = self; - - let input: Vec = inputs.iter().map(|input| input.txin.clone()).collect(); - let output: Vec = outputs.iter().map(|output| output.tx_out().clone()).collect(); - - Transaction { version: Version::TWO, lock_time: self.lock_time, input, output } + pub fn tx(&self) -> &Transaction { + &self.tx } pub fn outputs(&self) -> impl Iterator { @@ -352,23 +339,25 @@ impl ConstructedTransaction { } pub fn compute_txid(&self) -> Txid { - self.build_unsigned_tx().compute_txid() + self.tx().compute_txid() } /// Adds provided holder witnesses to holder inputs of unsigned transaction. /// /// Note that it is assumed that the witness count equals the holder input count. fn add_local_witnesses(&mut self, witnesses: Vec) { - self.inputs + self.tx + .input .iter_mut() + .zip(self.inputs.iter()) .enumerate() - .filter(|(_, input)| input.is_local(self.holder_is_initiator)) + .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(|(_, input)| &mut input.txin) + .map(|(_, (txin, _))| txin) .zip(witnesses) .for_each(|(input, witness)| input.witness = witness); } @@ -377,16 +366,18 @@ impl ConstructedTransaction { /// /// Note that it is assumed that the witness count equals the counterparty input count. fn add_remote_witnesses(&mut self, witnesses: Vec) { - self.inputs + self.tx + .input .iter_mut() + .zip(self.inputs.iter()) .enumerate() - .filter(|(_, input)| !input.is_local(self.holder_is_initiator)) + .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(|(_, input)| &mut input.txin) + .map(|(_, (txin, _))| txin) .zip(witnesses) .for_each(|(input, witness)| input.witness = witness); } @@ -594,18 +585,7 @@ impl InteractiveTxSigningSession { } fn finalize_funding_tx(&mut self) -> Transaction { - let lock_time = self.unsigned_tx.lock_time; - let ConstructedTransaction { inputs, outputs, shared_input_index, .. } = - &mut self.unsigned_tx; - - let mut tx = Transaction { - version: Version::TWO, - lock_time, - input: inputs.iter().cloned().map(|input| input.txin).collect(), - output: outputs.iter().cloned().map(|output| output.into_tx_out()).collect(), - }; - - if let Some(shared_input_index) = shared_input_index { + if let Some(shared_input_index) = self.unsigned_tx.shared_input_index { if let Some(holder_shared_input_sig) = self .holder_tx_signatures .as_ref() @@ -628,7 +608,7 @@ impl InteractiveTxSigningSession { witness.push_ecdsa_signature(&holder_sig); } witness.push(&shared_input_sig.witness_script); - tx.input[*shared_input_index as usize].witness = witness; + self.unsigned_tx.tx.input[shared_input_index as usize].witness = witness; } else { debug_assert!(false); } @@ -640,19 +620,19 @@ impl InteractiveTxSigningSession { } } - tx + self.unsigned_tx.tx.clone() } fn verify_interactive_tx_signatures( &self, secp_ctx: &Secp256k1, witnesses: &Vec, ) -> Result<(), String> { let unsigned_tx = self.unsigned_tx(); - let built_tx = unsigned_tx.build_unsigned_tx(); + let built_tx = unsigned_tx.tx(); let prev_outputs: Vec<&TxOut> = unsigned_tx.inputs().map(|input| input.prev_output()).collect::>(); let all_prevouts = sighash::Prevouts::All(&prev_outputs[..]); - let mut cache = SighashCache::new(&built_tx); + let mut cache = SighashCache::new(built_tx); let script_pubkeys = unsigned_tx .inputs() @@ -1855,9 +1835,9 @@ impl InteractiveTxInput { self.input.satisfaction_weight() } - fn into_negotiated_input(self) -> NegotiatedTxInput { + fn into_txin_and_negotiated_input(self) -> (TxIn, NegotiatedTxInput) { let (txin, prev_output) = self.input.into_tx_in_with_prev_output(); - NegotiatedTxInput { serial_id: self.serial_id, txin, prev_output } + (txin, NegotiatedTxInput { serial_id: self.serial_id, prev_output }) } } @@ -3321,16 +3301,12 @@ mod tests { fn do_verify_tx_signatures( transaction: Transaction, prev_outputs: Vec, ) -> Result<(), String> { - let inputs: Vec = transaction - .input - .iter() - .cloned() - .zip(prev_outputs.into_iter()) + let inputs: Vec = prev_outputs + .into_iter() .enumerate() - .map(|(idx, (txin, prev_output))| { + .map(|(idx, prev_output)| { NegotiatedTxInput { serial_id: idx as u64, // even values will be holder (initiator in this test) - txin, prev_output, } }) @@ -3351,11 +3327,11 @@ mod tests { holder_is_initiator: true, inputs, outputs, + tx: transaction.clone(), local_inputs_value_satoshis: 0, // N/A for test local_outputs_value_satoshis: 0, // N/A for test remote_inputs_value_satoshis: 0, // N/A for test remote_outputs_value_satoshis: 0, // N/A for test - lock_time: transaction.lock_time, shared_input_index: None, }; From d01df5acdb2b00a68c64dbbecd1c588ff68be11f Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 22 Sep 2025 15:54:34 -0500 Subject: [PATCH 05/10] Rename NegotiatedTxInput to TxInMetadata Now that NegotiatedTxInput no longer holds a TxIn, rename it accordingly. --- lightning/src/ln/interactivetxs.rs | 57 ++++++++++++++---------------- lightning/src/util/ser.rs | 4 +-- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index d3df003e93f..4e31d17c7c5 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -198,7 +198,7 @@ impl Display for AbortReason { pub(crate) struct ConstructedTransaction { holder_is_initiator: bool, - inputs: Vec, + input_metadata: Vec, outputs: Vec, tx: Transaction, @@ -212,12 +212,12 @@ pub(crate) struct ConstructedTransaction { } #[derive(Clone, Debug, Eq, PartialEq)] -pub(crate) struct NegotiatedTxInput { +pub(crate) struct TxInMetadata { serial_id: SerialId, prev_output: TxOut, } -impl NegotiatedTxInput { +impl TxInMetadata { pub(super) fn is_local(&self, holder_is_initiator: bool) -> bool { !is_serial_id_valid_for_counterparty(holder_is_initiator, self.serial_id) } @@ -227,14 +227,14 @@ impl NegotiatedTxInput { } } -impl_writeable_tlv_based!(NegotiatedTxInput, { +impl_writeable_tlv_based!(TxInMetadata, { (1, serial_id, required), (3, prev_output, required), }); impl_writeable_tlv_based!(ConstructedTransaction, { (1, holder_is_initiator, required), - (3, inputs, required), + (3, input_metadata, required), (5, outputs, required), (7, tx, required), (9, local_inputs_value_satoshis, required), @@ -279,11 +279,8 @@ impl ConstructedTransaction { value.saturating_add(input.satisfaction_weight().to_wu()) })); - let mut inputs: Vec<(TxIn, NegotiatedTxInput)> = context - .inputs - .into_values() - .map(|input| input.into_txin_and_negotiated_input()) - .collect(); + let mut inputs: Vec<(TxIn, TxInMetadata)> = + context.inputs.into_values().map(|input| input.into_txin_and_metadata()).collect(); let mut outputs: Vec = context.outputs.into_values().collect(); inputs.sort_unstable_by_key(|(_, input)| input.serial_id); outputs.sort_unstable_by_key(|output| output.serial_id); @@ -298,7 +295,7 @@ impl ConstructedTransaction { .map(|position| position as u32) }); - let (input, inputs): (Vec, Vec) = inputs.into_iter().unzip(); + let (input, input_metadata): (Vec, Vec) = inputs.into_iter().unzip(); let output = outputs.iter().map(|output| output.tx_out().clone()).collect(); let tx = @@ -318,7 +315,7 @@ impl ConstructedTransaction { remote_inputs_value_satoshis, remote_outputs_value_satoshis, - inputs, + input_metadata, outputs, tx, @@ -334,8 +331,8 @@ impl ConstructedTransaction { self.outputs.iter() } - pub fn inputs(&self) -> impl Iterator { - self.inputs.iter() + pub fn input_metadata(&self) -> impl Iterator { + self.input_metadata.iter() } pub fn compute_txid(&self) -> Txid { @@ -349,7 +346,7 @@ impl ConstructedTransaction { self.tx .input .iter_mut() - .zip(self.inputs.iter()) + .zip(self.input_metadata.iter()) .enumerate() .filter(|(_, (_, input))| input.is_local(self.holder_is_initiator)) .filter(|(index, _)| { @@ -369,7 +366,7 @@ impl ConstructedTransaction { self.tx .input .iter_mut() - .zip(self.inputs.iter()) + .zip(self.input_metadata.iter()) .enumerate() .filter(|(_, (_, input))| !input.is_local(self.holder_is_initiator)) .filter(|(index, _)| { @@ -535,7 +532,7 @@ impl InteractiveTxSigningSession { pub fn remote_inputs_count(&self) -> usize { let shared_index = self.unsigned_tx.shared_input_index.as_ref(); self.unsigned_tx - .inputs + .input_metadata .iter() .enumerate() .filter(|(_, input)| !input.is_local(self.unsigned_tx.holder_is_initiator)) @@ -547,7 +544,7 @@ impl InteractiveTxSigningSession { pub fn local_inputs_count(&self) -> usize { self.unsigned_tx - .inputs + .input_metadata .iter() .enumerate() .filter(|(_, input)| input.is_local(self.unsigned_tx.holder_is_initiator)) @@ -578,10 +575,10 @@ impl InteractiveTxSigningSession { self.local_inputs_count() > 0 || self.local_outputs_count() > 0 } - pub fn shared_input(&self) -> Option<&NegotiatedTxInput> { - self.unsigned_tx - .shared_input_index - .and_then(|shared_input_index| self.unsigned_tx.inputs.get(shared_input_index as usize)) + pub fn shared_input(&self) -> Option<&TxInMetadata> { + self.unsigned_tx.shared_input_index.and_then(|shared_input_index| { + self.unsigned_tx.input_metadata.get(shared_input_index as usize) + }) } fn finalize_funding_tx(&mut self) -> Transaction { @@ -629,13 +626,13 @@ impl InteractiveTxSigningSession { let unsigned_tx = self.unsigned_tx(); let built_tx = unsigned_tx.tx(); let prev_outputs: Vec<&TxOut> = - unsigned_tx.inputs().map(|input| input.prev_output()).collect::>(); + unsigned_tx.input_metadata().map(|input| input.prev_output()).collect::>(); let all_prevouts = sighash::Prevouts::All(&prev_outputs[..]); let mut cache = SighashCache::new(built_tx); let script_pubkeys = unsigned_tx - .inputs() + .input_metadata() .enumerate() .filter(|(_, input)| input.is_local(unsigned_tx.holder_is_initiator())) .filter(|(index, _)| { @@ -1835,9 +1832,9 @@ impl InteractiveTxInput { self.input.satisfaction_weight() } - fn into_txin_and_negotiated_input(self) -> (TxIn, NegotiatedTxInput) { + fn into_txin_and_metadata(self) -> (TxIn, TxInMetadata) { let (txin, prev_output) = self.input.into_tx_in_with_prev_output(); - (txin, NegotiatedTxInput { serial_id: self.serial_id, prev_output }) + (txin, TxInMetadata { serial_id: self.serial_id, prev_output }) } } @@ -2228,7 +2225,7 @@ mod tests { use super::{ get_output_weight, AddingRole, ConstructedTransaction, InteractiveTxOutput, - InteractiveTxSigningSession, NegotiatedTxInput, OutputOwned, P2TR_INPUT_WEIGHT_LOWER_BOUND, + InteractiveTxSigningSession, OutputOwned, TxInMetadata, P2TR_INPUT_WEIGHT_LOWER_BOUND, P2WPKH_INPUT_WEIGHT_LOWER_BOUND, P2WSH_INPUT_WEIGHT_LOWER_BOUND, TX_COMMON_FIELDS_WEIGHT, }; @@ -3301,11 +3298,11 @@ mod tests { fn do_verify_tx_signatures( transaction: Transaction, prev_outputs: Vec, ) -> Result<(), String> { - let inputs: Vec = prev_outputs + let input_metadata: Vec = prev_outputs .into_iter() .enumerate() .map(|(idx, prev_output)| { - NegotiatedTxInput { + TxInMetadata { serial_id: idx as u64, // even values will be holder (initiator in this test) prev_output, } @@ -3325,7 +3322,7 @@ mod tests { let unsigned_tx = ConstructedTransaction { holder_is_initiator: true, - inputs, + input_metadata, outputs, tx: transaction.clone(), local_inputs_value_satoshis: 0, // N/A for test diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index aa2d1058805..a5c35180372 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -15,7 +15,7 @@ use crate::io::{self, BufRead, Read, Write}; use crate::io_extras::{copy, sink}; -use crate::ln::interactivetxs::{InteractiveTxOutput, NegotiatedTxInput}; +use crate::ln::interactivetxs::{InteractiveTxOutput, TxInMetadata}; use crate::ln::onion_utils::{HMAC_COUNT, HMAC_LEN, HOLD_TIME_LEN, MAX_HOPS}; use crate::prelude::*; use crate::sync::{Mutex, RwLock}; @@ -1082,7 +1082,7 @@ impl_for_vec!(crate::ln::channelmanager::PaymentClaimDetails); impl_for_vec!(crate::ln::msgs::SocketAddress); impl_for_vec!((A, B), A, B); impl_for_vec!(SerialId); -impl_for_vec!(NegotiatedTxInput); +impl_for_vec!(TxInMetadata); impl_for_vec!(InteractiveTxOutput); impl_for_vec!(crate::ln::our_peer_storage::PeerStorageMonitorHolder); impl_for_vec!(crate::blinded_path::message::BlindedMessagePath); From 0cab3c3096347751ca65f0f3cf544a23040eed1e Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 19 Sep 2025 13:38:19 -0500 Subject: [PATCH 06/10] Remove duplicate TxOut persistence Now that ConstructedTransaction holds the actual Transaction, there's no need to keep track of and persist the outputs separately. However, the serial IDs are still needed to later reconstruct which outputs were contributed. --- lightning/src/ln/channel.rs | 4 +- lightning/src/ln/interactivetxs.rs | 72 ++++++++++++++---------------- lightning/src/util/ser.rs | 4 +- 3 files changed, 38 insertions(+), 42 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index a488b502eda..e5c7df6f3ee 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -6108,8 +6108,8 @@ where { let mut output_index = None; let expected_spk = funding.get_funding_redeemscript().to_p2wsh(); - for (idx, outp) in signing_session.unsigned_tx().outputs().enumerate() { - if outp.script_pubkey() == &expected_spk && outp.value() == funding.get_value_satoshis() { + 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); } diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 4e31d17c7c5..56800e1390b 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -199,7 +199,7 @@ pub(crate) struct ConstructedTransaction { holder_is_initiator: bool, input_metadata: Vec, - outputs: Vec, + output_metadata: Vec, tx: Transaction, local_inputs_value_satoshis: u64, @@ -217,6 +217,11 @@ pub(crate) struct TxInMetadata { prev_output: TxOut, } +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct TxOutMetadata { + serial_id: SerialId, +} + impl TxInMetadata { pub(super) fn is_local(&self, holder_is_initiator: bool) -> bool { !is_serial_id_valid_for_counterparty(holder_is_initiator, self.serial_id) @@ -227,15 +232,25 @@ impl TxInMetadata { } } +impl TxOutMetadata { + pub(super) fn is_local(&self, holder_is_initiator: bool) -> bool { + !is_serial_id_valid_for_counterparty(holder_is_initiator, self.serial_id) + } +} + impl_writeable_tlv_based!(TxInMetadata, { (1, serial_id, required), (3, prev_output, required), }); +impl_writeable_tlv_based!(TxOutMetadata, { + (1, serial_id, required), +}); + impl_writeable_tlv_based!(ConstructedTransaction, { (1, holder_is_initiator, required), (3, input_metadata, required), - (5, outputs, required), + (5, output_metadata, required), (7, tx, required), (9, local_inputs_value_satoshis, required), (11, local_outputs_value_satoshis, required), @@ -281,9 +296,10 @@ impl ConstructedTransaction { let mut inputs: Vec<(TxIn, TxInMetadata)> = context.inputs.into_values().map(|input| input.into_txin_and_metadata()).collect(); - let mut outputs: Vec = context.outputs.into_values().collect(); + let mut outputs: Vec<(TxOut, TxOutMetadata)> = + context.outputs.into_values().map(|output| output.into_txout_and_metadata()).collect(); inputs.sort_unstable_by_key(|(_, input)| input.serial_id); - outputs.sort_unstable_by_key(|output| output.serial_id); + outputs.sort_unstable_by_key(|(_, output)| output.serial_id); let shared_input_index = context.shared_funding_input.as_ref().and_then(|shared_funding_input| { @@ -296,7 +312,8 @@ impl ConstructedTransaction { }); let (input, input_metadata): (Vec, Vec) = inputs.into_iter().unzip(); - let output = outputs.iter().map(|output| output.tx_out().clone()).collect(); + let (output, output_metadata): (Vec, Vec) = + outputs.into_iter().unzip(); let tx = Transaction { version: Version::TWO, lock_time: context.tx_locktime, input, output }; @@ -316,7 +333,7 @@ impl ConstructedTransaction { remote_outputs_value_satoshis, input_metadata, - outputs, + output_metadata, tx, shared_input_index, @@ -327,10 +344,6 @@ impl ConstructedTransaction { &self.tx } - pub fn outputs(&self) -> impl Iterator { - self.outputs.iter() - } - pub fn input_metadata(&self) -> impl Iterator { self.input_metadata.iter() } @@ -559,15 +572,10 @@ impl InteractiveTxSigningSession { fn local_outputs_count(&self) -> usize { self.unsigned_tx - .outputs + .output_metadata .iter() .enumerate() - .filter(|(_, output)| { - !is_serial_id_valid_for_counterparty( - self.unsigned_tx.holder_is_initiator, - output.serial_id, - ) - }) + .filter(|(_, output)| output.is_local(self.unsigned_tx.holder_is_initiator)) .count() } @@ -1771,12 +1779,6 @@ pub(crate) struct InteractiveTxOutput { output: OutputOwned, } -impl_writeable_tlv_based!(InteractiveTxOutput, { - (1, serial_id, required), - (3, added_by, required), - (5, output, required), -}); - impl InteractiveTxOutput { pub fn tx_out(&self) -> &TxOut { self.output.tx_out() @@ -1801,6 +1803,11 @@ impl InteractiveTxOutput { pub fn script_pubkey(&self) -> &ScriptBuf { &self.output.tx_out().script_pubkey } + + fn into_txout_and_metadata(self) -> (TxOut, TxOutMetadata) { + let txout = self.output.into_tx_out(); + (txout, TxOutMetadata { serial_id: self.serial_id }) + } } impl InteractiveTxInput { @@ -2224,9 +2231,9 @@ mod tests { use core::ops::Deref; use super::{ - get_output_weight, AddingRole, ConstructedTransaction, InteractiveTxOutput, - InteractiveTxSigningSession, OutputOwned, TxInMetadata, P2TR_INPUT_WEIGHT_LOWER_BOUND, - P2WPKH_INPUT_WEIGHT_LOWER_BOUND, P2WSH_INPUT_WEIGHT_LOWER_BOUND, TX_COMMON_FIELDS_WEIGHT, + get_output_weight, ConstructedTransaction, InteractiveTxSigningSession, TxInMetadata, + P2TR_INPUT_WEIGHT_LOWER_BOUND, P2WPKH_INPUT_WEIGHT_LOWER_BOUND, + P2WSH_INPUT_WEIGHT_LOWER_BOUND, TX_COMMON_FIELDS_WEIGHT, }; const TEST_FEERATE_SATS_PER_KW: u32 = FEERATE_FLOOR_SATS_PER_KW * 10; @@ -3309,21 +3316,10 @@ mod tests { }) .collect(); - let outputs: Vec = transaction - .output - .iter() - .cloned() - .map(|txout| InteractiveTxOutput { - serial_id: 0, // N/A for test - added_by: AddingRole::Local, - output: OutputOwned::Single(txout), - }) - .collect(); - let unsigned_tx = ConstructedTransaction { holder_is_initiator: true, input_metadata, - outputs, + output_metadata: vec![], // N/A for test tx: transaction.clone(), local_inputs_value_satoshis: 0, // N/A for test local_outputs_value_satoshis: 0, // N/A for test diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index a5c35180372..afcde507c4c 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -15,7 +15,7 @@ use crate::io::{self, BufRead, Read, Write}; use crate::io_extras::{copy, sink}; -use crate::ln::interactivetxs::{InteractiveTxOutput, TxInMetadata}; +use crate::ln::interactivetxs::{TxInMetadata, TxOutMetadata}; use crate::ln::onion_utils::{HMAC_COUNT, HMAC_LEN, HOLD_TIME_LEN, MAX_HOPS}; use crate::prelude::*; use crate::sync::{Mutex, RwLock}; @@ -1083,7 +1083,7 @@ impl_for_vec!(crate::ln::msgs::SocketAddress); impl_for_vec!((A, B), A, B); impl_for_vec!(SerialId); impl_for_vec!(TxInMetadata); -impl_for_vec!(InteractiveTxOutput); +impl_for_vec!(TxOutMetadata); impl_for_vec!(crate::ln::our_peer_storage::PeerStorageMonitorHolder); impl_for_vec!(crate::blinded_path::message::BlindedMessagePath); impl_writeable_for_vec!(&crate::routing::router::BlindedTail); From a8d7646d51f2757c7900975f9bc99136b6899687 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 22 Sep 2025 16:33:25 -0500 Subject: [PATCH 07/10] Remove unused fields from ConstructedTransaction The output values are never used so there's no need to persist them. If needed, they can be re-computed though the shared output's local and remote amounts would be lost. --- lightning/src/ln/interactivetxs.rs | 31 +++++++----------------------- 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 56800e1390b..10f3a316a4b 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -203,10 +203,7 @@ pub(crate) struct ConstructedTransaction { tx: Transaction, local_inputs_value_satoshis: u64, - local_outputs_value_satoshis: u64, - remote_inputs_value_satoshis: u64, - remote_outputs_value_satoshis: u64, shared_input_index: Option, } @@ -253,10 +250,8 @@ impl_writeable_tlv_based!(ConstructedTransaction, { (5, output_metadata, required), (7, tx, required), (9, local_inputs_value_satoshis, required), - (11, local_outputs_value_satoshis, required), - (13, remote_inputs_value_satoshis, required), - (15, remote_outputs_value_satoshis, required), - (17, shared_input_index, option), + (11, remote_inputs_value_satoshis, required), + (13, shared_input_index, option), }); impl ConstructedTransaction { @@ -280,14 +275,7 @@ impl ConstructedTransaction { .inputs .iter() .fold(0u64, |value, (_, input)| value.saturating_add(input.local_value())); - - let local_outputs_value_satoshis = context - .outputs - .iter() - .fold(0u64, |value, (_, output)| value.saturating_add(output.local_value())); - let remote_inputs_value_satoshis = context.remote_inputs_value(); - let remote_outputs_value_satoshis = context.remote_outputs_value(); let satisfaction_weight = Weight::from_wu(context.inputs.iter().fold(0u64, |value, (_, input)| { @@ -326,16 +314,13 @@ impl ConstructedTransaction { Ok(Self { holder_is_initiator: context.holder_is_initiator, - local_inputs_value_satoshis, - local_outputs_value_satoshis, - - remote_inputs_value_satoshis, - remote_outputs_value_satoshis, - input_metadata, output_metadata, tx, + local_inputs_value_satoshis, + remote_inputs_value_satoshis, + shared_input_index, }) } @@ -3321,10 +3306,8 @@ mod tests { input_metadata, output_metadata: vec![], // N/A for test tx: transaction.clone(), - local_inputs_value_satoshis: 0, // N/A for test - local_outputs_value_satoshis: 0, // N/A for test - remote_inputs_value_satoshis: 0, // N/A for test - remote_outputs_value_satoshis: 0, // N/A for test + local_inputs_value_satoshis: 0, // N/A for test + remote_inputs_value_satoshis: 0, // N/A for test shared_input_index: None, }; From 105c6860e856d8b2abdffb018be64b56a068ecca Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Tue, 23 Sep 2025 11:37:32 -0500 Subject: [PATCH 08/10] Remove input value fields from ConstructedTransaction The local and remote input values are used to determine which node sends tx_signatures first. Instead of persisting these values, compute them only when needed from the input metadata. The spec states that the entire shared input value is to be included for the node sending the corresponding tx_add_input, so it isn't necessary to know the local and remote balances which the metadata does not contain. --- lightning/src/ln/interactivetxs.rs | 48 ++++++++++++++++-------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 10f3a316a4b..05b73323da8 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -197,14 +197,9 @@ impl Display for AbortReason { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct ConstructedTransaction { holder_is_initiator: bool, - input_metadata: Vec, output_metadata: Vec, tx: Transaction, - - local_inputs_value_satoshis: u64, - remote_inputs_value_satoshis: u64, - shared_input_index: Option, } @@ -249,9 +244,7 @@ impl_writeable_tlv_based!(ConstructedTransaction, { (3, input_metadata, required), (5, output_metadata, required), (7, tx, required), - (9, local_inputs_value_satoshis, required), - (11, remote_inputs_value_satoshis, required), - (13, shared_input_index, option), + (9, shared_input_index, option), }); impl ConstructedTransaction { @@ -271,12 +264,6 @@ impl ConstructedTransaction { return Err(AbortReason::MissingFundingOutput); } - let local_inputs_value_satoshis = context - .inputs - .iter() - .fold(0u64, |value, (_, input)| value.saturating_add(input.local_value())); - let remote_inputs_value_satoshis = context.remote_inputs_value(); - let satisfaction_weight = Weight::from_wu(context.inputs.iter().fold(0u64, |value, (_, input)| { value.saturating_add(input.satisfaction_weight().to_wu()) @@ -313,14 +300,9 @@ impl ConstructedTransaction { Ok(Self { holder_is_initiator: context.holder_is_initiator, - input_metadata, output_metadata, tx, - - local_inputs_value_satoshis, - remote_inputs_value_satoshis, - shared_input_index, }) } @@ -337,6 +319,26 @@ impl ConstructedTransaction { self.tx().compute_txid() } + /// Returns the total input value from all local contributions, including the entire shared + /// input value if applicable. + fn local_contributed_input_value(&self) -> Amount { + self.input_metadata + .iter() + .filter(|input| input.is_local(self.holder_is_initiator)) + .map(|input| input.prev_output.value) + .sum() + } + + /// Returns the total input value from all remote contributions, including the entire shared + /// input value if applicable. + fn remote_contributed_input_value(&self) -> Amount { + self.input_metadata + .iter() + .filter(|input| !input.is_local(self.holder_is_initiator)) + .map(|input| input.prev_output.value) + .sum() + } + /// Adds provided holder witnesses to holder inputs of unsigned transaction. /// /// Note that it is assumed that the witness count equals the holder input count. @@ -1379,11 +1381,13 @@ macro_rules! define_state_transitions { let tx = context.validate_tx()?; // Strict ordering prevents deadlocks during tx_signatures exchange + let local_contributed_input_value = tx.local_contributed_input_value(); + let remote_contributed_input_value = tx.remote_contributed_input_value(); let holder_sends_tx_signatures_first = - if tx.local_inputs_value_satoshis == tx.remote_inputs_value_satoshis { + if local_contributed_input_value == remote_contributed_input_value { holder_node_id.serialize() < counterparty_node_id.serialize() } else { - tx.local_inputs_value_satoshis < tx.remote_inputs_value_satoshis + local_contributed_input_value < remote_contributed_input_value }; let signing_session = InteractiveTxSigningSession { @@ -3306,8 +3310,6 @@ mod tests { input_metadata, output_metadata: vec![], // N/A for test tx: transaction.clone(), - local_inputs_value_satoshis: 0, // N/A for test - remote_inputs_value_satoshis: 0, // N/A for test shared_input_index: None, }; From f20a47732bf6cd16e1753f01f804087ede95f71c Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 22 Sep 2025 18:29:12 -0500 Subject: [PATCH 09/10] Persist witnesses in InteractiveTxSigningSession InteractiveTxSigningSession currently persists holder witnesses directly, but persists counterparty witnesses as part of its unsigned ConstructedTransaction. This makes the ConstructedTransaction actually partially signed even though it is held in a field named unsigned_tx. Instead, persists the counterparty witnesses alongside the holder witnesses directly in InteractiveTxSigningSession, leaving the transaction it holds unsigned. --- lightning/src/ln/interactivetxs.rs | 142 ++++++++++++++--------------- 1 file changed, 71 insertions(+), 71 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 05b73323da8..32f4a468cbe 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -17,7 +17,6 @@ use bitcoin::constants::WITNESS_SCALE_FACTOR; use bitcoin::ecdsa::Signature as BitcoinSignature; use bitcoin::key::Secp256k1; use bitcoin::policy::MAX_STANDARD_TX_WEIGHT; -use bitcoin::secp256k1::ecdsa::Signature; use bitcoin::secp256k1::{Message, PublicKey}; use bitcoin::sighash::SighashCache; use bitcoin::transaction::Version; @@ -339,11 +338,54 @@ impl ConstructedTransaction { .sum() } + fn finalize( + &self, holder_tx_signatures: &TxSignatures, counterparty_tx_signatures: &TxSignatures, + shared_input_sig: Option<&SharedInputSignature>, + ) -> Option { + let mut tx = self.tx.clone(); + self.add_local_witnesses(&mut tx, holder_tx_signatures.witnesses.clone()); + self.add_remote_witnesses(&mut tx, counterparty_tx_signatures.witnesses.clone()); + + if let Some(shared_input_index) = self.shared_input_index { + let holder_shared_input_sig = + holder_tx_signatures.shared_input_signature.or_else(|| { + debug_assert!(false); + None + })?; + let counterparty_shared_input_sig = + counterparty_tx_signatures.shared_input_signature.or_else(|| { + debug_assert!(false); + None + })?; + + let shared_input_sig = shared_input_sig.or_else(|| { + debug_assert!(false); + None + })?; + + let mut witness = Witness::new(); + witness.push(Vec::new()); + let holder_sig = BitcoinSignature::sighash_all(holder_shared_input_sig); + let counterparty_sig = BitcoinSignature::sighash_all(counterparty_shared_input_sig); + if shared_input_sig.holder_signature_first { + witness.push_ecdsa_signature(&holder_sig); + witness.push_ecdsa_signature(&counterparty_sig); + } else { + witness.push_ecdsa_signature(&counterparty_sig); + witness.push_ecdsa_signature(&holder_sig); + } + witness.push(&shared_input_sig.witness_script); + tx.input[shared_input_index as usize].witness = witness; + } + + Some(tx) + } + /// Adds provided holder witnesses to holder inputs of unsigned transaction. /// /// Note that it is assumed that the witness count equals the holder input count. - fn add_local_witnesses(&mut self, witnesses: Vec) { - self.tx + fn add_local_witnesses(&self, transaction: &mut Transaction, witnesses: Vec) { + transaction .input .iter_mut() .zip(self.input_metadata.iter()) @@ -362,8 +404,8 @@ impl ConstructedTransaction { /// Adds counterparty witnesses to counterparty inputs of unsigned transaction. /// /// Note that it is assumed that the witness count equals the counterparty input count. - fn add_remote_witnesses(&mut self, witnesses: Vec) { - self.tx + fn add_remote_witnesses(&self, transaction: &mut Transaction, witnesses: Vec) { + transaction .input .iter_mut() .zip(self.input_metadata.iter()) @@ -392,13 +434,11 @@ impl ConstructedTransaction { pub(crate) struct SharedInputSignature { holder_signature_first: bool, witness_script: ScriptBuf, - counterparty_signature: Option, } impl_writeable_tlv_based!(SharedInputSignature, { (1, holder_signature_first, required), (3, witness_script, required), - (5, counterparty_signature, required), }); /// The InteractiveTxSigningSession coordinates the signing flow of interactively constructed @@ -413,9 +453,9 @@ pub(crate) struct InteractiveTxSigningSession { unsigned_tx: ConstructedTransaction, holder_sends_tx_signatures_first: bool, has_received_commitment_signed: bool, - has_received_tx_signatures: bool, shared_input_signature: Option, holder_tx_signatures: Option, + counterparty_tx_signatures: Option, } impl InteractiveTxSigningSession { @@ -432,7 +472,7 @@ impl InteractiveTxSigningSession { } pub fn has_received_tx_signatures(&self) -> bool { - self.has_received_tx_signatures + self.counterparty_tx_signatures.is_some() } pub fn holder_tx_signatures(&self) -> &Option { @@ -457,7 +497,7 @@ impl InteractiveTxSigningSession { pub fn received_tx_signatures( &mut self, tx_signatures: &TxSignatures, ) -> Result<(Option, Option), String> { - if self.has_received_tx_signatures { + if self.has_received_tx_signatures() { return Err("Already received a tx_signatures message".to_string()); } if self.remote_inputs_count() != tx_signatures.witnesses.len() { @@ -470,11 +510,7 @@ impl InteractiveTxSigningSession { return Err("Unexpected shared input signature".to_string()); } - self.unsigned_tx.add_remote_witnesses(tx_signatures.witnesses.clone()); - if let Some(ref mut shared_input_sig) = self.shared_input_signature { - shared_input_sig.counterparty_signature = tx_signatures.shared_input_signature.clone(); - } - self.has_received_tx_signatures = true; + self.counterparty_tx_signatures = Some(tx_signatures.clone()); let holder_tx_signatures = if !self.holder_sends_tx_signatures_first { self.holder_tx_signatures.clone() @@ -482,14 +518,7 @@ impl InteractiveTxSigningSession { None }; - // Check if the holder has provided its signatures and if so, - // return the finalized funding transaction. - let funding_tx_opt = if self.holder_tx_signatures.is_some() { - Some(self.finalize_funding_tx()) - } else { - // This means we're still waiting for the holder to provide their signatures. - None - }; + let funding_tx_opt = self.maybe_finalize_funding_tx(); Ok((holder_tx_signatures, funding_tx_opt)) } @@ -516,15 +545,15 @@ impl InteractiveTxSigningSession { self.verify_interactive_tx_signatures(secp_ctx, &tx_signatures.witnesses)?; - self.unsigned_tx.add_local_witnesses(tx_signatures.witnesses.clone()); self.holder_tx_signatures = Some(tx_signatures); - let funding_tx_opt = self.has_received_tx_signatures.then(|| self.finalize_funding_tx()); - let holder_tx_signatures = - (self.holder_sends_tx_signatures_first || self.has_received_tx_signatures).then(|| { - debug_assert!(self.has_received_commitment_signed); - self.holder_tx_signatures.clone().expect("Holder tx_signatures were just provided") - }); + let funding_tx_opt = self.maybe_finalize_funding_tx(); + let holder_tx_signatures = (self.holder_sends_tx_signatures_first + || self.has_received_tx_signatures()) + .then(|| { + debug_assert!(self.has_received_commitment_signed); + self.holder_tx_signatures.clone().expect("Holder tx_signatures were just provided") + }); Ok((holder_tx_signatures, funding_tx_opt)) } @@ -576,43 +605,15 @@ impl InteractiveTxSigningSession { }) } - fn finalize_funding_tx(&mut self) -> Transaction { - if let Some(shared_input_index) = self.unsigned_tx.shared_input_index { - if let Some(holder_shared_input_sig) = self - .holder_tx_signatures - .as_ref() - .and_then(|holder_tx_sigs| holder_tx_sigs.shared_input_signature) - { - if let Some(ref shared_input_sig) = self.shared_input_signature { - if let Some(counterparty_shared_input_sig) = - shared_input_sig.counterparty_signature - { - let mut witness = Witness::new(); - witness.push(Vec::new()); - let holder_sig = BitcoinSignature::sighash_all(holder_shared_input_sig); - let counterparty_sig = - BitcoinSignature::sighash_all(counterparty_shared_input_sig); - if shared_input_sig.holder_signature_first { - witness.push_ecdsa_signature(&holder_sig); - witness.push_ecdsa_signature(&counterparty_sig); - } else { - witness.push_ecdsa_signature(&counterparty_sig); - witness.push_ecdsa_signature(&holder_sig); - } - witness.push(&shared_input_sig.witness_script); - self.unsigned_tx.tx.input[shared_input_index as usize].witness = witness; - } else { - debug_assert!(false); - } - } else { - debug_assert!(false); - } - } else { - debug_assert!(false); - } - } - - self.unsigned_tx.tx.clone() + fn maybe_finalize_funding_tx(&mut self) -> Option { + let holder_tx_signatures = self.holder_tx_signatures.as_ref()?; + let counterparty_tx_signatures = self.counterparty_tx_signatures.as_ref()?; + let shared_input_signature = self.shared_input_signature.as_ref(); + self.unsigned_tx.finalize( + holder_tx_signatures, + counterparty_tx_signatures, + shared_input_signature, + ) } fn verify_interactive_tx_signatures( @@ -781,7 +782,7 @@ impl_writeable_tlv_based!(InteractiveTxSigningSession, { (1, unsigned_tx, required), (3, has_received_commitment_signed, required), (5, holder_tx_signatures, required), - (7, has_received_tx_signatures, required), + (7, counterparty_tx_signatures, required), (9, holder_sends_tx_signatures_first, required), (11, shared_input_signature, required), }); @@ -1372,7 +1373,6 @@ macro_rules! define_state_transitions { .as_ref() .map(|shared_input| SharedInputSignature { holder_signature_first: shared_input.holder_sig_first, - counterparty_signature: None, witness_script: shared_input.witness_script.clone(), }); let holder_node_id = context.holder_node_id; @@ -1394,9 +1394,9 @@ macro_rules! define_state_transitions { unsigned_tx: tx, holder_sends_tx_signatures_first, has_received_commitment_signed: false, - has_received_tx_signatures: false, shared_input_signature, holder_tx_signatures: None, + counterparty_tx_signatures: None, }; Ok(NegotiationComplete(signing_session)) } @@ -3319,9 +3319,9 @@ mod tests { unsigned_tx, holder_sends_tx_signatures_first: false, // N/A for test has_received_commitment_signed: false, // N/A for test - has_received_tx_signatures: false, // N/A for test shared_input_signature: None, holder_tx_signatures: None, + counterparty_tx_signatures: None, } .verify_interactive_tx_signatures( &secp_ctx, From 86ed012a570791058e9dfc60297b019580f89fbf Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Tue, 23 Sep 2025 11:52:19 -0500 Subject: [PATCH 10/10] Reduce visibility of ConstructedTransaction methods --- lightning/src/ln/interactivetxs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 32f4a468cbe..3c683fcc5ee 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -310,7 +310,7 @@ impl ConstructedTransaction { &self.tx } - pub fn input_metadata(&self) -> impl Iterator { + fn input_metadata(&self) -> impl Iterator { self.input_metadata.iter() } @@ -421,7 +421,7 @@ impl ConstructedTransaction { .for_each(|(input, witness)| input.witness = witness); } - pub fn holder_is_initiator(&self) -> bool { + fn holder_is_initiator(&self) -> bool { self.holder_is_initiator }