multi: unroll + round-finality/sweep/fraud reorg safety

[ellemouton]

Part 3 of 3 of the condensed reorg-safety client stack (epic
lightninglabs/darepo#454). This is the subsystem act-on-reorg layer.

⚠️ Stacked on #896 (which is stacked on #895) — review/merge those first. The
diff is against reorg-canonicality-gates.

Makes the unilateral-exit and round subsystems reorg-safe:

• Reorg-safe unroll (unilateral exit): durable sweep-finalized terminal
  eligibility, reorg-aware reconciliation, snapshot handling.
• Reorg-safety extras: round finality gate, boarding-sweep reorg guard, and
  fraud invariants.

Commits (unchanged, re-grouped):

• unroll: Reorg-safe unilateral-exit subsystem
• multi: Reorg-safety extras (round finality gate, sweep, fraud)

Supersedes #410, #558 (condensed here).

This is the full-stack tip; the server PR (lightninglabs/darepo#626) pins its
client submodule at this commit so the server itests/systests exercise the
entire client + server reorg-safety behavior together.

Stack:

1. multi: reorg-aware chain observation (chainsource + backends) #895 — reorg-aware chain observation
2. multi: batch canonicality + VTXO admission gates #896 — batch canonicality + VTXO admission gates
3. this PR — unroll + round-finality/sweep/fraud reorg safety

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[ellemouton]

@claude review this

[claude]

Claude finished @ellemouton's task in 3m 5s —— View job

---

Review: PR #897 — unroll + round-finality/sweep/fraud reorg safety

I read the full diff against origin/reorg-canonicality-gates (unroll/ actor + FSM + reconcile + snapshot, wallet/boarding_sweep_actor.go, vhtlcrecovery/coordinator, darepod wiring, and the fraud/systest additions).

Overall this is a careful, well-documented change. The reconcile-vs-live symmetry (reconcileCheckpoint mirroring applyReorgedEvent), the provisional/finalized latch design, and the TLV checkpoint additions are all internally consistent, and the test coverage is thorough. A few things worth discussing before merge:

---

1. Boarding-sweep reorg leaves the store row permanently stale (wallet/boarding_sweep_actor.go:1143)

handleSweepSpendNotification on Status=Reorged deliberately does not roll the row back, and the doc-comment relies on "a re-spend on the new canonical chain … re-fires Spent". That reasoning holds when an external party re-spends, but if the reorged-out spender was our own aggregate sweep and it is not re-mined on the new tip, the input stays marked external_spent forever and never gets re-swept — the Done synthesizer won't fire for a spend that no longer exists on chain. The comment acknowledges "manual reconciliation at restart," but unlike the unroll subsystem there's no automatic restart reconciler for boarding sweeps here. Is a follow-up planned to roll the row back to pending on reorg (or reconcile boarding-sweep inputs on boot)? Worth calling out explicitly as the one place where reorg-safety is left incomplete.

2. Completed.IsTerminal() now always returns false → potential indefinite actor retention (unroll/fsm_types.go:572, unroll/actor.go:1953)

Making Completed non-terminal is necessary so a reorg event can re-enter the FSM — that's sound. But the actor is now only truly evicted once sweepFinalized latches, and the comment itself notes "the chainsource transport (lndclient over gRPC) does not surface a finality signal today, so production deployments may retain the child indefinitely." That means every completed unroll keeps a live actor + block subscription until the Phase-7 height-gating lands. For a daemon that does many exits over its lifetime this is an unbounded-growth footgun. Since chainsource already synthesizes ConfDoneEvent/SpendDoneEvent via FinalityDepth, could the sweep-confirmation path be wired through a finality-capable notifier now (or a height-based backstop added) rather than deferring entirely? At minimum the operational impact deserves a louder note than a Phase-7 aside.

3. PhaseExternalSpendObserved persists as Materializing in the control-plane DB (unroll/db_store.go:233)

statusForPhase has no case for the new phase, so it falls through default → UnilateralExitJobStatusMaterializing. The durable actor checkpoint reconstructs the true phase on restart (via ProvisionalExternalSpend in phaseFromPlannerState), so this is correct for restore — but the registry/DB record and any operator-facing status query will show a parked external-spend job as "Materializing", which is misleading for triage. Consider mapping it to a distinct status (or at least a comment documenting the intentional collapse).

4. Dead GetStateRequest branch in dispatch (unroll/actor.go:349)

Receive short-circuits GetStateRequest at line 254 before ever calling dispatch, so the new case *GetStateRequest: inside dispatch is unreachable. Harmless, but it can be dropped to avoid implying two live paths. Fix this →

---

Minor / non-blocking

• Restart reconcile cost (unroll/reconcile.go:117): reconcileCheckpoint issues one sequential ConfirmedTx probe per confirmed proof node, each up to ProbeTimeout (10s default). For a deep proof graph on a slow backend the worst case is N×timeout of serialized boot latency, and timeout-as-absent can trigger a spurious subtree prune + rebroadcast. This is documented and idempotent (and the new reconcileprobetimeoutsec knob helps), just flagging the tail behavior.
• Probe timeout classification (reconcile_chainsource.go:184): treating context.Canceled the same as DeadlineExceeded → "not on chain" means a caller-cancelled reconcile (e.g. shutdown) reports anchors as absent. Since the only caller aborts the whole restore on shutdown this is benign, but conflating cancel-by-shutdown with genuine timeout is slightly surprising.
• TLV record-type reuse (messages.go:83): txReorgedTxidRecType and txFinalizedTxidRecType are both tlv.Type(1). Correct because each lives in its own message stream, but the shared inner-type-1 convention is easy to trip over — the existing pattern, so fine.

Nothing here looks like a correctness blocker for the reorg-safety machinery itself; items 1 and 2 are the ones I'd want a maintainer decision on (roll-back gap and actor retention) before this is the full-stack tip.
· branch reorg-subsystem-safety

[ellemouton]

@claude review this — pushed fixes addressing the prior review comments.

[claude]

Claude finished @ellemouton's task in 5m 21s —— View job

---

Re-review: PR #897 — fixes for prior review comments

I re-read the current tip against origin/reorg-canonicality-gates, focused on the four blocking/minor items from the last pass. Verdict up front: #1 is well-handled, #2 is effectively resolved by the stack's wiring but its code comment is now stale/misleading, and #3 + #4 look untouched.

• Gather context
• lib: start adding lib helpers #1 — boarding-sweep reorg roll-back
• multI: add initial repo scaffolding #2 — Completed non-terminal / actor retention
• chainbackend+chainsource: add chainsource actor and backend impl #3 — PhaseExternalSpendObserved status mapping
• multi: stage boarding actor client  #4 — dead GetStateRequest branch
• Minor items
• Post review

---

✅ #1 — Boarding-sweep reorg roll-back — addressed

handleSweepTxNotification now reconciles on the sweep-tx-level notification, which is reorg-symmetric (Confirmed/Reorged/Finalized):

• BoardingSweepTxStatusConfirmed → reconcileSweepInputsOnConfirm (wallet/boarding_sweep_actor.go:1450) marks every still-pending input spent, idempotently.
• BoardingSweepTxStatusReorged (:1321) deliberately keeps the pending entry + watch alive, so a re-confirmation on the new tip re-runs reconciliation; if our own sweep is reorged out and never re-mined, the inputs simply stay pending and get re-swept — which is the correct outcome and exactly the case I flagged before.

The one residual gap — a reorged-out external spender leaving a row external_spent because handleSweepSpendNotification / MapSpendEvent isn't reorg-symmetric — is now explicitly documented (:1332-1340, :1182-1195) and scoped as "tracked separately." That's a reasonable resolution: the case I was worried about (our own sweep) is fixed, and the narrower remaining case is called out. 👍

⚠️ #2 — Actor retention — the concern is actually resolved, but the comment now lies about it

Tracing the eviction path in the current tree, the "retain the child indefinitely" worry no longer holds:

• The final sweep is tracked through txconfirm, and the unroll subscriber maps txconfirm.TxFinalized → TxFinalizedMsg (unroll/actor.go:1051).
• txconfirm reaches Finalized/emits TxFinalized off chainsource's height-based Done synthesis, which this stack explicitly enables via FinalityDepth: s.cfg.ResolveReorgSafetyDepth() (darepod/server.go:2111) — the comment there even says this exists so "conf/spend sub-actors driven through lndclient (whose Done channel is allocated-but-never-written) would never receive a finality signal" otherwise.
• That TxFinalizedMsg → maybeLatchSweepFinalized (unroll/actor.go:1918) → sweepFinalized = true → notifyRegistryIfTerminal fires → child evicted at the reorg-safety horizon.

So the actor is evicted in production. But the doc-comment at unroll/actor.go:1948-1951 still reads:

The chainsource transport (lndclient over gRPC) does not surface a finality signal today, so production deployments may retain the child indefinitely after a sweep confirms; height-based finality gating is a Phase 7 follow-up.

That directly contradicts the FinalityDepth wiring at server.go:2111 and the txconfirm.TxFinalized path. Either the comment is stale and should be rewritten to say "the sweep's TxFinalized arrives via txconfirm's FinalityDepth-synthesized Done, so the child is evicted at the reorg-safety depth," or — if there's a real reason the sweep's finalization doesn't fire (e.g. a backend where Done truly never lands) — that reason needs to be the comment instead of a generic "Phase 7." As written it will mislead the next reader into thinking eviction never happens. Fix this →

⚠️ #3 — PhaseExternalSpendObserved → Materializing — still open

Both mappers still collapse the parked external-spend phase into Materializing via their default arm, with no distinct status and no documenting comment:

• statusForPhase (unroll/db_store.go:213) — default → UnilateralExitJobStatusMaterializing.
• unrollPhaseToProto (darepod/rpc_server.go:4849) — default → UNROLL_JOB_STATUS_MATERIALIZING.

Since GetUnrollStatus prefers the live registry, even a live query for a parked external-spend job reports MATERIALIZING to the operator, which is misleading for triage. Restore is still correct (the checkpoint reconstructs the true phase), so this is a status-visibility issue, not a correctness one — but it's the same nit as last time and doesn't appear addressed. At minimum a comment on the default arm documenting the intentional collapse would help; ideally a distinct proto/db status. Fix this →

⚠️ #4 — dead GetStateRequest branch — still present

Receive still short-circuits GetStateRequest at unroll/actor.go:254 before ever calling dispatch, so the case *GetStateRequest: at unroll/actor.go:349 remains unreachable. Harmless, but still dead. If it's kept intentionally as defensive symmetry, a one-line comment saying so would stop a future reader from assuming two live paths. Fix this →

Minor

• Probe timeout classification (unroll/reconcile_chainsource.go:184): still treats context.Canceled and DeadlineExceeded identically as "not on chain," now with a warn log. Benign for the shutdown-only caller as noted before; the warn helps surface slow-backend regressions.
• TLV inner-type reuse (unroll/messages.go): unchanged, correct-per-stream, matches the existing convention.

---

Summary: #1 is genuinely fixed and #2's underlying risk is resolved by the finality wiring — but #2's doc-comment now actively contradicts the code and should be corrected, and #3/#4 (both minor/non-blocking) appear untouched. Nothing here is a correctness blocker; the #2 comment is the one I'd most want corrected before this is the full-stack tip, since it will otherwise mislead operators/reviewers about eviction behavior.
· branch reorg-subsystem-safety

[ellemouton]

@claude review this — addressed your 3 follow-ups (stale unroll doc-comment rewritten, external-spend phase mapping made explicit + documented, dead GetStateRequest branch removed).

[claude]

Claude finished @ellemouton's task in 1m 6s —— View job

---

Re-review: PR #897 — 3 follow-up fixes verified

Read the fix commit (7254db1) against origin/reorg-canonicality-gates and checked each change against the surrounding code. All three follow-ups are correctly addressed. ✅

• Gather context
• multI: add initial repo scaffolding #2 — stale unroll doc-comment rewritten
• chainbackend+chainsource: add chainsource actor and backend impl #3 — external-spend phase mapping made explicit + documented
• multi: stage boarding actor client  #4 — dead GetStateRequest branch removed

---

✅ #2 — stale doc-comment rewritten (unroll/actor.go:1948)

The old "retain the child indefinitely … Phase 7 follow-up" text is gone. The new comment now describes the actual eviction path — sweep finality arrives via txconfirm's TxFinalized, emitted off chainsource's height-based Done synthesis, enabled for every backend (incl. lndclient) by FinalityDepth = ResolveReorgSafetyDepth(). I verified this against the wiring: darepod/server.go:2111 sets exactly that, and the neighbouring comment (server.go:2105-2106) independently confirms the "lndclient Done channel is allocated-but-never-written" claim the rewrite leans on. The comment now matches the code rather than contradicting it. 👍

✅ #3 — external-spend phase mapping explicit (unroll/db_store.go:234, darepod/rpc_server.go:4866)

Both mappers now have an explicit case PhaseMaterializing: plus a dedicated case PhaseExternalSpendObserved: arm, each with a comment stating the collapse onto Materializing/MATERIALIZING is intentional, why it's safe (transient reorg-reversible phase, true phase reconstructed from the durable checkpoint on restore, no dedicated enum/proto value), and that a distinct status can be added if the parked window needs to be independently observable. This resolves the "silently misleading" concern — the collapse is now documented at both call sites rather than falling silently through default.

✅ #4 — dead GetStateRequest branch removed (unroll/actor.go:349)

The unreachable case *GetStateRequest: in dispatch is gone, replaced by a comment explaining the invariant: Receive short-circuits GetStateRequest at line 254 before dispatch is reached (it's a read-only probe that drives no FSM transition), so if one ever reaches this switch via a future refactor, the default arm's loud error is the desired signal rather than a silently duplicated read path. Good — this documents why the branch is absent instead of leaving a future reader to wonder. I also confirmed stateResponse() is not orphaned: it's still called on the live short-circuit path at actor.go:255.

---

Verdict: All three items are resolved cleanly. The commit is documentation + dead-code only (no behavior change), consistent with its message, and the rewritten comment's claims check out against the actual FinalityDepth wiring. Nothing further blocking from my side on these follow-ups.
· branch reorg-subsystem-safety

[ellemouton]

Superseded by the corrected, reviewable stack in #964, #980, and #990, with the corresponding Lumos integration in #626, #685, and #686.