Skip to content

Keep funding payment records accurate - #1057

Open
jkczyz wants to merge 19 commits into
lightningdevkit:mainfrom
jkczyz:2026-08-funding-payment-bugfixes
Open

jkczyz wants to merge 19 commits into
lightningdevkit:mainfrom
jkczyz:2026-08-funding-payment-bugfixes

Conversation

@jkczyz

@jkczyz jkczyz commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Five bugfixes for funding payment records (channel opens and splices). Found while building the splice-retry work stacked on top (#930's replacement) but independent of it.

  • Only adopt a funding payment's own transactions from wallet sync. Sync adopted the txid and confirmation of any transaction linked to a funding record through its conflicting txids. A cooperative close conflicts with a pending splice in exactly that way, so the splice record could adopt the close's confirmation and graduate as if the splice had confirmed.

  • Fail funding payments lost to a confirmed conflict. With the close's confirmation no longer adopted, a funding payment whose transaction was double-spent stayed Pending forever. It is now marked Failed once a conflict outside its candidate history has confirmed through ANTI_REORG_DELAY while neither its own transaction nor any RBF candidate can still confirm.

  • Retry funding-broadcast classification instead of dropping it. A broadcast whose payment-record classification failed was dropped. For interactive funding the counterparty broadcasts the same transaction anyway, so the drop keeps nothing off-chain — it just leaves the round unrecorded, permanently stranding its confirmation on a duplicate record. Classification is now retried, with the broadcast held back, until it succeeds or the node shuts down. Since splice rounds are now recorded at signing (below), their broadcast writes nothing and is never queued; the retry serves v1 channel opens, which are still recorded at broadcast, and the broadcaster's other record writes.

  • Record splice funding payments when signing. Recording a splice round only when it is broadcast races wallet sync: once tx_signatures are exchanged the counterparty may broadcast first, and sync then files the round under a duplicate record that shadows the funding record from then on. Writing the record while handling FundingTransactionReadyForSigning, before our signatures leave the node, avoids the race. A round recorded that early can still be abandoned before broadcast, so such rounds are dropped once LDK no longer holds them.

  • Resolve funding payments when LDK discards a splice round. A round of ours that LDK gives up on — kept through a close the monitor watched until it matured, or replaced by a sibling round we did not contribute to — stayed Pending forever, because the DiscardFunding handler only reclaimed the contribution's addresses. The event names this node's contribution rather than the round, so the payments are resolved from what LDK holds instead. As the promoted round's ChannelReady is handled, every funding payment of the channel left with no round of ours among the rounds the channel manager still holds, and none promoted before, is failed. ChannelClosed fails the payments a close leaves with no round of ours the channel's monitor still watches and none promoted before, and a DiscardFunding for a channel the manager no longer lists resolves them the same way from the monitor's funding and watched rounds. Each payment records the rounds LDK promoted to the funding, so a zero-conf round promoted once still counts at later discards and at the close. A DiscardFunding for a listed channel only drops a round nothing broadcast and reclaims the contribution's addresses.

Each fix has a test that fails without it; the commit messages have the details.

First of three stacked PRs replacing #930's restart persistence for this release, per the discussion there; #1079 (payment-model groundwork) and #1080 (in-flight splice tracking) follow.

Developed with assistance from Claude Code.

@ldk-reviews-bot

ldk-reviews-bot commented Aug 19, 2026

Copy link
Copy Markdown

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

@Jolah1 Jolah1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third commit: the funding-kind check only matches tx_type: Some(Funding | InteractiveFunding), so the stale untyped record the commit message calls out passes it. An on-chain RBF replacing channel funding stays reachable after this PR, narrower than main, but still a funding double-spend, and it now rides on the rest of the stack landing. Worth its own issue.

Comment thread src/tx_broadcaster.rs Outdated
/// elapses, the node is shutting down and the package is dropped with it.
pub(crate) fn requeue_failed_classify(&self, package: BroadcastPackage) {
let sender = self.queue_sender.clone();
tokio::spawn(async move {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only detached tokio::spawn left in non-test production code outside postgres_store. It's also what reorders the queue — the requeued package lands behind anything queued after it.
Holding the failed package in the loop and adding a sleep branch to the existing select! avoids both, and needs no runtime handle.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the ordering part is fixed at the current head. Suppose candidate A's classification fails and is parked. While A waits, newer candidate B arrives carrying history [A, B] and classifies successfully. When A retries, funding_reclassification_update can rotate the unconfirmed record back to A, while the pending update replaces [A, B] with [A]; A is then broadcast after B.

If B is subsequently observed, it can be treated as foreign and recorded as a duplicate. Could we preserve monotonic candidate history and freshness, with a regression test asserting that the record remains on B with history [A, B] after A retries?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the delta since my last pass. Commit 1 is unchanged apart from the async store conversion; the responses landed as the two f - fixups on top.

@joostjager is right that the ordering isn't fixed, and it's the second half of my own earlier comment: holding the package in the loop removed the detached task but not the reorder a parked package still classifies and
broadcasts after everything queued behind it, so "avoids both" was wrong of me.

I reproduced his A/B case at fb85dd0. The rotation isn't merely possible: both guards that could stop it are Confirmed-only (wallet/mod.rs:2718, payment/store.rs:290), so for an unconfirmed record it always applies, and classify_interactive_funding has no freshness check before persist_funding_payment. Candidate histories only grow, so persist_funding_payment, which already holds the cross-store lock, can read the pending entry and skip when the incoming list is a strict prefix of the stored one. Happy to hand over the regression test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we preserve monotonic candidate history and freshness, with a regression test asserting that the record remains on B with history [A, B] after A retries?

🤖 Done — essentially with @Jolah1's proposal generalized:

Candidate histories only grow, so persist_funding_payment, which already holds the cross-store lock, can read the pending entry and skip when the incoming list is a strict prefix of the stored one.

🤖 Rather than skip strict prefixes in one place, both writes now ignore stale candidate lists: the pending entry's stored list is only ever replaced by a list containing everything already in it, and the record is only updated by a classification whose list contains the record's current txid. A stale retry of A carries [A] — no B — so it changes nothing at either site.

@tnull tnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a rebase unfortunately.

Comment thread src/tx_broadcaster.rs Outdated
/// the counterparty broadcasts it regardless — it would only leave the transaction
/// confirming without a recorded candidate. If the queue has closed by the time the delay
/// elapses, the node is shutting down and the package is dropped with it.
pub(crate) fn requeue_failed_classify(&self, package: BroadcastPackage) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex:

  • [P1] Delayed requeue leaves the duplicate-record race open. /home/tnull/worktrees/ldk-node/pr-1057-review-20260819/src/tx_broadcaster.rs:164 removes the failed package and waits two seconds before requeueing it. If persistence recovers and wallet sync observes an interactive-RBF candidate
    during that interval, sync creates a generic record keyed by the active txid. Classification later creates the funding record keyed by the first candidate, while direct lookup continues to prefer the generic record. The funding record can therefore remain pending—the outcome this commit
    intends to prevent. The test only exercises a single Funding transaction whose payment ID equals its txid, without concurrent wallet sync.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Yeah, the retry only narrows the window — sync can still record the tx under its own txid while classification is failing. The follow-up PR handles that by merging the duplicate into the funding record once classification eventually succeeds. What this PR fixes is the drop: on main, one failure means classification never runs again, so the duplicate is permanent.

Comment thread src/tx_broadcaster.rs Outdated
/// elapses, the node is shutting down and the package is dropped with it.
pub(crate) fn requeue_failed_classify(&self, package: BroadcastPackage) {
let sender = self.queue_sender.clone();
tokio::spawn(async move {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted above, this likely should be spawn_cancellable_background_task. Though given the codex comment above, not even sure if doing it in the background is the right approach?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer applicable.

🤖 I ended up removing the spawn entirely rather than tracking it: the retry is a timer branch in the broadcast loop's select!, so it's cancelled with the loop on stop(). The detached task was also buggier than it looked — its comment claimed a re-send after shutdown would fail because the queue had closed, but the receiver isn't dropped until the Node is, so the send succeeded and a stale package could be broadcast after stop()/start(). Added failed_classification_retry_dies_at_stop for that.

Comment thread src/wallet/mod.rs Outdated
// funding history: its current txid or a classified candidate. A conflicting
// transaction that is neither — a close also spends the funding outpoint — must
// not overwrite the record.
let pending = self.pending_payment_store.get(&payment_id);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex:

  • [P2] Legitimate older candidates are classified as foreign. The gate at /home/tnull/worktrees/ldk-node/pr-1057-review-20260819/src/wallet/mod.rs:1986 accepts only the current txid or a recorded candidate. However, the persisted format explicitly permits an empty candidate list for older
    records at /home/tnull/worktrees/ldk-node/pr-1057-review-20260819/src/payment/pending_payment_store.rs:46. If an earlier RBF candidate exists only in conflicting_txids and confirms, it is treated as foreign, producing a duplicate and leaving the funding record pending.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly not a concern, but the follow-up will fix a gap when we crash.

🤖 To hit this you'd need a funding record with no candidates recorded at all, and I don't think a node can get into that state in practice: the pending store hasn't shipped in a release yet, so only a node that ran a few commits of main at the wrong time could have such a record. I'm also hesitant to loosen the check. A txid that only shows up in conflicting_txids could just as easily be a coop close or a third-party double-spend, and adopting one of those would corrupt the record. What can still go wrong is a crash before a round's classification finishes — nothing retries it after restart. The fix we have in mind is a startup pass that backfills the record's candidates from LDK's splice state; signed rounds survive restart with their txids, so it doesn't need any new persistence.

Comment thread src/wallet/mod.rs Outdated
},
)]);

// Let the loop fail at least one classification round; a failed classification must not

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex:

  • [P2] The retry regression test lacks a failure barrier. /home/tnull/worktrees/ldk-node/pr-1057-review-20260819/src/wallet/mod.rs:4265 sleeps for three seconds but never proves the queue attempted—and failed—classification. If the loop is delayed until writes are re-enabled, the test can
    pass on the pre-fix implementation. The store should signal/count an observed failed write before recovery is enabled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread src/wallet/mod.rs Outdated
// classification re-types records concurrently, and a classification landing after the
// funding-kind check below would let the RBF replace a funding transaction. Acquired
// after the persister, matching the lock order of the wallet sync paths.
let funding_guard = self.funding_payment_update_lock.lock().await;

@tnull tnull Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ngl, it's kind of odd that we now also mix in the funding lock here with the regular RBF flow.

Do we really need to fix this? IIUC, not only does it require the wallet sync racing the LDK classification, it also requires that the user calls bump_fee_rbf on the wrong (i.e., funding transaction) record at exactly the right time, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped. An RBF would need to spend the channel funding output, which isn't part of the wallet. But this still could be a problem for dual-funded channels, once supported. Opened #1072.

@tnull tnull added this to the 0.8 milestone Aug 19, 2026

@tnull tnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, if we now retry classification/broadcast anyways as the counterparty might also broadcast, couldn't we unblock the broadcast queue again, i.e., don't have it block on the persistence succeeding?

@jkczyz
jkczyz force-pushed the 2026-08-funding-payment-bugfixes branch from 6093418 to 9e29da5 Compare August 28, 2026 22:33
@jkczyz

jkczyz commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Third commit: the funding-kind check only matches tx_type: Some(Funding | InteractiveFunding), so the stale untyped record the commit message calls out passes it. An on-chain RBF replacing channel funding stays reachable after this PR, narrower than main, but still a funding double-spend, and it now rides on the rest of the stack landing. Worth its own issue.

@Jolah1 The bump will fail for splices, but will be a problem for dual-funded channels, once supported. Opened #1072.

Btw, if we now retry classification/broadcast anyways as the counterparty might also broadcast, couldn't we unblock the broadcast queue again, i.e., don't have it block on the persistence succeeding?

@tnull 🤖 Only the failing package waits — the queue keeps flowing. True, the counterparty can broadcast regardless; the retry narrows that window and the follow-up merges the duplicate. Broadcasting before recording would just make that race the norm.

@jkczyz
jkczyz force-pushed the 2026-08-funding-payment-bugfixes branch from 9e29da5 to fb85dd0 Compare August 28, 2026 22:44
@jkczyz

jkczyz commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Rebased

@jkczyz jkczyz changed the title Fix three funding payment record bugs Fix two funding payment record bugs Aug 31, 2026

@joostjager joostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixes LGTM aside from the small remarks below.

I do think that this PR and the gaps it leaves open reinforce the value of one consistent commit boundary for state, funding, payment records, and durable broadcast intent.

Comment thread src/chain/mod.rs Outdated
// Packages whose classification failed, each waiting out FAILED_CLASSIFY_RETRY_DELAY
// before its next attempt. New packages keep flowing while these wait, and pending
// retries die with the loop on shutdown rather than resurfacing after a later start.
let mut parked: Vec<(tokio::time::Instant, BroadcastPackage)> = Vec::new();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep classification retries bounded and deduplicated

receiver.recv() continues draining the 256-entry channel while every failed package is appended to this unbounded Vec. For a transaction whose first classification cannot persist, LDK's periodic claim or sweep rebroadcasts can enqueue additional copies while the store remains unavailable. Every copy is then retried and logged, while remove(0) shifts the remaining entries.

A store outage coinciding with a force-close wave can therefore grow memory, CPU, and store load without bound, then produce a duplicate broadcast burst on recovery. Could we keep this bounded and coalesce packages by transaction or package identity, using a VecDeque or equivalent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Done — failed packages now wait in a retry queue capped at the broadcast queue's own size. A package that's already queued isn't added twice; the cap exists for fee bumps — during a store outage LDK keeps re-sending its claims, and each send at a bumped fee is a new txid taking a new slot, so a single claim could grow the queue for as long as the outage lasts. Dropping the oldest entry once the cap is hit is safe because everything non-funding is regenerated on its own schedule (LDK's rebroadcast timer, the sweeper's per-block pass), so only the newest copy matters once the store recovers. Funding packages are exempt and never dropped: nothing re-sends them for us, and the payment record needs every negotiated version in its candidate history. The exemption can't grow the queue on its own — a new funding version only exists when another negotiation with the peer completes, never on a timer.

I did consider having a new package replace whatever queued entry it double-spends — that would size the queue naturally — but Claim and Sweep transactions combine many spends into one, so telling whether two entries are versions of the same transaction means comparing their inputs, with its own edge cases; the cap gets the same behavior with less machinery.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cap and the dedup cover the memory growth and the recovery burst. One thing that's now constant rather than bounded with a fixed 2s delay per package, the queue is re-attempted at cap/delay, so a full queue is roughly 128 classification attempts per second for as long as the store is unavailable, each one a store write and a log_error! from classify_and_broadcast. Against SQLite that's mostly log volume, but with VssStore every attempt is a round trip to the store that's already struggling. Is a backoff worth adding here, or is a constant rate the deliberate choice so recovery gets picked up promptly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the store is struggling (i.e., slow) rather than just being unavailable, we wouldn't be hitting that rate since the queue is processed sequentially. And yes, the constant rate is deliberate: the queue holds time-sensitive claims, so once the store recovers everything retries within ~2s.

Comment thread src/tx_broadcaster.rs Outdated
/// elapses, the node is shutting down and the package is dropped with it.
pub(crate) fn requeue_failed_classify(&self, package: BroadcastPackage) {
let sender = self.queue_sender.clone();
tokio::spawn(async move {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the ordering part is fixed at the current head. Suppose candidate A's classification fails and is parked. While A waits, newer candidate B arrives carrying history [A, B] and classifies successfully. When A retries, funding_reclassification_update can rotate the unconfirmed record back to A, while the pending update replaces [A, B] with [A]; A is then broadcast after B.

If B is subsequently observed, it can be treated as foreign and recorded as a duplicate. Could we preserve monotonic candidate history and freshness, with a regression test asserting that the record remains on B with history [A, B] after A retries?

Comment thread src/chain/mod.rs
async fn classify_and_broadcast(
&self, package: BroadcastPackage,
) -> Result<(), BroadcastPackage> {
if let Err(e) = self.tx_broadcaster.classify_package(&package).await {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why maintain separate immediate and retry paths instead of treating every broadcast as scheduled retryable work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the duplicated code, but kept the paths separate. Now that we have a bounded queue and deduplication, using the same path would mean we'd drop newer packages.

@Jolah1

Jolah1 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Also worth folding in before merge: ebc0086 doesn't compile its tests standalone (list_filter on the bounded payment store), so the series isn't bisectable until the fixups are squashed.

Minor, likely follow-up: after commit 1 declines the close, nothing ever ends the splice record's life — it stays Pending indefinitely. Intended for the payment-model PR i guess

@jkczyz jkczyz changed the title Fix two funding payment record bugs Keep funding payment records accurate Sep 2, 2026
@jkczyz
jkczyz force-pushed the 2026-08-funding-payment-bugfixes branch from fb85dd0 to b15d50d Compare September 2, 2026 23:59
@jkczyz

jkczyz commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Also worth folding in before merge: ebc0086 doesn't compile its tests standalone (list_filter on the bounded payment store), so the series isn't bisectable until the fixups are squashed.

The compilation will be fixed once the fixups are squashed.

Minor, likely follow-up: after commit 1 declines the close, nothing ever ends the splice record's life — it stays Pending indefinitely. Intended for the payment-model PR i guess

Added a commit marking the record Failed once a conflict from outside its candidate history confirms past ANTI_REORG_DELAY while no candidate can still confirm. Opened issue #1078 covering the remaining problems.

@joostjager joostjager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we agreed in the team meeting to press on with ldk-node under the current persistence model, but this PR and the follow-up work are changing my view.

Most of this PR is compensation for not having a consistent commit boundary. Especially now that AI highlights all the edge cases, it becomes increasingly difficult to reason about for a human. And it also becomes clear what we got ourselves into.

I think we should stop trying to force a release on top of this architecture and go back to the drawing board before adding more compensating logic.

Comment thread src/tx_broadcaster.rs Outdated
// periodically, while the incoming package may carry a fresher fee-bumped variant.
// A funding package is never dropped — nothing would re-broadcast it, and losing it
// leaves its transaction confirming without a recorded candidate.
match self.0.iter().position(|(_, _, waiting)| !waiting.contains_funding()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 The deduplication fixes the periodic-growth problem, but the eviction assumption does not hold for every non-funding package. A CooperativeClose goes through classify_regular_broadcast, so a payment-store failure can park it here. rust-lightning emits the fully signed close from a one-shot close path and then removes the channel; the claim and sweeper timers do not recreate it. Once it becomes the oldest non-funding entry, this code can evict it, or refuse it when only protected funding entries are waiting.

That can discard our only local broadcast attempt and leave us dependent on the peer to publish the close. Could eviction be limited to transaction types known to be periodically regenerated, while treating cooperative closes and other one-shot broadcasts as non-droppable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, nothing re-broadcasts a cooperative close. Would it be simpler to just panic if the queue is full? We already panic when ChannelMonitors and ChannelManager persistence fails.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But would a panic be recoverable then because anything still has the tx on disk?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But would a panic be recoverable then because anything still has the tx on disk?

🤖 Depends on the type. Claims and sweeps are on disk — the monitor and sweeper persist and re-broadcast them on their own, which is what made eviction safe for them. The closing tx is on disk nowhere: the channel is removed from the ChannelManager before the broadcaster is even called. What usually saves it is a rewind: if the store is down, the manager persist recording the removal also fails, we already panic on that, and the reloaded manager still has the channel — negotiation restarts on reconnect and broadcasts a fresh closing tx. So a queue-full panic mostly duplicates the persist panic that fired first. Where neither panic helps is a partial failure — manager persists, payment-store writes keep failing: there, only keeping the close in the queue recovers it once the store returns. The latest fixup does that: only claims, sweeps, and anchor bumps can be dropped at the bound now; cooperative closes wait alongside fundings.

Could eviction be limited to transaction types known to be periodically regenerated, while treating cooperative closes and other one-shot broadcasts as non-droppable?

Ended up adding a fixup doing this instead as noted above.

@TheBlueMatt TheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this PR is compensation for not having a consistent commit boundary.

Huh? AFAICT almost none of the code here would be fixed by some god-persistence write. It seems to ~all be due to BDK detecting a transaction on its own.

Comment thread src/tx_broadcaster.rs Outdated
Refused(BroadcastPackage),
}

/// Packages whose classification failed, each waiting out a retry delay before its next attempt.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a queue? Can't we just spawn a tokio task and rebroadcast in a loop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the queue isn't for rebroadcasting. It's for retrying failed persistence, which needs to succeed before broadcasting. Since LDK periodically re-broadcasts claims, if persistence is failing we need to dedup them rather than spawning more tasks.

Do you have any opinion on #1057 (comment)?

@jkczyz

jkczyz commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

I know we agreed in the team meeting to press on with ldk-node under the current persistence model, but this PR and the follow-up work are changing my view.

Most of this PR is compensation for not having a consistent commit boundary. Especially now that AI highlights all the edge cases, it becomes increasingly difficult to reason about for a human. And it also becomes clear what we got ourselves into.

I think we should stop trying to force a release on top of this architecture and go back to the drawing board before adding more compensating logic.

Discussed offline. The last PR in the stack (#1080) now creates the a payment record before signing when processing the FundingTransactionReadyForSigning event. This eliminates most of the edge cases resulting from having the wallet sync pick-up the counterparty's broadcast before we process our own.

@joostjager

Copy link
Copy Markdown
Contributor

Huh? AFAICT almost none of the code here would be fixed by some god-persistence write. It seems to ~all be due to BDK detecting a transaction on its own.

I did not mean one god commit spanning every store. My thinking was that if the creator of the operation performs the classification and commits it along with the rest of its state, the broadcaster would not need the retry queue or ordering logic.

@tnull tnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the first 3 commits so far.

Comment thread src/wallet/mod.rs
continue;
},
};
let mut rounds_of_ours = entry

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex:

  • [P1] Do not infer ownership from the record’s current txid — /home/tnull/worktrees/ldk-node/2026-09-pr-1057-review/src/wallet/mod.rs:943

    rounds_of_ours correctly filters candidates by amount_msat.is_some(), but then unconditionally adds record_txid. Wallet processing can rotate record_txid to any classified candidate—including a counterparty-only candidate with amount_msat == None at /home/tnull/worktrees/ldk-node/2026-09-
    pr-1057-review/src/wallet/mod.rs:2860. If that candidate is observed before its ChannelReady promotion, the promotion sees its txid in held_rounds and incorrectly keeps our funding payment instead of failing it. Only add record_txid separately when it is absent from the candidate history;
    when present, its recorded contribution must determine ownership.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Not reachable today, but tightened anyway. The record's txid is set when signing a round that we contributed to — interactive_funding_record returns None otherwise. The only writer that moves it onto another candidate is apply_funding_status_update_locked, which runs on a wallet event for that candidate. BDK derives wallet events from the transactions that its script-pubkey index finds relevant: those spending an output that it tracks or paying one of our script pubkeys. A round that we did not contribute to spends the funding outpoint and the counterparty's inputs and pays the new funding output and the counterparty's change — so it never shows up in a wallet event, and the record's txid never moves onto a round that we did not contribute to. That relies on how BDK filters transactions, and we don't check that anywhere. Rather than depend on it, the fixup counts the record's transaction only when it isn't among the recorded candidates. When it is, it counts only if we contributed to that round. The new tests move the record onto the counterparty's round through a TxUnconfirmed event and then promote and close.

Comment thread src/wallet/mod.rs
.await?
{
continue;
FundingStatusUpdate::Applied => continue,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ngl, this gets increasingly gets confusing as now we lookup the entry that would be the right one for 'normal' payment/RBF handling, then we override it if we detect its funding related (3 cases), then we proceed with the original flow.

Maybe it would be easier to follow if we'd refactor this to classify once and then act once respectively? Though, doesn't need to happen in this PR. Mostly noting that it gets harder and harder to follow what's going on and what edge cases there are.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look into this as a follow-up. Claude thinks it is doable:

🤖 Agreed, and it is the same change your store-operations comment asks for (#1057 (comment)). Each arm resolves an id, lets the funding path override it, re-reads the record to skip a settled funding payment, then runs the generic write. One payment_store.mutate per resolved id can see the record and pick between refreshing a funding record, skipping a settled one, and inserting or merging a generic one, re-run once under the txid id when the transaction turns out to be foreign to the resolved record. That also takes a new transaction from three backend reads to one. Follow-up, as you suggest; here I only removed the extra read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #1113 but haven't looked at it myself yet.

Comment thread src/chain/mod.rs Outdated
// Packages whose classification failed, each waiting out FAILED_CLASSIFY_RETRY_DELAY
// before its next attempt. New packages keep flowing while these wait, and pending
// retries die with the loop on shutdown rather than resurfacing after a later start.
let mut retries = RetryQueue::new();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding a second queue on top, can we make the single queue do prioritization so that it skips entries that failed classification but keeps them in the queue or similar?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1057 (comment).

The mpsc channel only allows push and pop, so we wouldn't be able to de-dup against LDK's periodic claim broadcasts if we re-queued. Or did you have something else in mind?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we could also replace the channel with an Arc<BroadcastQueue> that handles both in a single queue exposing respective APIs instead of having a channel and a RetryQueue? Honestly not quite sure if the former will be much simpler, but it might be easier to reason about kept state (i.e., memory footprint etc)?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC that's the same question as @joostjager had in 8371ae6#r3913031526

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Claude decided to make two internal queues inside BroadcastQueue so that fresh packages are attempted before retries. Its rationale is that some packages don't require a write, so they shouldn't wait behind ones that need a retry because persistence failure.

The new code is better encapsulated, but I'm not sure how much simpler it really is. WDYT?

Comment thread src/chain/mod.rs Outdated
loop {
let tx_bcast_logger = Arc::clone(&self.logger);
tokio::select! {
let next_retry_at = retries.next_retry_at();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need a next_retry_at delay? Couldn't we just biased the select so that retries are only processed once the earlier arms (i.e., current broadcasts) are exhausted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added biased, but do we want to immediately retry? If the store is failing fast, we'd just be filling our log with errors.

Comment thread src/tx_broadcaster.rs Outdated

/// The packaged transactions' txids in sorted order, identifying the package's effect on
/// chain: two packages with the same txids broadcast the same transactions.
pub(crate) fn sorted_txids(&self) -> Vec<Txid> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this follow the same rules as into_sorted_transactions? Otherwise having two very similarly named helpers with very different effects could be a footgun going forward.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is for de-duping, not broadcast order. Changed to use a BTreeSet and renamed to txids.

Comment thread src/wallet/mod.rs
/// conflict is final for them. The liveness check guards the case where the conflict
/// double-spent only one round of the negotiation: as long as some candidate — including one
/// classification hasn't recorded yet — can still confirm, the record must stay pending.
async fn fail_funding_payment_lost_to_conflict(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this method adds yet another store operation. Codex now calls 5 backend operations (3 reads, 2 writes) for every single transaction being synced.

It seems that this will become a considerable problem (if not a DoS vector) for nodes operating against remote storage (VSS, Postgres), especially since we now hold funding_payment_update_lock for many operations. So we might end up blocking many parts of the node while sync is ongoing, in particular also broadcasting (note we'll only see a persistence failure after exhausting retries in the VSS case for instance).

Yes, we might be able to lean a bit on the recently-introduced LRU cache, but generally we need to find an approach that allows classification in a single place and then write the result once rather than accessing the payment store so often during sync / broadcast.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this method adds yet another store operation. Codex now calls 5 backend operations (3 reads, 2 writes) for every single transaction being synced.

🤖 Dropped it. The funding-status check has already read the record under the resolved id, so its outcome says whether a settled funding record sits there; only a fallback to a different id still needs a read.

we need to find an approach that allows classification in a single place and then write the result once rather than accessing the payment store so often during sync / broadcast.

Agreed. Follow-up, together with #1057 (comment).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #1113 but haven't looked at it myself yet.

@jkczyz
jkczyz force-pushed the 2026-08-funding-payment-bugfixes branch from 4fd10b8 to 7b60504 Compare September 18, 2026 04:01
jkczyz and others added 4 commits September 21, 2026 15:53
Wallet sync resolves a funding payment's id for any transaction linked
to the record through its conflicting txids, and then adopted that
transaction's txid and confirmation outright. A cooperative close
conflicts with a pending splice in exactly that way: the splice record
would report the close's txid and confirmation under its
InteractiveFunding type and contribution figures and graduate as if
the splice had confirmed, while the close's own record never received
its confirmation. Adopt a transaction only when it is part of the
payment's funding history — the record's current txid or a classified
candidate. Anything else is recorded under its own txid-keyed id,
which also delivers the close's confirmation to the close's own
record.

Generated with assistance from Claude Code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A queued broadcast whose payment-record classification failed was
dropped outright, on the theory that broadcasting a transaction we
failed to record would leave it on-chain without a payment. For
interactive funding that theory doesn't hold: the counterparty
broadcasts the same transaction once the signature exchange completes,
so dropping the package keeps nothing off-chain -- it only guarantees
the round is never recorded as a candidate on our side. The
funding-status ownership gate then treats the round's confirmation as
foreign to the funding record and re-keys it to a stray duplicate
record, which shadows the funding record's txid lookups permanently:
the splice payment stays Pending forever while an untyped duplicate
holds the confirmation.

Keep the package alive instead: retry classification after a short
delay, holding the broadcast back until it succeeds. Other packages
keep flowing while a retry waits, and pending retries are dropped when
the node stops -- a retry that outlived a stop would classify and
broadcast a stale package after a later start. Classification failures
are persistence failures, so there is no limit on attempts -- a store
that never recovers keeps the node from functioning anyway -- and
every failed round is logged.

The waiting packages are deduplicated and bounded. LDK re-broadcasts
pending claims every 30 seconds and regenerates sweeps once per block
until they confirm, so over a long store outage a copy per rebroadcast
would otherwise pile up and replay as a burst on recovery. A package
whose transactions already await a retry is not queued again. At the
bound, an incoming package that LDK would re-broadcast anyway makes room
by dropping the oldest such waiting package, whose transactions return
with the next rebroadcast; if every waiting package is one nothing
re-broadcasts, the incoming package is dropped instead. Fundings and
cooperative closes are never dropped to make room and never refused at
the bound, since nothing re-broadcasts them: a dropped funding would
leave its transaction confirming without a recorded candidate, and a
dropped cooperative close might lose the only copy of the signed closing
transaction. Fee-bumped rebroadcasts carry new txids, so the bound, not
the deduplication, is what limits their accumulation.

Generated with assistance from Claude Code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Poll the broadcast queue ahead of due retries, so packages arriving during a
store outage are classified before the outage's retries rather than in random
order.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Name a package's set of txids for what it is: an identity for deduplication
and logging, not a broadcast order like the topologically sorted transactions
next to it.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jkczyz
jkczyz force-pushed the 2026-08-funding-payment-bugfixes branch from 7b60504 to 4f82716 Compare September 22, 2026 03:34
@jkczyz

jkczyz commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Rebased now that #1112 was merged to bump the LDK dependencies to 0.3 RC2.

@tnull tnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more comments. Unfortunately still think that the complexity increase here/required by the split classification through BroadcasterInterface is very hard to follow / unfeasible to maintain going forward. Working on a PoC draft for an alternative approach, but not sure we can still switch for LDK v0.3.

Comment thread src/chain/mod.rs
// without it, an empty channel would retry a fast-failing store back to back,
// logging an error each time.
biased;
_ = stop_tx_bcast_receiver.changed() => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex:

  • High — shutdown loses non-rebroadcastable transactions. /home/tnull/workspace/ldk-node-pr-1057-review-20260922/src/chain/mod.rs:623 immediately returns on shutdown, dropping the local retry queue. That queue can contain cooperative-close transactions after a persistence failure, even
    though /home/tnull/workspace/ldk-node-pr-1057-review-20260922/src/tx_broadcaster.rs:71 explicitly notes that nothing regenerates them after their channel leaves ChannelManager. Stopping during a store outage can therefore permanently lose this node’s only copy of a signed close
    transaction. Non-droppable retries need to survive stop/restart or be durably stored.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed when merging the queues into BroadcastQueue.

Comment thread src/chain/mod.rs Outdated
// Packages whose classification failed, each waiting out FAILED_CLASSIFY_RETRY_DELAY
// before its next attempt. New packages keep flowing while these wait, and pending
// retries die with the loop on shutdown rather than resurfacing after a later start.
let mut retries = RetryQueue::new();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC that's the same question as @joostjager had in 8371ae6#r3913031526

Comment thread src/wallet/mod.rs Outdated
// payment store back as it was while the record is still pending, or the replayed event
// would find the half-written record and take it for prior state.
let prior_details = self.payment_store.get(&payment_id).await?;
if let Err(e) = self.persist_funding_payment_locked(&guard, details, recorded).await {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this get/persist (i.e., mutate) pair is another opportunity to save an IOP, e.g., if persist_funding_payment_locked would return prior_details in an error variant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread src/wallet/mod.rs Outdated
self.pending_payment_store.remove(&payment_id).await?;
log_info!(
self.logger,
"Dropped abandoned splice round(s) {:?} and funding payment {} with them",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this log would confuse me if I read it. Do we already log on abandoning the round in LDK? Do we need to log here at all then, let alone on INFO?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded and made debug level.

@jkczyz jkczyz Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And LDK and LDK Node's SpliceNegotiationFailed handler logs at info level, but the latter doesn't say anything about the payment record.

Comment thread src/wallet/mod.rs Outdated
@@ -1588,8 +2089,7 @@ impl Wallet {
// A promoted-but-unconfirmed 0conf splice comes back through this generic path re-typed
// and carrying wallet-view figures; `funding_reclassification_update` declines the
// downgrade, leaving no trace that a re-broadcast arrived. Log the arrival so tests can
// observe the traffic. The read cannot go stale: only the broadcast loop writes
// interactive-funding classifications, and it runs this classification too.
// observe the traffic; the read serves the log line alone, so a stale read costs no more.
if let Some(current) = self.payment_store.get(&payment_id).await? {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preexisting, but here we seem to access the store just to log. Would be good to avoid that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread src/wallet/mod.rs Outdated
if abandoned_txids.contains(txid)
)
};
let record = self.payment_store.get(&payment_id).await?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we can avoid this get by moving hands_back determination it into the mutate below?

@jkczyz jkczyz Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Done. hands_back is now computed inside the payment store's mutate closure, from the record the closure is given, so that path no longer reads the record first. The removal path still reads first here because mutate cannot remove a record. Once #1080 adds remove_if, a fixup there moves that check into remove_if as well, and the drop pass no longer reads a record before writing it.

jkczyz and others added 15 commits September 22, 2026 09:28
Queue fresh packages and those awaiting a classification retry together,
in the broadcaster rather than in the task draining them.

One queue means one bound and one rule for what may be dropped at it:
until now a full queue dropped whichever package arrived next, a funding
or a cooperative close included, while only the retries spared the
packages nothing re-broadcasts. A re-broadcast of a package awaiting a
retry is recognized as it is queued, rather than after one more failed
write and error log per copy over a store outage. And since the queue
outlives the task, a package awaiting a retry when the node stops is
classified and broadcast after the next start, as a package not yet
attempted always was; a funding package has no other way back.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Since declining to adopt a conflicting close's confirmation, a funding
payment whose transaction was double-spent stayed Pending forever --
nothing wrote a terminal status for an on-chain record -- and the sync
loop kept re-queueing the dead transaction for rebroadcast on every
tip change.

Mark such a record Failed once a conflict from outside its candidate
history has confirmed through ANTI_REORG_DELAY while neither its own
transaction nor any RBF candidate can still confirm, mirroring the
anti-reorg finality the Succeeded transition already assumes. Removing
the payment's pending entry then stops the re-queueing.

Settling also removes the entry that maps candidate txids to the
record, so a later wallet event for a dead candidate falls back to
keying by that candidate's txid -- which, for the first candidate, is
the record's own id. Skip such events rather than let the generic
handling resurrect the settled record, and let a replayed replacement
event finish an entry removal a crash interrupted instead of stamping
the terminal status into the leftover entry.

Implemented with Claude Code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Drop the extra payment store read per synced transaction. The funding-status check has already read the record under the resolved id, so when that id is the transaction's own, whether a settled funding record sits there is known without another read. Only a fallback from a different id still reads, and a transaction the pending store maps to no funding record reads nothing more before the generic write.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Wallet sync can learn of a splice transaction before broadcast-time
classification records it: once tx_signatures are exchanged, the
counterparty may broadcast first, and sync then files the round under a
duplicate record keyed by its txid, which shadows the funding record's
txid lookups from then on. Retrying a failed classification only
narrows that window: a round the counterparty broadcasts is still
observed before our record exists.

Record the funding payment while handling
FundingTransactionReadyForSigning, before funding_transaction_signed
hands our signatures to LDK. The counterparty cannot broadcast without
them, so the record precedes anything wallet sync can observe, and every
later observer resolves to it. The record is written in full from the
channel's pending splice history, so the round's broadcast has nothing
left to record and records nothing. If the record cannot be written, the
event is replayed rather than proceeding unrecorded: LDK re-offers it
in-session and regenerates it across restarts while the transaction
remains unsigned. A failed write leaves no half-written record behind
for the replayed event to build on. Should undoing it fail as well, the
replayed event removes what was left of a first round once the round is
gone from the channel's history; the leftovers of a bump live under an
earlier round's record, which wallet sync moves on as that round
confirms or fails.

Recording before the round is negotiated means a recorded round can
still be abandoned: the counterparty may abort after we sign but before
its commitment_signed, or the channel may close, and until LDK has
released our signatures nothing can ever broadcast the transaction. Left
in place, the record would wait forever on a payment nothing can
confirm. The signed round is therefore marked as awaiting broadcast
until LDK reports the splice negotiated, which it does as it hands the
fully signed round to the broadcaster: from then on the counterparty
holds our signatures and can broadcast on its own. If the mark cannot be
cleared, that event is replayed as well. A marked round is dropped once
LDK no longer holds it, unless the wallet has seen its transaction: the
counterparty may broadcast a round it received our signatures for while
LDK still waits on its own. A round whose negotiation LDK has reported
keeps its place whether or not wallet sync has seen it yet, and so does
the channel's current funding: a zero-conf splice becomes the funding as
soon as splice_locked is exchanged, before its transaction confirms or
LDK's report of its negotiation has necessarily been handled. Dropping a
round leaves the record on the last remaining round this node
contributed to, moving it there if it still names the dropped round, or
removes the record when none remains. LDK's view is consulted when it
reports the failed negotiation of a channel it still lists, when the
channel closes -- a round awaiting the counterparty's signatures gets no
failure report then, and a failure reported once the channel is gone is
resolved by what this report carries, the channel's last funding, and by
the rounds its monitor still watches -- and at startup, before any
background task runs: LDK reports the loss of a negotiation its last
channel manager write carried mid-way, but a round committed, negotiated
and signed since that write gets no report if the node stops before the
next one. The channel manager forgets a closed channel's pending rounds,
but its monitor keeps watching every round the counterparty's
commitment_signed reached, and our signatures cannot have left the node
before that message: the counterparty may hold the fully signed
transaction and broadcast it, as when this node's contributed input
value is the smaller and its tx_signatures therefore go first, so such a
round is kept for wallet sync to resolve should it confirm, while a
marked round the monitor never watched is dropped, as nothing can
broadcast it. A round already missing from the channel's history when
the signing event is handled is not recorded at all.

Rounds without a local contribution emit no signing event and are not
recorded at broadcast either, as before; they are left to wallet sync.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the comments say LDK reports for a splice round still
awaiting the counterparty's signatures when its channel closes, and when
`SpliceNegotiated` is emitted, so they hold once the pinned LDK carries
the fix for rust-lightning issue 4967
(https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/4967):
the channel manager then reports such rounds after `ChannelClosed`, and
emits `SpliceNegotiated` at a force-close for a round whose
`tx_signatures` were ready to send, without any broadcast. At both
revisions the event means our signatures were ready to send, so the
counterparty may hold them; the comments no longer say it does. The
test of a round the monitor never watched notes what LDK reports for
it once fixed.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The pinned LDK now carries the fix for rust-lightning issue 4967
(https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/4967),
so the comments no longer hedge on when the channel manager reports a
splice round still awaiting the counterparty's signatures at the close,
and the test of a round the monitor never watched asserts what LDK
reports for it after `ChannelClosed`: the `SpliceNegotiationFailed` the
node passes on and the `DiscardFunding` whose handling reclaims node A's
addresses.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Take the payment store's prior record from the funding write's own read, instead of reading it once more just before the write: the rollback of a failed write pair puts that record back, and a write that failed before reading has nothing to roll back.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Decide whether a record still waits on the abandoned rounds inside the write that hands the remaining round back, from the record found there, instead of reading the record first; only a removal, which has no such critical section, still reads. The entry's copy of a record that graduated meanwhile is now left alone on both paths, where before that depended on whether the graduation landed before or after the read.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Say what became of a record whose abandoned rounds were never broadcast, and log both drops at debug: LDK and the event handler already report the failed negotiation at info, and what the wallet then did with its records is detail beneath that.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A splice round this node signed is kept at `ChannelClosed` when the
channel's monitor watches it: the counterparty committed to it, so our
signatures may have left the node, and the counterparty may broadcast
the round and see it confirm. A close the wallet sees as a conflict --
a cooperative close spending an input the round shares -- fails the
payment once it confirms beyond the reorg depth, but nothing resolved
such a record when a commitment transaction, which pays no wallet
script, won instead. Once the close matures -- after the reorg delay
for a counterparty's commitment transaction, and once the to_self_delay
on our balance has passed for one of our own -- the monitor stops
watching the rounds it kept and queues a `DiscardFunding` event for
each, and the handler only reclaimed the contribution's addresses: the
funding payment stayed `Pending` forever. Likewise for a round of ours
that a sibling round this node did not contribute to replaced on an
open channel: LDK discards our round as the sibling locks, and the
payment stayed `Pending` for a transaction that can no longer confirm.

Resolve the channel's funding payments by the rounds LDK holds. A round
nothing ever broadcast is dropped first, as `ChannelClosed` already
did, and with it a record no broadcast round of ours remains under. A
payment is then left alone if a round of ours that LDK still holds
remains in its record -- the round that locked, or one still pending --
or one LDK promoted to the funding before, and failed otherwise: no
round of ours can confirm anymore, whether the channel closed on a
commitment transaction or a round we did not contribute to locked. The
rounds LDK holds are the channel's pending rounds and funding while the
manager lists the channel, and once it does not, the funding its
monitor settled on plus whatever the monitor still watches. The monitor
is left out for a listed channel: its updates land after the manager's,
deferred to the background processor's flush, so it may still watch a
round the manager let go.

The event names this node's contribution, not the round: the inputs and
output scripts LDK returns of it. Matching that to a recorded round
would take the parts of every contribution on record. LDK discards the
round's siblings as it promotes the round and reports the promotion
through `ChannelReady`, so that event resolves the payments of a listed
channel instead: it records the promotion and resolves the channel's
other payments by the rounds the manager holds once updated -- the
promoted round, and whatever was negotiated behind it. For a channel
the manager no longer lists it records the promotion alone and leaves
the payments to the close. A `DiscardFunding` for a listed channel then
only drops a round nothing broadcast that the manager no longer holds
and reclaims the contribution's addresses.

A zero-conf splice is promoted to the funding as `splice_locked` is
exchanged, before its transaction confirms, and a later splice moves the
funding on again: at the close neither the manager nor the monitor holds
the earlier round, although it can still confirm, the later round
descending from it. So the funding payment records each promotion LDK
reports through `ChannelReady`, and a round promoted once counts as one
that can confirm wherever the rounds LDK holds decide: as a sibling
round is promoted, and when the channel closes.

The monitor's events can reach the handler ahead of the channel's
`ChannelClosed` when one sync delivers the close and its maturity: the
channel manager polls the monitor's report of the close at the start of
each event pass and on peer traffic, and the monitor's own events are
handled right after the manager's. Each event then finds the channel
still listed and leaves the payments, there being no promotion to
resolve them. So `ChannelClosed` fails every payment of the channel
left with no round of ours the monitor watches and none promoted
before, and a `DiscardFunding` event for a channel the manager no
longer lists resolves each record the same way, by the funding its
monitor settled on and whatever it still watches.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Say which rounds a closed channel's monitor watches: those the
background processor has flushed to it, as LDK Node's `ChainMonitor`
defers updates to that flush. A round whose `commitment_signed` the
manager processed since the last flush therefore looks unwatched at
`ChannelClosed` and is dropped from its record, which is right: our
`tx_signatures` are released only once the monitor update has been
persisted, so the counterparty cannot broadcast such a round. The test
of a round the monitor watches notes how its end state changes once
the pinned LDK carries the fix for rust-lightning issue 4967, which
reports `SpliceNegotiated` for the round at the close.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Count the record's own transaction as a round of ours only when no candidate records it. Wallet sync moves the record onto whichever of its candidates it sees, so once it names a round we did not contribute to, a promotion or close holding that round kept the payment pending although no round of ours could confirm. Such a round pays and spends nothing of ours, so the wallet should never see it and the record should never move onto one; the rule now holds without relying on that.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The pinned LDK now reports `SpliceNegotiated` after `ChannelClosed` for
a splice round whose `tx_signatures` this node had sent when the channel
was force-closed (rust-lightning issue 4967), so the test of a round the
monitor watches asserts that report and, at maturity, that the payment
fails rather than being dropped as one nothing broadcast: the
counterparty holds the fully signed round and may broadcast it, as the
test's tail shows.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t as a conflict

A pending-store entry lists the transactions that replaced its own, so a cooperative close (or any other wallet transaction) that a splice round replaces lists the round among its conflicting txids. The round's events then matched two entries, its own record's and the close's, and the pending cache's iteration order decided which one won. About one time in five the round's confirmation landed on the close's record, which took the round's txid, figures and confirmation and graduated, while the splice's payment never learned of the confirmation and stayed pending for good.

Prefer the entry that records the transaction as its own, whether as its current transaction or as a negotiated candidate, and fall back to an entry that only lists it as a conflict when no entry owns it. The conflict listing stays: it is how a replaced round of a record without candidates, an ordinary payment's RBF history or the replacement of an inbound transaction, maps back to its record.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Classifying a funding-typed broadcast read the payment store only to log that a re-broadcast of a promoted splice had met its interactive-funding record, then read it again inside the write. The write hands back what it found, so the log comes from that read and a channel-open funding costs one read fewer.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jkczyz
jkczyz force-pushed the 2026-08-funding-payment-bugfixes branch from 26afff9 to 5a889fc Compare September 22, 2026 19:58
@jkczyz
jkczyz requested a review from tnull September 22, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants