Keep funding payment records consistent across sync and classification

[jkczyz]

Funding broadcasts are classified into payment records off the broadcaster's queue, which runs concurrently with wallet sync — and can run after sync has already recorded the transaction, for instance when the counterparty's broadcast of a shared funding transaction is observed first. The two writers raced: a late classification could overwrite confirmation state that wallet sync had already advanced, sync could observe a half-written classification and record a duplicate generic payment, and graduation could roll back figures a concurrent classification had just written.

This makes each writer's decision and writes atomic against the others:

• Classification merges only the transaction type and our contribution figures into an existing record, leaving the confirmation state that wallet-sync events own in place. Once a record is confirmed, its txid and figures describe the candidate that actually confirmed and are kept on a late classification, except when the update names the confirmed txid itself.
• Wallet sync resolves a transaction to its funding record before deciding how to record it — including transactions known only as earlier RBF candidates — and serializes with classification's two-store write pair from that resolution through its final write.
• Graduation decides from the live record and updates only the payment status, so a stale snapshot cannot roll back concurrently written figures.
• A missing pending-store entry is recreated while the payment is still Pending, repairing the index after a crash or failed write between the two stores.

Raised by Codex in the review of #888 and hardened through subsequent review rounds.

Developed with assistance from Claude Code.

[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]

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.

[joostjager]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

[tnull]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

Does it also suggest how to implement this? Any of these options seem vastly invasive and over-engineered, and likely would mean yet-another persisted state just to keep track of whats-to-be-persisted?

[joostjager]

AI agrees with you that it is over-engineered and that we need a diff persistence model to really fix it.

[jkczyz]

🤖 The window is real, but narrower than it reads, and I'd rather fix it structurally in a follow-up than add repair machinery here.

Two things bound it today. If the payment record survives but the pending entry is lost, the first confirmation of the recorded txid recreates the entry (the tail write in apply_funding_status_update_locked), so anything still on its first candidate self-repairs. And after a crash, LDK re-delivers the broadcast itself whenever the manager hadn't yet persisted the completed signing session: channel_reestablish carries next_funding, the peer re-sends tx_signatures, and we re-broadcast — so classification runs again with the full candidate history. What stays broken needs an RBF'd splice, a failure between the two writes, an already-persisted signing session, and a counterparty broadcast — and the cost is a duplicate record, or one reporting the wrong candidate's amount/fee, not funds.

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

[jkczyz]

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

[tnull]

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

Hmm, feel free to explore it, but there are quite a few PRs that lean on/influence pending payment store still. Note that the biggest difference is also that PaymentDetails are meant to be user facing (and we don't wan to show all fields there necessarily), and pending payment store is kept in memory, also after #1024, while there we'll start reading payment store entries from disk (mod an in-memory cache) through paginated listing.

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

[joostjager]

My agent is almost happy. Just #962 (comment) and it flags PR description for not being fully accurate.

[tnull]

Codex:

• [P1] Reconcile sync-created RBF records before inserting — /home/tnull/worktrees/ldk-node/pr-962-latest-20260807/src/wallet/mod.rs:1490

  Interactive funding anchors details.id to the first candidate, while wallet sync’s generic fallback identifies a transaction by the active candidate’s txid. If wallet sync finishes before classification for an RBF candidate, mutate(&details.id) sees no record and inserts a second payment
  instead of reclassifying the existing active-txid record. Subsequent confirmation lookup favors that generic pending record, leaving duplicate payments and the classified record unable to advance correctly. The sync-first regression test uses payment_id == active_txid, so it misses this
  case; it should cover distinct first and active candidates.

[tnull]

Let's consider to take this in a follow up to keep things moving?

[jkczyz]

🤖 Sounds good. The persistence follow-up floated in #962 (comment) would cover this structurally — identity would resolve through a txid→PaymentId index before any insert — and its tests should include the distinct first/active candidate case Codex flagged.

[tnull]

Feel free to squash, #962 (comment) could happen in a follow-up if we deem it important.

[tnull]

Needs a rebase now.

[jkczyz]

Rebased

[tnull]

Landing this to keep making progress, but now opened #1044 to track known issues. @jkczyz let me know there if you agree with these / have anything to add.

[jkczyz]

Funding broadcasts are classified into payment records off the broadcaster's queue, which runs concurrently with wallet sync — and can run after sync has already recorded the transaction, for instance when the counterparty's broadcast of a shared funding transaction is observed first. The two writers raced: a late classification could overwrite confirmation state that wallet sync had already advanced, sync could observe a half-written classification and record a duplicate generic payment, and graduation could roll back figures a concurrent classification had just written.

This makes each writer's decision and writes atomic against the others:

• Classification merges only the transaction type and our contribution figures into an existing record, leaving the confirmation state that wallet-sync events own in place. Once a record is confirmed, its txid and figures describe the candidate that actually confirmed and are kept on a late classification, except when the update names the confirmed txid itself.
• Wallet sync resolves a transaction to its funding record before deciding how to record it — including transactions known only as earlier RBF candidates — and serializes with classification's two-store write pair from that resolution through its final write.
• Graduation decides from the live record and updates only the payment status, so a stale snapshot cannot roll back concurrently written figures.
• A missing pending-store entry is recreated while the payment is still Pending, repairing the index after a crash or failed write between the two stores.

Raised by Codex in the review of #888 and hardened through subsequent review rounds.

Developed with assistance from Claude Code.

[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]

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.

[joostjager]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

[tnull]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

Does it also suggest how to implement this? Any of these options seem vastly invasive and over-engineered, and likely would mean yet-another persisted state just to keep track of whats-to-be-persisted?

[joostjager]

AI agrees with you that it is over-engineered and that we need a diff persistence model to really fix it.

[jkczyz]

🤖 The window is real, but narrower than it reads, and I'd rather fix it structurally in a follow-up than add repair machinery here.

Two things bound it today. If the payment record survives but the pending entry is lost, the first confirmation of the recorded txid recreates the entry (the tail write in apply_funding_status_update_locked), so anything still on its first candidate self-repairs. And after a crash, LDK re-delivers the broadcast itself whenever the manager hadn't yet persisted the completed signing session: channel_reestablish carries next_funding, the peer re-sends tx_signatures, and we re-broadcast — so classification runs again with the full candidate history. What stays broken needs an RBF'd splice, a failure between the two writes, an already-persisted signing session, and a counterparty broadcast — and the cost is a duplicate record, or one reporting the wrong candidate's amount/fee, not funds.

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

[jkczyz]

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

[tnull]

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

Hmm, feel free to explore it, but there are quite a few PRs that lean on/influence pending payment store still. Note that the biggest difference is also that PaymentDetails are meant to be user facing (and we don't wan to show all fields there necessarily), and pending payment store is kept in memory, also after #1024, while there we'll start reading payment store entries from disk (mod an in-memory cache) through paginated listing.

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

[joostjager]

My agent is almost happy. Just #962 (comment) and it flags PR description for not being fully accurate.

[tnull]

Codex:

• [P1] Reconcile sync-created RBF records before inserting — /home/tnull/worktrees/ldk-node/pr-962-latest-20260807/src/wallet/mod.rs:1490

  Interactive funding anchors details.id to the first candidate, while wallet sync’s generic fallback identifies a transaction by the active candidate’s txid. If wallet sync finishes before classification for an RBF candidate, mutate(&details.id) sees no record and inserts a second payment
  instead of reclassifying the existing active-txid record. Subsequent confirmation lookup favors that generic pending record, leaving duplicate payments and the classified record unable to advance correctly. The sync-first regression test uses payment_id == active_txid, so it misses this
  case; it should cover distinct first and active candidates.

[tnull]

Let's consider to take this in a follow up to keep things moving?

[jkczyz]

🤖 Sounds good. The persistence follow-up floated in #962 (comment) would cover this structurally — identity would resolve through a txid→PaymentId index before any insert — and its tests should include the distinct first/active candidate case Codex flagged.

[tnull]

Feel free to squash, #962 (comment) could happen in a follow-up if we deem it important.

[tnull]

Needs a rebase now.

[jkczyz]

Rebased

[tnull]

Landing this to keep making progress, but now opened #1044 to track known issues. @jkczyz let me know there if you agree with these / have anything to add.

[jkczyz]

Funding broadcasts are classified into payment records off the broadcaster's queue, which runs concurrently with wallet sync — and can run after sync has already recorded the transaction, for instance when the counterparty's broadcast of a shared funding transaction is observed first. The two writers raced: a late classification could overwrite confirmation state that wallet sync had already advanced, sync could observe a half-written classification and record a duplicate generic payment, and graduation could roll back figures a concurrent classification had just written.

This makes each writer's decision and writes atomic against the others:

• Classification merges only the transaction type and our contribution figures into an existing record, leaving the confirmation state that wallet-sync events own in place. Once a record is confirmed, its txid and figures describe the candidate that actually confirmed and are kept on a late classification, except when the update names the confirmed txid itself.
• Wallet sync resolves a transaction to its funding record before deciding how to record it — including transactions known only as earlier RBF candidates — and serializes with classification's two-store write pair from that resolution through its final write.
• Graduation decides from the live record and updates only the payment status, so a stale snapshot cannot roll back concurrently written figures.
• A missing pending-store entry is recreated while the payment is still Pending, repairing the index after a crash or failed write between the two stores.

Raised by Codex in the review of #888 and hardened through subsequent review rounds.

Developed with assistance from Claude Code.

[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]

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.

[joostjager]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

[tnull]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

Does it also suggest how to implement this? Any of these options seem vastly invasive and over-engineered, and likely would mean yet-another persisted state just to keep track of whats-to-be-persisted?

[joostjager]

AI agrees with you that it is over-engineered and that we need a diff persistence model to really fix it.

[jkczyz]

🤖 The window is real, but narrower than it reads, and I'd rather fix it structurally in a follow-up than add repair machinery here.

Two things bound it today. If the payment record survives but the pending entry is lost, the first confirmation of the recorded txid recreates the entry (the tail write in apply_funding_status_update_locked), so anything still on its first candidate self-repairs. And after a crash, LDK re-delivers the broadcast itself whenever the manager hadn't yet persisted the completed signing session: channel_reestablish carries next_funding, the peer re-sends tx_signatures, and we re-broadcast — so classification runs again with the full candidate history. What stays broken needs an RBF'd splice, a failure between the two writes, an already-persisted signing session, and a counterparty broadcast — and the cost is a duplicate record, or one reporting the wrong candidate's amount/fee, not funds.

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

[jkczyz]

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

[tnull]

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

Hmm, feel free to explore it, but there are quite a few PRs that lean on/influence pending payment store still. Note that the biggest difference is also that PaymentDetails are meant to be user facing (and we don't wan to show all fields there necessarily), and pending payment store is kept in memory, also after #1024, while there we'll start reading payment store entries from disk (mod an in-memory cache) through paginated listing.

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

[joostjager]

My agent is almost happy. Just #962 (comment) and it flags PR description for not being fully accurate.

[tnull]

Codex:

• [P1] Reconcile sync-created RBF records before inserting — /home/tnull/worktrees/ldk-node/pr-962-latest-20260807/src/wallet/mod.rs:1490

  Interactive funding anchors details.id to the first candidate, while wallet sync’s generic fallback identifies a transaction by the active candidate’s txid. If wallet sync finishes before classification for an RBF candidate, mutate(&details.id) sees no record and inserts a second payment
  instead of reclassifying the existing active-txid record. Subsequent confirmation lookup favors that generic pending record, leaving duplicate payments and the classified record unable to advance correctly. The sync-first regression test uses payment_id == active_txid, so it misses this
  case; it should cover distinct first and active candidates.

[tnull]

Let's consider to take this in a follow up to keep things moving?

[jkczyz]

🤖 Sounds good. The persistence follow-up floated in #962 (comment) would cover this structurally — identity would resolve through a txid→PaymentId index before any insert — and its tests should include the distinct first/active candidate case Codex flagged.

[tnull]

Feel free to squash, #962 (comment) could happen in a follow-up if we deem it important.

[tnull]

Needs a rebase now.

[jkczyz]

Rebased

[tnull]

Landing this to keep making progress, but now opened #1044 to track known issues. @jkczyz let me know there if you agree with these / have anything to add.

[jkczyz]

Funding broadcasts are classified into payment records off the broadcaster's queue, which runs concurrently with wallet sync — and can run after sync has already recorded the transaction, for instance when the counterparty's broadcast of a shared funding transaction is observed first. The two writers raced: a late classification could overwrite confirmation state that wallet sync had already advanced, sync could observe a half-written classification and record a duplicate generic payment, and graduation could roll back figures a concurrent classification had just written.

This makes each writer's decision and writes atomic against the others:

• Classification merges only the transaction type and our contribution figures into an existing record, leaving the confirmation state that wallet-sync events own in place. Once a record is confirmed, its txid and figures describe the candidate that actually confirmed and are kept on a late classification, except when the update names the confirmed txid itself.
• Wallet sync resolves a transaction to its funding record before deciding how to record it — including transactions known only as earlier RBF candidates — and serializes with classification's two-store write pair from that resolution through its final write.
• Graduation decides from the live record and updates only the payment status, so a stale snapshot cannot roll back concurrently written figures.
• A missing pending-store entry is recreated while the payment is still Pending, repairing the index after a crash or failed write between the two stores.

Raised by Codex in the review of #888 and hardened through subsequent review rounds.

Developed with assistance from Claude Code.

[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]

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.

[joostjager]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

[tnull]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

Does it also suggest how to implement this? Any of these options seem vastly invasive and over-engineered, and likely would mean yet-another persisted state just to keep track of whats-to-be-persisted?

[joostjager]

AI agrees with you that it is over-engineered and that we need a diff persistence model to really fix it.

[jkczyz]

🤖 The window is real, but narrower than it reads, and I'd rather fix it structurally in a follow-up than add repair machinery here.

Two things bound it today. If the payment record survives but the pending entry is lost, the first confirmation of the recorded txid recreates the entry (the tail write in apply_funding_status_update_locked), so anything still on its first candidate self-repairs. And after a crash, LDK re-delivers the broadcast itself whenever the manager hadn't yet persisted the completed signing session: channel_reestablish carries next_funding, the peer re-sends tx_signatures, and we re-broadcast — so classification runs again with the full candidate history. What stays broken needs an RBF'd splice, a failure between the two writes, an already-persisted signing session, and a counterparty broadcast — and the cost is a duplicate record, or one reporting the wrong candidate's amount/fee, not funds.

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

[jkczyz]

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

[tnull]

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

Hmm, feel free to explore it, but there are quite a few PRs that lean on/influence pending payment store still. Note that the biggest difference is also that PaymentDetails are meant to be user facing (and we don't wan to show all fields there necessarily), and pending payment store is kept in memory, also after #1024, while there we'll start reading payment store entries from disk (mod an in-memory cache) through paginated listing.

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

[joostjager]

My agent is almost happy. Just #962 (comment) and it flags PR description for not being fully accurate.

[tnull]

Codex:

• [P1] Reconcile sync-created RBF records before inserting — /home/tnull/worktrees/ldk-node/pr-962-latest-20260807/src/wallet/mod.rs:1490

  Interactive funding anchors details.id to the first candidate, while wallet sync’s generic fallback identifies a transaction by the active candidate’s txid. If wallet sync finishes before classification for an RBF candidate, mutate(&details.id) sees no record and inserts a second payment
  instead of reclassifying the existing active-txid record. Subsequent confirmation lookup favors that generic pending record, leaving duplicate payments and the classified record unable to advance correctly. The sync-first regression test uses payment_id == active_txid, so it misses this
  case; it should cover distinct first and active candidates.

[tnull]

Let's consider to take this in a follow up to keep things moving?

[jkczyz]

🤖 Sounds good. The persistence follow-up floated in #962 (comment) would cover this structurally — identity would resolve through a txid→PaymentId index before any insert — and its tests should include the distinct first/active candidate case Codex flagged.

[tnull]

Feel free to squash, #962 (comment) could happen in a follow-up if we deem it important.

[tnull]

Needs a rebase now.

[jkczyz]

Rebased

[tnull]

Landing this to keep making progress, but now opened #1044 to track known issues. @jkczyz let me know there if you agree with these / have anything to add.

[jkczyz]

Funding broadcasts are classified into payment records off the broadcaster's queue, which runs concurrently with wallet sync — and can run after sync has already recorded the transaction, for instance when the counterparty's broadcast of a shared funding transaction is observed first. The two writers raced: a late classification could overwrite confirmation state that wallet sync had already advanced, sync could observe a half-written classification and record a duplicate generic payment, and graduation could roll back figures a concurrent classification had just written.

This makes each writer's decision and writes atomic against the others:

• Classification merges only the transaction type and our contribution figures into an existing record, leaving the confirmation state that wallet-sync events own in place. Once a record is confirmed, its txid and figures describe the candidate that actually confirmed and are kept on a late classification, except when the update names the confirmed txid itself.
• Wallet sync resolves a transaction to its funding record before deciding how to record it — including transactions known only as earlier RBF candidates — and serializes with classification's two-store write pair from that resolution through its final write.
• Graduation decides from the live record and updates only the payment status, so a stale snapshot cannot roll back concurrently written figures.
• A missing pending-store entry is recreated while the payment is still Pending, repairing the index after a crash or failed write between the two stores.

Raised by Codex in the review of #888 and hardened through subsequent review rounds.

Developed with assistance from Claude Code.

[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]

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.

[joostjager]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

[tnull]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

Does it also suggest how to implement this? Any of these options seem vastly invasive and over-engineered, and likely would mean yet-another persisted state just to keep track of whats-to-be-persisted?

[joostjager]

AI agrees with you that it is over-engineered and that we need a diff persistence model to really fix it.

[jkczyz]

🤖 The window is real, but narrower than it reads, and I'd rather fix it structurally in a follow-up than add repair machinery here.

Two things bound it today. If the payment record survives but the pending entry is lost, the first confirmation of the recorded txid recreates the entry (the tail write in apply_funding_status_update_locked), so anything still on its first candidate self-repairs. And after a crash, LDK re-delivers the broadcast itself whenever the manager hadn't yet persisted the completed signing session: channel_reestablish carries next_funding, the peer re-sends tx_signatures, and we re-broadcast — so classification runs again with the full candidate history. What stays broken needs an RBF'd splice, a failure between the two writes, an already-persisted signing session, and a counterparty broadcast — and the cost is a duplicate record, or one reporting the wrong candidate's amount/fee, not funds.

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

[jkczyz]

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

[tnull]

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

Hmm, feel free to explore it, but there are quite a few PRs that lean on/influence pending payment store still. Note that the biggest difference is also that PaymentDetails are meant to be user facing (and we don't wan to show all fields there necessarily), and pending payment store is kept in memory, also after #1024, while there we'll start reading payment store entries from disk (mod an in-memory cache) through paginated listing.

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

[joostjager]

My agent is almost happy. Just #962 (comment) and it flags PR description for not being fully accurate.

[tnull]

Codex:

• [P1] Reconcile sync-created RBF records before inserting — /home/tnull/worktrees/ldk-node/pr-962-latest-20260807/src/wallet/mod.rs:1490

  Interactive funding anchors details.id to the first candidate, while wallet sync’s generic fallback identifies a transaction by the active candidate’s txid. If wallet sync finishes before classification for an RBF candidate, mutate(&details.id) sees no record and inserts a second payment
  instead of reclassifying the existing active-txid record. Subsequent confirmation lookup favors that generic pending record, leaving duplicate payments and the classified record unable to advance correctly. The sync-first regression test uses payment_id == active_txid, so it misses this
  case; it should cover distinct first and active candidates.

[tnull]

Let's consider to take this in a follow up to keep things moving?

[jkczyz]

🤖 Sounds good. The persistence follow-up floated in #962 (comment) would cover this structurally — identity would resolve through a txid→PaymentId index before any insert — and its tests should include the distinct first/active candidate case Codex flagged.

[tnull]

Feel free to squash, #962 (comment) could happen in a follow-up if we deem it important.

[tnull]

Needs a rebase now.

[jkczyz]

Rebased

[tnull]

Landing this to keep making progress, but now opened #1044 to track known issues. @jkczyz let me know there if you agree with these / have anything to add.

[jkczyz]

Funding broadcasts are classified into payment records off the broadcaster's queue, which runs concurrently with wallet sync — and can run after sync has already recorded the transaction, for instance when the counterparty's broadcast of a shared funding transaction is observed first. The two writers raced: a late classification could overwrite confirmation state that wallet sync had already advanced, sync could observe a half-written classification and record a duplicate generic payment, and graduation could roll back figures a concurrent classification had just written.

This makes each writer's decision and writes atomic against the others:

• Classification merges only the transaction type and our contribution figures into an existing record, leaving the confirmation state that wallet-sync events own in place. Once a record is confirmed, its txid and figures describe the candidate that actually confirmed and are kept on a late classification, except when the update names the confirmed txid itself.
• Wallet sync resolves a transaction to its funding record before deciding how to record it — including transactions known only as earlier RBF candidates — and serializes with classification's two-store write pair from that resolution through its final write.
• Graduation decides from the live record and updates only the payment status, so a stale snapshot cannot roll back concurrently written figures.
• A missing pending-store entry is recreated while the payment is still Pending, repairing the index after a crash or failed write between the two stores.

Raised by Codex in the review of #888 and hardened through subsequent review rounds.

Developed with assistance from Claude Code.

[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]

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.

[joostjager]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

[tnull]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

Does it also suggest how to implement this? Any of these options seem vastly invasive and over-engineered, and likely would mean yet-another persisted state just to keep track of whats-to-be-persisted?

[joostjager]

AI agrees with you that it is over-engineered and that we need a diff persistence model to really fix it.

[jkczyz]

🤖 The window is real, but narrower than it reads, and I'd rather fix it structurally in a follow-up than add repair machinery here.

Two things bound it today. If the payment record survives but the pending entry is lost, the first confirmation of the recorded txid recreates the entry (the tail write in apply_funding_status_update_locked), so anything still on its first candidate self-repairs. And after a crash, LDK re-delivers the broadcast itself whenever the manager hadn't yet persisted the completed signing session: channel_reestablish carries next_funding, the peer re-sends tx_signatures, and we re-broadcast — so classification runs again with the full candidate history. What stays broken needs an RBF'd splice, a failure between the two writes, an already-persisted signing session, and a counterparty broadcast — and the cost is a duplicate record, or one reporting the wrong candidate's amount/fee, not funds.

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

[jkczyz]

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

[tnull]

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

Hmm, feel free to explore it, but there are quite a few PRs that lean on/influence pending payment store still. Note that the biggest difference is also that PaymentDetails are meant to be user facing (and we don't wan to show all fields there necessarily), and pending payment store is kept in memory, also after #1024, while there we'll start reading payment store entries from disk (mod an in-memory cache) through paginated listing.

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

[joostjager]

My agent is almost happy. Just #962 (comment) and it flags PR description for not being fully accurate.

[tnull]

Codex:

• [P1] Reconcile sync-created RBF records before inserting — /home/tnull/worktrees/ldk-node/pr-962-latest-20260807/src/wallet/mod.rs:1490

  Interactive funding anchors details.id to the first candidate, while wallet sync’s generic fallback identifies a transaction by the active candidate’s txid. If wallet sync finishes before classification for an RBF candidate, mutate(&details.id) sees no record and inserts a second payment
  instead of reclassifying the existing active-txid record. Subsequent confirmation lookup favors that generic pending record, leaving duplicate payments and the classified record unable to advance correctly. The sync-first regression test uses payment_id == active_txid, so it misses this
  case; it should cover distinct first and active candidates.

[tnull]

Let's consider to take this in a follow up to keep things moving?

[jkczyz]

🤖 Sounds good. The persistence follow-up floated in #962 (comment) would cover this structurally — identity would resolve through a txid→PaymentId index before any insert — and its tests should include the distinct first/active candidate case Codex flagged.

[tnull]

Feel free to squash, #962 (comment) could happen in a follow-up if we deem it important.

[tnull]

Needs a rebase now.

[jkczyz]

Rebased

[tnull]

Landing this to keep making progress, but now opened #1044 to track known issues. @jkczyz let me know there if you agree with these / have anything to add.

[jkczyz]

Funding broadcasts are classified into payment records off the broadcaster's queue, which runs concurrently with wallet sync — and can run after sync has already recorded the transaction, for instance when the counterparty's broadcast of a shared funding transaction is observed first. The two writers raced: a late classification could overwrite confirmation state that wallet sync had already advanced, sync could observe a half-written classification and record a duplicate generic payment, and graduation could roll back figures a concurrent classification had just written.

This makes each writer's decision and writes atomic against the others:

• Classification merges only the transaction type and our contribution figures into an existing record, leaving the confirmation state that wallet-sync events own in place. Once a record is confirmed, its txid and figures describe the candidate that actually confirmed and are kept on a late classification, except when the update names the confirmed txid itself.
• Wallet sync resolves a transaction to its funding record before deciding how to record it — including transactions known only as earlier RBF candidates — and serializes with classification's two-store write pair from that resolution through its final write.
• Graduation decides from the live record and updates only the payment status, so a stale snapshot cannot roll back concurrently written figures.
• A missing pending-store entry is recreated while the payment is still Pending, repairing the index after a crash or failed write between the two stores.

Raised by Codex in the review of #888 and hardened through subsequent review rounds.

Developed with assistance from Claude Code.

[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]

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.

[joostjager]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

[tnull]

AI: The four new fixups address the live locking, candidate lookup, graduation, and documentation findings. This crash-consistency issue appears unchanged, though: persist_funding_payment still commits the payment record before the pending entry, while a classification error causes the already-dequeued in-memory broadcast package to be discarded.

Please add a durable repair mechanism, atomic record, retry/requeue, or startup reconciliation, or explain why a counterparty broadcast after the partial write cannot leave the payment permanently without its pending index and candidate history.

Does it also suggest how to implement this? Any of these options seem vastly invasive and over-engineered, and likely would mean yet-another persisted state just to keep track of whats-to-be-persisted?

[joostjager]

AI agrees with you that it is over-engineered and that we need a diff persistence model to really fix it.

[jkczyz]

🤖 The window is real, but narrower than it reads, and I'd rather fix it structurally in a follow-up than add repair machinery here.

Two things bound it today. If the payment record survives but the pending entry is lost, the first confirmation of the recorded txid recreates the entry (the tail write in apply_funding_status_update_locked), so anything still on its first candidate self-repairs. And after a crash, LDK re-delivers the broadcast itself whenever the manager hadn't yet persisted the completed signing session: channel_reestablish carries next_funding, the peer re-sends tx_signatures, and we re-broadcast — so classification runs again with the full candidate history. What stays broken needs an RBF'd splice, a failure between the two writes, an already-persisted signing session, and a counterparty broadcast — and the cost is a duplicate record, or one reporting the wrong candidate's amount/fee, not funds.

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

[jkczyz]

As for the real fix: agreed that retry queues or reconciliation state would be over-engineered. What I'd propose in a follow-up instead is removing the second record: move the candidate history and conflicting txids onto PaymentDetails (one record, one atomic write), delete the pending store and the cross-store lock outright, and keep an in-memory txid→PaymentId map built during the existing load-everything pass at startup. That's a net deletion, and it also covers #962 (comment) and keeps txids of graduated payments resolvable (a gap the splice work runs into). Since no release persists pending_payments yet, doing it before the next release keeps it migration-free.

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

[tnull]

Do we want to consider this approach of using a single payment store? I can explore this approach today. #930 changes splices to generate a payment id upon initiation, so that needs to be considered but possibly could be done independently.

Hmm, feel free to explore it, but there are quite a few PRs that lean on/influence pending payment store still. Note that the biggest difference is also that PaymentDetails are meant to be user facing (and we don't wan to show all fields there necessarily), and pending payment store is kept in memory, also after #1024, while there we'll start reading payment store entries from disk (mod an in-memory cache) through paginated listing.

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

[joostjager]

My agent is almost happy. Just #962 (comment) and it flags PR description for not being fully accurate.

[tnull]

Codex:

• [P1] Reconcile sync-created RBF records before inserting — /home/tnull/worktrees/ldk-node/pr-962-latest-20260807/src/wallet/mod.rs:1490

  Interactive funding anchors details.id to the first candidate, while wallet sync’s generic fallback identifies a transaction by the active candidate’s txid. If wallet sync finishes before classification for an RBF candidate, mutate(&details.id) sees no record and inserts a second payment
  instead of reclassifying the existing active-txid record. Subsequent confirmation lookup favors that generic pending record, leaving duplicate payments and the classified record unable to advance correctly. The sync-first regression test uses payment_id == active_txid, so it misses this
  case; it should cover distinct first and active candidates.

[tnull]

Let's consider to take this in a follow up to keep things moving?

[jkczyz]

🤖 Sounds good. The persistence follow-up floated in #962 (comment) would cover this structurally — identity would resolve through a txid→PaymentId index before any insert — and its tests should include the distinct first/active candidate case Codex flagged.

[tnull]

Feel free to squash, #962 (comment) could happen in a follow-up if we deem it important.

[tnull]

Needs a rebase now.

[jkczyz]

Rebased

[tnull]

Landing this to keep making progress, but now opened #1044 to track known issues. @jkczyz let me know there if you agree with these / have anything to add.