Remove TransactionU16LenLimited

TransactionU16LenLimited was used to limit Transaction serialization
size to u16::MAX. This was because messages can not be longer than
u16::MAX bytes when serialized for the transport layer. However, this
limit doesn't take into account other fields in a message containing a
Transaction, including the length of the transaction itself.

Remove TransactionU16LenLimited and instead check any user supplied
transactions in the context of the enclosing message (e.g. TxAddInput).

Author: jkczyz

lightning/src/ln/channel.rs

@@ -72,6 +72,8 @@ use crate::ln::onion_utils::{
 };
 use crate::ln::script::{self, ShutdownScript};
 use crate::ln::types::ChannelId;
+#[cfg(splicing)]
+use crate::ln::LN_MAX_MSG_LEN;
 use crate::routing::gossip::NodeId;
 use crate::sign::ecdsa::EcdsaChannelSigner;
 use crate::sign::tx_builder::{SpecTxBuilder, TxBuilder};
@@ -85,9 +87,7 @@ use crate::util::config::{
 use crate::util::errors::APIError;
 use crate::util::logger::{Logger, Record, WithContext};
 use crate::util::scid_utils::{block_from_scid, scid_from_parts};
-use crate::util::ser::{
-	Readable, ReadableArgs, RequiredWrapper, TransactionU16LenLimited, Writeable, Writer,
-};
+use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, Writeable, Writer};
 
 use alloc::collections::{btree_map, BTreeMap};
 
@@ -5978,7 +5978,7 @@ pub(super) struct FundingNegotiationContext {
 	pub shared_funding_input: Option<SharedOwnedInput>,
 	/// The funding inputs we will be contributing to the channel.
 	#[allow(dead_code)] // TODO(dual_funding): Remove once contribution to V2 channels is enabled.
-	pub our_funding_inputs: Vec<(TxIn, TransactionU16LenLimited)>,
+	pub our_funding_inputs: Vec<(TxIn, Transaction)>,
 	/// The change output script. This will be used if needed or -- if not set -- generated using
 	/// `SignerProvider::get_destination_script`.
 	#[allow(dead_code)] // TODO(splicing): Remove once splicing is enabled.
@@ -10670,10 +10670,26 @@ where
 		})?;
 		// Convert inputs
 		let mut funding_inputs = Vec::new();
-		for (tx_in, tx, _w) in our_funding_inputs.into_iter() {
-			let tx16 = TransactionU16LenLimited::new(tx)
-				.map_err(|_e| APIError::APIMisuseError { err: format!("Too large transaction") })?;
-			funding_inputs.push((tx_in, tx16));
+		for (txin, tx, _) in our_funding_inputs.into_iter() {
+			const MESSAGE_TEMPLATE: msgs::TxAddInput = msgs::TxAddInput {
+				channel_id: ChannelId([0; 32]),
+				serial_id: 0,
+				prevtx: None,
+				prevtx_out: 0,
+				sequence: 0,
+				shared_input_txid: None,
+			};
+			let message_len = MESSAGE_TEMPLATE.serialized_length() + tx.serialized_length();
+			if message_len > LN_MAX_MSG_LEN {
+				return Err(APIError::APIMisuseError {
+					err: format!(
+						"Funding input references a prevtx that is too large for tx_add_input: {}",
+						txin.previous_output,
+					),
+				});
+			}
+
+			funding_inputs.push((txin, tx));
 		}
 
 		let prev_funding_input = self.funding.to_splice_funding_input();
@@ -12452,7 +12468,7 @@ where
 	pub fn new_outbound<ES: Deref, F: Deref, L: Deref>(
 		fee_estimator: &LowerBoundedFeeEstimator<F>, entropy_source: &ES, signer_provider: &SP,
 		counterparty_node_id: PublicKey, their_features: &InitFeatures, funding_satoshis: u64,
-		funding_inputs: Vec<(TxIn, TransactionU16LenLimited)>, user_id: u128, config: &UserConfig,
+		funding_inputs: Vec<(TxIn, Transaction)>, user_id: u128, config: &UserConfig,
 		current_chain_height: u32, outbound_scid_alias: u64, funding_confirmation_target: ConfirmationTarget,
 		logger: L,
 	) -> Result<Self, APIError>

lightning/src/ln/dual_funding_tests.rs

@@ -23,7 +23,6 @@ use {
 	crate::ln::msgs::{CommitmentSigned, TxAddInput, TxAddOutput, TxComplete, TxSignatures},
 	crate::ln::types::ChannelId,
 	crate::prelude::*,
-	crate::util::ser::TransactionU16LenLimited,
 	crate::util::test_utils,
 	bitcoin::Witness,
 };
@@ -51,7 +50,7 @@ fn do_test_v2_channel_establishment(session: V2ChannelEstablishmentTestSession)
 		&[session.initiator_input_value_satoshis],
 	)
 	.into_iter()
