WIP 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::*;
@@ -1174,21 +1173,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`].
@@ -1721,11 +1705,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

@@ -2011,23 +2011,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);
 
@@ -2810,12 +2806,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

@@ -960,12 +960,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
@@ -6315,8 +6309,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);
 
@@ -7721,23 +7714,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
@@ -7755,10 +7745,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,
@@ -7814,9 +7803,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);
@@ -7837,7 +7824,6 @@ where
 				));
 			},
 		}
-		push_forward_event
 	}
 
 	/// Provides a payment preimage in response to [`Event::PaymentClaimable`], generating any
@@ -9978,9 +9964,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) => {
@@ -9990,25 +9974,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,
@@ -10031,10 +10007,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 {
@@ -10132,8 +10105,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,
@@ -10152,7 +10123,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,
@@ -10165,30 +10136,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
@@ -15951,21 +15898,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());

lightning/src/ln/functional_test_utils.rs

@@ -1983,7 +1983,6 @@ macro_rules! expect_htlc_handling_failed_destinations {
 		let mut num_expected_failures = $expected_failures.len();
 		for event in $events {
 			match event {
-				$crate::events::Event::PendingHTLCsForwardable { .. } => { },
 				$crate::events::Event::HTLCHandlingFailed { ref failure_type, .. } => {
 					assert!($expected_failures.contains(&failure_type));
 					num_expected_failures -= 1;
@@ -1999,9 +1998,7 @@ macro_rules! expect_htlc_handling_failed_destinations {
 /// there are any [`Event::HTLCHandlingFailed`] events their [`HTLCHandlingFailureType`] is included in the
 /// `expected_failures` set.
 pub fn expect_pending_htlcs_forwardable_conditions(events: Vec<Event>, expected_failures: &[HTLCHandlingFailureType]) {
-	let count = expected_failures.len() + 1;
-	assert_eq!(events.len(), count);
-	assert!(events.iter().find(|event| matches!(event, Event::PendingHTLCsForwardable { .. })).is_some());
+	assert_eq!(events.len(), expected_failures.len());
 	if expected_failures.len() > 0 {
 		expect_htlc_handling_failed_destinations!(events, expected_failures)
 	}
@@ -2052,23 +2049,6 @@ macro_rules! expect_pending_htlcs_forwardable_and_htlc_handling_failed {
 	}}
 }
 
-#[cfg(any(test, feature = "_externalize_tests"))]
-macro_rules! expect_pending_htlcs_forwardable_from_events {
-	($node: expr, $events: expr, $ignore: expr) => {{
-		assert_eq!($events.len(), 1);
-		match $events[0] {
-			Event::PendingHTLCsForwardable { .. } => { },
-			_ => panic!("Unexpected event"),
-		};
-		if $ignore {
-			$node.node.process_pending_htlc_forwards();
-
-			// Ensure process_pending_htlc_forwards is idempotent.
-			$node.node.process_pending_htlc_forwards();
-		}
-	}}
-}
-
 #[macro_export]
 /// Performs the "commitment signed dance" - the series of message exchanges which occur after a
 /// commitment update.
@@ -2645,11 +2625,6 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 			}
 			_ => panic!("Unexpected second event"),
 		}
-	} else if conditions.retry_expected {
-		match &payment_failed_events[1] {
-			Event::PendingHTLCsForwardable { .. } => {},
-			_ => panic!("Unexpected second event"),
-		}
 	}
 }
 
@@ -2830,11 +2805,7 @@ pub fn do_pass_along_path<'a, 'b, 'c>(args: PassAlongPathArgs) -> Option<Event>
 				}
 				event = Some(events_2[0].clone());
 			} else if let Some(ref failure) = expected_failure {
-				// If we successfully decode the HTLC onion but then fail later in
-				// process_pending_htlc_forwards, then we'll queue the failure and generate a new
-				// `ProcessPendingHTLCForwards` event. If we fail during the process of decoding the HTLC,
-				// we'll fail it immediately with no intermediate forwarding event.
-				assert!(events_2.len() == 1 || events_2.len() == 2);
+				assert!(events_2.len() == 1);
 				expect_htlc_handling_failed_destinations!(events_2, &[failure]);
 				node.node.process_pending_htlc_forwards();
 				check_added_monitors!(node, 1);