From ca8c72803d345409edb9fc69e3261a474c15c098 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Tue, 25 Feb 2025 02:05:25 +0000 Subject: [PATCH 01/18] Provide built commitment transactions to `ChannelMonitor` Prior to this commit, `LatestCounterpartyCommitmentTXInfo` provided `ChannelMonitor` with the data it needed to build counterparty commitment transactions, instead of the fully built transaction. This was an unnecessary optimization, as the data still included all the htlcs. This meant that the size of `LatestCounterpartyCommitmentTXInfo` scaled with the number of htlcs, just like a fully built transaction. This commit switches to providing the fully built transaction. We take care to make this change both forward and backward compatible, so we still provide the former fields of `LatestCounterpartyCommitmentTXInfo`. --- lightning/src/chain/channelmonitor.rs | 72 ++++++++++++++++++--------- lightning/src/ln/channel.rs | 13 ++--- 2 files changed, 53 insertions(+), 32 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 4ae10b949e2..212f6323b7a 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -548,6 +548,7 @@ pub(crate) enum ChannelMonitorUpdateStep { feerate_per_kw: Option, to_broadcaster_value_sat: Option, to_countersignatory_value_sat: Option, + commitment_tx: Option, }, PaymentPreimage { payment_preimage: PaymentPreimage, @@ -599,6 +600,7 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, (4, their_per_commitment_point, required), (5, to_countersignatory_value_sat, option), (6, htlc_outputs, required_vec), + (7, commitment_tx, option), }, (2, PaymentPreimage) => { (0, payment_preimage, required), @@ -1027,6 +1029,10 @@ pub(crate) struct ChannelMonitorImpl { /// Ordering of tuple data: (their_per_commitment_point, feerate_per_kw, to_broadcaster_sats, /// to_countersignatory_sats) initial_counterparty_commitment_info: Option<(PublicKey, u32, u64, u64)>, + /// Initial counterparty commitment transaction + /// + /// We previously used the field above to re-build the transaction, we now provide it outright. + initial_counterparty_commitment_tx: Option, /// The first block height at which we had no remaining claimable balances. balances_empty_height: Option, @@ -1250,6 +1256,7 @@ impl Writeable for ChannelMonitorImpl { (23, self.holder_pays_commitment_tx_fee, option), (25, self.payment_preimages, required), (27, self.first_confirmed_funding_txo, required), + (29, self.initial_counterparty_commitment_tx, option), }); Ok(()) @@ -1461,6 +1468,7 @@ impl ChannelMonitor { best_block, counterparty_node_id: Some(counterparty_node_id), initial_counterparty_commitment_info: None, + initial_counterparty_commitment_tx: None, balances_empty_height: None, failed_back_htlc_ids: new_hash_set(), @@ -1500,17 +1508,14 @@ impl ChannelMonitor { /// This is used to provide the counterparty commitment information directly to the monitor /// before the initial persistence of a new channel. pub(crate) fn provide_initial_counterparty_commitment_tx( - &self, txid: Txid, htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)>, - commitment_number: u64, their_cur_per_commitment_point: PublicKey, feerate_per_kw: u32, - to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, logger: &L, + &self, htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)>, + commitment_tx: CommitmentTransaction, logger: &L, ) where L::Target: Logger { let mut inner = self.inner.lock().unwrap(); let logger = WithChannelMonitor::from_impl(logger, &*inner, None); - inner.provide_initial_counterparty_commitment_tx(txid, - htlc_outputs, commitment_number, their_cur_per_commitment_point, feerate_per_kw, - to_broadcaster_value_sat, to_countersignatory_value_sat, &logger); + inner.provide_initial_counterparty_commitment_tx(htlc_outputs, commitment_tx, &logger); } /// Informs this monitor of the latest counterparty (ie non-broadcastable) commitment transaction. @@ -2878,20 +2883,22 @@ impl ChannelMonitorImpl { } fn provide_initial_counterparty_commitment_tx( - &mut self, txid: Txid, htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)>, - commitment_number: u64, their_per_commitment_point: PublicKey, feerate_per_kw: u32, - to_broadcaster_value: u64, to_countersignatory_value: u64, logger: &WithChannelMonitor, + &mut self, htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)>, + commitment_tx: CommitmentTransaction, logger: &WithChannelMonitor, ) where L::Target: Logger { - self.initial_counterparty_commitment_info = Some((their_per_commitment_point.clone(), - feerate_per_kw, to_broadcaster_value, to_countersignatory_value)); + // We populate this field for downgrades + self.initial_counterparty_commitment_info = Some((commitment_tx.per_commitment_point(), + commitment_tx.feerate_per_kw(), commitment_tx.to_broadcaster_value_sat(), commitment_tx.to_countersignatory_value_sat())); #[cfg(debug_assertions)] { let rebuilt_commitment_tx = self.initial_counterparty_commitment_tx().unwrap(); - debug_assert_eq!(rebuilt_commitment_tx.trust().txid(), txid); + debug_assert_eq!(rebuilt_commitment_tx.trust().txid(), commitment_tx.trust().txid()); } - self.provide_latest_counterparty_commitment_tx(txid, htlc_outputs, commitment_number, - their_per_commitment_point, logger); + self.provide_latest_counterparty_commitment_tx(commitment_tx.trust().txid(), htlc_outputs, commitment_tx.commitment_number(), + commitment_tx.per_commitment_point(), logger); + // Soon, we will only populate this field + self.initial_counterparty_commitment_tx = Some(commitment_tx); } fn provide_latest_counterparty_commitment_tx( @@ -3442,15 +3449,20 @@ impl ChannelMonitorImpl { ret } - fn initial_counterparty_commitment_tx(&mut self) -> Option { - let (their_per_commitment_point, feerate_per_kw, to_broadcaster_value, - to_countersignatory_value) = self.initial_counterparty_commitment_info?; - let htlc_outputs = vec![]; + fn initial_counterparty_commitment_tx(&self) -> Option { + // This provides forward compatibility; an old monitor will not contain the full transaction; only enough + // information to rebuild it + if let Some((their_per_commitment_point, feerate_per_kw, to_broadcaster_value, + to_countersignatory_value)) = self.initial_counterparty_commitment_info { + let htlc_outputs = vec![]; - let commitment_tx = self.build_counterparty_commitment_tx(INITIAL_COMMITMENT_NUMBER, - &their_per_commitment_point, to_broadcaster_value, to_countersignatory_value, - feerate_per_kw, htlc_outputs); - Some(commitment_tx) + let commitment_tx = self.build_counterparty_commitment_tx(INITIAL_COMMITMENT_NUMBER, + &their_per_commitment_point, to_broadcaster_value, to_countersignatory_value, + feerate_per_kw, htlc_outputs); + Some(commitment_tx) + } else { + self.initial_counterparty_commitment_tx.clone() + } } fn build_counterparty_commitment_tx( @@ -3479,11 +3491,13 @@ impl ChannelMonitorImpl { fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec { update.updates.iter().filter_map(|update| { match update { + // Provided for forward compatibility; old updates won't contain the full transaction &ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, ref htlc_outputs, commitment_number, their_per_commitment_point, feerate_per_kw: Some(feerate_per_kw), to_broadcaster_value_sat: Some(to_broadcaster_value), - to_countersignatory_value_sat: Some(to_countersignatory_value) } => { + to_countersignatory_value_sat: Some(to_countersignatory_value), + commitment_tx: None } => { let nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| { htlc.transaction_output_index.map(|_| (htlc.clone(), None)) @@ -3497,6 +3511,15 @@ impl ChannelMonitorImpl { Some(commitment_tx) }, + &ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid: _, + htlc_outputs: _, commitment_number: _, their_per_commitment_point: _, + feerate_per_kw: _, + to_broadcaster_value_sat: _, + to_countersignatory_value_sat: _, + commitment_tx: Some(ref commitment_tx) } => { + + Some(commitment_tx.clone()) + }, _ => None, } }).collect() @@ -5079,6 +5102,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut spendable_txids_confirmed = Some(Vec::new()); let mut counterparty_fulfilled_htlcs = Some(new_hash_map()); let mut initial_counterparty_commitment_info = None; + let mut initial_counterparty_commitment_tx = None; let mut balances_empty_height = None; let mut channel_id = None; let mut holder_pays_commitment_tx_fee = None; @@ -5099,6 +5123,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (23, holder_pays_commitment_tx_fee, option), (25, payment_preimages_with_info, option), (27, first_confirmed_funding_txo, (default_value, funding_info.0)), + (29, initial_counterparty_commitment_tx, option), }); if let Some(payment_preimages_with_info) = payment_preimages_with_info { if payment_preimages_with_info.len() != payment_preimages.len() { @@ -5195,6 +5220,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP best_block, counterparty_node_id, initial_counterparty_commitment_info, + initial_counterparty_commitment_tx, balances_empty_height, failed_back_htlc_ids: new_hash_set(), }))) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 3e448119820..48558305fa7 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1985,7 +1985,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide fn initial_commitment_signed( &mut self, channel_id: ChannelId, counterparty_signature: Signature, holder_commitment_point: &mut HolderCommitmentPoint, - counterparty_commitment_number: u64, best_block: BestBlock, signer_provider: &SP, logger: &L, + _counterparty_commitment_number: u64, best_block: BestBlock, signer_provider: &SP, logger: &L, ) -> Result<(ChannelMonitor<::EcdsaSigner>, CommitmentTransaction), ChannelError> where L::Target: Logger @@ -2063,14 +2063,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide funding_redeemscript.clone(), self.funding().channel_value_satoshis, obscure_factor, holder_commitment_tx, best_block, context.counterparty_node_id, context.channel_id()); - channel_monitor.provide_initial_counterparty_commitment_tx( - counterparty_initial_bitcoin_tx.txid, Vec::new(), - counterparty_commitment_number, - context.counterparty_cur_commitment_point.unwrap(), - counterparty_initial_commitment_tx.feerate_per_kw(), - counterparty_initial_commitment_tx.to_broadcaster_value_sat(), - counterparty_initial_commitment_tx.to_countersignatory_value_sat(), - logger); + channel_monitor.provide_initial_counterparty_commitment_tx(Vec::new(), counterparty_initial_commitment_tx.clone(), logger); self.context_mut().cur_counterparty_commitment_transaction_number -= 1; @@ -8469,6 +8462,8 @@ impl FundedChannel where feerate_per_kw: Some(counterparty_commitment_tx.feerate_per_kw()), to_broadcaster_value_sat: Some(counterparty_commitment_tx.to_broadcaster_value_sat()), to_countersignatory_value_sat: Some(counterparty_commitment_tx.to_countersignatory_value_sat()), + // This is everything we need, but we still provide all the above fields for downgrades + commitment_tx: Some(counterparty_commitment_tx), }], channel_id: Some(self.context.channel_id()), }; From 3dcf40aa9d49ff4f4626e970140eada243333757 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 27 Feb 2025 19:03:07 +0000 Subject: [PATCH 02/18] Relax the requirements of building a `CommitmentTransaction` Building `CommitmentTransaction` previously required a pointer to a `Vec<(HTLCOutputInCommitment, T)>`, where each item in the vector represented a non-dust htlc. This forced the caller to place all the non-dust htlcs and their auxiliary data in contiguous memory prior to building a `CommitmentTransaction`. This requirement was not necessary, so we remove it in this commit. Instead, we choose to ask for an iterator that yields exclusive references to the non-dust HTLCs, so that `CommitmentTranscation` may populate their output indices during the build process. We decided against asking explicitly for a `Vec<&mut HTLCOutputInCommitment>` to avoid requiring an extra memory allocation. --- lightning/src/chain/channelmonitor.rs | 32 +++++++------- lightning/src/chain/onchaintx.rs | 7 ++- lightning/src/ln/chan_utils.rs | 63 +++++++++++++-------------- lightning/src/ln/channel.rs | 18 ++++---- lightning/src/ln/functional_tests.rs | 10 ++--- 5 files changed, 64 insertions(+), 66 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 212f6323b7a..a033bef6e54 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3454,21 +3454,21 @@ impl ChannelMonitorImpl { // information to rebuild it if let Some((their_per_commitment_point, feerate_per_kw, to_broadcaster_value, to_countersignatory_value)) = self.initial_counterparty_commitment_info { - let htlc_outputs = vec![]; + let mut htlc_outputs = Vec::new(); let commitment_tx = self.build_counterparty_commitment_tx(INITIAL_COMMITMENT_NUMBER, &their_per_commitment_point, to_broadcaster_value, to_countersignatory_value, - feerate_per_kw, htlc_outputs); + feerate_per_kw, htlc_outputs.iter_mut()); Some(commitment_tx) } else { self.initial_counterparty_commitment_tx.clone() } } - fn build_counterparty_commitment_tx( + fn build_counterparty_commitment_tx<'a>( &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: impl Iterator, ) -> CommitmentTransaction { let broadcaster_keys = &self.onchain_tx_handler.channel_transaction_parameters .counterparty_parameters.as_ref().unwrap().pubkeys; @@ -3482,9 +3482,9 @@ impl ChannelMonitorImpl { let channel_parameters = &self.onchain_tx_handler.channel_transaction_parameters.as_counterparty_broadcastable(); - CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, + CommitmentTransaction::new(commitment_number, to_broadcaster_value, to_countersignatory_value, broadcaster_funding_key, - countersignatory_funding_key, keys, feerate_per_kw, &mut nondust_htlcs, + countersignatory_funding_key, keys, feerate_per_kw, nondust_htlcs, channel_parameters) } @@ -3499,13 +3499,13 @@ impl ChannelMonitorImpl { to_countersignatory_value_sat: Some(to_countersignatory_value), commitment_tx: None } => { - let nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| { - htlc.transaction_output_index.map(|_| (htlc.clone(), None)) - }).collect::>(); + let mut nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| { + 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.iter_mut()); debug_assert_eq!(commitment_tx.trust().txid(), commitment_txid); @@ -5387,13 +5387,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 } @@ -5401,7 +5401,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, @@ -5463,7 +5463,7 @@ mod tests { let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs); monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(), - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap(); + htlcs.into_iter().map(|htlc| (htlc, Some(dummy_sig), None)).collect()).unwrap(); 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()), @@ -5501,7 +5501,7 @@ mod tests { let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]); let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs); monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(), - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap(); + htlcs.into_iter().map(|htlc| (htlc, Some(dummy_sig), None)).collect()).unwrap(); 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); @@ -5512,7 +5512,7 @@ mod tests { let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]); let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs); monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx, - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap(); + htlcs.into_iter().map(|htlc| (htlc, Some(dummy_sig), None)).collect()).unwrap(); 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); diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 2a43b006920..109e1b788ec 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1363,7 +1363,7 @@ mod tests { for i in 0..3 { let preimage = PaymentPreimage([i; 32]); let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array()); - htlcs.push(( + htlcs.push( HTLCOutputInCommitment { offered: true, amount_msat: 10000, @@ -1371,8 +1371,7 @@ mod tests { payment_hash: hash, transaction_output_index: Some(i as u32), }, - (), - )); + ); } let holder_commit = HolderCommitmentTransaction::dummy(&mut htlcs); let mut tx_handler = OnchainTxHandler::new( @@ -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 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 a471bd05c7d..19227710fe5 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -1105,7 +1105,7 @@ impl_writeable_tlv_based!(HolderCommitmentTransaction, { impl HolderCommitmentTransaction { #[cfg(test)] - pub fn dummy(htlcs: &mut Vec<(HTLCOutputInCommitment, ())>) -> Self { + pub fn dummy(nondust_htlcs: &mut 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()); @@ -1133,11 +1133,11 @@ impl HolderCommitmentTransaction { channel_type_features: ChannelTypeFeatures::only_static_remote_key(), }; 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, 0, 0, dummy_key.clone(), dummy_key.clone(), keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable()); - htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index); + let inner = CommitmentTransaction::new(0, 0, 0, dummy_key.clone(), dummy_key.clone(), keys, 0, nondust_htlcs.iter_mut(), &channel_parameters.as_counterparty_broadcastable()); + nondust_htlcs.sort_by_key(|htlc| htlc.transaction_output_index); HolderCommitmentTransaction { inner, counterparty_sig: dummy_sig, @@ -1440,18 +1440,15 @@ impl CommitmentTransaction { /// /// 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, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, broadcaster_funding_key: PublicKey, countersignatory_funding_key: PublicKey, keys: TxCreationKeys, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction { + pub fn new<'a>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, broadcaster_funding_key: PublicKey, countersignatory_funding_key: PublicKey, keys: TxCreationKeys, feerate_per_kw: u32, nondust_htlcs: impl Iterator, channel_parameters: &DirectedChannelTransactionParameters) -> 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); // 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, &broadcaster_funding_key, &countersignatory_funding_key).unwrap(); + let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters, &broadcaster_funding_key, &countersignatory_funding_key).unwrap(); 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); @@ -1462,7 +1459,7 @@ impl CommitmentTransaction { to_countersignatory_value_sat, to_broadcaster_delay: Some(channel_parameters.contest_delay()), feerate_per_kw, - htlcs, + htlcs: nondust_htlcs, channel_type_features: channel_parameters.channel_type_features().clone(), keys, built: BuiltCommitmentTransaction { @@ -1483,8 +1480,8 @@ impl CommitmentTransaction { fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result { 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 (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters, broadcaster_funding_key, countersignatory_funding_key)?; + let mut nondust_htlcs = self.htlcs.clone(); + let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, nondust_htlcs.iter_mut(), channel_parameters, broadcaster_funding_key, countersignatory_funding_key)?; let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); let txid = transaction.compute_txid(); @@ -1508,12 +1505,24 @@ impl CommitmentTransaction { // - 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, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<(Vec, Vec), ()> { + fn internal_build_outputs<'a>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: impl Iterator, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<(Vec, Vec), ()> { let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys(); let contest_delay = channel_parameters.contest_delay(); let mut txouts: Vec<(TxOut, Option<&mut HTLCOutputInCommitment>)> = Vec::new(); + for htlc in nondust_htlcs { + let script = get_htlc_redeemscript(&htlc, &channel_parameters.channel_type_features(), &keys); + let txout = TxOut { + script_pubkey: script.to_p2wsh(), + value: htlc.to_bitcoin_amount(), + }; + txouts.push((txout, Some(htlc))); + } + // This removes the need for the `ExactSizeIterator` bound on `nondust_htlcs` + let mut htlcs = Vec::with_capacity(txouts.len()); + let htlcs_empty = txouts.is_empty(); + if to_countersignatory_value_sat > Amount::ZERO { let script = if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() { get_to_countersignatory_with_anchors_redeemscript(&countersignatory_pubkeys.payment_point).to_p2wsh() @@ -1545,7 +1554,7 @@ impl CommitmentTransaction { } if channel_parameters.channel_type_features().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 || !htlcs_empty { let anchor_script = get_anchor_redeemscript(broadcaster_funding_key); txouts.push(( TxOut { @@ -1556,7 +1565,7 @@ impl CommitmentTransaction { )); } - if to_countersignatory_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() { + if to_countersignatory_value_sat > Amount::ZERO || !htlcs_empty { let anchor_script = get_anchor_redeemscript(countersignatory_funding_key); txouts.push(( TxOut { @@ -1568,16 +1577,6 @@ impl CommitmentTransaction { } } - let mut htlcs = Vec::with_capacity(htlcs_with_aux.len()); - for (htlc, _) in htlcs_with_aux { - let script = get_htlc_redeemscript(&htlc, &channel_parameters.channel_type_features(), &keys); - let txout = TxOut { - script_pubkey: script.to_p2wsh(), - value: htlc.to_bitcoin_amount(), - }; - txouts.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| { @@ -1915,7 +1914,7 @@ mod tests { counterparty_funding_pubkey: PublicKey, keys: TxCreationKeys, feerate_per_kw: u32, - htlcs_with_aux: Vec<(HTLCOutputInCommitment, ())>, + nondust_htlcs: Vec, channel_parameters: ChannelTransactionParameters, counterparty_pubkeys: ChannelPublicKeys, } @@ -1943,7 +1942,7 @@ mod tests { funding_outpoint: Some(chain::transaction::OutPoint { txid: Txid::all_zeros(), index: 0 }), channel_type_features: ChannelTypeFeatures::only_static_remote_key(), }; - let htlcs_with_aux = Vec::new(); + let nondust_htlcs = Vec::new(); Self { commitment_number: 0, @@ -1951,19 +1950,19 @@ mod tests { counterparty_funding_pubkey: counterparty_pubkeys.funding_pubkey, keys, feerate_per_kw: 1, - htlcs_with_aux, + nondust_htlcs, channel_parameters, counterparty_pubkeys, } } fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction { - CommitmentTransaction::new_with_auxiliary_htlc_data( + CommitmentTransaction::new( self.commitment_number, to_broadcaster_sats, to_countersignatory_sats, self.holder_funding_pubkey.clone(), self.counterparty_funding_pubkey.clone(), self.keys.clone(), self.feerate_per_kw, - &mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable() + self.nondust_htlcs.iter_mut(), &self.channel_parameters.as_holder_broadcastable() ) } } @@ -2009,7 +2008,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 = &builder.keys.clone(); assert_eq!(tx.built.transaction.output.len(), 3); @@ -2022,7 +2021,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 48558305fa7..ef15e23a4ba 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3618,15 +3618,15 @@ impl ChannelContext where SP::Target: SignerProvider { let channel_parameters = if local { self.channel_transaction_parameters.as_holder_broadcastable() } else { self.channel_transaction_parameters.as_counterparty_broadcastable() }; - let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, - value_to_a as u64, - value_to_b as u64, - funding_pubkey_a, - funding_pubkey_b, - keys.clone(), - feerate_per_kw, - &mut included_non_dust_htlcs, - &channel_parameters + let tx = CommitmentTransaction::new(commitment_number, + value_to_a as u64, + value_to_b as u64, + funding_pubkey_a, + funding_pubkey_b, + keys.clone(), + feerate_per_kw, + included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc), + &channel_parameters ); let mut htlcs_included = included_non_dust_htlcs; // The unwrap is safe, because all non-dust HTLCs have been assigned an output index diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index ff0646bfa6c..7858517a1a2 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -760,15 +760,15 @@ 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 mut htlcs: Vec = vec![]; + let commitment_tx = CommitmentTransaction::new( INITIAL_COMMITMENT_NUMBER - 1, push_sats, channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000, local_funding, remote_funding, commit_tx_keys.clone(), non_buffer_feerate + 4, - &mut htlcs, + htlcs.iter_mut(), &local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable() ); local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), Vec::new(), &secp_ctx).unwrap() @@ -1504,14 +1504,14 @@ 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, 95000, local_chan_balance, local_funding, remote_funding, commit_tx_keys.clone(), feerate_per_kw, - &mut vec![(accepted_htlc_info, ())], + vec![accepted_htlc_info].iter_mut(), &local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable() ); local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), Vec::new(), &secp_ctx).unwrap() From 2a82a1fc120a7ab4c5ae56073c41130bb3cee549 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 26 Feb 2025 05:17:18 +0000 Subject: [PATCH 03/18] Remove redundant parameters from the `CommitmentTransaction` constructor The `DirectedChannelTransactionParameters` argument of the `CommitmentTransaction` constructor already contains the broadcaster, and the countersignatory public keys, which include the respective funding keys. It is therefore not necessary to ask for these funding keys in additional arguments. --- lightning/src/chain/channelmonitor.rs | 15 +++---------- lightning/src/ln/chan_utils.rs | 26 ++++++++++------------- lightning/src/ln/channel.rs | 7 ------ lightning/src/ln/functional_tests.rs | 21 +++++++----------- lightning/src/util/test_channel_signer.rs | 4 ---- 5 files changed, 22 insertions(+), 51 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a033bef6e54..c817559ec08 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3470,22 +3470,13 @@ impl ChannelMonitorImpl { to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32, nondust_htlcs: impl Iterator, ) -> CommitmentTransaction { - let broadcaster_keys = &self.onchain_tx_handler.channel_transaction_parameters - .counterparty_parameters.as_ref().unwrap().pubkeys; - let countersignatory_keys = - &self.onchain_tx_handler.channel_transaction_parameters.holder_pubkeys; - - let broadcaster_funding_key = broadcaster_keys.funding_pubkey; - let countersignatory_funding_key = countersignatory_keys.funding_pubkey; - let keys = TxCreationKeys::from_channel_static_keys(&their_per_commitment_point, - &broadcaster_keys, &countersignatory_keys, &self.onchain_tx_handler.secp_ctx); let channel_parameters = &self.onchain_tx_handler.channel_transaction_parameters.as_counterparty_broadcastable(); + let keys = TxCreationKeys::from_channel_static_keys(&their_per_commitment_point, + channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), &self.onchain_tx_handler.secp_ctx); CommitmentTransaction::new(commitment_number, - to_broadcaster_value, to_countersignatory_value, broadcaster_funding_key, - countersignatory_funding_key, keys, feerate_per_kw, nondust_htlcs, - channel_parameters) + to_broadcaster_value, to_countersignatory_value, keys, feerate_per_kw, nondust_htlcs, channel_parameters) } fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 19227710fe5..477d973629f 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -1136,7 +1136,7 @@ impl HolderCommitmentTransaction { for _ in 0..nondust_htlcs.len() { counterparty_htlc_sigs.push(dummy_sig); } - let inner = CommitmentTransaction::new(0, 0, 0, dummy_key.clone(), dummy_key.clone(), keys, 0, nondust_htlcs.iter_mut(), &channel_parameters.as_counterparty_broadcastable()); + let inner = CommitmentTransaction::new(0, 0, 0, keys, 0, nondust_htlcs.iter_mut(), &channel_parameters.as_counterparty_broadcastable()); nondust_htlcs.sort_by_key(|htlc| htlc.transaction_output_index); HolderCommitmentTransaction { inner, @@ -1443,12 +1443,12 @@ impl CommitmentTransaction { /// 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<'a>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, broadcaster_funding_key: PublicKey, countersignatory_funding_key: PublicKey, keys: TxCreationKeys, feerate_per_kw: u32, nondust_htlcs: impl Iterator, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction { + pub fn new<'a>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, keys: TxCreationKeys, feerate_per_kw: u32, nondust_htlcs: impl Iterator, channel_parameters: &DirectedChannelTransactionParameters) -> 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); // 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, nondust_htlcs, channel_parameters, &broadcaster_funding_key, &countersignatory_funding_key).unwrap(); + let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters).unwrap(); 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); @@ -1477,11 +1477,11 @@ impl CommitmentTransaction { self } - fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result { + fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> Result { let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters); let mut nondust_htlcs = self.htlcs.clone(); - let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, nondust_htlcs.iter_mut(), channel_parameters, broadcaster_funding_key, countersignatory_funding_key)?; + let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, nondust_htlcs.iter_mut(), channel_parameters)?; let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); let txid = transaction.compute_txid(); @@ -1505,8 +1505,10 @@ impl CommitmentTransaction { // - 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<'a>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: impl Iterator, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<(Vec, Vec), ()> { + fn internal_build_outputs<'a>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: impl Iterator, channel_parameters: &DirectedChannelTransactionParameters) -> Result<(Vec, Vec), ()> { let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys(); + let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey; + let countersignatory_funding_key = &countersignatory_pubkeys.funding_pubkey; let contest_delay = channel_parameters.contest_delay(); let mut txouts: Vec<(TxOut, Option<&mut HTLCOutputInCommitment>)> = Vec::new(); @@ -1680,14 +1682,14 @@ impl CommitmentTransaction { /// /// An external validating signer must call this method before signing /// or using the built transaction. - pub fn verify(&self, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_keys: &ChannelPublicKeys, countersignatory_keys: &ChannelPublicKeys, secp_ctx: &Secp256k1) -> Result { + pub fn verify(&self, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1) -> Result { // This is the only field of the key cache that we trust let per_commitment_point = self.keys.per_commitment_point; - let keys = TxCreationKeys::from_channel_static_keys(&per_commitment_point, broadcaster_keys, countersignatory_keys, secp_ctx); + let keys = TxCreationKeys::from_channel_static_keys(&per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx); if keys != self.keys { return Err(()); } - let tx = self.internal_rebuild_transaction(&keys, channel_parameters, &broadcaster_keys.funding_pubkey, &countersignatory_keys.funding_pubkey)?; + let tx = self.internal_rebuild_transaction(&keys, channel_parameters)?; if self.built.transaction != tx.transaction || self.built.txid != tx.txid { return Err(()); } @@ -1910,8 +1912,6 @@ mod tests { struct TestCommitmentTxBuilder { commitment_number: u64, - holder_funding_pubkey: PublicKey, - counterparty_funding_pubkey: PublicKey, keys: TxCreationKeys, feerate_per_kw: u32, nondust_htlcs: Vec, @@ -1946,8 +1946,6 @@ mod tests { Self { commitment_number: 0, - holder_funding_pubkey: holder_pubkeys.funding_pubkey, - counterparty_funding_pubkey: counterparty_pubkeys.funding_pubkey, keys, feerate_per_kw: 1, nondust_htlcs, @@ -1959,8 +1957,6 @@ mod tests { fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction { CommitmentTransaction::new( self.commitment_number, to_broadcaster_sats, to_countersignatory_sats, - self.holder_funding_pubkey.clone(), - self.counterparty_funding_pubkey.clone(), self.keys.clone(), self.feerate_per_kw, self.nondust_htlcs.iter_mut(), &self.channel_parameters.as_holder_broadcastable() ) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index ef15e23a4ba..14f72eca260 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3595,11 +3595,6 @@ impl ChannelContext where SP::Target: SignerProvider { let mut value_to_a = if local { value_to_self } else { value_to_remote }; let mut value_to_b = if local { value_to_remote } else { value_to_self }; - let (funding_pubkey_a, funding_pubkey_b) = if local { - (self.get_holder_pubkeys().funding_pubkey, self.get_counterparty_pubkeys().funding_pubkey) - } else { - (self.get_counterparty_pubkeys().funding_pubkey, self.get_holder_pubkeys().funding_pubkey) - }; if value_to_a >= (broadcaster_dust_limit_satoshis as i64) { log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a); @@ -3621,8 +3616,6 @@ impl ChannelContext where SP::Target: SignerProvider { let tx = CommitmentTransaction::new(commitment_number, value_to_a as u64, value_to_b as u64, - funding_pubkey_a, - funding_pubkey_b, keys.clone(), feerate_per_kw, included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc), diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 7858517a1a2..1b89116fe9d 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -731,16 +731,15 @@ fn test_update_fee_that_funder_cannot_afford() { // Get the TestChannelSigner for each channel, which will be used to (1) get the keys // needed to sign the new commitment tx and (2) sign the new commitment tx. - let (local_revocation_basepoint, local_htlc_basepoint, local_funding) = { + let (local_revocation_basepoint, local_htlc_basepoint) = { let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let chan_signer = local_chan.get_signer(); let pubkeys = chan_signer.as_ref().pubkeys(); - (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, - pubkeys.funding_pubkey) + (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint) }; - let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = { + let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point) = { let per_peer_state = nodes[1].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap(); let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); @@ -748,7 +747,7 @@ fn test_update_fee_that_funder_cannot_afford() { let pubkeys = chan_signer.as_ref().pubkeys(); (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(), - pubkeys.funding_pubkey) + ) }; // Assemble the set of keys we can use for signatures for our commitment_signed message. @@ -765,7 +764,6 @@ fn test_update_fee_that_funder_cannot_afford() { INITIAL_COMMITMENT_NUMBER - 1, push_sats, channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000, - local_funding, remote_funding, commit_tx_keys.clone(), non_buffer_feerate + 4, htlcs.iter_mut(), @@ -1456,7 +1454,7 @@ fn test_fee_spike_violation_fails_htlc() { // Get the TestChannelSigner for each channel, which will be used to (1) get the keys // needed to sign the new commitment tx and (2) sign the new commitment tx. - let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point, local_funding) = { + let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point) = { let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); @@ -1467,18 +1465,16 @@ fn test_fee_spike_violation_fails_htlc() { let pubkeys = chan_signer.as_ref().pubkeys(); (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(), - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(), - chan_signer.as_ref().pubkeys().funding_pubkey) + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap()) }; - let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = { + let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point) = { let per_peer_state = nodes[1].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap(); let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let chan_signer = remote_chan.get_signer(); let pubkeys = chan_signer.as_ref().pubkeys(); (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(), - chan_signer.as_ref().pubkeys().funding_pubkey) + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap()) }; // Assemble the set of keys we can use for signatures for our commitment_signed message. @@ -1508,7 +1504,6 @@ fn test_fee_spike_violation_fails_htlc() { commitment_number, 95000, local_chan_balance, - local_funding, remote_funding, commit_tx_keys.clone(), feerate_per_kw, vec![accepted_htlc_info].iter_mut(), diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index 3bc095d2ba7..755afb9a456 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -548,8 +548,6 @@ impl TestChannelSigner { commitment_tx .verify( &self.inner.get_channel_parameters().unwrap().as_counterparty_broadcastable(), - self.inner.counterparty_pubkeys().unwrap(), - self.inner.pubkeys(), secp_ctx, ) .expect("derived different per-tx keys or built transaction") @@ -561,8 +559,6 @@ impl TestChannelSigner { commitment_tx .verify( &self.inner.get_channel_parameters().unwrap().as_holder_broadcastable(), - self.inner.pubkeys(), - self.inner.counterparty_pubkeys().unwrap(), secp_ctx, ) .expect("derived different per-tx keys or built transaction") From a50d32d5884206472ceedde01de8afccd05f91a9 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 26 Feb 2025 23:07:18 +0000 Subject: [PATCH 04/18] Let `CommitmentTransaction` build the `TxCreationKeys` it will cache Instead of asking callers to generate the `TxCreationKeys` on every new commitment before constructing a `CommitmentTransaction`, let `CommitmentTransaction` derive the `TxCreationKeys` it will use to build the raw commitment transaction that it will cache. This allows a tighter coupling between the per-commitment keys, and the corresponding commitment transaction. As new states are generated, callers now only have to derive new commitment points; `CommitmentTransaction` takes care of deriving the per-commitment keys. This also serves to limit the objects in LDK that derive per-commitment keys according to the LN Specification in preparation for enabling custom derivations in the future. --- lightning/src/chain/channelmonitor.rs | 9 ++---- lightning/src/ln/chan_utils.rs | 32 +++++++------------ lightning/src/ln/channel.rs | 5 +-- lightning/src/ln/functional_tests.rs | 45 +++++++-------------------- 4 files changed, 30 insertions(+), 61 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index c817559ec08..caf4bb14456 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -38,7 +38,7 @@ use crate::types::features::ChannelTypeFeatures; use crate::types::payment::{PaymentHash, PaymentPreimage}; use crate::ln::msgs::DecodeError; use crate::ln::channel_keys::{DelayedPaymentKey, DelayedPaymentBasepoint, HtlcBasepoint, HtlcKey, RevocationKey, RevocationBasepoint}; -use crate::ln::chan_utils::{self,CommitmentTransaction, CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction, TxCreationKeys}; +use crate::ln::chan_utils::{self,CommitmentTransaction, CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction}; use crate::ln::channelmanager::{HTLCSource, SentHTLCId, PaymentClaimDetails}; use crate::chain; use crate::chain::{BestBlock, WatchedOutput}; @@ -3472,11 +3472,8 @@ impl ChannelMonitorImpl { ) -> CommitmentTransaction { let channel_parameters = &self.onchain_tx_handler.channel_transaction_parameters.as_counterparty_broadcastable(); - let keys = TxCreationKeys::from_channel_static_keys(&their_per_commitment_point, - channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), &self.onchain_tx_handler.secp_ctx); - - CommitmentTransaction::new(commitment_number, - to_broadcaster_value, to_countersignatory_value, keys, feerate_per_kw, nondust_htlcs, channel_parameters) + 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 { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 477d973629f..ee361a2c5fb 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -1110,13 +1110,6 @@ impl HolderCommitmentTransaction { 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()); - let keys = TxCreationKeys { - per_commitment_point: dummy_key.clone(), - revocation_key: RevocationKey::from_basepoint(&secp_ctx, &RevocationBasepoint::from(dummy_key), &dummy_key), - broadcaster_htlc_key: HtlcKey::from_basepoint(&secp_ctx, &HtlcBasepoint::from(dummy_key), &dummy_key), - countersignatory_htlc_key: HtlcKey::from_basepoint(&secp_ctx, &HtlcBasepoint::from(dummy_key), &dummy_key), - broadcaster_delayed_payment_key: DelayedPaymentKey::from_basepoint(&secp_ctx, &DelayedPaymentBasepoint::from(dummy_key), &dummy_key), - }; let channel_pubkeys = ChannelPublicKeys { funding_pubkey: dummy_key.clone(), revocation_basepoint: RevocationBasepoint::from(dummy_key), @@ -1136,7 +1129,7 @@ impl HolderCommitmentTransaction { for _ in 0..nondust_htlcs.len() { counterparty_htlc_sigs.push(dummy_sig); } - let inner = CommitmentTransaction::new(0, 0, 0, keys, 0, nondust_htlcs.iter_mut(), &channel_parameters.as_counterparty_broadcastable()); + let inner = CommitmentTransaction::new(0, &dummy_key.clone(), 0, 0, 0, nondust_htlcs.iter_mut(), &channel_parameters.as_counterparty_broadcastable(), &secp_ctx); nondust_htlcs.sort_by_key(|htlc| htlc.transaction_output_index); HolderCommitmentTransaction { inner, @@ -1443,9 +1436,10 @@ impl CommitmentTransaction { /// 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<'a>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, keys: TxCreationKeys, feerate_per_kw: u32, nondust_htlcs: impl Iterator, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction { + pub fn new<'a>(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, nondust_htlcs: impl Iterator, 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, nondust_htlcs, channel_parameters).unwrap(); @@ -1896,8 +1890,8 @@ pub fn get_commitment_transaction_number_obscure_factor( mod tests { use super::{CounterpartyCommitmentSecrets, ChannelPublicKeys}; use crate::chain; - use crate::ln::chan_utils::{get_htlc_redeemscript, get_to_countersignatory_with_anchors_redeemscript, CommitmentTransaction, TxCreationKeys, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, HTLCOutputInCommitment}; - use bitcoin::secp256k1::{PublicKey, SecretKey, Secp256k1}; + use crate::ln::chan_utils::{get_htlc_redeemscript, get_to_countersignatory_with_anchors_redeemscript, CommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, HTLCOutputInCommitment}; + use bitcoin::secp256k1::{self, PublicKey, SecretKey, Secp256k1}; use crate::util::test_utils; use crate::sign::{ChannelSigner, SignerProvider}; use bitcoin::{Network, Txid, ScriptBuf, CompressedPublicKey}; @@ -1912,11 +1906,12 @@ mod tests { struct TestCommitmentTxBuilder { commitment_number: u64, - keys: TxCreationKeys, + per_commitment_point: PublicKey, feerate_per_kw: u32, nondust_htlcs: Vec, channel_parameters: ChannelTransactionParameters, counterparty_pubkeys: ChannelPublicKeys, + secp_ctx: Secp256k1::, } impl TestCommitmentTxBuilder { @@ -1927,13 +1922,10 @@ mod tests { let keys_provider = test_utils::TestKeysInterface::new(&seed, network); let signer = keys_provider.derive_channel_signer(3000, keys_provider.generate_channel_keys_id(false, 1_000_000, 0)); let counterparty_signer = keys_provider.derive_channel_signer(3000, keys_provider.generate_channel_keys_id(true, 1_000_000, 1)); - let delayed_payment_base = &signer.pubkeys().delayed_payment_basepoint; let per_commitment_secret = SecretKey::from_slice(&>::from_hex("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap(); let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret); - let htlc_basepoint = &signer.pubkeys().htlc_basepoint; let holder_pubkeys = signer.pubkeys(); let counterparty_pubkeys = counterparty_signer.pubkeys().clone(); - let keys = TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint); let channel_parameters = ChannelTransactionParameters { holder_pubkeys: holder_pubkeys.clone(), holder_selected_contest_delay: 0, @@ -1946,19 +1938,19 @@ mod tests { Self { commitment_number: 0, - keys, + per_commitment_point, feerate_per_kw: 1, nondust_htlcs, channel_parameters, counterparty_pubkeys, + secp_ctx, } } fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction { CommitmentTransaction::new( - self.commitment_number, to_broadcaster_sats, to_countersignatory_sats, - self.keys.clone(), self.feerate_per_kw, - self.nondust_htlcs.iter_mut(), &self.channel_parameters.as_holder_broadcastable() + self.commitment_number, &self.per_commitment_point, to_broadcaster_sats, to_countersignatory_sats, self.feerate_per_kw, + self.nondust_htlcs.iter_mut(), &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx ) } } @@ -2006,7 +1998,7 @@ mod tests { builder.channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key(); builder.nondust_htlcs = vec![received_htlc.clone(), offered_htlc.clone()]; let tx = builder.build(3000, 0); - let keys = &builder.keys.clone(); + let keys = &tx.trust().keys(); assert_eq!(tx.built.transaction.output.len(), 3); assert_eq!(tx.built.transaction.output[0].script_pubkey, get_htlc_redeemscript(&received_htlc, &ChannelTypeFeatures::only_static_remote_key(), &keys).to_p2wsh()); assert_eq!(tx.built.transaction.output[1].script_pubkey, get_htlc_redeemscript(&offered_htlc, &ChannelTypeFeatures::only_static_remote_key(), &keys).to_p2wsh()); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 14f72eca260..2fd785d47c4 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3614,12 +3614,13 @@ impl ChannelContext where SP::Target: SignerProvider { if local { self.channel_transaction_parameters.as_holder_broadcastable() } else { self.channel_transaction_parameters.as_counterparty_broadcastable() }; let tx = CommitmentTransaction::new(commitment_number, + &keys.per_commitment_point, value_to_a as u64, value_to_b as u64, - keys.clone(), feerate_per_kw, included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc), - &channel_parameters + &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 diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 1b89116fe9d..71e584f8f61 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -731,29 +731,14 @@ fn test_update_fee_that_funder_cannot_afford() { // Get the TestChannelSigner for each channel, which will be used to (1) get the keys // needed to sign the new commitment tx and (2) sign the new commitment tx. - let (local_revocation_basepoint, local_htlc_basepoint) = { - let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); - let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); - let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); - let chan_signer = local_chan.get_signer(); - let pubkeys = chan_signer.as_ref().pubkeys(); - (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint) - }; - let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point) = { + let remote_point = { let per_peer_state = nodes[1].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap(); let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let chan_signer = remote_chan.get_signer(); - let pubkeys = chan_signer.as_ref().pubkeys(); - (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(), - ) + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap() }; - // Assemble the set of keys we can use for signatures for our commitment_signed message. - let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint, - &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint); - let res = { let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); @@ -762,12 +747,13 @@ fn test_update_fee_that_funder_cannot_afford() { let mut 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, - commit_tx_keys.clone(), non_buffer_feerate + 4, htlcs.iter_mut(), - &local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable() + &local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable(), + &secp_ctx, ); local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), Vec::new(), &secp_ctx).unwrap() }; @@ -1454,7 +1440,7 @@ fn test_fee_spike_violation_fails_htlc() { // Get the TestChannelSigner for each channel, which will be used to (1) get the keys // needed to sign the new commitment tx and (2) sign the new commitment tx. - let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point) = { + let (local_secret, next_local_point) = { let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); @@ -1462,25 +1448,17 @@ fn test_fee_spike_violation_fails_htlc() { // Make the signer believe we validated another commitment, so we can release the secret chan_signer.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1; - let pubkeys = chan_signer.as_ref().pubkeys(); - (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(), + (chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(), chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap()) }; - let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point) = { + let remote_point = { let per_peer_state = nodes[1].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap(); let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let chan_signer = remote_chan.get_signer(); - let pubkeys = chan_signer.as_ref().pubkeys(); - (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap()) + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap() }; - // Assemble the set of keys we can use for signatures for our commitment_signed message. - let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint, - &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint); - // Build the remote commitment transaction so we can sign it, and then later use the // signature for the commitment_signed message. let local_chan_balance = 1313; @@ -1502,12 +1480,13 @@ fn test_fee_spike_violation_fails_htlc() { let local_chan_signer = local_chan.get_signer(); let commitment_tx = CommitmentTransaction::new( commitment_number, + &remote_point, 95000, local_chan_balance, - commit_tx_keys.clone(), feerate_per_kw, vec![accepted_htlc_info].iter_mut(), - &local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable() + &local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable(), + &secp_ctx, ); local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), Vec::new(), &secp_ctx).unwrap() }; From d60cfda60e4d6ade6006f0ac129f0668994a0275 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 26 Feb 2025 06:53:13 +0000 Subject: [PATCH 05/18] Begin removing per-commitment key derivations from channel objects Channel objects should not impose a particular per-commitment derivation scheme on the builders of commitment transactions. This will make it easier to enable custom derivations in the future. To get us started, we remove most instances of `TxCreationKeys` in channel. The last two instances are 1) checking a counterparty's htlc signatures 2) logging the keys we used to create htlc signatures for a counterparty Both of these cases will be abstracted away from channel in the future. --- lightning/src/ln/channel.rs | 72 ++++++++++--------------------------- 1 file changed, 19 insertions(+), 53 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 2fd785d47c4..7b13f6ccdec 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1968,8 +1968,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide ) -> Result where L::Target: Logger { let funding_script = self.context().get_funding_redeemscript(); - let keys = self.context().build_holder_transaction_keys(holder_commitment_point.current_point()); - let initial_commitment_tx = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &keys, true, false, logger).tx; + let initial_commitment_tx = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger).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().channel_value_satoshis); @@ -2006,8 +2005,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide } }; let context = self.context(); - let counterparty_keys = context.build_remote_transaction_keys(); - let counterparty_initial_commitment_tx = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; + let counterparty_initial_commitment_tx = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx; let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); @@ -3412,7 +3410,7 @@ impl ChannelContext where SP::Target: SignerProvider { /// 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, keys: &TxCreationKeys, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats + fn build_commitment_transaction(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats where L::Target: Logger { let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new(); @@ -3614,7 +3612,7 @@ impl ChannelContext where SP::Target: SignerProvider { if local { self.channel_transaction_parameters.as_holder_broadcastable() } else { self.channel_transaction_parameters.as_counterparty_broadcastable() }; let tx = CommitmentTransaction::new(commitment_number, - &keys.per_commitment_point, + &per_commitment_point, value_to_a as u64, value_to_b as u64, feerate_per_kw, @@ -3640,32 +3638,6 @@ impl ChannelContext where SP::Target: SignerProvider { } } - #[inline] - /// Creates a set of keys for build_commitment_transaction to generate a transaction which our - /// counterparty will sign (ie DO NOT send signatures over a transaction created by this to - /// our counterparty!) - /// The result is a transaction which we can revoke broadcastership of (ie a "local" transaction) - /// TODO Some magic rust shit to compile-time check this? - fn build_holder_transaction_keys(&self, per_commitment_point: PublicKey) -> TxCreationKeys { - let delayed_payment_base = &self.get_holder_pubkeys().delayed_payment_basepoint; - let htlc_basepoint = &self.get_holder_pubkeys().htlc_basepoint; - let counterparty_pubkeys = self.get_counterparty_pubkeys(); - - TxCreationKeys::derive_new(&self.secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint) - } - - #[inline] - /// Creates a set of keys for build_commitment_transaction to generate a transaction which we - /// will sign and send to our counterparty. - /// If an Err is returned, it is a ChannelError::Close (for get_funding_created) - fn build_remote_transaction_keys(&self) -> TxCreationKeys { - let revocation_basepoint = &self.get_holder_pubkeys().revocation_basepoint; - let htlc_basepoint = &self.get_holder_pubkeys().htlc_basepoint; - let counterparty_pubkeys = self.get_counterparty_pubkeys(); - - TxCreationKeys::derive_new(&self.secp_ctx, &self.counterparty_cur_commitment_point.unwrap(), &counterparty_pubkeys.delayed_payment_basepoint, &counterparty_pubkeys.htlc_basepoint, revocation_basepoint, htlc_basepoint) - } - /// Gets the redeemscript for the funding transaction output (ie the funding transaction output /// pays to get_funding_redeemscript().to_p2wsh()). /// Panics if called before accept_channel/InboundV1Channel::new @@ -4459,9 +4431,8 @@ impl ChannelContext where SP::Target: SignerProvider { SP::Target: SignerProvider, L::Target: Logger { - let counterparty_keys = self.build_remote_transaction_keys(); let counterparty_initial_commitment_tx = self.build_commitment_transaction( - funding, self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; + funding, self.cur_counterparty_commitment_transaction_number, &self.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx; match self.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ref ecdsa) => { @@ -5454,9 +5425,7 @@ impl FundedChannel where let funding_script = self.context.get_funding_redeemscript(); - let keys = self.context.build_holder_transaction_keys(self.holder_commitment_point.current_point()); - - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &keys, true, false, logger); + let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, false, logger); let commitment_txid = { let trusted_tx = commitment_stats.tx.trust(); let bitcoin_tx = trusted_tx.built_transaction(); @@ -5524,19 +5493,20 @@ impl FundedChannel where let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len()); let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len()); + let holder_keys = TxCreationKeys::from_channel_static_keys(&self.holder_commitment_point.current_point(), self.context.get_holder_pubkeys(), self.context.get_counterparty_pubkeys(), &self.context.secp_ctx); for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() { if let Some(_) = htlc.transaction_output_index { let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw, self.context.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.context.channel_type, - &keys.broadcaster_delayed_payment_key, &keys.revocation_key); + &holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key); - let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &keys); + let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &holder_keys); let htlc_sighashtype = if self.context.channel_type.supports_anchors_zero_fee_htlc_tx() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All }; let htlc_sighash = hash_to_message!(&sighash::SighashCache::new(&htlc_tx).p2wsh_signature_hash(0, &htlc_redeemscript, htlc.to_bitcoin_amount(), htlc_sighashtype).unwrap()[..]); log_trace!(logger, "Checking HTLC tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}.", - log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(keys.countersignatory_htlc_key.to_public_key().serialize()), + log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(holder_keys.countersignatory_htlc_key.to_public_key().serialize()), encode::serialize_hex(&htlc_tx), log_bytes!(htlc_sighash[..]), encode::serialize_hex(&htlc_redeemscript), &self.context.channel_id()); - if let Err(_) = self.context.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &keys.countersignatory_htlc_key.to_public_key()) { + if let Err(_) = self.context.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &holder_keys.countersignatory_htlc_key.to_public_key()) { return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned())); } if !separate_nondust_htlc_sources { @@ -6218,8 +6188,7 @@ impl FundedChannel where // Before proposing a feerate update, check that we can actually afford the new fee. let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate); - let keys = self.context.build_holder_transaction_keys(self.holder_commitment_point.current_point()); - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &keys, true, true, logger); + let commitment_stats = self.context.build_commitment_transaction(&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_stats.num_nondust_htlcs + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; let holder_balance_msat = commitment_stats.local_balance_msat - htlc_stats.outbound_holding_cell_msat; if holder_balance_msat < buffer_fee_msat + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { @@ -6532,8 +6501,7 @@ impl FundedChannel where self.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); } let funding_signed = if self.context.signer_pending_funding && !self.context.is_outbound() { - let counterparty_keys = self.context.build_remote_transaction_keys(); - let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number + 1, &counterparty_keys, false, false, logger).tx; + let counterparty_initial_commitment_tx = 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).tx; self.context.get_funding_signed_msg(logger, counterparty_initial_commitment_tx) } else { None }; // Provide a `channel_ready` message if we need to, but only if we're _not_ still pending @@ -8469,8 +8437,7 @@ impl FundedChannel where -> (Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, CommitmentTransaction) where L::Target: Logger { - let counterparty_keys = self.context.build_remote_transaction_keys(); - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger); + let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger); let counterparty_commitment_tx = commitment_stats.tx; #[cfg(any(test, fuzzing))] @@ -8501,8 +8468,7 @@ impl FundedChannel where #[cfg(any(test, fuzzing))] self.build_commitment_no_state_update(logger); - let counterparty_keys = self.context.build_remote_transaction_keys(); - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger); + let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger); let counterparty_commitment_txid = commitment_stats.tx.trust().txid(); match &self.context.holder_signer { @@ -8528,7 +8494,8 @@ impl FundedChannel where encode::serialize_hex(&commitment_stats.tx.trust().built_transaction().transaction), &counterparty_commitment_txid, encode::serialize_hex(&self.context.get_funding_redeemscript()), log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id()); - + + let counterparty_keys = TxCreationKeys::from_channel_static_keys(&self.context.counterparty_cur_commitment_point.unwrap(), self.context.get_counterparty_pubkeys(), self.context.get_holder_pubkeys(), &self.context.secp_ctx); for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) { log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}", encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.context.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), @@ -8980,8 +8947,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { /// Only allowed after [`ChannelContext::channel_transaction_parameters`] is set. fn get_funding_created_msg(&mut self, logger: &L) -> Option where L::Target: Logger { - let counterparty_keys = self.context.build_remote_transaction_keys(); - let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; + let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx; let signature = match &self.context.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ecdsa) => { @@ -11633,7 +11599,7 @@ mod tests { $( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), * } ) => { { let (commitment_tx, htlcs): (_, Vec) = { - let mut commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &keys, true, false, &logger); + let mut commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger); let htlcs = commitment_stats.htlcs_included.drain(..) .filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None }) From df9a561b9b63b5024112b479c53f8da889cbb6b0 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 26 Feb 2025 04:40:20 +0000 Subject: [PATCH 06/18] Remove `i64` casts in `ChannelContext::build_commitment_transaction` Instead of converting operands to `i64` and checking if the subtractions overflowed by checking if the `i64` is smaller than zero, we instead choose to do checked and saturating subtractions on the original unsigned integers. --- lightning/src/ln/channel.rs | 39 +++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 7b13f6ccdec..cdb1d0d9da1 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3559,14 +3559,15 @@ impl ChannelContext where SP::Target: SignerProvider { } } - let value_to_self_msat: i64 = (funding.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset; - assert!(value_to_self_msat >= 0); + // TODO: When MSRV >= 1.66.0, use u64::checked_add_signed + let mut value_to_self_msat = u64::try_from(funding.value_to_self_msat as i64 + value_to_self_msat_offset).unwrap(); // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie // AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to - // "violate" their reserve value by couting those against it. Thus, we have to convert - // everything to i64 before subtracting as otherwise we can overflow. - let value_to_remote_msat: i64 = (funding.channel_value_satoshis * 1000) as i64 - (funding.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset; - assert!(value_to_remote_msat >= 0); + // "violate" their reserve value by couting those against it. Thus, we have to do checked subtraction + // as otherwise we can overflow. + let mut value_to_remote_msat = u64::checked_sub(funding.channel_value_satoshis * 1000, value_to_self_msat).unwrap(); + value_to_self_msat = u64::checked_sub(value_to_self_msat, local_htlc_total_msat).unwrap(); + value_to_remote_msat = u64::checked_sub(value_to_remote_msat, remote_htlc_total_msat).unwrap(); #[cfg(debug_assertions)] { @@ -3577,30 +3578,30 @@ impl ChannelContext where SP::Target: SignerProvider { } else { funding.counterparty_max_commitment_tx_output.lock().unwrap() }; - debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap() as i64); - broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat as u64); - debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis as i64); - broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat as u64); + debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap()); + broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat); + debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis); + 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, included_non_dust_htlcs.len(), &self.channel_transaction_parameters.channel_type_features); - let anchors_val = if self.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 } as i64; + let anchors_val = if self.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; let (value_to_self, value_to_remote) = if self.is_outbound() { - (value_to_self_msat / 1000 - anchors_val - total_fee_sat as i64, value_to_remote_msat / 1000) + ((value_to_self_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat), value_to_remote_msat / 1000) } else { - (value_to_self_msat / 1000, value_to_remote_msat / 1000 - anchors_val - total_fee_sat as i64) + (value_to_self_msat / 1000, (value_to_remote_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat)) }; let mut value_to_a = if local { value_to_self } else { value_to_remote }; let mut value_to_b = if local { value_to_remote } else { value_to_self }; - if value_to_a >= (broadcaster_dust_limit_satoshis as i64) { + if value_to_a >= broadcaster_dust_limit_satoshis { log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a); } else { value_to_a = 0; } - if value_to_b >= (broadcaster_dust_limit_satoshis as i64) { + if value_to_b >= broadcaster_dust_limit_satoshis { log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b); } else { value_to_b = 0; @@ -3613,8 +3614,8 @@ impl ChannelContext where SP::Target: SignerProvider { else { self.channel_transaction_parameters.as_counterparty_broadcastable() }; let tx = CommitmentTransaction::new(commitment_number, &per_commitment_point, - value_to_a as u64, - value_to_b as u64, + value_to_a, + value_to_b, feerate_per_kw, included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc), &channel_parameters, @@ -3631,8 +3632,8 @@ impl ChannelContext where SP::Target: SignerProvider { total_fee_sat, num_nondust_htlcs, htlcs_included, - local_balance_msat: value_to_self_msat as u64, - remote_balance_msat: value_to_remote_msat as u64, + local_balance_msat: value_to_self_msat, + remote_balance_msat: value_to_remote_msat, inbound_htlc_preimages, outbound_htlc_preimages, } From ca3fa4ee7a721b93e100b868e26f574fa8d4522a Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 26 Feb 2025 04:56:22 +0000 Subject: [PATCH 07/18] Clean up tautologies in `ChannelContext::build_commitment_transaction` There is no need for an if statement if it will always be true. --- lightning/src/ln/channel.rs | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index cdb1d0d9da1..2aa184f226f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3504,13 +3504,9 @@ impl ChannelContext where SP::Target: SignerProvider { } 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(ref reason) => { - if generated_by_local { - if let &InboundHTLCRemovalReason::Fulfill(preimage) = reason { - inbound_htlc_preimages.push(preimage); - value_to_self_msat_offset += htlc.amount_msat as i64; - } - } + &InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => { + inbound_htlc_preimages.push(preimage); + value_to_self_msat_offset += htlc.amount_msat as i64; }, _ => {}, } @@ -3546,13 +3542,10 @@ impl ChannelContext where SP::Target: SignerProvider { } 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(_))|OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) => { - value_to_self_msat_offset -= htlc.amount_msat as i64; - }, + OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) | + OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) | OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) => { - if !generated_by_local { - value_to_self_msat_offset -= htlc.amount_msat as i64; - } + value_to_self_msat_offset -= htlc.amount_msat as i64; }, _ => {}, } From 570ca77186576bdc9458dd0bddf5f0b9917f4f1f Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 27 Feb 2025 17:51:44 +0000 Subject: [PATCH 08/18] Reorder the operations in `ChannelContext::build_commitment_transaction` This reorganizes the logic in `ChannelContext::build_commitment_transaction` to clearly separate the sorting of HTLCs for a commitment transaction based on their state from the trimming of HTLCs based on the broadcaster's dust limit. In the future, transaction builders will decide how to handle HTLCs below the dust limit, but they will not decide which HTLCs to include based on their state. --- lightning/src/ln/channel.rs | 152 +++++++++++++++++++++--------------- 1 file changed, 87 insertions(+), 65 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 2aa184f226f..9efcad735ac 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3413,9 +3413,16 @@ impl ChannelContext where SP::Target: SignerProvider { fn build_commitment_transaction(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats 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); + + // This will contain all the htlcs included on the commitment transaction due to their state, both dust, and non-dust. + // Non-dust htlcs will come first, with their output indices populated, then dust htlcs, with their output indices set to `None`. + let mut htlcs_in_tx: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); + + // We allocate this vector because we need to count the number of non-dust htlcs and calculate the total fee of the transaction + // before calling `CommitmentTransaction::new`. + // We could drop this vector and create two iterators: one to count the number of non-dust htlcs, and another to pass to `CommitmentTransaction::new` + let mut included_non_dust_htlcs: Vec<&mut HTLCOutputInCommitment> = Vec::with_capacity(num_htlcs); let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis }; let mut remote_htlc_total_msat = 0; @@ -3453,42 +3460,8 @@ impl ChannelContext where SP::Target: SignerProvider { } } - 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 self.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * htlc_timeout_tx_weight(self.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 self.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * htlc_success_tx_weight(self.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)); - } - } - } - } - + // Read the state of htlcs and determine their inclusion in the commitment transaction 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"), @@ -3499,8 +3472,9 @@ impl ChannelContext where SP::Target: SignerProvider { }; if include { - add_htlc_output!(htlc, false, None, state_name); - remote_htlc_total_msat += htlc.amount_msat; + log_trace!(logger, " ...including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name); + let htlc = get_htlc_in_commitment!(htlc, !local); + htlcs_in_tx.push((htlc, None)); } 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 { @@ -3515,7 +3489,6 @@ impl ChannelContext where SP::Target: SignerProvider { 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"), @@ -3537,8 +3510,9 @@ impl ChannelContext where SP::Target: SignerProvider { } if include { - add_htlc_output!(htlc, true, Some(&htlc.source), state_name); - local_htlc_total_msat += htlc.amount_msat; + log_trace!(logger, " ...including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name); + let htlc_in_tx = get_htlc_in_commitment!(htlc, local); + htlcs_in_tx.push((htlc_in_tx, 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, state_name); match htlc.state { @@ -3552,6 +3526,47 @@ impl ChannelContext where SP::Target: SignerProvider { } } + // Trim dust htlcs + for (htlc, _) in htlcs_in_tx.iter_mut() { + if htlc.offered { + let outbound = local; + if outbound { + local_htlc_total_msat += htlc.amount_msat; + } else { + remote_htlc_total_msat += htlc.amount_msat; + } + let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + feerate_per_kw as u64 * htlc_timeout_tx_weight(self.get_channel_type()) / 1000 + }; + if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { + log_trace!(logger, " ...creating output for {} non-dust HTLC (hash {}) with value {}", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat); + included_non_dust_htlcs.push(htlc); + } else { + log_trace!(logger, " ...trimming {} HTLC (hash {}) with value {} due to dust limit", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat); + } + } else { + let outbound = !local; + if outbound { + local_htlc_total_msat += htlc.amount_msat; + } else { + remote_htlc_total_msat += htlc.amount_msat; + } + let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + feerate_per_kw as u64 * htlc_success_tx_weight(self.get_channel_type()) / 1000 + }; + if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { + log_trace!(logger, " ...creating output for {} non-dust HTLC (hash {}) with value {}", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat); + included_non_dust_htlcs.push(htlc); + } else { + log_trace!(logger, " ...trimming {} HTLC (hash {}) with value {} due to dust limit", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat); + } + } + } + // TODO: When MSRV >= 1.66.0, use u64::checked_add_signed let mut value_to_self_msat = u64::try_from(funding.value_to_self_msat as i64 + value_to_self_msat_offset).unwrap(); // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie @@ -3562,21 +3577,6 @@ impl ChannelContext where SP::Target: SignerProvider { value_to_self_msat = u64::checked_sub(value_to_self_msat, local_htlc_total_msat).unwrap(); value_to_remote_msat = u64::checked_sub(value_to_remote_msat, remote_htlc_total_msat).unwrap(); - #[cfg(debug_assertions)] - { - // Make sure that the to_self/to_remote is always either past the appropriate - // channel_reserve *or* it is making progress towards it. - let mut broadcaster_max_commitment_tx_output = if generated_by_local { - funding.holder_max_commitment_tx_output.lock().unwrap() - } else { - funding.counterparty_max_commitment_tx_output.lock().unwrap() - }; - debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap()); - broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat); - debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis); - 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, included_non_dust_htlcs.len(), &self.channel_transaction_parameters.channel_type_features); let anchors_val = if self.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; let (value_to_self, value_to_remote) = if self.is_outbound() { @@ -3589,14 +3589,16 @@ impl ChannelContext where SP::Target: SignerProvider { let mut value_to_b = if local { value_to_remote } else { value_to_self }; if value_to_a >= broadcaster_dust_limit_satoshis { - log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a); + log_trace!(logger, " ...creating {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a); } else { + log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_local" } else { "to_remote" }, value_to_a); value_to_a = 0; } if value_to_b >= broadcaster_dust_limit_satoshis { - log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b); + log_trace!(logger, " ...creating {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b); } else { + log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_remote" } else { "to_local" }, value_to_b); value_to_b = 0; } @@ -3610,21 +3612,41 @@ impl ChannelContext where SP::Target: SignerProvider { value_to_a, value_to_b, feerate_per_kw, - included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc), + included_non_dust_htlcs.into_iter(), &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); + + #[cfg(debug_assertions)] + { + // Make sure that the to_self/to_remote is always either past the appropriate + // channel_reserve *or* it is making progress towards it. + let mut broadcaster_max_commitment_tx_output = if generated_by_local { + funding.holder_max_commitment_tx_output.lock().unwrap() + } else { + funding.counterparty_max_commitment_tx_output.lock().unwrap() + }; + debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap()); + broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat); + debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis); + broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat); + } + + htlcs_in_tx.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), + } + }); CommitmentStats { tx, feerate_per_kw, total_fee_sat, num_nondust_htlcs, - htlcs_included, + htlcs_included: htlcs_in_tx, local_balance_msat: value_to_self_msat, remote_balance_msat: value_to_remote_msat, inbound_htlc_preimages, From 82b40fc889cda84d9a3549bdc04888203e17233d Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 26 Feb 2025 07:51:49 +0000 Subject: [PATCH 09/18] Remove redundant fields in `CommitmentStats` The fields `feerate_per_kw` and `num_nondust_htlcs` of `CommitmentStats` can both be accessed directly from the `tx: CommitmentTransaction` field. --- lightning/src/ln/channel.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 9efcad735ac..c91f75da6d6 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -878,9 +878,7 @@ struct HTLCStats { /// An enum gathering stats on commitment transaction, either local or remote. struct CommitmentStats<'a> { tx: CommitmentTransaction, // the transaction info - feerate_per_kw: u32, // the feerate included to build the transaction total_fee_sat: u64, // the total fee included in the transaction - num_nondust_htlcs: usize, // the number of HTLC outputs (dust HTLCs *non*-included) htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction local_balance_msat: u64, // local balance before fees *not* considering dust limits remote_balance_msat: u64, // remote balance before fees *not* considering dust limits @@ -3602,8 +3600,6 @@ impl ChannelContext where SP::Target: SignerProvider { value_to_b = 0; } - let num_nondust_htlcs = included_non_dust_htlcs.len(); - let channel_parameters = if local { self.channel_transaction_parameters.as_holder_broadcastable() } else { self.channel_transaction_parameters.as_counterparty_broadcastable() }; @@ -3643,9 +3639,7 @@ impl ChannelContext where SP::Target: SignerProvider { CommitmentStats { tx, - feerate_per_kw, total_fee_sat, - num_nondust_htlcs, htlcs_included: htlcs_in_tx, local_balance_msat: value_to_self_msat, remote_balance_msat: value_to_remote_msat, @@ -5488,8 +5482,8 @@ impl FundedChannel where } } - if msg.htlc_signatures.len() != commitment_stats.num_nondust_htlcs { - return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs))); + if msg.htlc_signatures.len() != commitment_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_stats.tx.htlcs().len()))); } // Up to LDK 0.0.115, HTLC information was required to be duplicated in the @@ -5512,7 +5506,7 @@ impl FundedChannel where let holder_keys = TxCreationKeys::from_channel_static_keys(&self.holder_commitment_point.current_point(), self.context.get_holder_pubkeys(), self.context.get_counterparty_pubkeys(), &self.context.secp_ctx); for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() { if let Some(_) = htlc.transaction_output_index { - let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw, + let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.tx.feerate_per_kw(), self.context.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.context.channel_type, &holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key); @@ -6205,7 +6199,7 @@ impl FundedChannel where let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate); let commitment_stats = self.context.build_commitment_transaction(&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_stats.num_nondust_htlcs + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; + let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; let holder_balance_msat = commitment_stats.local_balance_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? @@ -8467,7 +8461,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, commitment_stats.num_nondust_htlcs, self.context.get_channel_type()) * 1000; + let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.htlcs().len(), self.context.get_channel_type()) * 1000; assert_eq!(actual_fee, info.fee); } } @@ -8514,7 +8508,7 @@ impl FundedChannel where let counterparty_keys = TxCreationKeys::from_channel_static_keys(&self.context.counterparty_cur_commitment_point.unwrap(), self.context.get_counterparty_pubkeys(), self.context.get_holder_pubkeys(), &self.context.secp_ctx); for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) { log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}", - encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.context.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), + encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.tx.feerate_per_kw(), self.context.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &counterparty_keys)), log_bytes!(counterparty_keys.broadcaster_htlc_key.to_public_key().serialize()), log_bytes!(htlc_sig.serialize_compact()[..]), &self.context.channel_id()); From 5c97de0619845851c29628fcaed952246a105fb6 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Thu, 27 Feb 2025 17:48:29 +0000 Subject: [PATCH 10/18] Tweak return values of `ChannelContext::build_commitment_transaction` We choose to include in `CommitmentStats` only fields that will be calculated or constructed solely by custom transaction builders. The other fields are created by channel, and placed in a new struct we call `CommitmentData`. --- lightning/src/ln/channel.rs | 80 +++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 31 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c91f75da6d6..b06a13aea9a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -875,15 +875,20 @@ struct HTLCStats { on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included } +/// An enum gathering data on a commitment, either local or remote. +struct CommitmentData<'a> { + 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 + inbound_htlc_preimages: Vec, // preimages for successful received HTLCs since last commitment +} + /// An enum gathering stats on commitment transaction, either local or remote. -struct CommitmentStats<'a> { +struct CommitmentStats { tx: CommitmentTransaction, // the transaction info total_fee_sat: u64, // the total fee included in the transaction - htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction local_balance_msat: u64, // local balance before fees *not* considering dust limits remote_balance_msat: u64, // remote balance before fees *not* considering dust limits - outbound_htlc_preimages: Vec, // preimages for successful offered HTLCs since last commitment - inbound_htlc_preimages: Vec, // preimages for successful received HTLCs since last commitment } /// Used when calculating whether we or the remote can afford an additional HTLC. @@ -1966,7 +1971,8 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide ) -> Result where L::Target: Logger { let funding_script = self.context().get_funding_redeemscript(); - let initial_commitment_tx = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger).tx; + 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 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().channel_value_satoshis); @@ -2003,7 +2009,8 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide } }; let context = self.context(); - let counterparty_initial_commitment_tx = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx; + 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_trusted_tx = counterparty_initial_commitment_tx.trust(); let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); @@ -3408,7 +3415,8 @@ impl ChannelContext where SP::Target: SignerProvider { /// 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) -> CommitmentStats + 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 num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); @@ -3637,12 +3645,16 @@ impl ChannelContext where SP::Target: SignerProvider { } }); - CommitmentStats { + let stats = CommitmentStats { tx, total_fee_sat, - htlcs_included: htlcs_in_tx, local_balance_msat: value_to_self_msat, remote_balance_msat: value_to_remote_msat, + }; + + CommitmentData { + stats, + htlcs_included: htlcs_in_tx, inbound_htlc_preimages, outbound_htlc_preimages, } @@ -4441,8 +4453,9 @@ impl ChannelContext where SP::Target: SignerProvider { SP::Target: SignerProvider, L::Target: Logger { - let counterparty_initial_commitment_tx = self.build_commitment_transaction( - funding, self.cur_counterparty_commitment_transaction_number, &self.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx; + 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; match self.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ref ecdsa) => { @@ -5435,7 +5448,8 @@ impl FundedChannel where let funding_script = self.context.get_funding_redeemscript(); - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, false, logger); + let commitment_data = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, false, logger); + let commitment_stats = commitment_data.stats; let commitment_txid = { let trusted_tx = commitment_stats.tx.trust(); let bitcoin_tx = trusted_tx.built_transaction(); @@ -5450,7 +5464,7 @@ impl FundedChannel where } bitcoin_tx.txid }; - let mut htlcs_cloned: Vec<_> = commitment_stats.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect(); + let mut 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. @@ -5541,7 +5555,7 @@ impl FundedChannel where self.context.counterparty_funding_pubkey() ); - self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages) + self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_data.outbound_htlc_preimages) .map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?; // Update state now that we've passed all the can-fail calls... @@ -6198,9 +6212,9 @@ impl FundedChannel where // Before proposing a feerate update, check that we can actually afford the new fee. let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate); - let commitment_stats = self.context.build_commitment_transaction(&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_stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; - let holder_balance_msat = commitment_stats.local_balance_msat - htlc_stats.outbound_holding_cell_msat; + let commitment_data = self.context.build_commitment_transaction(&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.context.get_channel_type()) * 1000; + let holder_balance_msat = commitment_data.stats.local_balance_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); @@ -6511,7 +6525,8 @@ impl FundedChannel where self.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); } let funding_signed = if self.context.signer_pending_funding && !self.context.is_outbound() { - let counterparty_initial_commitment_tx = 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).tx; + 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; self.context.get_funding_signed_msg(logger, counterparty_initial_commitment_tx) } else { None }; // Provide a `channel_ready` message if we need to, but only if we're _not_ still pending @@ -8447,8 +8462,8 @@ impl FundedChannel where -> (Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, CommitmentTransaction) where L::Target: Logger { - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger); - let counterparty_commitment_tx = commitment_stats.tx; + 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, true, logger); + let counterparty_commitment_tx = commitment_data.stats.tx; #[cfg(any(test, fuzzing))] { @@ -8468,7 +8483,7 @@ impl FundedChannel where } } - (commitment_stats.htlcs_included, counterparty_commitment_tx) + (commitment_data.htlcs_included, counterparty_commitment_tx) } /// Only fails in case of signer rejection. Used for channel_reestablish commitment_signed @@ -8478,7 +8493,9 @@ impl FundedChannel where #[cfg(any(test, fuzzing))] self.build_commitment_no_state_update(logger); - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger); + 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, true, logger); + let commitment_stats = commitment_data.stats; + let htlcs_included = commitment_data.htlcs_included; let counterparty_commitment_txid = commitment_stats.tx.trust().txid(); match &self.context.holder_signer { @@ -8486,15 +8503,15 @@ impl FundedChannel where let (signature, htlc_signatures); { - let mut htlcs = Vec::with_capacity(commitment_stats.htlcs_included.len()); - for &(ref htlc, _) in commitment_stats.htlcs_included.iter() { + let mut htlcs = Vec::with_capacity(htlcs_included.len()); + for &(ref htlc, _) in htlcs_included.iter() { htlcs.push(htlc); } let res = ecdsa.sign_counterparty_commitment( &commitment_stats.tx, - commitment_stats.inbound_htlc_preimages, - commitment_stats.outbound_htlc_preimages, + commitment_data.inbound_htlc_preimages, + commitment_data.outbound_htlc_preimages, &self.context.secp_ctx, ).map_err(|_| ChannelError::Ignore("Failed to get signatures for new commitment_signed".to_owned()))?; signature = res.0; @@ -8522,7 +8539,7 @@ impl FundedChannel where batch: None, #[cfg(taproot)] partial_signature_with_nonce: None, - }, (counterparty_commitment_txid, commitment_stats.htlcs_included))) + }, (counterparty_commitment_txid, htlcs_included))) }, // TODO (taproot|arik) #[cfg(taproot)] @@ -8957,7 +8974,8 @@ impl OutboundV1Channel where SP::Target: SignerProvider { /// Only allowed after [`ChannelContext::channel_transaction_parameters`] is set. fn get_funding_created_msg(&mut self, logger: &L) -> Option where L::Target: Logger { - let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger).tx; + 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 signature = match &self.context.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ecdsa) => { @@ -11609,12 +11627,12 @@ mod tests { $( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), * } ) => { { let (commitment_tx, htlcs): (_, Vec) = { - let mut commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger); + let mut commitment_data = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger); - let htlcs = commitment_stats.htlcs_included.drain(..) + let htlcs = commitment_data.htlcs_included.drain(..) .filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None }) .collect(); - (commitment_stats.tx, htlcs) + (commitment_data.stats.tx, htlcs) }; let trusted_tx = commitment_tx.trust(); let unsigned_tx = trusted_tx.built_transaction(); From 42d5a79ca0ca985b33d89d89d665519966cfdd20 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 26 Feb 2025 18:09:36 +0000 Subject: [PATCH 11/18] [Custom Transactions] Define the `TxBuilder` trait This commit defines the `TxBuilder` trait to give users the ability to customize the build of the lightning commitment transactions. The `TxBuilder` trait has a single method, `build_commitment_transaction`, which builds the commitment transaction, and populates the output indices of the HTLCs passed in. Besides that, it is mostly a copy paste of chunks of code from `ChannelContext::build_commitment_transaction`. --- lightning/src/ln/channel.rs | 122 ++--------------- lightning/src/sign/mod.rs | 1 + lightning/src/sign/tx_builder.rs | 222 +++++++++++++++++++++++++++++++ 3 files changed, 236 insertions(+), 109 deletions(-) create mode 100644 lightning/src/sign/tx_builder.rs diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b06a13aea9a..556f47de7f2 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -884,11 +884,11 @@ struct CommitmentData<'a> { } /// An enum gathering stats on commitment transaction, either local or remote. -struct CommitmentStats { - tx: CommitmentTransaction, // the transaction info - total_fee_sat: u64, // the total fee included in the transaction - local_balance_msat: u64, // local balance before fees *not* considering dust limits - remote_balance_msat: u64, // remote balance before fees *not* considering dust limits +pub(crate) struct CommitmentStats { + pub(crate) tx: CommitmentTransaction, // the transaction info + pub(crate) total_fee_sat: u64, // the total fee included in the transaction + pub(crate) local_balance_msat: u64, // local balance before fees *not* considering dust limits + pub(crate) remote_balance_msat: u64, // remote balance before fees *not* considering dust limits } /// Used when calculating whether we or the remote can afford an additional HTLC. @@ -3425,14 +3425,7 @@ impl ChannelContext where SP::Target: SignerProvider { // Non-dust htlcs will come first, with their output indices populated, then dust htlcs, with their output indices set to `None`. let mut htlcs_in_tx: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); - // We allocate this vector because we need to count the number of non-dust htlcs and calculate the total fee of the transaction - // before calling `CommitmentTransaction::new`. - // We could drop this vector and create two iterators: one to count the number of non-dust htlcs, and another to pass to `CommitmentTransaction::new` - let mut included_non_dust_htlcs: Vec<&mut HTLCOutputInCommitment> = Vec::with_capacity(num_htlcs); - let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis }; - let mut remote_htlc_total_msat = 0; - let mut local_htlc_total_msat = 0; let mut value_to_self_msat_offset = 0; let mut feerate_per_kw = self.feerate_per_kw; @@ -3532,94 +3525,12 @@ impl ChannelContext where SP::Target: SignerProvider { } } - // Trim dust htlcs - for (htlc, _) in htlcs_in_tx.iter_mut() { - if htlc.offered { - let outbound = local; - if outbound { - local_htlc_total_msat += htlc.amount_msat; - } else { - remote_htlc_total_msat += htlc.amount_msat; - } - let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * htlc_timeout_tx_weight(self.get_channel_type()) / 1000 - }; - if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { - log_trace!(logger, " ...creating output for {} non-dust HTLC (hash {}) with value {}", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat); - included_non_dust_htlcs.push(htlc); - } else { - log_trace!(logger, " ...trimming {} HTLC (hash {}) with value {} due to dust limit", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat); - } - } else { - let outbound = !local; - if outbound { - local_htlc_total_msat += htlc.amount_msat; - } else { - remote_htlc_total_msat += htlc.amount_msat; - } - let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * htlc_success_tx_weight(self.get_channel_type()) / 1000 - }; - if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { - log_trace!(logger, " ...creating output for {} non-dust HTLC (hash {}) with value {}", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat); - included_non_dust_htlcs.push(htlc); - } else { - log_trace!(logger, " ...trimming {} HTLC (hash {}) with value {} due to dust limit", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat); - } - } - } - // TODO: When MSRV >= 1.66.0, use u64::checked_add_signed - let mut value_to_self_msat = u64::try_from(funding.value_to_self_msat as i64 + value_to_self_msat_offset).unwrap(); - // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie - // AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to - // "violate" their reserve value by couting those against it. Thus, we have to do checked subtraction - // as otherwise we can overflow. - let mut value_to_remote_msat = u64::checked_sub(funding.channel_value_satoshis * 1000, value_to_self_msat).unwrap(); - value_to_self_msat = u64::checked_sub(value_to_self_msat, local_htlc_total_msat).unwrap(); - value_to_remote_msat = u64::checked_sub(value_to_remote_msat, remote_htlc_total_msat).unwrap(); - - let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), &self.channel_transaction_parameters.channel_type_features); - let anchors_val = if self.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; - let (value_to_self, value_to_remote) = if self.is_outbound() { - ((value_to_self_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat), value_to_remote_msat / 1000) - } else { - (value_to_self_msat / 1000, (value_to_remote_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat)) - }; + let value_to_self_with_offset_msat = u64::try_from(funding.value_to_self_msat as i64 + value_to_self_msat_offset).unwrap(); - let mut value_to_a = if local { value_to_self } else { value_to_remote }; - let mut value_to_b = if local { value_to_remote } else { value_to_self }; - - if value_to_a >= broadcaster_dust_limit_satoshis { - log_trace!(logger, " ...creating {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a); - } else { - log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_local" } else { "to_remote" }, value_to_a); - value_to_a = 0; - } - - if value_to_b >= broadcaster_dust_limit_satoshis { - log_trace!(logger, " ...creating {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b); - } else { - log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_remote" } else { "to_local" }, value_to_b); - value_to_b = 0; - } - - let channel_parameters = - if local { self.channel_transaction_parameters.as_holder_broadcastable() } - else { self.channel_transaction_parameters.as_counterparty_broadcastable() }; - let tx = CommitmentTransaction::new(commitment_number, - &per_commitment_point, - value_to_a, - value_to_b, - feerate_per_kw, - included_non_dust_htlcs.into_iter(), - &channel_parameters, - &self.secp_ctx, - ); + use crate::sign::tx_builder::{TxBuilder, SpecTxBuilder}; + let stats = TxBuilder::build_commitment_transaction(&SpecTxBuilder {}, local, commitment_number, per_commitment_point, &self.channel_transaction_parameters, &self.secp_ctx, + funding.channel_value_satoshis, value_to_self_with_offset_msat, htlcs_in_tx.iter_mut().map(|(htlc, _)| htlc), feerate_per_kw, broadcaster_dust_limit_satoshis, logger); #[cfg(debug_assertions)] { @@ -3630,10 +3541,10 @@ impl ChannelContext where SP::Target: SignerProvider { } else { funding.counterparty_max_commitment_tx_output.lock().unwrap() }; - debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap()); - broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat); - debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis); - broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat); + debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_msat || stats.local_balance_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap()); + broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_msat); + debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_msat || stats.remote_balance_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis); + broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_msat); } htlcs_in_tx.sort_unstable_by(|(htlc_a, _), (htlc_b, _)| { @@ -3645,13 +3556,6 @@ impl ChannelContext where SP::Target: SignerProvider { } }); - let stats = CommitmentStats { - tx, - total_fee_sat, - local_balance_msat: value_to_self_msat, - remote_balance_msat: value_to_remote_msat, - }; - CommitmentData { stats, htlcs_included: htlcs_in_tx, diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index 3ba950f548d..08a5db8ec86 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -81,6 +81,7 @@ pub(crate) mod type_resolver; pub mod ecdsa; #[cfg(taproot)] pub mod taproot; +pub(crate) mod tx_builder; /// Information about a spendable output to a P2WSH script. /// diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs new file mode 100644 index 00000000000..5c9d01746b1 --- /dev/null +++ b/lightning/src/sign/tx_builder.rs @@ -0,0 +1,222 @@ +//! Defines the `TxBuilder` trait, and the `SpecTxBuilder` type +#![allow(dead_code)] +#![allow(unused_variables)] + +use core::ops::Deref; + +use bitcoin::secp256k1::{self, PublicKey, Secp256k1}; + +use crate::ln::chan_utils::{ + self, ChannelTransactionParameters, CommitmentTransaction, HTLCOutputInCommitment, +}; +use crate::ln::channel::{self, CommitmentStats}; +use crate::prelude::*; +use crate::util::logger::Logger; + +/// A trait for types that can build commitment transactions, both for the holder, and the counterparty. +pub(crate) trait TxBuilder { + /// Build a commitment transaction, and populate the elements of `htlcs` with their output indices. + fn build_commitment_transaction<'a, L: Deref>( + &self, local: bool, commitment_number: u64, per_commitment_point: &PublicKey, + channel_transaction_parameters: &ChannelTransactionParameters, + secp_ctx: &Secp256k1, channel_value_satoshis: u64, + value_to_self_with_offset_msat: u64, + htlcs_in_tx: impl Iterator, feerate_per_kw: u32, + broadcaster_dust_limit_satoshis: u64, logger: &L, + ) -> CommitmentStats + where + L::Target: Logger; +} + +/// A type that builds commitment transactions according to the Lightning Specification. +#[derive(Clone, Debug, Default)] +pub(crate) struct SpecTxBuilder {} + +impl TxBuilder for SpecTxBuilder { + fn build_commitment_transaction<'a, L: Deref>( + &self, local: bool, commitment_number: u64, per_commitment_point: &PublicKey, + channel_transaction_parameters: &ChannelTransactionParameters, + secp_ctx: &Secp256k1, channel_value_satoshis: u64, + value_to_self_with_offset_msat: u64, + htlcs_in_tx: impl Iterator, feerate_per_kw: u32, + broadcaster_dust_limit_satoshis: u64, logger: &L, + ) -> CommitmentStats + where + L::Target: Logger, + { + // We allocate this vector because we need to count the number of non-dust htlcs and calculate the total fee of the transaction + // before calling `CommitmentTransaction::new`. + // We could drop this vector and create two iterators: one to count the number of non-dust htlcs, and another to pass to `CommitmentTransaction::new` + let mut included_non_dust_htlcs: Vec<&mut HTLCOutputInCommitment> = + Vec::with_capacity(htlcs_in_tx.size_hint().0); + + let mut remote_htlc_total_msat = 0; + let mut local_htlc_total_msat = 0; + + let channel_parameters = if local { + channel_transaction_parameters.as_holder_broadcastable() + } else { + channel_transaction_parameters.as_counterparty_broadcastable() + }; + let channel_type = channel_parameters.channel_type_features(); + + // Trim dust htlcs + for htlc in htlcs_in_tx { + if htlc.offered { + let outbound = local; + if outbound { + local_htlc_total_msat += htlc.amount_msat; + } else { + remote_htlc_total_msat += htlc.amount_msat; + } + let htlc_tx_fee = if channel_type.supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + feerate_per_kw as u64 * chan_utils::htlc_timeout_tx_weight(&channel_type) / 1000 + }; + if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { + log_trace!( + logger, + " ...creating output for {} non-dust HTLC (hash {}) with value {}", + if outbound { "outbound" } else { "inbound" }, + htlc.payment_hash, + htlc.amount_msat + ); + included_non_dust_htlcs.push(htlc); + } else { + log_trace!( + logger, + " ...trimming {} HTLC (hash {}) with value {} due to dust limit", + if outbound { "outbound" } else { "inbound" }, + htlc.payment_hash, + htlc.amount_msat + ); + } + } else { + let outbound = !local; + if outbound { + local_htlc_total_msat += htlc.amount_msat; + } else { + remote_htlc_total_msat += htlc.amount_msat; + } + let htlc_tx_fee = if channel_type.supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + feerate_per_kw as u64 * chan_utils::htlc_success_tx_weight(&channel_type) / 1000 + }; + if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { + log_trace!( + logger, + " ...creating output for {} non-dust HTLC (hash {}) with value {}", + if outbound { "outbound" } else { "inbound" }, + htlc.payment_hash, + htlc.amount_msat + ); + included_non_dust_htlcs.push(htlc); + } else { + log_trace!( + logger, + " ...trimming {} HTLC (hash {}) with value {} due to dust limit", + if outbound { "outbound" } else { "inbound" }, + htlc.payment_hash, + htlc.amount_msat + ); + } + } + } + + // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie + // AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to + // "violate" their reserve value by couting those against it. Thus, we have to do checked subtraction + // as otherwise we can overflow. + let mut value_to_remote_msat = + u64::checked_sub(channel_value_satoshis * 1000, value_to_self_with_offset_msat) + .unwrap(); + let value_to_self_msat = + u64::checked_sub(value_to_self_with_offset_msat, local_htlc_total_msat).unwrap(); + value_to_remote_msat = + u64::checked_sub(value_to_remote_msat, remote_htlc_total_msat).unwrap(); + + let total_fee_sat = chan_utils::commit_tx_fee_sat( + feerate_per_kw, + included_non_dust_htlcs.len(), + &channel_type, + ); + let anchors_val = if channel_type.supports_anchors_zero_fee_htlc_tx() { + channel::ANCHOR_OUTPUT_VALUE_SATOSHI * 2 + } else { + 0 + }; + let (value_to_self, value_to_remote) = + if channel_transaction_parameters.is_outbound_from_holder { + ( + (value_to_self_msat / 1000) + .saturating_sub(anchors_val) + .saturating_sub(total_fee_sat), + value_to_remote_msat / 1000, + ) + } else { + ( + value_to_self_msat / 1000, + (value_to_remote_msat / 1000) + .saturating_sub(anchors_val) + .saturating_sub(total_fee_sat), + ) + }; + + let mut value_to_a = if local { value_to_self } else { value_to_remote }; + let mut value_to_b = if local { value_to_remote } else { value_to_self }; + + if value_to_a >= broadcaster_dust_limit_satoshis { + log_trace!( + logger, + " ...creating {} output with value {}", + if local { "to_local" } else { "to_remote" }, + value_to_a + ); + } else { + log_trace!( + logger, + " ...trimming {} output with value {} due to dust limit", + if local { "to_local" } else { "to_remote" }, + value_to_a + ); + value_to_a = 0; + } + + if value_to_b >= broadcaster_dust_limit_satoshis { + log_trace!( + logger, + " ...creating {} output with value {}", + if local { "to_remote" } else { "to_local" }, + value_to_b + ); + } else { + log_trace!( + logger, + " ...trimming {} output with value {} due to dust limit", + if local { "to_remote" } else { "to_local" }, + value_to_b + ); + value_to_b = 0; + } + + let tx = CommitmentTransaction::new( + commitment_number, + &per_commitment_point, + value_to_a, + value_to_b, + feerate_per_kw, + included_non_dust_htlcs.into_iter(), + &channel_parameters, + &secp_ctx, + ); + + CommitmentStats { + tx, + total_fee_sat, + local_balance_msat: value_to_self_msat, + remote_balance_msat: value_to_remote_msat, + } + } +} From 300b3964423cb606cc23ead99af53374e6b099e8 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Sun, 2 Mar 2025 02:13:23 +0000 Subject: [PATCH 12/18] f: Just handle new updates for now, but don't set them --- lightning/src/chain/channelmonitor.rs | 36 ++++++++++++++++++--------- lightning/src/ln/channel.rs | 4 +-- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index caf4bb14456..a904281ae2e 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -548,7 +548,12 @@ pub(crate) enum ChannelMonitorUpdateStep { feerate_per_kw: Option, to_broadcaster_value_sat: Option, to_countersignatory_value_sat: Option, - commitment_tx: Option, + }, + LatestCounterpartyCommitmentTX { + // The dust and non-dust htlcs for that commitment + htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)>, + // Contains only the non-dust htlcs + commitment_tx: CommitmentTransaction, }, PaymentPreimage { payment_preimage: PaymentPreimage, @@ -577,6 +582,7 @@ impl ChannelMonitorUpdateStep { match self { ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { .. } => "LatestHolderCommitmentTXInfo", ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } => "LatestCounterpartyCommitmentTXInfo", + ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX { .. } => "LatestCounterpartyCommitmentTX", ChannelMonitorUpdateStep::PaymentPreimage { .. } => "PaymentPreimage", ChannelMonitorUpdateStep::CommitmentSecret { .. } => "CommitmentSecret", ChannelMonitorUpdateStep::ChannelForceClosed { .. } => "ChannelForceClosed", @@ -600,7 +606,6 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, (4, their_per_commitment_point, required), (5, to_countersignatory_value_sat, option), (6, htlc_outputs, required_vec), - (7, commitment_tx, option), }, (2, PaymentPreimage) => { (0, payment_preimage, required), @@ -616,6 +621,10 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, (5, ShutdownScript) => { (0, scriptpubkey, required), }, + (6, LatestCounterpartyCommitmentTX) => { + (0, htlc_outputs, required_vec), + (2, commitment_tx, required), + }, ); /// Indicates whether the balance is derived from a cooperative close, a force-close @@ -3256,6 +3265,10 @@ impl ChannelMonitorImpl { log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info"); self.provide_latest_counterparty_commitment_tx(*commitment_txid, htlc_outputs.clone(), *commitment_number, *their_per_commitment_point, logger) }, + ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX { htlc_outputs, commitment_tx } => { + log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info"); + self.provide_latest_counterparty_commitment_tx(commitment_tx.trust().txid(), htlc_outputs.clone(), commitment_tx.commitment_number(), commitment_tx.per_commitment_point(), logger) + }, ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage, payment_info } => { log_trace!(logger, "Updating ChannelMonitor with payment preimage"); self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array()), &payment_preimage, payment_info, broadcaster, &bounded_fee_estimator, logger) @@ -3318,6 +3331,7 @@ impl ChannelMonitorImpl { match update { ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { .. } |ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } + |ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX { .. } |ChannelMonitorUpdateStep::ShutdownScript { .. } |ChannelMonitorUpdateStep::CommitmentSecret { .. } => is_pre_close_update = true, @@ -3478,14 +3492,16 @@ impl ChannelMonitorImpl { fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec { update.updates.iter().filter_map(|update| { + // Soon we will drop the first branch here in favor of the second. + // In preparation, we just add the second branch without deleting the first. + // Next step: in channel, switch channel monitor updates to use the `LatestCounterpartyCommitmentTX` variant below. match update { - // Provided for forward compatibility; old updates won't contain the full transaction &ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, ref htlc_outputs, commitment_number, their_per_commitment_point, feerate_per_kw: Some(feerate_per_kw), to_broadcaster_value_sat: Some(to_broadcaster_value), - to_countersignatory_value_sat: Some(to_countersignatory_value), - commitment_tx: None } => { + to_countersignatory_value_sat: Some(to_countersignatory_value) + } => { let mut nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| { htlc.transaction_output_index.map(|_| htlc) @@ -3499,13 +3515,9 @@ impl ChannelMonitorImpl { Some(commitment_tx) }, - &ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid: _, - htlc_outputs: _, commitment_number: _, their_per_commitment_point: _, - feerate_per_kw: _, - to_broadcaster_value_sat: _, - to_countersignatory_value_sat: _, - commitment_tx: Some(ref commitment_tx) } => { - + &ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX { htlc_outputs: _, + ref commitment_tx, + } => { Some(commitment_tx.clone()) }, _ => None, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 556f47de7f2..4d3cf8a4d08 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -8345,6 +8345,8 @@ impl FundedChannel where let monitor_update = ChannelMonitorUpdate { update_id: self.context.latest_monitor_update_id, counterparty_node_id: Some(self.context.counterparty_node_id), + // Soon, we will switch this to `LatestCounterpartyCommitmentTX`, + // and provide the full commit tx instead of the information needed to rebuild it. updates: vec![ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid: counterparty_commitment_txid, htlc_outputs: htlcs.clone(), @@ -8353,8 +8355,6 @@ impl FundedChannel where feerate_per_kw: Some(counterparty_commitment_tx.feerate_per_kw()), to_broadcaster_value_sat: Some(counterparty_commitment_tx.to_broadcaster_value_sat()), to_countersignatory_value_sat: Some(counterparty_commitment_tx.to_countersignatory_value_sat()), - // This is everything we need, but we still provide all the above fields for downgrades - commitment_tx: Some(counterparty_commitment_tx), }], channel_id: Some(self.context.channel_id()), }; From 3318e4a5b576b1d8325220d35a51a05acc7e055d Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Sat, 1 Mar 2025 23:36:52 +0000 Subject: [PATCH 13/18] f: Remove parameter from `provide_initial_counterparty_commitment_tx` --- lightning/src/chain/channelmonitor.rs | 13 +++++-------- lightning/src/ln/channel.rs | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a904281ae2e..c22d2ecc0ee 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1517,14 +1517,12 @@ impl ChannelMonitor { /// This is used to provide the counterparty commitment information directly to the monitor /// before the initial persistence of a new channel. pub(crate) fn provide_initial_counterparty_commitment_tx( - &self, htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)>, - commitment_tx: CommitmentTransaction, logger: &L, - ) - where L::Target: Logger + &self, commitment_tx: CommitmentTransaction, logger: &L, + ) where L::Target: Logger { let mut inner = self.inner.lock().unwrap(); let logger = WithChannelMonitor::from_impl(logger, &*inner, None); - inner.provide_initial_counterparty_commitment_tx(htlc_outputs, commitment_tx, &logger); + inner.provide_initial_counterparty_commitment_tx(commitment_tx, &logger); } /// Informs this monitor of the latest counterparty (ie non-broadcastable) commitment transaction. @@ -2892,8 +2890,7 @@ impl ChannelMonitorImpl { } fn provide_initial_counterparty_commitment_tx( - &mut self, htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)>, - commitment_tx: CommitmentTransaction, logger: &WithChannelMonitor, + &mut self,commitment_tx: CommitmentTransaction, logger: &WithChannelMonitor, ) where L::Target: Logger { // We populate this field for downgrades self.initial_counterparty_commitment_info = Some((commitment_tx.per_commitment_point(), @@ -2904,7 +2901,7 @@ impl ChannelMonitorImpl { debug_assert_eq!(rebuilt_commitment_tx.trust().txid(), commitment_tx.trust().txid()); } - self.provide_latest_counterparty_commitment_tx(commitment_tx.trust().txid(), htlc_outputs, commitment_tx.commitment_number(), + self.provide_latest_counterparty_commitment_tx(commitment_tx.trust().txid(), Vec::new(), commitment_tx.commitment_number(), commitment_tx.per_commitment_point(), logger); // Soon, we will only populate this field self.initial_counterparty_commitment_tx = Some(commitment_tx); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 4d3cf8a4d08..9447785800a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2066,7 +2066,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide funding_redeemscript.clone(), self.funding().channel_value_satoshis, obscure_factor, holder_commitment_tx, best_block, context.counterparty_node_id, context.channel_id()); - channel_monitor.provide_initial_counterparty_commitment_tx(Vec::new(), counterparty_initial_commitment_tx.clone(), logger); + channel_monitor.provide_initial_counterparty_commitment_tx(counterparty_initial_commitment_tx.clone(), logger); self.context_mut().cur_counterparty_commitment_transaction_number -= 1; From 7c2d605bb424f43a041ba99fc256c9def00a126b Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Sat, 1 Mar 2025 23:21:29 +0000 Subject: [PATCH 14/18] f: Remove all explicit `TxCreationKeys` in channel --- lightning/src/ln/channel.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 9447785800a..4c612be59e7 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -41,7 +41,7 @@ use crate::ln::script::{self, ShutdownScript}; use crate::ln::channel_state::{ChannelShutdownState, CounterpartyForwardingInfo, InboundHTLCDetails, InboundHTLCStateDetails, OutboundHTLCDetails, OutboundHTLCStateDetails}; use crate::ln::channelmanager::{self, OpenChannelMessage, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentClaimDetails, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT}; use crate::ln::chan_utils::{ - CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight, + CounterpartyCommitmentSecrets, HTLCOutputInCommitment, htlc_success_tx_weight, htlc_timeout_tx_weight, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, @@ -5421,7 +5421,7 @@ impl FundedChannel where let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len()); let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len()); - let holder_keys = TxCreationKeys::from_channel_static_keys(&self.holder_commitment_point.current_point(), self.context.get_holder_pubkeys(), self.context.get_counterparty_pubkeys(), &self.context.secp_ctx); + let holder_keys = commitment_stats.tx.trust().keys(); for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() { if let Some(_) = htlc.transaction_output_index { let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.tx.feerate_per_kw(), @@ -8426,7 +8426,7 @@ impl FundedChannel where &counterparty_commitment_txid, encode::serialize_hex(&self.context.get_funding_redeemscript()), log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id()); - let counterparty_keys = TxCreationKeys::from_channel_static_keys(&self.context.counterparty_cur_commitment_point.unwrap(), self.context.get_counterparty_pubkeys(), self.context.get_holder_pubkeys(), &self.context.secp_ctx); + let counterparty_keys = commitment_stats.tx.trust().keys(); for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) { log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}", encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.tx.feerate_per_kw(), self.context.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), @@ -11439,7 +11439,7 @@ mod tests { use bitcoin::secp256k1::Message; use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, ecdsa::EcdsaChannelSigner}; use crate::types::payment::PaymentPreimage; - use crate::ln::channel::{HTLCOutputInCommitment ,TxCreationKeys}; + use crate::ln::channel::HTLCOutputInCommitment; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; use crate::ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters}; use crate::util::logger::Logger; @@ -11506,11 +11506,8 @@ mod tests { // We can't just use build_holder_transaction_keys here as the per_commitment_secret is not // derived from a commitment_seed, so instead we copy it here and call // build_commitment_transaction. - let delayed_payment_base = &chan.context.holder_signer.as_ref().pubkeys().delayed_payment_basepoint; let per_commitment_secret = SecretKey::from_slice(&>::from_hex("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap(); let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret); - let htlc_basepoint = &chan.context.holder_signer.as_ref().pubkeys().htlc_basepoint; - let keys = TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint); macro_rules! test_commitment { ( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $($remain:tt)* ) => { @@ -11579,6 +11576,7 @@ mod tests { let remote_signature = Signature::from_der(&>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap(); let ref htlc = 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.context.get_counterparty_selected_contest_delay().unwrap(), &htlc, $opt_anchors, &keys.broadcaster_delayed_payment_key, &keys.revocation_key); From ad05a2ce9d4f0b6612fca911ed643dfd06dde255 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Sat, 1 Mar 2025 23:29:27 +0000 Subject: [PATCH 15/18] f: Break up a long line --- lightning/src/ln/channel.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 4c612be59e7..58f9ed45caf 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5352,7 +5352,10 @@ impl FundedChannel where let funding_script = self.context.get_funding_redeemscript(); - let commitment_data = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, false, logger); + let commitment_data = self.context.build_commitment_transaction(&self.funding, + self.holder_commitment_point.transaction_number(), + &self.holder_commitment_point.current_point(), true, false, logger + ); let commitment_stats = commitment_data.stats; let commitment_txid = { let trusted_tx = commitment_stats.tx.trust(); From 338532fa966ad0739e9327fda0b8941017240f31 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Sun, 2 Mar 2025 00:03:50 +0000 Subject: [PATCH 16/18] f: Correct comments describing commitment structs --- lightning/src/ln/channel.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 58f9ed45caf..4bd951fad9e 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -875,7 +875,7 @@ struct HTLCStats { on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included } -/// An enum gathering data on a commitment, either local or remote. +/// A struct gathering data on a commitment, either local or remote. struct CommitmentData<'a> { stats: CommitmentStats, htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction @@ -883,7 +883,7 @@ struct CommitmentData<'a> { inbound_htlc_preimages: Vec, // preimages for successful received HTLCs since last commitment } -/// An enum gathering stats on commitment transaction, either local or remote. +/// A struct gathering stats on commitment transaction, either local or remote. pub(crate) struct CommitmentStats { pub(crate) tx: CommitmentTransaction, // the transaction info pub(crate) total_fee_sat: u64, // the total fee included in the transaction From 87a35dd92984b56b7a4482018b51004abc6f86e2 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Sun, 2 Mar 2025 00:18:05 +0000 Subject: [PATCH 17/18] f: Delete a big branch in non-dust htlc scan --- lightning/src/sign/tx_builder.rs | 91 +++++++++++--------------------- 1 file changed, 32 insertions(+), 59 deletions(-) diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index 5c9d01746b1..6b3ec88ab1b 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -62,66 +62,39 @@ impl TxBuilder for SpecTxBuilder { // Trim dust htlcs for htlc in htlcs_in_tx { - if htlc.offered { - let outbound = local; - if outbound { - local_htlc_total_msat += htlc.amount_msat; - } else { - remote_htlc_total_msat += htlc.amount_msat; - } - let htlc_tx_fee = if channel_type.supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * chan_utils::htlc_timeout_tx_weight(&channel_type) / 1000 - }; - if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { - log_trace!( - logger, - " ...creating output for {} non-dust HTLC (hash {}) with value {}", - if outbound { "outbound" } else { "inbound" }, - htlc.payment_hash, - htlc.amount_msat - ); - included_non_dust_htlcs.push(htlc); - } else { - log_trace!( - logger, - " ...trimming {} HTLC (hash {}) with value {} due to dust limit", - if outbound { "outbound" } else { "inbound" }, - htlc.payment_hash, - htlc.amount_msat - ); - } + let outbound = local == htlc.offered; + if outbound { + local_htlc_total_msat += htlc.amount_msat; } else { - let outbound = !local; - if outbound { - local_htlc_total_msat += htlc.amount_msat; - } else { - remote_htlc_total_msat += htlc.amount_msat; - } - let htlc_tx_fee = if channel_type.supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * chan_utils::htlc_success_tx_weight(&channel_type) / 1000 - }; - if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { - log_trace!( - logger, - " ...creating output for {} non-dust HTLC (hash {}) with value {}", - if outbound { "outbound" } else { "inbound" }, - htlc.payment_hash, - htlc.amount_msat - ); - included_non_dust_htlcs.push(htlc); - } else { - log_trace!( - logger, - " ...trimming {} HTLC (hash {}) with value {} due to dust limit", - if outbound { "outbound" } else { "inbound" }, - htlc.payment_hash, - htlc.amount_msat - ); - } + remote_htlc_total_msat += htlc.amount_msat; + } + let htlc_tx_fee = if channel_type.supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + feerate_per_kw as u64 + * if htlc.offered { + chan_utils::htlc_timeout_tx_weight(&channel_type) + } else { + chan_utils::htlc_success_tx_weight(&channel_type) + } / 1000 + }; + if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { + log_trace!( + logger, + " ...creating output for {} non-dust HTLC (hash {}) with value {}", + if outbound { "outbound" } else { "inbound" }, + htlc.payment_hash, + htlc.amount_msat + ); + included_non_dust_htlcs.push(htlc); + } else { + log_trace!( + logger, + " ...trimming {} HTLC (hash {}) with value {} due to dust limit", + if outbound { "outbound" } else { "inbound" }, + htlc.payment_hash, + htlc.amount_msat + ); } } From dcfb0f93b7032c31391a58634abf0d3c31918016 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Sun, 2 Mar 2025 01:40:18 +0000 Subject: [PATCH 18/18] Remove unnecessary allocation in `send_commitment_no_state_update` --- lightning/src/ln/channel.rs | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 4bd951fad9e..3057631f34a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -6618,7 +6618,7 @@ impl FundedChannel where log_trace!(logger, "Regenerating latest commitment update in channel {} with{} {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds", &self.context.channel_id(), if update_fee.is_some() { " update_fee," } else { "" }, update_add_htlcs.len(), update_fulfill_htlcs.len(), update_fail_htlcs.len(), update_fail_malformed_htlcs.len()); - let commitment_signed = if let Ok(update) = self.send_commitment_no_state_update(logger).map(|(cu, _)| cu) { + let commitment_signed = if let Ok(update) = self.send_commitment_no_state_update(logger) { if self.context.signer_pending_commitment_update { log_trace!(logger, "Commitment update generated: clearing signer_pending_commitment_update"); self.context.signer_pending_commitment_update = false; @@ -8395,14 +8395,13 @@ impl FundedChannel where /// Only fails in case of signer rejection. Used for channel_reestablish commitment_signed /// generation when we shouldn't change HTLC/channel state. - fn send_commitment_no_state_update(&self, logger: &L) -> Result<(msgs::CommitmentSigned, (Txid, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> where L::Target: Logger { + fn send_commitment_no_state_update(&self, logger: &L) -> Result where L::Target: Logger { // Get the fee tests from `build_commitment_no_state_update` #[cfg(any(test, fuzzing))] self.build_commitment_no_state_update(logger); 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, true, logger); let commitment_stats = commitment_data.stats; - let htlcs_included = commitment_data.htlcs_included; let counterparty_commitment_txid = commitment_stats.tx.trust().txid(); match &self.context.holder_signer { @@ -8410,11 +8409,6 @@ impl FundedChannel where let (signature, htlc_signatures); { - let mut htlcs = Vec::with_capacity(htlcs_included.len()); - for &(ref htlc, _) in htlcs_included.iter() { - htlcs.push(htlc); - } - let res = ecdsa.sign_counterparty_commitment( &commitment_stats.tx, commitment_data.inbound_htlc_preimages, @@ -8430,7 +8424,7 @@ impl FundedChannel where log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id()); let counterparty_keys = commitment_stats.tx.trust().keys(); - for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) { + for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(commitment_stats.tx.htlcs()) { log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}", encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.tx.feerate_per_kw(), self.context.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &counterparty_keys)), @@ -8439,14 +8433,14 @@ impl FundedChannel where } } - Ok((msgs::CommitmentSigned { + Ok(msgs::CommitmentSigned { channel_id: self.context.channel_id, signature, htlc_signatures, batch: None, #[cfg(taproot)] partial_signature_with_nonce: None, - }, (counterparty_commitment_txid, htlcs_included))) + }) }, // TODO (taproot|arik) #[cfg(taproot)] @@ -11530,14 +11524,8 @@ mod tests { ( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $opt_anchors: expr, { $( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), * } ) => { { - let (commitment_tx, htlcs): (_, Vec) = { - let mut commitment_data = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger); - - let htlcs = commitment_data.htlcs_included.drain(..) - .filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None }) - .collect(); - (commitment_data.stats.tx, htlcs) - }; + 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 trusted_tx = commitment_tx.trust(); let unsigned_tx = trusted_tx.built_transaction(); let redeemscript = chan.context.get_funding_redeemscript(); @@ -11552,10 +11540,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((htlcs[$htlc_idx].clone(), Some(remote_signature))); + per_htlc.push((commitment_tx.htlcs()[$htlc_idx].clone(), Some(remote_signature))); counterparty_htlc_sigs.push(remote_signature); })* - assert_eq!(htlcs.len(), per_htlc.len()); + assert_eq!(commitment_tx.htlcs().len(), per_htlc.len()); let holder_commitment_tx = HolderCommitmentTransaction::new( commitment_tx.clone(), @@ -11578,7 +11566,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 = htlcs[$htlc_idx]; + let ref htlc = commitment_tx.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.context.get_counterparty_selected_contest_delay().unwrap(),