f - use system/block time instead of ticks for removing stale AwaitingInvoice

Author: jkczyz

lightning/src/ln/channelmanager.rs

@@ -5017,7 +5017,18 @@ where
 				self.finish_close_channel(shutdown_res);
 			}
 
-			self.pending_outbound_payments.remove_stale_payments(&self.pending_events);
+			#[cfg(feature = "std")]
+			let duration_since_epoch = std::time::SystemTime::now()
+				.duration_since(std::time::SystemTime::UNIX_EPOCH)
+				.expect("SystemTime::now() should come after SystemTime::UNIX_EPOCH");
+			#[cfg(not(feature = "std"))]
+			let duration_since_epoch = Duration::from_secs(
+				self.highest_seen_timestamp.load(Ordering::Acquire).saturating_sub(7200) as u64
+			);
+
+			self.pending_outbound_payments.remove_stale_payments(
+				duration_since_epoch, &self.pending_events
+			);
 
 			// Technically we don't need to do this here, but if we have holding cell entries in a
 			// channel that need freeing, it's better to do that here and block a background task

lightning/src/ln/outbound_payment.rs

@@ -29,6 +29,7 @@ use crate::util::ser::ReadableArgs;
 
 use core::fmt::{self, Display, Formatter};
 use core::ops::Deref;
+use core::time::Duration;
 
 use crate::prelude::*;
 use crate::sync::Mutex;
@@ -39,20 +40,14 @@ use crate::sync::Mutex;
 /// [`ChannelManager::timer_tick_occurred`]: crate::ln::channelmanager::ChannelManager::timer_tick_occurred
 pub(crate) const IDEMPOTENCY_TIMEOUT_TICKS: u8 = 7;
 
