Remove exclusive reference and generic from CommitmentTransaction API

This commit reworks how channel is told about which HTLCs were assigned
which index upon the build of a `CommitmentTransaction`.

Previously, `CommitmentTransaction` was given an exclusive reference to
channel's HTLC-source table, and `CommitmentTransaction` populated the
transaction output indices in that table via this exclusive reference.

As a result, the public API of `CommitmentTransaction` included a
generic parameter, and an exclusive reference.

We remove both of these in preparation for the upcoming `TxBuilder`
trait. This cleans up the API, and makes it more bindings-friendly.

Henceforth, channel populates the HTLC-source table via a brute-force
search of each htlc in `CommitmentTransaction`. This is an O(n^2)
operation, but n is small enough that we ignore the performance hit.

We also take this opportunity to cleanup how we build and sort the
commitment transaction outputs together with the htlc output data
in `CommitmentTransaction`. The goal is to keep the number of vector
allocations to a minimum; we now only allocate a single vector to hold
the transaction outputs, and do all the sorting in-place.

Author: tankyleo

lightning/src/chain/channelmonitor.rs

@@ -3588,15 +3588,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					to_broadcaster_value,
 					to_countersignatory_value,
 				)| {
-					let htlc_outputs = vec![];
+					let nondust_htlcs = vec![];
 
 					let commitment_tx = self.build_counterparty_commitment_tx(
 						INITIAL_COMMITMENT_NUMBER,
 						&their_per_commitment_point,
 						to_broadcaster_value,
 						to_countersignatory_value,
 						feerate_per_kw,
-						htlc_outputs,
+						nondust_htlcs,
 					);
 					// Take the opportunity to populate this recently introduced field
 					self.initial_counterparty_commitment_tx = Some(commitment_tx.clone());
@@ -3609,11 +3609,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	fn build_counterparty_commitment_tx(
 		&self, commitment_number: u64, their_per_commitment_point: &PublicKey,
 		to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32,
-		mut nondust_htlcs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>
+		nondust_htlcs: Vec<HTLCOutputInCommitment>
 	) -> CommitmentTransaction {
 		let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable();
-		CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, their_per_commitment_point,
-			to_broadcaster_value, to_countersignatory_value, feerate_per_kw, &mut nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx)
+		CommitmentTransaction::new(commitment_number, their_per_commitment_point,
+			to_broadcaster_value, to_countersignatory_value, feerate_per_kw, nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx)
 	}
 
 	fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec<CommitmentTransaction> {
@@ -3629,7 +3629,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					to_countersignatory_value_sat: Some(to_countersignatory_value) } => {
 
 					let nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| {
-						htlc.transaction_output_index.map(|_| (htlc.clone(), None))
+						htlc.transaction_output_index.map(|_| htlc).cloned()
 					}).collect::<Vec<_>>();
 
 					let commitment_tx = self.build_counterparty_commitment_tx(commitment_number,
@@ -5620,21 +5620,21 @@ mod tests {
 				{
 					let mut res = Vec::new();
 					for (idx, preimage) in $preimages_slice.iter().enumerate() {
-						res.push((HTLCOutputInCommitment {
+						res.push(HTLCOutputInCommitment {
 							offered: true,
 							amount_msat: 0,
 							cltv_expiry: 0,
 							payment_hash: preimage.1.clone(),
 							transaction_output_index: Some(idx as u32),
-						}, ()));
+						});
 					}
 					res
 				}
 			}
 		}
 		macro_rules! preimages_slice_to_htlc_outputs {
 			($preimages_slice: expr) => {
-				preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|(htlc, _)| (htlc, None)).collect()
+				preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|htlc| (htlc, None)).collect()
 			}
 		}
 		let dummy_sig = crate::crypto::utils::sign(&secp_ctx,
@@ -5690,15 +5690,17 @@ mod tests {
 		let best_block = BestBlock::from_network(Network::Testnet);
 		let monitor = ChannelMonitor::new(
 			Secp256k1::new(), keys, Some(shutdown_script.into_inner()), 0, &ScriptBuf::new(),
-			&channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &mut Vec::new()),
+			&channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, Vec::new()),
 			best_block, dummy_key, channel_id,
 		);
 
-		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..10]);
-		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
+		let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..10]);
+		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, nondust_htlcs);
+		// These HTLCs now have their output indices assigned
+		let nondust_htlcs = dummy_commitment_tx.nondust_htlcs();
 
 		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
