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.

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

@@ -344,8 +344,8 @@ 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);
+			self.manager.blocks_disconnected(header.prev_blockhash, self.height as u32 - 1);
+			self.monitor.blocks_disconnected(header.prev_blockhash, self.height as u32 - 1);
 			self.height -= 1;
 			let removal_height = self.height;
 			self.txids_confirmed.retain(|_, height| removal_height != *height);

lightning-block-sync/src/init.rs

@@ -230,8 +230,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, new_best_block: BlockHash, new_best_height: u32) {
+		self.0.blocks_disconnected(new_best_block, new_best_height)
 	}
 }
 
@@ -257,7 +257,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, _new_best_block: BlockHash, _new_best_height: u32) {
 		unreachable!()
 	}
 }
@@ -300,19 +300,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(2))
 			.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(3))
 			.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(4))
 			.expect_block_connected(*main_chain.at_height(4));
 
 		let listeners = vec![
@@ -337,23 +334,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(2))
 			.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(2))
 			.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(2))
 			.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 +371,7 @@ mod tests {
 		let old_tip = fork_chain.tip();
 
 		let listener = MockChainListener::new()
-			.expect_block_disconnected(*old_tip)
+			.expect_blocks_disconnected(*old_tip)
 			.expect_block_connected(*new_tip);
 
 		let listeners = vec![(old_tip.block_hash, &listener as &dyn chain::Listen)];

lightning-block-sync/src/lib.rs

@@ -398,12 +398,14 @@ 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() {
+			self.chain_listener.blocks_disconnected(block.header.prev_blockhash, block.height - 1);
 		}
 	}
 
@@ -615,7 +617,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(*old_tip)
 			.expect_block_connected(*new_tip);
 		let mut notifier =
 			ChainNotifier { header_cache: &mut main_chain.header_cache(0..=2), chain_listener };
@@ -635,8 +637,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(2))
 			.expect_block_connected(*new_tip);
 		let mut notifier =
 			ChainNotifier { header_cache: &mut main_chain.header_cache(0..=3), chain_listener };
@@ -656,7 +657,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(*old_tip)
 			.expect_block_connected(*fork_chain.at_height(2))
 			.expect_block_connected(*new_tip);
 		let mut notifier =

lightning-block-sync/src/test_utils.rs

@@ -203,7 +203,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, _new_best_block: BlockHash, _new_best_height: u32) {}
 }
 
 pub struct MockChainListener {
@@ -231,7 +231,7 @@ impl MockChainListener {
 		self
 	}
 
-	pub fn expect_block_disconnected(self, block: BlockHeaderData) -> Self {
+	pub fn expect_blocks_disconnected(self, block: BlockHeaderData) -> Self {
 		self.expected_blocks_disconnected.borrow_mut().push_back(block);
 		self
 	}
@@ -264,14 +264,14 @@ impl chain::Listen for MockChainListener {
 		}
 	}
 
-	fn block_disconnected(&self, header: &Header, height: u32) {
+	fn blocks_disconnected(&self, new_best_block: BlockHash, new_height: u32) {
 		match self.expected_blocks_disconnected.borrow_mut().pop_front() {
 			None => {
-				panic!("Unexpected block disconnected: {:?}", header.block_hash());
+				panic!("Unexpected disconnect to {} at height {}", new_best_block, new_height);
 			},
 			Some(expected_block) => {
-				assert_eq!(header.block_hash(), expected_block.header.block_hash());
-				assert_eq!(height, expected_block.height);
+				assert_eq!(new_best_block, expected_block.header.prev_blockhash);
+				assert_eq!(new_height, expected_block.height - 1);
 			},
 		}
 	}

lightning-liquidity/src/manager.rs

@@ -583,14 +583,9 @@ 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, new_best_block: bitcoin::BlockHash, new_best_height: u32) {
 		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)
+			*best_block = BestBlock::new(new_best_block, new_best_height)
 		}
 
 		// TODO: Call block_disconnected on all sub-modules that require it, e.g., LSPS1MessageHandler.

lightning/src/chain/chainmonitor.rs

@@ -910,18 +910,18 @@ where
 		self.event_notifier.notify();
 	}
 
-	fn block_disconnected(&self, header: &Header, height: u32) {
+	fn blocks_disconnected(&self, new_best_block: BlockHash, new_best_height: u32) {
 		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 {}",
+			new_best_height,
+			new_best_block,
 		);
 		for monitor_state in monitor_states.values() {
-			monitor_state.monitor.block_disconnected(
-				header,
-				height,
+			monitor_state.monitor.blocks_disconnected(
+				new_best_block,
+				new_best_height,
 				&*self.broadcaster,
 				&*self.fee_estimator,
 				&self.logger,

lightning/src/chain/channelmonitor.rs

@@ -2099,13 +2099,8 @@ 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,
+	pub fn blocks_disconnected<B: Deref, F: Deref, L: Deref>(
+		&self, new_best_block: BlockHash, new_height: u32, broadcaster: B, fee_estimator: F,
 		logger: &L,
 	) where
 		B::Target: BroadcasterInterface,
@@ -2114,8 +2109,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
 		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(new_best_block, new_height, broadcaster, fee_estimator, &logger)
 	}
 
 	/// Processes transactions confirmed in a block with the given header and height, returning new
@@ -2149,10 +2143,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,
@@ -4591,12 +4585,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 containing 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 containing it.");
 						matured_htlcs.push(source.clone());
 					}
 
@@ -4734,26 +4728,26 @@ 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, new_best_block: BlockHash, new_height: u32, 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);
+		log_trace!(logger, "Block(s) disconnected to height {}", new_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);
 
 		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 = BestBlock::new(new_best_block, new_height);
 	}
 
 	#[rustfmt::skip]
@@ -5198,8 +5192,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, new_best_block: BlockHash, new_best_height: u32) {
+		self.0.blocks_disconnected(new_best_block, new_best_height, &*self.1, &*self.2, &self.3);
 	}
 }
 

lightning/src/chain/mod.rs

@@ -84,8 +84,10 @@ 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.
+	///
+	/// Indicates the new best tip is `new_best_block` at height `disconnected_height` - 1.
+	fn blocks_disconnected(&self, new_best_block: BlockHash, disconnected_height: u32);
 }
 
 /// The `Confirm` trait is used to notify LDK when relevant transactions have been confirmed on
@@ -272,7 +274,7 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
 	///
 	/// 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 +284,7 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
 	///
 	/// [`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<ChannelSigner>,
 	) -> Result<ChannelMonitorUpdateStatus, ()>;
@@ -393,8 +395,8 @@ impl<T: Listen> Listen for dyn core::ops::Deref<Target = T> {
 		(**self).filtered_block_connected(header, txdata, height);
 	}
 
-	fn block_disconnected(&self, header: &Header, height: u32) {
-		(**self).block_disconnected(header, height);
+	fn blocks_disconnected(&self, new_best_block: BlockHash, new_best_height: u32) {
+		(**self).blocks_disconnected(new_best_block, new_best_height);
 	}
 }
 
@@ -408,9 +410,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, new_best_block: BlockHash, new_best_height: u32) {
+		self.0.blocks_disconnected(new_best_block, new_best_height);
+		self.1.blocks_disconnected(new_best_block, new_best_height);
 	}
 }