darepod: activate the batch-canonicality reorg-safety gate (C9) - #822
darepod: activate the batch-canonicality reorg-safety gate (C9)#822ellemouton wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates a new batch-canonicality subsystem (including a durable store and manager actor) to provide reorg-safety gates for VTXO coin-selection, unroll-admission, round, and OOR paths. It also adds a comprehensive system test to verify the end-to-end reorg pipeline. The review feedback highlights a critical concurrency hazard where mgr.Reconcile is called directly on the manager behavior struct after registering it as an actor, which should be resolved by using actor messaging. Additionally, minor improvements are suggested in the new system test to prevent potential panics from out-of-bounds slice access and nil pointer dereferences.
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 err := mgr.Reconcile(ctx); err != nil { | ||
| s.actorSystem.StopAndRemoveActor("batch-canonicality") | ||
|
|
||
| return fmt.Errorf("unable to reconcile batch canonicality: %w", | ||
| err) | ||
| } |
There was a problem hiding this comment.
Data Race / Concurrency Hazard: Calling mgr.Reconcile(ctx) directly on the mgr behavior struct from the main thread after registering it with the actor system (actor.RegisterWithSystem) introduces a data race on the internal m.watches map.
Once RegisterWithSystem is called, the actor's event loop is started on a separate goroutine. Any incoming messages (such as RegisterBatchRequest from other actors or chainsource events) will execute Receive and access/mutate m.watches concurrently with the main thread executing Reconcile.
Suggested Solution
Define a ReconcileRequest message in the batchcanon package (implementing ManagerMsg) and handle it within Manager.Receive. Then, perform the reconciliation by sending an Ask to the registered actor reference:
if _, err := mgrRef.Ask(ctx, &batchcanon.ReconcileRequest{}).Await(ctx).Unpack(); err != nil {
s.actorSystem.StopAndRemoveActor("batch-canonicality")
return fmt.Errorf("unable to reconcile batch canonicality: %w", err)
}This guarantees that reconciliation runs entirely within the actor's single-threaded execution context, eliminating any concurrency issues.
| if err := mgr.Reconcile(ctx); err != nil { | |
| s.actorSystem.StopAndRemoveActor("batch-canonicality") | |
| return fmt.Errorf("unable to reconcile batch canonicality: %w", | |
| err) | |
| } | |
| if _, err := mgrRef.Ask(ctx, &batchcanon.ReconcileRequest{}).Await(ctx).Unpack(); err != nil { | |
| s.actorSystem.StopAndRemoveActor("batch-canonicality") | |
| return fmt.Errorf("unable to reconcile batch canonicality: %w", | |
| err) | |
| } |
| require.Equal( | ||
| t, originalBlocks[0].Hash, reorg.Disconnected[0].Hash, | ||
| "the reorg should have disconnected the confirmation block", | ||
| ) |
There was a problem hiding this comment.
Potential Panic (Index Out of Range): Accessing reorg.Disconnected[0] directly without first asserting that reorg.Disconnected is not empty can cause a panic if the reorg fails or behaves unexpectedly.
Asserting the length of reorg.Disconnected first ensures a clean test failure instead of a panic.
| require.Equal( | |
| t, originalBlocks[0].Hash, reorg.Disconnected[0].Hash, | |
| "the reorg should have disconnected the confirmation block", | |
| ) | |
| require.Len(t, reorg.Disconnected, 1, "expected exactly one disconnected block") | |
| require.Equal( | |
| t, originalBlocks[0].Hash, reorg.Disconnected[0].Hash, | |
| "the reorg should have disconnected the confirmation block", | |
| ) |
| got, ok := resp.(*batchcanon.GetBatchStateResponse) | ||
| if !ok || !got.Found { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Defensive Programming / Potential Nil Dereference: If got.Found is true but got.Record is nil (due to an unexpected issue or mock behavior), calling pred(got.Record) will panic because pred dereferences the record (e.g., rec.State on line 219).
Adding a nil check for got.Record before calling pred prevents potential panics.
| got, ok := resp.(*batchcanon.GetBatchStateResponse) | |
| if !ok || !got.Found { | |
| return false | |
| } | |
| got, ok := resp.(*batchcanon.GetBatchStateResponse) | |
| if !ok || !got.Found || got.Record == nil { | |
| return false | |
| } |
2000ae8 to
0306211
Compare
e2ecc19 to
f5ab5f3
Compare
dafebae to
54c1281
Compare
|
@ellemouton, remember to re-request review from reviewers when ready |
Squashed for the btcd v2 port. Unroll gates fresh admission on the source VTXO's batch-lineage canonicality (blocks only Invalidated, fail-permissive).
f5ab5f3 to
7e56139
Compare
54c1281 to
94a41fe
Compare
7e56139 to
4bf4885
Compare
94a41fe to
585d03a
Compare
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.
585d03a to
371232c
Compare
What
Stacked on #C8 (
c8-unroll-source-lineage-gate). This is the activation flag day for the batch-canonicality reorg-safety gate (epic #454), the first two F-series acceptance tests proving the live gate blocks, and a real-bug fix the F3 test surfaced. Slices C5–C8 landed every gate field defaulting tonil/None(dormant no-op); this PR wires it live indarepodand proves the whole pipeline against real bitcoind reorgs.Commits
systest: Prove batch canonicality survives a real reorg— manager re-anchors a batch across a real reorg.oor: Forward the canonicality manager ref to session actors—OORRegistryConfigfield +childConfigpropagation.darepod: Activate the batch-canonicality reorg-safety gate— build store, backfill, register+reconcile manager, thread store into VTXO+unroll gates and the manager ref into round+OOR.darepod: Document the batch-canonicality activation wiring.harness: Add ReorgExcludingMempool for off-chain reorg windows— mines empty replacement blocks so a tx stays off-chain in a stable ReorgedOut window.systest: Prove the gate blocks a reorged-out VTXO (F2)— VTXO excluded from coin selection while its batch is reorged out, admitted on reconfirm.db: Persist the pkScript of each batch consumed input—input_pk_scriptcolumn + sqlc.multi: Thread consumed-input pkScripts to the spend watch— the bug fix (see below).harness: Spend a chosen outpoint and expose its pkScript.systest: Prove the gate blocks an input-conflicted VTXO (F3)— VTXO excluded onConflictProvisional, admitted when the conflicting spend is reorged away.systest: Prove the gate blocks a reorged-out ancestor (F4)— a VTXO whose ancestor batch (not its direct commitment) reorgs out is excluded, admitted on ancestor reconfirm; proves the multi-parent lineage-depth dimension.darepod: Make the reorg-safety depth operator-configurable—--reorgsafetydepthwith a network-aware default (6; 100 on testnet, whose difficulty-reset rule produces deep reorgs). Bounds the deepest reorg the daemon detects.batchcanon: Restore forfeited VTXOs when their batch is invalidated— F6 manager half: records reverse-dependency edges (ForfeitedVTXOs) and, on the consumer batch reachingConflictFinalized, restores them via aRestoreConsumedVTXOcallback (drops edges onFinalized).multi: Add a VTXO-manager restore path for forfeited VTXOs—RestoreForfeitedVTXORequest+ a handler that re-materializes a forfeited VTXO toLivefrom its descriptor (reuses the unilateral-exit recovery pattern; no FSM-lifecycle change).darepod: Wire forfeit restore to the canonicality manager— the restore callback → VTXO manager.round: Declare forfeited VTXOs to the canonicality manager—VTXOCreatedNotification.ForfeitedVTXOs→RegisterBatchRequest.systest: Prove a forfeited VTXO is restored on invalidation (F6)— round-2 commitment invalidated → round-1 VTXO restored to Live + selectable.Bug found + fixed by F3
Writing F3 surfaced a real production bug:
batchcanon.armSpendWatchregistered the per-input spend watch with no pkScript, so lnd rejected it ("an output script must be provided") and conflict detection (LimboConflict) never armed against a real backend. Round-born batches register consumed inputs, so this silently disabled the double-spend half of the reorg-safety gate in production (confirmation-based reorg tracking was unaffected). Commits 7–9 thread the consumed input's pkScript end to end (manager →RegisterBatchRequest/Record→ DB → reconcile, sourced inroundfrom the commitment PSBT's witness UTXOs); an input with no script is skipped (logged) rather than failing the whole batch registration.Proof (real bitcoind+LND systests, all green)
TestBatchCanonicalityReorgRoundTrip— re-anchors across a real reorg.TestBatchCanonicalityGateBlocksReorgedVTXO(F2) —SelectAndReservefails while reorged out, succeeds on reconfirm.TestBatchCanonicalityGateBlocksConflictedVTXO(F3) —SelectAndReservefails onConflictProvisional, succeeds when the conflict is reorged away.TestBatchCanonicalityGateBlocksReorgedAncestor(F4) — ancestor reorged out blocks the VTXO while its direct commitment stays Provisional; admitted on ancestor reconfirm.TestBatchCanonicalityRestoresForfeitedVTXO(F6) — a round-2 commitment driven to ConflictFinalized restores the round-1 VTXO it forfeited back to Live + selectable.TestSendVTXOEndToEnd— full daemon boots + happy path unaffected with the gate live.go build ./...,go vet,make lint-changed-local(0 issues),make commitmsg-lint, batchcanon/db/round/darepod/oor unit tests green.F-series status
F2 (reorg), F3 (input-conflict), F4 (ancestor-depth), and F6 (forfeit-reversal restore) are all proven by systest. F1 (boarding-input conflict ≡ F3) and F5 (ancestor-input conflict ≡ F3+F4) are covered by code-path equivalence. The reorg-safety gate is live and proven on every failure mode it models.
Note on the F6 restore design
F6 restores a forfeited VTXO when its consumer batch is invalidated. Rather than introduce a new non-terminal "provisionally forfeited" FSM state (which would have made every forfeit provisional and required finalize-wiring for the common refresh/leave path), the forfeit still goes terminal but is reversible: the manager re-materializes the VTXO to
Livefrom its persisted descriptor on invalidation, reusing the unilateral-exit recovery pattern. This is lower-risk and avoids regressing the common path; the trade-off is the VTXO is briefly terminal-then-resurrected rather than staying provisional throughout the reorg window.Part of #454.