Repository navigation
perf(storage): share one rebuild between concurrent get_state misses on a root - #788
MegaRedHand wants to merge 7 commits into
Conversation
…on a root
A beacon state rebuild reads a 100+ MB snapshot, folds a delta into a
second full-size buffer and decodes a fresh registry: about a second of
CPU and several hundred MB at peak. Concurrent misses on one root each
paid that in full. A late block's custody columns are chain-checked one
sidecar per blocking task, so a parent state that had left the 32-entry
cache was rebuilt eight times at once. On the Hoodi follower that showed
up as +1 to +4 GiB resident-memory spikes.
read_state now routes a miss through StateRebuilds, a per-root registry
shared by the Store and the writer thread. The first miss leads: it
re-checks the cache and the write buffer, rebuilds, caches the result,
then deregisters the root and wakes the waiters, who take the same Arc.
A leader that errors or panics hands its waiters no result; they retry,
one of them as the new leader.
New counter lean_state_rebuilds_total{role="led"|"joined"} tells real
storage reads apart from shared ones.
🤖 Codex Code ReviewNo actionable findings. The per-root rebuild coordination, cache recheck, and unwind cleanup look sound. Consensus rules and SSZ decoding behavior are unchanged. Tests could not run because rustup attempted to write to the read-only Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt |
🤖 Claude Code ReviewReview: PR 788, share concurrent state rebuilds per rootThis is a solid single-flight implementation. What it gets right:
A few points worth considering: 1. Sharing
|
🤖 Gemini Code ReviewThis PR looks great. The singleflight coordination in
LGTM! Automated review by Gemini · gemini-4-argon · custom prompt |
🤖 Grok Code ReviewI'll read the rebuild coalescing path and its call sites to check races, lock ordering, and whether failures can strand waiters. Automated review by Grok (xAI) · grok-4.7 · custom prompt |
🤖 DeepSeek Code ReviewAutomated review by DeepSeek |
🤖 Kimi Code ReviewNo blocking correctness, security, or consensus issues found. The single-flight implementation handles the important races correctly: waiters cannot miss the wakeup, results are cached before deregistration, late arrivals re-check the cache/pending states, and One minor observability issue:
Automated review by Kimi (Moonshot AI) · kimi-k3.1-pro · custom prompt |
OnceLock::get_or_init already blocks concurrent callers until one initializer finishes and hands them all its value, and a panicking initializer leaves the cell for a blocked caller to fill. That replaces the Condvar, the outcome enum, the drop guard and the retry loop. The rebuild path has no Err return today (every failure is Ok(None) or an .expect() panic), so the cell stores Err(()) for an error instead of sharing a non-Clone Error, and a caller that finds it reads the state itself.
A state rebuild that could not read or decode what it found used to panic through an `.expect()`. It now returns `Error::Storage` for a failed read and `Error::CorruptState` for a snapshot, delta, lean diff or header that does not decode, so a caller can give up on one request instead of unwinding. `StateRebuilds` hands that error to every caller waiting on the rebuild, as `Error::StateRebuild`, so a failing root is read once rather than once per caller. Neither a state nor an error outlives the rebuild: the next miss reads storage again. The fulu column check used to treat an `Err` like a missing parent and park the sidecar. It now logs the error and drops the sidecar as `Ignore(Internal)`, since parking would wait on an import that has already happened.
…rough read_state `role="led"` was counted before the rebuild ran, so a lookup for a root with no state (one header read) and a failed read both counted as a 100+ MB rebuild. A parked column's parent lookups then produced the same "1 led + N joined" shape as a real late-block rebuild. Now `led` counts only a state read back from storage, and `joined` only a waiter that took one. The lean header read at the end of `reconstruct_state` mapped a decode failure to `CorruptState`, but the walk's slot lookup decodes that same row first and panics, so the mapping could never run. It is an `expect` again, and the docs no longer list a lean header among the rows that return an error. The docs also said only waiting callers get `Error::StateRebuild`; every caller does. New tests: eight concurrent `read_state` misses behind a gated backend open one read view; the leader's cache and write-buffer re-check reads nothing; and a caller leaving an old rebuild does not remove a newer rebuild's entry. Each fails when the code it covers is removed.
…on a root (#788) A late block whose parent state had left the 32-entry state cache had each of its custody columns checked on its own blocking task, and every one of them rebuilt that parent state from storage in parallel: a 100+ MB snapshot read, a delta fold and a registry decode each, about a second of CPU and several hundred MB at peak. On Hoodi that showed up as +1 to +4 GiB RSS spikes lasting under 30 s. - `StateRebuilds` keeps one `OnceLock` per root under way. The first miss runs the rebuild; every other miss on that root waits in `get_or_init` and takes the same `Arc`, or the same error. The first caller out removes the entry (guarded by `Arc::ptr_eq`, so a late caller cannot remove a newer rebuild's), so the map never pins a state. A panicking rebuild leaves the cell empty and one waiter rebuilds for the rest. - The leader re-checks the cache and the write buffer before reading, so a miss that arrives just after a rebuild settled does not read it again. - A rebuild that cannot read or decode what it finds returns `Error::Storage` or the new `Error::CorruptState` instead of panicking, wrapped in `Error::StateRebuild` for every caller. A diff frame shorter than its header and a `BlockHeaders` row that does not decode still panic. - The fulu column chain check parks a sidecar whose parent has no state, as before, and now logs and drops one whose parent state read fails (`Ignore(Internal)`). - New `lean_state_rebuilds_total{role="led"|"joined"}`: states actually read back from storage, and misses that took one from a rebuild under way.
🗒️ Description / Motivation
The Hoodi beacon follower on ethlambda-3 (nightly
b1fc143) has short resident-memory spikes, roughly one a day: +1 to +4 GiB that are gone within 30 s, against a ~3.3 GiB baseline (peak RSS since start: 9.09 GiB). All eight spikes between 11-03 and 11-10 show the same pattern in one 30 s scrape window:get_statemissesLate blocks whose columns did not queue had 0–1 misses and no spike.
How one late block turns into eight state rebuilds:
Queue(ParentNotReady).verdict.rshands each queued sidecar tocolumn_checks::check_and_forward(server, vec![*sidecar])on its own. That makes one group per sidecar, and up toCOLUMN_CHECK_PERMITS(16) groups run at once.chain_precheck_withcallsstore.get_state(parent_root). Every call misses, andread_staterebuilds the state independently each time.A beacon rebuild reads a 100+ MB snapshot, folds a delta into a second full-size buffer and decodes a fresh registry. That's about a second of CPU and several hundred MB at peak, paid 8 times at once for the same state.
The fix is in
read_staterather than in the column path, so it covers every caller that can miss on the same root concurrently: p2p's column checks, the chain actor, the RPC and the writer thread.What Changed
crates/storage/src/state_writer.rsStateRebuilds: oneOnceLockper root under way (Mutex<HashMap<H256, Arc<OnceLock<…>>>>).get_or_init. Concurrent misses on that root block inget_or_initand take the sameArc.read_statetakes the registry. Under the lead it re-checks the cache and the write buffer before reading, so a miss that arrives just after a rebuild finished finds that rebuild's state instead of reading it again.read_statemoves intorebuild_state.StateWriterholds the registry too, so the writer thread's lean parent read shares rebuilds with readers.Errinstead of a panic (state_writer.rs,state_codec.rs,beacon_state_delta.rs,store.rs,error.rs).BlockHeadersslot read, snapshot read or diff read isError::Storage.Error::CorruptState { root, reason }. That covers a beacon snapshot, a delta that does not apply to its base (beacon_state_delta::try_decode), and a lean snapshot or diff.rootis the state being read, so a corrupt snapshot is reported against every state read through it.get_statereturns every such error wrapped inError::StateRebuild, for the caller that ran the rebuild as well as the ones that waited on it.unframe, never written by a store), and aBlockHeadersrow that does not decode, on both chains (the slot lookup decodes it before any state row is read).crates/blockchain/state_transition/src/beacon/gossip/column.rs:chain_checksused to park a sidecar on any non-Ok(Some)parent read. NowOk(None)parks it as before, andErrlogs aterrorand drops the sidecar asIgnore(Internal): a failed read or a corrupt row is not a parent still to come, so parking would wait for an import that need never happen.IgnoreReason::Internal's doc now covers a failed store read.crates/storage/src/store.rs: astate_rebuildsfield, wired infrom_partsand passed byget_state.crates/storage/src/metrics.rs: newlean_state_rebuilds_total{role="led"|"joined"}.ledcounts a state actually read back from storage, andjoineda miss that waited on another caller's rebuild and took its state. A lookup that finds no state for its root (one header read) or fails counts as neither, so a column parked on an unimported parent does not look like a rebuild.docs/metrics.md,docs/data_storage.md: the new metric, the shared-rebuild behavior and the read errors.Correctness / Behavior Guarantees
Arc, which is what a cache hit right after the rebuild would have returned.Ok(None)(no state for the root) is shared the same way.Errgoes into the cell behind anArc(Erroris notClone). Every caller returns it asError::StateRebuild, which displays as the error it wraps. So a failing root is read once, not once per caller.OnceLock(not poisoned by a panic) lets a blocked caller run its own rebuild, so no waiter is left blocked.Errwhere they used to see a panic. Unchanged in effect: fork choice and the state writer.expect()the result and still panic; the RPC answers 500. Now quiet instead of a panic:beacon_envelope.rs'envelope_signature_holdsreads anErras an invalid signature. An envelope held for its columns is then dropped with an "invalid signature" warning, and re-fetched if children wait on it.verify_anchor_statesforwards the error with?, andmain.rsreturns any error other thanAnchorStateLost, so a corrupt finalized state now stops startup with that error rather than a panic. It does not fall back to checkpoint sync.beacon_columns.rs(the parked column stays parked), the head-event emitter, epoch precompute, operation validation (StateUnavailable), operation-pool pruning, the RPC'sproposal.rs:165andgloas_proposal.rs, lean reaggregation.get_or_init.lean_state_cache_lookups_totalstill counts each miss, so a late block shows 8get_statemisses with 1ledand 7joinedrebuilds.Tests Added / Run
state_writer.rs:concurrent_get_state_misses_on_one_root_read_storage_once: 8 concurrentread_statemisses on a stored lean state, through a backend that holds each read view until every other caller is waiting. One read view, and every caller gets the sameArc.a_lead_takes_a_state_settled_since_its_miss_without_reading: the leader's re-check finds a state cached, then one in the write buffer, since its miss, and reads nothing.a_caller_leaving_an_old_rebuild_keeps_a_newer_one_registered: a late caller of an earlier rebuild leaving does not remove the current rebuild's entry, so a new miss joins it instead of starting a second.concurrent_shares_of_one_root_run_one_rebuild: the same as the first, onStateRebuilds::sharealone.a_leader_that_panics_hands_the_rebuild_to_a_waitera_leader_error_is_handed_to_its_waiters_and_not_kept: 4 callers, one rebuild, every caller gets the leader's error, and a later miss reads again.store.rs:a_corrupt_beacon_state_reads_back_as_an_error_and_is_not_cachedtruncates a stored snapshot. Both the snapshot and the delta on it come back asCorruptState, and neither is cached.column.rs:chain_checks_drop_a_sidecar_whose_parent_state_does_not_read_back.read_statecalling the rebuild directly instead of throughshare: the first test fails.Arc::ptr_eqguard): the late-caller test fails.Errparked likeOk(None)again, the column test fails withAwaitParent.make fmt,make lint,cargo test --profile release-fast -p ethlambda-storage -p ethlambda-state-transition -p ethlambda-p2p(217, 632 and 362 passed; p2p as of the previous commit, which this one does not touch), and theethlambda-blockchainandethlambda-rpclib tests (292 and 335 passed, same).Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test— storage, state-transition and p2p crates locally; the fulltest-consensus/test-nodehalves left to CI