Merge pull request #2101 from TheBlueMatt/2023-03-one-less-sig

Support future removal of redundant per-HTLC signatures in `CMU`s

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -494,8 +494,12 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,
 pub(crate) enum ChannelMonitorUpdateStep {
 	LatestHolderCommitmentTXInfo {
 		commitment_tx: HolderCommitmentTransaction,
+		/// Note that LDK after 0.0.115 supports this only containing dust HTLCs (implying the
+		/// `Signature` field is never filled in). At that point, non-dust HTLCs are implied by the
+		/// HTLC fields in `commitment_tx` and the sources passed via `nondust_htlc_sources`.
 		htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
 		claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>,
+		nondust_htlc_sources: Vec<HTLCSource>,
 	},
 	LatestCounterpartyCommitmentTXInfo {
 		commitment_txid: Txid,
@@ -540,6 +544,7 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
 		(0, commitment_tx, required),
 		(1, claimed_htlcs, vec_type),
 		(2, htlc_outputs, vec_type),
+		(4, nondust_htlc_sources, optional_vec),
 	},
 	(1, LatestCounterpartyCommitmentTXInfo) => {
 		(0, commitment_txid, required),
@@ -1181,7 +1186,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 		&self, holder_commitment_tx: HolderCommitmentTransaction,
 		htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
 	) -> Result<(), ()> {
-		self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new()).map_err(|_| ())
+		self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new(), Vec::new()).map_err(|_| ())
 	}
 
 	/// This is used to provide payment preimage(s) out-of-band during startup without updating the
@@ -2150,7 +2155,53 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	/// is important that any clones of this channel monitor (including remote clones) by kept
 	/// up-to-date as our holder commitment transaction is updated.
 	/// Panics if set_on_holder_tx_csv has never been called.
-	fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)]) -> Result<(), &'static str> {
+	fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, mut htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], nondust_htlc_sources: Vec<HTLCSource>) -> Result<(), &'static str> {
+		if htlc_outputs.iter().any(|(_, s, _)| s.is_some()) {
+			// If we have non-dust HTLCs in htlc_outputs, ensure they match the HTLCs in the
+			// `holder_commitment_tx`. In the future, we'll no longer provide the redundant data
+			// and just pass in source data via `nondust_htlc_sources`.
+			debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.trust().htlcs().len());
+			for (a, b) in htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).map(|(h, _, _)| h).zip(holder_commitment_tx.trust().htlcs().iter()) {
+				debug_assert_eq!(a, b);
+			}
+			debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.counterparty_htlc_sigs.len());
+			for (a, b) in htlc_outputs.iter().filter_map(|(_, s, _)| s.as_ref()).zip(holder_commitment_tx.counterparty_htlc_sigs.iter()) {
+				debug_assert_eq!(a, b);
+			}
+			debug_assert!(nondust_htlc_sources.is_empty());
+		} else {
+			// If we don't have any non-dust HTLCs in htlc_outputs, assume they were all passed via
+			// `nondust_htlc_sources`, building up the final htlc_outputs by combining
+			// `nondust_htlc_sources` and the `holder_commitment_tx`
+			#[cfg(debug_assertions)] {
+				let mut prev = -1;
+				for htlc in holder_commitment_tx.trust().htlcs().iter() {
+					assert!(htlc.transaction_output_index.unwrap() as i32 > prev);
+					prev = htlc.transaction_output_index.unwrap() as i32;
+				}
+			}
+			debug_assert!(htlc_outputs.iter().all(|(htlc, _, _)| htlc.transaction_output_index.is_none()));
+			debug_assert!(htlc_outputs.iter().all(|(_, sig_opt, _)| sig_opt.is_none()));
+			debug_assert_eq!(holder_commitment_tx.trust().htlcs().len(), holder_commitment_tx.counterparty_htlc_sigs.len());
+
+			let mut sources_iter = nondust_htlc_sources.into_iter();
+
+			for (htlc, counterparty_sig) in holder_commitment_tx.trust().htlcs().iter()
+				.zip(holder_commitment_tx.counterparty_htlc_sigs.iter())
+			{
+				if htlc.offered {
+					let source = sources_iter.next().expect("Non-dust HTLC sources didn't match commitment tx");
+					#[cfg(debug_assertions)] {
+						assert!(source.possibly_matches_output(htlc));
+					}
+					htlc_outputs.push((htlc.clone(), Some(counterparty_sig.clone()), Some(source)));
+				} else {
+					htlc_outputs.push((htlc.clone(), Some(counterparty_sig.clone()), None));
+				}
+			}
+			debug_assert!(sources_iter.next().is_none());
+		}
+
 		let trusted_tx = holder_commitment_tx.trust();
 		let txid = trusted_tx.txid();
 		let tx_keys = trusted_tx.keys();
