erigontech / erigontech/erigon

db/kv/kvcache, engine_block_downloader: follow-ups from the #21293 split (#22532/#22535)

Open
#22,499 1 comment 0 reactions 1 assignee Claimed by @yperbasis View on GitHub
ErigonDB
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

Follow-ups from reviewing #21293 (since split — the Coherent-cache rewrite is #22532, the engine_block_downloader piece #22535). None of them block those PRs. Ordered by importance.

## 1. Coherent cache: memory bound is per-latest-root, not total

**Update: addressed in the current #22532 revision** — `advanceRoot` now clears every root's btrees on version advance, so frozen roots retain nothing and the aggregate stays within the configured budgets (pinned by `TestRetainedRootsShareCacheBudgets`). The multiplier below no longer applies; the metrics-gauge point is moot for empty roots. Kept for reference:

`Coherent.advanceRoot` re-`Init()`s the eviction lists on every version advance, so eviction only ever trims the latest root. Frozen predecessor roots — fully disjoint now that carry-over is gone — retain their batch feed plus all miss-fills accumulated during their tenure as latest, until `evictRoots` drops them at `latest − KeepViews`.

- Worst case is ≈ `(KeepViews + 1) × (CacheSize + CodeCacheSize)`, i.e. ~6× the `--state.cache` value with the default `KeepViews = 5`, reachable when heavy RPC traffic fills a large cache within one block window. Typical overshoot is much smaller (one block window of misses per frozen root), but it is not bounded by the flag's advertised size.
- The `--state.cache` help text documents the equally-sized code budget but not this multiplier.
- The `cache_keys_total` / `cache_list_total` gauges only describe the latest root, so the frozen-root residue is invisible in metrics.

Options: count frozen roots against the eviction budget (e.g. trim the latest root's allowance by the frozen residue, or clear a root's btrees when it stops being latest), or at minimum document the multiplier in the flag help. Relevant before #22269 makes the Coherent cache the default.

## 2. Document the fresh-roots hit-rate profile before #22269

Every version starts cold except the keys in that version's state-change batch; hot-but-unchanged keys miss once per key per block. This is a deliberate correctness-over-warmth trade-off (the state-change producers under-announce — see #22276, which also tracks restoring carry-over), but once #22269 turns the cache on by default, steady-state hit rates will be much lower than the pre-rewrite Coherent cache until #22276 lands. Worth an explicit note in the #22269 description and the flag docs so a "cache stopped working" perception doesn't get bisected to the wrong PR.

The standalone txpool binary shares the package but is unaffected for correctness: the pool applies state-change batches directly, and the kvcache is only its cold-sender fallback.

## 3. `retryBusy` waits forever and is visible only at Debug

`EngineBlockDownloader.retryBusy` (now in #22535) polls `ValidateChain`/`UpdateForkChoice` every 50 ms while they report `Busy`, with a heartbeat every 5 s at Debug level. A permanently stuck background commit therefore turns `execDownloadedBatch` into a silent infinite poll at default log levels — and this helper exists precisely so a stuck commit surfaces. Suggestion: escalate the heartbeat to Warn after a threshold (e.g. one minute of continuous `Busy`), including the accumulated wait time. Revisit alongside the planned removal of the extra commit semaphore, which changes how long `Busy` can last.

## 4. `advanceRoot`'s walk-into-evict-lists is dead code

**Update: addressed in the current #22532 revision** — the `Walk` loops are gone; `advanceRoot` clears every root on advance instead. Kept for reference:

After the rewrite, a pre-existing root is always empty at advance time: `add`/`addCode` skip non-latest versions (so reader-created roots never gain entries), and a same-version re-announce early-returns before the walk. The two `Walk` loops re-feeding `stateEvict`/`codeEvict` in `advanceRoot` can therefore only walk empty btrees.

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.