Fix funding-payment reclassification downgrade and deep-reorg duplication#962
Open
jkczyz wants to merge 2 commits into
Open
Fix funding-payment reclassification downgrade and deep-reorg duplication#962jkczyz wants to merge 2 commits into
jkczyz wants to merge 2 commits into
Conversation
Funding broadcasts are classified into payment records off the broadcaster's queue, which can run after wallet sync has already recorded the transaction -- for instance when LDK re-broadcasts a still-pending funding transaction on restart. In that case the classification overwrote a record wallet sync had already advanced, downgrading a confirmed or graduated funding payment back to unconfirmed/pending. Merge only the classification and our contribution figures into an existing record, leaving the confirmation state that the wallet-sync events own in place. Raised by Codex in the review of lightningdevkit#888. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A funding payment's id is anchored to its first negotiated candidate, but once it confirms the record is stamped with the candidate that actually confirmed. After it graduates and its pending-store entry is removed, a later reorg event carrying the confirmed candidate's txid could no longer be resolved to the payment, so it was recorded as a duplicate generic on-chain payment rather than reverting the splice payment. RBF splices, whose confirmed candidate differs from the first, are the reachable case. Resolve such an event against the payment store by on-chain txid once the pending store no longer holds it, and revert the funding payment to pending so wallet sync re-graduates it when it reconfirms. Raised by Codex in the review of lightningdevkit#888. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
reviewed
Jul 7, 2026
| // classification must not move it (which would downgrade an already-Confirmed/Succeeded | ||
| // record). `update` is a no-op when the entry is absent, so the pending index is not | ||
| // re-created for a payment the graduation path already removed. | ||
| let update = PaymentDetailsUpdate::funding_reclassification(details); |
Collaborator
There was a problem hiding this comment.
Hmm, given we still wait for 6-conf before deeming payment successful, I do wonder if we should just disallow unconfirming an onchain payment in update more generally? That seems more in-line with the update idempotency logic we already have there? Or maybe I'm not seeing why this wouldn't be possible?
| // below (gated on `Pending`) that graduation removed; without it a graduated payment would | ||
| // be left `Succeeded` with an `Unconfirmed` kind and no way to re-graduate. | ||
| if matches!(confirmation_status, ConfirmationStatus::Unconfirmed) { | ||
| payment.status = PaymentStatus::Pending; |
Collaborator
There was a problem hiding this comment.
As mentioned over at #888 (comment) I'm not sure we do this, as our base assumption is that anything beyond ANTI_REORG_DELAY can't be reorged anyways, hence why we have the entries only graduate after ANTI_REORG_DELAY? As mentioned in that comment, maybe it would be easier to fail early if the user tries to bump a confirmed splice?
tnull
reviewed
Jul 7, 2026
| // `find_payment_by_txid`'s payment-store fallback. Revert it like the | ||
| // `TxUnconfirmed`/`TxDropped` arms instead of mirroring a non-`Pending` record | ||
| // into the pending store, which graduation's pending-only scan would reject. | ||
| if payment.status != PaymentStatus::Pending |
Collaborator
There was a problem hiding this comment.
Codex:
- P1 /home/tnull/workspace/ldk-node-pr-962/src/wallet/mod.rs:410: TxReplaced uses BDK’s replaced txid as if it were the active unconfirmed tx. For a graduated funding payment, this stamps the old replaced txid and continues without recording conflicts; the later replacement TxUnconfirmed/
TxConfirmed will not map back and can create the duplicate this PR is trying to prevent.
| .find_payment_by_txid(txid) | ||
| .unwrap_or_else(|| PaymentId(txid.to_byte_array())); | ||
|
|
||
| if self.apply_funding_status_update(payment_id, txid, confirmation_status)? { |
Collaborator
There was a problem hiding this comment.
Codex:
- P1 /home/tnull/workspace/ldk-node-pr-962/src/wallet/mod.rs:265: TxConfirmed computes the depth-aware payment_status, but the funding short-circuit ignores it. A graduated funding payment re-confirmed in a new shallow block after a reorg can stay Succeeded, skip pending-store recreation,
and bypass ANTI_REORG_DELAY.
| self.runtime.block_on(self.pending_payment_store.insert_or_update(pending))?; | ||
| } | ||
| Ok(true) | ||
| } |
Collaborator
There was a problem hiding this comment.
Codex:
- P2 /home/tnull/workspace/ldk-node-pr-962/src/wallet/mod.rs:1551: re-created pending entries after graduation use an empty candidate list. If a different RBF candidate confirms after a deep reorg, pending.candidate(event_txid) cannot restore the correct amount/fee, so the payment can report
stale figures.
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
3 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.
Two independent fixes Codex raised in the review of #888: