Support future removal of redundant per-HTLC data in `ChanMonUpd`s 2023-03-one-less-sig
authorMatt Corallo <git@bluematt.me>
Fri, 10 Mar 2023 23:32:48 +0000 (23:32 +0000)
committerMatt Corallo <git@bluematt.me>
Fri, 24 Mar 2023 19:02:08 +0000 (19:02 +0000)
`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.

lightning/src/chain/channelmonitor.rs
lightning/src/ln/chan_utils.rs
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs

index cd18ffad12d385747da26ee6e7222f9dc457b5f3..66ab16911146e1e2c7cd09a8c0b3f3b0b6ac4664 100644 (file)
@@ -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,7 +4295,10 @@ 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);
@@ -4251,7 +4306,10 @@ mod tests {
                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);
index 378b4d794ff495231f1fcc8a133530769bd8bd1a..32daf1478ec319cd0658411d0a903b8b2bfc11f3 100644 (file)
@@ -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
                }
        }
index 9af0f6379e5a1672e851a61ef0c54d717235a1a5..f883297576abbbd81c932633e0075f8dff14eb0b 100644 (file)
@@ -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,
                        }]
                };
 
index 6e0cd1709809d5d15c03f3cc036904c1003c956c..0831649a34d67291ede605dd54faa8d6e4e01920 100644 (file)
@@ -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 {