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` for backwards compatibility. + // Includes both dust and non-dust HTLCs. The `Option` 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, Option)>, claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>, - nondust_htlc_sources: Vec, }, 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 Clone for ChannelMonitor 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, - dust_htlcs: Vec<(HTLCOutputInCommitment, Option)>, + htlcs: Vec<(HTLCOutputInCommitment, Option)>, } impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment { @@ -931,63 +922,26 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment fn try_from(value: (HolderCommitmentTransaction, HolderSignedTx)) -> Result { 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 { - 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)> { - 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 ChannelMonitor { 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 ChannelMonitor { &self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>, ) { - 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 ChannelMonitorImpl { fn provide_latest_holder_commitment_tx( &mut self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>, - claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], mut nondust_htlc_sources: Vec, + 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 ChannelMonitorImpl { 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 ChannelContext 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 ChannelContext 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 ChannelContext 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, Option)>, - pub nondust_htlc_sources: Vec, } /// Contents of a wire message that fails an HTLC backwards. Useful for [`FundedChannel::fail_htlc`] to @@ -5945,9 +5936,9 @@ impl FundedChannel 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 FundedChannel 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 {