unroll: cancel LND broadcast of a proof tx on terminal failure

[ellemouton]

Closes #609.

Depends on lightninglabs/lndclient#284 (WalletKit.RemoveTransaction), now merged — this PR bumps lndclient to the merged master commit (no replace).

Problem

After a unilateral-exit unroll job goes FAILED, LND keeps rebroadcasting the rejecting proof tx from its own wallet queue every ~60s indefinitely — the wavelength daemon has stopped touching the job, but the transaction (a zero-fee anchor parent that can never relay alone) stays in LND's rebroadcast set. Nothing tells LND to drop it.

Fix

Give the unroll actor a way to abandon its broadcast transactions on terminal failure:

1. go.mod — bump lndclient to the master commit adding WalletKit.RemoveTransaction.
2. chainbackends — TxBroadcaster gains RemoveTransaction; LNDBackend exposes it as the optional chainsource.TxRemover capability, wrapping lnd's WalletKit.RemoveTransaction RPC.
3. chainsource — a TxRemover optional-capability interface + a RemoveTxRequest routed through the chain-source actor. The handler forwards to a capable backend and treats a backend without the capability (a pure Esplora chain source) or an already-absent tx as a no-op success — so callers request removal uniformly without knowing the backend, and no ChainBackend implementer/fake had to change.
4. unroll — on terminal PhaseFailed, the actor removes its in-flight (broadcast-but-unconfirmed) proof/sweep txids via RemoveTxRequest. Confirmed txs are left alone (on-chain, never rebroadcast). The removal runs once, is best-effort, and detaches cancellation like the terminal registry handoff.

Backend scope: the LND backend is fully covered. lwwallet/btcwbackend sit on btcwallet, which also rebroadcasts — but since they don't implement TxRemover, they no-op today; a btcwallet removal path is a scoped follow-up.

Testing

• chainsource: TestChainSourceActorRemoveTx — forwards to a capable backend, treats an ignorable "not found" error as success, and no-ops on a backend without the capability.
• unroll: TestTerminalFailureRemovesAbandonedBroadcasts — a terminal failure removes the in-flight proof tx.

chainsource + chainbackends + unroll suites, go build, gofmt, and lint-changed-local are all green.

🤖 Generated with Claude Code

[ellemouton]

@claude please review this PR.

It closes #609: after an unroll job fails terminally, LND keeps rebroadcasting the doomed proof tx from its own wallet queue forever. The fix threads a wallet RemoveTransaction down through a new optional chainsource.TxRemover capability and has the unroll actor drop its in-flight broadcasts on terminal PhaseFailed. (Depends on the now-merged lndclient#284.)

Please scrutinise:

• Which txids get removed. removeAbandonedBroadcasts removes job.PlannerState.InFlightTxids plus the sweep txid (only when it advanced past pending), and deliberately NOT ConfirmedTxids. Is "in-flight only" the right set — any risk of removing a tx that actually confirmed, or missing one LND still holds? The once-flag (abandonedBroadcastsRemoved) sits before the registry-handoff guard so it fires independent of the registry — correct placement?

• The optional-capability design. TxRemover is type-asserted in handleRemoveTx; a backend without it (Esplora) and an already-absent tx (IsIgnorableRemoveError: "not found"/"no such"/etc.) both resolve to a no-op success. Is that no-op-vs-error boundary right, and is the substring-based ignorable classification too broad/narrow?

• Best-effort + lifecycle. The removal Asks chainsource with a context.WithoutCancel detached context (mirroring the terminal registry handoff) and only logs on failure. Any concern doing a cross-actor Ask on the terminal path, or with the once-flag not retrying a transient chainsource failure?

• Backend scope gap. Only the LND backend implements TxRemover; lwwallet/btcwbackend (btcwallet, which also rebroadcasts) no-op today. I called this out as a scoped follow-up — agree that's acceptable for closing unroll: LND keeps rebroadcasting a known-rejecting proof tx every ~60s after unroll job is marked Failed #609, or should the btcwallet path be in-scope?

Tests: TestChainSourceActorRemoveTx (handler: forward / ignorable / no-op) and TestTerminalFailureRemovesAbandonedBroadcasts (end-to-end).

