Skip to content

multi: Add configurable reorg recovery foundation#964

Open
ellemouton wants to merge 4 commits into
mainfrom
reorg-recovery-foundation
Open

multi: Add configurable reorg recovery foundation#964
ellemouton wants to merge 4 commits into
mainfrom
reorg-recovery-foundation

Conversation

@ellemouton

@ellemouton ellemouton commented Jul 16, 2026

Copy link
Copy Markdown
Member

What this PR does

Teaches existing client subsystems that confirmed chain facts can later
disappear. It does not enable one-confirmation VTXO spending — it
establishes the recovery behaviour that sits underneath that feature.

It makes these operations recoverable:

  • Unilateral exit and unroll transactions.
  • Boarding-input sweeps.
  • External-spend and broadcast observations.
  • Restart after a reorg.

Instead of treating "once confirmed" as permanent, the client persists the
evidence and rechecks it. If the fact disappears, the subsystem returns to the
appropriate earlier state.

A key invariant: broadcast ambiguity cannot resurrect a VTXO. A VTXO is
relived only with positive evidence that its spending transaction was
definitely not broadcast.

It also introduces the configurable reorg-safety horizon H (default 30);
terminal chain finality is computed as H + 1. reorgsafetydepth=0 resolves to
the safe default; the horizon is capped at 144 (the deepest reversible watch
window supported by every current backend).

Review focus

  • Does each recovery checkpoint survive restart correctly?
  • What precise evidence permits a VTXO to be relived?
  • Are the new durable messages TLV-compatible with rolling upgrades?
  • Is the distinction between seen, confirmed, and final applied
    consistently?
  • Does every ambiguous broadcast state fail closed?

Out of scope


Full design & diagrams: REORG_SAFETY_PR_REVIEW_GUIDE.md §4.1. Overall
architecture and PR stack: lumos#454.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces significant reorg-safety improvements for the boarding sweep and unroll subsystems. Key changes include adding a chain reconciliation mechanism that verifies persisted anchors against the canonical chain on restart, implementing a fail-closed guard (ReliveUnsafe) to prevent accidental recovery of spent VTXOs, and refining the FSM lifecycle to handle external spend observations and reorgs more robustly. The changes also include comprehensive test coverage for these new safety paths and updated configuration options for reorg safety depth and reconciliation timeouts. As there were no review comments provided, I have no feedback to offer.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@ellemouton
ellemouton force-pushed the reorg-recovery-foundation branch from fe01548 to dfe71da Compare July 16, 2026 18:21
@ellemouton
ellemouton force-pushed the reorg-observe branch 3 times, most recently from 292394e to 233abdd Compare July 16, 2026 20:43
@ellemouton
ellemouton force-pushed the reorg-recovery-foundation branch from dfe71da to f27fde5 Compare July 16, 2026 20:46
@ellemouton

Copy link
Copy Markdown
Member Author

Rebased the three recovery commits onto the rewritten #895 head 233abdd6; the PR diff is clean again and contains only Client PR 2. No recovery code changed during the rebase. Post-rebase verification: go test -race ./unroll ./wallet ./waved ./chainsource -count=1 and git diff --check passed. Fresh CI is running on f27fde5d.

@ellemouton
ellemouton force-pushed the reorg-recovery-foundation branch from f27fde5 to 57fe7a8 Compare July 16, 2026 22:32
@ellemouton
ellemouton changed the base branch from reorg-observe to main July 16, 2026 22:32
@ellemouton
ellemouton marked this pull request as ready for review July 16, 2026 22:45
@ellemouton

Copy link
Copy Markdown
Member Author

@claude please review this PR — Client PR 2 of the reorg-safety stack (configurable reorg recovery foundation), now rebased directly onto main after #895 merged. Focus on the unilateral-exit and boarding-sweep reorg/restart recovery correctness, the fail-closed-on-missing-evidence behavior, and the configurable reorgsafetydepth (default 30) / terminal-depth wiring.

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review: PR #964 — Configurable reorg recovery foundation

I reviewed the three focus areas (unilateral-exit recovery, boarding-sweep recovery, fail-closed-on-missing-evidence, and the reorgsafetydepth/terminal-depth wiring), reading the code directly and cross-checking with focused deep-dive passes over each subsystem.

Overall this is careful, high-quality work. The reconciliation core is genuinely fail-closed: reconcileCheckpoint refuses to proceed on any reconciler error, the production chainSourceReconciler never manufactures a negative answer (HasAuthoritativeNegativeEvidence() == false), terminal state is persisted before any VTXO relive, and the relive gate requires both !HadOnChainFootprint and !ReliveUnsafe. The config wiring, +1 derivation, v2-defined-but-not-advertised, and boarding provisional-until-Done/discard-on-Reorged lifecycle all check out. I found one issue worth fixing before merge and several lower-severity notes.


🔴 High — ReliveUnsafe decodes to false for pre-PR checkpoints (fail-open at the upgrade boundary)

unroll/snapshot.go:515-517 (decode) · unroll/snapshot.go:332-339 (encode) · consumed at unroll/registry.go:977-978, :1098-1099

encodeCheckpoint writes checkpointReliveUnsafeRecordType only when the flag is true, and decodeCheckpoint leaves it at its zero value (false) when the record is absent. checkpointVersion was not bumped (still 1, confirmed against 7ae15c7^), so a job that was in flight under the old code restores with ReliveUnsafe == false, and restore uses stateFromCheckpoint directly (no StartEvent, so the guard is never re-armed on that path).