@@ -2283,10 +2334,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&*fee_estimator);
 		for update in updates.updates.iter() {
 			match update {
-				ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, claimed_htlcs } => {
+				ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, claimed_htlcs, nondust_htlc_sources } => {
 					log_trace!(logger, "Updating ChannelMonitor with latest holder commitment transaction info");
 					if self.lockdown_from_offchain { panic!(); }
-					if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs) {
+					if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs, nondust_htlc_sources.clone()) {
 						log_error!(logger, "Providing latest holder commitment transaction failed/was refused:");
 						log_error!(logger, "    {}", e);
 						ret = Err(());
@@ -4139,7 +4190,7 @@ mod tests {
 			}
 		}
 
-		macro_rules! preimages_slice_to_htlc_outputs {
+		macro_rules! preimages_slice_to_htlcs {
 			($preimages_slice: expr) => {
 				{
 					let mut res = Vec::new();
@@ -4150,21 +4201,20 @@ mod tests {
 							cltv_expiry: 0,
 							payment_hash: preimage.1.clone(),
 							transaction_output_index: Some(idx as u32),
-						}, None));
+						}, ()));
 					}
 					res
 				}
 			}
 		}
-		macro_rules! preimages_to_holder_htlcs {
+		macro_rules! preimages_slice_to_htlc_outputs {
 			($preimages_slice: expr) => {
-				{
-					let mut inp = preimages_slice_to_htlc_outputs!($preimages_slice);
-					let res: Vec<_> = inp.drain(..).map(|e| { (e.0, None, e.1) }).collect();
-					res
-				}
+				preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|(htlc, _)| (htlc, None)).collect()
 			}
 		}
+		let dummy_sig = crate::util::crypto::sign(&secp_ctx,
+			&bitcoin::secp256k1::Message::from_slice(&[42; 32]).unwrap(),
+			&SecretKey::from_slice(&[42; 32]).unwrap());
 
 		macro_rules! test_preimages_exist {
 			($preimages_slice: expr, $monitor: expr) => {
@@ -4211,13 +4261,15 @@ mod tests {
 		let shutdown_pubkey = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let best_block = BestBlock::from_network(Network::Testnet);
 		let monitor = ChannelMonitor::new(Secp256k1::new(), keys,
-		                                  Some(ShutdownScript::new_p2wpkh_from_pubkey(shutdown_pubkey).into_inner()), 0, &Script::new(),
-		                                  (OutPoint { txid: Txid::from_slice(&[43; 32]).unwrap(), index: 0 }, Script::new()),
-		                                  &channel_parameters,
-		                                  Script::new(), 46, 0,
-		                                  HolderCommitmentTransaction::dummy(), best_block, dummy_key);
-
-		monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..10])).unwrap();
+			Some(ShutdownScript::new_p2wpkh_from_pubkey(shutdown_pubkey).into_inner()), 0, &Script::new(),
+			(OutPoint { txid: Txid::from_slice(&[43; 32]).unwrap(), index: 0 }, Script::new()),
+			&channel_parameters, Script::new(), 46, 0, HolderCommitmentTransaction::dummy(&mut Vec::new()),
+			best_block, dummy_key);
+
+		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..10]);
+		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);
+		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
+			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
 		monitor.provide_latest_counterparty_commitment_tx(Txid::from_inner(Sha256::hash(b"1").into_inner()),
 			preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
 		monitor.provide_latest_counterparty_commitment_tx(Txid::from_inner(Sha256::hash(b"2").into_inner()),
@@ -4250,15 +4302,21 @@ mod tests {
 
 		// Now update holder commitment tx info, pruning only element 18 as we still care about the
 		// previous commitment tx's preimages too
-		monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..5])).unwrap();
+		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
+		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);
+		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
+			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
 		secret[0..32].clone_from_slice(&hex::decode("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
-		monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..3])).unwrap();
+		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]);
+		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);
+		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx,
+			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
 		secret[0..32].clone_from_slice(&hex::decode("27cddaa5624534cb6cb9d7da077cf2b22ab21e9b506fd4998a51d54502e99116").unwrap());
 		monitor.provide_secret(281474976710652, secret.clone()).unwrap();
 		assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 5);

lightning/src/ln/chan_utils.rs

