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.

Author: TheBlueMatt

fuzz/src/full_stack.rs

@@ -345,8 +345,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);

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)];

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<ValidatedBlockHeader>) {
-		for header in disconnected_blocks.drain(..) {
+	fn disconnect_blocks(&mut self, disconnected_blocks: Vec<ValidatedBlockHeader>) {
+		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 =

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);
 			},
 		}
 	}

lightning-liquidity/src/manager.rs

@@ -754,14 +754,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.

lightning/src/chain/chainmonitor.rs

@@ -33,7 +33,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;
 use crate::ln::msgs::{self, BaseMessageHandler, Init, MessageSendEvent, SendOnlyMessageHandler};
@@ -929,18 +929,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,

lightning/src/chain/channelmonitor.rs

@@ -2209,23 +2209,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// Determines if the disconnected block contained any transactions of interest and updates
 	/// appropriately.
-	#[rustfmt::skip]
-	pub fn block_disconnected<B: Deref, F: Deref, L: Deref>(
-		&self,
-		header: &Header,
-		height: u32,
-		broadcaster: B,
-		fee_estimator: F,
-		logger: &L,
+	pub fn blocks_disconnected<B: Deref, F: Deref, L: Deref>(
+		&self, fork_point: BestBlock, broadcaster: B, fee_estimator: F, logger: &L,
 	) where
 		B::Target: BroadcasterInterface,
 		F::Target: FeeEstimator,
 		L::Target: Logger,
 	{
 		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
@@ -2259,10 +2252,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
 		&self,
@@ -5171,12 +5164,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 							!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());
 					}
 
@@ -5324,22 +5317,25 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	}
 
 	#[rustfmt::skip]
-	fn block_disconnected<B: Deref, F: Deref, L: Deref>(
-		&mut self, header: &Header, height: u32, broadcaster: B, fee_estimator: F, logger: &WithChannelMonitor<L>
+	fn blocks_disconnected<B: Deref, F: Deref, L: Deref>(
+		&mut self, fork_point: BestBlock, broadcaster: B, fee_estimator: F, logger: &WithChannelMonitor<L>
 	) 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.
 		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 {
 					// Cancel any previous claims that are no longer valid as they stemmed from a
@@ -5355,10 +5351,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		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
 		);
 
-		self.best_block = BestBlock::new(header.prev_blockhash, height - 1);
+
+		self.best_block = fork_point;
 	}
 
 	#[rustfmt::skip]
@@ -5818,8 +5815,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);
 	}
 }