-	.map(|(txin, tx, _)| (txin, TransactionU16LenLimited::new(tx).unwrap()))
+	.map(|(txin, tx, _)| (txin, tx))
 	.collect();
 
 	// Alice creates a dual-funded channel as initiator.
@@ -94,7 +93,7 @@ fn do_test_v2_channel_establishment(session: V2ChannelEstablishmentTestSession)
 		sequence: initiator_funding_inputs[0].0.sequence.0,
 		shared_input_txid: None,
 	};
-	let input_value = tx_add_input_msg.prevtx.as_ref().unwrap().as_transaction().output
+	let input_value = tx_add_input_msg.prevtx.as_ref().unwrap().output
 		[tx_add_input_msg.prevtx_out as usize]
 		.value;
 	assert_eq!(input_value.to_sat(), session.initiator_input_value_satoshis);

lightning/src/ln/interactivetxs.rs

@@ -32,7 +32,6 @@ use crate::ln::msgs;
 use crate::ln::msgs::{MessageSendEvent, SerialId, TxSignatures};
 use crate::ln::types::ChannelId;
 use crate::sign::{EntropySource, P2TR_KEY_PATH_WITNESS_WEIGHT, P2WPKH_WITNESS_WEIGHT};
-use crate::util::ser::TransactionU16LenLimited;
 
 use core::fmt::Display;
 use core::ops::Deref;
