Drop PendingHTLCsForwardable event

Author: tnull

lightning/src/events/mod.rs

@@ -50,7 +50,6 @@ use bitcoin::script::ScriptBuf;
 use bitcoin::secp256k1::PublicKey;
 use bitcoin::{OutPoint, Transaction};
 use core::ops::Deref;
-use core::time::Duration;
 
 #[allow(unused_imports)]
 use crate::prelude::*;
@@ -1196,21 +1195,6 @@ pub enum Event {
 		/// with channels in the public network graph.
 		short_channel_id: Option<u64>,
 	},
-	/// Used to indicate that [`ChannelManager::process_pending_htlc_forwards`] should be called at
-	/// a time in the future.
-	///
-	/// # Failure Behavior and Persistence
-	/// This event will eventually be replayed after failures-to-handle (i.e., the event handler
-	/// returning `Err(ReplayEvent ())`) and will be regenerated after restarts.
-	///
-	/// [`ChannelManager::process_pending_htlc_forwards`]: crate::ln::channelmanager::ChannelManager::process_pending_htlc_forwards
-	PendingHTLCsForwardable {
-		/// The minimum amount of time that should be waited prior to calling
-		/// process_pending_htlc_forwards. To increase the effort required to correlate payments,
-		/// you should wait a random amount of time in roughly the range (now + time_forwardable,
-		/// now + 5*time_forwardable).
-		time_forwardable: Duration,
-	},
 	/// Used to indicate that we've intercepted an HTLC forward. This event will only be generated if
 	/// you've encoded an intercept scid in the receiver's invoice route hints using
 	/// [`ChannelManager::get_intercept_scid`] and have set [`UserConfig::accept_intercept_htlcs`].
@@ -1815,11 +1799,7 @@ impl Writeable for Event {
 					(13, failure, required),
 				});
 			},
