Keep funding payment records accurate - #1057
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
Jolah1
left a comment
There was a problem hiding this comment.
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.
| /// 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
This needs a rebase unfortunately.
| /// 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🤖 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.
| /// 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| }, | ||
| )]); | ||
|
|
||
| // Let the loop fail at least one classification round; a failed classification must not |
There was a problem hiding this comment.
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.
| // 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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?
6093418 to
9e29da5
Compare
@Jolah1 The bump will fail for splices, but will be a problem for dual-funded channels, once supported. Opened #1072.
@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. |
9e29da5 to
fb85dd0
Compare
|
Rebased |
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: requeue it after a short delay and retry classification until it succeeds, holding the broadcast back the whole time. Classification failures are persistence failures, so the retry is unbounded — a store that never recovers keeps the node from functioning anyway — and every failed round is logged. Generated with assistance from Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The retry test slept a fixed three seconds and assumed classification had failed by then; if writes were re-enabled before the first attempt, the test would pass without any retry happening. Count failed writes in FailSwitchStore and wait for one before re-enabling writes. Also fix the test's store reads to use list_page: the payment store's cache is bounded, so list_filter is unavailable, and this commit did not compile its tests standalone (the conversion had landed in the following commit). Implemented with Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joostjager
left a comment
There was a problem hiding this comment.
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.
| // 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(); |
There was a problem hiding this comment.
[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?
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| /// 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 { |
There was a problem hiding this comment.
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?
| async fn classify_and_broadcast( | ||
| &self, package: BroadcastPackage, | ||
| ) -> Result<(), BroadcastPackage> { | ||
| if let Err(e) = self.tx_broadcaster.classify_package(&package).await { |
There was a problem hiding this comment.
Why maintain separate immediate and retry paths instead of treating every broadcast as scheduled retryable work?
There was a problem hiding this comment.
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.
|
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 |
The retry for a failed classification was a detached tokio::spawn that outlived the node. Its comment claimed a re-send after shutdown would fail because the queue had closed, but the queue receiver lives in the broadcaster and is only dropped with the node, so the re-send succeeded and a stale package would be classified and broadcast after a stop()/start() cycle. Queue failed packages inside the broadcast loop instead and retry them from a timer branch of the same select. New packages keep flowing while a retry waits, and pending retries are dropped when the loop stops. Implemented with Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A queued classification can retry after a newer candidate of the same funding already classified. The retry carries the candidate history as of its own broadcast, so applying it rotated the record's txid back to the older candidate and shrank the stored candidate history — after which wallet sync could no longer map the newer transaction to the record and would file it as a foreign duplicate. A fresh interactive-funding classification always carries the record's current txid in its history, so one that doesn't is stale: ignore it, and never let a candidate-history update drop stored candidates. Implemented with Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LDK re-broadcasts pending claims every 30 seconds (and sweeps once per block) until they confirm, so while the payment store is unavailable, the list of pending retries accumulated a copy per rebroadcast — memory, retry load on the struggling store, and a duplicate broadcast burst on recovery all growing with the outage's duration. A package whose transactions already await a retry is not queued again, and the rest are bounded: at the bound, the oldest waiting non-funding package is dropped to make room — its transactions return with LDK's next periodic rebroadcast — but never a funding package, whose transaction would be left confirming without a recorded candidate. Fee-bumped rebroadcast variants carry new txids, so the bound, not the dedup, is what limits their accumulation. Implemented with Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fb85dd0 to
b15d50d
Compare
The compilation will be fixed once the fixups are squashed.
Added a commit marking the record |
joostjager
left a comment
There was a problem hiding this comment.
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.
| // 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()) { |
There was a problem hiding this comment.
🤖 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But would a panic be recoverable then because anything still has the tx on disk?
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
| Refused(BroadcastPackage), | ||
| } | ||
|
|
||
| /// Packages whose classification failed, each waiting out a retry delay before its next attempt. |
There was a problem hiding this comment.
Why do we need a queue? Can't we just spawn a tokio task and rebroadcast in a loop?
There was a problem hiding this comment.
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)?
Discussed offline. The last PR in the stack (#1080) now creates the a payment record before signing when processing the |
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. |
joostjager
left a comment
There was a problem hiding this comment.
Creating the record before signing indeed seems like a good solution to avoid the race and retry loop.
I assume this PR cannot simply be reduced because other transactions still need to be (retryably) classified in the broadcaster? Wondering if for those, the payment records can also be created closer to where the txes originate as well, at a point where failure isn't unsafe, and making the second bug fix unnecessary?
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>
The race only existed for splices. The other types use the
Note that we still need to update the splice payment record when broadcasting since it contains |
b15d50d to
a55bf73
Compare
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 from the channel's pending splice history -- the same history LDK later hands the broadcaster, under the same id -- so the round's broadcast-time classification has nothing left to write but the fact of the broadcast. 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 its classification clears the mark, and 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 classification has run 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 its classification has necessarily run. 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 left to this report, which carries the channel's last funding -- 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. 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 left to broadcast-time classification, as before. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Keep, at ChannelClosed, the splice rounds the channel's monitor still watches. The channel manager forgets a pending round with the channel and reports no failed negotiation for one awaiting the counterparty's signatures, so the handler took back every recorded round but the last funding. The monitor, however, watches every round from the counterparty's commitment_signed on, and our signatures cannot have left the node before that message: the counterparty may hold a fully signed transaction and broadcast it, in which case the dropped record resurfaced as an untyped payment under the transaction's id, or a dropped bump marked the splice Failed when it confirmed. Reachable when this node's contribution is the smaller one -- a queued contribution merged into a counterparty-initiated round -- and on LDK's own force-close after a tx_abort that follows its commitment, as well as at reload when the monitor is ahead of the channel manager. The kept set is every round the monitor watches, not only those our signatures left for: the monitor cannot tell them apart, and a watched round still marked at close is in either case one whose counterparty signatures never arrived, since with both signature sets held LDK broadcasts the round and its classification clears the mark. Such a round stays a Pending record until the close spend matures and the monitor's DiscardFunding arrives; that handler only reclaims addresses today, so nothing terminates the record yet (pre-existing; the following commit adds that). A round is marked at signing, which LDK triggers at tx_complete, before the counterparty's commitment_signed, so a marked round that message never reached is still dropped; our signatures cannot have left for it. After a zero-conf lock the monitor stops watching the rounds the lock superseded -- an RBF sibling of the locked round and the previous funding scope alike -- so a later close still drops those, which matters only while their classification is queued. Two integration tests drive the handler into each case by holding back the peers' store writes, which precede their signing: one lets this node sign only once the counterparty's commitment_signed has arrived and keeps the counterparty's tx_signatures from ever arriving, and expects the record kept; the other keeps the counterparty from signing at all and expects the record dropped. When squashing, the base message's "which carries the channel's last funding" should read "which carries the channel's last funding, joined by the rounds its monitor still watches". 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 funding payment the event names. A round nothing ever broadcast is dropped first, as at `ChannelClosed`, and with it a record no broadcast round of ours remains under. The 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, and it learns a round only after the manager lists it. The event names a round by its transaction only when this node did not contribute to it; otherwise it describes what LDK returns of the contribution: the inputs and output scripts the round that replaced it does not reuse. Record each candidate's contributed inputs and output scripts so the event can be matched to the round, exactly or as the one recorded contribution with more parts. A round recorded before this carries no parts, and an event describing its contribution changes nothing while the channel is listed, as before. So does an event describing the channel's current funding: LDK also returns a contribution it refused before building a round from it, whole when the channel had no pending splice to check it against -- a fee bump adjusted from a round that locked as the bump was built, queued until the channel goes quiescent for it and returned once the node restarts, the channel force-closes or begins a cooperative close while no stfu is outstanding on it, the user cancels it, or the negotiation begun from it is refused, fails or is cut off by a disconnect -- and such a bump describes the locked round, while no round LDK discards can be the funding. 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: when LDK discards a sibling round, 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 with every round held and leaves the payment. So `ChannelClosed` now 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 by the rounds the monitor holds alone, without matching the event to a round: the close has settled what remains, and the held rounds decide for a record written without its parts or for records signed under different first-candidate ids that share one contribution, which a match cannot tell apart. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
After some offline discussion, we decided to use Separately, I filled two issues upstream to allow simplifying the current approach further:
No need to re-review yet. |
Two bugfixes for funding payment records (channel opens and splices), both reachable on current
main. 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.
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.
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.