Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified payment by elnafateh · Pull Request #1038 · lightningdevkit/ldk-node

elnafateh · 2026-08-10T14:59:37Z

UnifiedPayment::send previously fell back to the on-chain method after any
BOLT11 error, including Error::DuplicatePayment. Retrying a unified BIP21
payment could pay the recipient twice — once over Lightning, once on-chain.

Error::DuplicatePayment is now treated as terminal and returned to the
caller immediately, preventing the unsafe fallback.

Adds an integration test covering the retry scenario.
#1033

ldk-reviews-bot · 2026-08-10T14:59:40Z

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

joostjager · 2026-08-17T06:53:34Z

+							log_error!(self.logger, "Failed to send BOLT11 invoice: DuplicatePayment. This is part of a unified payment. Aborting to avoid duplicate payment.");
+							return Err(Error::DuplicatePayment);
+						},
+						Err(e) => {


It looks like this error can be just a persistence error happening in send/send_internal, with the payment being initiated. Fall back would be a duplicate payment.

No, persistence failures return a separate PersistenceFailed variant, so aborting on DuplicatePayment here is safe.

PersistenceFailed is the actual concern. It is caught by the Err(e) branch, and then leads to fallback, even though the payment might already have been initiated?

I understand your concern here, but that's a real pre-existing bug. It's also orthogonal to this PR, so I'll file it as a second follow-up rather than widen this change, as this PR is scoped to #1033.

It seemed similar enough to me to fix here too. But indeed, this PR is an improvement on its own ofc. Can you post the follow-up issue here too?

Actually I now see the original issue cannot be closed with this PR, because it says:

"It may also be worth reviewing other Lightning errors and separating them into:

Errors for which fallback is safe. Errors indicating that a payment already exists or may have been initiated, for which fallback must stop."

Maybe worth seeing if that's just a few more lines vs a bigger fix?

I've audited every error the BOLT11 and BOLT12 legs of UnifiedPayment::send can surface.

There are only two that indicate a payment was already initiated (or may have been): DuplicatePayment and PersistenceFailed.

Now both errors surface after pay_for_bolt11_invoice / pay_for_offer returns Ok, i.e. once the ChannelManager has the payment in-flight.

Every other error (PaymentSendingFailed, InvalidInvoice, route failures, etc.) is returned before that call succeeds, so falling back to on-chain is safe. So there are only two terminal errors, the rest safe.

here's how i want to resolve them errors:

I want to keep the BOLT11 DuplicatePayment impl and also fold the PersistenceFailed terminal arm there, so the BOLT11 leg is fully handled.

A follow-up issue/PR covers the BOLT12 leg (DuplicatePayment + PersistenceFailed). WDYT?

Sounds good. That fully addresses the original issue. Curious though how much bolt12 is. If that is similarly minimal perhaps it can all be one PR, but up to you.

the change is essentially the same, but I'd like to keep it separate.

I also have updated #1060 for Bolt12.

…ayments Error::DuplicatePayment is now terminal in UnifiedPayment::send, preventing a duplicate Lightning payment from falling back to an on-chain payment.

In `UnifiedPayment::send`, the BOLT11 leg's `bolt11_invoice.send` only returns `Err(PersistenceFailed)` *after* `pay_for_bolt11_invoice` has already succeeded and the Lightning payment is in-flight. The previous match treated every error (via `Err(e)`) as a fall-through to the next payment method, so a persistence failure after initiation would broadcast an on-chain transaction for the same URI — a duplicate payment. We now treat `Err(Error::PersistenceFailed)` on the BOLT11 leg as terminal, mirroring how `DuplicatePayment` is already handled, and abort the unified payment instead of falling back to on-chain. This is a regression hazard raised during review of the lightningdevkit#1033 fix (PR lightningdevkit#1038). It is pre-existing and orthogonal to lightningdevkit#1033 (which only made `DuplicatePayment` terminal); tracked separately as the unified variant of the broader post-commit persistence hazard. Adds `unified_send_bolt11_persistence_failure_no_onchain_fallback`, which arms a failing payment-store write on a `KVStore`-backed node and asserts that `send` returns `PersistenceFailed` without recording any on-chain payment. Co-Authored-By: Claude <noreply@anthropic.com>

The expect_payment_successful_event! macro takes a bare PaymentId, not an Option<PaymentId>. Adjust the lightningdevkit#1033 regression test accordingly so it compiles against current upstream/main.

In `UnifiedPayment::send`, the BOLT11 leg's `bolt11_invoice.send` only returns `Err(PersistenceFailed)` *after* `pay_for_bolt11_invoice` has already succeeded and the Lightning payment is in-flight. The previous match treated every error (via `Err(e)`) as a fall-through to the next payment method, so a persistence failure after initiation would broadcast an on-chain transaction for the same URI — a duplicate payment. We now treat `Err(Error::PersistenceFailed)` on the BOLT11 leg as terminal, mirroring how `DuplicatePayment` is already handled, and abort the unified payment instead of falling back to on-chain. This is a regression hazard raised during review of the lightningdevkit#1033 fix (PR lightningdevkit#1038). It is pre-existing and orthogonal to lightningdevkit#1033 (which only made `DuplicatePayment` terminal); tracked separately as the unified variant of the broader post-commit persistence hazard. Adds `unified_send_bolt11_persistence_failure_no_onchain_fallback`, which arms a failing payment-store write on a `KVStore`-backed node and asserts that `send` returns `PersistenceFailed` without recording any on-chain payment.

joostjager

You want to make sure each commit compiles, passes tests and is rustfmt'ed.

joostjager · 2026-08-21T12:51:48Z

+	}
+}
+
+impl PaginatedKVStore for PaymentFailingStore {


There is a lot of test code added. Isn't there a more compact way to cover this?

ldk-reviews-bot requested a review from tnull August 10, 2026 15:10

elnafateh force-pushed the fix/unified-payment-duplicate-fallback branch from d2e30c9 to 981bc8a Compare August 10, 2026 21:30

ajaysehwal reviewed Aug 11, 2026

View reviewed changes

Comment thread src/payment/unified.rs

elnafateh requested a review from ajaysehwal August 11, 2026 20:46

joostjager reviewed Aug 17, 2026

View reviewed changes

elnafateh requested a review from joostjager August 20, 2026 10:43

joostjager requested review from ajaysehwal and removed request for ajaysehwal and joostjager August 20, 2026 11:59

elnafateh added 2 commits August 20, 2026 21:57

Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified p…

ba5f712

…ayments Error::DuplicatePayment is now terminal in UnifiedPayment::send, preventing a duplicate Lightning payment from falling back to an on-chain payment.

Apply rustfmt to new unified payment test

e0fea6e

elnafateh force-pushed the fix/unified-payment-duplicate-fallback branch from 981bc8a to 678e1bc Compare August 20, 2026 21:42

elnafateh added 3 commits August 20, 2026 22:50

Fix regression test to match upstream macro signature

8261ca6

The expect_payment_successful_event! macro takes a bare PaymentId, not an Option<PaymentId>. Adjust the lightningdevkit#1033 regression test accordingly so it compiles against current upstream/main.

Fix unused mut warning in unified persistence fallback test

9c2d37c

elnafateh force-pushed the fix/unified-payment-duplicate-fallback branch from 678e1bc to 9c2d37c Compare August 20, 2026 21:54

elnafateh requested a review from joostjager August 21, 2026 10:22

joostjager reviewed Aug 21, 2026

View reviewed changes

Uh oh!

Conversation

elnafateh commented Aug 10, 2026

Uh oh!

ldk-reviews-bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elnafateh Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Aug 10, 2026 •

edited

Loading

joostjager Aug 18, 2026 •

edited

Loading

elnafateh Aug 20, 2026 •

edited

Loading

joostjager Aug 21, 2026 •

edited

Loading