erigontech / erigontech/erigon

kvcache: same-version re-announce merges batches into the latest root (stale entries can survive a failed FCU commit)

Open
#22,527 1 comment 0 reactions 2 assignees Claimed by @lupin012 View on GitHub
RPC
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

Found during review of #21293 (since split — the kvcache piece is #22532); should be fixed in #22532 or before #22269 flips the `--state.cache` default to `128MB`.

### Context (post-#22532 model)

#22532 makes the `Coherent` cache roots version-keyed with no carry-over: each `OnNewBlock` announce builds the canonical root for the announced `PlainStateVersion` from that batch's changes alone. The announce is dispatched **pre-commit** and predicts the post-commit version (`plainStateVersion++` in `Dispatcher.Dispatch`), relying on the single `TemporalMemBatch.flushLocked` bump that follows.

`advanceRoot` keeps an early-return for re-announces of the already-latest version ([cache.go#L219-L225 on the #22532 branch](https://github.com/erigontech/erigon/blob/495ffc62abf4c7fc1d8eb4fa4377cead81b08de5/db/kv/kvcache/cache.go#L219-L225)):

```go
r, rootExists := c.roots[stateVersionID]

// if nothing has progressed just return the existing root
if c.latestStateVersionID == stateVersionID && rootExists {
return r
}
```

This early-return is what keeps a warm root alive across change-free periodic announces (e.g. `Hook.SendNotifications` after a stage-loop run with no progress still sends a header-only batch at the unchanged version). But when a same-version re-announce **does** carry changes, `OnNewBlock` merges the new batch on top of the old entries instead of starting the root fresh.

### Failure scenario

1. An FCU executes to head A; dispatch announces version `V+1` with batch `B_A` (pre-commit).
2. The flush/commit never lands: a bg-commit error is logged and the node keeps serving (fg mode: the FCU errors and the CL retries), or erigon crashes after the announce already reached a remote rpcdaemon over the StateChanges stream. MDBX stays at `V`.
3. The CL retries with a different head B (next slot / reorg). Re-execution announces `V+1` again, now with batch `B_B`.
4. `advanceRoot(V+1)` hits the early-return (`latestStateVersionID == V+1` and the root exists), so the feed loop writes `B_B` on top of `B_A`.
5. The retry's commit lands `V+1`. Readers at committed `V+1` resolve root `V+1`; keys touched only by the abandoned execution (`B_A ∖ B_B`) are served values that never committed.

Unlike #22276 (incomplete producer announcements, which only cost cache warmth under the fresh-roots model), this edge can serve never-committed state — which is why it should be closed before the default flip.

### Exposure

- Requires a positive entry-retention budget: unreachable at today's `0MB` default, live once #22269 raises it to `128MB` (or whenever a user sets `--state.cache`).
- Requires a failed/crashed commit between announce and flush plus a head change across the retry — rare, but the bg-commit path explicitly tolerates commit errors, so it is reachable.
- Affects the standalone rpcdaemon (remote and `--datadir` modes); the embedded daemon uses the overlay `LocalCache` and is unaffected.

The early-return predates #22532 (main has it too), but on main the pre-commit announce is off-by-one anyway (which #22532 fixes), so this residual edge is specific to the post-#22532 model.

### Suggested fix

Keep the early-return only for change-free batches: compute in `OnNewBlock` whether the batch carries any account/storage/code changes and pass it down, e.g.

```go
r := c.advanceRoot(id, hasChanges)

// in advanceRoot:
if c.latestStateVersionID == stateVersionID && rootExists && !hasChanges {
return r
}
```

The existing clear-all path (`root.cache.Clear()` for every root + evict-list `Init()`) already does the right thing for the re-announce case, since all evict-list entries belong to the latest root. Alternative: always clear on a same-version re-announce, at the cost of losing warmth on idle stage-loop iterations.

Regression test shape (mirrors `TestCanonicalRootsStartFresh`): announce `V+1` with a batch containing key K1; announce `V+1` again with a batch containing only K2; assert K1 is no longer served from the root.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.