f - pass FundingNegotiationContext to calculate_change_output_value

Author: jkczyz

lightning/src/ln/channel.rs

@@ -2768,18 +2768,14 @@ where
 	#[allow(dead_code)] // TODO(dual_funding): Remove once contribution to V2 channels is enabled
 	fn begin_interactive_funding_tx_construction<ES: Deref>(
 		&mut self, signer_provider: &SP, entropy_source: &ES, holder_node_id: PublicKey,
-		is_initiator: bool, change_destination_opt: Option<ScriptBuf>,
-		shared_funding_input: Option<SharedOwnedInput>,
+		change_destination_opt: Option<ScriptBuf>, shared_funding_input: Option<SharedOwnedInput>,
 	) -> Result<Option<InteractiveTxMessageSend>, AbortReason>
 	where
 		ES::Target: EntropySource,
 	{
 		debug_assert!(matches!(self.context.channel_state, ChannelState::NegotiatingFunding(_)));
 		debug_assert!(self.interactive_tx_constructor.is_none());
 
-		let mut funding_inputs = Vec::new();
-		mem::swap(&mut self.funding_negotiation_context.our_funding_inputs, &mut funding_inputs);
-
 		// Add output for funding tx
 		// Note: For the error case when the inputs are insufficient, it will be handled after
 		// the `calculate_change_output_value` call below
@@ -2799,13 +2795,10 @@ where
 				.map_err(|_err| AbortReason::InternalError("Error getting destination script"))?
 		};
 		let change_value_opt = calculate_change_output_value(
-			is_initiator,
-			self.funding_negotiation_context.our_funding_satoshis,
-			&funding_inputs,
+			&self.funding_negotiation_context,
 			None,
 			&shared_funding_output.script_pubkey,
 			&funding_outputs,
-			self.funding_negotiation_context.funding_feerate_sat_per_1000_weight,
 			change_script.minimal_non_dust().to_sat(),
 		)?;
 		if let Some(change_value) = change_value_opt {
@@ -2824,6 +2817,9 @@ where
 			}
 		}
 
