Add PendingOutboundPayment::AwaitingInvoice

When a BOLT 12 invoice has been requested, in order to guarantee
invoice payment idempotency the future payment needs to be tracked. Add
an AwaitingInvoice variant to PendingOutboundPayment such that only
requested invoices are paid only once. Timeout after a few timer ticks
if a request has not been received.

Author: jkczyz

lightning/src/ln/channelmanager.rs

@@ -1268,9 +1268,13 @@ const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_G
 /// The number of ticks of [`ChannelManager::timer_tick_occurred`] until expiry of incomplete MPPs
 pub(crate) const MPP_TIMEOUT_TICKS: u8 = 3;
 
+/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until an invoice request without
+/// a response is timed out.
+pub(crate) const INVOICE_REQUEST_TIMEOUT_TICKS: u8 = 3;
+
 /// The number of ticks of [`ChannelManager::timer_tick_occurred`] until we time-out the
 /// idempotency of payments by [`PaymentId`]. See
-/// [`OutboundPayments::remove_stale_resolved_payments`].
+/// [`OutboundPayments::remove_stale_payments`].
 pub(crate) const IDEMPOTENCY_TIMEOUT_TICKS: u8 = 7;
 
 /// The number of ticks of [`ChannelManager::timer_tick_occurred`] where a peer is disconnected
