Repository navigation
perf(storage): share one rebuild between concurrent get_state misses on a root - #788
MegaRedHand wants to merge 8 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.
pablodeymo
left a comment
pablodeymo
left a comment
There was a problem hiding this comment.
Nice work on this. The diagnosis in the description is very clear, and the single-flight logic in StateRebuilds looks right to me. Nothing blocking, a few things:
1. A shared Err can now panic a caller that didn't do the read
The description says fork choice and the state writer are unchanged in effect because they still .expect() the result. That holds for a persistent failure, but not a transient one: if a p2p column check leads the rebuild of a root and its read fails once, the chain actor importing that root's child joins the same cell, takes the Err, and panics at fork_choice.rs:4830 (.expect("get")). Before, the actor ran its own read. The same goes for the lean writer's .expect("read parent state").
Would it make sense to share CorruptState (re-reading can't help, and it's the expensive case) but let a waiter retry an Error::Storage itself? Fine to leave as is if a transient read error isn't worth designing for, but then I'd reword the "unchanged in effect" line.
2. Log the failure where it happens
Only chain_checks logs the error. Every other caller reports it as something else ("invalid signature" in envelope_signature_holds, "Head state missing", "parent is not known") or says nothing. I know these are listed in the description, but a corrupt row used to be an unmissable panic and is now easy to miss. One error! in lead_rebuild would cover every caller, and it logs once per rebuild rather than once per waiter:
rebuild_state(backend, chain, cache, root)
.inspect(|state| { /* led counter, as now */ })
.inspect_err(|err| error!(%err, block_root = %ShortRoot(&root.0), "State rebuild failed"))3. Follow-up, not for this PR
has_state returns Result but still panics inside (block_slot's .expect("read view")). So the Err(_) => Ignore(Internal) arm in judgeable_block_gloas can't be reached for a failed read: a gloas column panics where a fulu one is now dropped cleanly.
Nits
lean_state_rebuilds_totalisn't seeded at zero, soincrease()misses the first burst after a restart, on an event that happens about once a day.joinedcan count without a matchingledwhen the leader's re-check hits the cache, which doesn't match howdocs/metrics.mddescribes the pair.lead_rebuildrepeatsread_state's cache-then-pending probe; a small helper would keep the lookup order in one place.
…the rebuild metric Addresses the review on #788. - A waiter handed a failed read (`Error::Storage`) no longer returns it: it goes round `StateRebuilds::share` again, so the waiters retrying together share one read of their own. A caller only returns a failed read it ran itself, so a p2p column check's transient failure cannot reach fork choice's or the state writer's `.expect()`. A corrupt state is still shared, since reading it again cannot help. - `lead_rebuild` logs every failed rebuild at `error`, once per read. `chain_checks` drops its own per-column log to `debug`. - The cell records whether the leader read storage or found the state already resident, and both `led` and `joined` count only a storage read, so a `joined` always has its `led`. - `lean_state_rebuilds_total` is seeded at zero for both roles through a new `ethlambda_storage::metrics::init`, called from `main.rs`. - `resident_state` holds the cache-then-write-buffer probe both `read_state` and `lead_rebuild` make.
|
Thanks for the review. Points 1, 2 and the nits are addressed in cb8ba9e. 1. Shared 2. Logging. 3. Nits
|
🗒️ 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. Both probes go through one helper,resident_state, so the lookup order lives in one place; only the first counts as a cache lookup.error(State rebuild failed, with the root) inlead_rebuild, once per read rather than once per caller handed the error.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. A corrupt state goes to the caller that ran the rebuild and every caller that waited on it; a failed read goes only to the caller that ran it (see below).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, andErrdrops the sidecar asIgnore(Internal), logged atdebugwith the column (the store has already logged the failure aterror): 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 the state it read, so everyjoinedhas itsled. 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; nor does a lead whose re-check finds the state already cached, or anyone who waited on it. Both roles are seeded at zero by the newethlambda_storage::metrics::init(called frommain.rsbeside the other crates'init), soincrease()sees the first burst after a restart.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 that takes it returns it asError::StateRebuild, which displays as the error it wraps.Error::CorruptStateis shared, since reading again cannot help: a corrupt root is read once, not once per caller. The flip side: a late block's 8 custody columns are all dropped, not one.Error::Storageis not, since it can be transient. A waiter handed one goes roundshareagain, so the waiters retrying together share one read of their own rather than each starting one. A caller only ever returns a failed read it ran itself. Each round's leader leaves with its own result, so under a persistent failure the reads add up to one per caller, as before this PR, one at a time per root.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, on a failed read of their own or on a corrupt state, which panicked before too. A read another caller made and failed is retried rather than handed to them, so a p2p column check's transient failure cannot panic the chain actor. The RPC answers 500. Every failed rebuild is logged aterrorby the store, once per read, so the callers below that report it as something else, or not at all, no longer hide it. Now handled 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'sCorruptState, and a later miss reads again.a_failed_read_is_retried_by_its_waiters_rather_than_shared: the leader's read fails withError::Storage. Only the leader returns it; the 3 waiters retry together in one second rebuild, held until the other two have joined it, and all get the state.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.Error::Storageshared like any other error: the retry test fails (one rebuild, four errors).share: the retry test fails (each waiter's second rebuild waits for joiners that never come).make fmt,make lint,cargo test --profile release-fast -p ethlambda-storage -p ethlambda-state-transition(218 and 632 passed), and theethlambda-blockchainandethlambda-rpclib tests (292 and 335 passed). p2p not re-run since the review commit (362 passed before it; it changes nothing in p2p).Related Issues / PRs
.expect(), propagate Result through storage layer #306: this PR takesget_state's rebuild off.expect(); the rest of the storage layer's DB-error panics (has_state's slot lookup among them) stay there.✅ 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