From 6f6ce445e5520363af872846daa82999507f5cd0 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:52 -0700 Subject: [PATCH 1/9] Provide destination_script from ChannelMonitor to OnchainTxHandler The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. This commit removes the internal usage of `OnchainTxHandler::destination_script` and deprecates it, opting to instead provide it as a function argument wherever needed. --- lightning/src/chain/channelmonitor.rs | 46 ++++++++++++++++++++------- lightning/src/chain/onchaintx.rs | 46 +++++++++++++++++---------- 2 files changed, 64 insertions(+), 28 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5ed524ae764..87dc2f6b0f4 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2131,12 +2131,14 @@ impl ChannelMonitor { L::Target: Logger, { let fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator); - let mut inner = self.inner.lock().unwrap(); + let mut lock = self.inner.lock().unwrap(); + let inner = &mut *lock; let logger = WithChannelMonitor::from_impl(logger, &*inner, None); let current_height = inner.best_block.height; let conf_target = inner.closure_conf_target(); inner.onchain_tx_handler.rebroadcast_pending_claims( - current_height, FeerateStrategy::HighestOfPreviousOrNew, &broadcaster, conf_target, &fee_estimator, &logger, + current_height, FeerateStrategy::HighestOfPreviousOrNew, &broadcaster, conf_target, + &inner.destination_script, &fee_estimator, &logger, ); } @@ -2157,12 +2159,14 @@ impl ChannelMonitor { L::Target: Logger, { let fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator); - let mut inner = self.inner.lock().unwrap(); + let mut lock = self.inner.lock().unwrap(); + let inner = &mut *lock; let logger = WithChannelMonitor::from_impl(logger, &*inner, None); let current_height = inner.best_block.height; let conf_target = inner.closure_conf_target(); inner.onchain_tx_handler.rebroadcast_pending_claims( - current_height, FeerateStrategy::RetryPrevious, &broadcaster, conf_target, &fee_estimator, &logger, + current_height, FeerateStrategy::RetryPrevious, &broadcaster, conf_target, + &inner.destination_script, &fee_estimator, &logger, ); } @@ -3212,7 +3216,10 @@ impl ChannelMonitorImpl { ($commitment_number: expr, $txid: expr, $htlcs: expr) => { let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info($commitment_number, $txid, None, $htlcs); let conf_target = self.closure_conf_target(); - self.onchain_tx_handler.update_claims_view_from_requests(htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger); + self.onchain_tx_handler.update_claims_view_from_requests( + htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, + conf_target, &self.destination_script, fee_estimator, logger, + ); } } if let Some(txid) = self.funding.current_counterparty_commitment_txid { @@ -3261,7 +3268,10 @@ impl ChannelMonitorImpl { // transactions. let (claim_reqs, _) = self.get_broadcasted_holder_claims(&holder_commitment.tx, self.best_block.height); let conf_target = self.closure_conf_target(); - self.onchain_tx_handler.update_claims_view_from_requests(claim_reqs, self.best_block.height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger); + self.onchain_tx_handler.update_claims_view_from_requests( + claim_reqs, self.best_block.height, self.best_block.height, broadcaster, + conf_target, &self.destination_script, fee_estimator, logger, + ); } } } @@ -3324,7 +3334,7 @@ impl ChannelMonitorImpl { let conf_target = self.closure_conf_target(); self.onchain_tx_handler.update_claims_view_from_requests( claimable_outpoints, self.best_block.height, self.best_block.height, broadcaster, - conf_target, fee_estimator, logger, + conf_target, &self.destination_script, fee_estimator, logger, ); } @@ -4216,7 +4226,9 @@ impl ChannelMonitorImpl { log_trace!(logger, "Best block re-orged, replaced with new block {} at height {}", block_hash, height); self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height <= height); let conf_target = self.closure_conf_target(); - self.onchain_tx_handler.block_disconnected(height + 1, broadcaster, conf_target, fee_estimator, logger); + self.onchain_tx_handler.block_disconnected( + height + 1, broadcaster, conf_target, &self.destination_script, fee_estimator, logger, + ); Vec::new() } else { Vec::new() } } @@ -4540,8 +4552,14 @@ impl ChannelMonitorImpl { } let conf_target = self.closure_conf_target(); - self.onchain_tx_handler.update_claims_view_from_requests(claimable_outpoints, conf_height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger); - self.onchain_tx_handler.update_claims_view_from_matched_txn(&txn_matched, conf_height, conf_hash, self.best_block.height, broadcaster, conf_target, fee_estimator, logger); + self.onchain_tx_handler.update_claims_view_from_requests( + claimable_outpoints, conf_height, self.best_block.height, broadcaster, conf_target, + &self.destination_script, fee_estimator, logger, + ); + self.onchain_tx_handler.update_claims_view_from_matched_txn( + &txn_matched, conf_height, conf_hash, self.best_block.height, broadcaster, conf_target, + &self.destination_script, fee_estimator, logger, + ); // Determine new outputs to watch by comparing against previously known outputs to watch, // updating the latter in the process. @@ -4581,7 +4599,9 @@ impl ChannelMonitorImpl { let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator); let conf_target = self.closure_conf_target(); - self.onchain_tx_handler.block_disconnected(height, broadcaster, conf_target, &bounded_fee_estimator, logger); + self.onchain_tx_handler.block_disconnected( + height, broadcaster, conf_target, &self.destination_script, &bounded_fee_estimator, logger + ); self.best_block = BestBlock::new(header.prev_blockhash, height - 1); } @@ -4616,7 +4636,9 @@ impl ChannelMonitorImpl { debug_assert!(!self.onchain_events_awaiting_threshold_conf.iter().any(|ref entry| entry.txid == *txid)); let conf_target = self.closure_conf_target(); - self.onchain_tx_handler.transaction_unconfirmed(txid, broadcaster, conf_target, fee_estimator, logger); + self.onchain_tx_handler.transaction_unconfirmed( + txid, broadcaster, conf_target, &self.destination_script, fee_estimator, logger + ); } /// Filters a block's `txdata` for transactions spending watched outputs or for any child diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 5ec0713bae6..96b9cf2a222 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -232,7 +232,7 @@ pub(crate) enum FeerateStrategy { pub struct OnchainTxHandler { channel_value_satoshis: u64, channel_keys_id: [u8; 32], - destination_script: ScriptBuf, + destination_script: ScriptBuf, // Deprecated as of 0.2. holder_commitment: HolderCommitmentTransaction, prev_holder_commitment: Option, @@ -487,7 +487,8 @@ impl OnchainTxHandler { /// connections, like on mobile. pub(super) fn rebroadcast_pending_claims( &mut self, current_height: u32, feerate_strategy: FeerateStrategy, broadcaster: &B, - conf_target: ConfirmationTarget, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, + conf_target: ConfirmationTarget, destination_script: &Script, + fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) where B::Target: BroadcasterInterface, @@ -500,7 +501,10 @@ impl OnchainTxHandler { bump_requests.push((*claim_id, request.clone())); } for (claim_id, request) in bump_requests { - self.generate_claim(current_height, &request, &feerate_strategy, conf_target, fee_estimator, logger) + self.generate_claim( + current_height, &request, &feerate_strategy, conf_target, destination_script, + fee_estimator, logger, + ) .map(|(_, new_feerate, claim)| { let mut feerate_was_bumped = false; if let Some(mut_request) = self.pending_claim_requests.get_mut(&claim_id) { @@ -551,8 +555,9 @@ impl OnchainTxHandler { /// Panics if there are signing errors, because signing operations in reaction to on-chain /// events are not expected to fail, and if they do, we may lose funds. fn generate_claim( - &mut self, cur_height: u32, cached_request: &PackageTemplate, feerate_strategy: &FeerateStrategy, - conf_target: ConfirmationTarget, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, + &mut self, cur_height: u32, cached_request: &PackageTemplate, + feerate_strategy: &FeerateStrategy, conf_target: ConfirmationTarget, + destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Option<(u32, u64, OnchainClaim)> where F::Target: FeeEstimator, { @@ -617,15 +622,15 @@ impl OnchainTxHandler { } } - let predicted_weight = cached_request.package_weight(&self.destination_script); + let predicted_weight = cached_request.package_weight(destination_script); if let Some((output_value, new_feerate)) = cached_request.compute_package_output( - predicted_weight, self.destination_script.minimal_non_dust().to_sat(), + predicted_weight, destination_script.minimal_non_dust().to_sat(), feerate_strategy, conf_target, fee_estimator, logger, ) { assert!(new_feerate != 0); let transaction = cached_request.maybe_finalize_malleable_package( - cur_height, self, Amount::from_sat(output_value), self.destination_script.clone(), logger + cur_height, self, Amount::from_sat(output_value), destination_script.into(), logger ).unwrap(); assert!(predicted_weight >= transaction.0.weight().to_wu()); return Some((new_timer, new_feerate, OnchainClaim::Tx(transaction))); @@ -727,7 +732,7 @@ impl OnchainTxHandler { /// `cur_height`, however it must never be higher than `cur_height`. pub(super) fn update_claims_view_from_requests( &mut self, mut requests: Vec, conf_height: u32, cur_height: u32, - broadcaster: &B, conf_target: ConfirmationTarget, + broadcaster: &B, conf_target: ConfirmationTarget, destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator, logger: &L ) where B::Target: BroadcasterInterface, @@ -815,7 +820,8 @@ impl OnchainTxHandler { // height timer expiration (i.e in how many blocks we're going to take action). for mut req in preprocessed_requests { if let Some((new_timer, new_feerate, claim)) = self.generate_claim( - cur_height, &req, &FeerateStrategy::ForceBump, conf_target, &*fee_estimator, &*logger, + cur_height, &req, &FeerateStrategy::ForceBump, conf_target, destination_script, + &*fee_estimator, &*logger, ) { req.set_timer(new_timer); req.set_feerate(new_feerate); @@ -881,7 +887,7 @@ impl OnchainTxHandler { pub(super) fn update_claims_view_from_matched_txn( &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32, broadcaster: &B, conf_target: ConfirmationTarget, - fee_estimator: &LowerBoundedFeeEstimator, logger: &L + destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator, logger: &L ) where B::Target: BroadcasterInterface, F::Target: FeeEstimator, @@ -1032,7 +1038,8 @@ impl OnchainTxHandler { for (claim_id, request) in bump_candidates.iter() { if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim( - cur_height, &request, &FeerateStrategy::ForceBump, conf_target, &*fee_estimator, &*logger, + cur_height, &request, &FeerateStrategy::ForceBump, conf_target, destination_script, + &*fee_estimator, &*logger, ) { match bump_claim { OnchainClaim::Tx(bump_tx) => { @@ -1068,6 +1075,7 @@ impl OnchainTxHandler { txid: &Txid, broadcaster: B, conf_target: ConfirmationTarget, + destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) where @@ -1083,13 +1091,15 @@ impl OnchainTxHandler { } if let Some(height) = height { - self.block_disconnected(height, broadcaster, conf_target, fee_estimator, logger); + self.block_disconnected( + height, broadcaster, conf_target, destination_script, fee_estimator, logger, + ); } } pub(super) fn block_disconnected( &mut self, height: u32, broadcaster: B, conf_target: ConfirmationTarget, - fee_estimator: &LowerBoundedFeeEstimator, logger: &L, + destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) where B::Target: BroadcasterInterface, F::Target: FeeEstimator, @@ -1122,7 +1132,8 @@ impl OnchainTxHandler { // `height` is the height being disconnected, so our `current_height` is 1 lower. let current_height = height - 1; if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim( - current_height, &request, &FeerateStrategy::ForceBump, conf_target, fee_estimator, logger + current_height, &request, &FeerateStrategy::ForceBump, conf_target, + destination_script, fee_estimator, logger ) { request.set_timer(new_timer); request.set_feerate(new_feerate); @@ -1375,10 +1386,11 @@ mod tests { )); } let holder_commit = HolderCommitmentTransaction::dummy(1000000, &mut htlcs); + let destination_script = ScriptBuf::new(); let mut tx_handler = OnchainTxHandler::new( 1000000, [0; 32], - ScriptBuf::new(), + destination_script.clone(), signer, chan_params, holder_commit, @@ -1418,6 +1430,7 @@ mod tests { 1, &&broadcaster, ConfirmationTarget::UrgentOnChainSweep, + &destination_script, &fee_estimator, &logger, ); @@ -1441,6 +1454,7 @@ mod tests { 2, &&broadcaster, ConfirmationTarget::UrgentOnChainSweep, + &destination_script, &fee_estimator, &logger, ); From b8ee823cabb19f8fc5a502a8825db62637479f28 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:53 -0700 Subject: [PATCH 2/9] Track HolderCommitmentTransaction in HolderFundingOutput The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `HolderCommitmentTransaction` for the specific `FundingScope` the `HolderFundingOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `HolderCommitmentTransaction` and ensures any alternative commitment transaction state due to splicing does not leak into the `OnchainTxHandler`. As a result, we can now include all the information required to emit a `BumpTransactionEvent::ChannelClose` within its intermediate representation `ClaimEvent::BumpCommitment`, as opposed to relying on the `ChannelMonitor` to provide it. Along the way, this commit also fixes a bug where `BumpTransactionEvent::ChannelClose` could be emitted with an unsigned commitment transaction, resulting in the child transaction not being broadcastable due to a non-existent ancestor. This isn't a huge deal as we have retry logic for such claims; once the signer returns a valid signature, the event is re-emitted properly. --- lightning/src/chain/channelmonitor.rs | 13 ++--- lightning/src/chain/onchaintx.rs | 75 +++++++++++++++------------ lightning/src/chain/package.rs | 61 ++++++++++++++++------ 3 files changed, 94 insertions(+), 55 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 87dc2f6b0f4..e02932bd377 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3278,6 +3278,7 @@ impl ChannelMonitorImpl { fn generate_claimable_outpoints_and_watch_outputs(&mut self, reason: ClosureReason) -> (Vec, Vec) { let funding_outp = HolderFundingOutput::build( + self.funding.current_holder_commitment.tx.clone(), self.funding.redeem_script.clone(), self.funding.channel_parameters.channel_value_satoshis, self.channel_type_features().clone(), @@ -3530,16 +3531,12 @@ impl ChannelMonitorImpl { for (claim_id, claim_event) in pending_claim_events { match claim_event { ClaimEvent::BumpCommitment { - package_target_feerate_sat_per_1000_weight, commitment_tx, anchor_output_idx, + package_target_feerate_sat_per_1000_weight, commitment_tx, + commitment_tx_fee_satoshis, pending_nondust_htlcs, anchor_output_idx, } => { let channel_id = self.channel_id; let counterparty_node_id = self.counterparty_node_id; let commitment_txid = commitment_tx.compute_txid(); - debug_assert_eq!(self.funding.current_holder_commitment.tx.trust().txid(), commitment_txid); - let pending_htlcs = self.funding.current_holder_commitment.tx.trust().htlcs().to_vec(); - let channel_value_satoshis = self.funding.channel_parameters.channel_value_satoshis; - let commitment_tx_fee_satoshis = channel_value_satoshis - - commitment_tx.output.iter().fold(0u64, |sum, output| sum + output.value.to_sat()); ret.push(Event::BumpTransaction(BumpTransactionEvent::ChannelClose { channel_id, counterparty_node_id, @@ -3550,7 +3547,7 @@ impl ChannelMonitorImpl { anchor_descriptor: AnchorDescriptor { channel_derivation_parameters: ChannelDerivationParameters { keys_id: self.channel_keys_id, - value_satoshis: channel_value_satoshis, + value_satoshis: self.funding.channel_parameters.channel_value_satoshis, transaction_parameters: self.funding.channel_parameters.clone(), }, outpoint: BitcoinOutPoint { @@ -3558,7 +3555,7 @@ impl ChannelMonitorImpl { vout: anchor_output_idx, }, }, - pending_htlcs, + pending_htlcs: pending_nondust_htlcs, })); }, ClaimEvent::BumpHTLC { diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 96b9cf2a222..ab32a6830a9 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -193,6 +193,8 @@ pub(crate) enum ClaimEvent { BumpCommitment { package_target_feerate_sat_per_1000_weight: u32, commitment_tx: Transaction, + commitment_tx_fee_satoshis: u64, + pending_nondust_htlcs: Vec, anchor_output_idx: u32, }, /// Event yielded to signal that the commitment transaction has confirmed and its HTLCs must be @@ -641,37 +643,55 @@ impl OnchainTxHandler { // which require external funding. let mut inputs = cached_request.inputs(); debug_assert_eq!(inputs.len(), 1); - let tx = match cached_request.maybe_finalize_untractable_package(self, logger) { - Some(tx) => tx, - None => return None, - }; + if !cached_request.requires_external_funding() { - return Some((new_timer, 0, OnchainClaim::Tx(tx))); + return cached_request.maybe_finalize_untractable_package(self, logger) + .map(|tx| (new_timer, 0, OnchainClaim::Tx(tx))) } + return inputs.find_map(|input| match input { // Commitment inputs with anchors support are the only untractable inputs supported // thus far that require external funding. PackageSolvingData::HolderFundingOutput(output) => { - debug_assert_eq!(tx.0.compute_txid(), self.holder_commitment.trust().txid(), - "Holder commitment transaction mismatch"); + let maybe_signed_commitment_tx = output.get_maybe_signed_commitment_tx(self); + let tx = if maybe_signed_commitment_tx.is_fully_signed() { + maybe_signed_commitment_tx.0 + } else { + // We couldn't sign the commitment as the signer was unavailable, but we + // should still retry it later. We return the unsigned transaction anyway to + // register the claim. + return Some((new_timer, 0, OnchainClaim::Tx(maybe_signed_commitment_tx))); + }; + + let holder_commitment = output.commitment_tx.as_ref() + .unwrap_or(self.current_holder_commitment_tx()); + let input_amount_sats = if let Some(funding_amount_sats) = output.funding_amount_sats { + funding_amount_sats + } else { + debug_assert!(false, "Funding amount should always exist for anchor-based claims"); + self.channel_value_satoshis + }; + + let fee_sat = input_amount_sats - tx.output.iter() + .map(|output| output.value.to_sat()).sum::(); + let commitment_tx_feerate_sat_per_1000_weight = + compute_feerate_sat_per_1000_weight(fee_sat, tx.weight().to_wu()); let package_target_feerate_sat_per_1000_weight = cached_request .compute_package_feerate(fee_estimator, conf_target, feerate_strategy); - if let Some(input_amount_sat) = output.funding_amount { - let fee_sat = input_amount_sat - tx.0.output.iter().map(|output| output.value.to_sat()).sum::(); - let commitment_tx_feerate_sat_per_1000_weight = - compute_feerate_sat_per_1000_weight(fee_sat, tx.0.weight().to_wu()); - if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight { - log_debug!(logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW", - tx.0.compute_txid(), commitment_tx_feerate_sat_per_1000_weight, - package_target_feerate_sat_per_1000_weight); - return Some((new_timer, 0, OnchainClaim::Tx(tx.clone()))); - } + if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight { + log_debug!(logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW", + tx.compute_txid(), commitment_tx_feerate_sat_per_1000_weight, + package_target_feerate_sat_per_1000_weight); + // The commitment transaction already meets the required feerate and doesn't + // need a CPFP. We still want to return something other than the event to + // register the claim. + return Some((new_timer, 0, OnchainClaim::Tx(MaybeSignedTransaction(tx)))); } // We'll locate an anchor output we can spend within the commitment transaction. let funding_pubkey = &self.channel_transaction_parameters.holder_pubkeys.funding_pubkey; - match chan_utils::get_keyed_anchor_output(&tx.0, funding_pubkey) { + match chan_utils::get_keyed_anchor_output(&tx, funding_pubkey) { // An anchor output was found, so we should yield a funding event externally. Some((idx, _)) => { // TODO: Use a lower confirmation target when both our and the @@ -681,7 +701,9 @@ impl OnchainTxHandler { package_target_feerate_sat_per_1000_weight as u64, OnchainClaim::Event(ClaimEvent::BumpCommitment { package_target_feerate_sat_per_1000_weight, - commitment_tx: tx.0.clone(), + commitment_tx: tx, + pending_nondust_htlcs: holder_commitment.htlcs().to_vec(), + commitment_tx_fee_satoshis: fee_sat, anchor_output_idx: idx, }), )) @@ -690,7 +712,7 @@ impl OnchainTxHandler { // attempt to broadcast the transaction with its current fee rate and hope // it confirms. This is essentially the same behavior as a commitment // transaction without anchor outputs. - None => Some((new_timer, 0, OnchainClaim::Tx(tx.clone()))), + None => Some((new_timer, 0, OnchainClaim::Tx(MaybeSignedTransaction(tx)))), } }, _ => { @@ -1193,17 +1215,6 @@ impl OnchainTxHandler { self.prev_holder_commitment = Some(replace(&mut self.holder_commitment, tx)); } - pub(crate) fn get_unsigned_holder_commitment_tx(&self) -> &Transaction { - &self.holder_commitment.trust().built_transaction().transaction - } - - pub(crate) fn get_maybe_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> MaybeSignedTransaction { - let tx = self.signer.sign_holder_commitment(&self.channel_transaction_parameters, &self.holder_commitment, &self.secp_ctx) - .map(|sig| self.holder_commitment.add_holder_sig(funding_redeemscript, sig)) - .unwrap_or_else(|_| self.get_unsigned_holder_commitment_tx().clone()); - MaybeSignedTransaction(tx) - } - #[cfg(any(test, feature="_test_utils", feature="unsafe_revoked_tx_signing"))] pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction { let sig = self.signer.unsafe_sign_holder_commitment(&self.channel_transaction_parameters, &self.holder_commitment, &self.secp_ctx).expect("sign holder commitment"); @@ -1410,7 +1421,7 @@ mod tests { let logger = TestLogger::new(); // 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 holder_commit_txid = tx_handler.current_holder_commitment_tx().trust().txid(); let mut requests = Vec::new(); for (htlc, _) in htlcs { requests.push(PackageTemplate::build_package( diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index a07fd9d46a2..75404983060 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -20,12 +20,14 @@ use bitcoin::transaction::{TxOut,TxIn, Transaction}; use bitcoin::transaction::OutPoint as BitcoinOutPoint; use bitcoin::script::{Script, ScriptBuf}; use bitcoin::hash_types::Txid; -use bitcoin::secp256k1::{SecretKey,PublicKey}; +use bitcoin::secp256k1::{SecretKey, PublicKey}; use bitcoin::sighash::EcdsaSighashType; use bitcoin::transaction::Version; use crate::types::payment::PaymentPreimage; -use crate::ln::chan_utils::{self, TxCreationKeys, HTLCOutputInCommitment}; +use crate::ln::chan_utils::{ + self, HolderCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment, +}; use crate::types::features::ChannelTypeFeatures; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; use crate::ln::channelmanager::MIN_CLTV_EXPIRY_DELTA; @@ -440,19 +442,41 @@ impl Readable for HolderHTLCOutput { #[derive(Clone, PartialEq, Eq)] pub(crate) struct HolderFundingOutput { funding_redeemscript: ScriptBuf, - pub(crate) funding_amount: Option, + pub(crate) funding_amount_sats: Option, channel_type_features: ChannelTypeFeatures, + pub(crate) commitment_tx: Option, } impl HolderFundingOutput { - pub(crate) fn build(funding_redeemscript: ScriptBuf, funding_amount: u64, channel_type_features: ChannelTypeFeatures) -> Self { + pub(crate) fn build( + commitment_tx: HolderCommitmentTransaction, funding_redeemscript: ScriptBuf, + funding_amount_sats: u64, channel_type_features: ChannelTypeFeatures, + ) -> Self { HolderFundingOutput { funding_redeemscript, - funding_amount: Some(funding_amount), + funding_amount_sats: Some(funding_amount_sats), channel_type_features, + commitment_tx: Some(commitment_tx), } } + + pub(crate) fn get_maybe_signed_commitment_tx( + &self, onchain_tx_handler: &mut OnchainTxHandler, + ) -> MaybeSignedTransaction { + let channel_parameters = &onchain_tx_handler.channel_transaction_parameters; + let commitment_tx = self.commitment_tx.as_ref() + .unwrap_or(onchain_tx_handler.current_holder_commitment_tx()); + let maybe_signed_tx = onchain_tx_handler.signer + .sign_holder_commitment(channel_parameters, commitment_tx, &onchain_tx_handler.secp_ctx) + .map(|holder_sig| { + commitment_tx.add_holder_sig(&self.funding_redeemscript, holder_sig) + }) + .unwrap_or_else(|_| { + commitment_tx.trust().built_transaction().transaction.clone() + }); + MaybeSignedTransaction(maybe_signed_tx) + } } impl Writeable for HolderFundingOutput { @@ -462,7 +486,8 @@ impl Writeable for HolderFundingOutput { (0, self.funding_redeemscript, required), (1, self.channel_type_features, required), (2, legacy_deserialization_prevention_marker, option), - (3, self.funding_amount, option), + (3, self.funding_amount_sats, option), + (5, self.commitment_tx, option), // Added in 0.2 }); Ok(()) } @@ -473,13 +498,15 @@ impl Readable for HolderFundingOutput { let mut funding_redeemscript = RequiredWrapper(None); let mut _legacy_deserialization_prevention_marker: Option<()> = None; let mut channel_type_features = None; - let mut funding_amount = None; + let mut funding_amount_sats = None; + let mut commitment_tx = None; read_tlv_fields!(reader, { (0, funding_redeemscript, required), (1, channel_type_features, option), (2, _legacy_deserialization_prevention_marker, option), - (3, funding_amount, option), + (3, funding_amount_sats, option), + (5, commitment_tx, option), // Added in 0.2. }); verify_channel_type_features(&channel_type_features, None)?; @@ -487,7 +514,8 @@ impl Readable for HolderFundingOutput { Ok(Self { funding_redeemscript: funding_redeemscript.0.unwrap(), channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()), - funding_amount + funding_amount_sats, + commitment_tx, }) } } @@ -519,7 +547,7 @@ impl PackageSolvingData { }, PackageSolvingData::HolderFundingOutput(ref outp) => { debug_assert!(outp.channel_type_features.supports_anchors_zero_fee_htlc_tx()); - outp.funding_amount.unwrap() + outp.funding_amount_sats.unwrap() } }; amt @@ -716,7 +744,7 @@ impl PackageSolvingData { onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage) } PackageSolvingData::HolderFundingOutput(ref outp) => { - Some(onchain_handler.get_maybe_signed_holder_tx(&outp.funding_redeemscript)) + Some(outp.get_maybe_signed_commitment_tx(onchain_handler)) } _ => { panic!("API Error!"); } } @@ -1406,7 +1434,7 @@ where mod tests { use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageTemplate, PackageSolvingData, RevokedHTLCOutput, RevokedOutput, WEIGHT_REVOKED_OUTPUT, weight_offered_htlc, weight_received_htlc, feerate_bump}; use crate::chain::Txid; - use crate::ln::chan_utils::HTLCOutputInCommitment; + use crate::ln::chan_utils::{HolderCommitmentTransaction, HTLCOutputInCommitment}; use crate::types::payment::{PaymentPreimage, PaymentHash}; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; @@ -1508,9 +1536,12 @@ mod tests { } macro_rules! dumb_funding_output { - () => { - PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build(ScriptBuf::new(), 0, ChannelTypeFeatures::only_static_remote_key())) - } + () => {{ + let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut Vec::new()); + PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build( + commitment_tx, ScriptBuf::new(), 0, ChannelTypeFeatures::only_static_remote_key() + )) + }} } #[test] From d2f283533a3cb350c36d0ef119722105f24eb2a0 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:54 -0700 Subject: [PATCH 3/9] Track ChannelTransactionParameters in HolderFundingOutput The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `HolderFundingOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`. --- lightning/src/chain/channelmonitor.rs | 9 ++++---- lightning/src/chain/onchaintx.rs | 6 ++++- lightning/src/chain/package.rs | 32 ++++++++++++++++++++------- 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index e02932bd377..b04c367db75 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3279,9 +3279,7 @@ impl ChannelMonitorImpl { fn generate_claimable_outpoints_and_watch_outputs(&mut self, reason: ClosureReason) -> (Vec, Vec) { let funding_outp = HolderFundingOutput::build( self.funding.current_holder_commitment.tx.clone(), - self.funding.redeem_script.clone(), - self.funding.channel_parameters.channel_value_satoshis, - self.channel_type_features().clone(), + self.funding.channel_parameters.clone(), ); let funding_outpoint = self.get_funding_txo(); let commitment_package = PackageTemplate::build_package( @@ -3533,6 +3531,7 @@ impl ChannelMonitorImpl { ClaimEvent::BumpCommitment { package_target_feerate_sat_per_1000_weight, commitment_tx, commitment_tx_fee_satoshis, pending_nondust_htlcs, anchor_output_idx, + channel_parameters, } => { let channel_id = self.channel_id; let counterparty_node_id = self.counterparty_node_id; @@ -3547,8 +3546,8 @@ impl ChannelMonitorImpl { anchor_descriptor: AnchorDescriptor { channel_derivation_parameters: ChannelDerivationParameters { keys_id: self.channel_keys_id, - value_satoshis: self.funding.channel_parameters.channel_value_satoshis, - transaction_parameters: self.funding.channel_parameters.clone(), + value_satoshis: channel_parameters.channel_value_satoshis, + transaction_parameters: channel_parameters, }, outpoint: BitcoinOutPoint { txid: commitment_txid, diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index ab32a6830a9..66935f2f52e 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -196,6 +196,7 @@ pub(crate) enum ClaimEvent { commitment_tx_fee_satoshis: u64, pending_nondust_htlcs: Vec, anchor_output_idx: u32, + channel_parameters: ChannelTransactionParameters, }, /// Event yielded to signal that the commitment transaction has confirmed and its HTLCs must be /// resolved by broadcasting a transaction with sufficient fee to claim them. @@ -690,7 +691,9 @@ impl OnchainTxHandler { } // We'll locate an anchor output we can spend within the commitment transaction. - let funding_pubkey = &self.channel_transaction_parameters.holder_pubkeys.funding_pubkey; + let channel_parameters = output.channel_parameters.as_ref() + .unwrap_or(&self.channel_transaction_parameters); + let funding_pubkey = &channel_parameters.holder_pubkeys.funding_pubkey; match chan_utils::get_keyed_anchor_output(&tx, funding_pubkey) { // An anchor output was found, so we should yield a funding event externally. Some((idx, _)) => { @@ -705,6 +708,7 @@ impl OnchainTxHandler { pending_nondust_htlcs: holder_commitment.htlcs().to_vec(), commitment_tx_fee_satoshis: fee_sat, anchor_output_idx: idx, + channel_parameters: channel_parameters.clone(), }), )) }, diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 75404983060..74bf2558d94 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -26,7 +26,8 @@ use bitcoin::transaction::Version; use crate::types::payment::PaymentPreimage; use crate::ln::chan_utils::{ - self, HolderCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment, + self, ChannelTransactionParameters, HolderCommitmentTransaction, TxCreationKeys, + HTLCOutputInCommitment, }; use crate::types::features::ChannelTypeFeatures; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; @@ -38,7 +39,7 @@ use crate::chain::transaction::MaybeSignedTransaction; use crate::sign::ecdsa::EcdsaChannelSigner; use crate::chain::onchaintx::{FeerateStrategy, ExternalHTLCClaim, OnchainTxHandler}; use crate::util::logger::Logger; -use crate::util::ser::{Readable, Writer, Writeable, RequiredWrapper}; +use crate::util::ser::{Readable, ReadableArgs, Writer, Writeable, RequiredWrapper}; use crate::io; use core::cmp; @@ -445,26 +446,31 @@ pub(crate) struct HolderFundingOutput { pub(crate) funding_amount_sats: Option, channel_type_features: ChannelTypeFeatures, pub(crate) commitment_tx: Option, + pub(crate) channel_parameters: Option, } impl HolderFundingOutput { pub(crate) fn build( - commitment_tx: HolderCommitmentTransaction, funding_redeemscript: ScriptBuf, - funding_amount_sats: u64, channel_type_features: ChannelTypeFeatures, + commitment_tx: HolderCommitmentTransaction, channel_parameters: ChannelTransactionParameters, ) -> Self { + let funding_redeemscript = channel_parameters.make_funding_redeemscript(); + let funding_amount_sats = channel_parameters.channel_value_satoshis; + let channel_type_features = channel_parameters.channel_type_features.clone(); HolderFundingOutput { funding_redeemscript, funding_amount_sats: Some(funding_amount_sats), channel_type_features, commitment_tx: Some(commitment_tx), + channel_parameters: Some(channel_parameters), } } pub(crate) fn get_maybe_signed_commitment_tx( &self, onchain_tx_handler: &mut OnchainTxHandler, ) -> MaybeSignedTransaction { - let channel_parameters = &onchain_tx_handler.channel_transaction_parameters; + let channel_parameters = self.channel_parameters.as_ref() + .unwrap_or(&onchain_tx_handler.channel_transaction_parameters); let commitment_tx = self.commitment_tx.as_ref() .unwrap_or(onchain_tx_handler.current_holder_commitment_tx()); let maybe_signed_tx = onchain_tx_handler.signer @@ -488,6 +494,7 @@ impl Writeable for HolderFundingOutput { (2, legacy_deserialization_prevention_marker, option), (3, self.funding_amount_sats, option), (5, self.commitment_tx, option), // Added in 0.2 + (7, self.channel_parameters, option), // Added in 0.2. }); Ok(()) } @@ -500,6 +507,7 @@ impl Readable for HolderFundingOutput { let mut channel_type_features = None; let mut funding_amount_sats = None; let mut commitment_tx = None; + let mut channel_parameters = None; read_tlv_fields!(reader, { (0, funding_redeemscript, required), @@ -507,15 +515,19 @@ impl Readable for HolderFundingOutput { (2, _legacy_deserialization_prevention_marker, option), (3, funding_amount_sats, option), (5, commitment_tx, option), // Added in 0.2. + (7, channel_parameters, (option: ReadableArgs, None)), // Added in 0.2. }); verify_channel_type_features(&channel_type_features, None)?; + let channel_type_features = + channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()); Ok(Self { funding_redeemscript: funding_redeemscript.0.unwrap(), - channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()), funding_amount_sats, + channel_type_features, commitment_tx, + channel_parameters, }) } } @@ -1434,7 +1446,9 @@ where mod tests { use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageTemplate, PackageSolvingData, RevokedHTLCOutput, RevokedOutput, WEIGHT_REVOKED_OUTPUT, weight_offered_htlc, weight_received_htlc, feerate_bump}; use crate::chain::Txid; - use crate::ln::chan_utils::{HolderCommitmentTransaction, HTLCOutputInCommitment}; + use crate::ln::chan_utils::{ + ChannelTransactionParameters, HolderCommitmentTransaction, HTLCOutputInCommitment, + }; use crate::types::payment::{PaymentPreimage, PaymentHash}; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; @@ -1538,8 +1552,10 @@ mod tests { macro_rules! dumb_funding_output { () => {{ let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut Vec::new()); + let mut channel_parameters = ChannelTransactionParameters::test_dummy(0); + channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key(); PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build( - commitment_tx, ScriptBuf::new(), 0, ChannelTypeFeatures::only_static_remote_key() + commitment_tx, channel_parameters, )) }} } From 0c9abbc9e1009999a6113eda1ebdd5ec53eadb0c Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:54 -0700 Subject: [PATCH 4/9] Track HTLCDescriptor in HolderHTLCOutput The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. This commit tracks the `HTLCDescriptor` for the specific `FundingScope` the `HolderHTLCOutput` claim originated from. Previously, when claiming `HolderHTLCOutput`s, the `OnchainTxHandler` had to check which holder commitment the HTLC belonged to in order to produce an `HTLCDescriptor` for signing. We still maintain the legacy logic for existing claims which have not had an `HTLCDescriptor` written yet. Along the way, we refactor the claim process for such outputs to decouple them from the `OnchainTxHandler`. As a result, the `OnchainTxHandler` is only used to obtain references to the signer and secp256k1 context. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. --- lightning/src/chain/channelmonitor.rs | 134 +++++++++++--------- lightning/src/chain/onchaintx.rs | 151 ++++++---------------- lightning/src/chain/package.rs | 176 +++++++++++++++++++++++--- lightning/src/ln/chan_utils.rs | 36 ------ lightning/src/ln/channel.rs | 16 ++- 5 files changed, 274 insertions(+), 239 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index b04c367db75..114c0daf885 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3562,29 +3562,12 @@ impl ChannelMonitorImpl { } => { let channel_id = self.channel_id; let counterparty_node_id = self.counterparty_node_id; - let mut htlc_descriptors = Vec::with_capacity(htlcs.len()); - for htlc in htlcs { - htlc_descriptors.push(HTLCDescriptor { - channel_derivation_parameters: ChannelDerivationParameters { - keys_id: self.channel_keys_id, - value_satoshis: self.funding.channel_parameters.channel_value_satoshis, - transaction_parameters: self.funding.channel_parameters.clone(), - }, - commitment_txid: htlc.commitment_txid, - per_commitment_number: htlc.per_commitment_number, - per_commitment_point: htlc.per_commitment_point, - feerate_per_kw: 0, - htlc: htlc.htlc, - preimage: htlc.preimage, - counterparty_sig: htlc.counterparty_sig, - }); - } ret.push(Event::BumpTransaction(BumpTransactionEvent::HTLCResolution { channel_id, counterparty_node_id, claim_id, target_feerate_sat_per_1000_weight, - htlc_descriptors, + htlc_descriptors: htlcs, tx_lock_time, })); } @@ -3973,14 +3956,50 @@ impl ChannelMonitorImpl { (claimable_outpoints, outputs_to_watch) } + fn get_broadcasted_holder_htlc_descriptors( + &self, holder_tx: &HolderCommitmentTransaction, + ) -> Vec { + let tx = holder_tx.trust(); + let mut htlcs = Vec::with_capacity(holder_tx.htlcs().len()); + debug_assert_eq!(holder_tx.htlcs().len(), holder_tx.counterparty_htlc_sigs.len()); + for (htlc, counterparty_sig) in holder_tx.htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter()) { + assert!(htlc.transaction_output_index.is_some(), "Expected transaction output index for non-dust HTLC"); + + let preimage = if htlc.offered { + None + } else if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) { + Some(*preimage) + } else { + // We can't build an HTLC-Success transaction without the preimage + continue; + }; + + htlcs.push(HTLCDescriptor { + // TODO(splicing): Consider alternative funding scopes. + channel_derivation_parameters: ChannelDerivationParameters { + value_satoshis: self.funding.channel_parameters.channel_value_satoshis, + keys_id: self.channel_keys_id, + transaction_parameters: self.funding.channel_parameters.clone(), + }, + commitment_txid: tx.txid(), + per_commitment_number: tx.commitment_number(), + per_commitment_point: tx.per_commitment_point(), + feerate_per_kw: tx.feerate_per_kw(), + htlc: htlc.clone(), + preimage, + counterparty_sig: *counterparty_sig, + }); + } + + htlcs + } + // Returns (1) `PackageTemplate`s that can be given to the OnchainTxHandler, so that the handler can // broadcast transactions claiming holder HTLC commitment outputs and (2) a holder revokable // script so we can detect whether a holder transaction has been seen on-chain. fn get_broadcasted_holder_claims( &self, holder_tx: &HolderCommitmentTransaction, conf_height: u32, ) -> (Vec, Option<(ScriptBuf, PublicKey, RevocationKey)>) { - let mut claim_requests = Vec::with_capacity(holder_tx.htlcs().len()); - let tx = holder_tx.trust(); let keys = tx.keys(); let redeem_script = chan_utils::get_revokeable_redeemscript( @@ -3990,36 +4009,22 @@ impl ChannelMonitorImpl { redeem_script.to_p2wsh(), holder_tx.per_commitment_point(), keys.revocation_key.clone(), )); - let txid = tx.txid(); - for htlc in holder_tx.htlcs() { - if let Some(transaction_output_index) = htlc.transaction_output_index { - let (htlc_output, counterparty_spendable_height) = if htlc.offered { - let htlc_output = HolderHTLCOutput::build_offered( - htlc.amount_msat, htlc.cltv_expiry, self.channel_type_features().clone() - ); - (htlc_output, conf_height) + let claim_requests = self.get_broadcasted_holder_htlc_descriptors(holder_tx).into_iter() + .map(|htlc_descriptor| { + let counterparty_spendable_height = if htlc_descriptor.htlc.offered { + conf_height } else { - let payment_preimage = if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) { - preimage.clone() - } else { - // We can't build an HTLC-Success transaction without the preimage - continue; - }; - let htlc_output = HolderHTLCOutput::build_accepted( - payment_preimage, htlc.amount_msat, self.channel_type_features().clone() - ); - (htlc_output, htlc.cltv_expiry) + htlc_descriptor.htlc.cltv_expiry }; - let htlc_package = PackageTemplate::build_package( - txid, transaction_output_index, - PackageSolvingData::HolderHTLCOutput(htlc_output), + let transaction_output_index = htlc_descriptor.htlc.transaction_output_index + .expect("Expected transaction output index for non-dust HTLC"); + PackageTemplate::build_package( + tx.txid(), transaction_output_index, + PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(htlc_descriptor)), counterparty_spendable_height, - ); - claim_requests.push(htlc_package); - } else { - debug_assert!(false, "Expected transaction output index for non-dust HTLC"); - } - } + ) + }) + .collect(); (claim_requests, broadcasted_holder_revokable_script) } @@ -4153,33 +4158,36 @@ impl ChannelMonitorImpl { &mut self, logger: &WithChannelMonitor ) -> Vec where L::Target: Logger { log_debug!(logger, "Getting signed copy of latest holder commitment transaction!"); - let commitment_tx = self.onchain_tx_handler.get_fully_signed_copy_holder_tx(&self.funding.redeem_script); - let txid = commitment_tx.compute_txid(); + let commitment_tx = { + let sig = self.onchain_tx_handler.signer.unsafe_sign_holder_commitment( + &self.funding.channel_parameters, &self.funding.current_holder_commitment.tx, + &self.onchain_tx_handler.secp_ctx, + ).expect("sign holder commitment"); + self.funding.current_holder_commitment.tx.add_holder_sig(&self.funding.redeem_script, sig) + }; let mut holder_transactions = vec![commitment_tx]; // When anchor outputs are present, the HTLC transactions are only final once the commitment // transaction confirms due to the CSV 1 encumberance. if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() { return holder_transactions; } - for htlc in self.funding.current_holder_commitment.tx.htlcs() { - if let Some(vout) = htlc.transaction_output_index { - let preimage = if !htlc.offered { - if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(preimage.clone()) } else { - // We can't build an HTLC-Success transaction without the preimage - continue; - } - } else { None }; - if let Some(htlc_tx) = self.onchain_tx_handler.get_maybe_signed_htlc_tx( - &::bitcoin::OutPoint { txid, vout }, &preimage + + self.get_broadcasted_holder_htlc_descriptors(&self.funding.current_holder_commitment.tx) + .into_iter() + .for_each(|htlc_descriptor| { + let txid = self.funding.current_holder_commitment.tx.trust().txid(); + let vout = htlc_descriptor.htlc.transaction_output_index + .expect("Expected transaction output index for non-dust HTLC"); + let htlc_output = HolderHTLCOutput::build(htlc_descriptor); + if let Some(htlc_tx) = htlc_output.get_maybe_signed_htlc_tx( + &mut self.onchain_tx_handler, &::bitcoin::OutPoint { txid, vout }, ) { if htlc_tx.is_fully_signed() { holder_transactions.push(htlc_tx.0); } } - } else { - debug_assert!(false, "Expected transaction output index for non-dust HTLC"); - } - } + }); + holder_transactions } diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 66935f2f52e..82aeca1452a 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -20,14 +20,12 @@ use bitcoin::script::{Script, ScriptBuf}; use bitcoin::hashes::{Hash, HashEngine}; use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::hash_types::{Txid, BlockHash}; -use bitcoin::secp256k1::PublicKey; use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature}; use bitcoin::secp256k1; use crate::chain::chaininterface::{ConfirmationTarget, compute_feerate_sat_per_1000_weight}; -use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, EntropySource, SignerProvider, ecdsa::EcdsaChannelSigner}; +use crate::sign::{EntropySource, HTLCDescriptor, SignerProvider, ecdsa::EcdsaChannelSigner}; use crate::ln::msgs::DecodeError; -use crate::types::payment::PaymentPreimage; use crate::ln::chan_utils::{self, ChannelTransactionParameters, HTLCOutputInCommitment, HolderCommitmentTransaction}; use crate::chain::ClaimId; use crate::chain::chaininterface::{FeeEstimator, BroadcasterInterface, LowerBoundedFeeEstimator}; @@ -173,17 +171,6 @@ impl Writeable for Option>> { } } -/// The claim commonly referred to as the pre-signed second-stage HTLC transaction. -#[derive(Clone, PartialEq, Eq)] -pub(crate) struct ExternalHTLCClaim { - pub(crate) commitment_txid: Txid, - pub(crate) per_commitment_number: u64, - pub(crate) htlc: HTLCOutputInCommitment, - pub(crate) preimage: Option, - pub(crate) counterparty_sig: Signature, - pub(crate) per_commitment_point: PublicKey, -} - // Represents the different types of claims for which events are yielded externally to satisfy said // claims. #[derive(Clone, PartialEq, Eq)] @@ -202,7 +189,7 @@ pub(crate) enum ClaimEvent { /// resolved by broadcasting a transaction with sufficient fee to claim them. BumpHTLC { target_feerate_sat_per_1000_weight: u32, - htlcs: Vec, + htlcs: Vec, tx_lock_time: LockTime, }, } @@ -234,7 +221,7 @@ pub(crate) enum FeerateStrategy { #[derive(Clone)] pub struct OnchainTxHandler { channel_value_satoshis: u64, - channel_keys_id: [u8; 32], + channel_keys_id: [u8; 32], // Deprecated as of 0.2. destination_script: ScriptBuf, // Deprecated as of 0.2. holder_commitment: HolderCommitmentTransaction, prev_holder_commitment: Option, @@ -610,19 +597,16 @@ impl OnchainTxHandler { let target_feerate_sat_per_1000_weight = cached_request.compute_package_feerate( fee_estimator, conf_target, feerate_strategy, ); - if let Some(htlcs) = cached_request.construct_malleable_package_with_external_funding(self) { - return Some(( - new_timer, - target_feerate_sat_per_1000_weight as u64, - OnchainClaim::Event(ClaimEvent::BumpHTLC { - target_feerate_sat_per_1000_weight, - htlcs, - tx_lock_time: LockTime::from_consensus(cached_request.package_locktime(cur_height)), - }), - )); - } else { - return None; - } + let htlcs = cached_request.construct_malleable_package_with_external_funding(self)?; + return Some(( + new_timer, + target_feerate_sat_per_1000_weight as u64, + OnchainClaim::Event(ClaimEvent::BumpHTLC { + target_feerate_sat_per_1000_weight, + htlcs, + tx_lock_time: LockTime::from_consensus(cached_request.package_locktime(cur_height)), + }), + )); } let predicted_weight = cached_request.package_weight(destination_script); @@ -1219,87 +1203,14 @@ impl OnchainTxHandler { self.prev_holder_commitment = Some(replace(&mut self.holder_commitment, tx)); } - #[cfg(any(test, feature="_test_utils", feature="unsafe_revoked_tx_signing"))] - pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction { - let sig = self.signer.unsafe_sign_holder_commitment(&self.channel_transaction_parameters, &self.holder_commitment, &self.secp_ctx).expect("sign holder commitment"); - self.holder_commitment.add_holder_sig(funding_redeemscript, sig) - } - - pub(crate) fn get_maybe_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option) -> Option { - let get_signed_htlc_tx = |holder_commitment: &HolderCommitmentTransaction| { - let trusted_tx = holder_commitment.trust(); - if trusted_tx.txid() != outp.txid { - return None; - } - let (htlc_idx, htlc) = trusted_tx.htlcs().iter().enumerate() - .find(|(_, htlc)| htlc.transaction_output_index.unwrap() == outp.vout) - .unwrap(); - let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx]; - let mut htlc_tx = trusted_tx.build_unsigned_htlc_tx( - &self.channel_transaction_parameters.as_holder_broadcastable(), htlc_idx, preimage, - ); - - let htlc_descriptor = HTLCDescriptor { - channel_derivation_parameters: ChannelDerivationParameters { - value_satoshis: self.channel_value_satoshis, - keys_id: self.channel_keys_id, - transaction_parameters: self.channel_transaction_parameters.clone(), - }, - commitment_txid: trusted_tx.txid(), - per_commitment_number: trusted_tx.commitment_number(), - per_commitment_point: trusted_tx.per_commitment_point(), - feerate_per_kw: trusted_tx.feerate_per_kw(), - htlc: htlc.clone(), - preimage: preimage.clone(), - counterparty_sig: counterparty_htlc_sig.clone(), - }; - if let Ok(htlc_sig) = self.signer.sign_holder_htlc_transaction(&htlc_tx, 0, &htlc_descriptor, &self.secp_ctx) { - htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness( - htlc_idx, &counterparty_htlc_sig, &htlc_sig, preimage, - ); - } - Some(MaybeSignedTransaction(htlc_tx)) - }; - - // Check if the HTLC spends from the current holder commitment first, or the previous. - get_signed_htlc_tx(&self.holder_commitment) - .or_else(|| self.prev_holder_commitment.as_ref().and_then(|prev_holder_commitment| get_signed_htlc_tx(prev_holder_commitment))) - } - - pub(crate) fn generate_external_htlc_claim( - &self, outp: &::bitcoin::OutPoint, preimage: &Option - ) -> Option { - let find_htlc = |holder_commitment: &HolderCommitmentTransaction| -> Option { - let trusted_tx = holder_commitment.trust(); - if outp.txid != trusted_tx.txid() { - return None; - } - trusted_tx.htlcs().iter().enumerate() - .find(|(_, htlc)| if let Some(output_index) = htlc.transaction_output_index { - output_index == outp.vout - } else { - false - }) - .map(|(htlc_idx, htlc)| { - let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx]; - ExternalHTLCClaim { - commitment_txid: trusted_tx.txid(), - per_commitment_number: trusted_tx.commitment_number(), - htlc: htlc.clone(), - preimage: *preimage, - counterparty_sig: counterparty_htlc_sig, - per_commitment_point: trusted_tx.per_commitment_point(), - } - }) - }; - // Check if the HTLC spends from the current holder commitment or the previous one otherwise. - find_htlc(&self.holder_commitment) - .or_else(|| self.prev_holder_commitment.as_ref().map(|c| find_htlc(c)).flatten()) - } - pub(crate) fn channel_type_features(&self) -> &ChannelTypeFeatures { &self.channel_transaction_parameters.channel_type_features } + + // Deprecated as of 0.2, only use in cases where it was not previously available. + pub(crate) fn channel_keys_id(&self) -> [u8; 32] { + self.channel_keys_id + } } #[cfg(test)] @@ -1320,7 +1231,7 @@ mod tests { }; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint, RevocationBasepoint}; use crate::ln::functional_test_utils::create_dummy_block; - use crate::sign::InMemorySigner; + use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, InMemorySigner}; use crate::types::payment::{PaymentHash, PaymentPreimage}; use crate::util::test_utils::{TestBroadcaster, TestFeeEstimator, TestLogger}; @@ -1425,17 +1336,27 @@ mod tests { let logger = TestLogger::new(); // Request claiming of each HTLC on the holder's commitment, with current block height 1. - let holder_commit_txid = tx_handler.current_holder_commitment_tx().trust().txid(); + let holder_commit = tx_handler.current_holder_commitment_tx(); + let holder_commit_txid = holder_commit.trust().txid(); let mut requests = Vec::new(); - for (htlc, _) in htlcs { + for (htlc, counterparty_sig) in holder_commit.htlcs().iter().zip(holder_commit.counterparty_htlc_sigs.iter()) { requests.push(PackageTemplate::build_package( holder_commit_txid, htlc.transaction_output_index.unwrap(), - PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build_offered( - htlc.amount_msat, - htlc.cltv_expiry, - ChannelTypeFeatures::only_static_remote_key(), - )), + PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(HTLCDescriptor { + channel_derivation_parameters: ChannelDerivationParameters { + value_satoshis: tx_handler.channel_value_satoshis, + keys_id: tx_handler.channel_keys_id, + transaction_parameters: tx_handler.channel_transaction_parameters.clone(), + }, + commitment_txid: holder_commit_txid, + per_commitment_number: holder_commit.commitment_number(), + per_commitment_point: holder_commit.per_commitment_point(), + feerate_per_kw: holder_commit.feerate_per_kw(), + htlc: htlc.clone(), + preimage: None, + counterparty_sig: *counterparty_sig, + })), 0, )); } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 74bf2558d94..16cea41c61f 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -24,6 +24,7 @@ use bitcoin::secp256k1::{SecretKey, PublicKey}; use bitcoin::sighash::EcdsaSighashType; use bitcoin::transaction::Version; +use crate::sign::{ChannelDerivationParameters, HTLCDescriptor}; use crate::types::payment::PaymentPreimage; use crate::ln::chan_utils::{ self, ChannelTransactionParameters, HolderCommitmentTransaction, TxCreationKeys, @@ -37,7 +38,7 @@ use crate::chain::channelmonitor::COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, INCREMENTAL_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, FEERATE_FLOOR_SATS_PER_KW}; use crate::chain::transaction::MaybeSignedTransaction; use crate::sign::ecdsa::EcdsaChannelSigner; -use crate::chain::onchaintx::{FeerateStrategy, ExternalHTLCClaim, OnchainTxHandler}; +use crate::chain::onchaintx::{FeerateStrategy, OnchainTxHandler}; use crate::util::logger::Logger; use crate::util::ser::{Readable, ReadableArgs, Writer, Writeable, RequiredWrapper}; @@ -372,25 +373,105 @@ pub(crate) struct HolderHTLCOutput { /// Defaults to 0 for HTLC-Success transactions, which have no expiry cltv_expiry: u32, channel_type_features: ChannelTypeFeatures, + htlc_descriptor: Option, } impl HolderHTLCOutput { - pub(crate) fn build_offered(amount_msat: u64, cltv_expiry: u32, channel_type_features: ChannelTypeFeatures) -> Self { - HolderHTLCOutput { - preimage: None, + pub(crate) fn build(htlc_descriptor: HTLCDescriptor) -> Self { + let amount_msat = htlc_descriptor.htlc.amount_msat; + let channel_type_features = htlc_descriptor.channel_derivation_parameters + .transaction_parameters.channel_type_features.clone(); + Self { + preimage: if htlc_descriptor.htlc.offered { + None + } else { + Some(htlc_descriptor.preimage.expect("Preimage required for accepted holder HTLC claim")) + }, amount_msat, - cltv_expiry, + cltv_expiry: if htlc_descriptor.htlc.offered { + htlc_descriptor.htlc.cltv_expiry + } else { + 0 + }, channel_type_features, + htlc_descriptor: Some(htlc_descriptor), } } - pub(crate) fn build_accepted(preimage: PaymentPreimage, amount_msat: u64, channel_type_features: ChannelTypeFeatures) -> Self { - HolderHTLCOutput { - preimage: Some(preimage), - amount_msat, - cltv_expiry: 0, - channel_type_features, + pub(crate) fn get_htlc_descriptor( + &self, onchain_tx_handler: &OnchainTxHandler, outp: &::bitcoin::OutPoint, + ) -> Option { + // Either we have the descriptor, or we have to go construct it because it was not written. + if let Some(htlc_descriptor) = self.htlc_descriptor.as_ref() { + return Some(htlc_descriptor.clone()); } + + let channel_parameters = &onchain_tx_handler.channel_transaction_parameters; + + let get_htlc_descriptor = |holder_commitment: &HolderCommitmentTransaction| { + let trusted_tx = holder_commitment.trust(); + if outp.txid != trusted_tx.txid() { + return None; + } + + let (htlc, counterparty_sig) = + trusted_tx.htlcs().iter().zip(holder_commitment.counterparty_htlc_sigs.iter()) + .find(|(htlc, _)| htlc.transaction_output_index.unwrap() == outp.vout) + .unwrap(); + + Some(HTLCDescriptor { + channel_derivation_parameters: ChannelDerivationParameters { + value_satoshis: channel_parameters.channel_value_satoshis, + keys_id: onchain_tx_handler.channel_keys_id(), + transaction_parameters: channel_parameters.clone(), + }, + commitment_txid: trusted_tx.txid(), + per_commitment_number: trusted_tx.commitment_number(), + per_commitment_point: trusted_tx.per_commitment_point(), + feerate_per_kw: trusted_tx.feerate_per_kw(), + htlc: htlc.clone(), + preimage: self.preimage.clone(), + counterparty_sig: *counterparty_sig, + }) + }; + + // Check if the HTLC spends from the current holder commitment or the previous one otherwise. + get_htlc_descriptor(onchain_tx_handler.current_holder_commitment_tx()) + .or_else(|| onchain_tx_handler.prev_holder_commitment_tx().and_then(|c| get_htlc_descriptor(c))) + } + + pub(crate) fn get_maybe_signed_htlc_tx( + &self, onchain_tx_handler: &mut OnchainTxHandler, outp: &::bitcoin::OutPoint, + ) -> Option { + let htlc_descriptor = self.get_htlc_descriptor(onchain_tx_handler, outp)?; + let channel_parameters = &htlc_descriptor.channel_derivation_parameters.transaction_parameters; + let directed_parameters = channel_parameters.as_holder_broadcastable(); + let keys = TxCreationKeys::from_channel_static_keys( + &htlc_descriptor.per_commitment_point, directed_parameters.broadcaster_pubkeys(), + directed_parameters.countersignatory_pubkeys(), &onchain_tx_handler.secp_ctx, + ); + + let mut htlc_tx = chan_utils::build_htlc_transaction( + &htlc_descriptor.commitment_txid, htlc_descriptor.feerate_per_kw, + directed_parameters.contest_delay(), &htlc_descriptor.htlc, + &channel_parameters.channel_type_features, &keys.broadcaster_delayed_payment_key, + &keys.revocation_key + ); + + if let Ok(htlc_sig) = onchain_tx_handler.signer.sign_holder_htlc_transaction( + &htlc_tx, 0, &htlc_descriptor, &onchain_tx_handler.secp_ctx, + ) { + let htlc_redeem_script = chan_utils::get_htlc_redeemscript_with_explicit_keys( + &htlc_descriptor.htlc, &channel_parameters.channel_type_features, + &keys.broadcaster_htlc_key, &keys.countersignatory_htlc_key, &keys.revocation_key, + ); + htlc_tx.input[0].witness = chan_utils::build_htlc_input_witness( + &htlc_sig, &htlc_descriptor.counterparty_sig, &htlc_descriptor.preimage, + &htlc_redeem_script, &channel_parameters.channel_type_features, + ); + } + + Some(MaybeSignedTransaction(htlc_tx)) } } @@ -403,6 +484,7 @@ impl Writeable for HolderHTLCOutput { (4, self.preimage, option), (6, legacy_deserialization_prevention_marker, option), (7, self.channel_type_features, required), + (9, self.htlc_descriptor, option), // Added in 0.2. }); Ok(()) } @@ -415,6 +497,7 @@ impl Readable for HolderHTLCOutput { let mut preimage = None; let mut _legacy_deserialization_prevention_marker: Option<()> = None; let mut channel_type_features = None; + let mut htlc_descriptor = None; read_tlv_fields!(reader, { (0, amount_msat, required), @@ -422,15 +505,19 @@ impl Readable for HolderHTLCOutput { (4, preimage, option), (6, _legacy_deserialization_prevention_marker, option), (7, channel_type_features, option), + (9, htlc_descriptor, option), // Added in 0.2. }); verify_channel_type_features(&channel_type_features, None)?; + let channel_type_features = + channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()); Ok(Self { amount_msat: amount_msat.0.unwrap(), cltv_expiry: cltv_expiry.0.unwrap(), preimage, - channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()) + channel_type_features, + htlc_descriptor, }) } } @@ -753,7 +840,7 @@ impl PackageSolvingData { match self { PackageSolvingData::HolderHTLCOutput(ref outp) => { debug_assert!(!outp.channel_type_features.supports_anchors_zero_fee_htlc_tx()); - onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage) + outp.get_maybe_signed_htlc_tx(onchain_handler, outpoint) } PackageSolvingData::HolderFundingOutput(ref outp) => { Some(outp.get_maybe_signed_commitment_tx(onchain_handler)) @@ -1064,14 +1151,14 @@ impl PackageTemplate { } pub(crate) fn construct_malleable_package_with_external_funding( &self, onchain_handler: &mut OnchainTxHandler, - ) -> Option> { + ) -> Option> { debug_assert!(self.requires_external_funding()); - let mut htlcs: Option> = None; + let mut htlcs: Option> = None; for (previous_output, input) in &self.inputs { match input { PackageSolvingData::HolderHTLCOutput(ref outp) => { debug_assert!(outp.channel_type_features.supports_anchors_zero_fee_htlc_tx()); - onchain_handler.generate_external_htlc_claim(&previous_output, &outp.preimage).map(|htlc| { + outp.get_htlc_descriptor(onchain_handler, &previous_output).map(|htlc| { htlcs.get_or_insert_with(|| Vec::with_capacity(self.inputs.len())).push(htlc); }); } @@ -1451,6 +1538,7 @@ mod tests { }; use crate::types::payment::{PaymentPreimage, PaymentHash}; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; + use crate::sign::{ChannelDerivationParameters, HTLCDescriptor}; use bitcoin::absolute::LockTime; use bitcoin::amount::Amount; @@ -1535,8 +1623,34 @@ mod tests { macro_rules! dumb_accepted_htlc_output { ($features: expr) => { { + let mut channel_parameters = ChannelTransactionParameters::test_dummy(0); + channel_parameters.channel_type_features = $features; let preimage = PaymentPreimage([2;32]); - PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build_accepted(preimage, 0, $features)) + let htlc = HTLCOutputInCommitment { + offered: false, + amount_msat: 1337000, + cltv_expiry: 420, + payment_hash: PaymentHash::from(preimage), + transaction_output_index: None, + }; + let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut vec![(htlc.clone(), ())]); + let trusted_tx = commitment_tx.trust(); + PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build( + HTLCDescriptor { + channel_derivation_parameters: ChannelDerivationParameters { + value_satoshis: channel_parameters.channel_value_satoshis, + keys_id: [0; 32], + transaction_parameters: channel_parameters, + }, + commitment_txid: trusted_tx.txid(), + per_commitment_number: trusted_tx.commitment_number(), + per_commitment_point: trusted_tx.per_commitment_point(), + feerate_per_kw: trusted_tx.feerate_per_kw(), + htlc, + preimage: Some(preimage), + counterparty_sig: commitment_tx.counterparty_htlc_sigs[0].clone(), + }, + )) } } } @@ -1544,7 +1658,33 @@ mod tests { macro_rules! dumb_offered_htlc_output { ($cltv_expiry: expr, $features: expr) => { { - PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build_offered(0, $cltv_expiry, $features)) + let mut channel_parameters = ChannelTransactionParameters::test_dummy(0); + channel_parameters.channel_type_features = $features; + let htlc = HTLCOutputInCommitment { + offered: true, + amount_msat: 1337000, + cltv_expiry: $cltv_expiry, + payment_hash: PaymentHash::from(PaymentPreimage([2;32])), + transaction_output_index: None, + }; + let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut vec![(htlc.clone(), ())]); + let trusted_tx = commitment_tx.trust(); + PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build( + HTLCDescriptor { + channel_derivation_parameters: ChannelDerivationParameters { + value_satoshis: channel_parameters.channel_value_satoshis, + keys_id: [0; 32], + transaction_parameters: channel_parameters, + }, + commitment_txid: trusted_tx.txid(), + per_commitment_number: trusted_tx.commitment_number(), + per_commitment_point: trusted_tx.per_commitment_point(), + feerate_per_kw: trusted_tx.feerate_per_kw(), + htlc, + preimage: None, + counterparty_sig: commitment_tx.counterparty_htlc_sigs[0].clone(), + }, + )) } } } diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 93fd78fbd32..8d59304d566 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -1846,42 +1846,6 @@ impl<'a> TrustedCommitmentTransaction<'a> { Ok(ret) } - /// Builds the second-level holder HTLC transaction for the HTLC with index `htlc_index`. - pub(crate) fn build_unsigned_htlc_tx( - &self, channel_parameters: &DirectedChannelTransactionParameters, htlc_index: usize, - preimage: &Option, - ) -> Transaction { - let keys = &self.inner.keys; - let this_htlc = &self.inner.htlcs[htlc_index]; - assert!(this_htlc.transaction_output_index.is_some()); - // if we don't have preimage for an HTLC-Success, we can't generate an HTLC transaction. - if !this_htlc.offered && preimage.is_none() { unreachable!(); } - // Further, we should never be provided the preimage for an HTLC-Timeout transaction. - if this_htlc.offered && preimage.is_some() { unreachable!(); } - - build_htlc_transaction( - &self.inner.built.txid, self.inner.feerate_per_kw, channel_parameters.contest_delay(), &this_htlc, - &self.channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key - ) - } - - - /// Builds the witness required to spend the input for the HTLC with index `htlc_index` in a - /// second-level holder HTLC transaction. - pub(crate) fn build_htlc_input_witness( - &self, htlc_index: usize, counterparty_signature: &Signature, signature: &Signature, - preimage: &Option - ) -> Witness { - let keys = &self.inner.keys; - let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys( - &self.inner.htlcs[htlc_index], &self.channel_type_features, &keys.broadcaster_htlc_key, - &keys.countersignatory_htlc_key, &keys.revocation_key - ); - build_htlc_input_witness( - signature, counterparty_signature, preimage, &htlc_redeemscript, &self.channel_type_features, - ) - } - /// Returns the index of the revokeable output, i.e. the `to_local` output sending funds to /// the broadcaster, in the built transaction, if any exists. /// diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 005ec566f0e..0965987c339 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -12260,7 +12260,7 @@ mod tests { } macro_rules! test_commitment_common { - ( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $opt_anchors: expr, { + ( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $channel_type_features: expr, { $( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), * } ) => { { let commitment_data = chan.context.build_commitment_transaction(&chan.funding, @@ -12310,9 +12310,9 @@ mod tests { let keys = commitment_tx.trust().keys(); let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw, chan.funding.get_counterparty_selected_contest_delay().unwrap(), - &htlc, $opt_anchors, &keys.broadcaster_delayed_payment_key, &keys.revocation_key); - let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, $opt_anchors, &keys); - let htlc_sighashtype = if $opt_anchors.supports_anchors_zero_fee_htlc_tx() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All }; + &htlc, $channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key); + let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, $channel_type_features, &keys); + let htlc_sighashtype = if $channel_type_features.supports_anchors_zero_fee_htlc_tx() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All }; let htlc_sighash = Message::from_digest(sighash::SighashCache::new(&htlc_tx).p2wsh_signature_hash(0, &htlc_redeemscript, htlc.to_bitcoin_amount(), htlc_sighashtype).unwrap().as_raw_hash().to_byte_array()); assert!(secp_ctx.verify_ecdsa(&htlc_sighash, &remote_signature, &keys.countersignatory_htlc_key.to_public_key()).is_ok(), "verify counterparty htlc sig"); @@ -12343,13 +12343,15 @@ mod tests { preimage: preimage.clone(), counterparty_sig: *htlc_counterparty_sig, }, &secp_ctx).unwrap(); - let num_anchors = if $opt_anchors.supports_anchors_zero_fee_htlc_tx() { 2 } else { 0 }; + let num_anchors = if $channel_type_features.supports_anchors_zero_fee_htlc_tx() { 2 } else { 0 }; assert_eq!(htlc.transaction_output_index, Some($htlc_idx + num_anchors), "output index"); let signature = Signature::from_der(&>::from_hex($htlc_sig_hex).unwrap()[..]).unwrap(); assert_eq!(signature, htlc_holder_sig, "htlc sig"); - let trusted_tx = holder_commitment_tx.trust(); - htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness($htlc_idx, htlc_counterparty_sig, &htlc_holder_sig, &preimage); + htlc_tx.input[0].witness = chan_utils::build_htlc_input_witness( + &htlc_holder_sig, htlc_counterparty_sig, &preimage, &htlc_redeemscript, + $channel_type_features, + ); log_trace!(logger, "htlc_tx = {}", serialize(&htlc_tx).as_hex()); assert_eq!(serialize(&htlc_tx)[..], >::from_hex($htlc_tx_hex).unwrap()[..], "htlc tx"); })* From e0b4353338312c25fdb2178833c9be9b77e81290 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:54 -0700 Subject: [PATCH 5/9] Track ChannelTransactionParameters in CounterpartyReceivedHTLCOutput The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `CounterpartyReceivedHTLCOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`. --- lightning/src/chain/channelmonitor.rs | 9 ++++--- lightning/src/chain/package.rs | 38 +++++++++++++++++++++------ 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 114c0daf885..872ac695973 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3895,10 +3895,11 @@ impl ChannelMonitorImpl { preimage.unwrap(), htlc.clone(), self.channel_type_features().clone())) } else { PackageSolvingData::CounterpartyReceivedHTLCOutput( - CounterpartyReceivedHTLCOutput::build(*per_commitment_point, - self.counterparty_commitment_params.counterparty_delayed_payment_base_key, - self.counterparty_commitment_params.counterparty_htlc_base_key, - htlc.clone(), self.channel_type_features().clone())) + CounterpartyReceivedHTLCOutput::build( + *per_commitment_point, htlc.clone(), + self.funding.channel_parameters.clone(), + ) + ) }; let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry); claimable_outpoints.push(counterparty_package); diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 16cea41c61f..606b14aad91 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -301,16 +301,26 @@ pub(crate) struct CounterpartyReceivedHTLCOutput { counterparty_htlc_base_key: HtlcBasepoint, htlc: HTLCOutputInCommitment, channel_type_features: ChannelTypeFeatures, + channel_parameters: Option, } impl CounterpartyReceivedHTLCOutput { - pub(crate) fn build(per_commitment_point: PublicKey, counterparty_delayed_payment_base_key: DelayedPaymentBasepoint, counterparty_htlc_base_key: HtlcBasepoint, htlc: HTLCOutputInCommitment, channel_type_features: ChannelTypeFeatures) -> Self { - CounterpartyReceivedHTLCOutput { + pub(crate) fn build( + per_commitment_point: PublicKey, htlc: HTLCOutputInCommitment, + channel_parameters: ChannelTransactionParameters, + ) -> Self { + let directed_params = channel_parameters.as_counterparty_broadcastable(); + let counterparty_keys = directed_params.broadcaster_pubkeys(); + let counterparty_delayed_payment_base_key = counterparty_keys.delayed_payment_basepoint; + let counterparty_htlc_base_key = counterparty_keys.htlc_basepoint; + let channel_type_features = channel_parameters.channel_type_features.clone(); + Self { per_commitment_point, counterparty_delayed_payment_base_key, counterparty_htlc_base_key, htlc, - channel_type_features + channel_type_features, + channel_parameters: Some(channel_parameters), } } } @@ -325,6 +335,7 @@ impl Writeable for CounterpartyReceivedHTLCOutput { (6, self.htlc, required), (8, legacy_deserialization_prevention_marker, option), (9, self.channel_type_features, required), + (11, self.channel_parameters, option), // Added in 0.2. }); Ok(()) } @@ -338,6 +349,7 @@ impl Readable for CounterpartyReceivedHTLCOutput { let mut htlc = RequiredWrapper(None); let mut _legacy_deserialization_prevention_marker: Option<()> = None; let mut channel_type_features = None; + let mut channel_parameters = None; read_tlv_fields!(reader, { (0, per_commitment_point, required), @@ -346,16 +358,20 @@ impl Readable for CounterpartyReceivedHTLCOutput { (6, htlc, required), (8, _legacy_deserialization_prevention_marker, option), (9, channel_type_features, option), + (11, channel_parameters, (option: ReadableArgs, None)), // Added in 0.2. }); verify_channel_type_features(&channel_type_features, None)?; + let channel_type_features = + channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()); Ok(Self { per_commitment_point: per_commitment_point.0.unwrap(), counterparty_delayed_payment_base_key: counterparty_delayed_payment_base_key.0.unwrap(), counterparty_htlc_base_key: counterparty_htlc_base_key.0.unwrap(), htlc: htlc.0.unwrap(), - channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()) + channel_type_features, + channel_parameters, }) } } @@ -807,8 +823,8 @@ impl PackageSolvingData { } }, PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => { - let directed_parameters = - &onchain_handler.channel_transaction_parameters.as_counterparty_broadcastable(); + let channel_parameters = outp.channel_parameters.as_ref().unwrap_or(channel_parameters); + let directed_parameters = channel_parameters.as_counterparty_broadcastable(); debug_assert_eq!( directed_parameters.broadcaster_pubkeys().delayed_payment_basepoint, outp.counterparty_delayed_payment_base_key, @@ -821,7 +837,9 @@ impl PackageSolvingData { &outp.per_commitment_point, directed_parameters.broadcaster_pubkeys(), directed_parameters.countersignatory_pubkeys(), &onchain_handler.secp_ctx, ); - let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, &onchain_handler.channel_type_features(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); + let witness_script = chan_utils::get_htlc_redeemscript( + &outp.htlc, &channel_parameters.channel_type_features, &chan_keys, + ); if let Ok(sig) = onchain_handler.signer.sign_counterparty_htlc_transaction(channel_parameters, &bumped_tx, i, &outp.htlc.amount_msat / 1000, &outp.per_commitment_point, &outp.htlc, &onchain_handler.secp_ctx) { let mut ser_sig = sig.serialize_der().to_vec(); @@ -1601,7 +1619,11 @@ mod tests { let dumb_point = PublicKey::from_secret_key(&secp_ctx, &dumb_scalar); let hash = PaymentHash([1; 32]); let htlc = HTLCOutputInCommitment { offered: true, amount_msat: $amt, cltv_expiry: $expiry, payment_hash: hash, transaction_output_index: None }; - PackageSolvingData::CounterpartyReceivedHTLCOutput(CounterpartyReceivedHTLCOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), htlc, $features)) + let mut channel_parameters = ChannelTransactionParameters::test_dummy(0); + channel_parameters.channel_type_features = $features; + PackageSolvingData::CounterpartyReceivedHTLCOutput( + CounterpartyReceivedHTLCOutput::build(dumb_point, htlc, channel_parameters) + ) } } } From 5959950f7236fa19d0cfd5da8265cda051e33bae Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:55 -0700 Subject: [PATCH 6/9] Track ChannelTransactionParameters in CounterpartyOfferedHTLCOutput The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `CounterpartyOfferedHTLCOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`. --- lightning/src/chain/channelmonitor.rs | 9 +++---- lightning/src/chain/package.rs | 34 ++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 872ac695973..e5b97c01953 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3889,10 +3889,11 @@ impl ChannelMonitorImpl { if preimage.is_some() || !htlc.offered { let counterparty_htlc_outp = if htlc.offered { PackageSolvingData::CounterpartyOfferedHTLCOutput( - CounterpartyOfferedHTLCOutput::build(*per_commitment_point, - self.counterparty_commitment_params.counterparty_delayed_payment_base_key, - self.counterparty_commitment_params.counterparty_htlc_base_key, - preimage.unwrap(), htlc.clone(), self.channel_type_features().clone())) + CounterpartyOfferedHTLCOutput::build( + *per_commitment_point, preimage.unwrap(), htlc.clone(), + self.funding.channel_parameters.clone(), + ) + ) } else { PackageSolvingData::CounterpartyReceivedHTLCOutput( CounterpartyReceivedHTLCOutput::build( diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 606b14aad91..bb42ef82e4c 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -224,10 +224,19 @@ pub(crate) struct CounterpartyOfferedHTLCOutput { preimage: PaymentPreimage, htlc: HTLCOutputInCommitment, channel_type_features: ChannelTypeFeatures, + channel_parameters: Option, } impl CounterpartyOfferedHTLCOutput { - pub(crate) fn build(per_commitment_point: PublicKey, counterparty_delayed_payment_base_key: DelayedPaymentBasepoint, counterparty_htlc_base_key: HtlcBasepoint, preimage: PaymentPreimage, htlc: HTLCOutputInCommitment, channel_type_features: ChannelTypeFeatures) -> Self { + pub(crate) fn build( + per_commitment_point: PublicKey, preimage: PaymentPreimage, htlc: HTLCOutputInCommitment, + channel_parameters: ChannelTransactionParameters, + ) -> Self { + let directed_params = channel_parameters.as_counterparty_broadcastable(); + let counterparty_keys = directed_params.broadcaster_pubkeys(); + let counterparty_delayed_payment_base_key = counterparty_keys.delayed_payment_basepoint; + let counterparty_htlc_base_key = counterparty_keys.htlc_basepoint; + let channel_type_features = channel_parameters.channel_type_features.clone(); CounterpartyOfferedHTLCOutput { per_commitment_point, counterparty_delayed_payment_base_key, @@ -235,6 +244,7 @@ impl CounterpartyOfferedHTLCOutput { preimage, htlc, channel_type_features, + channel_parameters: Some(channel_parameters), } } } @@ -250,6 +260,7 @@ impl Writeable for CounterpartyOfferedHTLCOutput { (8, self.htlc, required), (10, legacy_deserialization_prevention_marker, option), (11, self.channel_type_features, required), + (13, self.channel_parameters, option), // Added in 0.2. }); Ok(()) } @@ -264,6 +275,7 @@ impl Readable for CounterpartyOfferedHTLCOutput { let mut htlc = RequiredWrapper(None); let mut _legacy_deserialization_prevention_marker: Option<()> = None; let mut channel_type_features = None; + let mut channel_parameters = None; read_tlv_fields!(reader, { (0, per_commitment_point, required), @@ -273,9 +285,12 @@ impl Readable for CounterpartyOfferedHTLCOutput { (8, htlc, required), (10, _legacy_deserialization_prevention_marker, option), (11, channel_type_features, option), + (13, channel_parameters, (option: ReadableArgs, None)), // Added in 0.2. }); verify_channel_type_features(&channel_type_features, None)?; + let channel_type_features = + channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()); Ok(Self { per_commitment_point: per_commitment_point.0.unwrap(), @@ -283,7 +298,8 @@ impl Readable for CounterpartyOfferedHTLCOutput { counterparty_htlc_base_key: counterparty_htlc_base_key.0.unwrap(), preimage: preimage.0.unwrap(), htlc: htlc.0.unwrap(), - channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()) + channel_type_features, + channel_parameters, }) } } @@ -798,8 +814,8 @@ impl PackageSolvingData { } else { return false; } }, PackageSolvingData::CounterpartyOfferedHTLCOutput(ref outp) => { - let directed_parameters = - &onchain_handler.channel_transaction_parameters.as_counterparty_broadcastable(); + let channel_parameters = outp.channel_parameters.as_ref().unwrap_or(channel_parameters); + let directed_parameters = channel_parameters.as_counterparty_broadcastable(); debug_assert_eq!( directed_parameters.broadcaster_pubkeys().delayed_payment_basepoint, outp.counterparty_delayed_payment_base_key, @@ -812,7 +828,9 @@ impl PackageSolvingData { &outp.per_commitment_point, directed_parameters.broadcaster_pubkeys(), directed_parameters.countersignatory_pubkeys(), &onchain_handler.secp_ctx, ); - let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, &onchain_handler.channel_type_features(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); + let witness_script = chan_utils::get_htlc_redeemscript( + &outp.htlc, &channel_parameters.channel_type_features, &chan_keys, + ); if let Ok(sig) = onchain_handler.signer.sign_counterparty_htlc_transaction(channel_parameters, &bumped_tx, i, &outp.htlc.amount_msat / 1000, &outp.per_commitment_point, &outp.htlc, &onchain_handler.secp_ctx) { let mut ser_sig = sig.serialize_der().to_vec(); @@ -1637,7 +1655,11 @@ mod tests { let hash = PaymentHash([1; 32]); let preimage = PaymentPreimage([2;32]); let htlc = HTLCOutputInCommitment { offered: false, amount_msat: $amt, cltv_expiry: 0, payment_hash: hash, transaction_output_index: None }; - PackageSolvingData::CounterpartyOfferedHTLCOutput(CounterpartyOfferedHTLCOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), preimage, htlc, $features)) + let mut channel_parameters = ChannelTransactionParameters::test_dummy(0); + channel_parameters.channel_type_features = $features; + PackageSolvingData::CounterpartyOfferedHTLCOutput( + CounterpartyOfferedHTLCOutput::build(dumb_point, preimage, htlc, channel_parameters) + ) } } } From 97348c7f64cb98c789b49086f6543dae7eeb58e6 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:56 -0700 Subject: [PATCH 7/9] Track ChannelTransactionParameters in RevokedHTLCOutput The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `RevokedHTLCOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`. --- lightning/src/chain/channelmonitor.rs | 7 ++--- lightning/src/chain/package.rs | 37 +++++++++++++++++++++------ 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index e5b97c01953..b9848ec2491 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3762,11 +3762,8 @@ impl ChannelMonitorImpl { return (claimable_outpoints, to_counterparty_output_info); } let revk_htlc_outp = RevokedHTLCOutput::build( - per_commitment_point, - self.counterparty_commitment_params.counterparty_delayed_payment_base_key, - self.counterparty_commitment_params.counterparty_htlc_base_key, - per_commitment_key, htlc.amount_msat / 1000, htlc.clone(), - self.channel_type_features(), + per_commitment_point, per_commitment_key, htlc.clone(), + self.funding.channel_parameters.clone(), ); let counterparty_spendable_height = if htlc.offered { htlc.cltv_expiry diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index bb42ef82e4c..0e746cc56a2 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -181,19 +181,32 @@ pub(crate) struct RevokedHTLCOutput { weight: u64, amount: u64, htlc: HTLCOutputInCommitment, + channel_parameters: Option, } impl RevokedHTLCOutput { - pub(crate) fn build(per_commitment_point: PublicKey, counterparty_delayed_payment_base_key: DelayedPaymentBasepoint, counterparty_htlc_base_key: HtlcBasepoint, per_commitment_key: SecretKey, amount: u64, htlc: HTLCOutputInCommitment, channel_type_features: &ChannelTypeFeatures) -> Self { - let weight = if htlc.offered { weight_revoked_offered_htlc(channel_type_features) } else { weight_revoked_received_htlc(channel_type_features) }; + pub(crate) fn build( + per_commitment_point: PublicKey, per_commitment_key: SecretKey, + htlc: HTLCOutputInCommitment, channel_parameters: ChannelTransactionParameters, + ) -> Self { + let weight = if htlc.offered { + weight_revoked_offered_htlc(&channel_parameters.channel_type_features) + } else { + weight_revoked_received_htlc(&channel_parameters.channel_type_features) + }; + let directed_params = channel_parameters.as_counterparty_broadcastable(); + let counterparty_keys = directed_params.broadcaster_pubkeys(); + let counterparty_delayed_payment_base_key = counterparty_keys.delayed_payment_basepoint; + let counterparty_htlc_base_key = counterparty_keys.htlc_basepoint; RevokedHTLCOutput { per_commitment_point, counterparty_delayed_payment_base_key, counterparty_htlc_base_key, per_commitment_key, weight, - amount, - htlc + amount: htlc.amount_msat / 1000, + htlc, + channel_parameters: Some(channel_parameters), } } } @@ -206,6 +219,7 @@ impl_writeable_tlv_based!(RevokedHTLCOutput, { (8, weight, required), (10, amount, required), (12, htlc, required), + (13, channel_parameters, (option: ReadableArgs, None)), // Added in 0.2. }); /// A struct to describe a HTLC output on a counterparty commitment transaction. @@ -789,8 +803,8 @@ impl PackageSolvingData { } else { return false; } }, PackageSolvingData::RevokedHTLCOutput(ref outp) => { - let directed_parameters = - &onchain_handler.channel_transaction_parameters.as_counterparty_broadcastable(); + let channel_parameters = outp.channel_parameters.as_ref().unwrap_or(channel_parameters); + let directed_parameters = channel_parameters.as_counterparty_broadcastable(); debug_assert_eq!( directed_parameters.broadcaster_pubkeys().delayed_payment_basepoint, outp.counterparty_delayed_payment_base_key, @@ -803,7 +817,9 @@ impl PackageSolvingData { &outp.per_commitment_point, directed_parameters.broadcaster_pubkeys(), directed_parameters.countersignatory_pubkeys(), &onchain_handler.secp_ctx, ); - let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, &onchain_handler.channel_type_features(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); + let witness_script = chan_utils::get_htlc_redeemscript( + &outp.htlc, &channel_parameters.channel_type_features, &chan_keys + ); //TODO: should we panic on signer failure ? if let Ok(sig) = onchain_handler.signer.sign_justice_revoked_htlc(channel_parameters, &bumped_tx, i, outp.amount, &outp.per_commitment_key, &outp.htlc, &onchain_handler.secp_ctx) { let mut ser_sig = sig.serialize_der().to_vec(); @@ -1624,7 +1640,12 @@ mod tests { let dumb_point = PublicKey::from_secret_key(&secp_ctx, &dumb_scalar); let hash = PaymentHash([1; 32]); let htlc = HTLCOutputInCommitment { offered: false, amount_msat: 1_000_000, cltv_expiry: 0, payment_hash: hash, transaction_output_index: None }; - PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), dumb_scalar, 1_000_000 / 1_000, htlc, &ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies())) + let mut channel_parameters = ChannelTransactionParameters::test_dummy(0); + channel_parameters.channel_type_features = + ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(); + PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput::build( + dumb_point, dumb_scalar, htlc, channel_parameters + )) } } } From 23eb12e3ae98f5a67aadee356a36af8076299ccc Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:56 -0700 Subject: [PATCH 8/9] Track ChannelTransactionParameters in RevokedOutput The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `RevokedOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`. --- lightning/src/chain/channelmonitor.rs | 15 +++++--------- lightning/src/chain/package.rs | 28 ++++++++++++++++++++------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index b9848ec2491..a6c90e56484 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3734,12 +3734,9 @@ impl ChannelMonitorImpl { for (idx, outp) in tx.output.iter().enumerate() { if outp.script_pubkey == revokeable_p2wsh { let revk_outp = RevokedOutput::build( - per_commitment_point, - self.counterparty_commitment_params.counterparty_delayed_payment_base_key, - self.counterparty_commitment_params.counterparty_htlc_base_key, - per_commitment_key, outp.value, - self.counterparty_commitment_params.on_counterparty_tx_csv, - self.channel_type_features().supports_anchors_zero_fee_htlc_tx(), + per_commitment_point, per_commitment_key, outp.value, + self.funding.channel_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx(), + self.funding.channel_parameters.clone(), ); let justice_package = PackageTemplate::build_package( commitment_txid, idx as u32, @@ -3936,10 +3933,8 @@ impl ChannelMonitorImpl { if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() { log_error!(logger, "Got broadcast of revoked counterparty HTLC transaction, spending {}:{}", htlc_txid, idx); let revk_outp = RevokedOutput::build( - per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, - self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, - tx.output[idx].value, self.counterparty_commitment_params.on_counterparty_tx_csv, - false + per_commitment_point, per_commitment_key, tx.output[idx].value, false, + self.funding.channel_parameters.clone(), ); let justice_package = PackageTemplate::build_package( htlc_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 0e746cc56a2..c063c26f38c 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -136,10 +136,19 @@ pub(crate) struct RevokedOutput { amount: Amount, on_counterparty_tx_csv: u16, is_counterparty_balance_on_anchors: Option<()>, + channel_parameters: Option, } impl RevokedOutput { - pub(crate) fn build(per_commitment_point: PublicKey, counterparty_delayed_payment_base_key: DelayedPaymentBasepoint, counterparty_htlc_base_key: HtlcBasepoint, per_commitment_key: SecretKey, amount: Amount, on_counterparty_tx_csv: u16, is_counterparty_balance_on_anchors: bool) -> Self { + pub(crate) fn build( + per_commitment_point: PublicKey, per_commitment_key: SecretKey, amount: Amount, + is_counterparty_balance_on_anchors: bool, channel_parameters: ChannelTransactionParameters, + ) -> Self { + let directed_params = channel_parameters.as_counterparty_broadcastable(); + let counterparty_keys = directed_params.broadcaster_pubkeys(); + let counterparty_delayed_payment_base_key = counterparty_keys.delayed_payment_basepoint; + let counterparty_htlc_base_key = counterparty_keys.htlc_basepoint; + let on_counterparty_tx_csv = directed_params.contest_delay(); RevokedOutput { per_commitment_point, counterparty_delayed_payment_base_key, @@ -148,7 +157,8 @@ impl RevokedOutput { weight: WEIGHT_REVOKED_OUTPUT, amount, on_counterparty_tx_csv, - is_counterparty_balance_on_anchors: if is_counterparty_balance_on_anchors { Some(()) } else { None } + is_counterparty_balance_on_anchors: if is_counterparty_balance_on_anchors { Some(()) } else { None }, + channel_parameters: Some(channel_parameters), } } } @@ -161,7 +171,8 @@ impl_writeable_tlv_based!(RevokedOutput, { (8, weight, required), (10, amount, required), (12, on_counterparty_tx_csv, required), - (14, is_counterparty_balance_on_anchors, option) + (14, is_counterparty_balance_on_anchors, option), + (15, channel_parameters, (option: ReadableArgs, None)), // Added in 0.2. }); /// A struct to describe a revoked offered output and corresponding information to generate a @@ -778,8 +789,8 @@ impl PackageSolvingData { let channel_parameters = &onchain_handler.channel_transaction_parameters; match self { PackageSolvingData::RevokedOutput(ref outp) => { - let directed_parameters = - &onchain_handler.channel_transaction_parameters.as_counterparty_broadcastable(); + let channel_parameters = outp.channel_parameters.as_ref().unwrap_or(channel_parameters); + let directed_parameters = channel_parameters.as_counterparty_broadcastable(); debug_assert_eq!( directed_parameters.broadcaster_pubkeys().delayed_payment_basepoint, outp.counterparty_delayed_payment_base_key, @@ -1589,7 +1600,6 @@ mod tests { ChannelTransactionParameters, HolderCommitmentTransaction, HTLCOutputInCommitment, }; use crate::types::payment::{PaymentPreimage, PaymentHash}; - use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; use crate::sign::{ChannelDerivationParameters, HTLCDescriptor}; use bitcoin::absolute::LockTime; @@ -1627,7 +1637,11 @@ mod tests { let secp_ctx = Secp256k1::new(); let dumb_scalar = SecretKey::from_slice(&>::from_hex("0101010101010101010101010101010101010101010101010101010101010101").unwrap()[..]).unwrap(); let dumb_point = PublicKey::from_secret_key(&secp_ctx, &dumb_scalar); - PackageSolvingData::RevokedOutput(RevokedOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), dumb_scalar, Amount::ZERO, 0, $is_counterparty_balance_on_anchors)) + let channel_parameters = ChannelTransactionParameters::test_dummy(0); + PackageSolvingData::RevokedOutput(RevokedOutput::build( + dumb_point, dumb_scalar, Amount::ZERO, $is_counterparty_balance_on_anchors, + channel_parameters, + )) } } } From 9bd8557239dd25c72d7409a100ea0b978aa4b79b Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 1 Apr 2025 16:43:57 -0700 Subject: [PATCH 9/9] Track ChannelTransactionParameters in ChannelMonitor The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. This commit tracks the `ChannelTransactionParameters` for the current `FundingScope` of a `ChannelMonitor` and deprecates the one found in `OnchainTxHandler`. --- lightning/src/chain/channelmonitor.rs | 7 ++++++- lightning/src/chain/onchaintx.rs | 12 ++++++------ lightning/src/chain/package.rs | 6 +++--- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a6c90e56484..f709183f2d1 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1398,6 +1398,7 @@ impl Writeable for ChannelMonitorImpl { (25, self.payment_preimages, required), (27, self.first_confirmed_funding_txo, required), (29, self.initial_counterparty_commitment_tx, option), + (31, self.funding.channel_parameters, required), }); Ok(()) @@ -5271,6 +5272,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut holder_pays_commitment_tx_fee = None; let mut payment_preimages_with_info: Option> = None; let mut first_confirmed_funding_txo = RequiredWrapper(None); + let mut channel_parameters = None; read_tlv_fields!(reader, { (1, funding_spend_confirmed, option), (3, htlcs_resolved_on_chain, optional_vec), @@ -5287,6 +5289,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (25, payment_preimages_with_info, option), (27, first_confirmed_funding_txo, (default_value, outpoint)), (29, initial_counterparty_commitment_tx, option), + (31, channel_parameters, (option: ReadableArgs, None)), }); if let Some(payment_preimages_with_info) = payment_preimages_with_info { if payment_preimages_with_info.len() != payment_preimages.len() { @@ -5315,7 +5318,9 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP } } - let channel_parameters = onchain_tx_handler.channel_transaction_parameters.clone(); + let channel_parameters = channel_parameters.unwrap_or_else(|| { + onchain_tx_handler.channel_parameters().clone() + }); // Monitors for anchor outputs channels opened in v0.0.116 suffered from a bug in which the // wrong `counterparty_payment_script` was being tracked. Fix it now on deserialization to diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 82aeca1452a..645296d6411 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -42,7 +42,6 @@ use core::cmp; use core::ops::Deref; use core::mem::replace; use core::mem::swap; -use crate::types::features::ChannelTypeFeatures; const MAX_ALLOC_SIZE: usize = 64*1024; @@ -220,14 +219,14 @@ pub(crate) enum FeerateStrategy { /// do RBF bumping if possible. #[derive(Clone)] pub struct OnchainTxHandler { - channel_value_satoshis: u64, + channel_value_satoshis: u64, // Deprecated as of 0.2. channel_keys_id: [u8; 32], // Deprecated as of 0.2. destination_script: ScriptBuf, // Deprecated as of 0.2. holder_commitment: HolderCommitmentTransaction, prev_holder_commitment: Option, pub(super) signer: ChannelSigner, - pub(crate) channel_transaction_parameters: ChannelTransactionParameters, + channel_transaction_parameters: ChannelTransactionParameters, // Deprecated as of 0.2. // Used to track claiming requests. If claim tx doesn't confirm before height timer expiration we need to bump // it (RBF or CPFP). If an input has been part of an aggregate tx at first claim try, we need to keep it within @@ -676,7 +675,7 @@ impl OnchainTxHandler { // We'll locate an anchor output we can spend within the commitment transaction. let channel_parameters = output.channel_parameters.as_ref() - .unwrap_or(&self.channel_transaction_parameters); + .unwrap_or(self.channel_parameters()); let funding_pubkey = &channel_parameters.holder_pubkeys.funding_pubkey; match chan_utils::get_keyed_anchor_output(&tx, funding_pubkey) { // An anchor output was found, so we should yield a funding event externally. @@ -1203,8 +1202,9 @@ impl OnchainTxHandler { self.prev_holder_commitment = Some(replace(&mut self.holder_commitment, tx)); } - pub(crate) fn channel_type_features(&self) -> &ChannelTypeFeatures { - &self.channel_transaction_parameters.channel_type_features + // Deprecated as of 0.2, only use in cases where it was not previously available. + pub(crate) fn channel_parameters(&self) -> &ChannelTransactionParameters { + &self.channel_transaction_parameters } // Deprecated as of 0.2, only use in cases where it was not previously available. diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index c063c26f38c..14a884ca6f6 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -463,7 +463,7 @@ impl HolderHTLCOutput { return Some(htlc_descriptor.clone()); } - let channel_parameters = &onchain_tx_handler.channel_transaction_parameters; + let channel_parameters = onchain_tx_handler.channel_parameters(); let get_htlc_descriptor = |holder_commitment: &HolderCommitmentTransaction| { let trusted_tx = holder_commitment.trust(); @@ -614,7 +614,7 @@ impl HolderFundingOutput { &self, onchain_tx_handler: &mut OnchainTxHandler, ) -> MaybeSignedTransaction { let channel_parameters = self.channel_parameters.as_ref() - .unwrap_or(&onchain_tx_handler.channel_transaction_parameters); + .unwrap_or(onchain_tx_handler.channel_parameters()); let commitment_tx = self.commitment_tx.as_ref() .unwrap_or(onchain_tx_handler.current_holder_commitment_tx()); let maybe_signed_tx = onchain_tx_handler.signer @@ -786,7 +786,7 @@ impl PackageSolvingData { } } fn finalize_input(&self, bumped_tx: &mut Transaction, i: usize, onchain_handler: &mut OnchainTxHandler) -> bool { - let channel_parameters = &onchain_handler.channel_transaction_parameters; + let channel_parameters = onchain_handler.channel_parameters(); match self { PackageSolvingData::RevokedOutput(ref outp) => { let channel_parameters = outp.channel_parameters.as_ref().unwrap_or(channel_parameters);