@@ -1615,6 +1619,11 @@ pub enum ChannelShutdownState {
 /// These include payments that have yet to find a successful path, or have unresolved HTLCs.
 #[derive(Debug, PartialEq)]
 pub enum RecentPaymentDetails {
+	/// When an invoice was requested but not yet received, and thus a payment has not been sent.
+	AwaitingInvoice {
+		/// Identifier for the payment to ensure idempotency.
+		payment_id: PaymentId,
+	},
 	/// When a payment is still being sent and awaiting successful delivery.
 	Pending {
 		/// Hash of the payment that is currently being sent but has yet to be fulfilled or
@@ -2344,7 +2353,10 @@ where
 	/// [`Event::PaymentSent`]: events::Event::PaymentSent
 	pub fn list_recent_payments(&self) -> Vec<RecentPaymentDetails> {
 		self.pending_outbound_payments.pending_outbound_payments.lock().unwrap().iter()
-			.filter_map(|(_, pending_outbound_payment)| match pending_outbound_payment {
+			.filter_map(|(payment_id, pending_outbound_payment)| match pending_outbound_payment {
+				PendingOutboundPayment::AwaitingInvoice { .. } => {
+					Some(RecentPaymentDetails::AwaitingInvoice { payment_id: *payment_id })
+				},
 				PendingOutboundPayment::Retryable { payment_hash, total_msat, .. } => {
 					Some(RecentPaymentDetails::Pending {
 						payment_hash: *payment_hash,
@@ -4516,7 +4528,7 @@ where
 				let _ = handle_error!(self, err, counterparty_node_id);
 			}
 
-			self.pending_outbound_payments.remove_stale_resolved_payments(&self.pending_events);
+			self.pending_outbound_payments.remove_stale_payments(&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
@@ -8057,6 +8069,7 @@ where
 						session_priv.write(writer)?;
 					}
 				}
+				PendingOutboundPayment::AwaitingInvoice { .. } => {},
 				PendingOutboundPayment::Fulfilled { .. } => {},
 				PendingOutboundPayment::Abandoned { .. } => {},
 			}

lightning/src/ln/outbound_payment.rs

@@ -16,7 +16,7 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
 use crate::sign::{EntropySource, NodeSigner, Recipient};
 use crate::events::{self, PaymentFailureReason};
 use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
-use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId};
+use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, INVOICE_REQUEST_TIMEOUT_TICKS, PaymentId};
 use crate::ln::onion_utils::HTLCFailReason;
 use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router};
 use crate::util::errors::APIError;
@@ -38,6 +38,9 @@ pub(crate) enum PendingOutboundPayment {
 	Legacy {
 		session_privs: HashSet<[u8; 32]>,
 	},
+	AwaitingInvoice {
+		timer_ticks_without_response: u8,
+	},
 	Retryable {
 		retry_strategy: Option<Retry>,
 		attempts: PaymentAttempts,
@@ -107,6 +110,12 @@ impl PendingOutboundPayment {
 			params.previously_failed_channels.push(scid);
 		}
 	}
+	fn is_awaiting_invoice(&self) -> bool {
+		match self {
+			PendingOutboundPayment::AwaitingInvoice { .. } => true,
+			_ => false,
+		}
+	}
 	pub(super) fn is_fulfilled(&self) -> bool {
 		match self {
 			PendingOutboundPayment::Fulfilled { .. } => true,
@@ -129,6 +138,7 @@ impl PendingOutboundPayment {
 	fn payment_hash(&self) -> Option<PaymentHash> {
 		match self {
 			PendingOutboundPayment::Legacy { .. } => None,
+			PendingOutboundPayment::AwaitingInvoice { .. } => None,
 			PendingOutboundPayment::Retryable { payment_hash, .. } => Some(*payment_hash),
 			PendingOutboundPayment::Fulfilled { payment_hash, .. } => *payment_hash,
 			PendingOutboundPayment::Abandoned { payment_hash, .. } => Some(*payment_hash),
@@ -141,8 +151,8 @@ impl PendingOutboundPayment {
 			PendingOutboundPayment::Legacy { session_privs } |
 				PendingOutboundPayment::Retryable { session_privs, .. } |
 				PendingOutboundPayment::Fulfilled { session_privs, .. } |
-				PendingOutboundPayment::Abandoned { session_privs, .. }
-			=> session_privs,
+				PendingOutboundPayment::Abandoned { session_privs, .. } => session_privs,
+			PendingOutboundPayment::AwaitingInvoice { .. } => return,
 		});
 		let payment_hash = self.payment_hash();
 		*self = PendingOutboundPayment::Fulfilled { session_privs, payment_hash, timer_ticks_without_htlcs: 0 };
@@ -168,7 +178,8 @@ impl PendingOutboundPayment {
 				PendingOutboundPayment::Fulfilled { session_privs, .. } |
 				PendingOutboundPayment::Abandoned { session_privs, .. } => {
 					session_privs.remove(session_priv)
-				}
+				},
+			PendingOutboundPayment::AwaitingInvoice { .. } => false,
 		};
 		if remove_res {
 			if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
@@ -188,6 +199,7 @@ impl PendingOutboundPayment {
 				PendingOutboundPayment::Retryable { session_privs, .. } => {
 					session_privs.insert(session_priv)
 				}
+			PendingOutboundPayment::AwaitingInvoice { .. } => false,
 			PendingOutboundPayment::Fulfilled { .. } => false,
 			PendingOutboundPayment::Abandoned { .. } => false,
 		};
@@ -209,7 +221,8 @@ impl PendingOutboundPayment {
 				PendingOutboundPayment::Fulfilled { session_privs, .. } |
 				PendingOutboundPayment::Abandoned { session_privs, .. } => {
 					session_privs.len()
-				}
+				},
+			PendingOutboundPayment::AwaitingInvoice { .. } => 0,
 		}
 	}
 }
@@ -626,7 +639,7 @@ impl OutboundPayments {
 		let mut outbounds = self.pending_outbound_payments.lock().unwrap();
 		outbounds.retain(|pmt_id, pmt| {
 			let mut retain = true;
-			if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 {
+			if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_awaiting_invoice() {
 				pmt.mark_abandoned(PaymentFailureReason::RetriesExhausted);
 				if let PendingOutboundPayment::Abandoned { payment_hash, reason, .. } = pmt {
 					pending_events.lock().unwrap().push_back((events::Event::PaymentFailed {
@@ -644,7 +657,8 @@ impl OutboundPayments {
 	pub(super) fn needs_abandon(&self) -> bool {
 		let outbounds = self.pending_outbound_payments.lock().unwrap();
 		outbounds.iter().any(|(_, pmt)|
-			!pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_fulfilled())
+			!pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_fulfilled() &&
+			!pmt.is_awaiting_invoice())
 	}
 
 	/// Errors immediately on [`RetryableSendFailure`] error conditions. Otherwise, further errors may
@@ -779,6 +793,10 @@ impl OutboundPayments {
 							log_error!(logger, "Unable to retry payments that were initially sent on LDK versions prior to 0.0.102");
 							return
 						},
+						PendingOutboundPayment::AwaitingInvoice { .. } => {
+							log_error!(logger, "Payment not yet sent");
+							return
+						},
 						PendingOutboundPayment::Fulfilled { .. } => {
 							log_error!(logger, "Payment already completed");
 							return
@@ -951,37 +969,44 @@ impl OutboundPayments {
 		keysend_preimage: Option<PaymentPreimage>, route: &Route, retry_strategy: Option<Retry>,
 		payment_params: Option<PaymentParameters>, entropy_source: &ES, best_block_height: u32
 	) -> Result<Vec<[u8; 32]>, PaymentSendFailure> where ES::Target: EntropySource {
+		let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
+		let entry = pending_outbounds.entry(payment_id);
+		if let hash_map::Entry::Occupied(entry) = &entry {
+			if let PendingOutboundPayment::AwaitingInvoice { .. } = entry.get() { } else {
+				return Err(PaymentSendFailure::DuplicatePayment)
+			}
+		}
+
 		let mut onion_session_privs = Vec::with_capacity(route.paths.len());
 		for _ in 0..route.paths.len() {
 			onion_session_privs.push(entropy_source.get_secure_random_bytes());
 		}
 
-		let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
-		match pending_outbounds.entry(payment_id) {
-			hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::DuplicatePayment),
-			hash_map::Entry::Vacant(entry) => {
-				let payment = entry.insert(PendingOutboundPayment::Retryable {
-					retry_strategy,
-					attempts: PaymentAttempts::new(),
-					payment_params,
-					session_privs: HashSet::new(),
-					pending_amt_msat: 0,
-					pending_fee_msat: Some(0),
-					payment_hash,
-					payment_secret: recipient_onion.payment_secret,
-					payment_metadata: recipient_onion.payment_metadata,
-					keysend_preimage,
-					starting_block_height: best_block_height,
-					total_msat: route.get_total_amount(),
-				});
-
-				for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
-					assert!(payment.insert(*session_priv_bytes, path));
-				}
+		let mut payment = PendingOutboundPayment::Retryable {
+			retry_strategy,
+			attempts: PaymentAttempts::new(),
+			payment_params,
+			session_privs: HashSet::new(),
+			pending_amt_msat: 0,
+			pending_fee_msat: Some(0),
+			payment_hash,
+			payment_secret: recipient_onion.payment_secret,
+			payment_metadata: recipient_onion.payment_metadata,
+			keysend_preimage,
+			starting_block_height: best_block_height,
+			total_msat: route.get_total_amount(),
+		};
 
-				Ok(onion_session_privs)
-			},
+		for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
+			assert!(payment.insert(*session_priv_bytes, path));
+		}
+
+		match entry {
+			hash_map::Entry::Occupied(mut entry) => { entry.insert(payment); },
+			hash_map::Entry::Vacant(entry) => { entry.insert(payment); },
 		}
+
+		Ok(onion_session_privs)
 	}
 
 	fn pay_route_internal<NS: Deref, F>(
@@ -1188,19 +1213,19 @@ impl OutboundPayments {
 		}
 	}
 
-	pub(super) fn remove_stale_resolved_payments(&self,
-		pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>)
+	pub(super) fn remove_stale_payments(
+		&self, pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>)
 	{
-		// 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.
 		let mut pending_outbound_payments = self.pending_outbound_payments.lock().unwrap();
-		let pending_events = pending_events.lock().unwrap();
+		let mut pending_events = pending_events.lock().unwrap();
 		pending_outbound_payments.retain(|payment_id, 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 {
 				let mut no_remaining_entries = session_privs.is_empty();
 				if no_remaining_entries {
@@ -1225,6 +1250,16 @@ 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 {
+					true
+				} else {
+					pending_events.push_back(
+						(events::Event::InvoiceRequestFailed { payment_id: *payment_id }, None)
+					);
+					false
+				}
 			} else { true }
 		});
 	}
@@ -1429,6 +1464,9 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
 		(1, reason, option),
 		(2, payment_hash, required),
 	},
+	(5, AwaitingInvoice) => {
+		(0, timer_ticks_without_response, required),
+	},
 );
 
 #[cfg(test)]