fix Simplify calculate_change_output_value, inline prevtx check

Author: optout21

lightning/src/ln/interactivetxs.rs

@@ -1690,26 +1690,24 @@ pub(super) fn calculate_change_output_value(
 	funding_inputs: &Vec<(TxIn, TransactionU16LenLimited)>, funding_outputs: &Vec<OutputOwned>,
 	funding_feerate_sat_per_1000_weight: u32, change_output_dust_limit: u64,
 ) -> Result<Option<u64>, AbortReason> {
-	let funding_input_prev_outs = txouts_from_input_prev_txs(&funding_inputs)?;
-	calculate_change_output_value_prevout(
-		is_initiator,
-		our_contribution,
-		&funding_input_prev_outs,
-		funding_outputs,
-		funding_feerate_sat_per_1000_weight,
-		change_output_dust_limit,
-	)
-}
+	// Process inputs and their prev txs:
+	// calculate value sum and weight sum of inputs, also perform checks
+	let mut total_input_satoshis = 0u64;
+	let mut our_funding_inputs_weight = 0u64;
+	for (txin, tx) in funding_inputs.iter() {
+		let txid = tx.as_transaction().compute_txid();
+		if txin.previous_output.txid != txid {
+			return Err(AbortReason::PrevTxOutInvalid);
+		}
+		if let Some(output) = tx.as_transaction().output.get(txin.previous_output.vout as usize) {
+			total_input_satoshis = total_input_satoshis.saturating_add(output.value.to_sat());
+			our_funding_inputs_weight =
+				our_funding_inputs_weight.saturating_add(estimate_input_weight(output).to_wu());
+		} else {
+			return Err(AbortReason::PrevTxOutInvalid);
+		}
+	}
 
-fn calculate_change_output_value_prevout(
-	is_initiator: bool, our_contribution: u64, funding_inputs_prev_outputs: &Vec<&TxOut>,
-	funding_outputs: &Vec<OutputOwned>, funding_feerate_sat_per_1000_weight: u32,
-	change_output_dust_limit: u64,
-) -> Result<Option<u64>, AbortReason> {
-	let our_funding_inputs_weight =
-		funding_inputs_prev_outputs.iter().fold(0u64, |weight, prev_output| {
-			weight.saturating_add(estimate_input_weight(prev_output).to_wu())
-		});
 	let our_funding_outputs_weight = funding_outputs.iter().fold(0u64, |weight, out| {
 		weight.saturating_add(get_output_weight(&out.tx_out().script_pubkey).to_wu())
 	});
@@ -1721,9 +1719,6 @@ fn calculate_change_output_value_prevout(
 	}
 
 	let fees_sats = fee_for_weight(funding_feerate_sat_per_1000_weight, weight);
-	let total_input_satoshis: u64 =
-		funding_inputs_prev_outputs.iter().map(|out| out.value.to_sat()).sum();
-
 	// Note: in case of additional outputs, they will have to be subtracted here
 
 	let total_inputs_less_fees = total_input_satoshis.saturating_sub(fees_sats);
@@ -1741,34 +1736,12 @@ fn calculate_change_output_value_prevout(
 	}
 }
 