+		let mut funding_inputs = Vec::new();
+		mem::swap(&mut self.funding_negotiation_context.our_funding_inputs, &mut funding_inputs);
+
 		let constructor_args = InteractiveTxConstructorArgs {
 			entropy_source,
 			holder_node_id,
@@ -2832,7 +2828,7 @@ where
 			feerate_sat_per_kw: self
 				.funding_negotiation_context
 				.funding_feerate_sat_per_1000_weight,
-			is_initiator,
+			is_initiator: self.funding_negotiation_context.is_initiator,
 			funding_tx_locktime: self.funding_negotiation_context.funding_tx_locktime,
 			inputs_to_contribute: funding_inputs,
 			shared_funding_input,
@@ -5852,6 +5848,8 @@ fn check_v2_funding_inputs_sufficient(
 
 /// Context for negotiating channels (dual-funded V2 open, splicing)
 pub(super) struct FundingNegotiationContext {
+	/// Whether we initiated the funding negotiation.
+	pub is_initiator: bool,
 	/// The amount in satoshis we will be contributing to the channel.
 	pub our_funding_satoshis: u64,
 	/// The amount in satoshis our counterparty will be contributing to the channel.
@@ -11869,6 +11867,7 @@ where
 			holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx),
 		};
 		let funding_negotiation_context = FundingNegotiationContext {
+			is_initiator: true,
 			our_funding_satoshis: funding_satoshis,
 			// TODO(dual_funding) TODO(splicing) Include counterparty contribution, once that's enabled
 			their_funding_satoshis: None,
@@ -12029,6 +12028,7 @@ where
 		context.channel_id = channel_id;
 
 		let funding_negotiation_context = FundingNegotiationContext {
+			is_initiator: false,
 			our_funding_satoshis: our_funding_satoshis,
 			their_funding_satoshis: Some(msg.common_fields.funding_satoshis),
 			funding_tx_locktime: LockTime::from_consensus(msg.locktime),

lightning/src/ln/interactivetxs.rs

@@ -22,7 +22,7 @@ use bitcoin::{OutPoint, ScriptBuf, Sequence, Transaction, TxIn, TxOut, Txid, Wei
 use crate::chain::chaininterface::fee_for_weight;
 use crate::events::bump_transaction::{BASE_INPUT_WEIGHT, EMPTY_SCRIPT_SIG_WEIGHT};
 use crate::ln::chan_utils::FUNDING_TRANSACTION_WITNESS_WEIGHT;
-use crate::ln::channel::TOTAL_BITCOIN_SUPPLY_SATOSHIS;
+use crate::ln::channel::{FundingNegotiationContext, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
 use crate::ln::msgs;
 use crate::ln::msgs::{MessageSendEvent, SerialId, TxSignatures};
 use crate::ln::types::ChannelId;
@@ -1874,26 +1874,21 @@ impl InteractiveTxConstructor {
 ///   `Err(AbortReason::InsufficientFees)`
 ///
 /// Parameters:
-/// - `is_initiator` - Whether we are the negotiation initiator or not (acceptor).
-/// - `our_contribution` - The sats amount we intend to contribute to the funding
-///   transaction being negotiated.
-/// - `funding_inputs` - List of our inputs. It does not include the shared input, if there is one.
+/// - `context` - Context of the funding negotiation, including non-shared inputs and feerate.
 /// - `shared_input` - The locally owned amount of the shared input (in sats), if there is one.
 /// - `shared_output_funding_script` - The script of the shared output.
 /// - `funding_outputs` - Our funding outputs.
-/// - `funding_feerate_sat_per_1000_weight` - Fee rate to be used.
 /// - `change_output_dust_limit` - The dust limit (in sats) to consider.
 pub(super) fn calculate_change_output_value(
-	is_initiator: bool, our_contribution: u64,
-	funding_inputs: &Vec<(TxIn, TransactionU16LenLimited)>, shared_input: Option<u64>,
+	context: &FundingNegotiationContext, shared_input: Option<u64>,
 	shared_output_funding_script: &ScriptBuf, funding_outputs: &Vec<TxOut>,
-	funding_feerate_sat_per_1000_weight: u32, change_output_dust_limit: u64,
+	change_output_dust_limit: u64,
 ) -> Result<Option<u64>, AbortReason> {
 	// 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() {
+	for (txin, tx) in context.our_funding_inputs.iter() {
 		let txid = tx.as_transaction().compute_txid();
 		if txin.previous_output.txid != txid {
 			return Err(AbortReason::PrevTxOutInvalid);
@@ -1922,7 +1917,7 @@ pub(super) fn calculate_change_output_value(
 
 	// If we are the initiator, we must pay for the weight of the funding output and
 	// all common fields in the funding transaction.
-	if is_initiator {
+	if context.is_initiator {
 		weight = weight.saturating_add(get_output_weight(shared_output_funding_script).to_wu());
 		weight = weight.saturating_add(TX_COMMON_FIELDS_WEIGHT);
 
@@ -1931,15 +1926,15 @@ pub(super) fn calculate_change_output_value(
 		}
 	}
 
-	let fees_sats = fee_for_weight(funding_feerate_sat_per_1000_weight, weight);
+	let fees_sats = fee_for_weight(context.funding_feerate_sat_per_1000_weight, weight);
 
 	let net_total_less_fees =
 		total_input_satoshis.saturating_sub(total_output_satoshis).saturating_sub(fees_sats);
-	if net_total_less_fees < our_contribution {
+	if net_total_less_fees < context.our_funding_satoshis {
 		// Not enough to cover contribution plus fees
 		return Err(AbortReason::InsufficientFees);
 	}
-	let remaining_value = net_total_less_fees.saturating_sub(our_contribution);
+	let remaining_value = net_total_less_fees.saturating_sub(context.our_funding_satoshis);
 	if remaining_value < change_output_dust_limit {
 		// Enough to cover contribution plus fees, but leftover is below dust limit; no change
 		Ok(None)
@@ -1952,7 +1947,7 @@ pub(super) fn calculate_change_output_value(
 #[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::channel::{FundingNegotiationContext, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
 	use crate::ln::interactivetxs::{
 		calculate_change_output_value, generate_holder_serial_id, AbortReason,
 		HandleTxCompleteValue, InteractiveTxConstructor, InteractiveTxConstructorArgs,
@@ -2980,89 +2975,74 @@ mod tests {
 		let gross_change = total_inputs - total_outputs - our_contributed;
 		let fees = 1746;
 		let common_fees = 234;
-		{
-			// There is leftover for change
-			let res = calculate_change_output_value(
-				true,
-				our_contributed,
-				&inputs,
-				None,
-				&ScriptBuf::new(),
-				&outputs,
-				funding_feerate_sat_per_1000_weight,
-				300,
-			);
-			assert_eq!(res, Ok(Some(gross_change - fees - common_fees)));
-		}
-		{
-			// There is leftover for change, without common fees
-			let res = calculate_change_output_value(
-				false,
-				our_contributed,
-				&inputs,
-				None,
-				&ScriptBuf::new(),
-				&outputs,
-				funding_feerate_sat_per_1000_weight,
-				300,
-			);
-			assert_eq!(res, Ok(Some(gross_change - fees)));
-		}
-		{
-			// Larger fee, smaller change
-			let res = calculate_change_output_value(
-				true,
-				our_contributed,
-				&inputs,
-				None,
-				&ScriptBuf::new(),
-				&outputs,
-				funding_feerate_sat_per_1000_weight * 3,
-				300,
-			);
-			assert_eq!(res, Ok(Some(4060)));
-		}
-		{
-			// Insufficient inputs, no leftover
-			let res = calculate_change_output_value(
-				false,
-				130_000,
-				&inputs,
-				None,
-				&ScriptBuf::new(),
-				&outputs,
-				funding_feerate_sat_per_1000_weight,
-				300,
-			);
-			assert_eq!(res, Err(AbortReason::InsufficientFees));
-		}
-		{
-			// Very small leftover
-			let res = calculate_change_output_value(
-				false,
-				118_000,
-				&inputs,
-				None,
-				&ScriptBuf::new(),
-				&outputs,
-				funding_feerate_sat_per_1000_weight,
-				300,
-			);
-			assert_eq!(res, Ok(None));
-		}
-		{
-			// Small leftover, but not dust
-			let res = calculate_change_output_value(
-				false,
-				117_992,
-				&inputs,
-				None,
-				&ScriptBuf::new(),
-				&outputs,
-				funding_feerate_sat_per_1000_weight,
-				100,
-			);
-			assert_eq!(res, Ok(Some(262)));
-		}
+
+		// There is leftover for change
+		let context = FundingNegotiationContext {
+			is_initiator: true,
+			our_funding_satoshis: our_contributed,
+			their_funding_satoshis: None,
+			funding_tx_locktime: AbsoluteLockTime::ZERO,
+			funding_feerate_sat_per_1000_weight,
+			our_funding_inputs: inputs,
+		};
+		assert_eq!(
+			calculate_change_output_value(&context, None, &ScriptBuf::new(), &outputs, 300),
+			Ok(Some(gross_change - fees - common_fees)),
+		);
+
+		// There is leftover for change, without common fees
+		let context = FundingNegotiationContext {
+			is_initiator: false,
+			..context
+		};
+		assert_eq!(
+			calculate_change_output_value(&context, None, &ScriptBuf::new(), &outputs, 300),
+			Ok(Some(gross_change - fees)),
+		);
+
+		// Larger fee, smaller change
+		let context = FundingNegotiationContext {
+			is_initiator: true,
+			funding_feerate_sat_per_1000_weight: funding_feerate_sat_per_1000_weight * 3,
+			..context
+		};
+		assert_eq!(
+			calculate_change_output_value(&context, None, &ScriptBuf::new(), &outputs, 300),
+			Ok(Some(4060)),
+		);
+
+		// Insufficient inputs, no leftover
+		let context = FundingNegotiationContext {
+			is_initiator: false,
+			our_funding_satoshis: 130_000,
+			funding_feerate_sat_per_1000_weight: funding_feerate_sat_per_1000_weight,
+			..context
+		};
+		assert_eq!(
+			calculate_change_output_value(&context, None, &ScriptBuf::new(), &outputs, 300),
+			Err(AbortReason::InsufficientFees),
+		);
+
+		// Very small leftover
+		let context = FundingNegotiationContext {
+			is_initiator: false,
+			our_funding_satoshis: 118_000,
+			..context
+		};
+		assert_eq!(
+			calculate_change_output_value(&context, None, &ScriptBuf::new(), &outputs, 300),
+			Ok(None),
+		);
+
+		// Small leftover, but not dust
+		let context = FundingNegotiationContext {
+			is_initiator: false,
+			our_funding_satoshis: 117_992,
+			..context
+		};
+		assert_eq!(
+			calculate_change_output_value(&context, None, &ScriptBuf::new(), &outputs, 100),
+			Ok(Some(262)),
+		);
 	}
 }