Relax the requirements of building a CommitmentTransaction

Building `CommitmentTransaction` previously required a pointer to a
`Vec<(HTLCOutputInCommitment, T)>`, where each item in the vector
represented a non-dust htlc. This forced the caller to place all the
non-dust htlcs and their auxiliary data in contiguous memory prior to
building a `CommitmentTransaction`.

This requirement was not necessary, so we remove it in this commit.

Instead, we choose to ask for a `Vec<&mut HTLCOutputInCommitment>`,
where each element is an exclusive reference to a non-dust htlc, so that
`CommitmentTranscation` may populate their output indices during the
build process.

Author: tankyleo

lightning/src/chain/channelmonitor.rs

@@ -3488,12 +3488,12 @@ 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<&mut HTLCOutputInCommitment>,
 	) -> CommitmentTransaction {
 		let channel_parameters =
 			&self.onchain_tx_handler.channel_transaction_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> {
@@ -3509,13 +3509,13 @@ 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))
-					}).collect::<Vec<_>>();
+					let mut nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| {
+						htlc.transaction_output_index.map(|_| htlc)
+					}).cloned().collect::<Vec<_>>();
 
 					let commitment_tx = self.build_counterparty_commitment_tx(commitment_number,
 							&their_per_commitment_point, to_broadcaster_value,
-							to_countersignatory_value, feerate_per_kw, nondust_htlcs);
+							to_countersignatory_value, feerate_per_kw, nondust_htlcs.iter_mut().collect());
 
 					debug_assert_eq!(commitment_tx.trust().txid(), commitment_txid);
 
@@ -5381,21 +5381,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,
@@ -5458,7 +5458,7 @@ mod tests {
 		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
 
 		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
-			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect());
+			htlcs.into_iter().map(|htlc| (htlc, Some(dummy_sig), None)).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()),
@@ -5496,7 +5496,7 @@ mod tests {
 		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
 		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
 		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
-			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect());
+			htlcs.into_iter().map(|htlc| (htlc, Some(dummy_sig), None)).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);
@@ -5507,7 +5507,7 @@ mod tests {
 		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), None)).collect());
+			htlcs.into_iter().map(|htlc| (htlc, Some(dummy_sig), None)).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);

lightning/src/chain/onchaintx.rs

@@ -1363,16 +1363,15 @@ mod tests {
 		for i in 0..3 {
 			let preimage = PaymentPreimage([i; 32]);
 			let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array());
-			htlcs.push((
+			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 mut tx_handler = OnchainTxHandler::new(
@@ -1400,7 +1399,7 @@ mod tests {
 		// Request claiming of each HTLC on the holder's commitment, with current block height 1.
 		let holder_commit_txid = tx_handler.get_unsigned_holder_commitment_tx().compute_txid();
 		let mut requests = Vec::new();
-		for (htlc, _) in htlcs {
+		for htlc in htlcs {
 			requests.push(PackageTemplate::build_package(
 				holder_commit_txid,
 				htlc.transaction_output_index.unwrap(),

lightning/src/ln/chan_utils.rs

@@ -1145,7 +1145,7 @@ impl_writeable_tlv_based!(HolderCommitmentTransaction, {
 
 impl HolderCommitmentTransaction {
 	#[cfg(test)]
-	pub fn dummy(channel_value_satoshis: u64, htlcs: &mut Vec<(HTLCOutputInCommitment, ())>) -> Self {
+	pub fn dummy(channel_value_satoshis: u64, nondust_htlcs: &mut Vec<HTLCOutputInCommitment>) -> Self {
 		let secp_ctx = Secp256k1::new();
 		let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let dummy_sig = sign(&secp_ctx, &secp256k1::Message::from_digest([42; 32]), &SecretKey::from_slice(&[42; 32]).unwrap());
@@ -1168,11 +1168,11 @@ impl HolderCommitmentTransaction {
 			channel_value_satoshis,
 		};
 		let mut counterparty_htlc_sigs = Vec::new();
-		for _ in 0..htlcs.len() {
+		for _ in 0..nondust_htlcs.len() {
 			counterparty_htlc_sigs.push(dummy_sig);
 		}
-		let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, &dummy_key.clone(), 0, 0, 0, htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx);
-		htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index);
+		let inner = CommitmentTransaction::new(0, &dummy_key.clone(), 0, 0, 0, nondust_htlcs.iter_mut().collect(), &channel_parameters.as_counterparty_broadcastable(), &secp_ctx);
+		nondust_htlcs.sort_by_key(|htlc| htlc.transaction_output_index);
 		HolderCommitmentTransaction {
 			inner,
 			counterparty_sig: dummy_sig,
@@ -1471,23 +1471,16 @@ impl Readable for CommitmentTransaction {
 }
 
 impl CommitmentTransaction {
-	/// Construct an object of the class while assigning transaction output indices to HTLCs.
+	/// Construct an object of the class while assigning transaction output indices to HTLCs in `nondust_htlcs`.
 	///
-	/// Populates HTLCOutputInCommitment.transaction_output_index in htlcs_with_aux.
-	///
-	/// The generic T allows the caller to match the HTLC output index with auxiliary data.
-	/// This auxiliary data is not stored in this object.
-	///
-	/// Only include HTLCs that are above the dust limit for the channel.
-	///
-	/// This is not exported to bindings users due to the generic though we likely should expose a version without
-	pub fn new_with_auxiliary_htlc_data<T>(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<secp256k1::All>) -> CommitmentTransaction {
+	/// `nondust_htlcs` should only include HTLCs that are above the dust limit for the channel.
+	pub fn new(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, nondust_htlcs: Vec<&mut HTLCOutputInCommitment>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<secp256k1::All>) -> CommitmentTransaction {
 		let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat);
 		let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat);
 		let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);
 
-		// Sort outputs and populate output indices while keeping track of the auxiliary data
-		let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters).unwrap();
+		// Sort outputs and populate output indices
+		let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters).unwrap();
 
 		let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters);
 		let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
@@ -1498,7 +1491,7 @@ impl CommitmentTransaction {
 			to_countersignatory_value_sat,
 			to_broadcaster_delay: Some(channel_parameters.contest_delay()),
 			feerate_per_kw,
-			htlcs,
+			htlcs: nondust_htlcs,
 			channel_type_features: channel_parameters.channel_type_features().clone(),
 			keys,
 			built: BuiltCommitmentTransaction {
@@ -1519,8 +1512,8 @@ impl CommitmentTransaction {
 	fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> Result<BuiltCommitmentTransaction, ()> {
 		let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters);
 
-		let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect();
-		let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters)?;
+		let mut nondust_htlcs = self.htlcs.clone();
+		let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, nondust_htlcs.iter_mut().collect(), channel_parameters)?;
 
 		let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
 		let txid = transaction.compute_txid();
@@ -1544,7 +1537,7 @@ impl CommitmentTransaction {
 	// - initial sorting of outputs / HTLCs in the constructor, in which case T is auxiliary data the
 	//   caller needs to have sorted together with the HTLCs so it can keep track of the output index
 	// - building of a bitcoin transaction during a verify() call, in which case T is just ()
-	fn internal_build_outputs<T>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
+	fn internal_build_outputs(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: Vec<&mut HTLCOutputInCommitment>, channel_parameters: &DirectedChannelTransactionParameters) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
 		let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys();
 		let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey;
 		let countersignatory_funding_key = &countersignatory_pubkeys.funding_pubkey;
@@ -1583,7 +1576,7 @@ impl CommitmentTransaction {
 		}
 
 		if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
-			if to_broadcaster_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() {
+			if to_broadcaster_value_sat > Amount::ZERO || !nondust_htlcs.is_empty() {
 				let anchor_script = get_anchor_redeemscript(broadcaster_funding_key);
 				txouts.push((
 					TxOut {
@@ -1594,7 +1587,7 @@ impl CommitmentTransaction {
 				));
 			}
 
-			if to_countersignatory_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() {
+			if to_countersignatory_value_sat > Amount::ZERO || !nondust_htlcs.is_empty() {
 				let anchor_script = get_anchor_redeemscript(countersignatory_funding_key);
 				txouts.push((
 					TxOut {
@@ -1606,8 +1599,8 @@ impl CommitmentTransaction {
 			}
 		}
 
-		let mut htlcs = Vec::with_capacity(htlcs_with_aux.len());
-		for (htlc, _) in htlcs_with_aux {
+		let mut htlcs = Vec::with_capacity(nondust_htlcs.len());
+		for htlc in nondust_htlcs {
 			let script = get_htlc_redeemscript(&htlc, &channel_parameters.channel_type_features(), &keys);
 			let txout = TxOut {
 				script_pubkey: script.to_p2wsh(),
@@ -1951,7 +1944,7 @@ mod tests {
 		commitment_number: u64,
 		per_commitment_point: PublicKey,
 		feerate_per_kw: u32,
-		htlcs_with_aux: Vec<(HTLCOutputInCommitment, ())>,
+		nondust_htlcs: Vec<HTLCOutputInCommitment>,
 		channel_parameters: ChannelTransactionParameters,
 		counterparty_pubkeys: ChannelPublicKeys,
 		secp_ctx: Secp256k1::<secp256k1::All>,
@@ -1979,23 +1972,23 @@ mod tests {
 				channel_type_features: ChannelTypeFeatures::only_static_remote_key(),
 				channel_value_satoshis: 3000,
 			};
-			let htlcs_with_aux = Vec::new();
+			let nondust_htlcs = Vec::new();
 
 			Self {
 				commitment_number: 0,
 				per_commitment_point,
 				feerate_per_kw: 1,
-				htlcs_with_aux,
+				nondust_htlcs,
 				channel_parameters,
 				counterparty_pubkeys,
 				secp_ctx,
 			}
 		}
 
 		fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction {
-			CommitmentTransaction::new_with_auxiliary_htlc_data(
+			CommitmentTransaction::new(
 				self.commitment_number, &self.per_commitment_point, to_broadcaster_sats, to_countersignatory_sats, self.feerate_per_kw,
-				&mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx
+				self.nondust_htlcs.iter_mut().collect(), &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx
 			)
 		}
 	}
@@ -2041,7 +2034,7 @@ mod tests {
 
 		// Generate broadcaster output and received and offered HTLC outputs,  w/o anchors
 		builder.channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key();
-		builder.htlcs_with_aux = vec![(received_htlc.clone(), ()), (offered_htlc.clone(), ())];
+		builder.nondust_htlcs = vec![received_htlc.clone(), offered_htlc.clone()];
 		let tx = builder.build(3000, 0);
 		let keys = &tx.trust().keys();
 		assert_eq!(tx.built.transaction.output.len(), 3);
@@ -2054,7 +2047,7 @@ mod tests {
 
 		// Generate broadcaster output and received and offered HTLC outputs,  with anchors
 		builder.channel_parameters.channel_type_features = ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies();
-		builder.htlcs_with_aux = vec![(received_htlc.clone(), ()), (offered_htlc.clone(), ())];
+		builder.nondust_htlcs = vec![received_htlc.clone(), offered_htlc.clone()];
 		let tx = builder.build(3000, 0);
 		assert_eq!(tx.built.transaction.output.len(), 5);
 		assert_eq!(tx.built.transaction.output[2].script_pubkey, get_htlc_redeemscript(&received_htlc, &ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(), &keys).to_p2wsh());

lightning/src/ln/channel.rs

@@ -3791,14 +3791,14 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		let channel_parameters =
 			if local { funding.channel_transaction_parameters.as_holder_broadcastable() }
 			else { funding.channel_transaction_parameters.as_counterparty_broadcastable() };
-		let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
-		                                                             &keys.per_commitment_point,
-		                                                             value_to_a as u64,
-		                                                             value_to_b as u64,
-		                                                             feerate_per_kw,
-		                                                             &mut included_non_dust_htlcs,
-		                                                             &channel_parameters,
-		                                                             &self.secp_ctx,
+		let tx = CommitmentTransaction::new(commitment_number,
+		                                    &keys.per_commitment_point,
+		                                    value_to_a as u64,
+		                                    value_to_b as u64,
+		                                    feerate_per_kw,
+		                                    included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc).collect(),
+		                                    &channel_parameters,
+		                                    &self.secp_ctx,
 		);
 		let mut htlcs_included = included_non_dust_htlcs;
 		// The unwrap is safe, because all non-dust HTLCs have been assigned an output index