Concrete failure: a non-terminal old checkpoint (one proof node in InFlightTxids, nothing confirmed, sweep pending) with a TxFailedMsg buffered in the durable mailbox. On restart the replayed TxFailedMsg is processed before the registry's ResumeUnrollRequest (which is the only thing that would set ReliveUnsafe = true at fsm_logic.go:83). applyFailedEvent removes the sole in-flight txid and sets FailReason → terminal Failed. Now jobHadOnChainFootprint == false and ReliveUnsafe == false, so recoverable == true (registry.go:977) → ExitOutcomeRecoverable relives a VTXO that actually had an on-chain submission — exactly the "silence treated as absence" outcome the invariant forbids.

The invariant in CLAUDE.md/fsm_types.go:124 is "the live actor sets ReliveUnsafe before every chain-boundary attempt; only objective canonical-absence evidence may clear it." A restored pre-PR job silently starts with the guard down, which violates it. For new checkpoints the flag is always true (set at StartEvent/ResumeEvent, fsm_types.go:406,418), so only the upgrade boundary is exposed — but that boundary is real for any deployment that already has unroll jobs on disk.

Fix: default ReliveUnsafe to true when a Started checkpoint lacks the record (fail-closed), so an old in-flight job can never relive without positive evidence. Bumping checkpointVersion would reject old checkpoints outright, which is worse.

Fix this →


🟡 Medium — companion / liveness notes (all fail-closed, value-safe)

A. The recoverable-relive path is effectively inert in production — confirm this is the intended safety boundary. Nothing ever clears ReliveUnsafe (only-ever-set-to-true, verified by grep), and the only production reconciler reports no authoritative negative evidence. So for any new job, recoverable at registry.go:977 can never become true and ExitOutcomeRecoverable never fires. This matches the PR's stated boundary ("the next client PR adds the authoritative negative barrier"), so it's by design for this PR — but combined with the High finding above, the only production path that currently reaches ExitOutcomeRecoverable is the buggy upgrade one. Fixing the High finding makes the feature correctly unreachable everywhere until the next PR. Worth an explicit code comment near reconcileExternalSpend marking the clear-hook as deliberately deferred so it doesn't read as dead code.

B. Genuine offline reorg head-of-line-blocks restart indefinitely. unroll/reconcile_chainsource.go:139-206/:213-280: ConfirmedTx/SpentOutpoint return Some or an error — never None. If a proof tx recorded in ConfirmedTxids was truly reorged out while offline, reconcileCheckpoint gets ErrReconcileEvidenceUnavailable, ensureLoaded returns the error, the ResumeUnrollRequest is nacked/redelivered, and the actor retries every ~10s forever without resuming. Fail-closed, but an unbounded stall driven purely by silence — and it means the "prune reorged anchors" prune branches (reconcile.go:156-161,176-186,248-257) are dead for the shipped reconciler. Given the reconciler can't prove the negative, the remediation is either documenting the accepted stall or a bounded backoff; worth an explicit decision.

C. Boarding: a partially-externally-spent sweep strands its sibling inputs and re-broadcasts a dead tx forever. wallet/boarding_sweep_actor.go:1396-1450 + :1849-1872. Scenario: 2-input sweep {A,B}; A is double-spent externally → resolves to external_spent/swept (correct). The sweep tx {A,B} can now never confirm, so txconfirm emits TxFailed, which only sets pending.submitted = false and never calls MarkBoardingSweepFailed (no production caller remains — confirmed repo-wide). Every tip tick resumeOneBoardingSweep re-submits the identical permanently-invalid tx, and B stays pending/published, locked by idx_boarding_sweep_inputs_active_outpoint, so it can never be re-swept. Fail-closed (no loss/double-spend, only emitBackgroundTaskError alerting), but a real liveness hole — there's no path distinguishing an unbroadcastable/conflicting tx from a transient failure. Consider conflict detection or a bounded-retry→release path.


🟢 Low / nits

  • Restart can broadcast a doomed sweep before an offline external spend is observed. unroll/reconcile.go:234-239 skips the spend probe when there's no persisted anchor, relying on the re-armed live watch — but ensureSpendWatch only registers; the historical SpendObservedMsg is async. If the checkpoint is NeedSweep, the same-turn ResumeEvent routes RequestSweepBuildstartSweep and broadcasts before the spend notification parks in AwaitingExternalSpendFinality. Result is a rejected double-spend (→TxFailed, retry), fail-closed. The comment at reconcile.go:230-233 ("re-armed before Resume routes work") conflates registration with observation and slightly overstates the guarantee.
  • finishSweepSpend teardown keys on in-memory count. wallet/boarding_sweep_actor.go:1282-1288: resolved || len(pending.inputs) == 0 can cancel watches while the store still holds pending inputs (e.g. one whose watch failed to arm on resume). Self-heals on the next tip resume; consider gating teardown on the store's authoritative resolved only.
  • Resume Tells aren't coalesced. Each per-input persistence failure / rearmSweepSpendWatch / failed reconcileSweepInputsOnFinalized self-Tells a full ResumeBoardingSweepsRequest (each a full ListPendingBoardingSweeps + re-arm), bypassing the tip-tick inflight latch. Bounded by input count; a dedup latch would cut DB churn.
  • Config depth resolvers don't clamp. waved/config.go:1385-1396: reorgSafetyDepth()/chainFinalityDepth() rely entirely on Validate() having run. A Config built without validation near MaxUint32 overflows +1 to 0, which the conf/spend actors read as "finality synthesis disabled" (conf_actor.go:384) — silently leaking watches instead of ever firing Done. Not reachable in production (startup validates), but a cheap defense-in-depth clamp. Also: no minimum floor (reorgsafetydepth=1 silently yields a depth-2 horizon), and the boarding systest fixture still hardcodes chainsource.DefaultFinalityDepth (6) at systest/systest.go:221, so it doesn't exercise the config-derived horizon.
  • Question (possibly pre-existing, out of scope): emitSweepConfirmedLedger fires only from the txconfirm Finalized arm (boarding_sweep_actor.go:1388). A fully external_resolved sweep never gets Finalized, so its inputs move to swept with no clearing/ledger legs. Intentional?

