fix(l1): size the RocksDB block cache from available memory - #7093
Conversation
The default was a flat 12 GiB, which is 71% of a 16 GiB host and leaves no headroom for the trie-layer backlog, execution, the mempool and allocator slack. Default to 40% of the memory the process may use -- the smaller of physical memory and the cgroup limit, so a container is sized against its own limit rather than the machine it lands on -- clamped to 512 MiB..=12 GiB. The 12 GiB ceiling is unchanged, so hosts with 30 GiB or more keep today's value, and an undetectable limit also falls back to it. --rocksdb.block-cache-size becomes optional and still overrides in either direction; the default resolves at store construction so --help stays machine-independent.
|
🤖 Kimi Code ReviewThe PR correctly implements dynamic sizing for the RocksDB block cache based on available system memory with appropriate safety clamps. No critical issues found. Minor observations:
Code quality notes:
Nit (non-blocking): Overall: LGTM – well-structured change with proper bounds checking and test coverage. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewFindings
No EVM, gas-accounting, consensus, trie, or RLP logic is touched here; the risk is operational correctness around memory-constrained deployments. I couldn’t run the Rust tests in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThe PR replaces the fixed 12 GiB RocksDB block-cache default with a memory-aware default while preserving explicit CLI and environment overrides.
Confidence Score: 4/5The nested-cgroup detection defect should be fixed before merging because it leaves a realistic path to the same oversized-cache OOM behavior this PR intends to prevent. The percentage and override wiring are consistent, but the detector consults the cgroup mount root rather than the current process's cgroup, so service-level or non-namespaced container limits can be ignored. Files Needing Attention: crates/storage/store.rs
|
| Filename | Overview |
|---|---|
| crates/storage/store.rs | Adds memory-aware cache sizing, but fixed cgroup-root paths miss limits applied to nested process cgroups. |
| cmd/ethrex/cli.rs | Makes the cache option optional so runtime detection applies only when no CLI or environment override is supplied. |
| cmd/ethrex/initializers.rs | Correctly resolves the optional cache size before constructing the L1 StoreConfig. |
| cmd/ethrex/l2/initializers.rs | Mirrors the L1 cache-resolution behavior for L2 startup. |
| test/tests/storage/rocksdb_block_cache_tests.rs | Covers percentage and clamp behavior but not process membership in nested cgroups. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
CLI["CLI/env cache override"] --> Resolve{"Explicit value?"}
Resolve -->|Yes| Config["StoreConfig cache size"]
Resolve -->|No| Detect["Detect physical and cgroup memory"]
Detect --> Limit["Select smaller detected limit"]
Limit --> Clamp["40%, clamped to 512 MiB–12 GiB"]
Clamp --> Config
Config --> RocksDB["Open RocksDB shared block cache"]
Prompt To Fix All With AI
### Issue 1
crates/storage/store.rs:158-164
**Nested cgroup limit is ignored**
If ethrex runs in a nested cgroup with a memory limit, these fixed mount-root paths read the root cgroup rather than the process's cgroup. The limit is therefore ignored and startup can select the 12 GiB cache ceiling inside a smaller allocation, causing the node to be OOM-killed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(l1): size the RocksDB block cache fr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn cgroup_memory_limit_bytes() -> Option<usize> { | ||
| [ | ||
| "/sys/fs/cgroup/memory.max", | ||
| "/sys/fs/cgroup/memory/memory.limit_in_bytes", | ||
| ] | ||
| .into_iter() | ||
| .find_map(|path| std::fs::read_to_string(path).ok()?.trim().parse().ok()) |
There was a problem hiding this comment.
If ethrex runs in a nested cgroup with a memory limit, these fixed mount-root paths read the root cgroup rather than the process's cgroup. The limit is therefore ignored and startup can select the 12 GiB cache ceiling inside a smaller allocation, causing the node to be OOM-killed.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/storage/store.rs
Line: 158-164
Comment:
**Nested cgroup limit is ignored**
If ethrex runs in a nested cgroup with a memory limit, these fixed mount-root paths read the root cgroup rather than the process's cgroup. The limit is therefore ignored and startup can select the 12 GiB cache ceiling inside a smaller allocation, causing the node to be OOM-killed.
**Knowledge Base Used:**
- [Storage Layer](https://app.greptile.com/lambdaclass/-/custom-context/knowledge-base/lambdaclass/ethrex/-/docs/storage-layer.md)
- [CLI Entrypoint and Node Startup](https://app.greptile.com/lambdaclass/-/custom-context/knowledge-base/lambdaclass/ethrex/-/docs/cli-entrypoint.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewCode Review: PR 7093 — size the RocksDB block cache from available memorySummarySolid, well-scoped change: replaces the flat 12 GiB Findings1. cgroup detection assumes the process's own limit is visible at the fixed root path ( 2. Redundant default computation on the common path ( 3. Precision-losing division order ( Things done well
No security or consensus-correctness concerns — this is host resource sizing, not on-chain logic, and error paths degrade safely to the previous behavior. Automated review by Claude (Anthropic) · sonnet · custom prompt |
Walk /proc/self/cgroup up to the mount root and take the smallest limit, so a limit set on a systemd slice, a pod's parent or an outer container is not missed by reading the mount root alone.
| /// clamped to [`MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES`] ..= | ||
| /// [`MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES`]. Falls back to the ceiling when the limit | ||
| /// cannot be detected, preserving the previous behavior. | ||
| pub fn default_rocksdb_block_cache_size() -> usize { |
There was a problem hiding this comment.
Nothing logs the detected limit or the size this returns, and that is the one thing the PR'''s own motivation argues for.
The old value was a constant: wrong on small hosts, but knowable from the source. This replaces it with a number derived from /proc/meminfo, cgroup v2 memory.max, or cgroup v1 memory.limit_in_bytes, whichever is smallest — and None from all of them silently restores the old 12 GiB. So the cache size is now environment-dependent and invisible, on a code path that exists because a mis-sized cache was only discovered via an OOM kill.
Concretely, an operator debugging memory today cannot answer: was a cgroup limit detected at all, or did we fall back to physical memory? Did detection fail entirely and hand back 12 GiB? Was the result clamped? The clamp matters because it is silent in both directions — below ~1.3 GiB of detected limit, MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES (512 MiB) wins and the cache is more than 40% of the limit.
One line at startup would close it:
info!(detected_limit_bytes = ?limit, source = %source, cache_bytes = size, "sized RocksDB block cache");
Worth noting the blast radius is small, which is a point in the PR'''s favour and not stated: with a 40% factor clamped to 12 GiB, the default only moves below ~30 GiB of detected memory. docs/getting-started/hardware_requirements.md puts the RAM minimum at 32 GB for every network, so on any spec-compliant host this is a no-op (32 GB -> 12.8 GiB -> clamped to 12 GiB, unchanged). It changes behaviour only for under-spec machines and containers — which is exactly the intent, and worth saying explicitly so reviewers know a spec-compliant deployment sees no change.
2 participants
Motivation
DEFAULT_ROCKSDB_BLOCK_CACHE_SIZE_BYTESwas a flat 12 GiB regardless of the machine. Withcache_index_and_filter_blocksenabled that constant is the effective ceiling on RocksDB's resident memory, so on a 16 GiB host we were telling RocksDB it could claim 71% of the box — leaving nothing for the in-memory trie-layer backlog, block execution, the mempool, peer buffers and allocator slack.This showed up on ethpandaops' syncoor runners for glamsterdam-devnet-7 (16.8 GB, 8 cores), where ethrex is OOM-killed during sync. The dominant term there is a separate unbounded-trie-layer bug, but even with that fixed a 12 GiB cache ceiling on a 16.8 GB host has no headroom: the one genesis-sync run that survived peaked at 13.4 GB, right at the edge.
The problem is worse in containers than the number suggests:
/proc/meminforeports the host's memory, not the container's limit, so a 4 GiB-limited container also saw "12 GiB is fine".Description
Derive the default from the memory the process may actually use instead of hardcoding it:
MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES(12 GiB) is now documented as the ceiling, not the default. Its value and its tuning rationale are unchanged.default_rocksdb_block_cache_size()returnsROCKSDB_BLOCK_CACHE_MEMORY_PERCENT(40%) of the detected limit, clamped toMIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES(512 MiB) ..= 12 GiB.min(physical memory, cgroup memory limit)—MemTotalfrom/proc/meminfoagainstmemory.max(cgroup v2) /memory.limit_in_bytes(v1) — so a container is sized against its own limit rather than the machine it lands on. cgroup v1's "unlimited" sentinel is discarded by themin; v2's literalmaxfails to parse and is skipped./proc) falls back to the 12 GiB ceiling, so no host regresses relative to today.Effect: ≥30 GiB hosts keep 12 GiB (unchanged). A 16 GiB host gets ~6.4 GiB and keeps >half the machine for the node. That is below the ~8 GiB filter-thrash floor the ceiling's docs describe, which is the deliberate tradeoff — reduced throughput on a small host beats being OOM-killed on it.
No new dependencies; the detection is ~25 lines of
std::fs.CLI
--rocksdb.block-cache-sizebecomesOption<usize>and resolves at store construction. Explicitly setting it (orETHREX_ROCKSDB_BLOCK_CACHE_SIZE) still overrides in either direction.It is deliberately not
default_value_t: clap would then print the resolving machine's byte count into--help, and CI diffs--helpagainstdocs/CLI.md, so the check would pass or fail depending on the runner's RAM. The verboselong_helpis dropped in favor of one sentence; the sweep rationale now lives on the constants.Testing
test/tests/storage/rocksdb_block_cache_tests.rscovers the pure clamp viarocksdb_block_cache_size_for(Option<usize>), so it is machine-independent:Also run locally:
cargo clippy --workspace --all-targetsclean,cargo fmt --all --checkclean, storage suite 33 passed, and--helpverified byte-identical todocs/CLI.mdunder CI's owndiff -ubB(modulo the$HOME-dependent datadir default line, which CI generates as/home/runner).Fixes #7092