Fix funding-payment reclassification downgrade and deep-reorg duplication by jkczyz · Pull Request #962 · lightningdevkit/ldk-node

jkczyz · 2026-07-02T15:23:34Z

Two independent fixes Codex raised in the review of #888:

Preserve funding-payment confirmation state on late reclassification — a broadcaster-queue classification that runs after wallet sync no longer downgrades an already-advanced (confirmed/graduated) funding payment.
Revert reorged funding payments instead of duplicating them — a deep reorg of a graduated RBF splice now reverts the payment in place instead of recording a duplicate generic on-chain payment.

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>

ldk-reviews-bot · 2026-07-02T15:23:37Z

👋 I see @tnull was un-assigned.
If you'd like another reviewer assignment, please click here.

tnull · 2026-07-07T14:14:46Z

+		// 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;


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?

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?

Sorry, you're right. This commit isn't needed. Dropping it will address the codex issues, too.

As mentioned in that comment, maybe it would be easier to fail early if the user tries to bump a confirmed splice?

Yeah, that should be covered as we check the tx_type in bump_fee_rbf. Or did you mean when using bump_channel_funding_fee we should error when RBF is possible according to LDK (i.e., the splice isn't locked yet), but we've already reached ANTI_REORG_DELAY confirmations?

Right, IIUC we could avoid the race if we just don't proceed when we previously had reached ANTI_REORG_DELAY already?

Yeah, though we should use ChannelDetails::splice_details` from https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4687 rather than looking at the pending payment store.

Yeah, though we should use ChannelDetails::splice_details` from https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4687 rather than looking at the pending payment store.

Alright, so that probably means we want to wait for the backport of that PR to land and the API to become available?

Yeah, and it looks like there are other splicing backports that need to happen first.

tnull · 2026-07-07T14:16:47Z

+					// `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


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.

tnull · 2026-07-07T14:17:22Z

 						.find_payment_by_txid(txid)
 						.unwrap_or_else(|| PaymentId(txid.to_byte_array()));

 					if self.apply_funding_status_update(payment_id, txid, confirmation_status)? {


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.

tnull · 2026-07-07T14:17:57Z

 			self.runtime.block_on(self.pending_payment_store.insert_or_update(pending))?;
 		}
 		Ok(true)
 	}


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.

tnull

Needs a rebase now that #791 landed.

tnull

Hmm, it seems the wallet sync/broadcaster race will also be a problem for #448, as there we'd then emit OnchainPayment{Successful,Received} events for transactions that then will be reclassified as channel-related (for which we'd usually not emit these events).

@jkczyz Any idea how we could avoid this class of error entirely? Or maybe it won't be an issue in practice if we stick to emitting the event only after ANTI_REORG_DELAY conf I guess?

jkczyz · 2026-07-09T22:54:17Z

Hmm, it seems the wallet sync/broadcaster race will also be a problem for #448, as there we'd then emit OnchainPayment{Successful,Received} events for transactions that then will be reclassified as channel-related (for which we'd usually not emit these events).

@jkczyz Any idea how we could avoid this class of error entirely? Or maybe it won't be an issue in practice if we stick to emitting the event only after ANTI_REORG_DELAY conf I guess?

This won't work for the restart case if the node is offline during confirmation for more than ANTI_REORG_DELAY blocks. Here's Claude's recommendation:

There are three realistic options, which can be combined:

1. Just wait for ANTI_REORG_DELAY before emitting. The label normally arrives within
seconds of broadcast, and six blocks is about an hour, so the live race is effectively
closed. The weakness is restarts: a tx that confirms while the node is offline never gets
rebroadcast, so the label never arrives, and the startup sync sees it already six deep and
emits immediately. Cheap, but doesn't close the class.

2. Check channel state directly when about to emit. Instead of trusting the tx_type
label, ask at that moment: does this tx create or spend a funding outpoint that
ChannelManager/ChainMonitor knows about, or is the sweeper tracking it? A channel
always exists in that state before its funding tx could possibly have six confirmations,
so the check can't race and survives restarts. This is what the existing TODO in
create_payment_from_tx anticipates. Its one long-term gap -- monitors for closed channels
eventually get archived -- would be covered by a persistent channel record store.

