Repository navigation
Conversation
PutBlocks and PutBlock left the partially indexed block behind when putBlock or commit returned an error: the entries stayed in the shared batch and the total block/action counting indexes kept their bumped in-memory sizes. Height() then reported a height that was never written, retrying the same block failed with "wrong block height" forever, and the next successful commit flushed the stale entries. Clear the batch, drop the dirty address indexes and reload the counting indexes from the DB on failure, so the indexer is back at its last committed state.
The explorer was replaced by the api package long ago and sub-chain block forwarding no longer exists, so these notes point at nothing.
envestcc
left a comment
envestcc
left a comment
There was a problem hiding this comment.
The existing blockindex suite passes, but two fault-injection checks expose new rollback failures: nil index references and overwritten committed counters. Both checks pass against the PR base.
| return errors.Wrapf(cause, "failed to reload total block index: %v", err) | ||
| } | ||
| if x.tac, err = db.NewCountingIndexNX(x.kvStore, _totalActionsBucket); err != nil { | ||
| return errors.Wrapf(cause, "failed to reload total action index: %v", err) |
There was a problem hiding this comment.
[P2] Preserve index references when reloading fails
If storage reads and writes fail during rollback, NewCountingIndexNX returns (nil, error), and this assignment replaces x.tbk with nil before returning. Subsequent Height(), GetBlockIndex(), or indexing calls then panic, even after storage recovers. The same problem applies to x.tac. Load into temporary variables and handle failed recovery without leaving methods able to dereference nil. A fault-injection test reproduces the panic on this patch and passes on the base revision.
| delete(x.dirtyAddr, k) | ||
| } | ||
| var err error | ||
| if x.tbk, err = db.NewCountingIndexNX(x.kvStore, _totalBlocksBucket); err != nil { |
There was a problem hiding this comment.
[P2] Avoid recreating committed counters on reload read errors
If a counter read fails but writes remain available, NewCountingIndexNX treats the nil read result as a missing index and persists a zero counter. Using it here therefore lets rollback erase committed block/action counts: after committing block 1, injecting read failures during a rejected block resets the persisted block count from 2 to 0, making Height() underflow. Use a reload path that propagates read errors without initializing or overwriting existing counters. This fault-injection test passes on the base revision and fails on the patch.
… write Address review on the rollback approach: reloading the counting indexes in revert() could leave nil indexes when the reload failed, and could reset the committed counters when a read failed, since NewCountingIndexNX took any read error as a missing index. Instead of reverting, PutBlocks now builds a local batch and collects the entries of the counting indexes, puts them with AddToBatch, and updates the in-memory sizes of the total block and action indexes only after WriteBatch succeeds. A failure leaves the indexer and the DB unchanged, so revert(), the shared batch and the dirty address indexes are gone. - db: AddToBatch takes multiple values, so it can be used once per batch - db: NewCountingIndexNX returns read errors instead of resetting the index - db: add LoadCountingIndex, which does not write for a missing index, so new address indexes are only created when their batch is committed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Summary
fix(blockindex):
PutBlocks/PutBlockleft a partially indexed block behind whenputBlockorcommitfailed. The entries stayed in the shared batch and the total block/action counting indexes kept their bumped in-memory sizes, so:Height()reported a height that was never written,wrong block heightuntil restart,On failure the indexer now clears the batch, drops the dirty per-address indexes and reloads the counting indexes from the DB, i.e. it returns to its last committed state (the same thing a restart does today). Resolves the
TODO: Revert changesinPutBlocks.chore: drop 9 stale TODOs referring to the explorer removed in Remove JSON RPC related code #1057 #1085 (long superseded by the
apipackage) plus one note about sub-chain block forwarding, which no longer exists. Comment-only; gofmt realigned two struct field groups.Non-consensus: only the block/action index used by API queries is affected.
Test plan
TestIndexerPutBlocksRevertsOnFailure: fails on master (Height()returns 1 after a failedPutBlocks), passes with the fix; also checks the same blocks re-index to the same counts as a clean rungo test ./blockindex/ ./consensus/... ./dispatcher/ ./chainservice/go build ./...,go veton touched packages,gofmt🤖 Generated with Claude Code