From 403dc1a48bb71ae794f6883ae0b760aad44cda39 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 18 Jun 2025 20:51:16 +0000 Subject: [PATCH 1/5] Drop the need for fork headers when calling `Listen`'s disconnect The `Listen::block_disconnected` method is nice in that listeners learn about each block disconnected in series. Further, it included the header of the block that is being disconnected to allow the listeners to do some checking that the interface is being used correctly (namely, asserting that the header's block hash matches their current understanding of the best chain). However, this interface has some substantial drawbacks. Namely, the requirement that fork headers be passed in means that restarting with a new node that has no idea about a previous fork leaves us unable to replay the chain at all. Further, while when various listeners were initially written learning about each block disconnected in series seemed useful, but now we no longer rely on that anyway because the `Confirm` interface does not allow for it. Thus, here, we replace `Listen::block_disconnected` with a new `Listen::blocks_disconnected`, taking only information about the fork point/new best chain tip (in the form of its block hash and height) rather than information about previous fork blocks and only requiring a single call to complete multiple block disconnections during a reorg. We also swap to using a single `BestBlock` to describe the new chain tip, in anticipation of future extensions to `BestBlock`. This requires removing some assertions on block disconnection ordering, but because we now provide `lightning-block-sync` and expect users to use it when using the `Listen` interface, these assertions are much less critical. --- fuzz/src/full_stack.rs | 5 +-- lightning-block-sync/src/init.rs | 30 ++++++---------- lightning-block-sync/src/lib.rs | 20 ++++++----- lightning-block-sync/src/test_utils.rs | 20 ++++++----- lightning-liquidity/src/manager.rs | 11 +++--- lightning/src/chain/chainmonitor.rs | 15 ++++---- lightning/src/chain/channelmonitor.rs | 42 ++++++++++------------- lightning/src/chain/mod.rs | 23 ++++++++----- lightning/src/ln/channelmanager.rs | 21 +++++------- lightning/src/ln/functional_test_utils.rs | 5 +-- lightning/src/util/sweep.rs | 26 +++----------- 11 files changed, 98 insertions(+), 120 deletions(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 2e4e6bd4af2..eddf2cec474 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -343,8 +343,9 @@ impl<'a> MoneyLossDetector<'a> { self.header_hashes[self.height - 1].0, self.header_hashes[self.height].1, ); - self.manager.block_disconnected(&header, self.height as u32); - self.monitor.block_disconnected(&header, self.height as u32); + let best_block = BestBlock::new(header.prev_blockhash, self.height as u32 - 1); + self.manager.blocks_disconnected(best_block); + self.monitor.blocks_disconnected(best_block); self.height -= 1; let removal_height = self.height; self.txids_confirmed.retain(|_, height| removal_height != *height); diff --git a/lightning-block-sync/src/init.rs b/lightning-block-sync/src/init.rs index f71a72456dc..a870f8ca88c 100644 --- a/lightning-block-sync/src/init.rs +++ b/lightning-block-sync/src/init.rs @@ -9,6 +9,7 @@ use bitcoin::hash_types::BlockHash; use bitcoin::network::Network; use lightning::chain; +use lightning::chain::BestBlock; use std::ops::Deref; @@ -230,8 +231,8 @@ impl<'a, L: chain::Listen + ?Sized> chain::Listen for DynamicChainListener<'a, L unreachable!() } - fn block_disconnected(&self, header: &Header, height: u32) { - self.0.block_disconnected(header, height) + fn blocks_disconnected(&self, fork_point: BestBlock) { + self.0.blocks_disconnected(fork_point) } } @@ -257,7 +258,7 @@ impl<'a, L: chain::Listen + ?Sized> chain::Listen for ChainListenerSet<'a, L> { } } - fn block_disconnected(&self, _header: &Header, _height: u32) { + fn blocks_disconnected(&self, _fork_point: BestBlock) { unreachable!() } } @@ -300,19 +301,16 @@ mod tests { let fork_chain_3 = main_chain.fork_at_height(3); let listener_1 = MockChainListener::new() - .expect_block_disconnected(*fork_chain_1.at_height(4)) - .expect_block_disconnected(*fork_chain_1.at_height(3)) - .expect_block_disconnected(*fork_chain_1.at_height(2)) + .expect_blocks_disconnected(*fork_chain_1.at_height(1)) .expect_block_connected(*main_chain.at_height(2)) .expect_block_connected(*main_chain.at_height(3)) .expect_block_connected(*main_chain.at_height(4)); let listener_2 = MockChainListener::new() - .expect_block_disconnected(*fork_chain_2.at_height(4)) - .expect_block_disconnected(*fork_chain_2.at_height(3)) + .expect_blocks_disconnected(*fork_chain_2.at_height(2)) .expect_block_connected(*main_chain.at_height(3)) .expect_block_connected(*main_chain.at_height(4)); let listener_3 = MockChainListener::new() - .expect_block_disconnected(*fork_chain_3.at_height(4)) + .expect_blocks_disconnected(*fork_chain_3.at_height(3)) .expect_block_connected(*main_chain.at_height(4)); let listeners = vec![ @@ -337,23 +335,17 @@ mod tests { let fork_chain_3 = fork_chain_2.fork_at_height(3); let listener_1 = MockChainListener::new() - .expect_block_disconnected(*fork_chain_1.at_height(4)) - .expect_block_disconnected(*fork_chain_1.at_height(3)) - .expect_block_disconnected(*fork_chain_1.at_height(2)) + .expect_blocks_disconnected(*fork_chain_1.at_height(1)) .expect_block_connected(*main_chain.at_height(2)) .expect_block_connected(*main_chain.at_height(3)) .expect_block_connected(*main_chain.at_height(4)); let listener_2 = MockChainListener::new() - .expect_block_disconnected(*fork_chain_2.at_height(4)) - .expect_block_disconnected(*fork_chain_2.at_height(3)) - .expect_block_disconnected(*fork_chain_2.at_height(2)) + .expect_blocks_disconnected(*fork_chain_2.at_height(1)) .expect_block_connected(*main_chain.at_height(2)) .expect_block_connected(*main_chain.at_height(3)) .expect_block_connected(*main_chain.at_height(4)); let listener_3 = MockChainListener::new() - .expect_block_disconnected(*fork_chain_3.at_height(4)) - .expect_block_disconnected(*fork_chain_3.at_height(3)) - .expect_block_disconnected(*fork_chain_3.at_height(2)) + .expect_blocks_disconnected(*fork_chain_3.at_height(1)) .expect_block_connected(*main_chain.at_height(2)) .expect_block_connected(*main_chain.at_height(3)) .expect_block_connected(*main_chain.at_height(4)); @@ -380,7 +372,7 @@ mod tests { let old_tip = fork_chain.tip(); let listener = MockChainListener::new() - .expect_block_disconnected(*old_tip) + .expect_blocks_disconnected(*fork_chain.at_height(1)) .expect_block_connected(*new_tip); let listeners = vec![(old_tip.block_hash, &listener as &dyn chain::Listen)]; diff --git a/lightning-block-sync/src/lib.rs b/lightning-block-sync/src/lib.rs index 281a05a6a98..c1684532181 100644 --- a/lightning-block-sync/src/lib.rs +++ b/lightning-block-sync/src/lib.rs @@ -49,7 +49,7 @@ use bitcoin::hash_types::BlockHash; use bitcoin::pow::Work; use lightning::chain; -use lightning::chain::Listen; +use lightning::chain::{BestBlock, Listen}; use std::future::Future; use std::ops::Deref; @@ -398,12 +398,15 @@ where } /// Notifies the chain listeners of disconnected blocks. - fn disconnect_blocks(&mut self, mut disconnected_blocks: Vec) { - for header in disconnected_blocks.drain(..) { + fn disconnect_blocks(&mut self, disconnected_blocks: Vec) { + for header in disconnected_blocks.iter() { if let Some(cached_header) = self.header_cache.block_disconnected(&header.block_hash) { - assert_eq!(cached_header, header); + assert_eq!(cached_header, *header); } - self.chain_listener.block_disconnected(&header.header, header.height); + } + if let Some(block) = disconnected_blocks.last() { + let fork_point = BestBlock::new(block.header.prev_blockhash, block.height - 1); + self.chain_listener.blocks_disconnected(fork_point); } } @@ -615,7 +618,7 @@ mod chain_notifier_tests { let new_tip = fork_chain.tip(); let old_tip = main_chain.tip(); let chain_listener = &MockChainListener::new() - .expect_block_disconnected(*old_tip) + .expect_blocks_disconnected(*fork_chain.at_height(1)) .expect_block_connected(*new_tip); let mut notifier = ChainNotifier { header_cache: &mut main_chain.header_cache(0..=2), chain_listener }; @@ -635,8 +638,7 @@ mod chain_notifier_tests { let new_tip = fork_chain.tip(); let old_tip = main_chain.tip(); let chain_listener = &MockChainListener::new() - .expect_block_disconnected(*old_tip) - .expect_block_disconnected(*main_chain.at_height(2)) + .expect_blocks_disconnected(*main_chain.at_height(1)) .expect_block_connected(*new_tip); let mut notifier = ChainNotifier { header_cache: &mut main_chain.header_cache(0..=3), chain_listener }; @@ -656,7 +658,7 @@ mod chain_notifier_tests { let new_tip = fork_chain.tip(); let old_tip = main_chain.tip(); let chain_listener = &MockChainListener::new() - .expect_block_disconnected(*old_tip) + .expect_blocks_disconnected(*fork_chain.at_height(1)) .expect_block_connected(*fork_chain.at_height(2)) .expect_block_connected(*new_tip); let mut notifier = diff --git a/lightning-block-sync/src/test_utils.rs b/lightning-block-sync/src/test_utils.rs index 098f1a8769a..d307c4506eb 100644 --- a/lightning-block-sync/src/test_utils.rs +++ b/lightning-block-sync/src/test_utils.rs @@ -13,6 +13,7 @@ use bitcoin::transaction; use bitcoin::Transaction; use lightning::chain; +use lightning::chain::BestBlock; use std::cell::RefCell; use std::collections::VecDeque; @@ -203,7 +204,7 @@ impl chain::Listen for NullChainListener { &self, _header: &Header, _txdata: &chain::transaction::TransactionData, _height: u32, ) { } - fn block_disconnected(&self, _header: &Header, _height: u32) {} + fn blocks_disconnected(&self, _fork_point: BestBlock) {} } pub struct MockChainListener { @@ -231,8 +232,8 @@ impl MockChainListener { self } - pub fn expect_block_disconnected(self, block: BlockHeaderData) -> Self { - self.expected_blocks_disconnected.borrow_mut().push_back(block); + pub fn expect_blocks_disconnected(self, fork_point: BlockHeaderData) -> Self { + self.expected_blocks_disconnected.borrow_mut().push_back(fork_point); self } } @@ -264,14 +265,17 @@ impl chain::Listen for MockChainListener { } } - fn block_disconnected(&self, header: &Header, height: u32) { + fn blocks_disconnected(&self, fork_point: BestBlock) { match self.expected_blocks_disconnected.borrow_mut().pop_front() { None => { - panic!("Unexpected block disconnected: {:?}", header.block_hash()); + panic!( + "Unexpected block(s) disconnected {} at height {}", + fork_point.block_hash, fork_point.height, + ); }, - Some(expected_block) => { - assert_eq!(header.block_hash(), expected_block.header.block_hash()); - assert_eq!(height, expected_block.height); + Some(expected) => { + assert_eq!(fork_point.block_hash, expected.header.block_hash()); + assert_eq!(fork_point.height, expected.height); }, } } diff --git a/lightning-liquidity/src/manager.rs b/lightning-liquidity/src/manager.rs index 4cf97786d02..2495583bb51 100644 --- a/lightning-liquidity/src/manager.rs +++ b/lightning-liquidity/src/manager.rs @@ -767,14 +767,11 @@ where self.best_block_updated(header, height); } - fn block_disconnected(&self, header: &bitcoin::block::Header, height: u32) { - let new_height = height - 1; + fn blocks_disconnected(&self, fork_point: BestBlock) { if let Some(best_block) = self.best_block.write().unwrap().as_mut() { - assert_eq!(best_block.block_hash, header.block_hash(), - "Blocks must be disconnected in chain-order - the disconnected header must be the last connected header"); - assert_eq!(best_block.height, height, - "Blocks must be disconnected in chain-order - the disconnected block must have the correct height"); - *best_block = BestBlock::new(header.prev_blockhash, new_height) + assert!(best_block.height > fork_point.height, + "Blocks disconnected must indicate disconnection from the current best height, i.e. the new chain tip must be lower than the previous best height"); + *best_block = fork_point; } // TODO: Call block_disconnected on all sub-modules that require it, e.g., LSPS1MessageHandler. diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 386ef0a60c5..80163512aa6 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -35,7 +35,7 @@ use crate::chain::channelmonitor::{ WithChannelMonitor, }; use crate::chain::transaction::{OutPoint, TransactionData}; -use crate::chain::{ChannelMonitorUpdateStatus, Filter, WatchedOutput}; +use crate::chain::{BestBlock, ChannelMonitorUpdateStatus, Filter, WatchedOutput}; use crate::events::{self, Event, EventHandler, ReplayEvent}; use crate::ln::channel_state::ChannelDetails; #[cfg(peer_storage)] @@ -1008,18 +1008,17 @@ where self.event_notifier.notify(); } - fn block_disconnected(&self, header: &Header, height: u32) { + fn blocks_disconnected(&self, fork_point: BestBlock) { let monitor_states = self.monitors.read().unwrap(); log_debug!( self.logger, - "Latest block {} at height {} removed via block_disconnected", - header.block_hash(), - height + "Block(s) removed to height {} via blocks_disconnected. New best block is {}", + fork_point.height, + fork_point.block_hash, ); for monitor_state in monitor_states.values() { - monitor_state.monitor.block_disconnected( - header, - height, + monitor_state.monitor.blocks_disconnected( + fork_point, &*self.broadcaster, &*self.fee_estimator, &self.logger, diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index ee36c40130e..af58d96c85b 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2297,14 +2297,8 @@ impl ChannelMonitor { /// Determines if the disconnected block contained any transactions of interest and updates /// appropriately. - #[rustfmt::skip] - pub fn block_disconnected( - &self, - header: &Header, - height: u32, - broadcaster: B, - fee_estimator: F, - logger: &L, + pub fn blocks_disconnected( + &self, fork_point: BestBlock, broadcaster: B, fee_estimator: F, logger: &L, ) where B::Target: BroadcasterInterface, F::Target: FeeEstimator, @@ -2312,8 +2306,7 @@ impl ChannelMonitor { { let mut inner = self.inner.lock().unwrap(); let logger = WithChannelMonitor::from_impl(logger, &*inner, None); - inner.block_disconnected( - header, height, broadcaster, fee_estimator, &logger) + inner.blocks_disconnected(fork_point, broadcaster, fee_estimator, &logger) } /// Processes transactions confirmed in a block with the given header and height, returning new @@ -2347,10 +2340,10 @@ impl ChannelMonitor { /// Processes a transaction that was reorganized out of the chain. /// - /// Used instead of [`block_disconnected`] by clients that are notified of transactions rather + /// Used instead of [`blocks_disconnected`] by clients that are notified of transactions rather /// than blocks. See [`chain::Confirm`] for calling expectations. /// - /// [`block_disconnected`]: Self::block_disconnected + /// [`blocks_disconnected`]: Self::blocks_disconnected #[rustfmt::skip] pub fn transaction_unconfirmed( &self, @@ -5441,12 +5434,12 @@ impl ChannelMonitorImpl { !unmatured_htlcs.contains(&source), "An unmature HTLC transaction conflicts with a maturing one; failed to \ call either transaction_unconfirmed for the conflicting transaction \ - or block_disconnected for a block containing it."); + or blocks_disconnected for a block before it."); debug_assert!( !matured_htlcs.contains(&source), "A matured HTLC transaction conflicts with a maturing one; failed to \ call either transaction_unconfirmed for the conflicting transaction \ - or block_disconnected for a block containing it."); + or blocks_disconnected for a block before it."); matured_htlcs.push(source.clone()); } @@ -5594,23 +5587,26 @@ impl ChannelMonitorImpl { } #[rustfmt::skip] - fn block_disconnected( - &mut self, header: &Header, height: u32, broadcaster: B, fee_estimator: F, logger: &WithChannelMonitor + fn blocks_disconnected( + &mut self, fork_point: BestBlock, broadcaster: B, fee_estimator: F, logger: &WithChannelMonitor ) where B::Target: BroadcasterInterface, F::Target: FeeEstimator, L::Target: Logger, { - log_trace!(logger, "Block {} at height {} disconnected", header.block_hash(), height); + let new_height = fork_point.height; + log_trace!(logger, "Block(s) disconnected to height {}", new_height); + assert!(self.best_block.height > fork_point.height, + "Blocks disconnected must indicate disconnection from the current best height, i.e. the new chain tip must be lower than the previous best height"); //We may discard: //- htlc update there as failure-trigger tx (revoked commitment tx, non-revoked commitment tx, HTLC-timeout tx) has been disconnected //- maturing spendable output has transaction paying us has been disconnected - self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height < height); + self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height <= new_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 { + if *conf_height > new_height { self.alternative_funding_confirmed.take(); if self.holder_tx_signed || self.funding_spend_seen { // Cancel any previous claims that are no longer valid as they stemmed from a @@ -5627,7 +5623,7 @@ 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 + new_height + 1, &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 @@ -5636,7 +5632,7 @@ impl ChannelMonitorImpl { self.queue_latest_holder_commitment_txn_for_broadcast(&broadcaster, &bounded_fee_estimator, logger); } - self.best_block = BestBlock::new(header.prev_blockhash, height - 1); + self.best_block = fork_point; } #[rustfmt::skip] @@ -6110,8 +6106,8 @@ where self.0.block_connected(header, txdata, height, &*self.1, &*self.2, &self.3); } - fn block_disconnected(&self, header: &Header, height: u32) { - self.0.block_disconnected(header, height, &*self.1, &*self.2, &self.3); + fn blocks_disconnected(&self, fork_point: BestBlock) { + self.0.blocks_disconnected(fork_point, &*self.1, &*self.2, &self.3); } } diff --git a/lightning/src/chain/mod.rs b/lightning/src/chain/mod.rs index c16ee2519f7..35a01d7c46f 100644 --- a/lightning/src/chain/mod.rs +++ b/lightning/src/chain/mod.rs @@ -84,8 +84,13 @@ pub trait Listen { self.filtered_block_connected(&block.header, &txdata, height); } - /// Notifies the listener that a block was removed at the given height. - fn block_disconnected(&self, header: &Header, height: u32); + /// Notifies the listener that one or more blocks were removed in anticipation of a reorg. + /// + /// The provided [`BestBlock`] is the new best block after disconnecting blocks in the reorg + /// but before connecting new ones (i.e. the "fork point" block). For backwards compatibility, + /// you may instead walk the chain backwards, calling `blocks_disconnected` for each block + /// that is disconnected in a reorg. + fn blocks_disconnected(&self, fork_point_block: BestBlock); } /// The `Confirm` trait is used to notify LDK when relevant transactions have been confirmed on @@ -272,7 +277,7 @@ pub trait Watch { /// /// Implementations are responsible for watching the chain for the funding transaction along /// with any spends of outputs returned by [`get_outputs_to_watch`]. In practice, this means - /// calling [`block_connected`] and [`block_disconnected`] on the monitor. + /// calling [`block_connected`] and [`blocks_disconnected`] on the monitor. /// /// A return of `Err(())` indicates that the channel should immediately be force-closed without /// broadcasting the funding transaction. @@ -282,7 +287,7 @@ pub trait Watch { /// /// [`get_outputs_to_watch`]: channelmonitor::ChannelMonitor::get_outputs_to_watch /// [`block_connected`]: channelmonitor::ChannelMonitor::block_connected - /// [`block_disconnected`]: channelmonitor::ChannelMonitor::block_disconnected + /// [`blocks_disconnected`]: channelmonitor::ChannelMonitor::blocks_disconnected fn watch_channel( &self, channel_id: ChannelId, monitor: ChannelMonitor, ) -> Result; @@ -393,8 +398,8 @@ impl Listen for dyn core::ops::Deref { (**self).filtered_block_connected(header, txdata, height); } - fn block_disconnected(&self, header: &Header, height: u32) { - (**self).block_disconnected(header, height); + fn blocks_disconnected(&self, fork_point: BestBlock) { + (**self).blocks_disconnected(fork_point); } } @@ -408,9 +413,9 @@ where self.1.filtered_block_connected(header, txdata, height); } - fn block_disconnected(&self, header: &Header, height: u32) { - self.0.block_disconnected(header, height); - self.1.block_disconnected(header, height); + fn blocks_disconnected(&self, fork_point: BestBlock) { + self.0.blocks_disconnected(fork_point); + self.1.blocks_disconnected(fork_point); } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index cfef0540a97..78bd2fa11f4 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3716,12 +3716,12 @@ where /// Non-proportional fees are fixed according to our risk using the provided fee estimator. /// /// Users need to notify the new `ChannelManager` when a new block is connected or - /// disconnected using its [`block_connected`] and [`block_disconnected`] methods, starting + /// disconnected using its [`block_connected`] and [`blocks_disconnected`] methods, starting /// from after [`params.best_block.block_hash`]. See [`chain::Listen`] and [`chain::Confirm`] for /// more details. /// /// [`block_connected`]: chain::Listen::block_connected - /// [`block_disconnected`]: chain::Listen::block_disconnected + /// [`blocks_disconnected`]: chain::Listen::blocks_disconnected /// [`params.best_block.block_hash`]: chain::BestBlock::block_hash #[rustfmt::skip] pub fn new( @@ -13286,26 +13286,23 @@ where self.best_block_updated(header, height); } - fn block_disconnected(&self, header: &Header, height: u32) { + fn blocks_disconnected(&self, fork_point: BestBlock) { let _persistence_guard = PersistenceNotifierGuard::optionally_notify_skipping_background_events( self, || -> NotifyOption { NotifyOption::DoPersist }, ); - let new_height = height - 1; { let mut best_block = self.best_block.write().unwrap(); - assert_eq!(best_block.block_hash, header.block_hash(), - "Blocks must be disconnected in chain-order - the disconnected header must be the last connected header"); - assert_eq!(best_block.height, height, - "Blocks must be disconnected in chain-order - the disconnected block must have the correct height"); - *best_block = BestBlock::new(header.prev_blockhash, new_height) + assert!(best_block.height > fork_point.height, + "Blocks disconnected must indicate disconnection from the current best height, i.e. the new chain tip must be lower than the previous best height"); + *best_block = fork_point; } - self.do_chain_event(Some(new_height), |channel| { + self.do_chain_event(Some(fork_point.height), |channel| { channel.best_block_updated( - new_height, - header.time, + fork_point.height, + 0, self.chain_hash, &self.node_signer, &self.config.read().unwrap(), diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 53d5173ee17..68d73c749be 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -428,8 +428,9 @@ pub fn disconnect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, count: u32) match *node.connect_style.borrow() { ConnectStyle::FullBlockViaListen => { - node.chain_monitor.chain_monitor.block_disconnected(&orig.0.header, orig.1); - Listen::block_disconnected(node.node, &orig.0.header, orig.1); + let best_block = BestBlock::new(orig.0.header.prev_blockhash, orig.1 - 1); + node.chain_monitor.chain_monitor.blocks_disconnected(best_block); + Listen::blocks_disconnected(node.node, best_block); }, ConnectStyle::BestBlockFirstSkippingBlocks | ConnectStyle::TransactionsFirstSkippingBlocks diff --git a/lightning/src/util/sweep.rs b/lightning/src/util/sweep.rs index b72dddbcc7c..aba22585034 100644 --- a/lightning/src/util/sweep.rs +++ b/lightning/src/util/sweep.rs @@ -281,16 +281,6 @@ impl OutputSpendStatus { } } - fn confirmation_hash(&self) -> Option { - match self { - Self::PendingInitialBroadcast { .. } => None, - Self::PendingFirstConfirmation { .. } => None, - Self::PendingThresholdConfirmations { confirmation_hash, .. } => { - Some(*confirmation_hash) - }, - } - } - fn latest_spending_tx(&self) -> Option<&Transaction> { match self { Self::PendingInitialBroadcast { .. } => None, @@ -759,21 +749,15 @@ where self.best_block_updated_internal(&mut state_lock, header, height); } - fn block_disconnected(&self, header: &Header, height: u32) { + fn blocks_disconnected(&self, fork_point: BestBlock) { let mut state_lock = self.sweeper_state.lock().unwrap(); - let new_height = height - 1; - let block_hash = header.block_hash(); - - assert_eq!(state_lock.best_block.block_hash, block_hash, - "Blocks must be disconnected in chain-order - the disconnected header must be the last connected header"); - assert_eq!(state_lock.best_block.height, height, - "Blocks must be disconnected in chain-order - the disconnected block must have the correct height"); - state_lock.best_block = BestBlock::new(header.prev_blockhash, new_height); + assert!(state_lock.best_block.height > fork_point.height, + "Blocks disconnected must indicate disconnection from the current best height, i.e. the new chain tip must be lower than the previous best height"); + state_lock.best_block = fork_point; for output_info in state_lock.outputs.iter_mut() { - if output_info.status.confirmation_hash() == Some(block_hash) { - debug_assert_eq!(output_info.status.confirmation_height(), Some(height)); + if output_info.status.confirmation_height() > Some(fork_point.height) { output_info.status.unconfirmed(); } } From 2d3aaa576a93d0e1fc6e8f9205608aa956e6492b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 1 Jul 2025 20:59:32 +0000 Subject: [PATCH 2/5] Use similar `blocks_disconnected` semantics in `OnchainTxHandler` `OnchainTxHandler` is an internal struct and doesn't implement `Listen`, but its still nice to have its API mirror the `Listen` API so that internal code all looks similar. --- lightning/src/chain/channelmonitor.rs | 8 ++++---- lightning/src/chain/onchaintx.rs | 24 +++++++++++------------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index af58d96c85b..b1bc2c0fcd8 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -5106,8 +5106,8 @@ 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, &self.destination_script, fee_estimator, logger, + self.onchain_tx_handler.blocks_disconnected( + height, &broadcaster, conf_target, &self.destination_script, fee_estimator, logger, ); Vec::new() } else { Vec::new() } @@ -5622,8 +5622,8 @@ impl ChannelMonitorImpl { let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator); let conf_target = self.closure_conf_target(); - self.onchain_tx_handler.block_disconnected( - new_height + 1, &broadcaster, conf_target, &self.destination_script, &bounded_fee_estimator, logger + self.onchain_tx_handler.blocks_disconnected( + new_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 diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index deb1282f1f3..4a35c7b7723 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1127,15 +1127,15 @@ impl OnchainTxHandler { } if let Some(height) = height { - self.block_disconnected( - height, broadcaster, conf_target, destination_script, fee_estimator, logger, + self.blocks_disconnected( + height - 1, broadcaster, conf_target, destination_script, fee_estimator, logger, ); } } #[rustfmt::skip] - pub(super) fn block_disconnected( - &mut self, height: u32, broadcaster: &B, conf_target: ConfirmationTarget, + pub(super) fn blocks_disconnected( + &mut self, new_best_height: u32, broadcaster: &B, conf_target: ConfirmationTarget, destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) where B::Target: BroadcasterInterface, @@ -1145,21 +1145,21 @@ impl OnchainTxHandler { let onchain_events_awaiting_threshold_conf = self.onchain_events_awaiting_threshold_conf.drain(..).collect::>(); for entry in onchain_events_awaiting_threshold_conf { - if entry.height >= height { + if entry.height > new_best_height { //- our claim tx on a commitment tx output //- resurect outpoint back in its claimable set and regenerate tx match entry.event { OnchainEvent::ContentiousOutpoint { package } => { // We pass 0 to `package_locktime` to get the actual required locktime. let package_locktime = package.package_locktime(0); - if package_locktime >= height { + if package_locktime > new_best_height { self.locktimed_packages.entry(package_locktime).or_default().push(package); continue; } if let Some(pending_claim) = self.claimable_outpoints.get(package.outpoints()[0]) { if let Some(request) = self.pending_claim_requests.get_mut(&pending_claim.0) { - assert!(request.merge_package(package, height).is_ok()); + assert!(request.merge_package(package, new_best_height + 1).is_ok()); // Using a HashMap guarantee us than if we have multiple outpoints getting // resurrected only one bump claim tx is going to be broadcast bump_candidates.insert(pending_claim.clone(), request.clone()); @@ -1173,10 +1173,8 @@ impl OnchainTxHandler { } } for ((_claim_id, _), ref mut request) in bump_candidates.iter_mut() { - // `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, + new_best_height, &request, &FeerateStrategy::ForceBump, conf_target, destination_script, fee_estimator, logger ) { request.set_timer(new_timer); @@ -1210,9 +1208,9 @@ impl OnchainTxHandler { // right now if one of the outpoint get disconnected, just erase whole pending claim request. let mut remove_request = Vec::new(); self.claimable_outpoints.retain(|_, ref v| - if v.1 >= height { - remove_request.push(v.0.clone()); - false + if v.1 > new_best_height { + remove_request.push(v.0.clone()); + false } else { true }); for req in remove_request { self.pending_claim_requests.remove(&req); From fea0beea5ec563486e67c9a6f1bfceb29fc70c2a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 18 Jun 2025 20:58:36 +0000 Subject: [PATCH 3/5] Add more robust functional test of `Listen::blocks_disconnected` Now that the `Listen` interface allows blocks to be disconnected in batches rather than one at a time, we should test this. Here we add a new `ConnectStyle` for the functional test framework which tests doing so. --- lightning/src/ln/functional_test_utils.rs | 18 ++++++++++++++++-- lightning/src/ln/functional_tests.rs | 14 +++++++++----- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 68d73c749be..d3432924fdf 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -196,6 +196,9 @@ pub enum ConnectStyle { /// Provides the full block via the `chain::Listen` interface. In the current code this is /// equivalent to `TransactionsFirst` with some additional assertions. FullBlockViaListen, + /// Provides the full block via the `chain::Listen` interface, condensing multiple block + /// disconnections into a single `blocks_disconnected` call. + FullBlockDisconnectionsSkippingViaListen, } impl ConnectStyle { @@ -210,6 +213,7 @@ impl ConnectStyle { ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks => true, ConnectStyle::TransactionsFirstReorgsOnlyTip => true, ConnectStyle::FullBlockViaListen => false, + ConnectStyle::FullBlockDisconnectionsSkippingViaListen => false, } } @@ -224,6 +228,7 @@ impl ConnectStyle { ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks => false, ConnectStyle::TransactionsFirstReorgsOnlyTip => false, ConnectStyle::FullBlockViaListen => false, + ConnectStyle::FullBlockDisconnectionsSkippingViaListen => false, } } @@ -231,7 +236,7 @@ impl ConnectStyle { use core::hash::{BuildHasher, Hasher}; // Get a random value using the only std API to do so - the DefaultHasher let rand_val = std::collections::hash_map::RandomState::new().build_hasher().finish(); - let res = match rand_val % 9 { + let res = match rand_val % 10 { 0 => ConnectStyle::BestBlockFirst, 1 => ConnectStyle::BestBlockFirstSkippingBlocks, 2 => ConnectStyle::BestBlockFirstReorgsOnlyTip, @@ -241,6 +246,7 @@ impl ConnectStyle { 6 => ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks, 7 => ConnectStyle::TransactionsFirstReorgsOnlyTip, 8 => ConnectStyle::FullBlockViaListen, + 9 => ConnectStyle::FullBlockDisconnectionsSkippingViaListen, _ => unreachable!(), }; eprintln!("Using Block Connection Style: {:?}", res); @@ -371,7 +377,8 @@ fn do_connect_block_without_consistency_checks<'a, 'b, 'c, 'd>( node.node.transactions_confirmed(&block.header, &txdata, height); node.node.best_block_updated(&block.header, height); }, - ConnectStyle::FullBlockViaListen => { + ConnectStyle::FullBlockViaListen + | ConnectStyle::FullBlockDisconnectionsSkippingViaListen => { node.chain_monitor.chain_monitor.block_connected(&block, height); node.node.block_connected(&block, height); }, @@ -432,6 +439,13 @@ pub fn disconnect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, count: u32) node.chain_monitor.chain_monitor.blocks_disconnected(best_block); Listen::blocks_disconnected(node.node, best_block); }, + ConnectStyle::FullBlockDisconnectionsSkippingViaListen => { + if i == count - 1 { + let best_block = BestBlock::new(orig.0.header.prev_blockhash, orig.1 - 1); + node.chain_monitor.chain_monitor.blocks_disconnected(best_block); + Listen::blocks_disconnected(node.node, best_block); + } + }, ConnectStyle::BestBlockFirstSkippingBlocks | ConnectStyle::TransactionsFirstSkippingBlocks | ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 5e78049664c..03fd8167b77 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2328,11 +2328,15 @@ pub fn test_htlc_ignore_latest_remote_commitment() { let node_a_id = nodes[0].node.get_our_node_id(); let node_b_id = nodes[1].node.get_our_node_id(); - if *nodes[1].connect_style.borrow() == ConnectStyle::FullBlockViaListen { - // We rely on the ability to connect a block redundantly, which isn't allowed via - // `chain::Listen`, so we never run the test if we randomly get assigned that - // connect_style. - return; + match *nodes[1].connect_style.borrow() { + ConnectStyle::FullBlockViaListen + | ConnectStyle::FullBlockDisconnectionsSkippingViaListen => { + // We rely on the ability to connect a block redundantly, which isn't allowed via + // `chain::Listen`, so we never run the test if we randomly get assigned that + // connect_style. + return; + }, + _ => {}, } let funding_tx = create_announced_chan_between_nodes(&nodes, 0, 1).3; let message = "Channel force-closed".to_owned(); From 227989941873d4e3364cf72f9677c1ff13be2378 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Jun 2025 15:55:04 +0000 Subject: [PATCH 4/5] Don't pass a latest-block-time to `Channel` unless we have one When calling `Channel::best_block_updated` we pass it the timestamp of the block we're connecting so that it can track the highest timestamp it has seen. However, in some cases, we don't actually have a timestamp to pass, which `Channel::best_block_updated` will happily ignore as it always takes the `max` of its existing value. Thus, we really should pass a `None` to ensure the API is understandable, which we do here. --- lightning/src/ln/channel.rs | 17 ++++++++--------- lightning/src/ln/channelmanager.rs | 23 ++++++++++++++++++++--- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 3d25934d18d..95f76835159 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -10550,8 +10550,8 @@ where /// May return some HTLCs (and their payment_hash) which have timed out and should be failed /// back. pub fn best_block_updated( - &mut self, height: u32, highest_header_time: u32, chain_hash: ChainHash, node_signer: &NS, - user_config: &UserConfig, logger: &L, + &mut self, height: u32, highest_header_time: Option, chain_hash: ChainHash, + node_signer: &NS, user_config: &UserConfig, logger: &L, ) -> Result where NS::Target: NodeSigner, @@ -10567,7 +10567,7 @@ where #[rustfmt::skip] fn do_best_block_updated( - &mut self, height: u32, highest_header_time: u32, + &mut self, height: u32, highest_header_time: Option, chain_node_signer: Option<(ChainHash, &NS, &UserConfig)>, logger: &L ) -> Result<(Option, Vec<(HTLCSource, PaymentHash)>, Option), ClosureReason> where @@ -10591,7 +10591,9 @@ where } }); - self.context.update_time_counter = cmp::max(self.context.update_time_counter, highest_header_time); + if let Some(time) = highest_header_time { + self.context.update_time_counter = cmp::max(self.context.update_time_counter, time); + } // Check if the funding transaction was unconfirmed let funding_tx_confirmations = self.funding.get_funding_tx_confirmations(height); @@ -10747,12 +10749,9 @@ where // We handle the funding disconnection by calling best_block_updated with a height one // below where our funding was connected, implying a reorg back to conf_height - 1. let reorg_height = funding.funding_tx_confirmation_height - 1; - // We use the time field to bump the current time we set on channel updates if its - // larger. If we don't know that time has moved forward, we can just set it to the last - // time we saw and it will be ignored. - let best_time = self.context.update_time_counter; - match self.do_best_block_updated(reorg_height, best_time, None::<(ChainHash, &&dyn NodeSigner, &UserConfig)>, logger) { + let signer_config = None::<(ChainHash, &&dyn NodeSigner, &UserConfig)>; + match self.do_best_block_updated(reorg_height, None, signer_config, logger) { Ok((channel_ready, timed_out_htlcs, announcement_sigs)) => { assert!(channel_ready.is_none(), "We can't generate a funding with 0 confirmations?"); assert!(timed_out_htlcs.is_empty(), "We can't have accepted HTLCs with a timeout before our funding confirmation?"); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 78bd2fa11f4..6c8b742d7f7 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -13302,7 +13302,7 @@ where self.do_chain_event(Some(fork_point.height), |channel| { channel.best_block_updated( fork_point.height, - 0, + None, self.chain_hash, &self.node_signer, &self.config.read().unwrap(), @@ -13352,7 +13352,17 @@ where let last_best_block_height = self.best_block.read().unwrap().height; if height < last_best_block_height { let timestamp = self.highest_seen_timestamp.load(Ordering::Acquire); - self.do_chain_event(Some(last_best_block_height), |channel| channel.best_block_updated(last_best_block_height, timestamp as u32, self.chain_hash, &self.node_signer, &self.config.read().unwrap(), &&WithChannelContext::from(&self.logger, &channel.context, None))); + let do_update = |channel: &mut FundedChannel| { + channel.best_block_updated( + last_best_block_height, + Some(timestamp as u32), + self.chain_hash, + &self.node_signer, + &self.config.read().unwrap(), + &&WithChannelContext::from(&self.logger, &channel.context, None), + ) + }; + self.do_chain_event(Some(last_best_block_height), do_update); } } @@ -13412,7 +13422,14 @@ where } } - channel.best_block_updated(height, header.time, self.chain_hash, &self.node_signer, &self.config.read().unwrap(), &&WithChannelContext::from(&self.logger, &channel.context, None)) + channel.best_block_updated( + height, + Some(header.time), + self.chain_hash, + &self.node_signer, + &self.config.read().unwrap(), + &&WithChannelContext::from(&self.logger, &channel.context, None), + ) }); macro_rules! max_time { From a5b745aff77c658b1385d00cc191c9ad7a08f32b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 25 Jun 2025 18:31:41 +0000 Subject: [PATCH 5/5] Add further additional documentation to `Listen` `Listen` is somewhat quiet on high-level use and even requirements, which we document further here. --- lightning/src/chain/mod.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/lightning/src/chain/mod.rs b/lightning/src/chain/mod.rs index 35a01d7c46f..2a6d3d23e80 100644 --- a/lightning/src/chain/mod.rs +++ b/lightning/src/chain/mod.rs @@ -73,6 +73,24 @@ impl_writeable_tlv_based!(BestBlock, { /// By using [`Listen::filtered_block_connected`] this interface supports clients fetching the /// entire header chain and only blocks with matching transaction data using BIP 157 filters or /// other similar filtering. +/// +/// # Requirements +/// +/// Each block must be connected in chain order with one call to either +/// [`Listen::block_connected`] or [`Listen::filtered_block_connected`]. If a call to the +/// [`Filter`] interface was made during block processing and further transaction(s) from the same +/// block now match the filter, a second call to [`Listen::filtered_block_connected`] should be +/// made immediately for the same block (prior to any other calls to the [`Listen`] interface). +/// +/// In case of a reorg, you must call [`Listen::blocks_disconnected`] once with information on the +/// "fork point" block, i.e. the highest block that is in both forks. You may call +/// [`Listen::blocks_disconnected`] multiple times as you walk the chain backwards, but each must +/// include a fork point block that is before the last. +/// +/// # Object Birthday +/// +/// Note that most implementations take a [`BestBlock`] on construction and blocks only need to be +/// applied starting from that point. pub trait Listen { /// Notifies the listener that a block was added at the given height, with the transaction data /// possibly filtered.