-			&Event::PendingHTLCsForwardable { time_forwardable: _ } => {
-				4u8.write(writer)?;
-				// Note that we now ignore these on the read end as we'll re-generate them in
-				// ChannelManager, we write them here only for backwards compatibility.
-			},
+			// 4u8 used to be `PendingHTLCsForwardable`
 			&Event::SpendableOutputs { ref outputs, channel_id } => {
 				5u8.write(writer)?;
 				write_tlv_fields!(writer, {

lightning/src/ln/blinded_payment_tests.rs

@@ -636,10 +636,6 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
 					$curr_node.node.force_close_broadcasting_latest_txn(&$failed_chan_id, &$next_node.node.get_our_node_id(), error_message.to_string()).unwrap();
 					let events = $curr_node.node.get_and_clear_pending_events();
 					match events[0] {
-						crate::events::Event::PendingHTLCsForwardable { .. } => {},
-						_ => panic!("Unexpected event {:?}", events),
-					};
-					match events[1] {
 						crate::events::Event::ChannelClosed { .. } => {},
 						_ => panic!("Unexpected event {:?}", events),
 					}
@@ -1162,18 +1158,14 @@ fn blinded_path_retries() {
 			do_commitment_signed_dance(&nodes[0], &$intro_node, &updates.commitment_signed, false, false);
 
 			let mut events = nodes[0].node.get_and_clear_pending_events();
-			assert_eq!(events.len(), 2);
+			assert_eq!(events.len(), 1);
 			match events[0] {
 				Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently, ..  } => {
 					assert_eq!(payment_hash, ev_payment_hash);
 					assert_eq!(payment_failed_permanently, false);
 				},
 				_ => panic!("Unexpected event"),
 			}
-			match events[1] {
-				Event::PendingHTLCsForwardable { .. } => {},
-				_ => panic!("Unexpected event"),
-			}
 			nodes[0].node.process_pending_htlc_forwards();
 		}
 	}

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -2014,23 +2014,19 @@ fn test_monitor_update_on_pending_forwards() {
 	commitment_signed_dance!(nodes[0], nodes[1], bs_updates.commitment_signed, false, true);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
-	assert_eq!(events.len(), 3);
-	if let Event::PaymentPathFailed { payment_hash, payment_failed_permanently, .. } = events[1] {
+	assert_eq!(events.len(), 2);
+	if let Event::PaymentPathFailed { payment_hash, payment_failed_permanently, .. } = events[0] {
 		assert_eq!(payment_hash, payment_hash_1);
 		assert!(payment_failed_permanently);
 	} else {
 		panic!("Unexpected event!");
 	}
-	match events[2] {
+	match events[1] {
 		Event::PaymentFailed { payment_hash, .. } => {
 			assert_eq!(payment_hash, Some(payment_hash_1));
 		},
 		_ => panic!("Unexpected event"),
 	}
-	match events[0] {
-		Event::PendingHTLCsForwardable { .. } => {},
-		_ => panic!("Unexpected event"),
-	};
 	nodes[0].node.process_pending_htlc_forwards();
 	expect_payment_claimable!(nodes[0], payment_hash_2, payment_secret_2, 1000000);
 
@@ -2813,12 +2809,8 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) {
 	commitment_signed_dance!(nodes[1], nodes[0], (), false, true, false, false);
 
 	let events = nodes[1].node.get_and_clear_pending_events();
-	assert_eq!(events.len(), 2);
+	assert_eq!(events.len(), 1);
 	match events[0] {
-		Event::PendingHTLCsForwardable { .. } => {},
-		_ => panic!("Unexpected event"),
-	};
-	match events[1] {
 		Event::PaymentPathSuccessful { .. } => {},
 		_ => panic!("Unexpected event"),
 	};

lightning/src/ln/channelmanager.rs

@@ -962,12 +962,6 @@ impl MsgHandleErrInternal {
 	}
 }
 
-/// We hold back HTLCs we intend to relay for a random interval greater than this (see
-/// Event::PendingHTLCsForwardable for the API guidelines indicating how long should be waited).
-/// This provides some limited amount of privacy. Ideally this would range from somewhere like one
-/// second to 30 seconds, but people expect lightning to be, you know, kinda fast, sadly.
-pub(super) const MIN_HTLC_RELAY_HOLDING_CELL_MILLIS: u64 = 100;
-
 /// For events which result in both a RevokeAndACK and a CommitmentUpdate, by default they should
 /// be sent in the order they appear in the return value, however sometimes the order needs to be
 /// variable at runtime (eg FundedChannel::channel_reestablish needs to re-send messages in the order
@@ -6330,8 +6324,7 @@ where
 
 	/// Processes HTLCs which are pending waiting on random forward delay.
 	///
-	/// Should only really ever be called in response to a PendingHTLCsForwardable event.
-	/// Will likely generate further events.
+	/// Will regularly be called by the background processor.
 	pub fn process_pending_htlc_forwards(&self) {
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
 
@@ -7650,23 +7643,20 @@ where
 		&self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason,
 		destination: HTLCHandlingFailureType,
 	) {
-		let push_forward_event = self.fail_htlc_backwards_internal_without_forward_event(
+		self.fail_htlc_backwards_internal_without_forward_event(
 			source,
 			payment_hash,
 			onion_error,
 			destination,
 		);
-		if push_forward_event {
-			self.push_pending_forwards_ev();
-		}
 	}
 
 	/// Fails an HTLC backwards to the sender of it to us.
 	/// Note that we do not assume that channels corresponding to failed HTLCs are still available.
 	fn fail_htlc_backwards_internal_without_forward_event(
 		&self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason,
 		failure_type: HTLCHandlingFailureType,
-	) -> bool {
+	) {
 		// Ensure that no peer state channel storage lock is held when calling this function.
 		// This ensures that future code doesn't introduce a lock-order requirement for
 		// `forward_htlcs` to be locked after the `per_peer_state` peer locks, which calling
@@ -7684,10 +7674,9 @@ where
 		// Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
 		// from block_connected which may run during initialization prior to the chain_monitor
 		// being fully configured. See the docs for `ChannelManagerReadArgs` for more.
-		let mut push_forward_event;
 		match source {
 			HTLCSource::OutboundRoute { ref path, ref session_priv, ref payment_id, .. } => {
-				push_forward_event = self.pending_outbound_payments.fail_htlc(
+				self.pending_outbound_payments.fail_htlc(
 					source,
 					payment_hash,
 					onion_error,
@@ -7743,9 +7732,7 @@ where
 					},
 				};
 
-				push_forward_event = self.decode_update_add_htlcs.lock().unwrap().is_empty();
 				let mut forward_htlcs = self.forward_htlcs.lock().unwrap();
-				push_forward_event &= forward_htlcs.is_empty();
 				match forward_htlcs.entry(*short_channel_id) {
 					hash_map::Entry::Occupied(mut entry) => {
 						entry.get_mut().push(failure);
@@ -7766,7 +7753,6 @@ where
 				));
 			},
 		}
-		push_forward_event
 	}
 
 	/// Provides a payment preimage in response to [`Event::PaymentClaimable`], generating any
@@ -10046,9 +10032,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 	}
 
 	fn push_decode_update_add_htlcs(&self, mut update_add_htlcs: (u64, Vec<msgs::UpdateAddHTLC>)) {
-		let mut push_forward_event = self.forward_htlcs.lock().unwrap().is_empty();
 		let mut decode_update_add_htlcs = self.decode_update_add_htlcs.lock().unwrap();
-		push_forward_event &= decode_update_add_htlcs.is_empty();
 		let scid = update_add_htlcs.0;
 		match decode_update_add_htlcs.entry(scid) {
 			hash_map::Entry::Occupied(mut e) => {
@@ -10058,25 +10042,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 				e.insert(update_add_htlcs.1);
 			},
 		}
-		if push_forward_event {
-			self.push_pending_forwards_ev();
-		}
 	}
 
 	#[inline]
 	fn forward_htlcs(&self, per_source_pending_forwards: &mut [PerSourcePendingForward]) {
-		let push_forward_event =
-			self.forward_htlcs_without_forward_event(per_source_pending_forwards);
-		if push_forward_event {
-			self.push_pending_forwards_ev()
-		}
+		self.forward_htlcs_without_forward_event(per_source_pending_forwards);
 	}
 
 	#[inline]
 	fn forward_htlcs_without_forward_event(
 		&self, per_source_pending_forwards: &mut [PerSourcePendingForward],
-	) -> bool {
-		let mut push_forward_event = false;
+	) {
 		for &mut (
 			prev_short_channel_id,
 			prev_counterparty_node_id,
@@ -10099,10 +10075,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 					// Pull this now to avoid introducing a lock order with `forward_htlcs`.
 					let is_our_scid = self.short_to_chan_info.read().unwrap().contains_key(&scid);
 
-					let decode_update_add_htlcs_empty =
-						self.decode_update_add_htlcs.lock().unwrap().is_empty();
 					let mut forward_htlcs = self.forward_htlcs.lock().unwrap();
-					let forward_htlcs_empty = forward_htlcs.is_empty();
 					match forward_htlcs.entry(scid) {
 						hash_map::Entry::Occupied(mut entry) => {
 							entry.get_mut().push(HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo {
@@ -10202,10 +10175,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 									},
 								}
 							} else {
-								// We don't want to generate a PendingHTLCsForwardable event if only intercepted
-								// payments are being processed.
-								push_forward_event |=
-									forward_htlcs_empty && decode_update_add_htlcs_empty;
 								entry.insert(vec![HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo {
 									prev_short_channel_id,
 									prev_counterparty_node_id,
@@ -10224,7 +10193,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 			for (htlc_source, payment_hash, failure_reason, destination) in
 				failed_intercept_forwards.drain(..)
 			{
-				push_forward_event |= self.fail_htlc_backwards_internal_without_forward_event(
+				self.fail_htlc_backwards_internal_without_forward_event(
 					&htlc_source,
 					&payment_hash,
 					&failure_reason,
@@ -10237,30 +10206,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 				events.append(&mut new_intercept_events);
 			}
 		}
-		push_forward_event
-	}
-
-	fn push_pending_forwards_ev(&self) {
-		let mut pending_events = self.pending_events.lock().unwrap();
-		let is_processing_events = self.pending_events_processor.load(Ordering::Acquire);
-		let num_forward_events = pending_events
-			.iter()
-			.filter(|(ev, _)| matches!(ev, events::Event::PendingHTLCsForwardable { .. }))
-			.count();
-		// We only want to push a PendingHTLCsForwardable event if no others are queued. Processing
-		// events is done in batches and they are not removed until we're done processing each
-		// batch. Since handling a `PendingHTLCsForwardable` event will call back into the
-		// `ChannelManager`, we'll still see the original forwarding event not removed. Phantom
-		// payments will need an additional forwarding event before being claimed to make them look
-		// real by taking more time.
-		if (is_processing_events && num_forward_events <= 1) || num_forward_events < 1 {
-			pending_events.push_back((
-				Event::PendingHTLCsForwardable {
-					time_forwardable: Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS),
-				},
-				None,
-			));
-		}
 	}
 
 	/// Checks whether [`ChannelMonitorUpdate`]s generated by the receipt of a remote
@@ -10883,15 +10828,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		has_pending_monitor_events
 	}
 
-	/// In chanmon_consistency_target, we'd like to be able to restore monitor updating without
-	/// handling all pending events (i.e. not PendingHTLCsForwardable). Thus, we expose monitor
-	/// update events as a separate process method here.
-	#[cfg(fuzzing)]
-	pub fn process_monitor_events(&self) {
-		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-		self.process_pending_monitor_events();
-	}
-
 	/// Check the holding cell in each channel and free any pending HTLCs in them if possible.
 	/// Returns whether there were any updates such as if pending HTLCs were freed or a monitor
 	/// update was applied.
@@ -16226,21 +16162,6 @@ where
 			}
 		}
 
-		if !forward_htlcs.is_empty()
-			|| !decode_update_add_htlcs.is_empty()
-			|| pending_outbounds.needs_abandon()
-		{
-			// If we have pending HTLCs to forward, assume we either dropped a
-			// `PendingHTLCsForwardable` or the user received it but never processed it as they
-			// shut down before the timer hit. Either way, set the time_forwardable to a small
-			// constant as enough time has likely passed that we should simply handle the forwards
-			// now, or at least after the user gets a chance to reconnect to our peers.
-			pending_events_read.push_back((
-				events::Event::PendingHTLCsForwardable { time_forwardable: Duration::from_secs(2) },
-				None,
-			));
-		}
-
 		let expanded_inbound_key = args.node_signer.get_inbound_payment_key();
 
 		let mut claimable_payments = hash_map_with_capacity(claimable_htlcs_list.len());