batchcanon: BatchCanonicalityManager (C3/C4) - #795
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 the Manager actor to interpret chain observations into canonicality states, along with a database migration (version 21) to persist the confirmation_pk_script for rebuilding watches upon restart. Feedback on the implementation highlights three critical issues: a data race in the Reconcile method due to map mutations outside the actor's receive loop, a memory and actor leak caused by not pruning finalized batches or unregistering their watches, and a race condition during restart reconciliation that can transiently downgrade conflict states to provisional.
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.
| func (m *Manager) Reconcile(ctx context.Context) error { | ||
| // Non-final states whose watches must be re-armed. Finalized and | ||
| // conflict_finalized batches need no further watching. | ||
| live := []State{ | ||
| StateUnseen, StateProvisional, StateReorgedOut, | ||
| StateConflictProvisional, | ||
| } | ||
|
|
||
| for _, state := range live { | ||
| records, err := m.cfg.Store.ListBatchesByState(ctx, state) | ||
| if err != nil { | ||
| return fmt.Errorf("list %s batches: %w", state, err) | ||
| } | ||
|
|
||
| for _, record := range records { | ||
| m.reconcileOne(ctx, record) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The Reconcile method is called from outside the actor's receive loop (as shown in the tests and described in the PR summary) but directly accesses and mutates the internal m.watches map. Since Go maps are not thread-safe, this creates a critical data race with the actor's receive loop, which concurrently reads and writes m.watches when processing incoming events. To ensure thread safety and adhere to the actor model, Reconcile should be triggered by sending a message (e.g., a reconcileMsg) to the actor's mailbox so that all state mutations are serialized on the actor's single goroutine.
| func (m *Manager) deriveAndPersist(ctx context.Context, w *batchWatch) { | ||
| next := deriveState(w) | ||
| if next == w.persisted { | ||
| return | ||
| } | ||
|
|
||
| if err := m.cfg.Store.UpdateBatchState(ctx, w.txid, next); err != nil { | ||
| m.logger(ctx).WarnS(ctx, "Failed to persist batch state", err, | ||
| "batch", w.txid, "state", next.String()) | ||
|
|
||
| return | ||
| } | ||
| w.persisted = next | ||
| } |
There was a problem hiding this comment.
The m.watches map is never pruned, which causes a memory leak as finalized or conflict-finalized batches accumulate indefinitely over the lifetime of the daemon. Furthermore, when a batch becomes StateConflictFinalized, the confirmation watch and spend watches are never unregistered. Since each registered watch in chainsource spawns a dedicated sub-actor, this leads to a permanent leak of actors and goroutines. Once a batch reaches a terminal state (StateFinalized or StateConflictFinalized), we should unregister all active watches and delete the batch from m.watches.
func (m *Manager) deriveAndPersist(ctx context.Context, w *batchWatch) {
next := deriveState(w)
if next == w.persisted {
return
}
if err := m.cfg.Store.UpdateBatchState(ctx, w.txid, next); err != nil {
m.logger(ctx).WarnS(ctx, "Failed to persist batch state", err,
"batch", w.txid, "state", next.String())
return
}
w.persisted = next
if next == StateFinalized || next == StateConflictFinalized {
if next == StateConflictFinalized {
_ = m.cfg.ChainSource.Tell(ctx, &chainsource.UnregisterConfRequest{
CallerID: confCallerID(w.txid),
Txid: &w.txid,
PkScript: w.pkScript,
TargetConfs: usabilityConfs,
})
m.releaseSpendWatches(ctx, w)
}
delete(m.watches, w.txid)
}
}| switch record.State { | ||
| case StateProvisional, StateConflictProvisional: | ||
| w.conf = confConfirmed | ||
|
|
||
| case StateReorgedOut: | ||
| w.conf = confReorgedOut | ||
|
|
||
| default: | ||
| w.conf = confUnseen | ||
| } |
There was a problem hiding this comment.
There is a race condition during restart reconciliation for batches in StateConflictProvisional. When reconcileOne is called, it initializes the inputWatch structs with conflicting = false. If the confirmation watch immediately fires a batchConfirmedMsg (which is common on startup), deriveState will compute StateProvisional because the spend watch has not yet re-delivered the spend event. This causes deriveAndPersist to overwrite the database state to StateProvisional, transiently losing the conflict state. To prevent this, consider persisting the conflict/spend status of each input in the database, or avoiding state downgrades from conflict states during the initial reconciliation phase.
06f2b62 to
29f9f8b
Compare
7389b71 to
2afb589
Compare
29f9f8b to
b396fd6
Compare
95c13ec to
007743b
Compare
|
@ellemouton, remember to re-request review from reviewers when ready |
levmi
added
enhancement
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+).
b396fd6 to
c5e00e5
Compare
007743b to
4f0707f
Compare
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).
c5e00e5 to
e7470f4
Compare
4f0707f to
212e4d0
Compare
3 participants
Summary
Task C3/C4 of the reorg-safety epic (lightninglabs/darepo#454): the
BatchCanonicalityManager— the sole client-side interpreter of batch canonicality. It sits above #422's reorg-awarechainsourceand consumes the C2 data model (#794), turning raw chain observation into the durablebatchcanon.State. Stacked on #794 (C2).What it does
ConfirmationEvent/ConfReorgedEvent/ConfDoneEventandSpendEvent/SpendReorgedEvent/SpendDoneEventonto its own mailbox and derivesStateby the priorityconflict_finalized > conflict_provisional > reorged_out > finalized/provisional > unseen.provisional; chainsource Done →finalized(the manager treats Done as the policy-finality signal, no depth math); ConfReorged →reorged_outwith the confirmation cleared, so the derived effective expiry is erased and recomputed on reconfirmation.conflict_finalized; SpendReorged clears it.Reconcilere-arms watches for non-final batches on restart, seeding in-memory state from the persisted record so re-observation never transiently downgrades a persisted state.000021) so restart can re-register the conf watch on light-client backends.Out of scope (intentionally)
Tests (reorg safety, all run + race-clean)
A mock chainsource drives the manager through: confirm→finalize; reorg-out → reconfirm (with effective-expiry recompute); input conflict → conflict_provisional → conflict_finalized; conflict cleared on spend reorg; batch self-spend is not a conflict; conflict dominates reorged_out (state priority); spend-watch release on finalize; idempotent re-register merging dependents; and restart reconcile re-arming watches without downgrading persisted state.
🤖 Generated with Claude Code