-/// Obtain prev outputs for each supplied input and matching transaction.
-/// Will error when a prev tx does not have an output for the specified vout.
-/// Also checks for matching of transaction IDs.
-fn txouts_from_input_prev_txs(
-	inputs: &Vec<(TxIn, TransactionU16LenLimited)>,
-) -> Result<Vec<&TxOut>, AbortReason> {
-	let mut prev_outputs: Vec<&TxOut> = Vec::with_capacity(inputs.len());
-	// Check that vouts exist for each TxIn in provided transactions.
-	for (txin, tx) in inputs.iter() {
-		let txid = tx.as_transaction().compute_txid();
-		if txin.previous_output.txid != txid {
-			return Err(AbortReason::PrevTxOutInvalid);
-		}
-		if let Some(output) = tx.as_transaction().output.get(txin.previous_output.vout as usize) {
-			prev_outputs.push(output);
-		} else {
-			return Err(AbortReason::PrevTxOutInvalid);
-		}
-	}
-	Ok(prev_outputs)
-}
-
 #[cfg(test)]
 mod tests {
 	use crate::chain::chaininterface::{fee_for_weight, FEERATE_FLOOR_SATS_PER_KW};
 	use crate::ln::channel::TOTAL_BITCOIN_SUPPLY_SATOSHIS;
 	use crate::ln::interactivetxs::{
-		calculate_change_output_value_prevout, generate_holder_serial_id, AbortReason,
+		calculate_change_output_value, generate_holder_serial_id, AbortReason,
 		HandleTxCompleteValue, InteractiveTxConstructor, InteractiveTxConstructorArgs,
 		InteractiveTxMessageSend, MAX_INPUTS_OUTPUTS_COUNT, MAX_RECEIVED_TX_ADD_INPUT_COUNT,
 		MAX_RECEIVED_TX_ADD_OUTPUT_COUNT,
@@ -1786,7 +1759,7 @@ mod tests {
 	use bitcoin::secp256k1::{Keypair, PublicKey, Secp256k1, SecretKey};
 	use bitcoin::transaction::Version;
 	use bitcoin::{
-		OutPoint, PubkeyHash, ScriptBuf, Sequence, Transaction, TxIn, TxOut, WPubkeyHash,
+		OutPoint, PubkeyHash, ScriptBuf, Sequence, Transaction, TxIn, TxOut, WPubkeyHash, Witness,
 	};
 	use core::ops::Deref;
 
@@ -2704,6 +2677,25 @@ mod tests {
 			TxOut { value: Amount::from_sat(60_000), script_pubkey: ScriptBuf::new() },
 		];
 		let input_prevouts: Vec<&TxOut> = input_prevouts_owned.iter().collect();
+		let inputs = input_prevouts
+			.iter()
+			.map(|txout| {
+				let tx = Transaction {
+					input: Vec::new(),
+					output: vec![(*txout).clone()],
+					lock_time: AbsoluteLockTime::ZERO,
+					version: Version::TWO,
+				};
+				let txid = tx.compute_txid();
+				let txin = TxIn {
+					previous_output: OutPoint { txid, vout: 0 },
+					script_sig: ScriptBuf::new(),
+					sequence: Sequence::ZERO,
+					witness: Witness::new(),
+				};
+				(txin, TransactionU16LenLimited::new(tx).unwrap())
+			})
+			.collect::<Vec<(TxIn, TransactionU16LenLimited)>>();
 		let our_contributed = 110_000;
 		let txout = TxOut { value: Amount::from_sat(128_000), script_pubkey: ScriptBuf::new() };
 		let outputs = vec![OutputOwned::SharedControlFullyOwned(txout)];
@@ -2715,10 +2707,10 @@ mod tests {
 		let common_fees = 126;
 		{
 			// There is leftover for change
-			let res = calculate_change_output_value_prevout(
+			let res = calculate_change_output_value(
 				true,
 				our_contributed,
-				&input_prevouts,
+				&inputs,
 				&outputs,
 				funding_feerate_sat_per_1000_weight,
 				300,
@@ -2727,10 +2719,10 @@ mod tests {
 		}
 		{
 			// There is leftover for change, without common fees
-			let res = calculate_change_output_value_prevout(
+			let res = calculate_change_output_value(
 				false,
 				our_contributed,
-				&input_prevouts,
+				&inputs,
 				&outputs,
 				funding_feerate_sat_per_1000_weight,
 				300,
@@ -2739,22 +2731,16 @@ mod tests {
 		}
 		{
 			// Larger fee, smaller change
-			let res = calculate_change_output_value_prevout(
-				true,
-				our_contributed,
-				&input_prevouts,
-				&outputs,
-				9000,
-				300,
-			);
+			let res =
+				calculate_change_output_value(true, our_contributed, &inputs, &outputs, 9000, 300);
 			assert_eq!(res.unwrap().unwrap(), 14384);
 		}
 		{
 			// Insufficient inputs, no leftover
-			let res = calculate_change_output_value_prevout(
+			let res = calculate_change_output_value(
 				false,
 				130_000,
-				&input_prevouts,
+				&inputs,
 				&outputs,
 				funding_feerate_sat_per_1000_weight,
 				300,
@@ -2763,10 +2749,10 @@ mod tests {
 		}
 		{
 			// Very small leftover
-			let res = calculate_change_output_value_prevout(
+			let res = calculate_change_output_value(
 				false,
 				128_100,
-				&input_prevouts,
+				&inputs,
 				&outputs,
 				funding_feerate_sat_per_1000_weight,
 				300,
@@ -2775,10 +2761,10 @@ mod tests {
 		}
 		{
 			// Small leftover, but not dust
-			let res = calculate_change_output_value_prevout(
+			let res = calculate_change_output_value(
 				false,
 				128_100,
-				&input_prevouts,
+				&inputs,
 				&outputs,
 				funding_feerate_sat_per_1000_weight,
 				100,