Verified correct

Config: default 30 / max 144 / zero→default / +1 derivation and its alignment with LND's 144-block retention; v2 defined but never advertised (clientSupportedArkVersions() returns v1, and resolveArkVersionSelection rejects an operator forcing v2); single centralized FinalityDepth wiring at server.go:2149. Unroll: reconcile-before-side-effects ordering (actor.go:1333-1373), terminal-persist-before-relive with withhold-and-retry on write failure, applyReorgedEvent↔restart-reconcile convergence, provisional Completed gated on sweepFinalized, and clean chainsource-probe context hygiene (bounded probe ctx, context.Background() cleanup, self-cleaning one-shot subs). Boarding: provisional-until-Done/discard-on-Reorged, durable per-input finalization in one write-tx with monotonic guards, broadcaster-failure-as-uncertain, idempotent reconcileSweepInputsOnFinalized fallback, and context.WithoutCancel actor-owned recovery.


Review methodology: direct reading of the reconcile/FSM/config/store code plus three focused deep-dive passes; the High finding was verified against git show 7ae15c7^ (version unchanged) and grep (no ReliveUnsafe clear/true-default).

View job run · branch reorg-recovery-foundation

@ellemouton

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

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


Review: PR #964 — Configurable reorg recovery foundation

I re-read the three focus areas directly (unilateral-exit reconcile/relive, boarding-sweep reorg/restart recovery, fail-closed-on-missing-evidence, and the reorgsafetydepth/terminal-depth wiring) and independently re-verified the one previously-flagged high-severity item against the current tree. It still stands and is unaddressed. The rest of the design remains solid and genuinely fail-closed.


🔴 High — ReliveUnsafe decodes to false for pre-PR checkpoints (fail-open at the upgrade boundary)

unroll/snapshot.go:332-339 (encode) · :515-517 (decode) · consumed at unroll/registry.go:977-978 and :1098-1099.

Verified in the current tree:

  • checkpointVersion is still 1 (snapshot.go:51) — unchanged vs origin/main, so old rows decode instead of being rejected.
  • The field is new in this PR: origin/main:unroll/snapshot.go has no ReliveUnsafe record.
  • encodeCheckpoint writes checkpointReliveUnsafeRecordType only when the flag is true (:332); decodeCheckpoint leaves it at the zero value false when the record is absent (:515-517) with no Started-aware default.

Concrete fail-open at the upgrade boundary. A pre-PR daemon has a non-terminal in-flight job — one proof node in InFlightTxids, nothing confirmed, sweep pending — with a TxFailedMsg already delivered to the durable mailbox but not yet processed at shutdown. Operator upgrades to this PR. On restart the persisted mailbox message replays; if it is processed before the registry's ResumeUnrollRequest (the only thing that arms ReliveUnsafe = true, fsm_logic.go:83), then:

  1. applyFailedEvent (fsm_logic.go:498-505) removes the sole in-flight txid and sets FailReason → terminal Failed.
  2. jobHadOnChainFootprint (state_snapshot.go:65-77) is now computed over the post-removal state: empty InFlightTxids, sweep pending, nothing confirmed → false.
  3. ReliveUnsafe restored as false.
  4. recoverable = req.Phase == PhaseFailed && !HadOnChainFootprint && !ReliveUnsafe (registry.go:977-978) → trueExitOutcomeRecoverable relives a VTXO that in fact had an on-chain submission.

That is exactly the "silence treated as canonical absence" outcome the invariant at unroll/CLAUDE.md:73 / fsm_types.go:124-128 forbids ("the live actor sets ReliveUnsafe before every chain-boundary attempt; only objective canonical-absence evidence may clear it"). New checkpoints are safe — ReliveUnsafe is true from StartEvent/ResumeEvent (fsm_types.go:406,418) — so only the upgrade boundary is exposed, but that boundary is real for any deployment that already has unroll jobs on disk.

Fix: default ReliveUnsafe to true when a Started checkpoint lacks checkpointReliveUnsafeRecordType, so an old in-flight job fails closed. Bumping checkpointVersion would reject old rows outright (worse). A regression test in snapshot_test.go (Started checkpoint encoded without the record → decodes ReliveUnsafe == true) would pin it.

Fix this →


