From 4d0eca1850e48feccc3857dcfd5b661bf99277d1 Mon Sep 17 00:00:00 2001
From: Wilmer Paulino <wilmer@wilmerpaulino.com>
Date: Mon, 14 Apr 2025 10:03:39 -0700
Subject: [PATCH] Revert separate non-dust HTLC sources for holder commitments
We previously provided non-dust HTLC sources to avoid storing duplicate
non-dust HTLC data in the `htlc_outputs` `Vec` where all HTLCs would be
tracked in a holder commitment update. With splicing, we'll
unfortunately be forced to store redundant copies of non-dust HTLC data
within the commitment transaction for each relevant `FundingScope`. As a
result, providing non-dust HTLC sources separately no longer provides
any benefits. In the future, we also plan to rework how the HTLC data
for holder and counterparty commitments are tracked to avoid storing
duplicate `HTLCSource`s.
---
lightning/src/chain/channelmonitor.rs | 178 ++++++--------------------
lightning/src/ln/channel.rs | 31 ++---
lightning/src/ln/channelmanager.rs | 11 --
3 files changed, 47 insertions(+), 173 deletions(-)
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 9d9e3382ffb..d6f2d8b9c31 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -317,31 +317,30 @@ impl HolderCommitment {
let delayed_payment_key = &tx_keys.broadcaster_delayed_payment_key;
let per_commitment_point = &tx_keys.per_commitment_point;
- let mut nondust_htlcs = self.tx.nondust_htlcs().iter().zip(self.tx.counterparty_htlc_sigs.iter());
- let mut sources = self.nondust_htlc_sources.iter();
-
- // Use an iterator to write `htlc_outputs` to avoid allocations.
- let nondust_htlcs = core::iter::from_fn(move || {
- let (htlc, counterparty_htlc_sig) = if let Some(nondust_htlc) = nondust_htlcs.next() {
- nondust_htlc
- } else {
- debug_assert!(sources.next().is_none());
- return None;
- };
+ debug_assert!(
+ self.htlcs.iter().map(|(htlc, _)| htlc).zip(self.tx.nondust_htlcs().iter())
+ .all(|(htlc_a, htlc_b)| htlc_a == htlc_b)
+ );
- let mut source = None;
- if htlc.offered {
- source = sources.next();
- if source.is_none() {
- panic!("Every offered non-dust HTLC should have a corresponding source");
+ let mut htlcs = self.htlcs.iter();
+ let mut counterparty_htlc_sigs = self.tx.counterparty_htlc_sigs.iter();
+ let htlc_outputs = core::iter::from_fn(move || {
+ if let Some((htlc, source)) = htlcs.next() {
+ let mut counterparty_htlc_sig = None;
+ if htlc.transaction_output_index.is_some() {
+ counterparty_htlc_sig = Some(counterparty_htlc_sigs.next()
+ .expect("Every non-dust HTLC must have a counterparty signature"));
+ } else {
+ // Once we see a dust HTLC, we should not expect to see any non-dust ones.
+ debug_assert!(counterparty_htlc_sigs.next().is_none());
}
+ Some((htlc, counterparty_htlc_sig, source.as_ref()))
+ } else {
+ debug_assert!(counterparty_htlc_sigs.next().is_none());
+ None
}
- Some((htlc, Some(counterparty_htlc_sig), source))
});
-
- // Dust HTLCs go last.
- let dust_htlcs = self.dust_htlcs.iter().map(|(htlc, source)| (htlc, None::<&Signature>, source.as_ref()));
- let htlc_outputs = crate::util::ser::IterableOwned(nondust_htlcs.chain(dust_htlcs));
+ let htlc_outputs = crate::util::ser::IterableOwned(htlc_outputs);
write_tlv_fields!(writer, {
(0, txid, required),
@@ -572,15 +571,11 @@ pub(crate) enum ChannelMonitorUpdateStep {
// Update LatestHolderCommitmentTXInfo in channel.rs if adding new fields to this variant.
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`.
- /// Starting with 0.2, the non-dust HTLC sources will always be provided separately, and
- /// `htlc_outputs` will only include dust HTLCs. We still have to track the
- /// `Option<Signature>` for backwards compatibility.
+ // Includes both dust and non-dust HTLCs. The `Option<Signature>` must be `Some` for
+ // non-dust HTLCs due to backwards compatibility, even though they are already tracked
+ // within the `HolderCommitmentTransaction` above.
htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>,
- nondust_htlc_sources: Vec<HTLCSource>,
},
LatestCounterpartyCommitmentTXInfo {
commitment_txid: Txid,
@@ -638,7 +633,6 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
(0, commitment_tx, required),
(1, claimed_htlcs, optional_vec),
(2, htlc_outputs, required_vec),
- (4, nondust_htlc_sources, optional_vec),
},
(1, LatestCounterpartyCommitmentTXInfo) => {
(0, commitment_txid, required),
@@ -920,10 +914,7 @@ impl<Signer: EcdsaChannelSigner> Clone for ChannelMonitor<Signer> where Signer:
#[derive(Clone, PartialEq)]
struct HolderCommitment {
tx: HolderCommitmentTransaction,
- // These must be sorted in increasing output index order to match the expected order of the
- // HTLCs in the `CommitmentTransaction`.
- nondust_htlc_sources: Vec<HTLCSource>,
- dust_htlcs: Vec<(HTLCOutputInCommitment, Option<HTLCSource>)>,
+ htlcs: Vec<(HTLCOutputInCommitment, Option<HTLCSource>)>,
}
impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment {
@@ -931,63 +922,26 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment
fn try_from(value: (HolderCommitmentTransaction, HolderSignedTx)) -> Result<Self, Self::Error> {
let holder_commitment_tx = value.0;
let holder_signed_tx = value.1;
-
- // HolderSignedTx tracks all HTLCs included in the commitment (dust included). For
- // `HolderCommitment`, we'll need to extract the dust HTLCs and their sources, and non-dust
- // HTLC sources, separately. All offered, non-dust HTLCs must have a source available.
-
- let mut missing_nondust_source = false;
- let mut nondust_htlc_sources = Vec::with_capacity(holder_commitment_tx.nondust_htlcs().len());
- let dust_htlcs = holder_signed_tx.htlc_outputs.into_iter().filter_map(|(htlc, _, source)| {
- // Filter our non-dust HTLCs, while at the same time pushing their sources into
- // `nondust_htlc_sources`.
- if htlc.transaction_output_index.is_none() {
- return Some((htlc, source))
- }
- if htlc.offered {
- if let Some(source) = source {
- nondust_htlc_sources.push(source);
- } else {
- missing_nondust_source = true;
- }
- }
- None
- }).collect();
- if missing_nondust_source {
- return Err(());
- }
-
Ok(Self {
tx: holder_commitment_tx,
- nondust_htlc_sources,
- dust_htlcs,
+ htlcs: holder_signed_tx.htlc_outputs.into_iter()
+ .map(|(htlc, _, source)| (htlc, source))
+ .collect(),
})
}
}
impl HolderCommitment {
fn has_htlcs(&self) -> bool {
- self.tx.nondust_htlcs().len() > 0 || self.dust_htlcs.len() > 0
+ !self.htlcs.is_empty()
}
fn htlcs(&self) -> impl Iterator<Item = &HTLCOutputInCommitment> {
- self.tx.nondust_htlcs().iter().chain(self.dust_htlcs.iter().map(|(htlc, _)| htlc))
+ self.htlcs.iter().map(|(htlc, _)| htlc)
}
fn htlcs_with_sources(&self) -> impl Iterator<Item = (&HTLCOutputInCommitment, Option<&HTLCSource>)> {
- let mut sources = self.nondust_htlc_sources.iter();
- let nondust_htlcs = self.tx.nondust_htlcs().iter().map(move |htlc| {
- let mut source = None;
- if htlc.offered && htlc.transaction_output_index.is_some() {
- source = sources.next();
- if source.is_none() {
- panic!("Every offered non-dust HTLC should have a corresponding source");
- }
- }
- (htlc, source)
- });
- let dust_htlcs = self.dust_htlcs.iter().map(|(htlc, source)| (htlc, source.as_ref()));
- nondust_htlcs.chain(dust_htlcs)
+ self.htlcs.iter().map(|(htlc, source)| (htlc, source.as_ref()))
}
}
@@ -1554,8 +1508,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
current_holder_commitment: HolderCommitment {
tx: initial_holder_commitment_tx,
// There are never any HTLCs in the initial commitment transactions
- nondust_htlc_sources: Vec::new(),
- dust_htlcs: Vec::new(),
+ htlcs: Vec::new(),
},
prev_holder_commitment: None,
},
@@ -1680,7 +1633,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
&self, holder_commitment_tx: HolderCommitmentTransaction,
htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
) {
- self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new(), Vec::new())
+ self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new())
}
/// This is used to provide payment preimage(s) out-of-band during startup without updating the
@@ -3092,72 +3045,13 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
fn provide_latest_holder_commitment_tx(
&mut self, holder_commitment_tx: HolderCommitmentTransaction,
htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
- claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], mut nondust_htlc_sources: Vec<HTLCSource>,
+ claimed_htlcs: &[(SentHTLCId, PaymentPreimage)],
) {
- let dust_htlcs: Vec<_> = 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().nondust_htlcs().len());
- for (a, b) in htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).map(|(h, _, _)| h).zip(holder_commitment_tx.trust().nondust_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);
- }
-
- // Backfill the non-dust HTLC sources.
- debug_assert!(nondust_htlc_sources.is_empty());
- nondust_htlc_sources.reserve_exact(holder_commitment_tx.nondust_htlcs().len());
- let dust_htlcs = htlc_outputs.into_iter().filter_map(|(htlc, _, source)| {
- // Filter our non-dust HTLCs, while at the same time pushing their sources into
- // `nondust_htlc_sources`.
- if htlc.transaction_output_index.is_none() {
- return Some((htlc, source));
- }
- if htlc.offered {
- nondust_htlc_sources.push(source.expect("Outbound HTLCs should have a source"));
- }
- None
- }).collect();
-
- dust_htlcs
- } 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`
- {
- let mut prev = -1;
- for htlc in holder_commitment_tx.trust().nondust_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().nondust_htlcs().len(), holder_commitment_tx.counterparty_htlc_sigs.len());
-
- let mut sources = nondust_htlc_sources.iter();
- for htlc in holder_commitment_tx.trust().nondust_htlcs().iter() {
- if htlc.offered {
- let source = sources.next().expect("Non-dust HTLC sources didn't match commitment tx");
- assert!(source.possibly_matches_output(htlc));
- }
- }
- assert!(sources.next().is_none(), "All HTLC sources should have been exhausted");
-
- // This only includes dust HTLCs as checked above.
- htlc_outputs.into_iter().map(|(htlc, _, source)| (htlc, source)).collect()
- };
-
self.current_holder_commitment_number = holder_commitment_tx.trust().commitment_number();
self.onchain_tx_handler.provide_latest_holder_tx(holder_commitment_tx.clone());
let mut holder_commitment = HolderCommitment {
tx: holder_commitment_tx,
- nondust_htlc_sources,
- dust_htlcs,
+ htlcs: htlc_outputs.into_iter().map(|(htlc, _, source)| (htlc, source)).collect(),
};
mem::swap(&mut holder_commitment, &mut self.funding.current_holder_commitment);
self.funding.prev_holder_commitment = Some(holder_commitment);
@@ -3387,10 +3281,10 @@ impl<Signer: EcdsaChannelSigner> 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, nondust_htlc_sources } => {
+ ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, claimed_htlcs } => {
log_trace!(logger, "Updating ChannelMonitor with latest holder commitment transaction info");
if self.lockdown_from_offchain { panic!(); }
- self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs, nondust_htlc_sources.clone());
+ self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs);
}
// Soon we will drop the `LatestCounterpartyCommitmentTXInfo` variant in favor of `LatestCounterpartyCommitmentTX`.
// For now we just add the code to handle the new updates.
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index d4d613967c7..69c7d9d8c98 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3802,9 +3802,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
let holder_keys = commitment_data.tx.trust().keys();
- let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.nondust_htlcs().len());
- let mut dust_htlcs = Vec::with_capacity(commitment_data.htlcs_included.len() - commitment_data.tx.nondust_htlcs().len());
- for (idx, (htlc, mut source_opt)) in commitment_data.htlcs_included.into_iter().enumerate() {
+ let mut htlc_outputs = Vec::with_capacity(commitment_data.htlcs_included.len());
+ for (idx, (htlc, source_opt)) in commitment_data.htlcs_included.into_iter().enumerate() {
+ let mut counterparty_htlc_sig = None;
if let Some(_) = htlc.transaction_output_index {
let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.tx.feerate_per_kw(),
funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, funding.get_channel_type(),
@@ -3819,17 +3819,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if let Err(_) = self.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &holder_keys.countersignatory_htlc_key.to_public_key()) {
return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned()));
}
- if htlc.offered {
- if let Some(source) = source_opt.take() {
- nondust_htlc_sources.push(source.clone());
- } else {
- panic!("Missing outbound HTLC source");
- }
- }
- } else {
- dust_htlcs.push((htlc, None, source_opt.take().cloned()));
+
+ counterparty_htlc_sig = Some(msg.htlc_signatures[idx]);
}
- debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere");
+ htlc_outputs.push((htlc, counterparty_htlc_sig, source_opt.cloned()));
}
let holder_commitment_tx = HolderCommitmentTransaction::new(
@@ -3845,8 +3838,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
Ok(LatestHolderCommitmentTXInfo {
commitment_tx: holder_commitment_tx,
- htlc_outputs: dust_htlcs,
- nondust_htlc_sources,
+ htlc_outputs,
})
}
@@ -5155,7 +5147,6 @@ struct CommitmentTxInfoCached {
struct LatestHolderCommitmentTXInfo {
pub commitment_tx: HolderCommitmentTransaction,
pub htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
- pub nondust_htlc_sources: Vec<HTLCSource>,
}
/// Contents of a wire message that fails an HTLC backwards. Useful for [`FundedChannel::fail_htlc`] to
@@ -5945,9 +5936,9 @@ impl<SP: Deref> FundedChannel<SP> where
let updates = self
.context
.validate_commitment_signed(&self.funding, &self.holder_commitment_point, msg, logger)
- .map(|LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, nondust_htlc_sources }|
+ .map(|LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs }|
vec![ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo {
- commitment_tx, htlc_outputs, claimed_htlcs: vec![], nondust_htlc_sources,
+ commitment_tx, htlc_outputs, claimed_htlcs: vec![],
}]
)?;
@@ -5970,9 +5961,9 @@ impl<SP: Deref> FundedChannel<SP> where
.ok_or_else(|| ChannelError::close(format!("Peer did not send a commitment_signed for pending splice transaction: {}", funding_txid)))?;
self.context
.validate_commitment_signed(funding, &self.holder_commitment_point, msg, logger)
- .map(|LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, nondust_htlc_sources }|
+ .map(|LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs }|
ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo {
- commitment_tx, htlc_outputs, claimed_htlcs: vec![], nondust_htlc_sources,
+ commitment_tx, htlc_outputs, claimed_htlcs: vec![],
}
)
}
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 363f2ffdb65..61813fa7fe7 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -726,17 +726,6 @@ impl HTLCSource {
}
}
- /// 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
- }
- }
-
/// Returns the CLTV expiry of the inbound HTLC (i.e. the source referred to by this object),
/// if the source was a forwarded HTLC and the HTLC was first forwarded on LDK 0.1.1 or later.
pub(crate) fn inbound_htlc_expiry(&self) -> Option<u32> {