Merge pull request #2287 from TheBlueMatt/2023-05-no-background-event-dup-persist

Stop persisting background shutdown monitor updates

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -501,9 +501,11 @@ struct ClaimablePayments {
 /// for some reason. They are handled in timer_tick_occurred, so may be processed with
 /// quite some time lag.
 enum BackgroundEvent {
-	/// Handle a ChannelMonitorUpdate that closes a channel, broadcasting its current latest holder
-	/// commitment transaction.
-	ClosingMonitorUpdate((OutPoint, ChannelMonitorUpdate)),
+	/// Handle a ChannelMonitorUpdate
+	///
+	/// Note that any such events are lost on shutdown, so in general they must be updates which
+	/// are regenerated on startup.
+	MonitorUpdateRegeneratedOnStartup((OutPoint, ChannelMonitorUpdate)),
 }
 
 #[derive(Debug)]
@@ -3774,7 +3776,7 @@ where
 
 		for event in background_events.drain(..) {
 			match event {
-				BackgroundEvent::ClosingMonitorUpdate((funding_txo, update)) => {
+				BackgroundEvent::MonitorUpdateRegeneratedOnStartup((funding_txo, update)) => {
 					// The channel has already been closed, so no use bothering to care about the
 					// monitor updating completing.
 					let _ = self.chain_monitor.update_channel(funding_txo, &update);
@@ -5694,7 +5696,7 @@ where
 				if let ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast } = update.updates[0] {
 					assert!(should_broadcast);
 				} else { unreachable!(); }
-				self.pending_background_events.lock().unwrap().push(BackgroundEvent::ClosingMonitorUpdate((funding_txo, update)));
+				self.pending_background_events.lock().unwrap().push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup((funding_txo, update)));
 			}
 			self.finish_force_close_channel(failure);
 		}
@@ -7448,17 +7450,12 @@ where
 			}
 		}
 
-		let background_events = self.pending_background_events.lock().unwrap();
-		(background_events.len() as u64).write(writer)?;
-		for event in background_events.iter() {
-			match event {
-				BackgroundEvent::ClosingMonitorUpdate((funding_txo, monitor_update)) => {
-					0u8.write(writer)?;
-					funding_txo.write(writer)?;
-					monitor_update.write(writer)?;
-				},
-			}
-		}
+		// LDK versions prior to 0.0.116 wrote the `pending_background_events`
+		// `MonitorUpdateRegeneratedOnStartup`s here, however there was never a reason to do so -
+		// the closing monitor updates were always effectively replayed on startup (either directly
+		// by calling `broadcast_latest_holder_commitment_txn` on a `ChannelMonitor` during
+		// deserialization or, in 0.0.115, by regenerating the monitor update itself).
+		0u64.write(writer)?;
 
 		// Prior to 0.0.111 we tracked node_announcement serials here, however that now happens in
 		// `PeerManager`, and thus we simply write the `highest_seen_timestamp` twice, which is
@@ -7773,7 +7770,7 @@ where
 						log_bytes!(channel.channel_id()), monitor.get_latest_update_id(), channel.get_latest_monitor_update_id());
 					let (monitor_update, mut new_failed_htlcs) = channel.force_shutdown(true);
 					if let Some(monitor_update) = monitor_update {
-						pending_background_events.push(BackgroundEvent::ClosingMonitorUpdate(monitor_update));
+						pending_background_events.push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup(monitor_update));
 					}
 					failed_htlcs.append(&mut new_failed_htlcs);
 					channel_closures.push_back((events::Event::ChannelClosed {
@@ -7848,7 +7845,7 @@ where
 					update_id: CLOSED_CHANNEL_UPDATE_ID,
 					updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast: true }],
 				};
-				pending_background_events.push(BackgroundEvent::ClosingMonitorUpdate((*funding_txo, monitor_update)));
+				pending_background_events.push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup((*funding_txo, monitor_update)));
 			}
 		}
 
@@ -7905,13 +7902,11 @@ where
 		for _ in 0..background_event_count {
 			match <u8 as Readable>::read(reader)? {
 				0 => {
-					let (funding_txo, monitor_update): (OutPoint, ChannelMonitorUpdate) = (Readable::read(reader)?, Readable::read(reader)?);
-					if pending_background_events.iter().find(|e| {
-						let BackgroundEvent::ClosingMonitorUpdate((pending_funding_txo, pending_monitor_update)) = e;
-						*pending_funding_txo == funding_txo && *pending_monitor_update == monitor_update
-					}).is_none() {
-						pending_background_events.push(BackgroundEvent::ClosingMonitorUpdate((funding_txo, monitor_update)));
-					}
+					// LDK versions prior to 0.0.116 wrote pending `MonitorUpdateRegeneratedOnStartup`s here,
+					// however we really don't (and never did) need them - we regenerate all
+					// on-startup monitor updates.
+					let _: OutPoint = Readable::read(reader)?;
+					let _: ChannelMonitorUpdate = Readable::read(reader)?;
 				}
 				_ => return Err(DecodeError::InvalidValue),
 			}