🟡 Medium — liveness / boundary notes (all fail-closed, value-safe)

  • The recoverable-relive path is inert in production — confirm this is the intended boundary. Nothing ever clears ReliveUnsafe (grep confirms it is only ever set to true at fsm_logic.go:83, fsm_types.go:406,418), and the only production reconciler reports HasAuthoritativeNegativeEvidence() == false (reconcile_chainsource.go). So for any new job recoverable at registry.go:977 can never become true. This matches the PR's stated boundary (the authoritative negative barrier ships in the next client PR) — but it means the only production path currently reaching ExitOutcomeRecoverable is the buggy upgrade one above. Fixing the High finding makes the feature correctly unreachable everywhere until then. A code comment near reconcileExternalSpend marking the clear-hook as deliberately deferred would stop it reading as dead code.

  • Genuine offline reorg can head-of-line-block restart indefinitely. reconcile_chainsource.go ConfirmedTx/SpentOutpoint return Some or an error, never None. If a proof tx in ConfirmedTxids was truly reorged out while offline, reconcileCheckpoint gets an evidence-unavailable error → ensureLoaded fails → ResumeUnrollRequest is nacked/redelivered → the actor retries ~every 10s forever without resuming. Fail-closed, but an unbounded stall driven by silence, and it makes the prune branches (reconcile.go:156-161,176-186,248-257) dead for the shipped reconciler. Worth an explicit decision: document the accepted stall or add a bounded backoff.

  • Boarding: a partially-externally-spent sweep can strand sibling inputs and re-broadcast a dead tx. wallet/boarding_sweep_actor.go (~:1396-1450, :1849-1872). 2-input sweep {A,B}; A is double-spent externally → resolves correctly to external_spent/swept. The sweep tx {A,B} can now never confirm → TxFailed only flips pending.submitted = false (no production caller of MarkBoardingSweepFailed remains). Every tip tick resumeOneBoardingSweep re-submits the same permanently-invalid tx and B stays locked pending/published. Fail-closed (only alerting churn), but a real liveness hole — no path distinguishes an unbroadcastable/conflicting tx from a transient failure. Consider conflict detection or a bounded-retry→release path.

🟢 Low / nits

  • Restart can broadcast a doomed sweep before an offline external spend is observed. reconcile.go:234-239 skips the spend probe when there is no persisted anchor, relying on the re-armed live watch — but ensureSpendWatch only registers; the historical SpendObservedMsg is async. For a NeedSweep checkpoint the same-turn ResumeEvent can route RequestSweepBuild→broadcast before the spend notification parks in AwaitingExternalSpendFinality. Result: a rejected double-spend (→TxFailed, retry), fail-closed. The comment at reconcile.go:230-233 conflates registration with observation.
  • finishSweepSpend teardown keys on in-memory count. boarding_sweep_actor.go:~1282-1288: resolved || len(pending.inputs) == 0 can cancel watches while the store still holds pending inputs. Self-heals on the next tip resume; gating teardown on the store's authoritative resolved would be cleaner.
  • Resume Tells aren't coalesced. Each per-input persistence failure / re-arm self-Tells a full ResumeBoardingSweepsRequest, bypassing the tip-tick inflight latch. Bounded by input count; a dedup latch would cut DB churn.
  • Config depth resolvers don't clamp. waved/config.go:1384-1396: reorgSafetyDepth()/chainFinalityDepth() rely on Validate() having run. A Config near MaxUint32 overflows +1 to 0, read downstream as "finality synthesis disabled." Not reachable in production (startup validates), but a cheap defense-in-depth clamp; also no minimum floor, and the boarding systest still hardcodes chainsource.DefaultFinalityDepth rather than exercising the config-derived horizon.

Verified correct

Config: default 30 / max 144 / zero→default (config.go:154-162,1165,1386), +1 terminal derivation (chainFinalityDepth, :1396); v2 defined but never advertised. Unroll: reconcile-before-side-effects ordering (actor.go:1333-1373), reconcileCheckpoint refuses to proceed on any reconciler error (reconcile.go:114-207), terminal-persist-before-relive with withhold-and-retry on write failure (registry.go:1031-1055), the production reconciler never manufactures a negative (HasAuthoritativeNegativeEvidence() == false), and the relive gate requires both !HadOnChainFootprint and !ReliveUnsafe. Boarding: provisional-until-Done/discard-on-Reorged, durable per-input finalization, broadcaster-failure-as-uncertain.

Net: one fail-open at the upgrade boundary (High) worth fixing before merge; everything else is either intentional-for-this-PR or fail-closed liveness polish for follow-ups.


Methodology: direct reading of the reconcile/FSM/config/store code; the High finding re-verified against origin/main:unroll/snapshot.go (field absent), current checkpointVersion (still 1), and grep of every ReliveUnsafe set-site (only-ever-true).
· branch reorg-recovery-foundation

@ellemouton

Copy link
Copy Markdown
Member Author

@claude review this — the release-blocking upgrade finding is fixed in current head ce345672. Started legacy checkpoints with no relive-safety TLV now decode fail-closed as unsafe, while current started checkpoints encode an explicit boolean so a future authoritative clear remains distinguishable. The regression test removes the new record from a started checkpoint and pins ReliveUnsafe=true; go test -race ./unroll passes.

The remaining notes are value-safe liveness boundaries, not release blockers for this stack: authoritative-negative clearing is deliberately deferred, and unavailable chain evidence deliberately stalls rather than guesses. The boarding retry/coalescing items should stay follow-up work rather than expanding this reorg foundation. Please re-review the current head.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

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


Re-review: PR #964 — Configurable reorg recovery foundation

I re-verified the release-blocking upgrade finding against current head ce345672 (unroll: Fail closed on legacy checkpoints). It is correctly fixed.

  • Verify ReliveUnsafe fail-closed decode fix in snapshot.go
  • Verify the regression test pins ReliveUnsafe=true for legacy Started checkpoints
  • Confirm current Started checkpoints still encode an explicit boolean
  • Re-confirm the deferred liveness notes are value-safe follow-ups
  • Post verdict

