unroll: Reorg-safe unilateral-exit subsystem - #410
Conversation
There was a problem hiding this comment.
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.
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| if isTerminalTxState(state) { | ||
| if a.retryTerminalNotifications(ctx, entry) { | ||
| a.evictTerminal(ctx, entry) | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| if isTerminalTxState(state) { | ||
| if a.retryTerminalNotifications(ctx, entry) { | ||
| a.evictTerminal(ctx, entry) | ||
| } |
There was a problem hiding this comment.
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.
| if isTerminalTxState(state) { | ||
| a.evictTerminal(ctx, entry) | ||
| } |
There was a problem hiding this comment.
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.
5739f43 to
ac71c94
Compare
|
Claude finished @ellemouton's task in 6m 7s —— View job PR Review: unroll: Reorg-safe unilateral-exit subsystem
Overall AssessmentThe 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 PositivesAll four flagged call-sites ( // 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 Reconciliation Ordering Invariant (requested in test plan)The ordering in
Reconciliation runs before the FSM session is bound, and therefore before any Issue 1:
|
ac71c94 to
81194d3
Compare
f384aca to
ac3b5e8
Compare
5b0d113 to
0ce494d
Compare
57017bc to
d1c4f87
Compare
0ce494d to
eb1e0db
Compare
d1c4f87 to
f77cf15
Compare
b50815f to
d7c87b1
Compare
d430276 to
fe1f2b8
Compare
308c416 to
1e7b3bc
Compare
fe1f2b8 to
2ce8944
Compare
1e7b3bc to
53b0b2d
Compare
d849c1a to
e160c08
Compare
53b0b2d to
4b1f063
Compare
5a3bb65 to
8306b38
Compare
aebb4c7 to
3877221
Compare
270d74f to
01ecfd9
Compare
|
Triage linkage check — verified this PR's live-rollback reducer implements #207's full proposed solution (reorg notifications, roll back Suggest adding |
ca29548 to
a9ee5e8
Compare
01ecfd9 to
778e719
Compare
c86210e to
1ab3019
Compare
778e719 to
38cf5e6
Compare
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.
3eb6f61 to
449b6cf
Compare
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.
3609304 to
fc520a1
Compare
32871de to
4a1f9b1
Compare
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.
fc520a1 to
d52c121
Compare
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:
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.Finalizedterminal state;Confirmedis no longer terminal until finality.TxReorgedEvent/SpendReorgedEventthat prune the reorged subtree fromConfirmedTxids+InFlightTxidsand downgrade the sweep when the target is lost.AwaitingExternalSpendFinalitystate with a persistedProvisionalExternalSpendanchor so an external spend doesn't terminally fail the job until it's finalized.PhaseCompletedis provisional until the sweep isFinalized; the registry only firesUnrollTerminatedMsgafter finality.ChainReconcilerinterface. 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
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