From c5763db1fa0cf7d526056ba2c9cc85173444a078 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 6 Aug 2025 11:05:44 -0700 Subject: [PATCH 1/6] Add note for alternative_funding_confirmed assertion in promote_funding --- lightning/src/chain/channelmonitor.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index c7011af43a9..92d39f065cf 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3851,6 +3851,9 @@ impl ChannelMonitorImpl { self.outputs_to_watch.remove(&funding.funding_txid()); } if let Some((alternative_funding_txid, _)) = self.alternative_funding_confirmed.take() { + // In exceedingly rare cases, it's possible there was a reorg that caused a potential funding to + // be locked in that this `ChannelMonitor` has not yet seen. Thus, we avoid a runtime assertion + // and only assert in debug mode. debug_assert_eq!(alternative_funding_txid, new_funding_txid); } From 5d7a3f2f1680e8a62cade364a23df01af75f7a28 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 6 Aug 2025 11:09:26 -0700 Subject: [PATCH 2/6] Make splice-funding confirmation assertions non-debug We generally want to hard assert on cases that could cause us to lose money. --- lightning/src/chain/channelmonitor.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 92d39f065cf..ff7cbf386f9 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -4949,13 +4949,13 @@ impl ChannelMonitorImpl { .iter() .find(|funding| funding.funding_txid() == txid) { - debug_assert!(self.alternative_funding_confirmed.is_none()); - debug_assert!( + assert!(self.alternative_funding_confirmed.is_none()); + assert!( !self.onchain_events_awaiting_threshold_conf.iter() .any(|e| matches!(e.event, OnchainEvent::AlternativeFundingConfirmation {})) ); - debug_assert!(self.funding_spend_confirmed.is_none()); - debug_assert!( + assert!(self.funding_spend_confirmed.is_none()); + assert!( !self.onchain_events_awaiting_threshold_conf.iter() .any(|e| matches!(e.event, OnchainEvent::FundingSpendConfirmation { .. })) ); From 746bbea7b61b740cc8d489dfc31e4b01104258a6 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 6 Aug 2025 11:11:05 -0700 Subject: [PATCH 3/6] Broadcast holder commitment immediately on alternative funding reorg We can't rely waiting on another (or the same) renegotiated funding transaction to confirm, since it may never happen. We also don't want to rely on the counterparty to broadcast for us, or require manual intervention from the user, so we choose to broadcast the new holder commitment immediately. This ensures we're able to claim funds from an already force closed channel after an alternative funding reorg. --- lightning/src/chain/channelmonitor.rs | 34 ++++++++++++++++++++------- lightning/src/chain/onchaintx.rs | 4 ++-- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index ff7cbf386f9..e5f351ea874 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -4866,7 +4866,7 @@ impl ChannelMonitorImpl { 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, &self.destination_script, fee_estimator, logger, + height + 1, &broadcaster, conf_target, &self.destination_script, fee_estimator, logger, ); Vec::new() } else { Vec::new() } @@ -5341,16 +5341,18 @@ impl ChannelMonitorImpl { self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height < height); // TODO: Replace with `take_if` once our MSRV is >= 1.80. + let mut should_broadcast_commitment = false; if let Some((_, conf_height)) = self.alternative_funding_confirmed.as_ref() { if *conf_height == height { self.alternative_funding_confirmed.take(); - if self.holder_tx_signed { + if self.holder_tx_signed || self.funding_spend_seen { // Cancel any previous claims that are no longer valid as they stemmed from a - // different funding transaction. We'll wait until we see a funding transaction - // confirm again before attempting to broadcast the new valid holder commitment. + // different funding transaction. let new_holder_commitment_txid = self.funding.current_holder_commitment_tx.trust().txid(); self.cancel_prev_commitment_claims(&logger, &new_holder_commitment_txid); + + should_broadcast_commitment = true; } } } @@ -5358,9 +5360,15 @@ 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, &self.destination_script, &bounded_fee_estimator, logger + height, &broadcaster, conf_target, &self.destination_script, &bounded_fee_estimator, logger ); + // Only attempt to broadcast the new commitment after the `block_disconnected` call above so that + // it doesn't get removed from the set of pending claims. + if should_broadcast_commitment { + self.queue_latest_holder_commitment_txn_for_broadcast(&broadcaster, &bounded_fee_estimator, logger); + } + self.best_block = BestBlock::new(header.prev_blockhash, height - 1); } @@ -5395,24 +5403,32 @@ impl ChannelMonitorImpl { debug_assert!(!self.onchain_events_awaiting_threshold_conf.iter().any(|ref entry| entry.txid == *txid)); // TODO: Replace with `take_if` once our MSRV is >= 1.80. + let mut should_broadcast_commitment = false; if let Some((alternative_funding_txid, _)) = self.alternative_funding_confirmed.as_ref() { if alternative_funding_txid == txid { self.alternative_funding_confirmed.take(); - if self.holder_tx_signed { + if self.holder_tx_signed || self.funding_spend_seen { // Cancel any previous claims that are no longer valid as they stemmed from a - // different funding transaction. We'll wait until we see a funding transaction - // confirm again before attempting to broadcast the new valid holder commitment. + // different funding transaction. let new_holder_commitment_txid = self.funding.current_holder_commitment_tx.trust().txid(); self.cancel_prev_commitment_claims(&logger, &new_holder_commitment_txid); + + should_broadcast_commitment = true; } } } let conf_target = self.closure_conf_target(); self.onchain_tx_handler.transaction_unconfirmed( - txid, broadcaster, conf_target, &self.destination_script, fee_estimator, logger + txid, &broadcaster, conf_target, &self.destination_script, fee_estimator, logger ); + + // Only attempt to broadcast the new commitment after the `transaction_unconfirmed` call above so + // that it doesn't get removed from the set of pending claims. + if should_broadcast_commitment { + self.queue_latest_holder_commitment_txn_for_broadcast(&broadcaster, 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 95df05f4fd3..67ee01971ee 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1120,7 +1120,7 @@ impl OnchainTxHandler { pub(super) fn transaction_unconfirmed( &mut self, txid: &Txid, - broadcaster: B, + broadcaster: &B, conf_target: ConfirmationTarget, destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator, @@ -1146,7 +1146,7 @@ impl OnchainTxHandler { #[rustfmt::skip] pub(super) fn block_disconnected( - &mut self, height: u32, broadcaster: B, conf_target: ConfirmationTarget, + &mut self, height: u32, broadcaster: &B, conf_target: ConfirmationTarget, destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) where B::Target: BroadcasterInterface, From 1d8b3db79bf222a6a8305a29b9f72a935e64fd2e Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 29 Jul 2025 22:51:35 -0700 Subject: [PATCH 4/6] Consider currently confirmed FundingScope when claiming commitments Once a commitment transaction confirms, we may have outputs to claim. To determine whom the commitment transaction belongs to, we generally compare its `txid` against what we know be ours and the counterparty's. This, however, relies on being able to match on the expected `FundingScope`, such that we can produce the necessary output claims. Since a commitment transaction confirming implies that the funding transaction it spends has also confirmed, we rely on `alternative_funding_confirmed` to obtain the corresponding `FundingScope`. --- lightning/src/chain/channelmonitor.rs | 290 +++++++++++++++----------- 1 file changed, 167 insertions(+), 123 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index e5f351ea874..04e81c93650 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1302,30 +1302,51 @@ pub(crate) struct ChannelMonitorImpl { alternative_funding_confirmed: Option<(Txid, u32)>, } +// Returns a `&FundingScope` for the one we are currently observing/handling commitment transactions +// for on the chain. +macro_rules! get_confirmed_funding_scope { + ($self: expr) => { + $self + .alternative_funding_confirmed + .map(|(alternative_funding_txid, _)| { + $self + .pending_funding + .iter() + .find(|funding| funding.funding_txid() == alternative_funding_txid) + .expect("FundingScope for confirmed alternative funding must exist") + }) + .unwrap_or(&$self.funding) + }; +} + // Macro helper to access holder commitment HTLC data (including both non-dust and dust) while // holding mutable references to `self`. Unfortunately, if these were turned into helper functions, // we'd be unable to mutate `self` while holding an immutable iterator (specifically, returned from // a function) over `self`. #[rustfmt::skip] macro_rules! holder_commitment_htlcs { - ($self: expr, CURRENT) => { - $self.funding.current_holder_commitment_tx.nondust_htlcs().iter() + ($self: expr, CURRENT) => {{ + let funding = get_confirmed_funding_scope!($self); + funding.current_holder_commitment_tx.nondust_htlcs().iter() .chain($self.current_holder_htlc_data.dust_htlcs.iter().map(|(htlc, _)| htlc)) - }; + }}; ($self: expr, CURRENT_WITH_SOURCES) => {{ + let funding = get_confirmed_funding_scope!($self); holder_commitment_htlcs!( - &$self.funding.current_holder_commitment_tx, &$self.current_holder_htlc_data + &funding.current_holder_commitment_tx, &$self.current_holder_htlc_data ) }}; ($self: expr, PREV) => {{ - $self.funding.prev_holder_commitment_tx.as_ref().map(|tx| { + let funding = get_confirmed_funding_scope!($self); + funding.prev_holder_commitment_tx.as_ref().map(|tx| { let dust_htlcs = $self.prev_holder_htlc_data.as_ref().unwrap().dust_htlcs.iter() .map(|(htlc, _)| htlc); tx.nondust_htlcs().iter().chain(dust_htlcs) }) }}; ($self: expr, PREV_WITH_SOURCES) => {{ - $self.funding.prev_holder_commitment_tx.as_ref().map(|tx| { + let funding = get_confirmed_funding_scope!($self); + funding.prev_holder_commitment_tx.as_ref().map(|tx| { holder_commitment_htlcs!(tx, $self.prev_holder_htlc_data.as_ref().unwrap()) }) }}; @@ -2408,7 +2429,8 @@ impl ChannelMonitor { pub fn get_spendable_outputs(&self, tx: &Transaction, confirmation_height: u32) -> Vec { let inner = self.inner.lock().unwrap(); let current_height = inner.best_block.height; - let mut spendable_outputs = inner.get_spendable_outputs(tx); + let funding = get_confirmed_funding_scope!(inner); + let mut spendable_outputs = inner.get_spendable_outputs(&funding, tx); spendable_outputs.retain(|descriptor| { let mut conf_threshold = current_height.saturating_sub(ANTI_REORG_DELAY) + 1; if let SpendableOutputDescriptor::DelayedPaymentOutput(descriptor) = descriptor { @@ -3015,17 +3037,19 @@ impl ChannelMonitor { } } - let txid = confirmed_txid.unwrap(); - if Some(txid) == us.funding.current_counterparty_commitment_txid || Some(txid) == us.funding.prev_counterparty_commitment_txid { - walk_htlcs!(false, us.funding.counterparty_claimable_outpoints.get(&txid).unwrap().iter().filter_map(|(a, b)| { + let commitment_txid = confirmed_txid.unwrap(); + let funding_spent = get_confirmed_funding_scope!(us); + + if Some(commitment_txid) == funding_spent.current_counterparty_commitment_txid || Some(commitment_txid) == funding_spent.prev_counterparty_commitment_txid { + walk_htlcs!(false, funding_spent.counterparty_claimable_outpoints.get(&commitment_txid).unwrap().iter().filter_map(|(a, b)| { if let &Some(ref source) = b { Some((a, Some(&**source))) } else { None } })); - } else if txid == us.funding.current_holder_commitment_tx.trust().txid() { + } else if commitment_txid == funding_spent.current_holder_commitment_tx.trust().txid() { walk_htlcs!(true, holder_commitment_htlcs!(us, CURRENT_WITH_SOURCES)); - } else if let Some(prev_commitment_tx) = &us.funding.prev_holder_commitment_tx { - if txid == prev_commitment_tx.trust().txid() { + } else if let Some(prev_commitment_tx) = &funding_spent.prev_holder_commitment_tx { + if commitment_txid == prev_commitment_tx.trust().txid() { walk_htlcs!(true, holder_commitment_htlcs!(us, PREV_WITH_SOURCES).unwrap()); } } @@ -3562,12 +3586,13 @@ impl ChannelMonitorImpl { } else { return; }; + let funding_spent = get_confirmed_funding_scope!(self); // If the channel is force closed, try to claim the output from this preimage. // First check if a counterparty commitment transaction has been broadcasted: macro_rules! claim_htlcs { ($commitment_number: expr, $txid: expr, $htlcs: expr) => { - let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info($commitment_number, $txid, None, $htlcs, confirmed_spend_height); + let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info(funding_spent, $commitment_number, $txid, None, $htlcs, confirmed_spend_height); 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, @@ -3575,10 +3600,10 @@ impl ChannelMonitorImpl { ); } } - if let Some(txid) = self.funding.current_counterparty_commitment_txid { + if let Some(txid) = funding_spent.current_counterparty_commitment_txid { if txid == confirmed_spend_txid { if let Some(commitment_number) = self.counterparty_commitment_txn_on_chain.get(&txid) { - claim_htlcs!(*commitment_number, txid, self.funding.counterparty_claimable_outpoints.get(&txid)); + claim_htlcs!(*commitment_number, txid, funding_spent.counterparty_claimable_outpoints.get(&txid)); } else { debug_assert!(false); log_error!(logger, "Detected counterparty commitment tx on-chain without tracking commitment number"); @@ -3586,10 +3611,10 @@ impl ChannelMonitorImpl { return; } } - if let Some(txid) = self.funding.prev_counterparty_commitment_txid { + if let Some(txid) = funding_spent.prev_counterparty_commitment_txid { if txid == confirmed_spend_txid { if let Some(commitment_number) = self.counterparty_commitment_txn_on_chain.get(&txid) { - claim_htlcs!(*commitment_number, txid, self.funding.counterparty_claimable_outpoints.get(&txid)); + claim_htlcs!(*commitment_number, txid, funding_spent.counterparty_claimable_outpoints.get(&txid)); } else { debug_assert!(false); log_error!(logger, "Detected counterparty commitment tx on-chain without tracking commitment number"); @@ -3604,9 +3629,9 @@ impl ChannelMonitorImpl { // *we* sign a holder commitment transaction, not when e.g. a watchtower broadcasts one of our // holder commitment transactions. if self.broadcasted_holder_revokable_script.is_some() { - let holder_commitment_tx = if self.funding.current_holder_commitment_tx.trust().txid() == confirmed_spend_txid { - Some(&self.funding.current_holder_commitment_tx) - } else if let Some(prev_holder_commitment_tx) = &self.funding.prev_holder_commitment_tx { + let holder_commitment_tx = if funding_spent.current_holder_commitment_tx.trust().txid() == confirmed_spend_txid { + Some(&funding_spent.current_holder_commitment_tx) + } else if let Some(prev_holder_commitment_tx) = &funding_spent.prev_holder_commitment_tx { if prev_holder_commitment_tx.trust().txid() == confirmed_spend_txid { Some(prev_holder_commitment_tx) } else { @@ -3619,7 +3644,9 @@ impl ChannelMonitorImpl { // Assume that the broadcasted commitment transaction confirmed in the current best // block. Even if not, its a reasonable metric for the bump criteria on the HTLC // transactions. - let (claim_reqs, _) = self.get_broadcasted_holder_claims(&self.funding, holder_commitment_tx, self.best_block.height); + let (claim_reqs, _) = self.get_broadcasted_holder_claims( + funding_spent, 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, @@ -3633,14 +3660,7 @@ impl ChannelMonitorImpl { fn generate_claimable_outpoints_and_watch_outputs( &mut self, generate_monitor_event_with_reason: Option, ) -> (Vec, Vec) { - let funding = self.alternative_funding_confirmed - .map(|(alternative_funding_txid, _)| { - self.pending_funding - .iter() - .find(|funding| funding.funding_txid() == alternative_funding_txid) - .expect("FundingScope for confirmed alternative funding must exist") - }) - .unwrap_or(&self.funding); + let funding = get_confirmed_funding_scope!(self); let holder_commitment_tx = &funding.current_holder_commitment_tx; let funding_outp = HolderFundingOutput::build( holder_commitment_tx.clone(), @@ -4283,7 +4303,7 @@ impl ChannelMonitorImpl { /// Returns packages to claim the revoked output(s) and general information about the output that /// is to the counterparty in the commitment transaction. #[rustfmt::skip] - fn check_spend_counterparty_transaction(&mut self, tx: &Transaction, height: u32, block_hash: &BlockHash, logger: &L) + fn check_spend_counterparty_transaction(&mut self, commitment_txid: Txid, commitment_tx: &Transaction, height: u32, block_hash: &BlockHash, logger: &L) -> (Vec, CommitmentTxCounterpartyOutputInfo) where L::Target: Logger { // Most secp and related errors trying to create keys means we have no hope of constructing @@ -4291,8 +4311,8 @@ impl ChannelMonitorImpl { let mut claimable_outpoints = Vec::new(); let mut to_counterparty_output_info = None; - let commitment_txid = tx.compute_txid(); //TODO: This is gonna be a performance bottleneck for watchtowers! - let per_commitment_option = self.funding.counterparty_claimable_outpoints.get(&commitment_txid); + let funding_spent = get_confirmed_funding_scope!(self); + let per_commitment_option = funding_spent.counterparty_claimable_outpoints.get(&commitment_txid); macro_rules! ignore_error { ( $thing : expr ) => { @@ -4303,8 +4323,11 @@ impl ChannelMonitorImpl { }; } - let commitment_number = 0xffffffffffff - ((((tx.input[0].sequence.0 as u64 & 0xffffff) << 3*8) | (tx.lock_time.to_consensus_u32() as u64 & 0xffffff)) ^ self.commitment_transaction_number_obscure_factor); + let funding_txid_spent = commitment_tx.input[0].previous_output.txid; + let commitment_number = 0xffffffffffff - ((((commitment_tx.input[0].sequence.0 as u64 & 0xffffff) << 3*8) | (commitment_tx.lock_time.to_consensus_u32() as u64 & 0xffffff)) ^ self.commitment_transaction_number_obscure_factor); if commitment_number >= self.get_min_seen_secret() { + assert_eq!(funding_spent.funding_txid(), funding_txid_spent); + let secret = self.get_secret(commitment_number).unwrap(); let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret)); let per_commitment_point = PublicKey::from_secret_key(&self.onchain_tx_handler.secp_ctx, &per_commitment_key); @@ -4315,13 +4338,12 @@ impl ChannelMonitorImpl { let revokeable_p2wsh = revokeable_redeemscript.to_p2wsh(); // First, process non-htlc outputs (to_holder & to_counterparty) - for (idx, outp) in tx.output.iter().enumerate() { + for (idx, outp) in commitment_tx.output.iter().enumerate() { if outp.script_pubkey == revokeable_p2wsh { let revk_outp = RevokedOutput::build( 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(), - height, + funding_spent.channel_type_features().supports_anchors_zero_fee_htlc_tx(), + funding_spent.channel_parameters.clone(), height, ); let justice_package = PackageTemplate::build_package( commitment_txid, idx as u32, @@ -4338,15 +4360,14 @@ impl ChannelMonitorImpl { if let Some(per_commitment_claimable_data) = per_commitment_option { for (htlc, _) in per_commitment_claimable_data { if let Some(transaction_output_index) = htlc.transaction_output_index { - if transaction_output_index as usize >= tx.output.len() || - tx.output[transaction_output_index as usize].value != htlc.to_bitcoin_amount() { + if transaction_output_index as usize >= commitment_tx.output.len() || + commitment_tx.output[transaction_output_index as usize].value != htlc.to_bitcoin_amount() { // per_commitment_data is corrupt or our commitment signing key leaked! return (claimable_outpoints, to_counterparty_output_info); } let revk_htlc_outp = RevokedHTLCOutput::build( per_commitment_point, per_commitment_key, htlc.clone(), - self.funding.channel_parameters.clone(), - height, + funding_spent.channel_parameters.clone(), height, ); let counterparty_spendable_height = if htlc.offered { htlc.cltv_expiry @@ -4371,7 +4392,7 @@ impl ChannelMonitorImpl { self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number); if let Some(per_commitment_claimable_data) = per_commitment_option { - fail_unbroadcast_htlcs!(self, "revoked_counterparty", commitment_txid, tx, height, + fail_unbroadcast_htlcs!(self, "revoked_counterparty", commitment_txid, commitment_tx, height, block_hash, per_commitment_claimable_data.iter().map(|(htlc, htlc_source)| (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref())) ), logger); @@ -4381,11 +4402,13 @@ impl ChannelMonitorImpl { // commitment transactions. Thus, we can only debug-assert here when not // fuzzing. debug_assert!(cfg!(fuzzing), "We should have per-commitment option for any recognized old commitment txn"); - fail_unbroadcast_htlcs!(self, "revoked counterparty", commitment_txid, tx, height, + fail_unbroadcast_htlcs!(self, "revoked counterparty", commitment_txid, commitment_tx, height, block_hash, [].iter().map(|reference| *reference), logger); } } } else if let Some(per_commitment_claimable_data) = per_commitment_option { + assert_eq!(funding_spent.funding_txid(), funding_txid_spent); + // While this isn't useful yet, there is a potential race where if a counterparty // revokes a state at the same time as the commitment transaction for that state is // confirmed, and the watchtower receives the block before the user, the user could @@ -4396,25 +4419,26 @@ impl ChannelMonitorImpl { self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number); log_info!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid); - fail_unbroadcast_htlcs!(self, "counterparty", commitment_txid, tx, height, block_hash, + fail_unbroadcast_htlcs!(self, "counterparty", commitment_txid, commitment_tx, height, block_hash, per_commitment_claimable_data.iter().map(|(htlc, htlc_source)| (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref())) ), logger); let (htlc_claim_reqs, counterparty_output_info) = - self.get_counterparty_output_claim_info(commitment_number, commitment_txid, Some(tx), per_commitment_option, Some(height)); + self.get_counterparty_output_claim_info(funding_spent, commitment_number, commitment_txid, Some(commitment_tx), per_commitment_option, Some(height)); to_counterparty_output_info = counterparty_output_info; for req in htlc_claim_reqs { claimable_outpoints.push(req); } - } + (claimable_outpoints, to_counterparty_output_info) } /// Returns the HTLC claim package templates and the counterparty output info #[rustfmt::skip] fn get_counterparty_output_claim_info( - &self, commitment_number: u64, commitment_txid: Txid, tx: Option<&Transaction>, + &self, funding_spent: &FundingScope, commitment_number: u64, commitment_txid: Txid, + tx: Option<&Transaction>, per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>, confirmation_height: Option, ) -> (Vec, CommitmentTxCounterpartyOutputInfo) { @@ -4476,7 +4500,7 @@ impl ChannelMonitorImpl { CounterpartyOfferedHTLCOutput::build( *per_commitment_point, preimage.unwrap(), htlc.clone(), - self.funding.channel_parameters.clone(), + funding_spent.channel_parameters.clone(), confirmation_height, ) ) @@ -4485,7 +4509,7 @@ impl ChannelMonitorImpl { CounterpartyReceivedHTLCOutput::build( *per_commitment_point, htlc.clone(), - self.funding.channel_parameters.clone(), + funding_spent.channel_parameters.clone(), confirmation_height, ) ) @@ -4511,6 +4535,9 @@ impl ChannelMonitorImpl { }; let per_commitment_point = PublicKey::from_secret_key(&self.onchain_tx_handler.secp_ctx, &per_commitment_key); + let funding_spent = get_confirmed_funding_scope!(self); + debug_assert!(funding_spent.counterparty_claimable_outpoints.contains_key(commitment_txid)); + let htlc_txid = tx.compute_txid(); let mut claimable_outpoints = vec![]; let mut outputs_to_watch = None; @@ -4529,7 +4556,7 @@ impl ChannelMonitorImpl { log_error!(logger, "Got broadcast of revoked counterparty HTLC transaction, spending {}:{}", htlc_txid, idx); let revk_outp = RevokedOutput::build( per_commitment_point, per_commitment_key, tx.output[idx].value, false, - self.funding.channel_parameters.clone(), + funding_spent.channel_parameters.clone(), height, ); let justice_package = PackageTemplate::build_package( @@ -4643,66 +4670,65 @@ impl ChannelMonitorImpl { /// Should not be used if check_spend_revoked_transaction succeeds. /// Returns None unless the transaction is definitely one of our commitment transactions. fn check_spend_holder_transaction( - &mut self, tx: &Transaction, height: u32, block_hash: &BlockHash, logger: &L, + &mut self, commitment_txid: Txid, commitment_tx: &Transaction, height: u32, + block_hash: &BlockHash, logger: &L, ) -> Option<(Vec, TransactionOutputs)> where L::Target: Logger, { - let commitment_txid = tx.compute_txid(); - let mut claim_requests = Vec::new(); - let mut watch_outputs = Vec::new(); - - macro_rules! append_onchain_update { - ($updates: expr, $to_watch: expr) => { - claim_requests = $updates.0; - self.broadcasted_holder_revokable_script = $updates.1; - watch_outputs.append(&mut $to_watch); - }; - } + let funding_spent = get_confirmed_funding_scope!(self); // HTLCs set may differ between last and previous holder commitment txn, in case of one them hitting chain, ensure we cancel all HTLCs backward - let mut is_holder_tx = false; - - if self.funding.current_holder_commitment_tx.trust().txid() == commitment_txid { - is_holder_tx = true; - log_info!(logger, "Got broadcast of latest holder commitment tx {}, searching for available HTLCs to claim", commitment_txid); - let holder_commitment_tx = &self.funding.current_holder_commitment_tx; - let res = - self.get_broadcasted_holder_claims(&self.funding, holder_commitment_tx, height); - let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_commitment_tx); - append_onchain_update!(res, to_watch); - fail_unbroadcast_htlcs!( - self, - "latest holder", - commitment_txid, - tx, - height, - block_hash, - holder_commitment_htlcs!(self, CURRENT_WITH_SOURCES), - logger - ); - } else if let Some(holder_commitment_tx) = &self.funding.prev_holder_commitment_tx { - if holder_commitment_tx.trust().txid() == commitment_txid { - is_holder_tx = true; - log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid); - let res = - self.get_broadcasted_holder_claims(&self.funding, holder_commitment_tx, height); - let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_commitment_tx); - append_onchain_update!(res, to_watch); + let holder_commitment_tx = Some((&funding_spent.current_holder_commitment_tx, true)) + .filter(|(current_holder_commitment_tx, _)| { + current_holder_commitment_tx.trust().txid() == commitment_txid + }) + .or_else(|| { + funding_spent + .prev_holder_commitment_tx + .as_ref() + .map(|prev_holder_commitment_tx| (prev_holder_commitment_tx, false)) + .filter(|(prev_holder_commitment_tx, _)| { + prev_holder_commitment_tx.trust().txid() == commitment_txid + }) + }); + + if let Some((holder_commitment_tx, current)) = holder_commitment_tx { + let funding_txid_spent = commitment_tx.input[0].previous_output.txid; + assert_eq!(funding_spent.funding_txid(), funding_txid_spent); + + let current_msg = if current { "latest holder" } else { "previous holder" }; + log_info!(logger, "Got broadcast of {current_msg} commitment tx {commitment_txid}, searching for available HTLCs to claim"); + + let (claim_requests, broadcasted_holder_revokable_script) = + self.get_broadcasted_holder_claims(funding_spent, holder_commitment_tx, height); + self.broadcasted_holder_revokable_script = broadcasted_holder_revokable_script; + let watch_outputs = self.get_broadcasted_holder_watch_outputs(holder_commitment_tx); + + if current { fail_unbroadcast_htlcs!( self, - "previous holder", + current_msg, commitment_txid, - tx, + commitment_tx, + height, + block_hash, + holder_commitment_htlcs!(self, CURRENT_WITH_SOURCES), + logger + ); + } else { + fail_unbroadcast_htlcs!( + self, + current_msg, + commitment_txid, + commitment_tx, height, block_hash, holder_commitment_htlcs!(self, PREV_WITH_SOURCES).unwrap(), logger ); } - } - if is_holder_tx { Some((claim_requests, (commitment_txid, watch_outputs))) } else { None @@ -5014,18 +5040,30 @@ impl ChannelMonitorImpl { // commitment transactions and HTLC transactions will all only ever have one input // (except for HTLC transactions for channels with anchor outputs), which is an easy // way to filter out any potential non-matching txn for lazy filters. - // - // TODO(splicing): Produce commitment claims for currently confirmed funding. - let prevout = &tx.input[0].previous_output; - let funding_outpoint = self.get_funding_txo(); - if prevout.txid == funding_outpoint.txid && prevout.vout == funding_outpoint.index as u32 { - let mut balance_spendable_csv = None; - log_info!(logger, "Channel {} closed by funding output spend in txid {}.", - &self.channel_id(), txid); + if let Some(funding_txid_spent) = core::iter::once(&self.funding) + .chain(self.pending_funding.iter()) + .find(|funding| { + let funding_outpoint = funding.funding_outpoint().into_bitcoin_outpoint(); + funding_outpoint == tx.input[0].previous_output + }) + .map(|funding| funding.funding_txid()) + { + assert_eq!( + funding_txid_spent, + self.alternative_funding_confirmed + .map(|(txid, _)| txid) + .unwrap_or_else(|| self.funding.funding_txid()) + ); + log_info!(logger, "Channel {} closed by funding output spend in txid {txid}", + self.channel_id()); self.funding_spend_seen = true; + + let mut balance_spendable_csv = None; let mut commitment_tx_to_counterparty_output = None; + + // Is it a commitment transaction? if (tx.input[0].sequence.0 >> 8*3) as u8 == 0x80 && (tx.lock_time.to_consensus_u32() >> 8*3) as u8 == 0x20 { - if let Some((mut new_outpoints, new_outputs)) = self.check_spend_holder_transaction(&tx, height, &block_hash, &logger) { + if let Some((mut new_outpoints, new_outputs)) = self.check_spend_holder_transaction(txid, &tx, height, &block_hash, &logger) { if !new_outputs.1.is_empty() { watch_outputs.push(new_outputs); } @@ -5040,7 +5078,7 @@ impl ChannelMonitorImpl { watch_outputs.push((txid, new_watch_outputs)); let (mut new_outpoints, counterparty_output_idx_sats) = - self.check_spend_counterparty_transaction(&tx, height, &block_hash, &logger); + self.check_spend_counterparty_transaction(txid, &tx, height, &block_hash, &logger); commitment_tx_to_counterparty_output = counterparty_output_idx_sats; claimable_outpoints.append(&mut new_outpoints); @@ -5053,6 +5091,7 @@ impl ChannelMonitorImpl { should_broadcast_commitment = false; } } + self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry { txid, transaction: Some((*tx).clone()), @@ -5063,6 +5102,7 @@ impl ChannelMonitorImpl { commitment_tx_to_counterparty_output, }, }); + // Now that we've detected a confirmed commitment transaction, attempt to cancel // pending claims for any commitments that were previously confirmed such that // we don't continue claiming inputs that no longer exist. @@ -5556,6 +5596,8 @@ impl ChannelMonitorImpl { fn is_resolving_htlc_output( &mut self, tx: &Transaction, height: u32, block_hash: &BlockHash, logger: &WithChannelMonitor, ) where L::Target: Logger { + let funding_spent = get_confirmed_funding_scope!(self); + 'outer_loop: for input in &tx.input { let mut payment_data = None; let htlc_claim = HTLCClaim::from_witness(&input.witness); @@ -5617,7 +5659,7 @@ impl ChannelMonitorImpl { } macro_rules! scan_commitment { - ($htlcs: expr, $tx_info: expr, $holder_tx: expr) => { + ($funding_spent: expr, $htlcs: expr, $tx_info: expr, $holder_tx: expr) => { for (ref htlc_output, source_option) in $htlcs { if Some(input.previous_output.vout) == htlc_output.transaction_output_index { if let Some(ref source) = source_option { @@ -5629,12 +5671,12 @@ impl ChannelMonitorImpl { // resolve the source HTLC with the original sender. payment_data = Some(((*source).clone(), htlc_output.payment_hash, htlc_output.amount_msat)); } else if !$holder_tx { - if let Some(current_counterparty_commitment_txid) = &self.funding.current_counterparty_commitment_txid { - check_htlc_valid_counterparty!(htlc_output, self.funding.counterparty_claimable_outpoints.get(current_counterparty_commitment_txid).unwrap()); + if let Some(current_counterparty_commitment_txid) = &$funding_spent.current_counterparty_commitment_txid { + check_htlc_valid_counterparty!(htlc_output, $funding_spent.counterparty_claimable_outpoints.get(current_counterparty_commitment_txid).unwrap()); } if payment_data.is_none() { - if let Some(prev_counterparty_commitment_txid) = &self.funding.prev_counterparty_commitment_txid { - check_htlc_valid_counterparty!(htlc_output, self.funding.counterparty_claimable_outpoints.get(prev_counterparty_commitment_txid).unwrap()); + if let Some(prev_counterparty_commitment_txid) = &$funding_spent.prev_counterparty_commitment_txid { + check_htlc_valid_counterparty!(htlc_output, $funding_spent.counterparty_claimable_outpoints.get(prev_counterparty_commitment_txid).unwrap()); } } } @@ -5662,23 +5704,24 @@ impl ChannelMonitorImpl { } } - if input.previous_output.txid == self.funding.current_holder_commitment_tx.trust().txid() { + if input.previous_output.txid == funding_spent.current_holder_commitment_tx.trust().txid() { scan_commitment!( - holder_commitment_htlcs!(self, CURRENT_WITH_SOURCES), + funding_spent, holder_commitment_htlcs!(self, CURRENT_WITH_SOURCES), "our latest holder commitment tx", true ); } - if let Some(prev_holder_commitment_tx) = self.funding.prev_holder_commitment_tx.as_ref() { + if let Some(prev_holder_commitment_tx) = funding_spent.prev_holder_commitment_tx.as_ref() { if input.previous_output.txid == prev_holder_commitment_tx.trust().txid() { scan_commitment!( - holder_commitment_htlcs!(self, PREV_WITH_SOURCES).unwrap(), + funding_spent, holder_commitment_htlcs!(self, PREV_WITH_SOURCES).unwrap(), "our previous holder commitment tx", true ); } } - if let Some(ref htlc_outputs) = self.funding.counterparty_claimable_outpoints.get(&input.previous_output.txid) { - scan_commitment!(htlc_outputs.iter().map(|&(ref a, ref b)| (a, b.as_ref().map(|boxed| &**boxed))), - "counterparty commitment tx", false); + if let Some(ref htlc_outputs) = funding_spent.counterparty_claimable_outpoints.get(&input.previous_output.txid) { + let htlcs = htlc_outputs.iter() + .map(|&(ref a, ref b)| (a, b.as_ref().map(|boxed| &**boxed))); + scan_commitment!(funding_spent, htlcs, "counterparty commitment tx", false); } // Check that scan_commitment, above, decided there is some source worth relaying an @@ -5758,7 +5801,7 @@ impl ChannelMonitorImpl { } #[rustfmt::skip] - fn get_spendable_outputs(&self, tx: &Transaction) -> Vec { + fn get_spendable_outputs(&self, funding_spent: &FundingScope, tx: &Transaction) -> Vec { let mut spendable_outputs = Vec::new(); for (i, outp) in tx.output.iter().enumerate() { if outp.script_pubkey == self.destination_script { @@ -5777,8 +5820,8 @@ impl ChannelMonitorImpl { output: outp.clone(), revocation_pubkey: broadcasted_holder_revokable_script.2, channel_keys_id: self.channel_keys_id, - channel_value_satoshis: self.funding.channel_parameters.channel_value_satoshis, - channel_transaction_parameters: Some(self.funding.channel_parameters.clone()), + channel_value_satoshis: funding_spent.channel_parameters.channel_value_satoshis, + channel_transaction_parameters: Some(funding_spent.channel_parameters.clone()), })); } } @@ -5787,8 +5830,8 @@ impl ChannelMonitorImpl { outpoint: OutPoint { txid: tx.compute_txid(), index: i as u16 }, output: outp.clone(), channel_keys_id: self.channel_keys_id, - channel_value_satoshis: self.funding.channel_parameters.channel_value_satoshis, - channel_transaction_parameters: Some(self.funding.channel_parameters.clone()), + channel_value_satoshis: funding_spent.channel_parameters.channel_value_satoshis, + channel_transaction_parameters: Some(funding_spent.channel_parameters.clone()), })); } if self.shutdown_script.as_ref() == Some(&outp.script_pubkey) { @@ -5808,7 +5851,8 @@ impl ChannelMonitorImpl { fn check_tx_and_push_spendable_outputs( &mut self, tx: &Transaction, height: u32, block_hash: &BlockHash, logger: &WithChannelMonitor, ) where L::Target: Logger { - for spendable_output in self.get_spendable_outputs(tx) { + let funding_spent = get_confirmed_funding_scope!(self); + for spendable_output in self.get_spendable_outputs(funding_spent, tx) { let entry = OnchainEventEntry { txid: tx.compute_txid(), transaction: Some(tx.clone()), From 08892b5f40efd2ea97fcb0680693fc9014095736 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 7 Aug 2025 16:50:31 -0700 Subject: [PATCH 5/6] Make `channel_parameters` explicit to `build_counterparty_commitment_tx` It's best to let the caller decide what the appropriate `ChannelTransactionParameters` are as it has the most context. --- lightning/src/chain/channelmonitor.rs | 30 +++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 04e81c93650..15bd6516317 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -4187,8 +4187,14 @@ impl ChannelMonitorImpl { to_countersignatory_value, )| { let nondust_htlcs = vec![]; + // Since we're expected to only reach here during the initial persistence of a + // monitor (i.e., via [`Persist::persist_new_channel`]), we expect to only have + // one `FundingScope` present. + debug_assert!(self.pending_funding.is_empty()); + let channel_parameters = &self.funding.channel_parameters; let commitment_tx = self.build_counterparty_commitment_tx( + channel_parameters, INITIAL_COMMITMENT_NUMBER, &their_per_commitment_point, to_broadcaster_value, @@ -4206,11 +4212,12 @@ impl ChannelMonitorImpl { #[rustfmt::skip] fn build_counterparty_commitment_tx( - &self, commitment_number: u64, their_per_commitment_point: &PublicKey, - to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32, + &self, channel_parameters: &ChannelTransactionParameters, commitment_number: u64, + their_per_commitment_point: &PublicKey, to_broadcaster_value: u64, + to_countersignatory_value: u64, feerate_per_kw: u32, nondust_htlcs: Vec ) -> CommitmentTransaction { - let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable(); + let channel_parameters = &channel_parameters.as_counterparty_broadcastable(); CommitmentTransaction::new(commitment_number, their_per_commitment_point, to_broadcaster_value, to_countersignatory_value, feerate_per_kw, nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx) } @@ -4232,9 +4239,20 @@ impl ChannelMonitorImpl { htlc.transaction_output_index.map(|_| htlc).cloned() }).collect::>(); - let commitment_tx = self.build_counterparty_commitment_tx(commitment_number, - &their_per_commitment_point, to_broadcaster_value, - to_countersignatory_value, feerate_per_kw, nondust_htlcs); + // This monitor update variant is only applicable while there's a single + // `FundingScope` active, otherwise we expect to see + // `LatestCounterpartyCommitment` instead. + debug_assert!(self.pending_funding.is_empty()); + let channel_parameters = &self.funding.channel_parameters; + let commitment_tx = self.build_counterparty_commitment_tx( + channel_parameters, + commitment_number, + &their_per_commitment_point, + to_broadcaster_value, + to_countersignatory_value, + feerate_per_kw, + nondust_htlcs, + ); debug_assert_eq!(commitment_tx.trust().txid(), commitment_txid); From 775f3d9ce1a59bb08487f7ad133d022a43a1138a Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 7 Aug 2025 16:57:53 -0700 Subject: [PATCH 6/6] Use `FundingScope` spent when signing watchtower justice transactions Since there may be multiple counterparty commitment transactions for the same commitment number due to splicing, we have to locate the matching `FundingScope::channel_parameters` to provide the signer. Since this is intended to be called during `Persist::update_persisted_channel`, the monitor should have already had the update applied. --- lightning/src/chain/channelmonitor.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 15bd6516317..38eed18a814 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2125,6 +2125,10 @@ impl ChannelMonitor { /// to the commitment transaction being revoked, this will return a signed transaction, but /// the signature will not be valid. /// + /// Note that due to splicing, this can also return an `Err` when the counterparty commitment + /// this transaction is attempting to claim is no longer valid because the corresponding funding + /// transaction was spliced. + /// /// [`EcdsaChannelSigner::sign_justice_revoked_output`]: crate::sign::ecdsa::EcdsaChannelSigner::sign_justice_revoked_output /// [`Persist`]: crate::chain::chainmonitor::Persist #[rustfmt::skip] @@ -4285,7 +4289,17 @@ impl ChannelMonitorImpl { let revokeable_redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.counterparty_commitment_params.on_counterparty_tx_csv, &delayed_key); - let channel_parameters = &self.funding.channel_parameters; + let commitment_txid = &justice_tx.input[input_idx].previous_output.txid; + // Since there may be multiple counterparty commitment transactions for the same commitment + // number due to splicing, we have to locate the matching `FundingScope::channel_parameters` + // to provide the signer. Since this is intended to be called during + // `Persist::update_persisted_channel`, the monitor should have already had the update + // applied. + let channel_parameters = core::iter::once(&self.funding) + .chain(&self.pending_funding) + .find(|funding| funding.counterparty_claimable_outpoints.contains_key(commitment_txid)) + .map(|funding| &funding.channel_parameters) + .ok_or(())?; let sig = self.onchain_tx_handler.signer.sign_justice_revoked_output( &channel_parameters, &justice_tx, input_idx, value, &per_commitment_key, &self.onchain_tx_handler.secp_ctx,