@@ -984,7 +984,7 @@ impl_writeable_tlv_based!(HolderCommitmentTransaction, {
 
 impl HolderCommitmentTransaction {
 	#[cfg(test)]
-	pub fn dummy() -> Self {
+	pub fn dummy(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_slice(&[42; 32]).unwrap(), &SecretKey::from_slice(&[42; 32]).unwrap());
@@ -1012,12 +1012,16 @@ impl HolderCommitmentTransaction {
 			opt_anchors: None,
 			opt_non_zero_fee_anchors: None,
 		};
-		let mut htlcs_with_aux: Vec<(_, ())> = Vec::new();
-		let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, false, dummy_key.clone(), dummy_key.clone(), keys, 0, &mut htlcs_with_aux, &channel_parameters.as_counterparty_broadcastable());
+		let mut counterparty_htlc_sigs = Vec::new();
+		for _ in 0..htlcs.len() {
+			counterparty_htlc_sigs.push(dummy_sig);
+		}
+		let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, false, dummy_key.clone(), dummy_key.clone(), keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable());
+		htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index);
 		HolderCommitmentTransaction {
 			inner,
 			counterparty_sig: dummy_sig,
-			counterparty_htlc_sigs: Vec::new(),
+			counterparty_htlc_sigs,
 			holder_sig_first: false
 		}
 	}

lightning/src/ln/channel.rs

@@ -3134,9 +3134,24 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 			return Err(ChannelError::Close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs)));
 		}
 
-		// TODO: Sadly, we pass HTLCs twice to ChannelMonitor: once via the HolderCommitmentTransaction and once via the update
+		// Up to LDK 0.0.115, HTLC information was required to be duplicated in the
+		// `htlcs_and_sigs` vec and in the `holder_commitment_tx` itself, both of which were passed
+		// in the `ChannelMonitorUpdate`. In 0.0.115, support for having a separate set of
+		// outbound-non-dust-HTLCSources in the `ChannelMonitorUpdate` was added, however for
+		// backwards compatibility, we never use it in production. To provide test coverage, here,
+		// we randomly decide (in test/fuzzing builds) to use the new vec sometimes.
+		#[allow(unused_assignments, unused_mut)]
+		let mut separate_nondust_htlc_sources = false;
+		#[cfg(all(feature = "std", any(test, fuzzing)))] {
+			use core::hash::{BuildHasher, Hasher};
+			// Get a random value using the only std API to do so - the DefaultHasher
+			let rand_val = std::collections::hash_map::RandomState::new().build_hasher().finish();
+			separate_nondust_htlc_sources = rand_val % 2 == 0;
+		}
+
+		let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len());
 		let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len());
-		for (idx, (htlc, source)) in htlcs_cloned.drain(..).enumerate() {
+		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.feerate_per_kw,
 					self.get_counterparty_selected_contest_delay().unwrap(), &htlc, self.opt_anchors(),
@@ -3151,10 +3166,18 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 				if let Err(_) = self.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &keys.countersignatory_htlc_key) {
 					return Err(ChannelError::Close("Invalid HTLC tx signature from peer".to_owned()));
 				}
-				htlcs_and_sigs.push((htlc, Some(msg.htlc_signatures[idx]), source));
+				if !separate_nondust_htlc_sources {
+					htlcs_and_sigs.push((htlc, Some(msg.htlc_signatures[idx]), source_opt.take()));
+				}
 			} else {
-				htlcs_and_sigs.push((htlc, None, source));
+				htlcs_and_sigs.push((htlc, None, source_opt.take()));
+			}
+			if separate_nondust_htlc_sources {
+				if let Some(source) = source_opt.take() {
+					nondust_htlc_sources.push(source);
+				}
 			}
+			debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere");
 		}
 
 		let holder_commitment_tx = HolderCommitmentTransaction::new(
@@ -3217,6 +3240,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 				commitment_tx: holder_commitment_tx,
 				htlc_outputs: htlcs_and_sigs,
 				claimed_htlcs,
+				nondust_htlc_sources,
 			}]
 		};
 

lightning/src/ln/channelmanager.rs

@@ -308,9 +308,9 @@ impl core::hash::Hash for HTLCSource {
 		}
 	}
 }
-#[cfg(not(feature = "grind_signatures"))]
-#[cfg(test)]
 impl HTLCSource {
+	#[cfg(not(feature = "grind_signatures"))]
+	#[cfg(test)]
 	pub fn dummy() -> Self {
 		HTLCSource::OutboundRoute {
 			path: Vec::new(),
@@ -320,6 +320,18 @@ impl HTLCSource {
 			payment_secret: None,
 		}
 	}
+
+	#[cfg(debug_assertions)]
+	/// Checks whether this HTLCSource could possibly match the given HTLC output in a commitment
+	/// transaction. Useful to ensure different datastructures match up.
+	pub(crate) fn possibly_matches_output(&self, htlc: &super::chan_utils::HTLCOutputInCommitment) -> bool {
+		if let HTLCSource::OutboundRoute { first_hop_htlc_msat, .. } = self {
+			*first_hop_htlc_msat == htlc.amount_msat
+		} else {
+			// There's nothing we can check for forwarded HTLCs
+			true
+		}
+	}
 }
 
 struct ReceiveError {