Fix funding-payment reclassification downgrade and deep-reorg duplication

[jkczyz]

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]

👋 Thanks for assigning @joostjager 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]

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?

[jkczyz]

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?

[tnull]

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

[jkczyz]

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.

[tnull]

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?

[jkczyz]

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

[tnull]

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]

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]

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]

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]

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]

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:

1. 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.

2. 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.

3. 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]

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.

[jkczyz]

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

[jkczyz]

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]

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!

[jkczyz]

PTAL

Also, some additional claude comments:

1. 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.

2. 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.
3. 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]

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.

[jkczyz]

Added a fixup addressing this for now.

[joostjager]

P2: The missing-index repair still depends on classification running again, which is not guaranteed. persist_funding_payment commits the payment-store record before writing the pending entry. If the second write fails or the process stops between them, the stores remain inconsistent.

In the failure case, classify_package returns an error and the broadcast-queue consumer continues after discarding the already-dequeued in-memory package. There is therefore no automatic retry. This is particularly relevant for shared funding transactions because the counterparty may still broadcast the transaction.

The in-process mutex cannot provide crash atomicity. Please either persist payment details and candidate/index metadata as one record, introduce durable retry or startup reconciliation for this invariant, or otherwise demonstrate how every partial write is repaired without relying on a future LDK rebroadcast.

[jkczyz]

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?

[joostjager]

AI drive-by

P1: This still races with graduation. recorded is an unlocked snapshot taken at line 1429. After it observes Pending, ChainTipChanged can update the authoritative payment to Succeeded and remove the pending entry. This update_or_insert then sees the entry absent and recreates it from the stale Pending snapshot. If that snapshot was also Unconfirmed, later chain-tip processing can repeatedly rebroadcast an already-graduated transaction. The two stores have independent mutation locks, so the status decision and index insertion need one cross-store critical section or transactional record.

[jkczyz]

Moved the mutate method from #930 here to address this, but updated it to clone. Still worth considering options for more robust handling as mentioned here: #962 (comment)

[tnull]

What approach were you thinking? Adding a secondary index (txid -> payment_id) and maybe consolidating the two stores?

Well, it seems to me that the fundamental issue is the chosen approach of classifying at time of broadcast which can happen before or after the wallet sync, i.e., after we actually see the transaction. So fixing it would be reconsidering that model, i.e., offer an LDK interface along the lines of ChannelManager::classify_transaction(txid: Txid) -> TransactionType that we can use to query the type during sync? But, that was deemed unfeasible at the time, hence why we arrived here.

[tnull]

Codex:

• [P1] Reconcile confirmations during pending-index persistence — /home/tnull/worktrees/ldk-node/pr-962-latest-20260805/src/wallet/mod.rs:1431

  After the payment-store write, candidate history is not visible until pending_payment_store.mutate() finishes persisting. A concurrent confirmation can therefore see no candidates, stamp confirmed candidate A with active candidate B’s figures, and create the pending entry. The
  classification path subsequently adds the candidates but never repairs the authoritative payment record, leaving the wrong amount/fee permanently. Candidate history must be visible before confirmation handling, or the payment record must be reconciled afterward.

Hmm, maybe for now we need to introduce a funding_payment_update_lock: tokio::sync::Mutex<()> that we can hold across both payment store and pending payment store updates to ensure they are always in-sync?

[jkczyz]

Yeah, good idea.

[joostjager]

P1: The mutex still does not cover the complete decision and write sequence. TxConfirmed resolves payment_id before calling apply_funding_status_update, while that helper releases the mutex before the caller performs its generic fallback writes.

The remaining interleaving is:

1. Wallet sync looks up the payment and enters apply_funding_status_update.
2. The helper observes no classified funding record, returns false, and releases the mutex.
3. Classification acquires the mutex and commits the classified payment plus candidate history.
4. Wallet sync continues through the generic fallback outside the mutex, potentially replacing contribution-derived figures with wallet-derived figures or creating a second payment under the event txid.

