Add idempotency check to prevent duplicate InvoiceReceived events
#3658
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
There was a problem hiding this comment.
it seems send_payment_for_bolt12_invoice is not idempotent either so this would effect everyone using manually_handle_bolt12_invoices
Hi Ben! AFAIU, send_payment_for_bolt12_invoice does seem idempotent, as in, it won’t allow paying the same invoice twice (see this link).
Let me know if I’m misunderstanding the issue or if there’s still a potential problem here that I might be missing. Thanks!
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
@TheBlueMatt Is the following line still correct after this change? |
lightning/src/ln/outbound_payment.rs
Outdated
| // current one is handled by the user. | ||
| match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { | ||
| hash_map::Entry::Occupied(entry) => match entry.get() { | ||
| PendingOutboundPayment::InvoiceReceived { |
There was a problem hiding this comment.
Isn't this race-y? If a user (for some reason, they shouldn't, but still) calls send_payment_for_bolt12_invoice twice in two separate threads we could end up actually sending the payment twice.
There was a problem hiding this comment.
Wouldn't the second lock acquisition happening in send_payment_for_bolt12_invoice_internal prevent that? We'd still attempt to find a path twice but would fail because the first attempt transitions the state to Retryable causing the second attempt to return DuplicateInvoice.
lightning/src/ln/outbound_payment.rs
Outdated
| if !with_manual_handling { self.mark_invoice_received(payment_id, payment_hash)? } | ||
|
|
||
| let (retry_strategy, params_config) = self.received_invoice_details(payment_id)?; |
There was a problem hiding this comment.
Can we avoid taking the lock twice here when with_manual_handling is false?
There was a problem hiding this comment.
Thanks for the pointer, Jeff!
I’ve updated the mark_invoice_received function to also return the retry and route_params, so we can avoid taking the lock twice in pr3658.04.
Let me know if this looks good — thanks again!
lightning/src/ln/outbound_payment.rs
Outdated
| fn received_invoice_details( | ||
| &self, payment_id: PaymentId, | ||
| ) -> Result<(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, .. | ||
| } => { | ||
| return Ok((*retry_strategy, *route_params_config)) | ||
| }, | ||
| _ => return Err(Bolt12PaymentError::DuplicateInvoice), | ||
| }, | ||
| hash_map::Entry::Vacant(_) => return Err(Bolt12PaymentError::UnexpectedInvoice), | ||
| } | ||
| } |
There was a problem hiding this comment.
Probably can re-inline this to avoid taking the lock again.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3658 +/- ##
==========================================
+ Coverage 89.21% 91.11% +1.90%
==========================================
Files 155 156 +1
Lines 118966 135669 +16703
Branches 118966 135669 +16703
==========================================
+ Hits 106133 123613 +17480
+ Misses 10253 9606 -647
+ Partials 2580 2450 -130 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Solves #3653
When
manually_handle_bolt12_invoiceis enabled, we shouldn’t emit multipleInvoiceReceivedevents for the same invoice — it can be confusing and misleading for consumers of the event stream.This PR adds a simple idempotency check to make sure we only emit the event once per invoice.
Also added a test to make sure this behavior stays in place going forward.