From 21ec6080c7b6082b06e0c081a25f5ea3b150ec22 Mon Sep 17 00:00:00 2001 From: shaavan Date: Wed, 12 Mar 2025 17:45:00 +0530 Subject: [PATCH 1/3] Make InvoiceReceived event generation idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ensures `InvoiceReceived` events are not generated multiple times when `manually_handle_bolt12_invoice` is enabled. Duplicate events for the same invoice could cause confusion—this change introduces an idempotency check to prevent that. --- lightning/src/ln/channelmanager.rs | 8 ++- lightning/src/ln/outbound_payment.rs | 88 ++++++++++++++++++++-------- 2 files changed, 69 insertions(+), 27 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 00a536539ea..1a4d5bd34cd 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4824,7 +4824,8 @@ where invoice, payment_id, &self.router, self.list_usable_channels(), features, || self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, &self, &self.secp_ctx, best_block_height, &self.logger, &self.pending_events, - |args| self.send_payment_along_path(args) + |args| self.send_payment_along_path(args), + self.default_configuration.manually_handle_bolt12_invoices ) } @@ -12477,6 +12478,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, }; diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 15a370592c0..add313d4e35 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -851,7 +851,7 @@ impl OutboundPayments { entropy_source: &ES, node_signer: &NS, node_id_lookup: &NL, secp_ctx: &Secp256k1, best_block_height: u32, logger: &L, pending_events: &Mutex)>>, - send_payment_along_path: SP, + send_payment_along_path: SP, with_manual_handling: bool ) -> Result<(), Bolt12PaymentError> where R::Target: Router, @@ -862,26 +862,15 @@ 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), - } + // 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. + let (payment_hash, retry_strategy, params_config) = if with_manual_handling { + self.received_invoice_details(invoice, payment_id)? + } else { + self.mark_invoice_received_and_get_details(invoice, payment_id)? + }; if invoice.invoice_features().requires_unknown_bits_from(&features) { self.abandon_payment( @@ -1789,6 +1778,53 @@ 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).map(|_| ()) + } + + fn mark_invoice_received_and_get_details( + &self, invoice: &Bolt12Invoice, payment_id: PaymentId + ) -> Result<(PaymentHash, Retry, RouteParametersConfig), 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)) + }, + _ => Err(Bolt12PaymentError::DuplicateInvoice), + }, + hash_map::Entry::Vacant(_) => Err(Bolt12PaymentError::UnexpectedInvoice), + } + } + + fn received_invoice_details( + &self, invoice: &Bolt12Invoice, payment_id: PaymentId, + ) -> Result<(PaymentHash, Retry, RouteParametersConfig), Bolt12PaymentError> { + match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { + hash_map::Entry::Occupied(entry) => match entry.get() { + PendingOutboundPayment::InvoiceReceived { + retry_strategy, route_params_config, .. + } => { + Ok((invoice.payment_hash(), *retry_strategy, *route_params_config)) + }, + _ => Err(Bolt12PaymentError::DuplicateInvoice), + }, + hash_map::Entry::Vacant(_) => Err(Bolt12PaymentError::UnexpectedInvoice), + } + } + fn pay_route_internal( &self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields, keysend_preimage: Option, invoice_request: Option<&InvoiceRequest>, @@ -2868,7 +2904,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| panic!() + &secp_ctx, 0, &&logger, &pending_events, |_| panic!(), false ), Err(Bolt12PaymentError::SendingFailed(RetryableSendFailure::PaymentExpired)), ); @@ -2930,7 +2966,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| panic!() + &secp_ctx, 0, &&logger, &pending_events, |_| panic!(), false ), Err(Bolt12PaymentError::SendingFailed(RetryableSendFailure::RouteNotFound)), ); @@ -3005,7 +3041,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| panic!() + &secp_ctx, 0, &&logger, &pending_events, |_| panic!(), false ), Err(Bolt12PaymentError::UnexpectedInvoice), ); @@ -3025,7 +3061,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| Ok(()) + &secp_ctx, 0, &&logger, &pending_events, |_| Ok(()), false ), Ok(()), ); @@ -3036,7 +3072,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| panic!() + &secp_ctx, 0, &&logger, &pending_events, |_| panic!(), false ), Err(Bolt12PaymentError::DuplicateInvoice), ); From ddc27b7804ffc99f601933928a9649efd60d6b00 Mon Sep 17 00:00:00 2001 From: shaavan Date: Tue, 1 Apr 2025 19:59:35 +0530 Subject: [PATCH 2/3] f: Remove is_manual_handling param --- lightning/src/ln/channelmanager.rs | 3 +- lightning/src/ln/outbound_payment.rs | 60 ++++++++++++---------------- 2 files changed, 27 insertions(+), 36 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 1a4d5bd34cd..91a1885381c 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4824,8 +4824,7 @@ where invoice, payment_id, &self.router, self.list_usable_channels(), features, || self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, &self, &self.secp_ctx, best_block_height, &self.logger, &self.pending_events, - |args| self.send_payment_along_path(args), - self.default_configuration.manually_handle_bolt12_invoices + |args| self.send_payment_along_path(args) ) } diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index add313d4e35..88716249dcf 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -73,9 +73,9 @@ pub(crate) enum PendingOutboundPayment { route_params_config: RouteParametersConfig, retryable_invoice_request: Option }, - // 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, @@ -851,7 +851,7 @@ impl OutboundPayments { entropy_source: &ES, node_signer: &NS, node_id_lookup: &NL, secp_ctx: &Secp256k1, best_block_height: u32, logger: &L, pending_events: &Mutex)>>, - send_payment_along_path: SP, with_manual_handling: bool + send_payment_along_path: SP, ) -> Result<(), Bolt12PaymentError> where R::Target: Router, @@ -862,15 +862,9 @@ impl OutboundPayments { IH: Fn() -> InFlightHtlcs, SP: Fn(SendAlongPathArgs) -> Result<(), APIError>, { - // 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. - let (payment_hash, retry_strategy, params_config) = if with_manual_handling { - self.received_invoice_details(invoice, payment_id)? - } else { - self.mark_invoice_received_and_get_details(invoice, payment_id)? - }; + + let (payment_hash, retry_strategy, params_config, _) = self + .get_received_invoice_details(invoice, payment_id)?; if invoice.invoice_features().requires_unknown_bits_from(&features) { self.abandon_payment( @@ -1781,12 +1775,17 @@ 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).map(|_| ()) + self.get_received_invoice_details(invoice, payment_id) + .and_then(|(_, _, _, invoice_marked_received)| { + invoice_marked_received + .then_some(()) + .ok_or(Bolt12PaymentError::DuplicateInvoice) + }) } - fn mark_invoice_received_and_get_details( + fn get_received_invoice_details( &self, invoice: &Bolt12Invoice, payment_id: PaymentId - ) -> Result<(PaymentHash, Retry, RouteParametersConfig), Bolt12PaymentError> { + ) -> 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 { @@ -1801,23 +1800,16 @@ impl OutboundPayments { route_params_config: config, }; - Ok((payment_hash, retry, config)) + Ok((payment_hash, retry, config, true)) }, - _ => Err(Bolt12PaymentError::DuplicateInvoice), - }, - hash_map::Entry::Vacant(_) => Err(Bolt12PaymentError::UnexpectedInvoice), - } - } - - fn received_invoice_details( - &self, invoice: &Bolt12Invoice, payment_id: PaymentId, - ) -> Result<(PaymentHash, Retry, RouteParametersConfig), Bolt12PaymentError> { - match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { - hash_map::Entry::Occupied(entry) => match entry.get() { + // 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)) + Ok((invoice.payment_hash(), *retry_strategy, *route_params_config, false)) }, _ => Err(Bolt12PaymentError::DuplicateInvoice), }, @@ -2904,7 +2896,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| panic!(), false + &secp_ctx, 0, &&logger, &pending_events, |_| panic!() ), Err(Bolt12PaymentError::SendingFailed(RetryableSendFailure::PaymentExpired)), ); @@ -2966,7 +2958,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| panic!(), false + &secp_ctx, 0, &&logger, &pending_events, |_| panic!() ), Err(Bolt12PaymentError::SendingFailed(RetryableSendFailure::RouteNotFound)), ); @@ -3041,7 +3033,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| panic!(), false + &secp_ctx, 0, &&logger, &pending_events, |_| panic!() ), Err(Bolt12PaymentError::UnexpectedInvoice), ); @@ -3061,7 +3053,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| Ok(()), false + &secp_ctx, 0, &&logger, &pending_events, |_| Ok(()) ), Ok(()), ); @@ -3072,7 +3064,7 @@ mod tests { outbound_payments.send_payment_for_bolt12_invoice( &invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(), || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {}, - &secp_ctx, 0, &&logger, &pending_events, |_| panic!(), false + &secp_ctx, 0, &&logger, &pending_events, |_| panic!() ), Err(Bolt12PaymentError::DuplicateInvoice), ); From bf897d2ed17325f3c028f877d46313612e4e0cc6 Mon Sep 17 00:00:00 2001 From: shaavan Date: Mon, 10 Mar 2025 23:18:45 +0530 Subject: [PATCH 3/3] Introduce idempotency check in tests --- lightning/src/ln/offers_tests.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index 55d2497bf8c..891fe6131e8 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -1186,7 +1186,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)