multi: batch canonicality data model (C2) - #794
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the batchcanon package and its database persistence layer to implement a client-side, reorg-aware batch canonicality data model. It adds the Record data structure, canonicality state enums, and a sqlc-backed BatchCanonicalityPersistenceStore with a new database migration. Feedback on the changes highlights opportunities to improve robustness and performance, specifically by adding defensive nil checks in UpsertBatch and EffectiveExpiry, and resolving N+1 query issues in ListBatchesByState and query-in-loop overhead during backfill in BackfillFromVTXOs.
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 (s *BatchCanonicalityPersistenceStore) UpsertBatch(ctx context.Context, | ||
| record *batchcanon.Record) error { | ||
|
|
||
| now := s.clock.Now().Unix() | ||
| txid := record.BatchTxID |
There was a problem hiding this comment.
Defensive Programming: Missing Nil Check on Pointer Parameter\n\nThe record parameter is a pointer (*batchcanon.Record) and is dereferenced directly without a nil check:\ngo\ntxid := record.BatchTxID\n\nIf record is nil, this will cause a runtime panic (nil pointer dereference). To ensure robust and defensive programming, a nil check should be added at the beginning of the function.
func (s *BatchCanonicalityPersistenceStore) UpsertBatch(ctx context.Context,\n\trecord *batchcanon.Record) error {\n\n\tif record == nil {\n\t return errors.New(\"record cannot be nil\")\n\t}\n\n\tnow := s.clock.Now().Unix()\n\ttxid := record.BatchTxID| func (r *Record) EffectiveExpiry() fn.Option[int32] { | ||
| return fn.MapOption( | ||
| func(height int32) int32 { | ||
| return height + r.CSVExpiryDelta | ||
| })(r.ConfirmationHeight) | ||
| } |
There was a problem hiding this comment.
Defensive Programming: Missing Nil Receiver Check\n\nIf EffectiveExpiry is called on a nil *Record pointer (for example, if a caller fails to check an error or retrieves a nil record), it will trigger a runtime panic. Adding a defensive nil check on the receiver prevents this.
func (r *Record) EffectiveExpiry() fn.Option[int32] {\n\tif r == nil {\n\t return fn.None[int32]()\n\t}\n\treturn fn.MapOption(\n\t\tfunc(height int32) int32 {\n\t\t return height + r.CSVExpiryDelta\n\t\t})(r.ConfirmationHeight)\n}| records = make([]*batchcanon.Record, 0, len(rows)) | ||
| for _, row := range rows { | ||
| rec, err := s.hydrateRecord(ctx, q, row) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| records = append(records, rec) | ||
| } |
There was a problem hiding this comment.
N+1 Query Performance Issue\n\nIn ListBatchesByState, the code iterates over all retrieved batch rows and calls hydrateRecord for each one:\ngo\nfor _, row := range rows {\n rec, err := s.hydrateRecord(ctx, q, row)\n ...\n}\n\nEach call to hydrateRecord executes two additional database queries (ListBatchConsumedInputs and ListBatchDependentVTXOs). This results in a classic N+1 query problem, where retrieving $N$ batches requires $2N + 1$ database queries.\n\nWhile the number of provisional or reorged batches is typically small, this method can be called with any state, including StateFinalized. As the wallet history grows, the number of finalized batches will increase indefinitely. Calling ListBatchesByState for finalized batches could execute thousands of queries, leading to severe performance degradation, database lock contention, or timeouts.\n\n#### Suggested Improvements:\n1. Avoid Hydration for State Checks: If the caller only needs to check canonicality states or confirmation heights (e.g., during finality checks), consider adding a lightweight list method that does not hydrate the consumed inputs and dependent VTXOs.\n2. Batch Hydration: Query all consumed inputs and dependent VTXOs for the retrieved batch IDs in bulk (e.g., using a single query with an IN clause or fetching all active edges and mapping them in memory) to reduce the query count to $O(1)$ .
| // Skip batches that already have a record so a re-run | ||
| // never overwrites advanced state. | ||
| _, err := q.GetBatchCanonicality(ctx, txid[:]) | ||
| if err == nil { | ||
| continue | ||
| } | ||
| if !errors.Is(err, sql.ErrNoRows) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Query-in-Loop during Backfill\n\nIn BackfillFromVTXOs, the code queries the database for each unique batch transaction ID to check if a record already exists:\ngo\nfor txid, g := range groups {\n ...\n _, err := q.GetBatchCanonicality(ctx, txid[:])\n ...\n}\n\nThis executes $O(U)$ queries, where $U$ is the number of unique batch transaction IDs in the vtxos table. For an upgrading node with a long history of VTXOs, this loop can significantly slow down the startup/migration process.\n\n#### Suggested Improvement:\nQuery all existing batch transaction IDs from batch_canonicality once at the beginning of the transaction, load them into a map[chainhash.Hash]struct{} in memory, and perform the existence check against this map. This reduces the database round-trips to $O(1)$ .
c86210e to
1ab3019
Compare
06f2b62 to
29f9f8b
Compare
3eb6f61 to
449b6cf
Compare
29f9f8b to
b396fd6
Compare
32871de to
4a1f9b1
Compare
b396fd6 to
c5e00e5
Compare
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+).
c5e00e5 to
e7470f4
Compare
|
@ellemouton, remember to re-request review from reviewers when ready |
Summary
Task C2 of the reorg-safety epic (lightninglabs/darepo#454): the client-side batch canonicality data model. This is the durable substrate the
BatchCanonicalityManager(C3/C4) will interpret — it persists how each batch (commitment) tx is faring against the best chain, the inputs each batch consumes, the VTXOs it anchors, and the reverse dependencies needed to restore a provisionally consumed VTXO.Deliberately data + query/update interfaces only — no admission/rejection, no chain watching, no round/OOR/unroll/server behavior change (those are later tasks). Stacked on #422 (Task 1,
reorg-safe-chainsource).What's here
batchcanonpackage — the domain types:Stateenum (unseen,provisional,finalized,reorged_out,conflict_provisional,conflict_finalized) + reservedPolicyState. Append-only typed-int, reorg-reversible — no state is a terminal verdict at this layer.Recordkeyed by txid (identity is never(txid, block hash); the block hash is an observation attribute only).EffectiveExpiry()derives the absolute expiry fromCSVExpiryDelta + ConfirmationHeight, returningNonewhen unconfirmed — the structural guarantee that expiry is recomputed on every reconfirmation, never frozen.ProvisionalConsumerreverse-dependency edge for the VTXO-restore path.Store— behavior-free durable query/update interface.000020_batch_canonicality(4 tables keyed by txid/outpoint; effective expiry deliberately not stored) + sqlc queries.db.BatchCanonicalityPersistenceStore— implementsbatchcanon.Store;BackfillFromVTXOsseeds initial records from existing VTXOs (idempotent, create-only; recovers the CSV-relative delta asbatch_expiry - created_height).batchcanon/CLAUDE.md: the call sites that currently treatBatchExpiry/Expiredas one-way terminal (primarilyvtxo/transitions.goExpiryStatusExpired → FailedState{Recoverable:false}andvtxo/expiry.go), flagged for the manager (C3/C4) to rewire ontoEffectiveExpiry(). No behavior changed here.Out of scope (intentionally)
BatchCanonicalityManager/ chain watching (C3/C4).Tests
batchcanonunit tests (enum value/string stability, effective-expiry recompute across reorg/reconfirm) +dbstore tests (round-trip incl. edges, not-found sentinel, edge-set replacement, reorg-recompute, free state movement = no terminal verdict, state-filtered listing, input-conflict lookup, provisional-consumer restore lifecycle, and backfill from seeded VTXOs incl. idempotency). sqlite by default; postgres via the CI db harness.🤖 Generated with Claude Code