✅ High finding resolved — legacy Started checkpoints now fail closed

The fix closes the fail-open at the upgrade boundary exactly as it should:

  • Decode (snapshot.go:519-527) — when checkpointReliveUnsafeRecordType is absent and the checkpoint is Started, ReliveUnsafe now defaults to true. checkpoint.Started is populated at :464, before this branch, so the gate reads the correct value. A restored pre-PR in-flight job therefore decodes with the guard up, and recoverable = req.Phase == PhaseFailed && !HadOnChainFootprint && !ReliveUnsafe (registry.go:292-293, :374-376) evaluates to false → the VTXO is held in unilateral exit instead of relived. The precise "silence treated as canonical absence" outcome the invariant forbids is gone.

  • Encode (snapshot.go:330-341) — a Started checkpoint now always writes the record (0 or 1) rather than only-when-true. This is the key detail that keeps a future authoritative clear (ReliveUnsafe = false written explicitly) distinguishable from a legacy omission: an explicit 0 on a Started checkpoint means "cleared by evidence," while an absent record on a Started checkpoint means "legacy, assume unsafe." Non-started checkpoints correctly omit it (no chain-boundary work possible). checkpointVersion stays 1, so old rows are still accepted rather than rejected — the right call.

  • Regression test (snapshot_test.go:225-251)TestCheckpointCodecLegacyStartedDefaultsReliveUnsafe encodes a Started checkpoint, strips the trailing {29, 1, 0} record to reproduce the pre-field wire shape, and asserts the decode yields Started == true && ReliveUnsafe == true. I confirmed record type 29 is the highest record type in the codec (snapshot.go:129), so it is genuinely the last record on the wire and the suffix-strip is sound. Good, targeted pin.

The comments on both the constant (:124-129) and the decode branch (:522-526) now describe the semantics accurately.


Remaining notes — agreed, all follow-up work

I confirm the outstanding items are value-safe liveness boundaries, not release blockers for this stack, and agree with deferring them:

  • Authoritative-negative clearing deliberately deferred — nothing clears ReliveUnsafe and the production reconciler reports no authoritative negative evidence, so ExitOutcomeRecoverable is intentionally unreachable for new jobs until the next client PR adds the durable canonicality barrier. With the High fix in place, the previously-buggy upgrade path is now also closed, so the recoverable-relive path is uniformly inert — exactly the intended boundary.
  • Unavailable chain evidence stalls rather than guesses — the fail-closed head-of-line stall on genuinely-reorged-while-offline anchors is a deliberate correctness-over-liveness choice; a bounded backoff is reasonable follow-up.
  • Boarding retry / coalescing items (partially-externally-spent sweep re-broadcast, finishSweepSpend teardown, un-coalesced resume Tells) — real liveness polish, but all fail-closed (no loss / no double-spend), so keeping them out of this foundation PR is the right scope call.

Verdict

The one release-blocking finding is fixed correctly and pinned by a regression test; everything else is intentional-for-this-PR or fail-closed liveness polish tracked for follow-ups. No blocking issues remain from my review. LGTM. 🎉

Methodology: read the fix diff at ce345672, verified decode ordering (Started set before the guard branch), the encode-always-for-Started change, the relive gate consumers in registry.go, that checkpointVersion is unchanged, and that record type 29 is the highest so the test's suffix-strip reproduces the legacy wire shape.
· branch reorg-recovery-foundation

@ellemouton
ellemouton force-pushed the reorg-recovery-foundation branch 2 times, most recently from 747f805 to 428fd45 Compare July 18, 2026 00:00
@ellemouton

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

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


Re-review: PR #964 — Configurable reorg recovery foundation

Re-reviewed at current head 428fd45. The stack was reworked since my last LGTM (ce345672): the passive "legacy fail-closed decode" fix is now folded into a stronger evidence-based relive mechanism (unroll: Make VTXO reliving evidence-based). I read the new commit end-to-end and re-verified the previously-flagged upgrade-boundary finding. The fix is correct and, if anything, cleaner than before.

  • Diff the new unroll: Make VTXO reliving evidence-based commit
  • Re-verify the legacy-checkpoint fail-closed fix
  • Re-check unilateral-exit reconcile/relive correctness
  • Re-check the new DefinitelyNotBroadcast evidence path
  • Re-check reorgsafetydepth/terminal-depth wiring (unchanged)
  • Post verdict

✅ What changed and why it's sound

The relive guard is now cleared only by positive evidence, not by absence:

  1. New DefinitelyNotBroadcast evidence flag flows txconfirm.EnsureConfirmedRespTxFailedEventapplyFailedEvent (fsm_logic.go:504-506). It clears ReliveUnsafe only when the responder can prove no broadcast crossed the chain boundary. The field's conservative default is false (txconfirm/messages.go:175-179).
  2. The real txconfirm actor never sets it — every EnsureConfirmedResp constructed in txconfirm/actor.go (:1345, :1417, etc.) leaves it at the zero value. Grep confirms the only true producer is the test-only faultyUnrollTxConfirm decorator (waved/server.go:5851), which reproduces the vtxo: failed unilateral exit silently removes VTXO from wallet's live set despite no on-chain spend #602 pre-broadcast-rejection scenario. So in production the guard is never cleared through this path → the recoverable-relive path stays fail-closed exactly as the PR's stated boundary requires, but the mechanism is now genuinely exercised end-to-end rather than being dead code.
  3. Legacy Started checkpoints still fail closed. decodeCheckpoint (snapshot.go:519-527) defaults ReliveUnsafe = true when the record is absent and checkpoint.Started is true. Started is populated at :464, before the branch, so the gate reads correctly. checkpointVersion stays 1 (:51), so old rows are accepted rather than rejected — the right call.
  4. Encode is now explicit for Started checkpoints. encodeCheckpoint (:330-341) always writes 0 or 1 for a Started checkpoint, so a future authoritative clear (explicit 0) stays distinguishable from a legacy omission (record absent). Non-started checkpoints correctly omit it.