The same boundary issue applies to TxUnconfirmed and TxDropped. Please acquire the shared lock before find_payment_by_txid and hold it until either the classified update or the complete generic payment-plus-pending update has finished. The helper will need a variant that assumes the caller already holds the lock. A deterministic barrier test should exercise both orderings.

[tnull]

Ah, yeah, seems our LLMs agree: #962 (comment)

[jkczyz]

🤖 You're right — the lock only covered the helper, while the arm's decision starts at id resolution and ends at the generic fallback. Each sync arm now holds the lock from find_payment_by_txid through its last write (including TxReplaced, which had the same hole: its pending write embeds a read of the payment record). Since all callers now hold the lock, apply_funding_status_update became apply_funding_status_update_locked, taking a guard reference as a reminder rather than keeping a self-locking twin nobody uses. Two barrier tests pin both orderings by parking one writer inside its critical section before dispatching the other.

[jkczyz]

Comments addressed. Please throw your agents at it again.

[tnull]

Comments addressed. Please throw your agents at it again.

Seems LLM ping-pong is how it's done by now..

[tnull]

Codex:

• P1 — Resolve the payment ID under the new lock. The event handler calls find_payment_by_txid before /home/tnull/workspace/ldk-node-pr-962/src/wallet/mod.rs:1590. If classification is between its payment-store and pending-store writes, a replacement txid misses the pending index and falls
  back to its own txid. After waiting for classification, the update uses that stale ID, fails to find the stable funding record, and creates a duplicate generic payment. The lookup and funding update need one locked operation.

[joostjager]

One additional subcase from my resolved duplicate remains: moving this lookup under the lock fixes the partial-classification window, but a complete pending entry still does not map every candidate. find_payment_by_txid checks the stable ID, current txid, and conflicting_txids, but not p.candidates.

With three or more candidates, a middle candidate that has not reached conflicting_txids still falls back to its own payment ID even after classification has fully completed. Please also match p.candidate(target_txid).is_some() when resolving the stable ID.

[jkczyz]

One additional subcase from my resolved duplicate remains: moving this lookup under the lock fixes the partial-classification window, but a complete pending entry still does not map every candidate. find_payment_by_txid checks the stable ID, current txid, and conflicting_txids, but not p.candidates.

With three or more candidates, a middle candidate that has not reached conflicting_txids still falls back to its own payment ID even after classification has fully completed. Please also match p.candidate(target_txid).is_some() when resolving the stable ID.

🤖 Good catch — find_payment_by_txid now also matches the candidate history. A new test seeds a three-candidate entry and checks the middle candidate (not the record's id, not its current txid, never got its own TxReplaced) resolves to the stable id. A side effect worth noting: TxReplaced can now resolve these ids too, so its "payment already exists" comment was extended to cover classification-authored records.

Codex:

• P1 — Resolve the payment ID under the new lock. The event handler calls find_payment_by_txid before /home/tnull/workspace/ldk-node-pr-962/src/wallet/mod.rs:1590. If classification is between its payment-store and pending-store writes, a replacement txid misses the pending index and falls
  back to its own txid. After waiting for classification, the update uses that stale ID, fails to find the stable funding record, and creates a duplicate generic payment. The lookup and funding update need one locked operation.

🤖 Fixed by the same fixup as above: the id lookup now happens under the lock, so sync can't resolve against a half-written candidate index. funding_confirmation_waits_for_classification reproduces this exact scenario — classification parked between its payment-store and pending-store writes, then the replacement candidate's confirmation dispatched. Before the fix it produced two records; now the confirmation waits and updates the classified record in place.

[tnull]

Codex:

• P1 — Serialize graduation with classification. ChainTipChanged snapshots pending entries and later performs a full /home/tnull/workspace/ldk-node-pr-962/src/wallet/mod.rs:324 without the new cross-store lock. It can snapshot wallet-derived figures, classification can then write the correct
  candidate figures, and graduation can overwrite them from its stale snapshot before removing the pending entry. Because that update carries a confirmation status, keep_confirmed_figures does not protect them. Graduation must either share the lock from snapshot through removal or atomically
  update status only.

[jkczyz]

🤖 Went with your second option: graduation now decides from the live record inside the payment store's mutate closure and writes a status-only update. Since the write carries no figures, txid, or confirmation status, there is nothing a concurrent classification could lose — which also keeps graduation off the cross-store lock (nothing extra in the every-block path). If the live record has diverged from the snapshot, graduation declines and leaves the entry for future events; that arm is hardening (no current writer produces such divergence) but falls out naturally from deciding on live state. Two tests: one pins figure preservation across graduation, one pins the decline.

[joostjager]

Wasn't reviewing this PR yet, but no problem to point my agent at it again. Left its feedback.

[joostjager]

This documentation is stale after moving candidate selection into payment_store.mutate. current is now the state protected by the payment store's mutation lock, not an unlocked snapshot.

[jkczyz]

🤖 Right, that doc predates moving the candidate choice into the mutate closure. Reworded: current is observed inside the payment store's critical section, so it can't go stale against a concurrent confirmation; the confirmed-figures merge rule remains as second-line arbitration rather than the safety argument.

[jkczyz]

Comments addressed. Please throw your agents at it again.

Seems LLM ping-pong is how it's done by now..

Probably should have used buzz...

[jkczyz]

Fixes planned and implemented with Claude.

[jkczyz]

🤖 You're right — the lock only covered the helper, while the arm's decision starts at id resolution and ends at the generic fallback. Each sync arm now holds the lock from find_payment_by_txid through its last write (including TxReplaced, which had the same hole: its pending write embeds a read of the payment record). Since all callers now hold the lock, apply_funding_status_update became apply_funding_status_update_locked, taking a guard reference as a reminder rather than keeping a self-locking twin nobody uses. Two barrier tests pin both orderings by parking one writer inside its critical section before dispatching the other.

[jkczyz]

One additional subcase from my resolved duplicate remains: moving this lookup under the lock fixes the partial-classification window, but a complete pending entry still does not map every candidate. find_payment_by_txid checks the stable ID, current txid, and conflicting_txids, but not p.candidates.

With three or more candidates, a middle candidate that has not reached conflicting_txids still falls back to its own payment ID even after classification has fully completed. Please also match p.candidate(target_txid).is_some() when resolving the stable ID.

🤖 Good catch — find_payment_by_txid now also matches the candidate history. A new test seeds a three-candidate entry and checks the middle candidate (not the record's id, not its current txid, never got its own TxReplaced) resolves to the stable id. A side effect worth noting: TxReplaced can now resolve these ids too, so its "payment already exists" comment was extended to cover classification-authored records.

Codex:

• P1 — Resolve the payment ID under the new lock. The event handler calls find_payment_by_txid before /home/tnull/workspace/ldk-node-pr-962/src/wallet/mod.rs:1590. If classification is between its payment-store and pending-store writes, a replacement txid misses the pending index and falls
  back to its own txid. After waiting for classification, the update uses that stale ID, fails to find the stable funding record, and creates a duplicate generic payment. The lookup and funding update need one locked operation.

🤖 Fixed by the same fixup as above: the id lookup now happens under the lock, so sync can't resolve against a half-written candidate index. funding_confirmation_waits_for_classification reproduces this exact scenario — classification parked between its payment-store and pending-store writes, then the replacement candidate's confirmation dispatched. Before the fix it produced two records; now the confirmation waits and updates the classified record in place.

[jkczyz]

🤖 Went with your second option: graduation now decides from the live record inside the payment store's mutate closure and writes a status-only update. Since the write carries no figures, txid, or confirmation status, there is nothing a concurrent classification could lose — which also keeps graduation off the cross-store lock (nothing extra in the every-block path). If the live record has diverged from the snapshot, graduation declines and leaves the entry for future events; that arm is hardening (no current writer produces such divergence) but falls out naturally from deciding on live state. Two tests: one pins figure preservation across graduation, one pins the decline.

[jkczyz]

🤖 Right, that doc predates moving the candidate choice into the mutate closure. Reworded: current is observed inside the payment store's critical section, so it can't go stale against a concurrent confirmation; the confirmed-figures merge rule remains as second-line arbitration rather than the safety argument.