-/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until an invoice request without
-/// a response is timed out.
-///
-/// [`ChannelManager::timer_tick_occurred`]: crate::ln::channelmanager::ChannelManager::timer_tick_occurred
-const INVOICE_REQUEST_TIMEOUT_TICKS: Option<u64> = Some(3);
-
 /// Stores the session_priv for each part of a payment that is still pending. For versions 0.0.102
 /// and later, also stores information for retrying the payment.
 pub(crate) enum PendingOutboundPayment {
 	Legacy {
 		session_privs: HashSet<[u8; 32]>,
 	},
 	AwaitingInvoice {
-		timer_ticks_without_response_remaining: Option<u64>,
+		absolute_expiry: Duration,
 		retry_strategy: Retry,
 		max_total_routing_fee_msat: Option<u64>,
 	},
@@ -1273,36 +1268,17 @@ impl OutboundPayments {
 		(payment, onion_session_privs)
 	}
 
-	#[allow(unused)]
-	pub(super) fn add_new_awaiting_invoice_for_offer(
-		&self, payment_id: PaymentId, retry_strategy: Retry, max_total_routing_fee_msat: Option<u64>
-	) -> Result<(), ()> {
-		self.add_new_awaiting_invoice(
-			payment_id, retry_strategy, max_total_routing_fee_msat, INVOICE_REQUEST_TIMEOUT_TICKS
-		)
-	}
-
-	#[allow(unused)]
-	pub(super) fn add_new_awaiting_invoice_for_refund(
-		&self, payment_id: PaymentId, retry_strategy: Retry,
-		max_total_routing_fee_msat: Option<u64>, timer_ticks_before_expiration: Option<u64>
-	) -> Result<(), ()> {
-		self.add_new_awaiting_invoice(
-			payment_id, retry_strategy, max_total_routing_fee_msat, timer_ticks_before_expiration
-		)
-	}
-
 	#[allow(unused)]
 	fn add_new_awaiting_invoice(
-		&self, payment_id: PaymentId, retry_strategy: Retry,
-		max_total_routing_fee_msat: Option<u64>, timer_ticks_before_expiration: Option<u64>
+		&self, payment_id: PaymentId, absolute_expiry: Duration, retry_strategy: Retry,
+		max_total_routing_fee_msat: Option<u64>
 	) -> Result<(), ()> {
 		let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
 		match pending_outbounds.entry(payment_id) {
 			hash_map::Entry::Occupied(_) => Err(()),
 			hash_map::Entry::Vacant(entry) => {
 				entry.insert(PendingOutboundPayment::AwaitingInvoice {
-					timer_ticks_without_response_remaining: timer_ticks_before_expiration,
+					absolute_expiry,
 					retry_strategy,
 					max_total_routing_fee_msat,
 				});
@@ -1531,22 +1507,23 @@ impl OutboundPayments {
 	}
 
 	pub(super) fn remove_stale_payments(
-		&self, pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>)
+		&self, duration_since_epoch: Duration,
+		pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>)
 	{
 		let mut pending_outbound_payments = self.pending_outbound_payments.lock().unwrap();
 		#[cfg(not(invreqfailed))]
 		let pending_events = pending_events.lock().unwrap();
 		#[cfg(invreqfailed)]
 		let mut pending_events = pending_events.lock().unwrap();
-		pending_outbound_payments.retain(|payment_id, payment| {
+		pending_outbound_payments.retain(|payment_id, payment| match payment {
 			// If an outbound payment was completed, and no pending HTLCs remain, we should remove it
 			// from the map. However, if we did that immediately when the last payment HTLC is claimed,
 			// this could race the user making a duplicate send_payment call and our idempotency
 			// guarantees would be violated. Instead, we wait a few timer ticks to do the actual
 			// removal. This should be more than sufficient to ensure the idempotency of any
 			// `send_payment` calls that were made at the same time the `PaymentSent` event was being
 			// processed.
-			if let PendingOutboundPayment::Fulfilled { session_privs, timer_ticks_without_htlcs, .. } = payment {
+			PendingOutboundPayment::Fulfilled { session_privs, timer_ticks_without_htlcs, .. } => {
 				let mut no_remaining_entries = session_privs.is_empty();
 				if no_remaining_entries {
 					for (ev, _) in pending_events.iter() {
@@ -1570,11 +1547,9 @@ impl OutboundPayments {
 					*timer_ticks_without_htlcs = 0;
 					true
 				}
-			} else if let PendingOutboundPayment::AwaitingInvoice {
-				timer_ticks_without_response_remaining: Some(timer_ticks_remaining), ..
-			} = payment {
-				if *timer_ticks_remaining > 0 {
-					*timer_ticks_remaining -= 1;
+			},
+			PendingOutboundPayment::AwaitingInvoice { absolute_expiry, ..  } => {
+				if duration_since_epoch < *absolute_expiry {
 					true
 				} else {
 					#[cfg(invreqfailed)]
@@ -1583,7 +1558,8 @@ impl OutboundPayments {
 					);
 					false
 				}
-			} else { true }
+			},
+			_ => true,
 		});
 	}
 
@@ -1800,7 +1776,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
 		(2, payment_hash, required),
 	},
 	(5, AwaitingInvoice) => {
-		(0, timer_ticks_without_response_remaining, option),
+		(0, absolute_expiry, required),
 		(2, retry_strategy, required),
 		(4, max_total_routing_fee_msat, option),
 	},
@@ -1816,14 +1792,14 @@ mod tests {
 	use bitcoin::network::constants::Network;
 	use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};
 
+	use core::time::Duration;
+
 	use crate::events::{Event, PathFailure, PaymentFailureReason};
 	use crate::ln::PaymentHash;
 	use crate::ln::channelmanager::{PaymentId, RecipientOnionFields};
 	use crate::ln::features::{ChannelFeatures, NodeFeatures};
 	use crate::ln::msgs::{ErrorAction, LightningError};
 	use crate::ln::outbound_payment::{Bolt12PaymentError, OutboundPayments, Retry, RetryableSendFailure};
-	#[cfg(invreqfailed)]
-	use crate::ln::outbound_payment::INVOICE_REQUEST_TIMEOUT_TICKS;
 	use crate::offers::invoice::DEFAULT_RELATIVE_EXPIRY;
 	use crate::offers::offer::OfferBuilder;
 	use crate::offers::test_utils::*;
@@ -2033,22 +2009,28 @@ mod tests {
 		let pending_events = Mutex::new(VecDeque::new());
 		let outbound_payments = OutboundPayments::new();
 		let payment_id = PaymentId([0; 32]);
+		let absolute_expiry = 100;
+		let tick_interval = 10;
 
 		assert!(!outbound_payments.has_pending_payments());
 		assert!(
 			outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), None, INVOICE_REQUEST_TIMEOUT_TICKS
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), None
 			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());
 
-		for _ in 0..INVOICE_REQUEST_TIMEOUT_TICKS.unwrap() {
-			outbound_payments.remove_stale_payments(&pending_events);
+		for seconds_since_epoch in (0..absolute_expiry).step_by(tick_interval) {
+			let duration_since_epoch = Duration::from_secs(seconds_since_epoch);
+			outbound_payments.remove_stale_payments(duration_since_epoch, &pending_events);
+
 			assert!(outbound_payments.has_pending_payments());
 			assert!(pending_events.lock().unwrap().is_empty());
 		}
 
-		outbound_payments.remove_stale_payments(&pending_events);
+		let duration_since_epoch = Duration::from_secs(absolute_expiry);
+		outbound_payments.remove_stale_payments(duration_since_epoch, &pending_events);
+
 		assert!(!outbound_payments.has_pending_payments());
 		assert!(!pending_events.lock().unwrap().is_empty());
 		assert_eq!(
@@ -2059,14 +2041,14 @@ mod tests {
 
 		assert!(
 			outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), None, INVOICE_REQUEST_TIMEOUT_TICKS
+				payment_id, Duration::from_secs(absolute_expiry + 1), Retry::Attempts(0), None
 			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());
 
 		assert!(
 			outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), None, INVOICE_REQUEST_TIMEOUT_TICKS
+				payment_id, Duration::from_secs(absolute_expiry + 1), Retry::Attempts(0), None
 			).is_err()
 		);
 	}
@@ -2077,11 +2059,12 @@ mod tests {
 		let pending_events = Mutex::new(VecDeque::new());
 		let outbound_payments = OutboundPayments::new();
 		let payment_id = PaymentId([0; 32]);
+		let absolute_expiry = 100;
 
 		assert!(!outbound_payments.has_pending_payments());
 		assert!(
 			outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), None, INVOICE_REQUEST_TIMEOUT_TICKS
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), None
 			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());
@@ -2110,10 +2093,11 @@ mod tests {
 		let pending_events = Mutex::new(VecDeque::new());
 		let outbound_payments = OutboundPayments::new();
 		let payment_id = PaymentId([0; 32]);
+		let absolute_expiry = 100;
 
 		assert!(
 			outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), None, INVOICE_REQUEST_TIMEOUT_TICKS
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), None
 			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());
@@ -2160,6 +2144,7 @@ mod tests {
 		let pending_events = Mutex::new(VecDeque::new());
 		let outbound_payments = OutboundPayments::new();
 		let payment_id = PaymentId([0; 32]);
+		let absolute_expiry = 100;
 
 		let invoice = OfferBuilder::new("foo".into(), recipient_pubkey())
 			.amount_msats(1000)
@@ -2173,8 +2158,8 @@ mod tests {
 
 		assert!(
 			outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000),
-				INVOICE_REQUEST_TIMEOUT_TICKS
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0),
+				Some(invoice.amount_msats() / 100 + 50_000)
 			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());
@@ -2218,6 +2203,7 @@ mod tests {
 		let pending_events = Mutex::new(VecDeque::new());
 		let outbound_payments = OutboundPayments::new();
 		let payment_id = PaymentId([0; 32]);
+		let absolute_expiry = 100;
 
 		let invoice = OfferBuilder::new("foo".into(), recipient_pubkey())
 			.amount_msats(1000)
@@ -2231,8 +2217,8 @@ mod tests {
 
 		assert!(
 			outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000),
-				INVOICE_REQUEST_TIMEOUT_TICKS
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0),
+				Some(invoice.amount_msats() / 100 + 50_000)
 			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());
@@ -2276,6 +2262,7 @@ mod tests {
 		let pending_events = Mutex::new(VecDeque::new());
 		let outbound_payments = OutboundPayments::new();
 		let payment_id = PaymentId([0; 32]);
+		let absolute_expiry = 100;
 
 		let invoice = OfferBuilder::new("foo".into(), recipient_pubkey())
 			.amount_msats(1000)
@@ -2328,7 +2315,7 @@ mod tests {
 
 		assert!(
 			outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), Some(1234), INVOICE_REQUEST_TIMEOUT_TICKS
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), Some(1234)
 			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());