Correctness of the per-tx clear vs. a job-level guard: clearing a job-wide ReliveUnsafe from a single tx's DefinitelyNotBroadcast is sound here because a proof-tx failure is terminal (applyFailedEvent's proof-tx arm is first-failure-wins), and the sweep-failure arm returns early via applySweepBuildFailed (fsm_logic.go:489-495) before ever reaching the clear. So only a single, terminal, proof-tx failure can clear the guard, and the relive gate still requires both !HadOnChainFootprint && !ReliveUnsafe (registry.go). Minor suggestion (nit, non-blocking): a one-line comment at fsm_logic.go:504 noting that this per-tx clear is only safe because proof-tx failure is terminal would protect it against a future change that batches or de-terminalizes proof failures.

Tests pin all three behaviors:

  • TestProofTxFailureTransitionsToFailed now asserts ReliveUnsafe stays true on an ambiguous failure.
  • TestDefiniteNoBroadcastClearsReliveGuard asserts an explicit no-broadcast failure clears it.
  • TestCheckpointCodecLegacyStartedDefaultsReliveUnsafe strips the trailing {29,1,0} record (type 29 is the highest, so it's genuinely last on the wire) and pins the legacy fail-closed decode.

Remaining notes — unchanged, all fail-closed follow-ups

The value-safe liveness boundaries from prior rounds still stand and remain appropriate to defer:

  • Authoritative-negative clearing still deferred — the real txconfirm doesn't yet populate DefinitelyNotBroadcast, so for real jobs the recoverable path stays inert until the next client PR wires the durable canonicality barrier. Intentional.
  • Genuine offline reorg can head-of-line-block restart (reconcile_chainsource.go returns Some-or-error, never None) — deliberate correctness-over-liveness stall; a bounded backoff is reasonable follow-up.
  • Boarding retry/coalescing polish (partially-externally-spent sweep re-broadcast, finishSweepSpend in-memory teardown, un-coalesced resume Tells) and the config depth clamp / systest horizon nit — all fail-closed, correctly out of scope for this foundation PR.

Verdict

The previously release-blocking upgrade-boundary fail-open is fixed, and the rework replaces "silence-is-not-evidence" with an explicit positive-evidence clear that keeps production fail-closed while making the recoverable path testable. Regression tests pin all three behaviors; checkpointVersion correctly unchanged. No blocking issues remain. LGTM. 🎉

Methodology: read the full 428fd45 diff; traced DefinitelyNotBroadcast from txconfirm through the FSM to the relive gate; grep-confirmed the real txconfirm never sets it (only the test-only fault decorator does); verified decode ordering (Started set before the guard branch), the terminal proof-tx-failure semantics that make the per-tx clear sound, and that checkpointVersion is still 1.
· branch reorg-recovery-foundation

@ellemouton
ellemouton force-pushed the reorg-recovery-foundation branch from 428fd45 to b6c2fd0 Compare July 20, 2026 16:24
@ellemouton

Copy link
Copy Markdown
Member Author

@claude review this — final head is now rebased onto current main, including the round-status follow-up required by Lumos master.

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

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


Re-review: PR #964 — Configurable reorg recovery foundation (rebased final head)

Re-reviewed at current head b6c2fd0, now rebased onto current main. I re-verified the full recovery diff against origin/main and re-checked every invariant from my prior LGTM. The rebase is clean and the recovery contract is unchanged and still correct.

  • Diff current head vs origin/main; identify changes since last review
  • Locate & assess the "round-status follow-up"
  • Re-verify the upgrade-boundary fail-closed fix
  • Re-check unroll reconcile/relive + boarding-sweep recovery
  • Re-check reorgsafetydepth/terminal-depth wiring
  • Post verdict

Scope of this rebase

The PR's own diff is functionally identical to the 428fd45 head I last approved: same four commits (unilateral exits recoverableboarding sweeps recoverablewire configurable policyVTXO reliving evidence-based), same changed-file set. The round-status follow-up "required by Lumos master" is a base-side change — none of this PR's 40 changed files touch round/; the merge of #992 (forfeit-release-reconcile, which owns round/status_reconcile*) landed on main and is what necessitated the rebase. It is correctly outside this PR's review diff. No conflict artifacts remain in the recovery packages (git grep for merge markers is clean).

Re-verified invariants on the rebased tree

  • Legacy checkpoints still fail closed. decodeCheckpoint (snapshot.go:519-526) defaults ReliveUnsafe = true when the record is absent and checkpoint.Started; encode always writes an explicit 0/1 for Started checkpoints (:333-341), keeping a future authoritative clear distinguishable from a legacy omission. checkpointVersion is still 1 (:51), so old rows are accepted, not rejected — the right call.
  • Relive is cleared only by positive evidence. applyFailedEvent clears ReliveUnsafe solely on event.DefinitelyNotBroadcast (fsm_logic.go:504-505), and the proof-tx-failure arm is terminal (first-failure-wins), so only a single terminal proof-tx failure can reach the clear. Grep confirms ReliveUnsafe is otherwise only ever set to true (fsm_logic.go:83, fsm_types.go:411,423).
  • Production still cannot clear the guard. The only DefinitelyNotBroadcast: true producer is the test-only fault decorator (waved/server.go:5865); every real EnsureConfirmedResp in txconfirm leaves it at the conservative false (messages.go:175-179). So the recoverable-relive path stays inert for real jobs until the next PR wires the durable canonicality barrier — the PR's stated boundary.
  • Relive gate unchanged. recoverable = req.Phase == PhaseFailed && !req.HadOnChainFootprint && !req.ReliveUnsafe (registry.go:977-978, :1098-1099).
  • Config/version wiring unchanged. reorgsafetydepth default 30 / max 144, +1 terminal derivation; ArkProtocolVersionV2 defined but clientSupportedArkVersions() returns v1 only. The PhaseExternalSpendObserved → MATERIALIZING proto collapse (rpc_server.go:5283) is documented as an intentional coarsening of the parked window, with the true phase always reconstructed from the durable checkpoint.

Remaining notes — unchanged, all fail-closed follow-ups

Same value-safe liveness boundaries as prior rounds, still appropriate to defer: authoritative-negative clearing deferred to the next client PR; genuine-offline-reorg head-of-line stall (correctness-over-liveness); boarding retry/coalescing polish (partially-externally-spent sweep re-broadcast, finishSweepSpend in-memory teardown, un-coalesced resume Tells); and the config-depth clamp / systest-horizon nit. All fail-closed (no loss, no double-spend).

Verdict

The rebase preserves the evidence-based relive fix intact, introduces no recovery-code changes, and carries no conflict residue; the round-status follow-up is a base-side dependency correctly kept out of this diff. No blocking issues remain. LGTM. 🎉

Note: go build needs approval under the current tool permissions, so I verified the rebased head by direct code reading + git grep rather than a local compile; CI covers the build/test matrix. Methodology: diffed all 40 files vs origin/main, re-traced DefinitelyNotBroadcast from txconfirm→FSM→relive gate, grep-confirmed the real txconfirm never sets it, checked decode ordering (Started before the guard branch), confirmed checkpointVersion still 1, and scanned the recovery packages for merge markers (none).
· branch reorg-recovery-foundation

Unilateral exits previously treated confirmation and spend observations
as one-way facts.  A chain replacement could therefore leave an actor
driving side effects from stale proof, checkpoint, or sweep state,
especially after a restart replayed that state without first checking
the canonical chain.

Persist the chain anchors behind each transition and keep confirmation,
sweep, and external-spend evidence reversible until policy finality.
Reconcile every persisted positive observation before restart side
effects, while treating a missing observation or lookup timeout as
unknown rather than proof of absence.

Keep the source VTXO under a durable unsafe hold whenever an admitted
exit has an ambiguous footprint.  A failed exit may restore the VTXO
only after both the local footprint is empty and the chain boundary
supplies objective canonical-absence evidence.  Persist terminal exit
state before forwarding the corresponding VTXO lifecycle outcome.

Use versioned TLV messages and snapshots for the new durable state so
rolling upgrades can replay it safely.  Extend the actor, registry,
reconciliation, restart, and reorg tests to cover reordered and
duplicated observations.
Boarding sweeps previously collapsed the first confirmation or spend
notification into terminal wallet state.  If that evidence was later
reorged out, the actor could no longer distinguish a canonical spend
from a provisional one and could restore or retire inputs incorrectly.

Keep the sweep confirmation and a separate spend watch for every
consumed boarding input active until the chain source reports policy
finality.  Record positive observations provisionally, remove them on
Reorged, and finalize each consumed input durably so a later sweep-level
failure cannot make already spent value available again.

Treat a broadcast failure as an uncertain attempt instead of evidence
that no transaction reached the chain.  Recovery remains bounded and
tip-paced, and a restart reconstructs the outstanding watches from
persisted actor state.

Add focused actor and store coverage plus a real LND/Bitcoin system test
that drives a boarding sweep through confirmed, reorged, and
reconfirmed states.
The reorg-aware actors need one explicit policy boundary.  Without
shared wiring, individual subsystems can finalize at different depths or
retain watches indefinitely when a backend allocates a Done channel but
never writes to it.

Add reorgsafetydepth as the deepest chain replacement the client
promises to recover from.  Default it to 30 blocks, cap it at the
current cross-backend limit of 144, and begin terminal finality one
confirmation beyond the selected horizon.  Feed that boundary into
chain-source Done synthesis and unilateral exit restart reconciliation.

Define the reorg-safe Ark protocol version, but deliberately keep client
advertisement pinned to v1.  One-confirmation VTXO exposure remains
disabled until batch lineage, admission gates, server recovery, and
end-to-end safety tests land in the remaining stack.

Document the configuration boundary and test default, override,
validation, finality, and negotiation behavior.
Treat legacy started checkpoints and ambiguous broadcast failures as
unsafe. Only an explicit no-broadcast proof may clear the relive guard,
so a failed exit cannot restore a coin that may have crossed the chain
boundary.
@ellemouton
ellemouton force-pushed the reorg-recovery-foundation branch from b6c2fd0 to 604b856 Compare July 20, 2026 18:46
@ellemouton

Copy link
Copy Markdown
Member Author

@claude review this — current restacked head 604b856d after rebasing the full stack onto the latest upstream base. Please verify prior findings remain addressed and flag any new blocking issues.

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

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


Re-review: PR #964 — Configurable reorg recovery foundation (restacked head 604b856)

Re-reviewed at current head 604b856, restacked onto the latest upstream base. I re-diffed the full recovery set against origin/main and re-verified every invariant from my prior LGTM. The restack is clean, carries no conflict residue, and the recovery contract is unchanged and still correct.

  • Diff current head vs origin/main; identify changes since last review
  • Re-verify the upgrade-boundary fail-closed fix (ReliveUnsafe decode)
  • Re-verify evidence-based relive (DefinitelyNotBroadcast) path
  • Re-check unroll reconcile/relive + boarding-sweep recovery
  • Re-check reorgsafetydepth/terminal-depth + version wiring
  • Post verdict

Scope of this restack

The PR diff is the same four commits and the same 40-file set I approved at 428fd45 (unilateral exits recoverableboarding sweeps recoverablewire configurable policyVTXO reliving evidence-based). The restack is a base-side move onto latest main; no recovery-package file gained a functional change, and git grep for merge-conflict markers across unroll/, wallet/, waved/, db/, txconfirm/, chainsource/ is clean.

Re-verified invariants on the restacked tree

  • Legacy checkpoints still fail closed. decodeCheckpoint (unroll/snapshot.go:519-527) defaults ReliveUnsafe = true when the record is absent and checkpoint.Started is set; Started is populated before that branch, so the gate reads the correct value. encodeCheckpoint (:333-341) always writes an explicit 0/1 for Started checkpoints, keeping a future authoritative clear (0) distinguishable from a legacy omission. checkpointVersion is still 1 (:51), so old rows are accepted rather than rejected — the right call.
  • Relive is cleared only by positive evidence. applyFailedEvent clears ReliveUnsafe solely on event.DefinitelyNotBroadcast (unroll/fsm_logic.go:504-505), and the proof-tx-failure arm is terminal (first-failure-wins) — so only a single terminal proof-tx failure can reach the clear. Grep confirms ReliveUnsafe is otherwise only ever set to true (fsm_logic.go:83, fsm_types.go:411,423).
  • Production still cannot clear the guard. The only DefinitelyNotBroadcast: true producer is the test-only faultyUnrollTxConfirm decorator (waved/server.go:5865), which maybeFaultyUnrollTxConfirm only installs when Config.FailUnrollBroadcastReason != "" (:5813-5824); production returns the ref unchanged. Every real EnsureConfirmedResp in txconfirm leaves it at the conservative false (txconfirm/messages.go:175-179). So the recoverable-relive path stays inert for real jobs until the next PR wires the durable canonicality barrier — the PR's stated boundary.
  • Relive gate unchanged. recoverable = req.Phase == PhaseFailed && !req.HadOnChainFootprint && !req.ReliveUnsafe (unroll/registry.go:977-978, and :1099).
  • Config / version wiring unchanged. reorgsafetydepth default 30 / max 144 (config.go:154-162,1222), zero→default and +1 terminal derivation (reorgSafetyDepth/chainFinalityDepth, :1394-1406); ArkProtocolVersionV2 defined (arkrpc/version.go:20) but clientSupportedArkVersions() returns v1 only (operator_negotiation.go:80-81).

Prior findings — all remain addressed

  • High (upgrade-boundary fail-open) — fixed and intact. Legacy Started checkpoints decode with the guard up; the fix survives the restack unchanged.
  • Deferred liveness notes — unchanged, still value-safe follow-ups. Authoritative-negative clearing deferred to the next client PR; genuine-offline-reorg head-of-line stall (deliberate correctness-over-liveness); boarding retry/coalescing polish (partially-externally-spent sweep re-broadcast, finishSweepSpend in-memory teardown, un-coalesced resume Tells); and the config-depth clamp / systest-horizon nit. All fail-closed (no loss, no double-spend), correctly out of scope for this foundation PR.

Verdict

The restack preserves the evidence-based relive fix intact, introduces no recovery-code changes, and carries no conflict residue. All prior findings remain addressed and I found no new blocking issues. LGTM. 🎉

Note: git fetch/go build need approval under the current tool permissions, so I verified the restacked head by direct code reading + git grep/git diff against the already-present origin/main ref rather than a fresh fetch/compile; CI covers the build/test matrix. Methodology: diffed all 40 files vs origin/main, re-traced DefinitelyNotBroadcast from txconfirm→FSM→relive gate, grep-confirmed the real txconfirm never sets it (only the FailUnrollBroadcastReason-gated fault decorator does), checked decode ordering (Started before the guard branch), confirmed checkpointVersion still 1, and scanned the recovery packages for merge markers (none).
· branch reorg-recovery-foundation

@Roasbeef Roasbeef left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this review, I'm treating #964 as the client-side recovery foundation. The H/H+1 arithmetic and reversible chainsource lifecycle look sound as disabled substrate. The remaining comment is a cross-stack contract issue that needs to be closed before the capability is advertised.

Comment thread waved/config.go

// reorgSafetyDepth resolves the daemon's configured recovery horizon.
func (c *Config) reorgSafetyDepth() uint32 {
if c.ReorgSafetyDepth == 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the client layer, zero means use the 30-block default. Lumos #626 gives an explicit zero a disable/immediate-finality meaning. We can't advertise v2 with two meanings for the same knob: one deployment config can otherwise produce 31-conf client finality and first-conf server finality. This is fine as disabled foundation, but capability negotiation/config validation needs to carry the same nonzero H on both sides and reject a mismatch before activation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants