Keep funding payment records accurate by jkczyz · Pull Request #1057 · lightningdevkit/ldk-node

jkczyz · 2026-08-19T02:26:11Z

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 · 2026-08-19T02:26:13Z

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

Jolah1

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 · 2026-08-19T09:36:07Z

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


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.

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?

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.

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

This needs a rebase unfortunately.

tnull · 2026-08-19T13:14:02Z

+	/// 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) {


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.

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

tnull · 2026-08-19T13:16:31Z

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


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?

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.

tnull · 2026-08-19T13:16:57Z

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


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.

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.

tnull · 2026-08-19T13:17:33Z

+			},
+		)]);
+
+		// Let the loop fail at least one classification round; a failed classification must not


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.

tnull · 2026-08-19T13:28:22Z

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


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?

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

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 · 2026-08-28T22:33:42Z

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 · 2026-08-28T22:44:32Z

Rebased

joostjager

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.

joostjager · 2026-09-02T09:39:54Z

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


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

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

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?

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.

joostjager · 2026-09-02T09:40:33Z

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


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?

joostjager · 2026-09-02T10:13:22Z

+	async fn classify_and_broadcast(
+		&self, package: BroadcastPackage,
+	) -> Result<(), BroadcastPackage> {
+		if let Err(e) = self.tx_broadcaster.classify_package(&package).await {


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

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 · 2026-09-02T16:44:35Z

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 · 2026-09-03T00:00:01Z

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

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.

joostjager · 2026-09-03T11:58:59Z

+			// 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()) {


🤖 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?

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.

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

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

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.

TheBlueMatt · 2026-09-03T16:46:40Z

+	Refused(BroadcastPackage),
+}
+
+/// Packages whose classification failed, each waiting out a retry delay before its next attempt.


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

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 · 2026-09-04T04:30:30Z

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 · 2026-09-04T08:24:49Z

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.

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>

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>

What a transaction is, is known only to the channel that produced it, and only while the event announcing it is being handled. Record that knowledge durably, keyed by transaction id, so it is still available whenever the transaction is looked at later. Facts are immutable and merged rather than replaced, because several channel events describe the same transaction from different angles: re-recording what is already known writes nothing, so an event handler may replay freely, while a report contradicting a recorded fact is rejected and logged rather than overwriting it. Co-Authored-By: HAL 9000

The channel events that hand a transaction over are the only place this node learns what that transaction is; record it there, so the knowledge outlives the handler. A funding transaction this node builds is recorded before LDK is allowed to release it, because the event is regenerated rather than persisted: recording afterwards could lose the outpoint to a crash. Sweeps are recorded once the sweeper holds the outputs; anchor bumps and HTLC claims once the bump handler has been handed the event, whose outcome it does not report. In both cases a failed write is logged rather than reported, so that bookkeeping can never withhold a claim. The remaining channel events record the same funding outpoints a second time as a backstop, which the merge absorbs. Outputs the sweeper is told to leave alone are left out of the record as well: LDK reports an output paying a script of this wallet's own, such as the closing output of a cooperative close, as a static output, and whatever spends it next is an ordinary wallet transaction, not a sweep. Recording it would label that transaction a sweep and refuse to fee-bump it. Co-Authored-By: HAL 9000 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

A funding transaction this node generates is withheld from LDK until its facts are on record, so a failed write replays the event and the channel becomes pending only once the write goes through. Every other report accompanies a transaction already released, so its failure is logged and the event proceeds. Cover both with a store whose writes to the facts namespace fail while the test says so. This change was made with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

What a transaction is follows from what this node's channels said about it and about the transactions it spends from, so derive it there rather than from the tag its broadcast carried: a tag describes one broadcast, while the facts describe the transaction and survive re-broadcasts and replacements unchanged. A funding output spent in a shape no channel produces stays unnamed. Guessing would put a classification on a payment record that nothing later corrects, and an unnamed record is the honest answer. Co-Authored-By: HAL 9000

…ng output What a transaction spends settles its type before what it creates, so moving a channel's resolved output into a new funding output is the closed channel's sweep. Pin that order with a test of its own. This change was made with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Wallet sync recorded every on-chain transaction as unclassified, leaving what the transaction is to the classification the broadcast queue wrote separately. Name it from the recorded facts instead, so the record wallet sync creates already says what its transaction is. The facts live behind an async store while the record is built under the wallet lock, so each caller reads them first and passes them in, looking up only the transactions the inputs actually reference. A transaction the wallet sees before its channel reports it is named on a later chain tip, from the same scan that graduates confirmed payments. The retry only names a record that is still unnamed, read inside the payment store's critical section, so it can add a name but never replace one. Where a producer reported this node's share of an interactively negotiated funding, that share describes the payment better than the wallet's view does, which reads a shared funding input as wholly this node's. Co-Authored-By: HAL 9000

Wallet sync classifies on-chain payments from recorded provenance and owns the payment record. The second classifier, which ran on the broadcaster's queue and had to hold a broadcast back until its record was persisted, is now redundant: it wrote records sync would write anyway, under merge rules that existed only to keep the two writers from clobbering each other. Broadcasting no longer waits on persistence, so the queue needs neither a bound nor a handle on the wallet: it is a plain FIFO that the chain source drains and sends. The LDK-supplied transaction type is ignored on arrival. The classifier's helpers go with it: the per-candidate stake aggregation, the confirmed-figures guard on payment updates, and DataStore::mutate_async, which only its two-store write pair called. The wallet-view derivation of a transaction's figures stays for the test that checks a reported share outranks it. Tests deleted with their subjects: - zero_conf_splice_{out,in}_funding_rebroadcast_canary, together with the rust-lightning#4878 TODO they pin. They assert log lines emitted by the funding-over-interactive-funding guards, which are gone; with no tag to re-type, the upstream behaviour they watch is unobservable. - funding_reclassification_* and funding_classification_*, plus transaction_type_from_ldk_variants: their subjects are funding_reclassification_update, PaymentDetailsUpdate:: funding_reclassification, the confirmed-figures guard and the LdkTransactionType conversion. - funding_confirmation_waits_for_classification and funding_classification_waits_for_wallet_sync: race tests between classification's two-store write pair and a sync arm. There is no second writer left to race. - classify_funding's own tests, including its rebroadcast handling. - mutate_async_awaits_fallible_reads, with its subject. Tests re-expressed rather than deleted: the funding-record fixture the conflict and graduation tests build on now writes the payment record and its pending entry the way wallet sync does instead of calling the deleted classification path. The queue's arrival order and its wake-on-push get tests of their own. Co-Authored-By: HAL 9000 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The channel monitor's claims on a counterparty's commitment, resolving an HTLC or punishing a revoked commitment, pay this wallet's destination script directly rather than an output the sweeper takes charge of. Since classification stopped reading the broadcast tag, the event handler never learned which channel these transactions belonged to, so their payments came out untyped where the broadcast-time classification had typed them. LDK reports each such output as a static spendable output once the claim matures. The event handler now records the transaction creating it as the channel's payment straight to this wallet, and a transaction that creates such an output without spending a recorded funding output is typed as a claim. A cooperative close pays its shutdown output the same way: with its funding on record it is a close, as before, and without one it is left untyped rather than mistaken for a claim. The report arrives at the depth the claim's payment record graduates at, and the chain-tip pass names only records still pending. The event handler therefore names the transaction's record as it records a channel's report, instead of leaving a graduated record untyped for good. This change was made with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

LDK reports the output a claim paid this wallet at the very depth the claim's payment record graduates at, and polling Bitcoin Core hands each block to the wallet before the channel monitor. The record therefore graduates unnamed and only the naming the event handler runs after recording the report gives the claim its type. Cover that route through the event itself: a held HTLC claimed on chain after the counterparty force-closes. This change was made with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The RBF gate refused a payment whose recorded type named a funding transaction and allowed everything else, so a record carrying no type at all -- one wallet sync wrote before it could name the transaction, or one from a node version that predates classification -- passed as an ordinary payment and could have its replacement broadcast behind LDK's back. Decide it the other way round: allow the bump only when the recorded facts make nothing of the transaction and every input is an output this wallet owns and can re-sign. A transaction that spends a funding, anchor, HTLC or spendable output is refused by its inputs alone, since the wallet holds none of those. A v1 funding transaction this node built from its own coins spends only wallet outputs, so for it the refusal rests on the Funding fact that FundingGenerationReady records before the transaction is released, an event that is replayed if the write fails. A fact that cannot be read is no answer about the transaction: the bump is refused with the error rather than allowed for want of a reason to refuse it. The confirmation, direction and payment-kind checks are unchanged. This change was made with the help of an AI tool. Co-Authored-By: HAL 9000 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Wallet sync can observe a splice transaction before this node has recorded anything about it: once tx_signatures are exchanged, the counterparty may broadcast first, and sync then files the round as a plain on-chain payment of its own, with the wallet's view of a funding output both parties own as its figures. Record the round 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. What is recorded is what the round is: an interactive funding of its channels, this node's share of it and the funding payment it belongs to, all under the round's transaction id, plus the round's place in the channel's splice history. No payment record is written: wallet sync creates one when it observes the transaction and resolves its identity through the recorded facts, so sync stays the only creator of funding payment records. A pending-store entry therefore tracks a splice before any payment record exists, and names the channels of the funding it tracks, since with no record nothing else says which channel's splice a signed round belongs to. An entry left tracking nothing is removed. 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. Both writes are idempotent, and a replay adopts the figures already on record rather than deriving a second answer the facts would refuse. 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 round would sit in the channel's record forever. The signed round is therefore marked as awaiting broadcast until LDK reports the splice negotiated, which it does once our tx_signatures were ready to send, normally as it hands the fully signed round to the broadcaster: from then on the counterparty may hold our signatures and 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. LDK's view is consulted when it reports the failed negotiation of a channel it still lists, when the channel closes, 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. A round this node contributed nothing to is not recorded here and is left to wallet sync, as before. An entry written before this change does not decode under the new layout, and a stale entry fails node startup. The pending store has not been in a release, so no released node holds one. Developed with assistance from Claude Code. Co-Authored-By: Elias Rohrer <dev@tnull.de> 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. An entry of a round LDK released that wallet sync never observed holds no payment record yet. It is resolved on the same terms: with no round of ours held or locked, the attempt is failed under a record written for it then, from the share of its newest round of ours that the signing recorded, so that it shows in the payment list as one wallet sync had observed would, and the entry is removed. Wallet sync otherwise creates every funding record; here it never saw the transaction, so the record is written from what the signing kept. That newest round may have no share on record: the signing records nothing for a round that moves no wallet funds, such as a splice-out to an external address, while a later round signed with it in the history lists it as ours by its contribution. Such a round was never a payment of the wallet's, so once the later round is dropped the entry is removed without a record. A share that cannot be read is no answer about the round: the pass fails with the error, and the event is replayed with the entry kept, still listing the round. Developed with assistance from Claude Code. Co-Authored-By: Elias Rohrer <dev@tnull.de> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

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. A round this node signed is named by the facts the signing recorded before either entry is consulted, so the order decides for a round without such facts, as one this node contributed nothing to. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The writers of funding payment records take a lock guard so that a caller has to hold the funding lock to reach them. The parameter took a guard of any `Mutex<()>`, though, and the wallet has another one, for refilling the address pool, so a caller holding the wrong lock compiled. Wrap the lock in a type whose guard only it can produce and have the writers take that guard, so holding this lock is the only way to call them. Whether the caller's reads before the write happened under the same acquisition is still up to the caller. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The writers of funding payment records take the funding lock's guard, but the stores are fields of the wallet and a writer can still call them directly. Three did, with no lock: on-chain payment graduation, the naming of a recorded transaction once its channel's facts arrive, and the fee bump. Move both stores and the lock into one type. Writes are methods of the guard the lock hands out, so a write compiles only for a holder of the lock; reads take no lock. The three writers take the lock too. Graduation was kept off it on purpose, since its status-only write could clobber nothing a concurrent writer wrote; it locks now so that the API needs no unlocked write, per payment, because the conflict check in the same loop takes the lock itself. The fee bump locks after the wallet persister, the order wallet sync takes the two locks in. Developed with assistance from Claude Code. Co-Authored-By: Elias Rohrer <dev@tnull.de> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

What a transaction's facts record names its payment from the moment a round is signed, which is before wallet sync creates the record and for as long as the facts are kept after `remove_payment` has taken it away. Those facts describe a transaction that happened and still classify later ones, so a bookkeeping removal leaves them where they are. Resolving a replaced transaction can therefore name a payment nothing holds a record of. That now skips the event, as a transaction resolving to no payment at all already did, rather than failing: the failure abandoned every remaining event of the batch, and the wallet's own view of the chain went unpersisted with it, discarding an ordinary sync. Co-Authored-By: HAL 9000

The store of what this node's channels reported about the transactions they produced grew for the lifetime of the node: nothing ever removed a record, so a node kept evidence about channels it had settled years ago. A transaction's record now goes once every use this node has for it is over: nothing has been learned about the transaction for about a year, none of the channels it names is still held by the channel manager, the chain monitor or the output sweeper, no pending payment still refers to it, and every spend the wallet holds of a channel funding it records is confirmed to twice the depth that counts as safe from a reorg, with its own payment settled. Any one of those keeps the record, and the wallet keeps everything while it cannot reach the node's channel state at all, so the loss of that view is never mistaken for a node with no channels. A funding the wallet holds no spend of does not keep it: a commitment transaction paying none of the wallet's scripts never enters the wallet's graph, so a channel closed that way would otherwise keep its record for good, and whether the channel may still produce a transaction is already answered by whether the node still holds it. The check shares the chain tip pass that graduates payments and resumes where the previous tip left it, so it costs one page of records a block however large the store is, and it runs after the pass has named what it could. A record is dropped only while it still is the one the check looked at, since a producer may have reported something about the transaction in between. Because a payment is classified when its transaction is observed, expiring a record never takes a classification back. It means a transaction of a long-resolved channel, met for the first time after its evidence expired, is reported without one -- which the public API now says. Co-Authored-By: HAL 9000 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The test's comment said the spender's record had yet to learn what the transaction is, but the record it inserts is typed already. What keeps the facts is that the pending store still refers to the spender. This change was made with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Records of what a channel reported about its transactions are dropped only once that channel has resolved, so between two of those passes a counterparty decides how much this node stores: how many HTLCs it puts on a commitment transaction, and how many channels and negotiated fundings it drives. A record is now refused once it would outgrow what one record may take up, and a transaction this node holds no record of at all is refused once the store holds as many records as it may. What this node already took on is still kept up to date however full the store is, so an obligation is never half-kept; refusing is only ever about taking on a new one. The store's size comes from the walk the dropping pass already makes: it visits every record over consecutive chain tips, so the count it arrives at is the store's own, without a second pass over it and without holding an index of every transaction in memory. A refusal is reported as an incomplete record rather than as a failure. There is nothing to retry -- a replay would meet the same full store -- and the cost is a transaction reported without a classification, which is bounded loss of detail rather than a lost write. Co-Authored-By: HAL 9000

A channel's facts are recorded by the event that creates its outputs: the funding when the channel is opened, the outputs a channel resolved to this node when LDK reports them. A node upgraded from a version without the facts store holds channels none of those events will fire for again, and a report that failed in an earlier session is not repeated either. A close or a sweep of such a channel then goes unclassified for good. On start, before anything syncs, the node now records what LDK still holds for its channels: the funding output of every channel the channel manager lists or the chain monitor watches, and every output the sweeper tracks for a channel. A node whose producers reported everything finds each of those on record already and writes nothing. A failure to record one costs that output's classification until the next start, not the start itself. The channel manager alone knows a channel's local identifier; a monitor and the sweeper report without it. A fact reported without the identifier therefore no longer contradicts one recorded with it: the identifier fills in where absent and counts only where both reports carry one. This change was made with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The unreachable-state test asserted an empty namespace against a state that held nothing, which an empty answer would satisfy as well as the unreachable one. The state now holds an output while unreachable, and the pass records it once the state can be consulted. The held-outputs test asserted the reported funding's outputs unchanged, which a same-content rewrite would satisfy too. The pass now runs at a later tip and the whole record, its date included, is asserted equal. This change was made with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

ldk-reviews-bot requested a review from tnull August 19, 2026 02:36

jkczyz mentioned this pull request Aug 19, 2026

Retry user-initiated splices across restarts and disconnects #930

Closed

Jolah1 reviewed Aug 19, 2026

View reviewed changes

tnull reviewed Aug 19, 2026

View reviewed changes

tnull added this to the 0.8 milestone Aug 19, 2026

tnull reviewed Aug 19, 2026

View reviewed changes

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

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

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

jkczyz requested review from Jolah1, joostjager and tnull September 1, 2026 15:39

joostjager reviewed Sep 2, 2026

View reviewed changes

jkczyz changed the title ~~Fix two funding payment record bugs~~ Keep funding payment records accurate Sep 2, 2026

jkczyz mentioned this pull request Sep 2, 2026

On-chain payment records can stay Pending forever when their transaction is double-spent #1078

Open

jkczyz force-pushed the 2026-08-funding-payment-bugfixes branch from fb85dd0 to b15d50d Compare September 2, 2026 23:59

jkczyz requested a review from joostjager September 3, 2026 00:02

This was referenced Sep 3, 2026

Track each channel funding in a single payment record #1079

Draft

Track in-flight splices for failure reporting and crash recovery #1080

Draft

joostjager reviewed Sep 3, 2026

View reviewed changes

TheBlueMatt reviewed Sep 3, 2026

View reviewed changes

jkczyz requested review from TheBlueMatt and joostjager September 4, 2026 04:31

jkczyz requested a review from tnull September 29, 2026 18:41

tnull mentioned this pull request Sep 30, 2026

Simplify TX classification #1121

Draft

jkczyz mentioned this pull request Oct 6, 2026

Fix wallet UTXO reuse for funding transactions and onchain spends #1037

Open

tnull removed request for Jolah1 and tnull October 6, 2026 10:53

joostjager removed their request for review October 9, 2026 06:57

jkczyz and others added 23 commits October 9, 2026 09:32

jkczyz force-pushed the 2026-08-funding-payment-bugfixes branch from 80556d3 to c7ac268 Compare October 9, 2026 20:47

Conversation

jkczyz commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Jolah1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Aug 28, 2026

Uh oh!

jkczyz commented Aug 28, 2026

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jolah1 commented Sep 2, 2026

Uh oh!

jkczyz commented Sep 3, 2026

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Aug 19, 2026 •

edited

Loading

ldk-reviews-bot commented Aug 19, 2026 •

edited

Loading

tnull Aug 19, 2026 •

edited

Loading