Skip to content

unroll: Reorg-safe unilateral-exit subsystem - #410

Closed
ellemouton wants to merge 1 commit into
reorg-safe-chainsourcefrom
unroll-reorg-safety
Closed

unroll: Reorg-safe unilateral-exit subsystem#410
ellemouton wants to merge 1 commit into
reorg-safe-chainsourcefrom
unroll-reorg-safety

Conversation

@ellemouton

@ellemouton ellemouton commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

Makes the client-side unilateral-exit subsystem reorg-safe end-to-end. Today the unroll actor treats every confirmation as monotonic: if a proof-graph anchor or the sweep tx is reorged out (or vanishes during daemon downtime), the planner happily progresses off stale state and can broadcast a sweep against a target the chain no longer holds. This PR makes the entire stack reversible:

  • chainsource gains reorg-aware conf/spend watches with reorg + finality events (ConfReorgedEvent, ConfDoneEvent, spend equivalents). The conf/spend sub-actors are multi-shot, and finality is synthesized height-based (FinalityDepth) so backends that drop the underlying signal (lndclient) still produce Done.
  • chainbackends forwards lndclient reorg pings into the new event surface; bare correlation IDs only, since gRPC transport drops the depth/block-hash payloads.
  • txconfirm confirmation watches become reorg-aware end-to-end with a new Finalized terminal state; Confirmed is no longer terminal until finality.
  • unroll FSM gains:
    • Rollback semantics for TxReorgedEvent / SpendReorgedEvent that prune the reorged subtree from ConfirmedTxids + InFlightTxids and downgrade the sweep when the target is lost.
    • A new AwaitingExternalSpendFinality state with a persisted ProvisionalExternalSpend anchor so an external spend doesn't terminally fail the job until it's finalized.
    • PhaseCompleted is provisional until the sweep is Finalized; the registry only fires UnrollTerminatedMsg after finality.
    • Restart reconciliation against the canonical chain via a new ChainReconciler interface. The chainsource-backed implementation probes via future-mode RegisterConf/RegisterSpend with bounded timeouts, baked per-actor caller-IDs (target outpoint) so two restored actors probing a shared proof-graph ancestor cannot collide on chainsource service keys.

Tests

  • Unit: chainsource reorg-aware conf/spend actor tests, lndclient forwarder tests, txconfirm reorg lifecycle tests, unroll reorg-safety integration tests (proof-root reorg blocks downstream materialization, target reorg clears CSV maturity, sweep-confirmation reorg reverses Completed, external-spend reorg resumes actor, sweep stays provisional until finalized), restart-reconciliation tests (target gone, sweep gone, external-spend gone, target height refresh, combined offline-reorg restart), concurrent-probe service-key collision test.
  • Systest (against bitcoind + lnd in regtest): full chainsource conf reorg round-trip, full txconfirm reorg round-trip including the new TxReorged event.

Stacked on

Targets `recipient-fraud-watcher-client` because the two stacks both extend `unrollplan.State`, the unroll FSM, and the actor checkpoint codec. No functional dependency, but rebasing onto main would require manual conflict resolution in 7-8 files; keeping the stack until the fraud PR lands.

Test plan

  • CI green on unroll, chainsource, chainbackends, txconfirm, darepod unit suites
  • CI green on systest (`TestChainSourceConfReorgRoundTrip`, `TestTxConfirmReorgRoundTrip`)
  • `make lint-native` clean
  • Reviewer sanity-check that the restart-reconciliation ordering invariant (reconcile runs before FSM session is bound; before block sub / spend watch / ResumeEvent reissue) holds in `actor.go:ensureLoaded`

@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 comprehensive reorg-awareness across the chain notification pipeline, extending from the LND backend through the chainsource and txconfirm subsystems to the unroll registry. Key changes include the implementation of multi-shot confirmation and spend watches, height-based finality synthesis for gRPC-based backends, and a restart reconciliation mechanism to prune stale anchors. My feedback focuses on several resource leaks identified in the txconfirm actor, where the underlying chainsource watch is not explicitly unregistered before evicting terminal entries during cancellation, notification retries, or asynchronous completion.

Comment thread txconfirm/actor.go
Comment on lines 533 to 535
return nil, err
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The chainsource confirmation watch should be explicitly unregistered when a terminal entry is cancelled. If the entry reached a terminal state (Finalized or Failed) but was waiting for notification completion or retry, the watch might still be active in the background. Unregistering here ensures resource hygiene.

Comment thread txconfirm/actor.go
Comment on lines +589 to 593
if isTerminalTxState(state) {
if a.retryTerminalNotifications(ctx, entry) {
a.evictTerminal(ctx, entry)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The chainsource confirmation watch is leaked when a terminal notification is successfully delivered via the retry path in handleConfirmationObserved. While handleConfirmationDone correctly unregisters the watch upon immediate successful notification, this retry branch calls evictTerminal without first invoking unregisterConfWatch. This results in the underlying chainsource sub-actor remaining active indefinitely for finalized transactions that required notification retries.

Comment thread txconfirm/actor.go
Comment on lines +753 to 756
if isTerminalTxState(state) {
if a.retryTerminalNotifications(ctx, entry) {
a.evictTerminal(ctx, entry)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The chainsource confirmation watch is leaked when a terminal notification is successfully delivered via the retry path in handleBlockObserved. Similar to the issue in handleConfirmationObserved, this branch calls evictTerminal without first invoking unregisterConfWatch, leading to a resource leak in the chainsource actor system.

Comment thread txconfirm/actor.go
Comment on lines +1323 to 1325
if isTerminalTxState(state) {
a.evictTerminal(ctx, entry)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The chainsource confirmation watch is leaked when an asynchronous terminal notification completes. If handleConfirmationDone (or handleConfirmationFailed) defers notification to a goroutine, it returns early without unregistering the watch. When the result eventually arrives here, the entry is evicted but the watch is never cleaned up. Adding an explicit unregister call before eviction ensures the sub-actor is released.

@ellemouton
ellemouton force-pushed the recipient-fraud-watcher-client branch 3 times, most recently from 5739f43 to ac71c94 Compare May 13, 2026 15:57
@claude

claude Bot commented May 13, 2026

Copy link
Copy Markdown

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


PR Review: unroll: Reorg-safe unilateral-exit subsystem

  • Gather context: read PR diff and key files
  • Review chainsource reorg-aware conf/spend actors
  • Review txconfirm reorg lifecycle and resource cleanup
  • Review unroll FSM rollback semantics and new states
  • Review restart reconciliation ordering invariants
  • Review chainbackends lndclient forwarding
  • Synthesize findings and post full review

Overall Assessment

The design is sound end-to-end. The reorg rollback semantics, finality synthesis, provisional-state promotion, and restart-reconciliation ordering all hold up under scrutiny. A few targeted notes below.


Gemini's Resource-Leak Findings Are False Positives

All four flagged call-sites (actor.go:535, actor.go:593, actor.go:756, actor.go:1325) pass through evictTerminal, which already guards the unregister:

// txconfirm/actor.go:1240-1246
func (a *TxBroadcasterActor) evictTerminal(ctx context.Context, entry *trackedTx) {
    if entry.confWatchRegistered {
        if err := a.unregisterConfWatch(ctx, entry); err != nil { ... }
    }
    ...
}

The confWatchRegistered sentinel means double-unregisters are impossible and the watch is freed exactly once regardless of which eviction path wins. No leak.


Reconciliation Ordering Invariant (requested in test plan)

The ordering in ensureLoaded (unroll/actor.go:611–688) is correct:

  1. Load proof
  2. Load descriptor
  3. Construct planner
  4. reconcileOnRestart() — mutates b.pending in place before any session exists
  5. Build FSM session from post-reconciliation checkpoint
  6. ensureBlockSubscription
  7. ensureSpendWatch

Reconciliation runs before the FSM session is bound, and therefore before any ResumeEvent reissue path can fire (which lives inside driveEvent → routeOutbox). No stale sweep can be re-broadcast off a checkpoint the reconciler has not yet validated.


Issue 1: reconcileCheckpoint probes already-pruned txids — potential slow restart

reconcileCheckpoint (unroll/reconcile.go:117–150) iterates over a snapshot of confirmedIn, but pruneReorgedSubtree removes descendants from the live slice while the loop runs. If the slice contains both an ancestor A and descendant B (sorted by txid hash, not topology), and A is processed first and its subtree (including B) is pruned, the loop will still issue reconciler.ConfirmedTx(B) when it reaches B — a 10-second probe that can never return a useful answer.

With a deep confirmation chain and multiple reorged ancestors, each redundant probe burns up to defaultReconcileProbeTimeout (10s). A 5-deep proof graph with 2 reorged subtrees could add 30+ seconds to restart.

Suggested fix — skip txids already absent from checkpoint.State.ConfirmedTxids:

for _, txid := range confirmedIn {
    // Skip: already pruned by an earlier ancestor's subtree eviction.
    if !containsHash(checkpoint.State.ConfirmedTxids, txid) {
        continue
    }
    anchor, err := reconciler.ConfirmedTx(ctx, txid)
    ...
}

Fix this →


Issue 2: cleanupConfWatch fires unconditionally — wasted unregister per successful probe

In unroll/reconcile_chainsource.go:179–180:

defer r.cleanupConfWatch(callerID, &probeTxid, probePkScript)
event, err := confResp.Future.Await(probeCtx).Unpack()

The defer runs even on the success path, sending an UnregisterConfRequest for a service key whose ConfActor already self-exited after delivering the future-mode event. The request is harmless (chainsource no-ops on unknown keys) but redundant — one extra round-trip per confirmed txid per restart. The same applies to cleanupSpendWatch (reconcile_chainsource.go:254–255).

Consider returning early from the deferred cleanup when the future succeeded:

probeSucceeded := false
defer func() {
    if !probeSucceeded {
        r.cleanupConfWatch(callerID, &probeTxid, probePkScript)
    }
}()
event, err := confResp.Future.Await(probeCtx).Unpack()
if err == nil {
    probeSucceeded = true
    return fn.Some(...), nil
}

Fix this →


Issue 3: Stale comment in notifyRegistryIfTerminal

unroll/actor.go:1322:

// 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.

This was accurate before this PR stack, but darepod: Enable height-based finality on the chainsource actor (commit 6f18ca9) wires FinalityDepth > 0 in production, so TxFinalizedMsg will now arrive for confirmed sweeps and sweepFinalized will be latched. The "Phase 7 follow-up" reference is now stale and could mislead future reviewers. Suggest updating to note that height-based finality is active when FinalityDepth > 0 is configured (production default), with the lndclient-no-Done caveat still applying to deployments that have not set it.


Correctness Notes (all confirmed good)

maybeLatchSweepFinalized ordering (actor.go:235–242): The latch runs after ensureLoaded but before handleEvent, so notifyRegistryIfTerminal inside driveEvent sees sweepFinalized = true and fires UnrollTerminatedMsg on the same message turn. Correct.

ConfActor height-based finality (chainsource/conf_actor.go:318–334): Block subscription arms on first confirmation (a.blockReg == nil guard), resets confirmHeight = 0 on reorg, and resumes depth counting from the next reconfirmation height. The single block registration correctly services multiple confirm→reorg→reconfirm cycles. Correct.

handleConfirmationDone state guard (txconfirm/actor.go:683–685): Drops ConfDoneEvent when state is not Confirmed (e.g. after a reorg moved it back to AwaitingConfirmation). This is safe because the chainsource ConfActor continues watching and will fire a fresh Confirmed then Done sequence after re-confirmation. Correct.

reconcileExternalSpend ordering vs sweep reconciliation (unroll/reconcile.go:193–272): Appears unsafe at first glance — if the sweep was Broadcasted its txid is cleared by the sweep reconciliation block, so the benign-spender check Sweep.Txid.IsSome() would miss. However, SpentOutpoint only fires for confirmed spends; a sweep that is only in the mempool does not produce a spend event within the probe timeout. And a confirmed sweep would have ConfirmedTx = Some, so its txid would not be cleared in the first place. Ordering is safe.

lndclient Done channel (chainbackends/lndclient_adapters.go:279–287): Correctly allocated but never written to, with FinalityDepth acting as the production substitute. The comment is accurate. The chainntnfs.ConfirmationEvent.Done and SpendEvent.Done being live-but-never-signalled channels never race because downstream code (ConfActor/SpendActor) checks them in select arms that have competing height-based finality arms — whichever fires first wins and the goroutine returns.


@ellemouton
ellemouton force-pushed the recipient-fraud-watcher-client branch from ac71c94 to 81194d3 Compare May 13, 2026 16:43
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from f384aca to ac3b5e8 Compare May 13, 2026 23:24
@ellemouton
ellemouton changed the base branch from recipient-fraud-watcher-client to reorg-safe-chainsource May 13, 2026 23:24
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch 4 times, most recently from 5b0d113 to 0ce494d Compare May 14, 2026 18:21
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch 2 times, most recently from 57017bc to d1c4f87 Compare May 14, 2026 19:04
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch from 0ce494d to eb1e0db Compare May 14, 2026 23:32
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from d1c4f87 to f77cf15 Compare May 14, 2026 23:41
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch 2 times, most recently from b50815f to d7c87b1 Compare May 15, 2026 00:03
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch 2 times, most recently from d430276 to fe1f2b8 Compare May 15, 2026 01:49
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch 2 times, most recently from 308c416 to 1e7b3bc Compare May 15, 2026 12:40
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from fe1f2b8 to 2ce8944 Compare May 15, 2026 13:07
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch from 1e7b3bc to 53b0b2d Compare May 15, 2026 15:43
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch 2 times, most recently from d849c1a to e160c08 Compare May 15, 2026 17:21
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch from 53b0b2d to 4b1f063 Compare May 15, 2026 17:48
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch 2 times, most recently from 5a3bb65 to 8306b38 Compare May 15, 2026 17:53
@levmi

levmi commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Triage linkage check — verified this PR's live-rollback reducer implements #207's full proposed solution (reorg notifications, roll back ConfirmedTxids, re-register conf watches, reset CSV-maturity baseline, backward state transition) and then some.

Suggest adding Closes #207 so merge auto-closes it. (Heads-up: this is a draft stacked on a non-main base — the auto-close will only fire once it lands on the default branch.)

@levmi levmi added P1 Priority 1 — high and removed P2 Priority 2 — medium labels Jun 15, 2026
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch 3 times, most recently from ca29548 to a9ee5e8 Compare June 23, 2026 17:15
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from 01ecfd9 to 778e719 Compare June 24, 2026 22:03
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch 2 times, most recently from c86210e to 1ab3019 Compare June 29, 2026 15:05
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from 778e719 to 38cf5e6 Compare June 29, 2026 15:33
ellemouton added a commit that referenced this pull request Jun 29, 2026
An unroll broadcasts the VTXO's exit tree, which spends a commitment
batch output. If a batch in the VTXO's source lineage is permanently
invalidated (a consumed input was double-spent past finality), the exit
tree can never confirm, so a fresh admission is pointless. EnsureUnroll
now consults the target VTXO's full source-lineage canonicality (the
direct commitment txid plus every ancestor commitment txid) before
spawning a new child and refuses with ErrSourceLineageUnavailable when
the lineage is Invalidated (darepo#454).

It deliberately blocks ONLY the terminal Invalidated verdict, not the
transient LimboReorg / LimboConflict states: a reorged-out batch is
expected to re-confirm on its own and a not-yet-final conflict may still
resolve in the batch's favor, so blocking those would risk dropping a
needed critical-expiry / fraud-triggered exit during exactly the window
it matters — and the critical-expiry safety net reaches this gate via a
fire-and-forget Tell whose refusal cannot be observed or retried. An
already-admitted unroll tolerates a transiently-absent parent by
reconciling its own anchors (#410), so a fresh safety exit is admitted
for the same transient condition. The gate is also fail-permissive: a
descriptor-load or canonicality-lookup error logs and admits rather than
blocking an exit.

Gated behind an optional RegistryConfig.BatchCanonicality store (nil =
dormant, matching the C5-C7 contract); permissive for unseen /
unregistered lineage; only fresh admissions are gated.

Unit tests cover the blocked-on-invalidated-ancestor case, the
permitted transient-reorg and canonical cases, the unregistered and
load-failure (permissive) cases, the dormant no-op, and the
errors.Is-matchable wrapped sentinel.
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch from 3eb6f61 to 449b6cf Compare July 1, 2026 16:09
ellemouton added a commit that referenced this pull request Jul 1, 2026
An unroll broadcasts the VTXO's exit tree, which spends a commitment
batch output. If a batch in the VTXO's source lineage is permanently
invalidated (a consumed input was double-spent past finality), the exit
tree can never confirm, so a fresh admission is pointless. EnsureUnroll
now consults the target VTXO's full source-lineage canonicality (the
direct commitment txid plus every ancestor commitment txid) before
spawning a new child and refuses with ErrSourceLineageUnavailable when
the lineage is Invalidated (darepo#454).

It deliberately blocks ONLY the terminal Invalidated verdict, not the
transient LimboReorg / LimboConflict states: a reorged-out batch is
expected to re-confirm on its own and a not-yet-final conflict may still
resolve in the batch's favor, so blocking those would risk dropping a
needed critical-expiry / fraud-triggered exit during exactly the window
it matters — and the critical-expiry safety net reaches this gate via a
fire-and-forget Tell whose refusal cannot be observed or retried. An
already-admitted unroll tolerates a transiently-absent parent by
reconciling its own anchors (#410), so a fresh safety exit is admitted
for the same transient condition. The gate is also fail-permissive: a
descriptor-load or canonicality-lookup error logs and admits rather than
blocking an exit.

Gated behind an optional RegistryConfig.BatchCanonicality store (nil =
dormant, matching the C5-C7 contract); permissive for unseen /
unregistered lineage; only fresh admissions are gated.

Unit tests cover the blocked-on-invalidated-ancestor case, the
permitted transient-reorg and canonical cases, the unregistered and
load-failure (permissive) cases, the dormant no-op, and the
errors.Is-matchable wrapped sentinel.
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from 3609304 to fc520a1 Compare July 1, 2026 16:17
@levmi levmi added reorg safety Fund-safety: stuck, lost, or mis-counted funds unroll labels Jul 6, 2026
@ellemouton
ellemouton force-pushed the reorg-safe-chainsource branch 2 times, most recently from 32871de to 4a1f9b1 Compare July 8, 2026 20:41
Squashed for the btcd v2 port. Reorg-safe unroll: reversible
external-spend detection, sweep/target/proof-node rollback on
TxReorged, PhaseCompleted provisional until sweep finality, and a
ChainReconciler for restart reconciliation. Persisted in the
checkpoint so a mid-finality-window restart rehydrates.
@ellemouton

Copy link
Copy Markdown
Member Author

Superseded by #897 as part of condensing the reorg-safety client stack (epic lightninglabs/darepo#454) from 12 PRs into 3. The commits are carried over unchanged; see #897. Branch retained as a backup.

@ellemouton ellemouton closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Priority 1 — high reorg safety Fund-safety: stuck, lost, or mis-counted funds unroll

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants