Reorg return values of ChannelContext::build_commitment_transaction

We choose to include in `CommitmentStats` only fields that will be
calculated or constructed by transaction builders.

The other fields are created by channel, and placed in a new struct
we call `CommitmentData`.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -877,15 +877,20 @@ struct HTLCStats {
 	on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included
 }
 
-/// An enum gathering stats on commitment transaction, either local or remote.
-struct CommitmentStats<'a> {
+/// A struct gathering data on a commitment, either local or remote.
+struct CommitmentData<'a> {
+	stats: CommitmentStats,
+	htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction
+	outbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful offered HTLCs since last commitment
+	inbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful received HTLCs since last commitment
+}
+
+/// A struct gathering stats on a commitment transaction, either local or remote.
+struct CommitmentStats {
 	tx: CommitmentTransaction, // the transaction info
 	total_fee_sat: u64, // the total fee included in the transaction
-	htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction
 	local_balance_before_fee_msat: u64, // local balance before fees *not* considering dust limits
 	remote_balance_before_fee_msat: u64, // remote balance before fees *not* considering dust limits
-	outbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful offered HTLCs since last commitment
-	inbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful received HTLCs since last commitment
 }
 
 /// Used when calculating whether we or the remote can afford an additional HTLC.
@@ -2043,7 +2048,10 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
 	) -> Result<CommitmentTransaction, ChannelError> where L::Target: Logger {
 		let funding_script = self.funding().get_funding_redeemscript();
 
-		let initial_commitment_tx = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger).tx;
+		let commitment_data = self.context().build_commitment_transaction(self.funding(),
+			holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
+			true, false, logger);
+		let initial_commitment_tx = commitment_data.stats.tx;
 		let trusted_tx = initial_commitment_tx.trust();
 		let initial_commitment_bitcoin_tx = trusted_tx.built_transaction();
 		let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.funding().get_value_satoshis());
@@ -2080,7 +2088,10 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
 			}
 		};
 		let context = self.context();
-		let counterparty_initial_commitment_tx = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
+		let commitment_data = context.build_commitment_transaction(self.funding(),
+			context.cur_counterparty_commitment_transaction_number,
+			&context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+		let counterparty_initial_commitment_tx = commitment_data.stats.tx;
 		let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
 		let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();
 
@@ -3477,9 +3488,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 	{
 		let funding_script = funding.get_funding_redeemscript();
 
-		let commitment_stats = self.build_commitment_transaction(funding, holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger);
+		let commitment_data = self.build_commitment_transaction(funding,
+			holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
+			true, false, logger);
 		let commitment_txid = {
-			let trusted_tx = commitment_stats.tx.trust();
+			let trusted_tx = commitment_data.stats.tx.trust();
 			let bitcoin_tx = trusted_tx.built_transaction();
 			let sighash = bitcoin_tx.get_sighash_all(&funding_script, funding.get_value_satoshis());
 
@@ -3492,7 +3505,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			}
 			bitcoin_tx.txid
 		};
-		let mut htlcs_cloned: Vec<_> = commitment_stats.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect();
+		let mut htlcs_cloned: Vec<_> = commitment_data.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect();
 
 		// If our counterparty updated the channel fee in this commitment transaction, check that
 		// they can actually afford the new fee now.
@@ -3502,7 +3515,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		if update_fee {
 			debug_assert!(!funding.is_outbound());
 			let counterparty_reserve_we_require_msat = funding.holder_selected_channel_reserve_satoshis * 1000;
-			if commitment_stats.remote_balance_before_fee_msat < commitment_stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat {
+			if commitment_data.stats.remote_balance_before_fee_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat {
 				return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned()));
 			}
 		}
@@ -3518,14 +3531,14 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 						&& info.next_holder_htlc_id == self.next_holder_htlc_id
 						&& info.next_counterparty_htlc_id == self.next_counterparty_htlc_id
 						&& info.feerate == self.feerate_per_kw {
-							assert_eq!(commitment_stats.total_fee_sat, info.fee / 1000);
+							assert_eq!(commitment_data.stats.total_fee_sat, info.fee / 1000);
 						}
 				}
 			}
 		}
 
-		if msg.htlc_signatures.len() != commitment_stats.tx.htlcs().len() {
-			return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.tx.htlcs().len())));
+		if msg.htlc_signatures.len() != commitment_data.stats.tx.htlcs().len() {
+			return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.stats.tx.htlcs().len())));
 		}
 
 		// Up to LDK 0.0.115, HTLC information was required to be duplicated in the