[claude]

Claude finished @ellemouton's task in 4m 33s —— View job

---

Review: unroll: cancel LND broadcast of a proof tx on terminal failure

I read the full diff (da2f544..HEAD), the surrounding notifyRegistryIfTerminal/driveEvent call path, effectiveSweepTxid, and the in-flight↔confirmed state transitions in unrollplan/fsm_logic.go. Overall this is a clean, well-documented change and the core mechanism is sound. I have one substantive concern (unbounded blocking Ask on the terminal path) plus answers to each of your scrutiny points.

• Read the diff (chainsource, chainbackends, unroll)
• Scrutinise txid-removal set & once-flag placement
• Scrutinise optional-capability / ignorable-error design
• Scrutinise best-effort + lifecycle on terminal path
• Assess backend scope gap
• Post review

---

🔴 Main concern: blocking cross-actor Ask with a cancel-detached, un-deadlined context on the terminal path

removeAbandonedBroadcasts runs synchronously inside Receive → driveEvent step 4 (unroll/actor.go:415, :2169), i.e. on the unroll actor's own message-processing goroutine, and it loops:

removeCtx := context.WithoutCancel(ctx)
for _, txid := range txids {
        _, err := b.cfg.ChainSource.Ask(
                removeCtx, &chainsource.RemoveTxRequest{Txid: txid},
        ).Await(removeCtx).Unpack()
        ...
}

The PR says this "detaches cancellation like the terminal registry handoff," but the handoff a few lines below is a Tell (RegistryRef.Tell, :2200) — it enqueues and returns immediately. Here you've kept the WithoutCancel but switched to a blocking Ask...Await, and removeCtx has no timeout. So:

• RemoveTransaction is a network RPC to lnd. If lnd is slow/unresponsive or the chainsource mailbox is backed up, Await(removeCtx) blocks the unroll actor's Receive with no deadline.
• This runs before the registry handoff sets terminalNotified. A hang here means the load-bearing terminal handoff never happens and the child wedges — arguably worse than the unroll: LND keeps rebroadcasting a known-rejecting proof tx every ~60s after unroll job is marked Failed #609 symptom it fixes, since best-effort cleanup is now gating the terminal path.
• The loop is sequential over every in-flight txid, so latency stacks.

Best-effort cleanup should not be able to stall (or outright wedge) the terminal completion. Two clean options:

1. Bound it: removeCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), someTimeout); defer cancel().
2. Detach it: fire the removals async (spawn a detached goroutine, or do the registry handoff first and then the removal), since the result is only logged anyway.

Given the whole point is "best-effort, only logs on failure," I'd lean toward doing the registry handoff first and/or bounding the Ask. Fix this →

---

Your specific questions

1. Which txids get removed — "in-flight only" & once-flag placement.
✅ The set is right. A proof tx moves from InFlightTxids → ConfirmedTxids on TxConfirmed (fsm_logic.go:383-387), so at terminal time InFlightTxids is exactly the unconfirmed set. Excluding ConfirmedTxids is correct — mined txs aren't rebroadcast, and lnd's RemoveTransaction refuses confirmed txs anyway. The confirm-vs-remove race (tx confirmed on-chain but the TxConfirmed event not yet applied, so still in InFlightTxids) is handled gracefully by the "already confirmed" ignorable substring → no-op. Nice.

Once-flag placement is correct: for PhaseFailed, emitExitCostIfCompleted returns true immediately (:2290, phase ≠ Completed), so the guard above never defers a failure, and the flag sits ahead of the registry-handoff guard as intended. One nice bonus worth noting in a comment: lnd's removal is recursive over descendants, so removing an in-flight proof parent also drops any fee-paying/anchor child lnd is holding — that covers the "missing one lnd still holds" case for CPFP children without tracking them explicitly.

2. Optional-capability + ignorable classification.
✅ The no-op-vs-error boundary is reasonable, and the substrings match the actual lnd walletkit strings ("unable to locate transaction", confirmed-tx rejection). Two small caveats:

