Split in two, add unit test

Author: optout21

lightning/src/ln/channel.rs

@@ -1748,7 +1748,7 @@ pub(super) trait InteractivelyFunded<SP: Deref> where SP::Target: SignerProvider
 
 		maybe_add_funding_change_output(signer_provider, self.is_initiator(), self.dual_funding_context().our_funding_satoshis,
 			&funding_inputs_prev_outputs, &mut funding_outputs, self.dual_funding_context().funding_feerate_sat_per_1000_weight,
-			total_input_satoshis, self.context().holder_dust_limit_satoshis, self.context().channel_keys_id).map_err(
+			self.context().holder_dust_limit_satoshis, self.context().channel_keys_id).map_err(
 				|_| APIError::APIMisuseError { err: "Could not create change output".to_string() })?;
 
 		let constructor_args = InteractiveTxConstructorArgs {
@@ -4253,13 +4253,11 @@ fn get_v2_channel_reserve_satoshis(channel_value_satoshis: u64, dust_limit_satos
 }
 
 #[allow(dead_code)] // TODO(dual_funding): Remove once begin_interactive_funding_tx_construction() is used
-pub(super) fn maybe_add_funding_change_output<SP: Deref>(signer_provider: &SP, is_initiator: bool,
+fn need_to_add_funding_change_output(is_initiator: bool,
 	our_funding_satoshis: u64, funding_inputs_prev_outputs: &Vec<TxOut>,
-	funding_outputs: &mut Vec<OutputOwned>, funding_feerate_sat_per_1000_weight: u32,
-	total_input_satoshis: u64, holder_dust_limit_satoshis: u64, channel_keys_id: [u8; 32],
-) -> Result<Option<TxOut>, ChannelError> where
-	SP::Target: SignerProvider,
-{
+	funding_outputs: &Vec<OutputOwned>, funding_feerate_sat_per_1000_weight: u32,
+	holder_dust_limit_satoshis: u64,
+) -> Result<Option<u64>, ChannelError> {
 	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())
 	});
@@ -4275,32 +4273,54 @@ pub(super) fn maybe_add_funding_change_output<SP: Deref>(signer_provider: &SP, i
 		fees_sats = fees_sats.saturating_add(common_fees);
 	}
 
+	let total_input_satoshis: u64 = funding_inputs_prev_outputs.iter().map(|out| out.value.to_sat()).sum();
+
 	let remaining_value = total_input_satoshis
 		.saturating_sub(our_funding_satoshis)
 		.saturating_sub(fees_sats);
 
 	if remaining_value < holder_dust_limit_satoshis {
 		Ok(None)
 	} else {
-		let change_script = signer_provider.get_destination_script(channel_keys_id).map_err(
-			|_| ChannelError::Close((
-				"Failed to get change script as new destination script".to_owned(),
-				ClosureReason::ProcessingError { err: "Failed to get change script as new destination script".to_owned() }
-			))
-		)?;
-		let mut change_output = TxOut {
-			value: Amount::from_sat(remaining_value),
-			script_pubkey: change_script,
-		};
-		let change_output_weight = get_output_weight(&change_output.script_pubkey).to_wu();
-
-		let change_output_fee = fee_for_weight(funding_feerate_sat_per_1000_weight, change_output_weight);
-		change_output.value = Amount::from_sat(remaining_value.saturating_sub(change_output_fee));
-		funding_outputs.push(OutputOwned::Single(change_output.clone()));
-		Ok(Some(change_output))
+		Ok(Some(remaining_value))
 	}
 }
 
