From 2fac625eb6e44d2d60af4418065bf56e1a21f6b7 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Tue, 1 Apr 2025 16:01:46 +0000 Subject: [PATCH 1/5] Add `ChannelContext::build_commitment_stats` It can be useful to get the stats on a potential commitment transaction without actually building it. Therefore, this commit splits the stats calculations from the actual build of a commitment transaction. This introduces an extra loop over the pending htlcs when actually building a commitment transaction, but current network behavior produces very few concurrent htlcs on channels. Furthermore, each iteration of the loop in the stats calculation is very cheap. The motivating use case for `build_commitment_stats` is to calculate the balances of the channel parties in order to validate the `funding_contribution_satoshis` field of `splice_init` and `splice_ack` messages without building a full commitment transaction. --- lightning/src/ln/channel.rs | 415 ++++++++++++++++++++++-------------- 1 file changed, 253 insertions(+), 162 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 005ec566f0e..ebc0482aff5 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -226,6 +226,37 @@ impl From<&InboundHTLCState> for Option { } } +impl fmt::Display for InboundHTLCState { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + InboundHTLCState::RemoteAnnounced(_) => write!(f, "RemoteAnnounced"), + InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => write!(f, "AwaitingRemoteRevokeToAnnounce"), + InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => write!(f, "AwaitingAnnouncedRemoteRevoke"), + InboundHTLCState::Committed => write!(f, "Committed"), + InboundHTLCState::LocalRemoved(_) => write!(f, "LocalRemoved"), + } + } +} + +impl InboundHTLCState { + fn included_in_commitment(&self, generated_by_local: bool) -> bool { + match self { + InboundHTLCState::RemoteAnnounced(_) => !generated_by_local, + InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => !generated_by_local, + InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => true, + InboundHTLCState::Committed => true, + InboundHTLCState::LocalRemoved(_) => !generated_by_local, + } + } + + fn preimage(&self) -> Option { + match self { + InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => Some(*preimage), + _ => None, + } + } +} + struct InboundHTLCOutput { htlc_id: u64, amount_msat: u64, @@ -234,6 +265,24 @@ struct InboundHTLCOutput { state: InboundHTLCState, } +impl InboundHTLCOutput { + fn is_dust(&self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool { + let htlc_tx_fee_sat = if features.supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + let htlc_tx_weight = if !local { + // this is an offered htlc + htlc_timeout_tx_weight(features) + } else { + htlc_success_tx_weight(features) + }; + // As required by the spec, round down + feerate_per_kw as u64 * htlc_tx_weight / 1000 + }; + self.amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat + } +} + #[cfg_attr(test, derive(Clone, Debug, PartialEq))] enum OutboundHTLCState { /// Added by us and included in a commitment_signed (if we were AwaitingRemoteRevoke when we @@ -290,6 +339,39 @@ impl From<&OutboundHTLCState> for OutboundHTLCStateDetails { } } +impl fmt::Display for OutboundHTLCState { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + OutboundHTLCState::LocalAnnounced(_) => write!(f, "LocalAnnounced"), + OutboundHTLCState::Committed => write!(f, "Committed"), + OutboundHTLCState::RemoteRemoved(_) => write!(f, "RemoteRemoved"), + OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => write!(f, "AwaitingRemoteRevokeToRemove"), + OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => write!(f, "AwaitingRemovedRemoteRevoke"), + } + } +} + +impl OutboundHTLCState { + fn included_in_commitment(&self, generated_by_local: bool) -> bool { + match self { + OutboundHTLCState::LocalAnnounced(_) => generated_by_local, + OutboundHTLCState::Committed => true, + OutboundHTLCState::RemoteRemoved(_) => generated_by_local, + OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => generated_by_local, + OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => false, + } + } + + fn preimage(&self) -> Option { + match self { + OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(p)) | + OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(p)) | + OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(p)) => p.as_ref().copied(), + _ => None, + } + } +} + #[derive(Clone)] #[cfg_attr(test, derive(Debug, PartialEq))] enum OutboundHTLCOutcome { @@ -329,6 +411,24 @@ struct OutboundHTLCOutput { send_timestamp: Option, } +impl OutboundHTLCOutput { + fn is_dust(&self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool { + let htlc_tx_fee_sat = if features.supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + let htlc_tx_weight = if local { + // this is an offered htlc + htlc_timeout_tx_weight(features) + } else { + htlc_success_tx_weight(features) + }; + // As required by the spec, round down + feerate_per_kw as u64 * htlc_tx_weight / 1000 + }; + self.amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat + } +} + /// See AwaitingRemoteRevoke ChannelState for more info #[cfg_attr(test, derive(Clone, Debug, PartialEq))] enum HTLCUpdateAwaitingACK { @@ -883,6 +983,7 @@ struct HTLCStats { /// A struct gathering data on a commitment, either local or remote. struct CommitmentData<'a> { + tx: CommitmentTransaction, stats: CommitmentStats, htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction outbound_htlc_preimages: Vec, // preimages for successful offered HTLCs since last commitment @@ -891,10 +992,12 @@ struct CommitmentData<'a> { /// A struct gathering stats on a commitment transaction, either local or remote. struct CommitmentStats { - tx: CommitmentTransaction, // the transaction info + feerate_per_kw: u32, // the feerate of the commitment transaction total_fee_sat: u64, // the total fee included in the transaction - local_balance_before_fee_msat: u64, // local balance before fees *not* considering dust limits - remote_balance_before_fee_msat: u64, // remote balance before fees *not* considering dust limits + total_anchors_sat: u64, // the sum of the anchors' amounts + broadcaster_dust_limit_sat: u64, // the broadcaster's dust limit + local_balance_before_fee_anchors_msat: u64, // local balance before fees and anchors *not* considering dust limits + remote_balance_before_fee_anchors_msat: u64, // remote balance before fees and anchors *not* considering dust limits } /// Used when calculating whether we or the remote can afford an additional HTLC. @@ -2124,7 +2227,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide let commitment_data = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger); - let initial_commitment_tx = commitment_data.stats.tx; + let initial_commitment_tx = commitment_data.tx; let trusted_tx = initial_commitment_tx.trust(); let initial_commitment_bitcoin_tx = trusted_tx.built_transaction(); let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.funding().get_value_satoshis()); @@ -2164,7 +2267,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide let commitment_data = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &context.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.stats.tx; + let counterparty_initial_commitment_tx = commitment_data.tx; let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); @@ -3646,7 +3749,7 @@ impl ChannelContext where SP::Target: SignerProvider { holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger); let commitment_txid = { - let trusted_tx = commitment_data.stats.tx.trust(); + let trusted_tx = commitment_data.tx.trust(); let bitcoin_tx = trusted_tx.built_transaction(); let sighash = bitcoin_tx.get_sighash_all(&funding_script, funding.get_value_satoshis()); @@ -3669,7 +3772,7 @@ impl ChannelContext where SP::Target: SignerProvider { if update_fee { debug_assert!(!funding.is_outbound()); let counterparty_reserve_we_require_msat = funding.holder_selected_channel_reserve_satoshis * 1000; - if commitment_data.stats.remote_balance_before_fee_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { + if commitment_data.stats.remote_balance_before_fee_anchors_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned())); } } @@ -3691,16 +3794,16 @@ impl ChannelContext where SP::Target: SignerProvider { } } - if msg.htlc_signatures.len() != commitment_data.stats.tx.htlcs().len() { - return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.stats.tx.htlcs().len()))); + if msg.htlc_signatures.len() != commitment_data.tx.htlcs().len() { + return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.htlcs().len()))); } - let holder_keys = commitment_data.stats.tx.trust().keys(); - let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.stats.tx.htlcs().len()); - let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.stats.tx.htlcs().len()); + let holder_keys = commitment_data.tx.trust().keys(); + let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.htlcs().len()); + let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.tx.htlcs().len()); for (idx, (htlc, mut source_opt)) in htlcs_cloned.into_iter().enumerate() { if let Some(_) = htlc.transaction_output_index { - let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.stats.tx.feerate_per_kw(), + 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(), &holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key); @@ -3727,7 +3830,7 @@ impl ChannelContext where SP::Target: SignerProvider { } let holder_commitment_tx = HolderCommitmentTransaction::new( - commitment_data.stats.tx, + commitment_data.tx, msg.signature, msg.htlc_signatures.clone(), &funding.get_holder_pubkeys().funding_pubkey, @@ -3744,28 +3847,12 @@ impl ChannelContext where SP::Target: SignerProvider { }) } - /// Transaction nomenclature is somewhat confusing here as there are many different cases - a - /// transaction is referred to as "a's transaction" implying that a will be able to broadcast - /// the transaction. Thus, b will generally be sending a signature over such a transaction to - /// a, and a can revoke the transaction by providing b the relevant per_commitment_secret. As - /// such, a transaction is generally the result of b increasing the amount paid to a (or adding - /// an HTLC to a). - /// @local is used only to convert relevant internal structures which refer to remote vs local - /// to decide value of outputs and direction of HTLCs. - /// @generated_by_local is used to determine *which* HTLCs to include - noting that the HTLC - /// state may indicate that one peer has informed the other that they'd like to add an HTLC but - /// have not yet committed it. Such HTLCs will only be included in transactions which are being - /// generated by the peer which proposed adding the HTLCs, and thus we need to understand both - /// which peer generated this transaction and "to whom" this transaction flows. + /// Builds stats on a potential commitment transaction build, without actually building the + /// commitment transaction. See `build_commitment_transaction` for further docs. #[inline] - fn build_commitment_transaction(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData - where L::Target: Logger - { - let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new(); - let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); - let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); - - let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis }; + fn build_commitment_stats(&self, funding: &FundingScope, local: bool, generated_by_local: bool) -> CommitmentStats { + let broadcaster_dust_limit_sat = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis }; + let mut non_dust_htlc_count = 0; let mut remote_htlc_total_msat = 0; let mut local_htlc_total_msat = 0; let mut value_to_self_msat_offset = 0; @@ -3783,122 +3870,34 @@ impl ChannelContext where SP::Target: SignerProvider { } } - log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...", - commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number), - get_commitment_transaction_number_obscure_factor(&funding.get_holder_pubkeys().payment_point, &funding.get_counterparty_pubkeys().payment_point, funding.is_outbound()), - &self.channel_id, - if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw); - - macro_rules! get_htlc_in_commitment { - ($htlc: expr, $offered: expr) => { - HTLCOutputInCommitment { - offered: $offered, - amount_msat: $htlc.amount_msat, - cltv_expiry: $htlc.cltv_expiry, - payment_hash: $htlc.payment_hash, - transaction_output_index: None - } - } - } - - macro_rules! add_htlc_output { - ($htlc: expr, $outbound: expr, $source: expr, $state_name: expr) => { - if $outbound == local { // "offered HTLC output" - let htlc_in_tx = get_htlc_in_commitment!($htlc, true); - let htlc_tx_fee = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * htlc_timeout_tx_weight(funding.get_channel_type()) / 1000 - }; - if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { - log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat); - included_non_dust_htlcs.push((htlc_in_tx, $source)); - } else { - log_trace!(logger, " ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat); - included_dust_htlcs.push((htlc_in_tx, $source)); - } - } else { - let htlc_in_tx = get_htlc_in_commitment!($htlc, false); - let htlc_tx_fee = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * htlc_success_tx_weight(funding.get_channel_type()) / 1000 - }; - if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { - log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat); - included_non_dust_htlcs.push((htlc_in_tx, $source)); - } else { - log_trace!(logger, " ...including {} {} dust HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat); - included_dust_htlcs.push((htlc_in_tx, $source)); - } + for htlc in self.pending_inbound_htlcs.iter() { + if htlc.state.included_in_commitment(generated_by_local) { + if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { + non_dust_htlc_count += 1; } - } - } - - let mut inbound_htlc_preimages: Vec = Vec::new(); - - for ref htlc in self.pending_inbound_htlcs.iter() { - let (include, state_name) = match htlc.state { - InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"), - InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => (!generated_by_local, "AwaitingRemoteRevokeToAnnounce"), - InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => (true, "AwaitingAnnouncedRemoteRevoke"), - InboundHTLCState::Committed => (true, "Committed"), - InboundHTLCState::LocalRemoved(_) => (!generated_by_local, "LocalRemoved"), - }; - - if include { - add_htlc_output!(htlc, false, None, state_name); remote_htlc_total_msat += htlc.amount_msat; } else { - log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name); - match &htlc.state { - &InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => { - inbound_htlc_preimages.push(preimage); - value_to_self_msat_offset += htlc.amount_msat as i64; - }, - _ => {}, + if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_preimage)) = htlc.state { + value_to_self_msat_offset += htlc.amount_msat as i64; } } - } - - - let mut outbound_htlc_preimages: Vec = Vec::new(); - - for ref htlc in self.pending_outbound_htlcs.iter() { - let (include, state_name) = match htlc.state { - OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"), - OutboundHTLCState::Committed => (true, "Committed"), - OutboundHTLCState::RemoteRemoved(_) => (generated_by_local, "RemoteRemoved"), - OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => (generated_by_local, "AwaitingRemoteRevokeToRemove"), - OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => (false, "AwaitingRemovedRemoteRevoke"), - }; - - let preimage_opt = match htlc.state { - OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(p)) => p, - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(p)) => p, - OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(p)) => p, - _ => None, - }; - - if let Some(preimage) = preimage_opt { - outbound_htlc_preimages.push(preimage); - } + }; - if include { - add_htlc_output!(htlc, true, Some(&htlc.source), state_name); + for htlc in self.pending_outbound_htlcs.iter() { + if htlc.state.included_in_commitment(generated_by_local) { + if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { + non_dust_htlc_count += 1; + } local_htlc_total_msat += htlc.amount_msat; } else { - log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name); - match htlc.state { - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) | + if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) | OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) | - OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) => { - value_to_self_msat_offset -= htlc.amount_msat as i64; - }, - _ => {}, + OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) + = htlc.state { + value_to_self_msat_offset -= htlc.amount_msat as i64; } } - } + }; // # Panics // @@ -3927,31 +3926,129 @@ impl ChannelContext where SP::Target: SignerProvider { broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat); } + let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, non_dust_htlc_count, &funding.channel_transaction_parameters.channel_type_features); + let total_anchors_sat = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; + + CommitmentStats { + feerate_per_kw, + total_fee_sat, + total_anchors_sat, + broadcaster_dust_limit_sat, + local_balance_before_fee_anchors_msat: value_to_self_msat, + remote_balance_before_fee_anchors_msat: value_to_remote_msat, + } + } + + /// Transaction nomenclature is somewhat confusing here as there are many different cases - a + /// transaction is referred to as "a's transaction" implying that a will be able to broadcast + /// the transaction. Thus, b will generally be sending a signature over such a transaction to + /// a, and a can revoke the transaction by providing b the relevant per_commitment_secret. As + /// such, a transaction is generally the result of b increasing the amount paid to a (or adding + /// an HTLC to a). + /// @local is used only to convert relevant internal structures which refer to remote vs local + /// to decide value of outputs and direction of HTLCs. + /// @generated_by_local is used to determine *which* HTLCs to include - noting that the HTLC + /// state may indicate that one peer has informed the other that they'd like to add an HTLC but + /// have not yet committed it. Such HTLCs will only be included in transactions which are being + /// generated by the peer which proposed adding the HTLCs, and thus we need to understand both + /// which peer generated this transaction and "to whom" this transaction flows. + #[inline] + fn build_commitment_transaction(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData + where L::Target: Logger + { + let stats = self.build_commitment_stats(funding, local, generated_by_local); + let CommitmentStats { + feerate_per_kw, + total_fee_sat, + total_anchors_sat, + broadcaster_dust_limit_sat, + local_balance_before_fee_anchors_msat, + remote_balance_before_fee_anchors_msat + } = stats; + + let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new(); + let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); + let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); + + log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...", + commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number), + get_commitment_transaction_number_obscure_factor(&funding.get_holder_pubkeys().payment_point, &funding.get_counterparty_pubkeys().payment_point, funding.is_outbound()), + self.channel_id, + if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw); + + macro_rules! get_htlc_in_commitment { + ($htlc: expr, $offered: expr) => { + HTLCOutputInCommitment { + offered: $offered, + amount_msat: $htlc.amount_msat, + cltv_expiry: $htlc.cltv_expiry, + payment_hash: $htlc.payment_hash, + transaction_output_index: None + } + } + } + + macro_rules! add_htlc_output { + ($htlc: expr, $outbound: expr, $source: expr) => { + let htlc_in_tx = get_htlc_in_commitment!($htlc, $outbound == local); + if $htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { + log_trace!(logger, " ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat); + included_dust_htlcs.push((htlc_in_tx, $source)); + } else { + log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat); + included_non_dust_htlcs.push((htlc_in_tx, $source)); + } + } + } + + let mut inbound_htlc_preimages: Vec = Vec::new(); + let mut outbound_htlc_preimages: Vec = Vec::new(); + + for htlc in self.pending_inbound_htlcs.iter() { + if htlc.state.included_in_commitment(generated_by_local) { + add_htlc_output!(htlc, false, None); + } else { + log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, htlc.payment_hash, htlc.amount_msat, htlc.state); + if let Some(preimage) = htlc.state.preimage() { + inbound_htlc_preimages.push(preimage); + } + } + }; + + for htlc in self.pending_outbound_htlcs.iter() { + if let Some(preimage) = htlc.state.preimage() { + outbound_htlc_preimages.push(preimage); + } + if htlc.state.included_in_commitment(generated_by_local) { + add_htlc_output!(htlc, true, Some(&htlc.source)); + } else { + log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, htlc.payment_hash, htlc.amount_msat, htlc.state); + } + }; + // We MUST use saturating subs here, as the funder's balance is not guaranteed to be greater - // than or equal to the sum of `total_fee_sat` and `anchors_val`. + // than or equal to the sum of `total_fee_sat` and `total_anchors_sat`. // // This is because when the remote party sends an `update_fee` message, we build the new // commitment transaction *before* checking whether the remote party's balance is enough to // cover the total fee and the anchors. - let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), funding.get_channel_type()); - let anchors_val = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; let (value_to_self, value_to_remote) = if funding.is_outbound() { - ((value_to_self_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat), value_to_remote_msat / 1000) + ((local_balance_before_fee_anchors_msat / 1000).saturating_sub(total_anchors_sat).saturating_sub(total_fee_sat), remote_balance_before_fee_anchors_msat / 1000) } else { - (value_to_self_msat / 1000, (value_to_remote_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat)) + (local_balance_before_fee_anchors_msat / 1000, (remote_balance_before_fee_anchors_msat / 1000).saturating_sub(total_anchors_sat).saturating_sub(total_fee_sat)) }; let mut to_broadcaster_value_sat = if local { value_to_self } else { value_to_remote }; let mut to_countersignatory_value_sat = if local { value_to_remote } else { value_to_self }; - if to_broadcaster_value_sat >= broadcaster_dust_limit_satoshis { + if to_broadcaster_value_sat >= broadcaster_dust_limit_sat { log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, to_broadcaster_value_sat); } else { to_broadcaster_value_sat = 0; } - if to_countersignatory_value_sat >= broadcaster_dust_limit_satoshis { + if to_countersignatory_value_sat >= broadcaster_dust_limit_sat { log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, to_countersignatory_value_sat); } else { to_countersignatory_value_sat = 0; @@ -3974,14 +4071,8 @@ impl ChannelContext where SP::Target: SignerProvider { htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap()); htlcs_included.append(&mut included_dust_htlcs); - let stats = CommitmentStats { - tx, - total_fee_sat, - local_balance_before_fee_msat: value_to_self_msat, - remote_balance_before_fee_msat: value_to_remote_msat, - }; - CommitmentData { + tx, stats, htlcs_included, inbound_htlc_preimages, @@ -4773,7 +4864,7 @@ impl ChannelContext where SP::Target: SignerProvider { let commitment_data = self.build_commitment_transaction(funding, self.cur_counterparty_commitment_transaction_number, &self.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.stats.tx; + let counterparty_initial_commitment_tx = commitment_data.tx; match self.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ref ecdsa) => { @@ -6557,8 +6648,8 @@ impl FundedChannel where &self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, true, logger, ); - let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000; - let holder_balance_msat = commitment_data.stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat; + let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000; + let holder_balance_msat = commitment_data.stats.local_balance_before_fee_anchors_msat - htlc_stats.outbound_holding_cell_msat; if holder_balance_msat < buffer_fee_msat + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { //TODO: auto-close after a number of failures? log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw); @@ -6872,7 +6963,7 @@ impl FundedChannel where let commitment_data = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number + 1, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.stats.tx; + let counterparty_initial_commitment_tx = commitment_data.tx; self.context.get_funding_signed_msg(&self.funding.channel_transaction_parameters, logger, counterparty_initial_commitment_tx) } else { None }; // Provide a `channel_ready` message if we need to, but only if we're _not_ still pending @@ -8991,7 +9082,7 @@ impl FundedChannel where funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger, ); - let counterparty_commitment_tx = commitment_data.stats.tx; + let counterparty_commitment_tx = commitment_data.tx; #[cfg(any(test, fuzzing))] { @@ -9042,7 +9133,7 @@ impl FundedChannel where funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger, ); - let counterparty_commitment_tx = commitment_data.stats.tx; + let counterparty_commitment_tx = commitment_data.tx; match &self.context.holder_signer { ChannelSignerType::Ecdsa(ecdsa) => { @@ -9535,7 +9626,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { let commitment_data = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.stats.tx; + let counterparty_initial_commitment_tx = commitment_data.tx; let signature = match &self.context.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ecdsa) => { @@ -12265,7 +12356,7 @@ mod tests { } ) => { { let commitment_data = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger); - let commitment_tx = commitment_data.stats.tx; + let commitment_tx = commitment_data.tx; let trusted_tx = commitment_tx.trust(); let unsigned_tx = trusted_tx.built_transaction(); let redeemscript = chan.funding.get_funding_redeemscript(); From 14f8107ac82f4fb56bef78595fe87ba389bcc8f9 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Mon, 24 Mar 2025 23:12:55 +0000 Subject: [PATCH 2/5] Rename `CommitmentTransaction::htlcs` to `nondust_htlcs` --- lightning/src/chain/channelmonitor.rs | 38 +++++++++++++-------------- lightning/src/chain/onchaintx.rs | 4 +-- lightning/src/ln/chan_utils.rs | 34 ++++++++++++------------ lightning/src/ln/channel.rs | 22 ++++++++-------- lightning/src/sign/mod.rs | 4 +-- 5 files changed, 51 insertions(+), 51 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5ed524ae764..b4c4bceec4a 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -317,7 +317,7 @@ 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.htlcs().iter().zip(self.tx.counterparty_htlc_sigs.iter()); + 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. @@ -937,7 +937,7 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment // 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.htlcs().len()); + 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`. @@ -967,16 +967,16 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment impl HolderCommitment { fn has_htlcs(&self) -> bool { - self.tx.htlcs().len() > 0 || self.dust_htlcs.len() > 0 + self.tx.nondust_htlcs().len() > 0 || self.dust_htlcs.len() > 0 } fn htlcs(&self) -> impl Iterator { - self.tx.htlcs().iter().chain(self.dust_htlcs.iter().map(|(htlc, _)| htlc)) + self.tx.nondust_htlcs().iter().chain(self.dust_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.htlcs().iter().map(move |htlc| { + 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(); @@ -3093,8 +3093,8 @@ impl ChannelMonitorImpl { // 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!(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()); @@ -3104,7 +3104,7 @@ impl ChannelMonitorImpl { // Backfill the non-dust HTLC sources. debug_assert!(nondust_htlc_sources.is_empty()); - nondust_htlc_sources.reserve_exact(holder_commitment_tx.htlcs().len()); + 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`. @@ -3124,7 +3124,7 @@ impl ChannelMonitorImpl { // `nondust_htlc_sources` and the `holder_commitment_tx` { let mut prev = -1; - for htlc in holder_commitment_tx.trust().htlcs().iter() { + 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; } @@ -3132,10 +3132,10 @@ impl ChannelMonitorImpl { 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()); + 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().htlcs().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)); @@ -3526,7 +3526,7 @@ impl ChannelMonitorImpl { let counterparty_node_id = self.counterparty_node_id; let commitment_txid = commitment_tx.compute_txid(); debug_assert_eq!(self.funding.current_holder_commitment.tx.trust().txid(), commitment_txid); - let pending_htlcs = self.funding.current_holder_commitment.tx.trust().htlcs().to_vec(); + let pending_htlcs = self.funding.current_holder_commitment.tx.trust().nondust_htlcs().to_vec(); let channel_value_satoshis = self.funding.channel_parameters.channel_value_satoshis; let commitment_tx_fee_satoshis = channel_value_satoshis - commitment_tx.output.iter().fold(0u64, |sum, output| sum + output.value.to_sat()); @@ -3973,7 +3973,7 @@ impl ChannelMonitorImpl { fn get_broadcasted_holder_claims( &self, holder_tx: &HolderCommitmentTransaction, conf_height: u32, ) -> (Vec, Option<(ScriptBuf, PublicKey, RevocationKey)>) { - let mut claim_requests = Vec::with_capacity(holder_tx.htlcs().len()); + let mut claim_requests = Vec::with_capacity(holder_tx.nondust_htlcs().len()); let tx = holder_tx.trust(); let keys = tx.keys(); @@ -3985,7 +3985,7 @@ impl ChannelMonitorImpl { )); let txid = tx.txid(); - for htlc in holder_tx.htlcs() { + for htlc in holder_tx.nondust_htlcs() { if let Some(transaction_output_index) = htlc.transaction_output_index { let (htlc_output, counterparty_spendable_height) = if htlc.offered { let htlc_output = HolderHTLCOutput::build_offered( @@ -4020,9 +4020,9 @@ impl ChannelMonitorImpl { // Returns holder HTLC outputs to watch and react to in case of spending. fn get_broadcasted_holder_watch_outputs(&self, holder_tx: &HolderCommitmentTransaction) -> Vec<(u32, TxOut)> { - let mut watch_outputs = Vec::with_capacity(holder_tx.htlcs().len()); + let mut watch_outputs = Vec::with_capacity(holder_tx.nondust_htlcs().len()); let tx = holder_tx.trust(); - for htlc in holder_tx.htlcs() { + for htlc in holder_tx.nondust_htlcs() { if let Some(transaction_output_index) = htlc.transaction_output_index { watch_outputs.push(( transaction_output_index, @@ -4115,7 +4115,7 @@ impl ChannelMonitorImpl { let txid = self.funding.current_holder_commitment.tx.trust().txid(); log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}", txid); let mut outpoint = BitcoinOutPoint { txid, vout: 0 }; - for htlc in self.funding.current_holder_commitment.tx.htlcs() { + for htlc in self.funding.current_holder_commitment.tx.nondust_htlcs() { if let Some(vout) = htlc.transaction_output_index { outpoint.vout = vout; self.onchain_tx_handler.abandon_claim(&outpoint); @@ -4129,7 +4129,7 @@ impl ChannelMonitorImpl { if txid != *confirmed_commitment_txid { log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}", txid); let mut outpoint = BitcoinOutPoint { txid, vout: 0 }; - for htlc in prev_holder_commitment.tx.htlcs() { + for htlc in prev_holder_commitment.tx.nondust_htlcs() { if let Some(vout) = htlc.transaction_output_index { outpoint.vout = vout; self.onchain_tx_handler.abandon_claim(&outpoint); @@ -4155,7 +4155,7 @@ impl ChannelMonitorImpl { if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() { return holder_transactions; } - for htlc in self.funding.current_holder_commitment.tx.htlcs() { + for htlc in self.funding.current_holder_commitment.tx.nondust_htlcs() { if let Some(vout) = htlc.transaction_output_index { let preimage = if !htlc.offered { if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(preimage.clone()) } else { diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 5ec0713bae6..28764782a23 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1205,7 +1205,7 @@ impl OnchainTxHandler { if trusted_tx.txid() != outp.txid { return None; } - let (htlc_idx, htlc) = trusted_tx.htlcs().iter().enumerate() + let (htlc_idx, htlc) = trusted_tx.nondust_htlcs().iter().enumerate() .find(|(_, htlc)| htlc.transaction_output_index.unwrap() == outp.vout) .unwrap(); let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx]; @@ -1248,7 +1248,7 @@ impl OnchainTxHandler { if outp.txid != trusted_tx.txid() { return None; } - trusted_tx.htlcs().iter().enumerate() + trusted_tx.nondust_htlcs().iter().enumerate() .find(|(_, htlc)| if let Some(output_index) = htlc.transaction_output_index { output_index == outp.vout } else { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 93fd78fbd32..44cc5d21c07 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -1430,7 +1430,7 @@ pub struct CommitmentTransaction { feerate_per_kw: u32, // The set of non-dust HTLCs included in the commitment. They must be sorted in increasing // output index order. - htlcs: Vec, + nondust_htlcs: Vec, // Note that on upgrades, some features of existing outputs may be missed. channel_type_features: ChannelTypeFeatures, // A cache of the parties' pubkeys required to construct the transaction, see doc for trust() @@ -1446,7 +1446,7 @@ impl PartialEq for CommitmentTransaction { self.to_broadcaster_value_sat == o.to_broadcaster_value_sat && self.to_countersignatory_value_sat == o.to_countersignatory_value_sat && self.feerate_per_kw == o.feerate_per_kw && - self.htlcs == o.htlcs && + self.nondust_htlcs == o.nondust_htlcs && self.channel_type_features == o.channel_type_features && self.keys == o.keys; if eq { @@ -1468,7 +1468,7 @@ impl Writeable for CommitmentTransaction { (6, self.feerate_per_kw, required), (8, self.keys, required), (10, self.built, required), - (12, self.htlcs, required_vec), + (12, self.nondust_htlcs, required_vec), (14, legacy_deserialization_prevention_marker, option), (15, self.channel_type_features, required), }); @@ -1486,7 +1486,7 @@ impl Readable for CommitmentTransaction { (6, feerate_per_kw, required), (8, keys, required), (10, built, required), - (12, htlcs, required_vec), + (12, nondust_htlcs, required_vec), (14, _legacy_deserialization_prevention_marker, (option, explicit_type: ())), (15, channel_type_features, option), }); @@ -1503,7 +1503,7 @@ impl Readable for CommitmentTransaction { feerate_per_kw: feerate_per_kw.0.unwrap(), keys: keys.0.unwrap(), built: built.0.unwrap(), - htlcs, + nondust_htlcs, channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()) }) } @@ -1526,7 +1526,7 @@ impl CommitmentTransaction { let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx); // Sort outputs and populate output indices while keeping track of the auxiliary data - let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters); + let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters); let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); @@ -1537,7 +1537,7 @@ impl CommitmentTransaction { to_countersignatory_value_sat, to_broadcaster_delay: Some(channel_parameters.contest_delay()), feerate_per_kw, - htlcs, + nondust_htlcs, channel_type_features: channel_parameters.channel_type_features().clone(), keys, built: BuiltCommitmentTransaction { @@ -1558,7 +1558,7 @@ impl CommitmentTransaction { fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> BuiltCommitmentTransaction { let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters); - let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect(); + let mut htlcs_with_aux = self.nondust_htlcs.iter().map(|h| (h.clone(), ())).collect(); let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); @@ -1653,7 +1653,7 @@ impl CommitmentTransaction { } } - let mut htlcs = Vec::with_capacity(htlcs_with_aux.len()); + let mut nondust_htlcs = Vec::with_capacity(htlcs_with_aux.len()); for (htlc, _) in htlcs_with_aux { let script = get_htlc_redeemscript(htlc, channel_type, keys); let txout = TxOut { @@ -1683,11 +1683,11 @@ impl CommitmentTransaction { for (idx, out) in txouts.drain(..).enumerate() { if let Some(htlc) = out.1 { htlc.transaction_output_index = Some(idx as u32); - htlcs.push(htlc.clone()); + nondust_htlcs.push(htlc.clone()); } outputs.push(out.0); } - (outputs, htlcs) + (outputs, nondust_htlcs) } fn internal_build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec) { @@ -1746,8 +1746,8 @@ impl CommitmentTransaction { /// /// This is not exported to bindings users as we cannot currently convert Vec references to/from C, though we should /// expose a less effecient version which creates a Vec of references in the future. - pub fn htlcs(&self) -> &Vec { - &self.htlcs + pub fn nondust_htlcs(&self) -> &Vec { + &self.nondust_htlcs } /// Trust our pre-built transaction and derived transaction creation public keys. @@ -1831,10 +1831,10 @@ impl<'a> TrustedCommitmentTransaction<'a> { let inner = self.inner; let keys = &inner.keys; let txid = inner.built.txid; - let mut ret = Vec::with_capacity(inner.htlcs.len()); + let mut ret = Vec::with_capacity(inner.nondust_htlcs.len()); let holder_htlc_key = derive_private_key(secp_ctx, &inner.keys.per_commitment_point, htlc_base_key); - for this_htlc in inner.htlcs.iter() { + for this_htlc in inner.nondust_htlcs.iter() { assert!(this_htlc.transaction_output_index.is_some()); let htlc_tx = build_htlc_transaction(&txid, inner.feerate_per_kw, channel_parameters.contest_delay(), &this_htlc, &self.channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key); @@ -1852,7 +1852,7 @@ impl<'a> TrustedCommitmentTransaction<'a> { preimage: &Option, ) -> Transaction { let keys = &self.inner.keys; - let this_htlc = &self.inner.htlcs[htlc_index]; + let this_htlc = &self.inner.nondust_htlcs[htlc_index]; assert!(this_htlc.transaction_output_index.is_some()); // if we don't have preimage for an HTLC-Success, we can't generate an HTLC transaction. if !this_htlc.offered && preimage.is_none() { unreachable!(); } @@ -1874,7 +1874,7 @@ impl<'a> TrustedCommitmentTransaction<'a> { ) -> Witness { let keys = &self.inner.keys; let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys( - &self.inner.htlcs[htlc_index], &self.channel_type_features, &keys.broadcaster_htlc_key, + &self.inner.nondust_htlcs[htlc_index], &self.channel_type_features, &keys.broadcaster_htlc_key, &keys.countersignatory_htlc_key, &keys.revocation_key ); build_htlc_input_witness( diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index ebc0482aff5..98a40c559ac 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3794,13 +3794,13 @@ impl ChannelContext where SP::Target: SignerProvider { } } - if msg.htlc_signatures.len() != commitment_data.tx.htlcs().len() { - return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.htlcs().len()))); + if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() { + return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.nondust_htlcs().len()))); } let holder_keys = commitment_data.tx.trust().keys(); - let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.htlcs().len()); - let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.tx.htlcs().len()); + let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.nondust_htlcs().len()); + let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.tx.nondust_htlcs().len()); for (idx, (htlc, mut source_opt)) in htlcs_cloned.into_iter().enumerate() { if let Some(_) = htlc.transaction_output_index { let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.tx.feerate_per_kw(), @@ -6648,7 +6648,7 @@ impl FundedChannel where &self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, true, logger, ); - let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000; + let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.nondust_htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000; let holder_balance_msat = commitment_data.stats.local_balance_before_fee_anchors_msat - htlc_stats.outbound_holding_cell_msat; if holder_balance_msat < buffer_fee_msat + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { //TODO: auto-close after a number of failures? @@ -9095,7 +9095,7 @@ impl FundedChannel where && info.next_holder_htlc_id == self.context.next_holder_htlc_id && info.next_counterparty_htlc_id == self.context.next_counterparty_htlc_id && info.feerate == self.context.feerate_per_kw { - let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.htlcs().len(), self.funding.get_channel_type()) * 1000; + let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.nondust_htlcs().len(), self.funding.get_channel_type()) * 1000; assert_eq!(actual_fee, info.fee); } } @@ -9157,8 +9157,8 @@ impl FundedChannel where log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id()); let counterparty_keys = trusted_tx.keys(); - debug_assert_eq!(htlc_signatures.len(), trusted_tx.htlcs().len()); - for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.htlcs()) { + debug_assert_eq!(htlc_signatures.len(), trusted_tx.nondust_htlcs().len()); + for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.nondust_htlcs()) { log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}", encode::serialize_hex(&chan_utils::build_htlc_transaction(&trusted_tx.txid(), trusted_tx.feerate_per_kw(), funding.get_holder_selected_contest_delay(), htlc, funding.get_channel_type(), &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, funding.get_channel_type(), &counterparty_keys)), @@ -12371,10 +12371,10 @@ mod tests { counterparty_htlc_sigs.clear(); // Don't warn about excess mut for no-HTLC calls $({ let remote_signature = Signature::from_der(&>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap(); - per_htlc.push((commitment_tx.htlcs()[$htlc_idx].clone(), Some(remote_signature))); + per_htlc.push((commitment_tx.nondust_htlcs()[$htlc_idx].clone(), Some(remote_signature))); counterparty_htlc_sigs.push(remote_signature); })* - assert_eq!(commitment_tx.htlcs().len(), per_htlc.len()); + assert_eq!(commitment_tx.nondust_htlcs().len(), per_htlc.len()); let holder_commitment_tx = HolderCommitmentTransaction::new( commitment_tx.clone(), @@ -12397,7 +12397,7 @@ mod tests { log_trace!(logger, "verifying htlc {}", $htlc_idx); let remote_signature = Signature::from_der(&>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap(); - let ref htlc = commitment_tx.htlcs()[$htlc_idx]; + let ref htlc = commitment_tx.nondust_htlcs()[$htlc_idx]; let keys = commitment_tx.trust().keys(); let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw, chan.funding.get_counterparty_selected_contest_delay().unwrap(), diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index df79df6bab8..eb3d57e6dec 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -1364,8 +1364,8 @@ impl EcdsaChannelSigner for InMemorySigner { ); let commitment_txid = built_tx.txid; - let mut htlc_sigs = Vec::with_capacity(commitment_tx.htlcs().len()); - for htlc in commitment_tx.htlcs() { + let mut htlc_sigs = Vec::with_capacity(commitment_tx.nondust_htlcs().len()); + for htlc in commitment_tx.nondust_htlcs() { let holder_selected_contest_delay = channel_parameters.holder_selected_contest_delay; let chan_type = &channel_parameters.channel_type_features; let htlc_tx = chan_utils::build_htlc_transaction( From ea8533f388ad89baca6461327ea98553b1c91ce0 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Tue, 1 Apr 2025 15:59:59 +0000 Subject: [PATCH 3/5] Remove unnecessary clones of the HTLC-HTLCSource table --- lightning/src/ln/channel.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 98a40c559ac..98bc7473e99 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3762,7 +3762,6 @@ impl ChannelContext where SP::Target: SignerProvider { } bitcoin_tx.txid }; - let htlcs_cloned: Vec<_> = commitment_data.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect(); // If our counterparty updated the channel fee in this commitment transaction, check that // they can actually afford the new fee now. @@ -3800,8 +3799,8 @@ 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(htlcs_cloned.len() - commitment_data.tx.nondust_htlcs().len()); - for (idx, (htlc, mut source_opt)) in htlcs_cloned.into_iter().enumerate() { + 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() { 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(), @@ -3818,13 +3817,13 @@ impl ChannelContext where SP::Target: SignerProvider { } if htlc.offered { if let Some(source) = source_opt.take() { - nondust_htlc_sources.push(source); + nondust_htlc_sources.push(source.clone()); } else { panic!("Missing outbound HTLC source"); } } } else { - dust_htlcs.push((htlc, None, source_opt.take())); + dust_htlcs.push((htlc, None, source_opt.take().cloned())); } debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere"); } @@ -9033,10 +9032,10 @@ impl FundedChannel where let mut updates = Vec::with_capacity(self.pending_funding.len() + 1); for funding in core::iter::once(&self.funding).chain(self.pending_funding.iter()) { - let (mut htlcs_ref, counterparty_commitment_tx) = + let (htlcs_ref, counterparty_commitment_tx) = self.build_commitment_no_state_update(funding, logger); let htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)> = - htlcs_ref.drain(..).map(|(htlc, htlc_source)| (htlc, htlc_source.map(|source_ref| Box::new(source_ref.clone())))).collect(); + htlcs_ref.into_iter().map(|(htlc, htlc_source)| (htlc, htlc_source.map(|source_ref| Box::new(source_ref.clone())))).collect(); if self.pending_funding.is_empty() { // Soon, we will switch this to `LatestCounterpartyCommitmentTX`, From 285f1b9978c152459956e2bac1cec0507f185d85 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 3 Apr 2025 19:17:02 +0000 Subject: [PATCH 4/5] Require all HTLC success states to hold their corresponding preimage This allows us to DRY the code that calculates the `value_to_self_msat_offset` in `ChannelContext::build_commitment_stats`. HTLC success states have held their corresponding preimage since 0.0.105, and the release notes of 0.1 already require users running 0.0.123 and earlier to resolve their HTLCs before upgrading to 0.1. So this change is fully compatible with existing upgrade paths to the yet-to-be-shipped 0.2 release. --- lightning/src/ln/channel.rs | 56 ++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 98bc7473e99..dff645eca28 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -364,9 +364,12 @@ impl OutboundHTLCState { fn preimage(&self) -> Option { match self { - OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(p)) | - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(p)) | - OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(p)) => p.as_ref().copied(), + OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(preimage)) | + OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(preimage)) | + OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(preimage)) => { + debug_assert!(preimage.is_some()); + *preimage + }, _ => None, } } @@ -375,7 +378,10 @@ impl OutboundHTLCState { #[derive(Clone)] #[cfg_attr(test, derive(Debug, PartialEq))] enum OutboundHTLCOutcome { - /// LDK version 0.0.105+ will always fill in the preimage here. + /// Except briefly during deserialization and state transitions between success states, + /// we require all success states to hold their corresponding preimage. + /// We started always filling in the preimages here in 0.0.105, and the requirement + /// that the preimages always be filled in was added in 0.2. Success(Option), Failure(HTLCFailReason), } @@ -3876,7 +3882,7 @@ impl ChannelContext where SP::Target: SignerProvider { } remote_htlc_total_msat += htlc.amount_msat; } else { - if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_preimage)) = htlc.state { + if htlc.state.preimage().is_some() { value_to_self_msat_offset += htlc.amount_msat as i64; } } @@ -3889,10 +3895,7 @@ impl ChannelContext where SP::Target: SignerProvider { } local_htlc_total_msat += htlc.amount_msat; } else { - if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) | - OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) | - OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) - = htlc.state { + if htlc.state.preimage().is_some() { value_to_self_msat_offset -= htlc.amount_msat as i64; } } @@ -5801,6 +5804,7 @@ impl FundedChannel where #[inline] fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, check_preimage: Option, fail_reason: Option) -> Result<&OutboundHTLCOutput, ChannelError> { assert!(!(check_preimage.is_some() && fail_reason.is_some()), "cannot fail while we have a preimage"); + assert!(!(check_preimage.is_none() && fail_reason.is_none()), "success states must hold their corresponding preimage"); for htlc in self.context.pending_outbound_htlcs.iter_mut() { if htlc.htlc_id == htlc_id { let outcome = match check_preimage { @@ -10667,6 +10671,7 @@ impl Writeable for FundedChannel where SP::Target: SignerProvider &OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref outcome) => { 3u8.write(writer)?; if let OutboundHTLCOutcome::Success(preimage) = outcome { + debug_assert!(preimage.is_some(), "success states must hold their corresponding preimage"); preimages.push(preimage); } let reason: Option<&HTLCFailReason> = outcome.into(); @@ -10675,6 +10680,7 @@ impl Writeable for FundedChannel where SP::Target: SignerProvider &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref outcome) => { 4u8.write(writer)?; if let OutboundHTLCOutcome::Success(preimage) = outcome { + debug_assert!(preimage.is_some(), "success states must hold their corresponding preimage"); preimages.push(preimage); } let reason: Option<&HTLCFailReason> = outcome.into(); @@ -11169,7 +11175,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel // only, so we default to that if none was written. let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key()); let mut channel_creation_height = 0u32; - let mut preimages_opt: Option>> = None; + let mut preimages: Vec> = Vec::new(); // If we read an old Channel, for simplicity we just treat it as "we never sent an // AnnouncementSignatures" which implies we'll re-send it on reconnect, but that's fine. @@ -11223,7 +11229,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel (10, monitor_pending_update_adds, option), // Added in 0.0.122 (11, monitor_pending_finalized_fulfills, optional_vec), (13, channel_creation_height, required), - (15, preimages_opt, optional_vec), + (15, preimages, required_vec), // The preimages transitioned from optional to required in 0.2 (17, announcement_sigs_state, required), (19, latest_inbound_scid_alias, option), (21, outbound_scid_alias, required), @@ -11251,23 +11257,21 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel let holder_signer = signer_provider.derive_channel_signer(channel_keys_id); - if let Some(preimages) = preimages_opt { - let mut iter = preimages.into_iter(); - for htlc in pending_outbound_htlcs.iter_mut() { - match &htlc.state { - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(None)) => { - htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?)); - } - OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(None)) => { - htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?)); - } - _ => {} + let mut iter = preimages.into_iter(); + for htlc in pending_outbound_htlcs.iter_mut() { + match &htlc.state { + OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(None)) => { + htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?)); } + OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(None)) => { + htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?)); + } + _ => {} } - // We expect all preimages to be consumed above - if iter.next().is_some() { - return Err(DecodeError::InvalidValue); - } + } + // We expect all preimages to be consumed above + if iter.next().is_some() { + return Err(DecodeError::InvalidValue); } let chan_features = channel_type.unwrap(); From f4a3cc336a4474a6a187c14464f00a12354c0a58 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Fri, 28 Mar 2025 19:30:32 +0000 Subject: [PATCH 5/5] Remove exclusive reference and generic from `CommitmentTransaction` API This commit reworks how channel is told about which HTLCs were assigned which index upon the build of a `CommitmentTransaction`. Previously, `CommitmentTransaction` was given an exclusive reference to channel's HTLC-source table, and `CommitmentTransaction` populated the transaction output indices in that table via this exclusive reference. As a result, the public API of `CommitmentTransaction` included a generic parameter, and an exclusive reference. We remove both of these in preparation for the upcoming `TxBuilder` trait. This cleans up the API, and makes it more bindings-friendly. Henceforth, channel populates the HTLC-source table via a brute-force search of each htlc in `CommitmentTransaction`. This is an O(n^2) operation, but n is small enough that we ignore the performance hit. --- lightning/src/chain/channelmonitor.rs | 50 +++++----- lightning/src/chain/onchaintx.rs | 15 ++- lightning/src/ln/chan_utils.rs | 129 ++++++++++++++------------ lightning/src/ln/channel.rs | 58 ++++++++---- lightning/src/ln/functional_tests.rs | 16 ++-- 5 files changed, 153 insertions(+), 115 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index b4c4bceec4a..ac3a1f66030 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3598,7 +3598,7 @@ impl ChannelMonitorImpl { to_broadcaster_value, to_countersignatory_value, )| { - let htlc_outputs = vec![]; + let nondust_htlcs = vec![]; let commitment_tx = self.build_counterparty_commitment_tx( INITIAL_COMMITMENT_NUMBER, @@ -3606,7 +3606,7 @@ impl ChannelMonitorImpl { to_broadcaster_value, to_countersignatory_value, feerate_per_kw, - htlc_outputs, + &nondust_htlcs, ); // Take the opportunity to populate this recently introduced field self.initial_counterparty_commitment_tx = Some(commitment_tx.clone()); @@ -3619,11 +3619,11 @@ impl ChannelMonitorImpl { fn build_counterparty_commitment_tx( &self, commitment_number: u64, their_per_commitment_point: &PublicKey, to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32, - mut nondust_htlcs: Vec<(HTLCOutputInCommitment, Option>)> + nondust_htlcs: &Vec ) -> CommitmentTransaction { let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable(); - CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, their_per_commitment_point, - to_broadcaster_value, to_countersignatory_value, feerate_per_kw, &mut nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx) + CommitmentTransaction::new(commitment_number, their_per_commitment_point, + to_broadcaster_value, to_countersignatory_value, feerate_per_kw, nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx) } fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec { @@ -3639,12 +3639,12 @@ impl ChannelMonitorImpl { to_countersignatory_value_sat: Some(to_countersignatory_value) } => { let nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| { - htlc.transaction_output_index.map(|_| (htlc.clone(), None)) + htlc.transaction_output_index.map(|_| htlc).cloned() }).collect::>(); let commitment_tx = self.build_counterparty_commitment_tx(commitment_number, &their_per_commitment_point, to_broadcaster_value, - to_countersignatory_value, feerate_per_kw, nondust_htlcs); + to_countersignatory_value, feerate_per_kw, &nondust_htlcs); debug_assert_eq!(commitment_tx.trust().txid(), commitment_txid); @@ -5595,13 +5595,13 @@ mod tests { { let mut res = Vec::new(); for (idx, preimage) in $preimages_slice.iter().enumerate() { - res.push((HTLCOutputInCommitment { + res.push(HTLCOutputInCommitment { offered: true, amount_msat: 0, cltv_expiry: 0, payment_hash: preimage.1.clone(), transaction_output_index: Some(idx as u32), - }, ())); + }); } res } @@ -5609,7 +5609,7 @@ mod tests { } macro_rules! preimages_slice_to_htlc_outputs { ($preimages_slice: expr) => { - preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|(htlc, _)| (htlc, None)).collect() + preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|htlc| (htlc, None)).collect() } } let dummy_sig = crate::crypto::utils::sign(&secp_ctx, @@ -5665,15 +5665,17 @@ mod tests { let best_block = BestBlock::from_network(Network::Testnet); let monitor = ChannelMonitor::new( Secp256k1::new(), keys, Some(shutdown_script.into_inner()), 0, &ScriptBuf::new(), - &channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &mut Vec::new()), + &channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &Vec::new()), best_block, dummy_key, channel_id, ); - let mut htlcs = preimages_slice_to_htlcs!(preimages[0..10]); - let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs); + let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..10]); + let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &nondust_htlcs); + // These htlcs now have their output indices assigned + let nondust_htlcs = dummy_commitment_tx.nondust_htlcs(); monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(), - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect()); + nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect()); monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"1").to_byte_array()), preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger); monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"2").to_byte_array()), @@ -5708,10 +5710,12 @@ mod tests { // Now update holder commitment tx info, pruning only element 18 as we still care about the // previous commitment tx's preimages too - let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]); - let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs); + let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..5]); + let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &nondust_htlcs); + // These htlcs now have their output indices assigned + let nondust_htlcs = dummy_commitment_tx.nondust_htlcs(); monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(), - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect()); + nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect()); secret[0..32].clone_from_slice(&>::from_hex("2273e227a5b7449b6e70f1fb4652864038b1cbf9cd7c043a7d6456b7fc275ad8").unwrap()); monitor.provide_secret(281474976710653, secret.clone()).unwrap(); assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 12); @@ -5719,10 +5723,12 @@ mod tests { test_preimages_exist!(&preimages[18..20], monitor); // But if we do it again, we'll prune 5-10 - let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]); - let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs); - monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx, - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect()); + let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..3]); + let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &nondust_htlcs); + // These htlcs now have their output indices assigned + let nondust_htlcs = dummy_commitment_tx.nondust_htlcs(); + monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(), + nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect()); secret[0..32].clone_from_slice(&>::from_hex("27cddaa5624534cb6cb9d7da077cf2b22ab21e9b506fd4998a51d54502e99116").unwrap()); monitor.provide_secret(281474976710652, secret.clone()).unwrap(); assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 5); @@ -5917,7 +5923,7 @@ mod tests { let best_block = BestBlock::from_network(Network::Testnet); let monitor = ChannelMonitor::new( Secp256k1::new(), keys, Some(shutdown_script.into_inner()), 0, &ScriptBuf::new(), - &channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &mut Vec::new()), + &channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &Vec::new()), best_block, dummy_key, channel_id, ); diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 28764782a23..be6608b57e7 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1359,29 +1359,28 @@ mod tests { // Create an OnchainTxHandler for a commitment containing HTLCs with CLTV expiries of 0, 1, // and 2 blocks. - let mut htlcs = Vec::new(); + let mut nondust_htlcs = Vec::new(); for i in 0..3 { let preimage = PaymentPreimage([i; 32]); let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array()); - htlcs.push(( + nondust_htlcs.push( HTLCOutputInCommitment { offered: true, amount_msat: 10000, cltv_expiry: i as u32, payment_hash: hash, transaction_output_index: Some(i as u32), - }, - (), - )); + } + ); } - let holder_commit = HolderCommitmentTransaction::dummy(1000000, &mut htlcs); + let holder_commit = HolderCommitmentTransaction::dummy(1000000, &nondust_htlcs); let mut tx_handler = OnchainTxHandler::new( 1000000, [0; 32], ScriptBuf::new(), signer, chan_params, - holder_commit, + holder_commit.clone(), secp_ctx, ); @@ -1400,7 +1399,7 @@ mod tests { // Request claiming of each HTLC on the holder's commitment, with current block height 1. let holder_commit_txid = tx_handler.get_unsigned_holder_commitment_tx().compute_txid(); let mut requests = Vec::new(); - for (htlc, _) in htlcs { + for htlc in holder_commit.nondust_htlcs() { requests.push(PackageTemplate::build_package( holder_commit_txid, htlc.transaction_output_index.unwrap(), diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 44cc5d21c07..3404075b3a0 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -614,6 +614,13 @@ impl HTLCOutputInCommitment { pub const fn to_bitcoin_amount(&self) -> Amount { Amount::from_sat(self.amount_msat / 1000) } + + pub(crate) fn is_data_equal(&self, other: &HTLCOutputInCommitment) -> bool { + self.offered == other.offered + && self.amount_msat == other.amount_msat + && self.cltv_expiry == other.cltv_expiry + && self.payment_hash == other.payment_hash + } } impl_writeable_tlv_based!(HTLCOutputInCommitment, { @@ -1182,7 +1189,7 @@ impl_writeable_tlv_based!(HolderCommitmentTransaction, { impl HolderCommitmentTransaction { #[cfg(test)] - pub fn dummy(channel_value_satoshis: u64, htlcs: &mut Vec<(HTLCOutputInCommitment, ())>) -> Self { + pub fn dummy(channel_value_satoshis: u64, nondust_htlcs: &Vec) -> 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_digest([42; 32]), &SecretKey::from_slice(&[42; 32]).unwrap()); @@ -1205,11 +1212,10 @@ impl HolderCommitmentTransaction { channel_value_satoshis, }; let mut counterparty_htlc_sigs = Vec::new(); - for _ in 0..htlcs.len() { + for _ in 0..nondust_htlcs.len() { counterparty_htlc_sigs.push(dummy_sig); } - let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, &dummy_key, 0, 0, 0, htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx); - htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index); + let inner = CommitmentTransaction::new(0, &dummy_key, 0, 0, 0, nondust_htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx); HolderCommitmentTransaction { inner, counterparty_sig: dummy_sig, @@ -1510,25 +1516,16 @@ impl Readable for CommitmentTransaction { } impl CommitmentTransaction { - /// Construct an object of the class while assigning transaction output indices to HTLCs. - /// - /// Populates HTLCOutputInCommitment.transaction_output_index in htlcs_with_aux. - /// - /// The generic T allows the caller to match the HTLC output index with auxiliary data. - /// This auxiliary data is not stored in this object. - /// /// Only include HTLCs that are above the dust limit for the channel. - /// - /// This is not exported to bindings users due to the generic though we likely should expose a version without - pub fn new_with_auxiliary_htlc_data(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1) -> CommitmentTransaction { + /// The broadcaster and countersignatory amounts MUST be EITHER 0 or ABOVE DUST. + pub fn new(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, nondust_htlcs: &Vec, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1) -> CommitmentTransaction { let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat); let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat); let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx); - // Sort outputs and populate output indices while keeping track of the auxiliary data - let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters); + let (outputs, nondust_htlcs) = Self::build_outputs_and_htlcs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters); - let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters); + let (obscured_commitment_transaction_number, txins) = Self::build_inputs(commitment_number, channel_parameters); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); let txid = transaction.compute_txid(); CommitmentTransaction { @@ -1555,11 +1552,11 @@ impl CommitmentTransaction { self } - fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> BuiltCommitmentTransaction { - let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters); + fn rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> BuiltCommitmentTransaction { + let (obscured_commitment_transaction_number, txins) = Self::build_inputs(self.commitment_number, channel_parameters); - let mut htlcs_with_aux = self.nondust_htlcs.iter().map(|h| (h.clone(), ())).collect(); - let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters); + let txout_htlc_pairs: Vec<(TxOut, Option<&HTLCOutputInCommitment>)> = Self::build_txout_htlc_pairs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &self.nondust_htlcs, channel_parameters); + let outputs = txout_htlc_pairs.into_iter().map(|(output, _)| output).collect(); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); let txid = transaction.compute_txid(); @@ -1579,25 +1576,44 @@ impl CommitmentTransaction { } } - // Builds the set of outputs for the commitment transaction. There will be an output per - // non-dust HTLC, one output for the holder, another for the counterparty, and possibly anchor - // output(s). + // This function splits the txout-HTLC pairs into two vectors, one for the txouts, and another for the HTLCs. + // As it pushes each HTLC into the HTLC vector, this function also populates the output index of that HTLC. + fn build_outputs_and_htlcs(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: &Vec, channel_parameters: &DirectedChannelTransactionParameters) -> (Vec, Vec) { + let txout_htlc_pairs: Vec<(TxOut, Option<&HTLCOutputInCommitment>)> = Self::build_txout_htlc_pairs(keys, to_broadcaster_value_sat, to_countersignatory_value_sat, &nondust_htlcs, channel_parameters); + let mut outputs: Vec = Vec::with_capacity(txout_htlc_pairs.len()); + let mut nondust_htlcs: Vec = Vec::with_capacity(txout_htlc_pairs.len()); + for (idx, (output, htlc)) in txout_htlc_pairs.into_iter().enumerate() { + if let Some(htlc) = htlc { + let mut htlc = htlc.clone(); + htlc.transaction_output_index = Some(idx as u32); + nondust_htlcs.push(htlc); + } + outputs.push(output); + } + (outputs, nondust_htlcs) + } + + // Builds the set of outputs for the commitment transaction. Each HTLC output is paired with its corresponding data. + // There will be an output for all passed HTLCs, since they are all non-dust. + // Then depending on amounts, and features, this function assigns holder, counterparty, and anchor outputs. // // The set of outputs are sorted according to BIP-69 ordering, guaranteeing that HTLCs are // returned in increasing output index order. // // This is used in two cases: - // - initial sorting of outputs / HTLCs in the constructor, in which case T is auxiliary data the - // caller needs to have sorted together with the HTLCs so it can keep track of the output index - // - building of a bitcoin transaction during a verify() call, in which case T is just () - fn internal_build_outputs(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> (Vec, Vec) { + // - initial sorting of outputs / HTLCs in the constructor, via `build_outputs_and_htlcs` + // - building of a bitcoin transaction during a verify() call + // + // Note that the HTLC data in the second column of the returned table is NOT populated with the + // output indices of the HTLCs; this is done in `build_outputs_and_htlcs`. + fn build_txout_htlc_pairs<'a>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: &'a Vec, channel_parameters: &DirectedChannelTransactionParameters) -> Vec<(TxOut, Option<&'a HTLCOutputInCommitment>)> { let countersignatory_payment_point = &channel_parameters.countersignatory_pubkeys().payment_point; let countersignatory_funding_key = &channel_parameters.countersignatory_pubkeys().funding_pubkey; let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey; let channel_type = channel_parameters.channel_type_features(); let contest_delay = channel_parameters.contest_delay(); - let mut txouts: Vec<(TxOut, Option<&mut HTLCOutputInCommitment>)> = Vec::new(); + let mut txout_htlc_pairs: Vec<(TxOut, Option<&HTLCOutputInCommitment>)> = Vec::with_capacity(nondust_htlcs.len() + 4); if to_countersignatory_value_sat > Amount::ZERO { let script = if channel_type.supports_anchors_zero_fee_htlc_tx() { @@ -1605,7 +1621,7 @@ impl CommitmentTransaction { } else { ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_payment_point.serialize()).into()) }; - txouts.push(( + txout_htlc_pairs.push(( TxOut { script_pubkey: script.clone(), value: to_countersignatory_value_sat, @@ -1620,7 +1636,7 @@ impl CommitmentTransaction { contest_delay, &keys.broadcaster_delayed_payment_key, ); - txouts.push(( + txout_htlc_pairs.push(( TxOut { script_pubkey: redeem_script.to_p2wsh(), value: to_broadcaster_value_sat, @@ -1630,9 +1646,9 @@ impl CommitmentTransaction { } if channel_type.supports_anchors_zero_fee_htlc_tx() { - if to_broadcaster_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() { + if to_broadcaster_value_sat > Amount::ZERO || !nondust_htlcs.is_empty() { let anchor_script = get_keyed_anchor_redeemscript(broadcaster_funding_key); - txouts.push(( + txout_htlc_pairs.push(( TxOut { script_pubkey: anchor_script.to_p2wsh(), value: Amount::from_sat(ANCHOR_OUTPUT_VALUE_SATOSHI), @@ -1641,9 +1657,9 @@ impl CommitmentTransaction { )); } - if to_countersignatory_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() { + if to_countersignatory_value_sat > Amount::ZERO || !nondust_htlcs.is_empty() { let anchor_script = get_keyed_anchor_redeemscript(countersignatory_funding_key); - txouts.push(( + txout_htlc_pairs.push(( TxOut { script_pubkey: anchor_script.to_p2wsh(), value: Amount::from_sat(ANCHOR_OUTPUT_VALUE_SATOSHI), @@ -1653,44 +1669,35 @@ impl CommitmentTransaction { } } - let mut nondust_htlcs = Vec::with_capacity(htlcs_with_aux.len()); - for (htlc, _) in htlcs_with_aux { + for htlc in nondust_htlcs { let script = get_htlc_redeemscript(htlc, channel_type, keys); let txout = TxOut { script_pubkey: script.to_p2wsh(), value: htlc.to_bitcoin_amount(), }; - txouts.push((txout, Some(htlc))); + txout_htlc_pairs.push((txout, Some(htlc))); } // Sort output in BIP-69 order (amount, scriptPubkey). Tie-breaks based on HTLC // CLTV expiration height. - sort_outputs(&mut txouts, |a, b| { - if let &Some(ref a_htlcout) = a { - if let &Some(ref b_htlcout) = b { - a_htlcout.cltv_expiry.cmp(&b_htlcout.cltv_expiry) + sort_outputs(&mut txout_htlc_pairs, |a: &Option<&HTLCOutputInCommitment>, b: &Option<&HTLCOutputInCommitment>| { + if let Some(a) = a { + if let Some(b) = b { + a.cltv_expiry.cmp(&b.cltv_expiry) // Note that due to hash collisions, we have to have a fallback comparison // here for fuzzing mode (otherwise at least chanmon_fail_consistency // may fail)! - .then(a_htlcout.payment_hash.0.cmp(&b_htlcout.payment_hash.0)) + .then(a.payment_hash.cmp(&b.payment_hash)) // For non-HTLC outputs, if they're copying our SPK we don't really care if we // close the channel due to mismatches - they're doing something dumb: } else { cmp::Ordering::Equal } } else { cmp::Ordering::Equal } }); - let mut outputs = Vec::with_capacity(txouts.len()); - for (idx, out) in txouts.drain(..).enumerate() { - if let Some(htlc) = out.1 { - htlc.transaction_output_index = Some(idx as u32); - nondust_htlcs.push(htlc.clone()); - } - outputs.push(out.0); - } - (outputs, nondust_htlcs) + txout_htlc_pairs } - fn internal_build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec) { + fn build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec) { let broadcaster_pubkeys = channel_parameters.broadcaster_pubkeys(); let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys(); let commitment_transaction_number_obscure_factor = get_commitment_transaction_number_obscure_factor( @@ -1773,7 +1780,7 @@ impl CommitmentTransaction { if keys != self.keys { return Err(()); } - let tx = self.internal_rebuild_transaction(&keys, channel_parameters); + let tx = self.rebuild_transaction(&keys, channel_parameters); if self.built.transaction != tx.transaction || self.built.txid != tx.txid { return Err(()); } @@ -1998,7 +2005,7 @@ mod tests { commitment_number: u64, per_commitment_point: PublicKey, feerate_per_kw: u32, - htlcs_with_aux: Vec<(HTLCOutputInCommitment, ())>, + nondust_htlcs: Vec, channel_parameters: ChannelTransactionParameters, counterparty_pubkeys: ChannelPublicKeys, secp_ctx: Secp256k1::, @@ -2026,13 +2033,13 @@ mod tests { channel_type_features: ChannelTypeFeatures::only_static_remote_key(), channel_value_satoshis: 3000, }; - let htlcs_with_aux = Vec::new(); + let nondust_htlcs = Vec::new(); Self { commitment_number: 0, per_commitment_point, feerate_per_kw: 1, - htlcs_with_aux, + nondust_htlcs, channel_parameters, counterparty_pubkeys, secp_ctx, @@ -2040,9 +2047,9 @@ mod tests { } fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction { - CommitmentTransaction::new_with_auxiliary_htlc_data( + CommitmentTransaction::new( self.commitment_number, &self.per_commitment_point, to_broadcaster_sats, to_countersignatory_sats, self.feerate_per_kw, - &mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx + &self.nondust_htlcs, &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx ) } } @@ -2088,7 +2095,7 @@ mod tests { // Generate broadcaster output and received and offered HTLC outputs, w/o anchors builder.channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key(); - builder.htlcs_with_aux = vec![(received_htlc.clone(), ()), (offered_htlc.clone(), ())]; + builder.nondust_htlcs = vec![received_htlc.clone(), offered_htlc.clone()]; let tx = builder.build(3000, 0); let keys = tx.trust().keys(); assert_eq!(tx.built.transaction.output.len(), 3); @@ -2101,7 +2108,7 @@ mod tests { // Generate broadcaster output and received and offered HTLC outputs, with anchors builder.channel_parameters.channel_type_features = ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(); - builder.htlcs_with_aux = vec![(received_htlc.clone(), ()), (offered_htlc.clone(), ())]; + builder.nondust_htlcs = vec![received_htlc.clone(), offered_htlc.clone()]; let tx = builder.build(3000, 0); assert_eq!(tx.built.transaction.output.len(), 5); assert_eq!(tx.built.transaction.output[2].script_pubkey, get_htlc_redeemscript(&received_htlc, &ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(), &keys).to_p2wsh()); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index dff645eca28..aae37977262 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3968,9 +3968,9 @@ impl ChannelContext where SP::Target: SignerProvider { remote_balance_before_fee_anchors_msat } = stats; - let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new(); let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); - let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); + let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); + let mut nondust_htlcs: Vec = Vec::with_capacity(num_htlcs); log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...", commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number), @@ -3993,12 +3993,12 @@ impl ChannelContext where SP::Target: SignerProvider { macro_rules! add_htlc_output { ($htlc: expr, $outbound: expr, $source: expr) => { let htlc_in_tx = get_htlc_in_commitment!($htlc, $outbound == local); + htlcs_included.push((htlc_in_tx.clone(), $source)); if $htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { log_trace!(logger, " ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat); - included_dust_htlcs.push((htlc_in_tx, $source)); } else { log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat); - included_non_dust_htlcs.push((htlc_in_tx, $source)); + nondust_htlcs.push(htlc_in_tx); } } } @@ -4059,19 +4059,45 @@ impl ChannelContext where SP::Target: SignerProvider { let channel_parameters = if local { funding.channel_transaction_parameters.as_holder_broadcastable() } else { funding.channel_transaction_parameters.as_counterparty_broadcastable() }; - let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, - per_commitment_point, - to_broadcaster_value_sat, - to_countersignatory_value_sat, - feerate_per_kw, - &mut included_non_dust_htlcs, - &channel_parameters, - &self.secp_ctx, + let tx = CommitmentTransaction::new( + commitment_number, + per_commitment_point, + to_broadcaster_value_sat, + to_countersignatory_value_sat, + feerate_per_kw, + &nondust_htlcs, + &channel_parameters, + &self.secp_ctx, ); - let mut htlcs_included = included_non_dust_htlcs; - // The unwrap is safe, because all non-dust HTLCs have been assigned an output index - htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap()); - htlcs_included.append(&mut included_dust_htlcs); + + // This populates the htlc-source table with the indices from the htlcs in the commitment + // transaction. + // + // This brute-force search is O(n^2) over ~1k HTLCs in the worst case. This case is very + // rare (non-existent?) at the moment. + for nondust_htlc in tx.nondust_htlcs() { + let htlc = htlcs_included + .iter_mut() + .filter(|(htlc, _source)| htlc.transaction_output_index.is_none()) + .filter_map(|(htlc, _source)| { + if htlc.is_data_equal(nondust_htlc) { + Some(htlc) + } else { + None + } + }) + .nth(0) + .unwrap(); + htlc.transaction_output_index = Some(nondust_htlc.transaction_output_index.unwrap()); + } + htlcs_included.sort_unstable_by(|(htlc_a, _), (htlc_b, _)| { + match (htlc_a.transaction_output_index, htlc_b.transaction_output_index) { + // `None` is smaller than `Some`, but we want `Some` ordered before `None` in the vector + (None, Some(_)) => cmp::Ordering::Greater, + (Some(_), None) => cmp::Ordering::Less, + (l, r) => cmp::Ord::cmp(&l, &r), + } + }); CommitmentData { tx, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 4aaa1a3f31e..0e4d5384b4e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -746,14 +746,14 @@ pub fn test_update_fee_that_funder_cannot_afford() { let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = local_chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let local_chan_signer = local_chan.get_signer(); - let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![]; - let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( + let nondust_htlcs: Vec = vec![]; + let commitment_tx = CommitmentTransaction::new( INITIAL_COMMITMENT_NUMBER - 1, &remote_point, push_sats, channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000, non_buffer_feerate + 4, - &mut htlcs, + &nondust_htlcs, &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), &secp_ctx, ); @@ -831,8 +831,8 @@ pub fn test_update_fee_that_saturates_subs() { let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = local_chan_lock.channel_by_id.get(&chan_id).and_then(Channel::as_funded).unwrap(); let local_chan_signer = local_chan.get_signer(); - let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![]; - let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( + let nondust_htlcs: Vec = vec![]; + let commitment_tx = CommitmentTransaction::new( INITIAL_COMMITMENT_NUMBER, &remote_point, 8500, @@ -840,7 +840,7 @@ pub fn test_update_fee_that_saturates_subs() { // he will do a saturating subtraction of the total fees from node 0's balance. 0, FEERATE, - &mut htlcs, + &nondust_htlcs, &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), &secp_ctx, ); @@ -1565,13 +1565,13 @@ pub fn test_fee_spike_violation_fails_htlc() { let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = local_chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let local_chan_signer = local_chan.get_signer(); - let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( + let commitment_tx = CommitmentTransaction::new( commitment_number, &remote_point, 95000, local_chan_balance, feerate_per_kw, - &mut vec![(accepted_htlc_info, ())], + &vec![accepted_htlc_info], &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), &secp_ctx, );