Skip to content

Add idempotency check to prevent duplicate InvoiceReceived events #3658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lightning/src/ln/channelmanager.rs
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benthecarman

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!

Original file line number Diff line number Diff line change
@@ -12477,6 +12477,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,
};
9 changes: 8 additions & 1 deletion lightning/src/ln/offers_tests.rs
Original file line number Diff line number Diff line change
@@ -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)
74 changes: 51 additions & 23 deletions lightning/src/ln/outbound_payment.rs
Original file line number Diff line number Diff line change
@@ -73,9 +73,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,
@@ -862,26 +862,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
.get_received_invoice_details(invoice, payment_id)?;

if invoice.invoice_features().requires_unknown_bits_from(&features) {
self.abandon_payment(
@@ -1789,6 +1772,51 @@ impl OutboundPayments {
}
}

pub(super) fn mark_invoice_received(
&self, invoice: &Bolt12Invoice, payment_id: PaymentId
) -> Result<(), Bolt12PaymentError> {
self.get_received_invoice_details(invoice, payment_id)
.and_then(|(_, _, _, invoice_marked_received)| {
invoice_marked_received
.then_some(())
.ok_or(Bolt12PaymentError::DuplicateInvoice)
})
}

fn get_received_invoice_details(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming a method get_ implies heavily that it won't actually change our current state, but this is actually marking an invoice as received. We could consider naming this one mark_invoice_received and the above fn mark_pending_invoice_received? I don't really have a great name suggestion but we def shouldn't call this get.

&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>,