Await for invoices using an absolute expiry

PendingOutboundPayment::AwaitingInvoice counts the number of timer ticks
that have passed awaiting a Bolt12Invoice for an InvoiceRequest. When a
constant INVOICE_REQUEST_TIMEOUT_TICKS has passed, the payment is
forgotten. However, this mechanism is insufficient for the Refund
scenario, where the Refund's expiration should be used instead.

Change AwaitingInvoice to store an absolute expiry instead. When
removing stale payments, pass the `SystemTime` in `std` and the highest
block time minus two hours in `no-std`.

Author: jkczyz

lightning/src/ln/channelmanager.rs

@@ -4791,6 +4791,10 @@ where
 	///    with the current [`ChannelConfig`].
 	///  * Removing peers which have disconnected but and no longer have any channels.
 	///  * Force-closing and removing channels which have not completed establishment in a timely manner.
+	///  * Forgetting about stale outbound payment, either those that have already been fulfilled or
+	///    those awaiting an invoice that hasn't been delivered in the necessary amount of time. The
+	///    latter is determined using the system clock in `std` and the block time minus two hours
+	///    in `no-std`.
 	///
 	/// Note that this may cause reentrancy through [`chain::Watch::update_channel`] calls or feerate
 	/// estimate fetches.
@@ -5019,7 +5023,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: u8 = 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: u8,
+		absolute_expiry: Duration,
 		retry_strategy: Retry,
 		max_total_routing_fee_msat: Option<u64>,
 	},
@@ -1274,15 +1269,16 @@ impl OutboundPayments {
 	}
 
 	#[allow(unused)]
-	pub(super) fn add_new_awaiting_invoice(
-		&self, payment_id: PaymentId, retry_strategy: Retry, max_total_routing_fee_msat: Option<u64>
+	fn add_new_awaiting_invoice(
+		&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: 0,
+					absolute_expiry,
 					retry_strategy,
 					max_total_routing_fee_msat,
 				});
@@ -1511,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() {
@@ -1550,9 +1547,9 @@ impl OutboundPayments {
 					*timer_ticks_without_htlcs = 0;
 					true
 				}
-			} else if let PendingOutboundPayment::AwaitingInvoice { timer_ticks_without_response, .. } = payment {
-				*timer_ticks_without_response += 1;
-				if *timer_ticks_without_response <= INVOICE_REQUEST_TIMEOUT_TICKS {
+			},
+			PendingOutboundPayment::AwaitingInvoice { absolute_expiry, ..  } => {
+				if duration_since_epoch < *absolute_expiry {
 					true
 				} else {
 					#[cfg(invreqfailed)]
@@ -1561,7 +1558,8 @@ impl OutboundPayments {
 					);
 					false
 				}
-			} else { true }
+			},
+			_ => true,
 		});
 	}
 
@@ -1778,7 +1776,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
 		(2, payment_hash, required),
 	},
 	(5, AwaitingInvoice) => {
-		(0, timer_ticks_without_response, required),
+		(0, absolute_expiry, required),
 		(2, retry_strategy, required),
 		(4, max_total_routing_fee_msat, option),
 	},
@@ -1794,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::*;
@@ -2011,20 +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).is_ok()
+			outbound_payments.add_new_awaiting_invoice(
+				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 {
-			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!(
@@ -2034,13 +2040,16 @@ mod tests {
 		assert!(pending_events.lock().unwrap().is_empty());
 
 		assert!(
-			outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok()
+			outbound_payments.add_new_awaiting_invoice(
+				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)
-				.is_err()
+			outbound_payments.add_new_awaiting_invoice(
+				payment_id, Duration::from_secs(absolute_expiry + 1), Retry::Attempts(0), None
+			).is_err()
 		);
 	}
 
@@ -2050,10 +2059,13 @@ 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).is_ok()
+			outbound_payments.add_new_awaiting_invoice(
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), None
+			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());
 
@@ -2081,9 +2093,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.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok()
+			outbound_payments.add_new_awaiting_invoice(
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), None
+			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());
 
@@ -2129,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)
@@ -2140,9 +2156,11 @@ mod tests {
 			.build().unwrap()
 			.sign(recipient_sign).unwrap();
 
-		assert!(outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000))
-			.is_ok()
+		assert!(
+			outbound_payments.add_new_awaiting_invoice(
+				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());
 
@@ -2185,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)
@@ -2196,9 +2215,11 @@ mod tests {
 			.build().unwrap()
 			.sign(recipient_sign).unwrap();
 
-		assert!(outbound_payments.add_new_awaiting_invoice(
-				payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000))
-			.is_ok()
+		assert!(
+			outbound_payments.add_new_awaiting_invoice(
+				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());
 
@@ -2241,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)
@@ -2292,7 +2314,9 @@ mod tests {
 		assert!(pending_events.lock().unwrap().is_empty());
 
 		assert!(
-			outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), Some(1234)).is_ok()
+			outbound_payments.add_new_awaiting_invoice(
+				payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), Some(1234)
+			).is_ok()
 		);
 		assert!(outbound_payments.has_pending_payments());