+#[allow(dead_code)] // TODO(dual_funding): Remove once begin_interactive_funding_tx_construction() is used
+fn maybe_add_funding_change_output<SP: Deref>(signer_provider: &SP, is_initiator: bool,
+	our_funding_satoshis: u64, funding_inputs_prev_outputs: &Vec<TxOut>,
+	funding_outputs: &mut Vec<OutputOwned>, funding_feerate_sat_per_1000_weight: u32,
+	holder_dust_limit_satoshis: u64, channel_keys_id: [u8; 32],
+) -> Result<Option<TxOut>, ChannelError> where SP::Target: SignerProvider {
+	let remaining_value = match need_to_add_funding_change_output(
+		is_initiator, our_funding_satoshis, funding_inputs_prev_outputs,
+		funding_outputs, funding_feerate_sat_per_1000_weight, holder_dust_limit_satoshis
+	)? {
+		None => {
+			// No need to add
+			return Ok(None);
+		}
+		Some(remaining_value) => remaining_value,
+	};
+
+	let change_script = signer_provider.get_destination_script(channel_keys_id).map_err(
+		|_| ChannelError::Close((
+			"Failed to get change script as new destination script".to_owned(),
+			ClosureReason::ProcessingError { err: "Failed to get change script as new destination script".to_owned() }
+		))
+	)?;
+	let mut change_output = TxOut {
+		value: Amount::from_sat(remaining_value),
+		script_pubkey: change_script,
+	};
+	let change_output_weight = get_output_weight(&change_output.script_pubkey).to_wu();
+
+	let change_output_fee = fee_for_weight(funding_feerate_sat_per_1000_weight, change_output_weight);
+	change_output.value = Amount::from_sat(remaining_value.saturating_sub(change_output_fee));
+	funding_outputs.push(OutputOwned::Single(change_output.clone()));
+	Ok(Some(change_output))
+}
+
 pub(super) fn calculate_our_funding_satoshis(
 	is_initiator: bool, funding_inputs: &[(TxIn, TransactionU16LenLimited)],
 	total_witness_weight: Weight, funding_feerate_sat_per_1000_weight: u32,
@@ -10307,8 +10327,9 @@ mod tests {
 	use crate::ln::channel_keys::{RevocationKey, RevocationBasepoint};
 	use crate::ln::channelmanager::{self, HTLCSource, PaymentId};
 	use crate::ln::channel::InitFeatures;
-	use crate::ln::channel::{AwaitingChannelReadyFlags, Channel, ChannelState, InboundHTLCOutput, OutboundV1Channel, InboundV1Channel, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator, HTLCUpdateAwaitingACK, commit_tx_fee_sat};
+	use crate::ln::channel::{AwaitingChannelReadyFlags, Channel, ChannelState, InboundHTLCOutput, OutboundV1Channel, InboundV1Channel, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator, HTLCUpdateAwaitingACK, commit_tx_fee_sat, need_to_add_funding_change_output};
 	use crate::ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS, MIN_THEIR_CHAN_RESERVE_SATOSHIS};
+	use crate::ln::interactivetxs::{OutputOwned, SharedOwnedOutput};
 	use crate::types::features::{ChannelFeatures, ChannelTypeFeatures, NodeFeatures};
 	use crate::ln::msgs;
 	use crate::ln::msgs::{ChannelUpdate, DecodeError, UnsignedChannelUpdate, MAX_VALUE_MSAT};
@@ -12071,4 +12092,46 @@ mod tests {
 		assert_eq!(node_a_chan.context.channel_state, ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::THEIR_CHANNEL_READY));
 		assert!(node_a_chan.check_get_channel_ready(0, &&logger).is_some());
 	}
+
+	#[test]
+	fn test_need_to_add_funding_change_output() {
+		let prevouts = vec![
+			TxOut { value: Amount::from_sat(70_000), script_pubkey: ScriptBuf::new()},
+			TxOut { value: Amount::from_sat(60_000), script_pubkey: ScriptBuf::new()},
+		];
+		let txout = TxOut { value: Amount::from_sat(125_000), script_pubkey: ScriptBuf::new()};
+		let outputs = vec![OutputOwned::Shared(SharedOwnedOutput::new(txout, 105_000))];
+		let our_contributed = 110_000;
+		let funding_feerate_sat_per_1000_weight = 3000;
+
+		let total_inputs: u64 = prevouts.iter().map(|o| o.value.to_sat()).sum();
+		let gross_change = total_inputs - our_contributed;
+		let fees = 1746;
+		let common_fees = 126;
+		{
+			// There is leftover for change
+			let res = need_to_add_funding_change_output(true, our_contributed, &prevouts, &outputs, funding_feerate_sat_per_1000_weight, 300); 
+			assert_eq!(res.unwrap().unwrap(), gross_change - fees - common_fees);
+		}
+		{
+			// There is leftover for change, without common fees
+			let res = need_to_add_funding_change_output(false, our_contributed, &prevouts, &outputs, funding_feerate_sat_per_1000_weight, 300); 
+			assert_eq!(res.unwrap().unwrap(), gross_change - fees);
+		}
+		{
+			// Insufficient inputs, no leftover
+			let res = need_to_add_funding_change_output(false, 130_000, &prevouts, &outputs, funding_feerate_sat_per_1000_weight, 300); 
+			assert!(res.unwrap().is_none());
+		}
+		{
+			// Very small leftover
+			let res = need_to_add_funding_change_output(false, 128_100, &prevouts, &outputs, funding_feerate_sat_per_1000_weight, 300); 
+			assert!(res.unwrap().is_none());
+		}
+		{
+			// Small leftover, but not dust
+			let res = need_to_add_funding_change_output(false, 128_100, &prevouts, &outputs, funding_feerate_sat_per_1000_weight, 100); 
+			assert_eq!(res.unwrap().unwrap(), 154);
+		}
+	}
 }