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.
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 —
advanceRootnow clears every root's btrees on version advance, so frozen roots retain nothing and the aggregate stays within the configured budgets (pinned byTestRetainedRootsShareCacheBudgets). The multiplier below no longer applies; the metrics-gauge point is moot for empty roots. Kept for reference:Coherent.advanceRootre-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, untilevictRootsdrops them atlatest − KeepViews.(KeepViews + 1) × (CacheSize + CodeCacheSize), i.e. ~6× the--state.cachevalue with the defaultKeepViews = 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.--state.cachehelp text documents the equally-sized code budget but not this multiplier.cache_keys_total/cache_list_totalgauges 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.
retryBusywaits forever and is visible only at DebugEngineBlockDownloader.retryBusy(now in #22535) pollsValidateChain/UpdateForkChoiceevery 50 ms while they reportBusy, with a heartbeat every 5 s at Debug level. A permanently stuck background commit therefore turnsexecDownloadedBatchinto 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 continuousBusy), including the accumulated wait time. Revisit alongside the planned removal of the extra commit semaphore, which changes how longBusycan last.4.
advanceRoot's walk-into-evict-lists is dead codeUpdate: addressed in the current #22532 revision — the
Walkloops are gone;advanceRootclears every root on advance instead. Kept for reference:After the rewrite, a pre-existing root is always empty at advance time:
add/addCodeskip non-latest versions (so reader-created roots never gain entries), and a same-version re-announce early-returns before the walk. The twoWalkloops re-feedingstateEvict/codeEvictinadvanceRootcan therefore only walk empty btrees.