3. Label more transaction types at broadcast. Today only funding txs get labeled;
closes, sweeps, and anchor txs never do, so #448 would emit events for every coop close
and sweep even without any race. For txs only we can broadcast (sweeps, anchors, claims),
labeling before broadcast is race-free by construction. This doesn't help for shared txs
the counterparty can broadcast first, but it's a prerequisite for the tx_type check to
mean anything.

Weaker ideas I'd rule out: forcing the wallet sync to wait for the broadcast queue to
drain (doesn't help when the label never comes, e.g. after a restart), and emitting
corrective "reclassified" events later (pushes the problem onto users).

My take: 3 is needed regardless, 2 is what actually eliminates the class, and 1 is a
reasonable stopgap until then.

tnull · 2026-07-10T09:02:01Z

1. Just wait for ANTI_REORG_DELAY before emitting. The label normally arrives within seconds of broadcast, and six blocks is about an hour, so the live race is effectively closed. The weakness is restarts: a tx that confirms while the node is offline never gets rebroadcast, so the label never arrives, and the startup sync sees it already six deep and emits immediately. Cheap, but doesn't close the class.

2. Check channel state directly when about to emit. Instead of trusting the tx_type label, ask at that moment: does this tx create or spend a funding outpoint that ChannelManager/ChainMonitor knows about, or is the sweeper tracking it? A channel always exists in that state before its funding tx could possibly have six confirmations, so the check can't race and survives restarts. This is what the existing TODO in create_payment_from_tx anticipates. Its one long-term gap -- monitors for closed channels eventually get archived -- would be covered by a persistent channel record store.

3. Label more transaction types at broadcast. Today only funding txs get labeled; closes, sweeps, and anchor txs never do, so #448 would emit events for every coop close and sweep even without any race. For txs only we can broadcast (sweeps, anchors, claims), labeling before broadcast is race-free by construction. This doesn't help for shared txs the counterparty can broadcast first, but it's a prerequisite for the tx_type check to mean anything.

Weaker ideas I'd rule out: forcing the wallet sync to wait for the broadcast queue to drain (doesn't help when the label never comes, e.g. after a restart), and emitting corrective "reclassified" events later (pushes the problem onto users).

My take: 3 is needed regardless, 2 is what actually eliminates the class, and 1 is a reasonable stopgap until then.

Hmm, so 3 is already done in #791 (so this comment seems somewhat stale), in the current version of #448 we already do 1. But, it seems 2 / the restart issue might be a good reason to move forward with #946 after all?

jkczyz · 2026-07-10T22:53:39Z

Hmm, so 3 is already done in #791 (so this comment seems somewhat stale), in the current version of #448 we already do 1. But, it seems 2 / the restart issue might be a good reason to move forward with #946 after all?

Yeah, though for splicing most data can still live in the pending payment store. The channel store would only need funding outpoints to check. When we introduce batching, we wouldn't want to duplicate all that data across each channel in the storage.

jkczyz and others added 2 commits July 1, 2026 21:47

jkczyz requested a review from tnull July 2, 2026 15:23

jkczyz mentioned this pull request Jul 2, 2026

Retry user-initiated splices across restarts and disconnects #930

Draft

jkczyz added this to Weekly Goals Jul 2, 2026

tnull reviewed Jul 7, 2026

View reviewed changes

jkczyz requested a review from tnull July 9, 2026 05:50

tnull removed their request for review July 9, 2026 10:32

tnull reviewed Jul 9, 2026

View reviewed changes

Uh oh!

Conversation

jkczyz commented Jul 2, 2026

Uh oh!

ldk-reviews-bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull left a comment • 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!

jkczyz commented Jul 9, 2026

Uh oh!

tnull commented Jul 10, 2026

Uh oh!

jkczyz commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ldk-reviews-bot commented Jul 2, 2026 •

edited

Loading

tnull left a comment •

edited

Loading