Support future removal of redundant per-HTLC data in ChanMonUpds

`ChannelMonitorUpdate`s are our most size-sensitive objects - they
are the minimal objects which need to be written to disk on each
commitment update. Thus, we should be careful to ensure we don't
pack too much extraneous information into each one.

Here we add future support for removing the per-HTLC explicit
`Option<Signature>` and `HTLCInCommitmentUpdate` for non-dust HTLCs
in holder commitment tx updates, which are redundant with the
`HolderCommitmentTransaction`.

While we cannot remove them entirely as previous versions rely on
them, adding support for filling in the in-memory structures from
the redundant fields will let us remove them in a future version.

We also add test-only generation logic to test the new derivation.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -493,8 +493,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,
@@ -539,6 +543,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),
@@ -1180,7 +1185,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
@@ -2160,7 +2165,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();
@@ -2285,10 +2336,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(());
@@ -4132,7 +4183,7 @@ mod tests {
 			}
 		}
 
-		macro_rules! preimages_slice_to_htlc_outputs {
+		macro_rules! preimages_slice_to_htlcs {
 			($preimages_slice: expr) => {
 				{
 					let mut res = Vec::new();
@@ -4143,21 +4194,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) => {
@@ -4204,13 +4254,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()),
@@ -4243,15 +4295,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

@@ -3122,9 +3122,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(),
@@ -3139,10 +3154,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(
@@ -3205,6 +3228,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

@@ -302,9 +302,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(),
@@ -315,6 +315,18 @@ impl HTLCSource {
 			payment_params: 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 {