multi: batch canonicality + VTXO admission gates - #896
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a batch canonicality data model and manager to handle reorg-safety for batch (commitment) transactions. It adds a new batchcanon package to track batch states (e.g., Provisional, Finalized, ReorgedOut, ConflictFinalized) and a BatchCanonicalityPersistenceStore for durable storage. The RoundClientActor and VTXOManager are updated to integrate with this gate, ensuring that VTXOs are excluded from coin selection if their batch lineage is not canonical. My review identified a critical synchronization issue in the VTXO manager's forfeit restore logic, a potential data loss scenario in the batch canonicality manager's restore retry logic, and a memory leak in the round actor's confirmation cache.
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.
| if _, ok := m.actors[req.Outpoint]; ok { | ||
| m.logger(ctx).DebugS(ctx, "Forfeited VTXO already has a live "+ | ||
| "actor; skipping restore", | ||
| slog.String("outpoint", req.Outpoint.String())) | ||
|
|
||
| return fn.Ok[ManagerResp](&RestoreForfeitedVTXOResponse{}) | ||
| } |
There was a problem hiding this comment.
If UpdateVTXOStatus fails on the first attempt, the function returns an error, but the spawned actor remains in m.actors.
On a subsequent retry, the check _, ok := m.actors[req.Outpoint] succeeds, causing the function to return fn.Ok with Restored: false without ever updating the DB status to VTXOStatusLive.
This leaves the VTXO in VTXOStatusForfeited in the database. Upon daemon restart, the VTXO will not be loaded as live, resulting in a permanent loss of funds.
To fix this, if the actor is already resident but the DB status is still Forfeited, we should still execute UpdateVTXOStatus(ctx, req.Outpoint, VTXOStatusLive) to ensure the database is synchronized with the in-memory state.
| if _, ok := m.actors[req.Outpoint]; ok { | |
| m.logger(ctx).DebugS(ctx, "Forfeited VTXO already has a live "+ | |
| "actor; skipping restore", | |
| slog.String("outpoint", req.Outpoint.String())) | |
| return fn.Ok[ManagerResp](&RestoreForfeitedVTXOResponse{}) | |
| } | |
| if _, ok := m.actors[req.Outpoint]; ok { | |
| m.logger(ctx).DebugS(ctx, "Forfeited VTXO already has a live "+ | |
| "actor; ensuring DB status is updated", | |
| slog.String("outpoint", req.Outpoint.String())) | |
| if err := m.cfg.Store.UpdateVTXOStatus( | |
| ctx, req.Outpoint, VTXOStatusLive, | |
| ); err != nil { | |
| return fn.Err[ManagerResp]( | |
| fmt.Errorf("restore forfeited vtxo status: %w", err), | |
| ) | |
| } | |
| return fn.Ok[ManagerResp](&RestoreForfeitedVTXOResponse{Restored: true}) | |
| } |
| for _, op := range consumed { | ||
| if err := m.cfg.RestoreConsumedVTXO(ctx, op); err != nil { | ||
| m.logger(ctx).WarnS(ctx, "Failed to restore forfeited "+ | ||
| "VTXO after batch invalidation", err, | ||
| slog.String("batch", txid.String()), | ||
| slog.String("vtxo", op.String())) | ||
|
|
||
| // Keep the edges so the restore can be retried; do not | ||
| // drop them on partial failure. | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
If RestoreConsumedVTXO fails for any of the outpoints, restoreProvisionalConsumers returns early. While this keeps the remaining edges in the database, the batch's state has already been updated to StateConflictFinalized in both the database and the in-memory w.persisted field.
Because w.persisted is now StateConflictFinalized, any future events for this batch will derive StateConflictFinalized, matching w.persisted, and deriveAndPersist will return early without calling handleConsumerLifecycle or restoreProvisionalConsumers again. Furthermore, since StateConflictFinalized is considered a final state, it is not loaded or watched during Reconcile on startup.
This means any failed restore will never be retried, leaving the forfeited VTXOs permanently locked/forfeited and the provisional consumer edges orphaned in the database.
Consider implementing a retry mechanism, or scanning for and restoring any remaining provisional consumers in the database during startup/reconciliation.
|
|
||
| return fn.Ok[actormsg.RoundActorResp](nil) | ||
| } | ||
| delete(a.pendingCommitmentConfs, event.Txid) |
There was a problem hiding this comment.
The pendingCommitmentConfs map caches confirmation events for tracked commitment transactions. While entries are deleted upon successful finalization in handleCommitmentFinalized, they are never cleaned up if a round fails, aborts, or times out. This can lead to a gradual memory leak of cached confirmation events over the lifetime of the daemon.
Ensure that pendingCommitmentConfs is also cleaned up whenever a round is removed or cleaned up from the actor's tracking maps (e.g., during round failure or abort).
4a5959f to
94b2927
Compare
11c74f1 to
4a2b582
Compare
|
@claude review this |
|
Claude finished @ellemouton's task in 3m 37s —— View job Claude PR Review — batch canonicality + VTXO admission gatesI reviewed the diff against I focused on correctness of the reorg-reversal paths and the gate wiring. I confirm all three findings from the automated review and add two of my own. Confirmed findings1. 2.
The orphaned edges + still-forfeited VTXOs are stranded. This compounds with finding #1 (even if the edge were retried, the resident-actor guard would swallow it). Worth a startup scan that restores any batch still carrying 3. Additional observations4. 5. Minor: Things that looked good on close inspection
Nothing here is a merge blocker for the stack's intent, but findings #1 and #2 are reorg-reversal correctness gaps (funds stuck forfeited) that I'd fix before this gates live in anger. |
|
@claude review this — pushed fixes addressing the prior review comments. |
|
Claude finished @ellemouton's task in 1m 19s —— View job Re-review — fixes for prior review commentsI re-reviewed the three new commits (
✅ Finding #1 — resident forfeited-VTXO DB reconcile (
|
dd14210 to
1750f7b
Compare
588a5b6 to
4e321b4
Compare
1750f7b to
d63bd67
Compare
d0fc059 to
e2a235a
Compare
Add the reorg-aware chain-observation substrate the rest of the reorg-safety stack consumes: a seq-ordered conf/spend watch lifecycle (Confirmed -> Reorged -> re-Confirmed -> Done) with height-based finality synthesis for backends that cannot deliver a native Done (gRPC lndclient and lwwallet). Finality synthesis is armed off the select loop, tied to the sub-actor's long-lived context (a per-attempt timeout would tear the in-process block-epoch forwarder down the instant it armed), retried with a capped exponential backoff until it succeeds or the watch's context is cancelled (a single-confirmation tx has no later event to piggy-back a retry on), and evaluated against the best height captured at arm time so a tx already buried past FinalityDepth when the watch arms finalizes immediately instead of hanging for the next block.
Deliver the full TxConfirmed/TxReorged/re-TxConfirmed/TxFinalized/ TxFailed cycle on confirmation watches, with a terminal seal guarding the reversible fire-and-forget deliveries and a catch-up TxReorged when a reorg lands while a subscriber's initial TxConfirmed is still parked on the async notify path.
Forward the lnd and lndclient chain-notifier reorg/finality signals (NegativeConf -> reorg) into the reorg-aware chainsource lifecycle with buffered forwarding.
Reorg-aware lwwallet/Esplora backend: TipPoller same-height and deeper reorg detection via PrevBlock continuity, a unified ChainEvent stream, and BlockDisconnected emission to btcwallet before connecting the replacement tip.
Forward Neutrino chain-notifier reorg signals into the reorg-aware chainsource lifecycle.
Consume the reorg-aware substrate at the edges: make the wallet boarding sweep reorg-aware, enable height-based finality on the darepod chainsource actor, add the harness GetRawTransaction / SignedV3Tx helpers, and add the end-to-end reorg systests for chainsource and txconfirm.
Squashed for the btcd v2 port. Durable batch-canonicality data model: batchcanon package (State/Record/EffectiveExpiry/ProvisionalConsumer, behavior-free Store), DB migration + persistence store + BackfillFromVTXOs, and the expiry-as-terminal audit. No manager, chain watching, or admission (those are C3+).
Squashed for the btcd v2 port. The batchcanon.Manager actor: one reorg-aware conf watch per batch + one spend watch per consumed input via chainsource, derives State by priority, recomputes effective expiry on reconfirm, and reconciles non-final watches on restart. No admission (that is C5).
Squashed for the btcd v2 port. batchcanon Availability vocab (available_final/provisional/unknown, limbo_reorg/conflict, invalidated) + CombineAvailability + store-driven LineageBlocked, and the vtxo.Manager coin-selection/forfeit admission gate that drops candidates whose batch lineage is limbo/invalidated. Permissive for unseen/unregistered; no-op when the store is nil.
Squashed for the btcd v2 port. The round registers its round-born batch + consumed inputs with the canonicality manager, and gates pre-commitment progression on consumed-input canonicality (finality gate kept as interim safety).
Squashed for the btcd v2 port. OOR registers every batch parent in the received-VTXO proof lineage with the canonicality manager, and the VTXO gate combines availability across all ancestry parents (worst-state AND) for multi-parent OOR VTXOs.
Squashed for the btcd v2 port. Unroll gates fresh admission on the source VTXO's batch-lineage canonicality (blocks only Invalidated, fail-permissive).
Squashed for the btcd v2 port. Flag-day activation: darepod builds the batchcanon store, backfills from VTXOs at best height, registers + reconciles the manager, and threads the store into vtxo/unroll configs and fn.Some(ref) into round/oor. Includes the F-series reorg systests (F2/F3/F4/F6, ReorgExcludingMempool harness helper) + reorg-safety depth config and the consumed-input pkScript fix.
RestoreForfeitedVTXO spawns the live actor before persisting the VTXOStatusLive flip, so a failed status write is re-driven on the next restore attempt. But the retry short-circuited: a resident actor made the function return early without ever completing the missed status write, leaving the coin marked Forfeited in the DB. On the next restart that VTXO is dropped from live recovery -- a permanent loss of funds. Complete the missed status flip in the resident-actor branch so the DB is reconciled with the already-live actor.
When a conflict-finalized batch's forfeit restore fails partway through, restoreProvisionalConsumers deliberately keeps the reverse-dependency edges for a retry. But conflict_finalized is terminal: its watches are not re-armed on Reconcile and the spend-done event that first triggered the restore never fires again, so the retained edges were never re-driven and the consumed VTXOs stayed forfeited forever. Sweep conflict-finalized batches on Reconcile and re-drive their restores. The restore is idempotent (no-ops when no edges remain), so this safely completes any interrupted restore across a restart.
pendingCommitmentConfs caches a provisional commitment confirmation so the matching finality event can replay it. Entries were only cleared on successful finality; a round that was cancelled, reaped after failure, or completed via onRoundComplete left its cached conf behind, so the map grew without bound over the daemon's lifetime. Drop the cached conf at every round-removal site (cancel, reap, and complete), keyed on the commitment txid.
Add conflicting / conflict_final columns (0/1 INTEGER, default 0) to batch_consumed_inputs, extend ListBatchConsumedInputs to return them, and add a targeted RecordBatchInputConflict update. Regenerated via make sqlc. These persist the observed conflict status of each consumed batch input so restart reconciliation can rebuild the per-input conflict view rather than re-deriving it only from the batch tx confirmation. The columns default to 0 (no conflict), so the first-see insert path is unchanged; only the manager's conflict transitions write them. Folded into the existing 000014 migration (pre-production: DBs are nuked and recreated, so no forward ALTER migration is needed).
On restart, reconcileOne seeded every consumed input's watch as non-conflicting and seeded the batch's confirmation view from the persisted state. For a persisted conflict_provisional batch this meant that if the confirmation was re-observed before the conflicting spend was re-observed, deriveState recomputed (confirmed, no conflict) -> provisional and persisted it, transiently admitting a coin whose input is double-spent. persisted was only a write-dedup guard, not a floor. Persist each input's conflict flags (RecordInputConflict, written before the batch state so a crash leaves the flags ahead of, never behind, the state) and seed the per-input conflict view from them on reconcile, so a bare re-confirmation can no longer clear a conflict it did not resolve. Also harden reconcile watch-arm failure: arm before recording the watch (mirroring the initial registration path) and return on failure, leaving m.watches untouched so a later Reconcile retries the full arm rather than treating the batch as permanently armed. A closing deriveAndPersist reconciles any state/flag drift toward the most-restrictive state.
d63bd67 to
3412852
Compare
e2a235a to
d6dec4e
Compare
292394e to
233abdd
Compare
|
@ellemouton, remember to re-request review from reviewers when ready |
2 participants
Part 2 of 3 of the condensed reorg-safety client stack (epic
lightninglabs/darepo#454). This is the interpret + gate layer.
reorg-observe.Turns raw reorg observation into a durable per-batch canonicality verdict and
gates VTXO admission on it:
batchcanondata model (reorg-reversibleState, effective-expiryrecompute, provisional-consumer edges) + the
BatchCanonicalityManageractor (the sole client-side interpreter).
admission gate, wired across every VTXO source: round-born, OOR-received
(multi-parent lineage), and unroll source-lineage — then activated live in
darepod.
Commits (unchanged, re-grouped): C2 data model → C3/C4 manager → C5 vtxo
availability + gate → C6 round → C7 OOR multi-parent → C8 unroll lineage →
C9 darepod activation (7 commits).
Supersedes #794, #795, #796, #818, #819, #820, #822 (condensed here).
Stack: