Fix three funding payment record bugs - #1057
Conversation
|
I've assigned @tnull as a reviewer! |
Jolah1
left a comment
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.
tnull
left a comment
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
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. |
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>
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. Park 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>
9e29da5 to
fb85dd0
Compare
|
Rebased |
4 participants
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; #XXXX (payment-model groundwork) and #XXXX (in-session splice retry) follow.
Developed with assistance from Claude Code.