Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified payment - #1038
Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified payment#1038elnafateh wants to merge 5 commits into
Conversation
|
👋 Thanks for assigning @ajaysehwal as a reviewer! |
d2e30c9 to
981bc8a
Compare
| 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) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No, persistence failures return a separate PersistenceFailed variant, so aborting on DuplicatePayment here is safe.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
981bc8a to
678e1bc
Compare
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.
678e1bc to
9c2d37c
Compare
4 participants
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