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.

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>

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?

Got it! Extracted the shared node and collapsed the duplicate arms.

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 remaining error 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. Err(Error::PersistenceFailed) on the BOLT11 leg is now terminal, mirroring how DuplicatePayment is handled, and aborts the unified payment instead of falling back to on-chain. This is a regression hazard raised during review of the DuplicatePayment fix (PR lightningdevkit#1038). It is pre-existing and orthogonal to lightningdevkit#1033; tracked here as the unified variant of the broader post-commit persistence hazard. Adds PaymentFailingStore, a KVStore wrapper that fails writes to the payments namespace on demand, and a regression test, unified_send_bolt11_persistence_failure_no_onchain_fallback, which arms it and asserts send() returns PersistenceFailed without recording an on-chain payment. Reuses the fund_and_open_ready_channel(), wait_for_node_announcement(), and receive_bolt11_only_uri() helpers from the previous commit.

joostjager · 2026-08-25T08:21:10Z

 	assert_eq!(node_b.list_balances().total_lightning_balance_sats, 200_000);
 }

+/// Funds `node_a`, opens an announced channel to `node_b`, mines it to `ChannelReady` on both


🤖 unified_send_receive_bip21_uri above already performs the funding, channel setup, announcement wait, and successful BOLT11 send, and it already has uri_str_without_offer. Could we add the duplicate retry assertion there instead of introducing these helpers and a separate test?

The same existing test could use a toggleable payment-namespace-failing store from the start, then enable failures for one fresh URI at the end to cover PersistenceFailed. That would reuse the same nodes and channel, leaving only the store adapter and short assertions.

All concerns are resolved and node_a now starts on an inert PaymentFailingStore armed only at the end for the persistence-failure check. All tests passes locally.

…ayments UnifiedPayment::send previously treated any error from the BOLT11 leg of a unified payment as non-terminal and fell through to the on-chain payment method. This meant a retried BOLT11 payment that returns Error::DuplicatePayment would still result in an on-chain transaction being broadcast for the same invoice — a duplicate payment. Error::DuplicatePayment is now terminal in UnifiedPayment::send: the unified payment aborts instead of falling back to on-chain. Fixes lightningdevkit#1033. unified_send_receive_bip21_uri already funds two nodes, opens a channel, and sends a successful BOLT11 payment via uri_str_without_offer partway through. Add the regression assertion right there — retry the same uri_str_without_offer and assert DuplicatePayment, not a new on-chain payment — instead of duplicating that setup in a standalone test.

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 remaining error 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. Err(Error::PersistenceFailed) on the BOLT11 leg is now terminal, mirroring how DuplicatePayment is handled, and aborts the unified payment instead of falling back to on-chain. This is a regression hazard raised during review of the DuplicatePayment fix (PR lightningdevkit#1038). It is pre-existing and orthogonal to lightningdevkit#1033; tracked here as the unified variant of the broader post-commit persistence hazard. Per review discussion: rather than a dedicated node/channel fixture, build unified_send_receive_bip21_uri's node_a on a PaymentFailingStore (inert until armed) from the start, and add a PersistenceFailed assertion at the end of that test using a fresh BOLT11-only URI. This reuses the funding/channel/announcement setup and the successful-send flow the test already has, rather than duplicating it. Adds PaymentFailingStore (a KVStore wrapper that fails writes to the payments namespace on demand) and setup_two_nodes_with_failing_store_a (mirrors setup_two_nodes, but node_a is built on PaymentFailingStore).

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

Comment thread src/payment/unified.rs

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 force-pushed the fix/unified-payment-duplicate-fallback branch from 981bc8a to 678e1bc Compare August 20, 2026 21:42

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

elnafateh force-pushed the fix/unified-payment-duplicate-fallback branch from 9c2d37c to 93c18b3 Compare August 22, 2026 15:32

elnafateh requested a review from joostjager August 23, 2026 21:42

joostjager reviewed Aug 25, 2026

View reviewed changes

elnafateh added 2 commits August 25, 2026 12:38

elnafateh force-pushed the fix/unified-payment-duplicate-fallback branch from 93c18b3 to d7ee094 Compare August 25, 2026 13:10

elnafateh requested a review from joostjager August 25, 2026 16:01

Navigation Menu

Uh oh!

Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified payment - #1038

Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified payment#1038
elnafateh wants to merge 2 commits into
lightningdevkit:mainfrom
elnafateh:fix/unified-payment-duplicate-fallback

elnafateh commented Aug 10, 2026

Uh oh!

ldk-reviews-bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

joostjager left a comment

Uh oh!

joostjager Aug 21, 2026 •

edited

Loading

Uh oh!

elnafateh Aug 22, 2026

Uh oh!

joostjager Aug 25, 2026

Uh oh!

elnafateh Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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!

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!

elnafateh Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

elnafateh Aug 25, 2026

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 21, 2026 •

edited

Loading