@@ -869,10 +868,9 @@ impl NegotiationContext {
 				return Err(AbortReason::UnexpectedFundingInput);
 			}
 		} else if let Some(prevtx) = &msg.prevtx {
-			let transaction = prevtx.as_transaction();
-			let txid = transaction.compute_txid();
+			let txid = prevtx.compute_txid();
 
-			if let Some(tx_out) = transaction.output.get(msg.prevtx_out as usize) {
+			if let Some(tx_out) = prevtx.output.get(msg.prevtx_out as usize) {
 				if !tx_out.script_pubkey.is_witness_program() {
 					// The receiving node:
 					//  - MUST fail the negotiation if:
@@ -1053,14 +1051,9 @@ impl NegotiationContext {
 				return Err(AbortReason::UnexpectedFundingInput);
 			}
 		} else if let Some(prevtx) = &msg.prevtx {
-			let prev_txid = prevtx.as_transaction().compute_txid();
+			let prev_txid = prevtx.compute_txid();
 			let prev_outpoint = OutPoint { txid: prev_txid, vout: msg.prevtx_out };
-			let prev_output = prevtx
-				.as_transaction()
-				.output
-				.get(vout)
-				.ok_or(AbortReason::PrevTxOutInvalid)?
-				.clone();
+			let prev_output = prevtx.output.get(vout).ok_or(AbortReason::PrevTxOutInvalid)?.clone();
 			let txin = TxIn {
 				previous_output: prev_outpoint,
 				sequence: Sequence(msg.sequence),
@@ -1441,7 +1434,7 @@ impl_writeable_tlv_based_enum!(AddingRole,
 #[derive(Clone, Debug, Eq, PartialEq)]
 struct SingleOwnedInput {
 	input: TxIn,
-	prev_tx: TransactionU16LenLimited,
+	prev_tx: Transaction,
 	prev_output: TxOut,
 }
 
@@ -1843,7 +1836,7 @@ where
 	pub feerate_sat_per_kw: u32,
 	pub is_initiator: bool,
 	pub funding_tx_locktime: AbsoluteLockTime,
-	pub inputs_to_contribute: Vec<(TxIn, TransactionU16LenLimited)>,
+	pub inputs_to_contribute: Vec<(TxIn, Transaction)>,
 	pub shared_funding_input: Option<SharedOwnedInput>,
 	pub shared_funding_output: SharedOwnedOutput,
 	pub outputs_to_contribute: Vec<TxOut>,
@@ -1885,7 +1878,7 @@ impl InteractiveTxConstructor {
 		// Check for the existence of prevouts'
 		for (txin, tx) in inputs_to_contribute.iter() {
 			let vout = txin.previous_output.vout as usize;
-			if tx.as_transaction().output.get(vout).is_none() {
+			if tx.output.get(vout).is_none() {
 				return Err(AbortReason::PrevTxOutInvalid);
 			}
 		}
@@ -1894,7 +1887,7 @@ impl InteractiveTxConstructor {
 			.map(|(txin, tx)| {
 				let serial_id = generate_holder_serial_id(entropy_source, is_initiator);
 				let vout = txin.previous_output.vout as usize;
-				let prev_output = tx.as_transaction().output.get(vout).unwrap().clone(); // checked above
+				let prev_output = tx.output.get(vout).unwrap().clone(); // checked above
 				let input =
 					InputOwned::Single(SingleOwnedInput { input: txin, prev_tx: tx, prev_output });
 				(serial_id, input)
@@ -2083,12 +2076,11 @@ pub(super) fn calculate_change_output_value(
 	let mut total_input_satoshis = 0u64;
 	let mut our_funding_inputs_weight = 0u64;
 	for (txin, tx) in context.our_funding_inputs.iter() {
-		let txid = tx.as_transaction().compute_txid();
+		let txid = tx.compute_txid();
 		if txin.previous_output.txid != txid {
 			return Err(AbortReason::PrevTxOutInvalid);
 		}
 		let output = tx
-			.as_transaction()
 			.output
 			.get(txin.previous_output.vout as usize)
 			.ok_or(AbortReason::PrevTxOutInvalid)?;
@@ -2145,7 +2137,6 @@ mod tests {
 	use crate::ln::types::ChannelId;
 	use crate::sign::EntropySource;
 	use crate::util::atomic_counter::AtomicCounter;
-	use crate::util::ser::TransactionU16LenLimited;
 	use bitcoin::absolute::LockTime as AbsoluteLockTime;
 	use bitcoin::amount::Amount;
 	use bitcoin::hashes::Hash;
@@ -2211,12 +2202,12 @@ mod tests {
 
 	struct TestSession {
 		description: &'static str,
-		inputs_a: Vec<(TxIn, TransactionU16LenLimited)>,
+		inputs_a: Vec<(TxIn, Transaction)>,
 		a_shared_input: Option<(OutPoint, TxOut, u64)>,
 		/// The funding output, with the value contributed
 		shared_output_a: (TxOut, u64),
 		outputs_a: Vec<TxOut>,
-		inputs_b: Vec<(TxIn, TransactionU16LenLimited)>,
+		inputs_b: Vec<(TxIn, Transaction)>,
 		b_shared_input: Option<(OutPoint, TxOut, u64)>,
 		/// The funding output, with the value contributed
 		shared_output_b: (TxOut, u64),
@@ -2482,7 +2473,7 @@ mod tests {
 		}
 	}
 
-	fn generate_inputs(outputs: &[TestOutput]) -> Vec<(TxIn, TransactionU16LenLimited)> {
+	fn generate_inputs(outputs: &[TestOutput]) -> Vec<(TxIn, Transaction)> {
 		let tx = generate_tx(outputs);
 		let txid = tx.compute_txid();
 		tx.output
@@ -2495,7 +2486,7 @@ mod tests {
 					sequence: Sequence::ENABLE_RBF_NO_LOCKTIME,
 					witness: Default::default(),
 				};
-				(txin, TransactionU16LenLimited::new(tx.clone()).unwrap())
+				(txin, tx.clone())
 			})
 			.collect()
 	}
@@ -2543,12 +2534,12 @@ mod tests {
 		(generate_txout(&TestOutput::P2WSH(value)), local_value)
 	}
 
-	fn generate_fixed_number_of_inputs(count: u16) -> Vec<(TxIn, TransactionU16LenLimited)> {
+	fn generate_fixed_number_of_inputs(count: u16) -> Vec<(TxIn, Transaction)> {
 		// 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, TransactionU16LenLimited)> = Vec::with_capacity(count as usize);
+		let mut inputs: Vec<(TxIn, Transaction)> = Vec::with_capacity(count as usize);
 
 		while remaining > 0 {
 			let tx_output_count = remaining.min(max_outputs_per_prevtx);
@@ -2561,7 +2552,7 @@ mod tests {
 			);
 			let txid = tx.compute_txid();
 
-			let mut temp: Vec<(TxIn, TransactionU16LenLimited)> = tx
+			let mut temp: Vec<(TxIn, Transaction)> = tx
 				.output
 				.iter()
 				.enumerate()
@@ -2572,7 +2563,7 @@ mod tests {
 						sequence: Sequence::ENABLE_RBF_NO_LOCKTIME,
 						witness: Default::default(),
 					};
-					(input, TransactionU16LenLimited::new(tx.clone()).unwrap())
+					(input, tx.clone())
 				})
 				.collect();
 
@@ -2783,10 +2774,9 @@ mod tests {
 			expect_error: Some((AbortReason::PrevTxOutInvalid, ErrorCulprit::NodeA)),
 		});
 
-		let tx =
-			TransactionU16LenLimited::new(generate_tx(&[TestOutput::P2WPKH(1_000_000)])).unwrap();
+		let tx = generate_tx(&[TestOutput::P2WPKH(1_000_000)]);
 		let invalid_sequence_input = TxIn {
-			previous_output: OutPoint { txid: tx.as_transaction().compute_txid(), vout: 0 },
+			previous_output: OutPoint { txid: tx.compute_txid(), vout: 0 },
 			..Default::default()
 		};
 		do_test_interactive_tx_constructor(TestSession {
@@ -2802,7 +2792,7 @@ mod tests {
 			expect_error: Some((AbortReason::IncorrectInputSequenceValue, ErrorCulprit::NodeA)),
 		});
 		let duplicate_input = TxIn {
-			previous_output: OutPoint { txid: tx.as_transaction().compute_txid(), vout: 0 },
+			previous_output: OutPoint { txid: tx.compute_txid(), vout: 0 },
 			sequence: Sequence::ENABLE_RBF_NO_LOCKTIME,
 			..Default::default()
 		};
@@ -2820,7 +2810,7 @@ mod tests {
 		});
 		// Non-initiator uses same prevout as initiator.
 		let duplicate_input = TxIn {
-			previous_output: OutPoint { txid: tx.as_transaction().compute_txid(), vout: 0 },
+			previous_output: OutPoint { txid: tx.compute_txid(), vout: 0 },
 			sequence: Sequence::ENABLE_RBF_NO_LOCKTIME,
 			..Default::default()
 		};
@@ -2837,7 +2827,7 @@ mod tests {
 			expect_error: Some((AbortReason::PrevTxOutInvalid, ErrorCulprit::NodeA)),
 		});
 		let duplicate_input = TxIn {
-			previous_output: OutPoint { txid: tx.as_transaction().compute_txid(), vout: 0 },
+			previous_output: OutPoint { txid: tx.compute_txid(), vout: 0 },
 			sequence: Sequence::ENABLE_RBF_NO_LOCKTIME,
 			..Default::default()
 		};
@@ -3170,9 +3160,9 @@ mod tests {
 					sequence: Sequence::ZERO,
 					witness: Witness::new(),
 				};
-				(txin, TransactionU16LenLimited::new(tx).unwrap())
+				(txin, tx)
 			})
-			.collect::<Vec<(TxIn, TransactionU16LenLimited)>>();
+			.collect::<Vec<(TxIn, Transaction)>>();
 		let our_contributed = 110_000;
 		let txout = TxOut { value: Amount::from_sat(10_000), script_pubkey: ScriptBuf::new() };
 		let outputs = vec![txout];