• "no such" is the broadest entry — harmless here since the error only originates from the RemoveTransaction RPC, but it's the one most likely to over-match a future unrelated error. Consider narrowing to "no such transaction" if lnd's wording allows.
• An older lnd without the RPC returns a gRPC Unimplemented error, which is not in the ignorable list → it'd log a warning per txid rather than no-op. Not fatal (best-effort), and go.mod bumps lndclient so the RPC is expected present, but worth a one-line acknowledgement.

3. Best-effort + lifecycle / once-flag not retrying.
See the main concern above re: the Ask. On the once-flag + transient failure: in practice this is ~one shot per process — for PhaseFailed the registry handoff proceeds immediately and stops the child, so there's no later height tick to re-enter and retry within the lifetime. The restart backstop (in-memory flag resets, restored terminal checkpoint re-drives notifyRegistryIfTerminal once) is the real retry path. That's defensible for a best-effort cleanup, but if you want a transient chainsource failure to retry before handoff, only set abandonedBroadcastsRemoved = true when all removals succeeded/were ignorable.

4. Backend scope gap (lwwallet/btcwbackend no-op).
✅ Acceptable for closing #609. The issue is specifically about the lnd wallet's rebroadcast queue, the LND backend is fully covered, and the optional-capability design means the btcwallet path can be added later without touching any caller. Calling it out as a scoped follow-up is the right move — just keep the follow-up issue linked so the btcwallet rebroadcast isn't silently forgotten.

---

Minor / style