@@ -3545,10 +3558,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 
 		let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len());
 		let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len());
-		let holder_keys = commitment_stats.tx.trust().keys();
+		let holder_keys = commitment_data.stats.tx.trust().keys();
 		for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() {
 			if let Some(_) = htlc.transaction_output_index {
-				let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.tx.feerate_per_kw(),
+				let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.stats.tx.feerate_per_kw(),
 					funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.channel_type,
 					&holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key);
 
@@ -3576,14 +3589,14 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		}
 
 		let holder_commitment_tx = HolderCommitmentTransaction::new(
-			commitment_stats.tx,
+			commitment_data.stats.tx,
 			msg.signature,
 			msg.htlc_signatures.clone(),
 			&funding.get_holder_pubkeys().funding_pubkey,
 			funding.counterparty_funding_pubkey()
 		);
 
-		self.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages)
+		self.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_data.outbound_htlc_preimages)
 			.map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?;
 
 		Ok(LatestHolderCommitmentTXInfo {
@@ -3607,7 +3620,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 	/// generated by the peer which proposed adding the HTLCs, and thus we need to understand both
 	/// which peer generated this transaction and "to whom" this transaction flows.
 	#[inline]
-	fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats
+	fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData
 		where L::Target: Logger
 	{
 		let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
@@ -3823,12 +3836,16 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
 		htlcs_included.append(&mut included_dust_htlcs);
 
-		CommitmentStats {
+		let stats = CommitmentStats {
 			tx,
 			total_fee_sat,
-			htlcs_included,
 			local_balance_before_fee_msat: value_to_self_msat,
 			remote_balance_before_fee_msat: value_to_remote_msat,
+		};
+
+		CommitmentData {
+			stats,
+			htlcs_included,
 			inbound_htlc_preimages,
 			outbound_htlc_preimages,
 		}
@@ -4616,8 +4633,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		SP::Target: SignerProvider,
 		L::Target: Logger
 	{
-		let counterparty_initial_commitment_tx = self.build_commitment_transaction(
-			funding, self.cur_counterparty_commitment_transaction_number, &self.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
+		let commitment_data = self.build_commitment_transaction(funding,
+			self.cur_counterparty_commitment_transaction_number,
+			&self.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+		let counterparty_initial_commitment_tx = commitment_data.stats.tx;
 		match self.holder_signer {
 			// TODO (taproot|arik): move match into calling method for Taproot
 			ChannelSignerType::Ecdsa(ref ecdsa) => {
@@ -6330,9 +6349,11 @@ impl<SP: Deref> FundedChannel<SP> where
 		// Before proposing a feerate update, check that we can actually afford the new fee.
 		let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator);
 		let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate);
-		let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, true, logger);
-		let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000;
-		let holder_balance_msat = commitment_stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
+		let commitment_data = self.context.build_commitment_transaction(&self.funding,
+			self.holder_commitment_point.transaction_number(),
+			&self.holder_commitment_point.current_point(), true, true, logger);
+		let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000;
+		let holder_balance_msat = commitment_data.stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
 		if holder_balance_msat < buffer_fee_msat  + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
 			//TODO: auto-close after a number of failures?
 			log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw);
@@ -6643,7 +6664,10 @@ impl<SP: Deref> FundedChannel<SP> where
 			self.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger);
 		}
 		let funding_signed = if self.context.signer_pending_funding && !self.funding.is_outbound() {
-			let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number + 1, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
+			let commitment_data = self.context.build_commitment_transaction(&self.funding,
+				self.context.cur_counterparty_commitment_transaction_number + 1,
+				&self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+			let counterparty_initial_commitment_tx = commitment_data.stats.tx;
 			self.context.get_funding_signed_msg(&self.funding.channel_transaction_parameters, logger, counterparty_initial_commitment_tx)
 		} else { None };
 		// Provide a `channel_ready` message if we need to, but only if we're _not_ still pending
@@ -8716,8 +8740,10 @@ impl<SP: Deref> FundedChannel<SP> where
 	-> (Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, CommitmentTransaction)
 	where L::Target: Logger
 	{
-		let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
-		let counterparty_commitment_tx = commitment_stats.tx;
+		let commitment_data = self.context.build_commitment_transaction(&self.funding,
+			self.context.cur_counterparty_commitment_transaction_number,
+			&self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
+		let counterparty_commitment_tx = commitment_data.stats.tx;
 
 		#[cfg(any(test, fuzzing))]
 		{
@@ -8737,7 +8763,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			}
 		}
 
-		(commitment_stats.htlcs_included, counterparty_commitment_tx)
+		(commitment_data.htlcs_included, counterparty_commitment_tx)
 	}
 
 	/// Only fails in case of signer rejection. Used for channel_reestablish commitment_signed
@@ -8747,8 +8773,10 @@ impl<SP: Deref> FundedChannel<SP> where
 		#[cfg(any(test, fuzzing))]
 		self.build_commitment_no_state_update(logger);
 
-		let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
-		let counterparty_commitment_txid = commitment_stats.tx.trust().txid();
+		let commitment_data = self.context.build_commitment_transaction(&self.funding,
+			self.context.cur_counterparty_commitment_transaction_number,
+			&self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger);
+		let counterparty_commitment_tx = commitment_data.stats.tx;
 
 		match &self.context.holder_signer {
 			ChannelSignerType::Ecdsa(ecdsa) => {
@@ -8757,24 +8785,25 @@ impl<SP: Deref> FundedChannel<SP> where
 				{
 					let res = ecdsa.sign_counterparty_commitment(
 							&self.funding.channel_transaction_parameters,
-							&commitment_stats.tx,
-							commitment_stats.inbound_htlc_preimages,
-							commitment_stats.outbound_htlc_preimages,
+							&counterparty_commitment_tx,
+							commitment_data.inbound_htlc_preimages,
+							commitment_data.outbound_htlc_preimages,
 							&self.context.secp_ctx,
 						).map_err(|_| ChannelError::Ignore("Failed to get signatures for new commitment_signed".to_owned()))?;
 					signature = res.0;
 					htlc_signatures = res.1;
 
+					let trusted_tx = counterparty_commitment_tx.trust();
 					log_trace!(logger, "Signed remote commitment tx {} (txid {}) with redeemscript {} -> {} in channel {}",
-						encode::serialize_hex(&commitment_stats.tx.trust().built_transaction().transaction),
-						&counterparty_commitment_txid, encode::serialize_hex(&self.funding.get_funding_redeemscript()),
+						encode::serialize_hex(&trusted_tx.built_transaction().transaction),
+						&trusted_tx.txid(), encode::serialize_hex(&self.funding.get_funding_redeemscript()),
 						log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id());
 
-					let counterparty_keys = commitment_stats.tx.trust().keys();
-					debug_assert_eq!(htlc_signatures.len(), commitment_stats.tx.htlcs().len());
-					for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(commitment_stats.tx.htlcs()) {
+					let counterparty_keys = trusted_tx.keys();
+					debug_assert_eq!(htlc_signatures.len(), trusted_tx.htlcs().len());
+					for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.htlcs()) {
 						log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}",
-							encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.tx.feerate_per_kw(), self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
+							encode::serialize_hex(&chan_utils::build_htlc_transaction(&trusted_tx.txid(), trusted_tx.feerate_per_kw(), self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
 							encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &counterparty_keys)),
 							log_bytes!(counterparty_keys.broadcaster_htlc_key.to_public_key().serialize()),
 							log_bytes!(htlc_sig.serialize_compact()[..]), &self.context.channel_id());
@@ -9228,7 +9257,10 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 
 	/// Only allowed after [`FundingScope::channel_transaction_parameters`] is set.
 	fn get_funding_created_msg<L: Deref>(&mut self, logger: &L) -> Option<msgs::FundingCreated> where L::Target: Logger {
-		let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx;
+		let commitment_data = self.context.build_commitment_transaction(&self.funding,
+			self.context.cur_counterparty_commitment_transaction_number,
+			&self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+		let counterparty_initial_commitment_tx = commitment_data.stats.tx;
 		let signature = match &self.context.holder_signer {
 			// TODO (taproot|arik): move match into calling method for Taproot
 			ChannelSignerType::Ecdsa(ecdsa) => {
@@ -11879,8 +11911,9 @@ mod tests {
 			( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $opt_anchors: expr, {
 				$( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), *
 			} ) => { {
-				let commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger);
-				let commitment_tx = commitment_stats.tx;
+				let commitment_data = chan.context.build_commitment_transaction(&chan.funding,
+					0xffffffffffff - 42, &per_commitment_point, true, false, &logger);
+				let commitment_tx = commitment_data.stats.tx;
 				let trusted_tx = commitment_tx.trust();
 				let unsigned_tx = trusted_tx.built_transaction();
 				let redeemscript = chan.funding.get_funding_redeemscript();