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.
| } | ||
| } | ||
|
|
||
| impl PaginatedKVStore for PaymentFailingStore { |
Contributor
There was a problem hiding this comment.
There is a lot of test code added. Isn't there a more compact way to cover this?
Contributor
Author
There was a problem hiding this comment.
Got it! Extracted the shared node and collapsed the duplicate arms.
…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. Adds a regression test, unified_send_bolt11_duplicate_payment_no_onchain_fallback, along with fund_and_open_ready_channel(), wait_for_node_announcement(), and receive_bolt11_only_uri() test helpers reused by the PersistenceFailed regression test in the next commit.
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
| 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 |
Contributor
There was a problem hiding this comment.
🤖 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.
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
4 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