• handleRemoveTx formatting wart (chainsource/chainsource.go): the ignorable branch is written as

  a.
          logger(ctx).
          DebugS(
                  ctx,
                  "RemoveTx returned ignorable "+"error",

  The chained a.\n logger(ctx).\n DebugS(...) and the nonsensical "ignorable "+"error" split literal look like an auto-format artifact. The no-op branch right above uses the clean a.logger(ctx).DebugS(ctx, ...) form — please match it and collapse the string to "RemoveTx returned ignorable error". Fix this →
• Test coverage: TestTerminalFailureRemovesAbandonedBroadcasts asserts the in-flight root is removed. Consider also asserting a confirmed txid is not in removedTxSnapshot() (locks in the "in-flight only" contract) and covering the broadcast-sweep case. Optional.

Nothing here blocks the intent of the PR; the blocking/un-deadlined Ask is the one I'd want addressed before merge.
· agent/unroll-remove-broadcast-on-fail

[ellemouton]

Thanks for the review — addressed all four points and force-pushed (the two fix sets are folded into their respective commits, so the history stays a clean 4-commit story).

1. Blocking/un-deadlined Ask on the terminal path (the pre-merge concern).
removeAbandonedBroadcasts no longer runs the ChainSource.Ask loop synchronously on the actor's Receive goroutine. The txid set is still gathered synchronously (cheap), but the removals now fire on a detached goroutine — mirroring txconfirm's async wallet-lease release — so a slow or hung lnd RemoveTransaction RPC can never sit in front of the terminal registry handoff or stall later messages for the target. Each op carries its own context.WithTimeout(context.WithoutCancel(ctx), removeAbandonedBroadcastTimeout) (new named const, 30s, documented) so a stuck backend bounds the goroutine's lifetime rather than leaking it. Cancellation stays detached so a caller disconnect can't suppress the cleanup.

2. handleRemoveTx ignorable-branch formatting wart. Collapsed to the clean a.logger(ctx).DebugS(ctx, "RemoveTx returned ignorable error", …) form and removed the split string literal.

3. "no such" → "no such transaction" in ignorableRemoveErrs, so the substring match is specific to the wallet's "unknown tx" phrasing rather than the broadest possible token.

4. Confirmed txids never removed. Added TestAbandonedBroadcastRemovalSkipsConfirmedTxids, which drives a job state carrying both an in-flight and a confirmed txid through the cleanup and asserts the in-flight one is removed while the confirmed one never is.

On the older-lnd note: pre-0.18 nodes return Unimplemented for WalletKit.RemoveTransaction. That is not in ignorableRemoveErrs, so it surfaces as a logged warning on the best-effort path (it doesn't affect the terminal outcome) rather than being silently swallowed — which is the behavior we want until the min-lnd floor moves. The btcwallet-backed removal path (lwwallet/btcwbackend) remains a scoped follow-up.

[ellemouton]

@claude please review this PR

[claude]

Claude finished @ellemouton's task in 2m 4s —— View job

---

Re-review: unroll: cancel LND broadcast of a proof tx on terminal failure

• Read the current diff (all 12 files)
• Verify the 4 addressed points
• Scrutinise the new async/detached goroutine cleanup path
• Post review

I re-read the updated diff (da2f544..HEAD) end-to-end, focusing on the reworked removeAbandonedBroadcasts async path, the handler formatting, the ignorable-error list, and the new confirmed-txid test. All four points from the previous round are cleanly addressed, and I have no remaining blocking concerns. Details below.

---

✅ Prior main concern (blocking un-deadlined Ask) — resolved

removeAbandonedBroadcasts now gathers the txid set synchronously (cheap, pure in-memory) and fires the removals on a detached goroutine, each op wrapped in context.WithTimeout(context.WithoutCancel(ctx), removeAbandonedBroadcastTimeout) (new documented 30s const, unroll/actor.go:746). This addresses the wedge risk exactly:

• A slow/hung RemoveTransaction RPC can no longer sit in front of the terminal registry handoff — the handoff proceeds on the Receive goroutine while the removals run off to the side.
• The per-op WithTimeout bounds the goroutine's lifetime, so a stuck backend can't leak it indefinitely.
• WithoutCancel keeps a caller disconnect from suppressing cleanup while still carrying log/trace values.

The comment (unroll/actor.go:2280-2290) accurately explains why it's detached and cites the txconfirm precedent. Good.

Two small notes, neither blocking:

• The goroutine isn't tracked by a WaitGroup, so a graceful shutdown won't wait for it. Fine for best-effort housekeeping bounded by a 30s timeout, and consistent with the txconfirm lease-release precedent you're mirroring.
• The once-flag abandonedBroadcastsRemoved is set synchronously right after go func(){…}() launches, so a transient removal failure inside the goroutine won't be retried within the process lifetime. As discussed last round this is the intended best-effort contract (the restart backstop re-drives notifyRegistryIfTerminal once), so no change needed — just confirming it's deliberate.

---

✅ Other three points

2. Formatting wart — handleRemoveTx now uses the clean a.logger(ctx).DebugS(ctx, "RemoveTx returned ignorable error", …) form with a single string literal (chainsource/chainsource.go). Matches the no-op branch.

3. "no such transaction" — ignorableRemoveErrs (chainsource/broadcast_errors.go:45-50) now carries the specific "no such transaction" instead of the broad "no such". Combined with "not found", "unable to locate transaction", "already confirmed", the set is tight and scoped to the RemoveTx RPC's actual phrasings.

4. Confirmed-txid test — TestAbandonedBroadcastRemovalSkipsConfirmedTxids (unroll/actor_test.go:2534) drives a JobState carrying both an in-flight and a confirmed txid, asserts the in-flight one is removed, and then asserts the confirmed one never appears in removedTxSnapshot(). The ordering is sound: since in-flight is the only txid ever enqueued and the goroutine processes sequentially, once the in-flight removal is observed the goroutine has finished, making the "confirmed absent" assertion stable rather than racy. Locks in the "in-flight only" contract nicely.

---

Correctness spot-checks on the reworked path

• Dedup + sweep inclusion (unroll/actor.go:2225-2258): the seen map de-dupes InFlightTxids and guards the effectiveSweepTxid append against a sweep txid already present in-flight. Early-return on empty set avoids spawning an idle goroutine. Correct.
• Detached-context values: context.WithoutCancel(ctx) retains logger/trace context while dropping cancellation — the right primitive here, and opCtx is still valid for the post-cancel() WarnS. No race: chainSource/log are captured by value; ctx is only read.
• Placement (unroll/actor.go:2170): the removal block sits after the emitExitCostIfCompleted guard and before the RegistryRef/terminalNotified guard. For PhaseFailed, emitExitCostIfCompleted returns true immediately (phase ≠ Completed), so the block is always reached on a failure and the once-flag is set exactly once per lifetime. Correct.
• Backend plumbing: TxBroadcaster.RemoveTransaction → LNDBackend.RemoveTx (satisfies chainsource.TxRemover) → LndClientTxBroadcaster.RemoveTransaction (WalletKit RPC). The var _ TxBroadcaster = (*LndClientTxBroadcaster)(nil) assertion still holds. Clean, and no ChainBackend implementer/fake outside LND had to change.

---

Verdict

LGTM. The one pre-merge blocker from the last round is properly fixed (detached + per-op timeout), and the remaining items were addressed as described. Backend scope gap (lwwallet/btcwbackend no-op until a btcwallet removal path lands) remains an acceptable scoped follow-up for closing #609 — just keep that follow-up issue linked so the btcwallet rebroadcast isn't forgotten.

Note: I couldn't execute go build/tests in this sandbox (commands gated), so the green-suite claim rests on your CI run rather than a local re-run here.
· agent/unroll-remove-broadcast-on-fail

[Roasbeef]

Pushed a commit on top of this. Digging through the terminal path, the cleanup doesn't actually fire on the case #609 describes.

applyFailedEvent strips the failing txid from InFlightTxids right before it stamps FailReason:

job.PlannerState.InFlightTxids = removeHash(
        job.PlannerState.InFlightTxids, event.Txid,
)
...
job.FailReason = event.Reason

So by the time removeAbandonedBroadcasts reads planner state, the tx that killed the job is no longer in the list. And that's precisely the tx lnd is sitting on and rebroadcasting. Confirmed it by driving the real path (setImmediateFailed on the root proof tx, same shape as TestProofTxFailureTransitionsToFailed): job reaches PhaseFailed, removal set comes back empty.

Both tests here route around it. TestTerminalFailureRemovesAbandonedBroadcasts goes through the external-spend path, and the comment in it even calls out the choice ("A target spend, unlike a root-output spend, does not confirm the still-in-flight root"). The other one calls removeAbandonedBroadcasts directly with a hand-built JobState. So the suite is green over a different failure than the one we're fixing.

The fix tracks failed txids on the behavior and unions them into the removal set. In-memory only, since the registry only restores non-terminal records, so a terminal job never re-runs this after a restart. Keeps the checkpoint codec untouched. The new test drives the hard-failure path, and fails without the fix. Two small things while in there: the sweep txid was appended without being added to seen, harmless while it was the last entry but not once something follows it, and the final list is now sorted since map iteration order isn't stable.

Worth writing down somewhere in the code: the proof-tx hard-failure path is the safest removal we have. An operator-signed proof node can't be rebuilt or replaced, so the failure is terminal by construction, and txconfirm has already evicted the entry (TxStateFailed is terminal) and released the fee-input lease via broadcaster.Evict. Nothing else in the system is still trying to land it. Which is what makes it funny that it was the one path not covered.

Two things I left alone that are worth a look before this goes in.

The external-spend path is where removal is actually racy. Nothing failed there, so txconfirm is still tracking those txs, and CancelInterestReq has no production caller anywhere (only txconfirm's own tests use it). For a tx still in TxStateBroadcasting, retryBroadcastingParents will re-publish it and put it right back in lnd's wallet. And since RemoveTransaction takes descendants with it, the CPFP child leaves the wallet and frees its fee input while txconfirm still holds the lease on it. If we want that path solid, we should cancel first, then remove.

Second, the comment on the detached goroutine claims the 30s timeout bounds a hung backend. Not quite: ChainSourceActor.Receive dispatches handleRemoveTx on actorCtx rather than the caller's ctx, so the deadline frees the unroll goroutine while the shared chainsource Receive stays blocked on the RPC until it returns on its own. Same is already true of handleBroadcastTx, so it's not new here, but the comment promises more than we get.

[github-actions]

Successfully created backport PR for v0.1.x-branch:

• [v0.1.x-branch] Backport #1068: unroll: cancel LND broadcast of a proof tx on terminal failure #1097