-			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect());
+			nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect());
 		monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"1").to_byte_array()),
 			preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
 		monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"2").to_byte_array()),
@@ -5733,21 +5735,25 @@ mod tests {
 
 		// Now update holder commitment tx info, pruning only element 18 as we still care about the
 		// previous commitment tx's preimages too
-		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
-		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
+		let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
+		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, nondust_htlcs);
+		// These HTLCs now have their output indices assigned
+		let nondust_htlcs = dummy_commitment_tx.nondust_htlcs();
 		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
-			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect());
+			nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect());
 		secret[0..32].clone_from_slice(&<Vec<u8>>::from_hex("2273e227a5b7449b6e70f1fb4652864038b1cbf9cd7c043a7d6456b7fc275ad8").unwrap());
 		monitor.provide_secret(281474976710653, secret.clone()).unwrap();
 		assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 12);
 		test_preimages_exist!(&preimages[0..10], monitor);
 		test_preimages_exist!(&preimages[18..20], monitor);
 
 		// But if we do it again, we'll prune 5-10
-		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]);
-		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
-		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx,
-			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect());
+		let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..3]);
+		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, nondust_htlcs);
+		// These HTLCs now have their output indices assigned
+		let nondust_htlcs = dummy_commitment_tx.nondust_htlcs();
+		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
+			nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect());
 		secret[0..32].clone_from_slice(&<Vec<u8>>::from_hex("27cddaa5624534cb6cb9d7da077cf2b22ab21e9b506fd4998a51d54502e99116").unwrap());
 		monitor.provide_secret(281474976710652, secret.clone()).unwrap();
 		assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 5);
@@ -5942,7 +5948,7 @@ mod tests {
 		let best_block = BestBlock::from_network(Network::Testnet);
 		let monitor = ChannelMonitor::new(
 			Secp256k1::new(), keys, Some(shutdown_script.into_inner()), 0, &ScriptBuf::new(),
-			&channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &mut Vec::new()),
+			&channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, Vec::new()),
 			best_block, dummy_key, channel_id,
 		);
 

lightning/src/chain/onchaintx.rs

@@ -1296,22 +1296,21 @@ mod tests {
 
 		// Create an OnchainTxHandler for a commitment containing HTLCs with CLTV expiries of 0, 1,
 		// and 2 blocks.
-		let mut htlcs = Vec::new();
+		let mut nondust_htlcs = Vec::new();
 		for i in 0..3 {
 			let preimage = PaymentPreimage([i; 32]);
 			let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array());
-			htlcs.push((
+			nondust_htlcs.push(
 				HTLCOutputInCommitment {
 					offered: true,
 					amount_msat: 10000,
 					cltv_expiry: i as u32,
 					payment_hash: hash,
 					transaction_output_index: Some(i as u32),
-				},
-				(),
-			));
+				}
+			);
 		}
-		let holder_commit = HolderCommitmentTransaction::dummy(1000000, &mut htlcs);
+		let holder_commit = HolderCommitmentTransaction::dummy(1000000, nondust_htlcs);
 		let destination_script = ScriptBuf::new();
 		let mut tx_handler = OnchainTxHandler::new(
 			1000000,

lightning/src/chain/package.rs

@@ -1712,7 +1712,7 @@ mod tests {
 					payment_hash: PaymentHash::from(preimage),
 					transaction_output_index: None,
 				};
-				let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut vec![(htlc.clone(), ())]);
+				let commitment_tx = HolderCommitmentTransaction::dummy(0, vec![htlc.clone()]);
 				let trusted_tx = commitment_tx.trust();
 				PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(
 					HTLCDescriptor {
@@ -1746,7 +1746,7 @@ mod tests {
 					payment_hash: PaymentHash::from(PaymentPreimage([2;32])),
 					transaction_output_index: None,
 				};
-				let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut vec![(htlc.clone(), ())]);
+				let commitment_tx = HolderCommitmentTransaction::dummy(0, vec![htlc.clone()]);
 				let trusted_tx = commitment_tx.trust();
 				PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(
 					HTLCDescriptor {
@@ -1770,7 +1770,7 @@ mod tests {
 
 	macro_rules! dumb_funding_output {
 		() => {{
-			let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut Vec::new());
+			let commitment_tx = HolderCommitmentTransaction::dummy(0, Vec::new());
 			let mut channel_parameters = ChannelTransactionParameters::test_dummy(0);
 			channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key();
 			PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build(