Merge pull request #3658 from shaavan/i3653

Add idempotency check to prevent duplicate `InvoiceReceived` events

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -12605,6 +12605,11 @@ where
 				);
 
 				if self.default_configuration.manually_handle_bolt12_invoices {
+					// Update the corresponding entry in `PendingOutboundPayment` for this invoice.
+					// This ensures that event generation remains idempotent in case we receive
+					// the same invoice multiple times.
+					self.pending_outbound_payments.mark_invoice_received(&invoice, payment_id).ok()?;
+
 					let event = Event::InvoiceReceived {
 						payment_id, invoice, context, responder,
 					};

lightning/src/ln/offers_tests.rs

@@ -1184,7 +1184,14 @@ fn pays_bolt12_invoice_asynchronously() {
 	let onion_message = alice.onion_messenger.next_onion_message_for_peer(bob_id).unwrap();
 	bob.onion_messenger.handle_onion_message(alice_id, &onion_message);
 
-	let (invoice, context) = match get_event!(bob, Event::InvoiceReceived) {
+	// Re-process the same onion message to ensure idempotency — 
+	// we should not generate a duplicate `InvoiceReceived` event.
+	bob.onion_messenger.handle_onion_message(alice_id, &onion_message);
+
+	let mut events = bob.node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+
+	let (invoice, context) = match events.pop().unwrap() {
 		Event::InvoiceReceived { payment_id: actual_payment_id, invoice, context, .. } => {
 			assert_eq!(actual_payment_id, payment_id);
 			(invoice, context)

lightning/src/ln/outbound_payment.rs

@@ -72,9 +72,9 @@ pub(crate) enum PendingOutboundPayment {
 		route_params_config: RouteParametersConfig,
 		retryable_invoice_request: Option<RetryableInvoiceRequest>
 	},
-	// This state will never be persisted to disk because we transition from `AwaitingInvoice` to
-	// `Retryable` atomically within the `ChannelManager::total_consistency_lock`. Useful to avoid
-	// holding the `OutboundPayments::pending_outbound_payments` lock during pathfinding.
+	// Represents the state after the invoice has been received, transitioning from the corresponding
+	// `AwaitingInvoice` state.
+	// Helps avoid holding the `OutboundPayments::pending_outbound_payments` lock during pathfinding.
 	InvoiceReceived {
 		payment_hash: PaymentHash,
 		retry_strategy: Retry,
@@ -931,26 +931,9 @@ impl OutboundPayments {
 		IH: Fn() -> InFlightHtlcs,
 		SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
 	{
-		let payment_hash = invoice.payment_hash();
-		let params_config;
-		let retry_strategy;
-		match self.pending_outbound_payments.lock().unwrap().entry(payment_id) {
-			hash_map::Entry::Occupied(entry) => match entry.get() {
-				PendingOutboundPayment::AwaitingInvoice {
-					retry_strategy: retry, route_params_config, ..
-				} => {
-					retry_strategy = *retry;
-					params_config = *route_params_config;
-					*entry.into_mut() = PendingOutboundPayment::InvoiceReceived {
-						payment_hash,
-						retry_strategy: *retry,
-						route_params_config: *route_params_config,
-					};
-				},
-				_ => return Err(Bolt12PaymentError::DuplicateInvoice),
-			},
-			hash_map::Entry::Vacant(_) => return Err(Bolt12PaymentError::UnexpectedInvoice),
-		}
+
+		let (payment_hash, retry_strategy, params_config, _) = self
+			.mark_invoice_received_and_get_details(invoice, payment_id)?;
 
 		if invoice.invoice_features().requires_unknown_bits_from(&features) {
 			self.abandon_payment(
@@ -1863,6 +1846,51 @@ impl OutboundPayments {
 		}
 	}
 
+	pub(super) fn mark_invoice_received(
+		&self, invoice: &Bolt12Invoice, payment_id: PaymentId
+	) -> Result<(), Bolt12PaymentError> {
+		self.mark_invoice_received_and_get_details(invoice, payment_id)
+			.and_then(|(_, _, _, is_newly_marked)| {
+				is_newly_marked
+					.then_some(())
+					.ok_or(Bolt12PaymentError::DuplicateInvoice)
+			})
+	}
+
+	fn mark_invoice_received_and_get_details(
+		&self, invoice: &Bolt12Invoice, payment_id: PaymentId
+	) -> Result<(PaymentHash, Retry, RouteParametersConfig, bool), Bolt12PaymentError> {
+		match self.pending_outbound_payments.lock().unwrap().entry(payment_id) {
+			hash_map::Entry::Occupied(entry) => match entry.get() {
+				PendingOutboundPayment::AwaitingInvoice {
+					retry_strategy: retry, route_params_config, ..
+				} => {
+					let payment_hash = invoice.payment_hash();
+					let retry = *retry;
+					let config = *route_params_config;
+					*entry.into_mut() = PendingOutboundPayment::InvoiceReceived {
+						payment_hash,
+						retry_strategy: retry,
+						route_params_config: config,
+					};
+
+					Ok((payment_hash, retry, config, true))
+				},
+				// When manual invoice handling is enabled, the corresponding `PendingOutboundPayment` entry
+				// is already updated at the time the invoice is received. This ensures that `InvoiceReceived`
+				// event generation remains idempotent, even if the same invoice is received again before the
+				// event is handled by the user.
+				PendingOutboundPayment::InvoiceReceived {
+					retry_strategy, route_params_config, ..
+				} => {
+					Ok((invoice.payment_hash(), *retry_strategy, *route_params_config, false))
+				},
+				_ => Err(Bolt12PaymentError::DuplicateInvoice),
+			},
+			hash_map::Entry::Vacant(_) => Err(Bolt12PaymentError::UnexpectedInvoice),
+		}
+	}
+
 	fn pay_route_internal<NS: Deref, F>(
 		&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
 		keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>,