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.

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

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

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

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.

tnull

This needs a rebase by now.

Also, some additional claude comments:

Conflicts and candidates are dropped when reverting a graduated payment. The revert path re-creates the pending entry via create_pending_payment_from_tx(payment, Vec::new()) — empty conflicting_txids and empty candidates (the graduation removal already discarded the originals).
Consequence: if, after the deep reorg, a different RBF candidate confirms than the stamped one, an earlier/middle candidate's txid no longer maps to the payment (duplicate record — the very bug class this PR fixes), and even for the first candidate the confirmed-candidate figures can't be
re-stamped since pending.candidate(event_txid) finds nothing. In practice LDK's re-broadcast of the candidate re-runs classify_interactive_funding, whose merge path (commit 1) restores the full candidate history, so this likely self-heals — but that's an implicit dependency worth a comment
or an upstream question. Similarly, the graduated-funding TxReplaced path continues without recording the event's conflict txids.

Residual TOCTOU in persist_funding_payment (src/wallet/mod.rs:1418). contains_key and the subsequent insert aren't atomic; a wallet sync landing in between would make the fresh-insert path do a full insert_or_update merge that could still clobber confirmation state. The window is tiny and
strictly better than before (the old code clobbered unconditionally), so a nit only.

Documented invariant worth upstream confirmation. The funding_reclassification doc comment leans on "LDK only re-broadcasts the active/confirmed funding candidate" to justify unconditionally overwriting txid/amount/fee. If LDK ever rebroadcasts a non-confirmed candidate for an
already-confirmed record, the stamped figures would be silently replaced. Fine to rely on, but it's the kind of cross-crate invariant a reviewer on the LDK side should ack.

tnull · 2026-07-27T11:48:57Z

+		// merges into it), so the first match is unambiguous.
+		if let Some(funding) = self
+			.payment_store
+			.list_filter(|p| {


I really don't think we can do this - this would scan all payment store entries constantly, which is a no-go even if all of them live in memory. And going forward we'll also want to only keep a cache of payments in memory while most of them live just in the KVStore.

To make this more efficient we probably need a secondary index, similar to what we'll do in #948.

I believe this is no longer a problem since the commit adding this is now dropped.

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 the counterparty's broadcast of the funding transaction is observed by wallet sync first. 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>

jkczyz · 2026-07-29T05:31:31Z

Push is just a rebase plus dropping a commit as per #962 (comment). I have some of the other issues addressed locally but need to verify the work still.

tnull · 2026-07-29T08:21:39Z

Push is just a rebase plus dropping a commit as per #962 (comment). I have some of the other issues addressed locally but need to verify the work still.

Alright, please re-request review when it's ready!

…ce check Co-Authored-By: Claude <noreply@anthropic.com>

…fication Co-Authored-By: Claude <noreply@anthropic.com>

jkczyz · 2026-07-29T20:34:37Z

PTAL

Also, some additional claude comments:

Conflicts and candidates are dropped when reverting a graduated payment. The revert path re-creates the pending entry via create_pending_payment_from_tx(payment, Vec::new()) — empty conflicting_txids and empty candidates (the graduation removal already discarded the originals).
Consequence: if, after the deep reorg, a different RBF candidate confirms than the stamped one, an earlier/middle candidate's txid no longer maps to the payment (duplicate record — the very bug class this PR fixes), and even for the first candidate the confirmed-candidate figures can't be
re-stamped since pending.candidate(event_txid) finds nothing. In practice LDK's re-broadcast of the candidate re-runs classify_interactive_funding, whose merge path (commit 1) restores the full candidate history, so this likely self-heals — but that's an implicit dependency worth a comment
or an upstream question. Similarly, the graduated-funding TxReplaced path continues without recording the event's conflict txids.

Dropping the earlier commit fixes this.

Residual TOCTOU in persist_funding_payment (src/wallet/mod.rs:1418). contains_key and the subsequent insert aren't atomic; a wallet sync landing in between would make the fresh-insert path do a full insert_or_update merge that could still clobber confirmation state. The window is tiny and
strictly better than before (the old code clobbered unconditionally), so a nit only.

Documented invariant worth upstream confirmation. The funding_reclassification doc comment leans on "LDK only re-broadcasts the active/confirmed funding candidate" to justify unconditionally overwriting txid/amount/fee. If LDK ever rebroadcasts a non-confirmed candidate for an
already-confirmed record, the stamped figures would be silently replaced. Fine to rely on, but it's the kind of cross-crate invariant a reviewer on the LDK side should ack.

Added fixups for these two.

tnull

Still have to take a closer look.

In general, I do wonder if with the current design we'll really be able to whack-a-mole all edge cases here :(

tnull · 2026-07-30T14:09:16Z

+		// confirmed, which need not be the last one broadcast. An update that doesn't assert the
+		// confirmation state was built without knowing it — e.g. a late funding classification
+		// whose candidate lost to the counterparty's broadcast — so it must not move them.
+		let keep_confirmed_figures = update.confirmation_status.is_none()


Codex:

P1 — /home/tnull/worktrees/ldk-node/pr-962-latest-20260730/src/payment/store.rs:250: keep_confirmed_figures assumes every confirmed record already contains the correct funding-candidate figures. In the race this PR handles, wallet sync may have created a generic tx_type: None record using
BDK-derived figures, which the code itself says cannot represent our contribution to shared funding. Late classification now preserves those incorrect figures even though its candidate history contains the correct values. It should select the candidate matching the stored confirmed txid,
rather than unconditionally preserving generic figures.

Added a fixup addressing this for now.

tnull · 2026-07-30T14:10:12Z

+				let pending = PendingPaymentDetails::new(details, Vec::new(), candidates);
+				self.pending_payment_store.update_or_insert(pending_update, pending).await?;
+			},
+			DataStoreUpdateOrInsertResult::Updated | DataStoreUpdateOrInsertResult::Unchanged => {


Codex:

P2 — /home/tnull/worktrees/ldk-node/pr-962-latest-20260730/src/wallet/mod.rs:1428: an absent pending entry is treated as necessarily graduated. If the payment-store write succeeds but the pending-store write fails—or the node stops between them—a retry takes the Updated/Unchanged branch
and silently ignores NotFound. Main’s unconditional insert_or_update repaired this state. For an RBF splice, the missing pending index prevents find_payment_by_txid from mapping the replacement txid to the stable payment ID, potentially producing a duplicate generic payment. A missing
entry should be recreated when the authoritative payment is still Pending.

Added a fixup addressing this for now.

jkczyz · 2026-07-30T15:43:12Z

Still have to take a closer look.

In general, I do wonder if with the current design we'll really be able to whack-a-mole all edge cases here :(

Yeah, I seem to be running into similar problems in the dependent PR (#930). What approach were you thinking? Adding a secondary index (txid -> payment_id) and maybe consolidating the two stores?

The keep-confirmed-figures guard overshot: when wallet sync recorded the confirmation first, the record carries the wallet's own view of amount/fee, which cannot represent our contribution to a shared funding output, and the guard then discarded the late classification's correct contribution-derived figures along with the losing-candidate updates it was meant to block. Let an update that names the confirmed txid move the figures — it describes the very candidate that confirmed — and have classification build its update from the candidate matching an already-confirmed record, mirroring what apply_funding_status_update reports when confirmation arrives after classification. The txid comparison happens under the store's mutation lock at apply time, so the unlocked snapshot read cannot misapply figures. Generated with assistance from Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Classification treated an absent pending entry as proof the payment had graduated and only merged into existing entries. But a crash or failed write between the payment-store and pending-store writes leaves a Pending record with no index entry, and that state was never repaired: the payment could no longer graduate (graduation iterates the pending store) and its candidate txids could no longer be mapped back to the record, which for an RBF splice invites a duplicate generic payment. Decide by the post-write payment status instead: while the record is still Pending, insert the missing entry (embedding the post-write record, so a confirmation wallet sync already mirrored keeps driving graduation); once it advanced beyond Pending, keep treating absence as graduated. A graduated payment is never Pending, so the no-reindex rule is preserved by the status gate itself. The repaired state is not constructible in a test: it requires a failure injected between the two store writes, and no such seam exists. The store primitives the decision rests on are unit-tested. Generated with assistance from Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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

jkczyz mentioned this pull request Jul 13, 2026

Wallet mutex held across Runtime::block_on(persist_async) can deadlock the entire node runtime #978

Closed

tnull added this to the 0.8 milestone Jul 27, 2026

tnull reviewed Jul 27, 2026

View reviewed changes

jkczyz force-pushed the 2026-07-funding-payment-lifecycle-fixes branch from f01e83e to 46096b0 Compare July 28, 2026 23:12

jkczyz and others added 2 commits July 29, 2026 14:28

f - Make funding-payment classification write atomic with its existen…

cb99564

…ce check Co-Authored-By: Claude <noreply@anthropic.com>

f - Keep a confirmed funding record's txid and figures on late classi…

0e44736

…fication Co-Authored-By: Claude <noreply@anthropic.com>

jkczyz force-pushed the 2026-07-funding-payment-lifecycle-fixes branch from 46096b0 to 0e44736 Compare July 29, 2026 20:29

jkczyz requested a review from tnull July 29, 2026 20:34

tnull reviewed Jul 30, 2026

View reviewed changes

jkczyz and others added 2 commits July 30, 2026 10:50

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!

tnull left a comment

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!

jkczyz commented Jul 29, 2026

Uh oh!

tnull commented Jul 29, 2026

Uh oh!

jkczyz commented Jul 29, 2026

Uh oh!

tnull left a comment

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!

jkczyz commented Jul 30, 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