Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified payment - #1038
Open
elnafateh wants to merge 2 commits into
Open
Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified payment#1038elnafateh wants to merge 2 commits into
elnafateh wants to merge 2 commits into
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
elnafateh
force-pushed
the
fix/unified-payment-duplicate-fallback
branch
from
August 10, 2026 21:30
d2e30c9 to
981bc8a
Compare
ajaysehwal
reviewed
Aug 11, 2026
joostjager
reviewed
Aug 17, 2026
joostjager
requested review from
ajaysehwal
and removed request for
ajaysehwal and
joostjager
August 20, 2026 11:59
elnafateh
added a commit
to elnafateh/ldk-node
that referenced
this pull request
Aug 20, 2026
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>
elnafateh
force-pushed
the
fix/unified-payment-duplicate-fallback
branch
from
August 20, 2026 21:42
981bc8a to
678e1bc
Compare
elnafateh
added a commit
to elnafateh/ldk-node
that referenced
this pull request
Aug 20, 2026
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.
elnafateh
force-pushed
the
fix/unified-payment-duplicate-fallback
branch
from
August 20, 2026 21:54
678e1bc to
9c2d37c
Compare
joostjager
reviewed
Aug 21, 2026
joostjager
left a comment
joostjager
left a comment
Contributor
There was a problem hiding this comment.
You want to make sure each commit compiles, passes tests and is rustfmt'ed.
elnafateh
added a commit
to elnafateh/ldk-node
that referenced
this pull request
Aug 22, 2026
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.
elnafateh
force-pushed
the
fix/unified-payment-duplicate-fallback
branch
from
August 22, 2026 15:32
9c2d37c to
93c18b3
Compare
joostjager
reviewed
Aug 25, 2026
…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).
elnafateh
force-pushed
the
fix/unified-payment-duplicate-fallback
branch
from
August 25, 2026 13:10
93c18b3 to
d7ee094
Compare
Contributor
Author
|
Hey @joostjager Whenever you get a chance, could you take a quick look at the test adjustments from last week? Thanks! |
joostjager
approved these changes
Sep 1, 2026
joostjager
left a comment
joostjager
left a comment
Contributor
There was a problem hiding this comment.
LGTM. PersistenceFailed remains undesirable because callers cannot tell whether the payment started. This ties back to the broader persistence discussion in #1003, which we can hopefully make progress on soon.
Contributor
Author
|
@tnull could you pls review this PR when u have the time, so that I can proceed with follow-up PR as discussed w @joostjager |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UnifiedPayment::sendpreviously fell back to the on-chain method after anyBOLT11 error, including
Error::DuplicatePayment. Retrying a unified BIP21payment could pay the recipient twice — once over Lightning, once on-chain.
Error::DuplicatePaymentis now treated as terminal and returned to thecaller immediately, preventing the unsafe fallback.
Adds an integration test covering the retry scenario.
#1033