Skip to content

perf(beacon): keep inactivity scores and the ring buffers on the tree - #650

Merged
MegaRedHand merged 2 commits into
feat/prune-finalized-statesfrom
perf/beacon-more-state-fields-on-tree
Oct 27, 2026
Merged

MegaRedHand merged 2 commits into
feat/prune-finalized-statesfrom
perf/beacon-more-state-fields-on-tree

Conversation

@MegaRedHand

@MegaRedHand MegaRedHand commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

Now that validators and balances are persistent trees, every cached state still holds a private deep copy of the other large fields and rehashes them in full on each state root.

Field(s) Mainnet size per state Flat cost today
inactivity_scores ~18 MiB copied per state, rehashed per root, rewritten for every eligible index each epoch
randao_mixes ~2 MiB same
block_roots, state_roots, slashings ~0.6 MiB same
eth1_data_votes, historical_roots, historical_summaries ~0.25 MiB same

Estimates, not measurements: ~26 MiB of flat fields per state and ~0.85M Merkle nodes rehashed per root, of which this PR moves the large majority of the nodes (participation lists and queues stay flat). The import-replay benchmark was not run for this PR, and no memory or node-count number was measured; the estimates come from the field sizes above.

Changes

Commit What
perf(ssz-tree): flush tree fields through one Buffered list per fork Object-safe Buffered trait (impls for List and Vector); tree_fields! lists each fork's tree fields once and generates both apply_pending_mutations and has_pending_mutations. H256 writes HashTreeRoot by hand and forwards is_basic_type (as H160 does). New ssz-tree properties: multi-leaf u8 lists, u64 and 32-byte-newtype vectors, decode parity, rebase of mostly-zero lists and vectors.
perf(beacon): keep inactivity scores on the tree and skip no-op writes InactivityScores is a tree list (BTreeMap update map). process_inactivity_updates computes the same value with the same checked arithmetic and writes only if new != score. Joins the flush list and BeaconState::rebase_on.
perf(beacon): keep the roots and randao ring buffers on the tree BlockRoots, StateRoots, RandaoMixes, Slashings become tree vectors; Eth1DataVotes, HistoricalRoots, HistoricalSummaries become tree lists (alias swap). All join the flush list and rebase_on. Block production now flushes before it hashes the post-state. Test-only sites that sliced or iterated in place use indexing.

SSZ bytes, roots and the storage delta format are unchanged.

Design decisions

  • H256 change done locally, not in libssz's transparent derive (no dependency bump). The lean chain shares H256, so ethlambda-types has unit tests that an SszList/SszVector of H256 hashes like one of [u8; 32], and that a container holding H256 collections keeps its root. The lean state's pinned genesis root test passes unchanged.
  • BTreeMap update map for inactivity scores. Outside a leak almost no score changes, so writes are sparse. A leak writes nearly every score, which costs about one B-tree insert per validator per epoch (~1M at the benchmark's registry size) until a bulk write path exists. A dense map (VecMap) was the alternative; it allocates 16 B per validator on the first push or write.
  • Skip loop written by hand in process_inactivity_updates: read the score, compute the new one exactly as before (overflow still raises ArithmeticOverflow before the recovery step), write only if different. A write rebuilds its leaf even for an equal value, which would unshare the list from the parent state's.
  • rebase_on pairs scores through the altair accessors, so it works across forks and skips phase0 (no scores).
  • process_slot leaves its two roots writes buffered until the next flush (the next slot's, or process_slots' last step). process_historical_summaries_update hashes the roots vectors with that write pending: correct, and the vectors are small. Tests pin this.
  • Block production flush: produce_block hashed the post-state without apply_pending_mutations, so the computed hashes were discarded; one added call.
  • Buffered and tree_fields! are additive in ssz-tree and containers/mod.rs, to keep a merge with other work in the same files easy.

Not in this PR

Item Why
Participation lists on the tree (previous_/current_epoch_participation) Blocked on a zip pass for get_unslashed_participating_indices (#632): its per-index reads would cost ~2M tree descents per block in the pulled-up tip. Also needs List::filled for an O(1) rotation.
pending_deposits on the tree Depends on the mainnet queue length, which has not been measured.
Sync committees (Arc + cached-root newtype) Small win (~1.5 MiB per state), separate design.
Rebase across an epoch boundary (decoded previous equals the base's current) Positional rebase misses it; one if in rebase_on, left for the participation PR.

Testing

Command Result
cargo test -p ethlambda-ssz-tree --profile release-fast 35 passed (properties), 0 failed
cargo test -p ethlambda-types --profile release-fast --lib 201 passed
cargo test -p ethlambda-storage --profile release-fast --lib 139 passed, 1 ignored
cargo test -p ethlambda-state-transition --profile release-fast --lib 329 passed, 16 ignored
cargo test -p ethlambda-rpc --profile release-fast --lib 110 passed
Beacon spec suite, mainnet preset (full) 5734 passed, 152 ignored; 4 failed
Beacon spec suite, minimal preset (full) 40038 passed, 3692 ignored; 4 failed
cargo clippy (workspace, and state-transition with beacon-spec-tests) with -D warnings, cargo fmt --check, cargo check -p ethlambda clean

The 4 failures in each spec run are the gossip/* trials, which report "matched no fixture cases": the gossip fixture directory is intentionally empty in the local setup. Every ssz_static, sanity, finality, random, epoch_processing and transition trial passes. The lean ssz_spectests and the import-replay benchmark were not run.

@MegaRedHand

Copy link
Copy Markdown
Collaborator Author

Import-replay benchmark

Method. ethlambda benchmark import replay, release build, 128 consecutive mainnet blocks (slots 15279073..=15279200, 4 of them epoch-start blocks) on a 16-core host. This PR's leg and a base leg (beacon-chain-integration) were interleaved, 2 rounds each; the table shows the mean of the two. Both legs carry #651, so stf excludes the state-writer hand-off wait. Block roots were identical to the base in every leg. Base vs base noise floor: 1.004.

base this PR change
peak RSS 3.90 GiB 3.31 GiB −0.59 GiB
stf ordinary block p50 577 ms 530 ms −8%
stf epoch-start block (mean of 4) 1505 ms 1436 ms −5%
get_head p50 200 ms 197 ms
block wall p50 777 ms 728 ms −6%
user CPU (whole run) 214.7 s 212.2 s −1%
paired stf, ratio of totals (round 1 / round 2) 0.922 / 0.915

This matches the estimates: peak RSS was expected at 4.0 → 3.3 GiB and the ordinary-block saving at ~30 ms (47 ms measured). It is the only one of these PRs that lowers memory. RSS at the end of the import was not recorded, only the peak. The peak was consistent across both rounds (3.31 / 3.31 GiB).

All five performance PRs together (#647, #648, #649, #650, #633), measured with a 1 s delay between blocks so the state writer drains (--block-delay, #651):

  • ordinary stf 527 → 163 ms; epoch-start 1440 → 420 ms;
  • block wall p50 729 → 185 ms (3.9x); user CPU −30%.

In that build #633's fused epoch pass reads and writes the tree-backed inactivity scores in order (iter, and iter_cow from #647).

MegaRedHand added a commit that referenced this pull request Oct 2, 2026
…3-64-633-636-638-gloas-live

Conflicted files:
- crates/blockchain/state_transition/src/beacon/stf/epoch/altair.rs
- crates/blockchain/state_transition/src/beacon/stf/mod.rs
- crates/common/ssz-tree/tests/model.rs
- crates/common/types/src/beacon/containers/mod.rs
- crates/common/types/src/beacon/containers/shared.rs

Gloas adaptations:
- tree_fields! gets a gloas line: validators and balances (progressive
  trees) plus block_roots, state_roots, historical_roots, eth1_data_votes,
  randao_mixes, slashings and historical_summaries, which gloas takes from
  the shared aliases. inactivity_scores is left out on purpose: gloas
  declares its own flat libssz ProgressiveList for it, which buffers
  nothing, so there is nothing to flush or rebase.
- ssz-tree: ProgressiveList implements Buffered, so the field list can
  hold the progressive registry beside the bounded fields.
- apply_pending_mutations / has_pending_mutations dispatch over all eight
  beacon forks through buffered()/buffered_mut(); the lean guard on
  has_pending_mutations is kept (the merge had dropped it with the old
  registry match).
- rebase_on: the validators/balances list-kind match stays (so a gloas
  state still rebases onto a gloas base only); the other fields are shared
  types in every fork and rebase unconditionally; inactivity scores
  rebase only for a Tree/Tree pair.
- Inactivity scores are a tree List before gloas and a flat slice-like list
  in gloas, so no &[u64] can serve both. altair_validator_lists now returns
  an InactivityScoresRef view (len, get, in-order iter, ptr_eq,
  has_pending_updates, PartialEq) as its third element, and
  inactivity_scores_mut (a &mut [u64]) becomes inactivity_score_mut(index),
  an element write that dispatches over both kinds.
- stf/epoch/altair.rs update_inactivity_scores: decides over an in-order
  walk of the scores (in step with the summary) and writes only changed
  scores afterwards, which subsumes both #633's "write if different" and
  #650's no-op skip; the error order (first eligible validator without a
  score, then checked-add overflow) is unchanged.
- stf/mod.rs tests: BEACON_FORKS now includes Gloas, the historical
  summaries write covers gloas, and the inactivity score case expects no
  pending write on gloas. process_slot's registry check uses the new
  BeaconState::registry_has_pending_updates, since the per-slot roots
  writes stay buffered by design.
- storage store.rs test and the epoch-processing fixture runner comments
  follow the renamed accessors.

Other: shared.rs imports both List/Vector and the progressive alias; model.rs
keeps both sides' tests (the base side was empty).
MegaRedHand added a commit that referenced this pull request Oct 5, 2026
Base automatically changed from beacon-chain-integration to main October 20, 2026 16:19
@MegaRedHand
MegaRedHand force-pushed the perf/beacon-more-state-fields-on-tree branch from 1fef7d8 to 203ab96 Compare October 20, 2026 17:32
MegaRedHand added a commit that referenced this pull request Oct 21, 2026
…#650)

Squash of perf/beacon-more-state-fields-on-tree at 203ab96 (merge-base 35c074b).

Adapted onto #634 and #647: gloas's progressive inactivity scores and ring
buffers go through the same element accessors (inactivity_score_mut,
InactivityScoresRef), and ProgressiveList is Buffered.
MegaRedHand added a commit that referenced this pull request Oct 22, 2026
…tes (#683)

Squash of feat/prune-finalized-states at 43143ae (merge-base aab9864).

Adapted onto #628, #634 and #650: the key-layout bump is DB_VERSION 7,
since nightly already spends 5 (#634's gloas config keys) and 6 (#628's
unrealized justifications). The evict doc keeps #634's three scratch
caches, and the mid-interval test bumps its balance through #650's
balance_mut.

States are now keyed by their block's slot as read from
latest_block_header, so the fixtures that stored a state whose header
slot is not its block's give it that slot, as this PR does for main's
tests: #628's and #634's fork-choice resume tests, #634's
proposer-preferences eviction test and #650's tree-sharing test. The
slot-peek test covers a gloas block, since #634 made gloas the newest
fork.
@MegaRedHand
MegaRedHand marked this pull request as ready for review October 26, 2026 16:10
@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Looks good overall. The PR is performance-focused and the key correctness-sensitive changes appear sound: flushing buffered tree writes before state-root computation closes a real cache/pathology gap without changing consensus results, and the inactivity-score optimization preserves semantics while avoiding unnecessary tree unsharing.

  • No correctness bug stood out in the consensus paths I checked:

    • crates/blockchain/state_transition/src/beacon/block_production.rs:350 now flushes pending mutations before hash_tree_root(), which matches the existing STF invariant that roots must be computed on fully folded state.
    • crates/blockchain/state_transition/src/beacon/stf/epoch/altair.rs:130 switches from get_mut to get + conditional write; that preserves overflow behavior and avoids spurious writes for unchanged scores.
  • The tree-buffering abstraction is also reasonable:

    • crates/common/ssz-tree/src/lib.rs:77 introduces an object-safe Buffered trait.
    • crates/common/types/src/beacon/containers/mod.rs:399 derives both flush and pending-check coverage from the same tree_fields! list, which reduces maintenance risk versus duplicating field lists.
  • Storage/cache behavior also looks intentionally safer:

    • crates/storage/src/store.rs:2897 flushes before wrapping state in Arc, which is important because shared cached states cannot be mutated later without violating assumptions behind cached Merkle roots.
  • I did not find a security issue in the reviewed diff. The changes do not alter fork choice, attestation validation, justification/finalization rules, XMSS verification, or SSZ wire encodings in a way that appears consensus-breaking.

  • Minor maintainability note:

    • crates/common/types/src/beacon/containers/mod.rs:408 / :414 create arrays of trait objects from macro-expanded field lists. It’s correct, but this machinery is subtle; if future tree-backed fields are added in a new fork, missing the tree_fields! entry would be a silent performance/caching bug. The new tests help a lot here.
  • Performance impact should be positive:

    • avoiding equal-value writes in crates/blockchain/state_transition/src/beacon/stf/epoch/altair.rs:142
    • ensuring flush-before-root on hot paths in crates/blockchain/state_transition/src/beacon/block_production.rs:350
    • sharing decoded child-state trees with resident parent state in storage tests

Briefly: I don’t see blocking issues in this PR. The added tests around pending mutations and subtree sharing make the optimization credible.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review of PR 650: move more beacon state fields onto the persistent tree

I read the diff and did not build it or run the tests. Overall it looks correct and well tested. I found one minor gap and a few small points.

What the PR does

  • It moves block_roots, state_roots, historical_roots, eth1_data_votes, randao_mixes, slashings, inactivity_scores and historical_summaries onto ethlambda_ssz_tree lists and vectors.
  • A tree_fields! macro lists each fork's tree-backed fields once. The flush (apply_pending_mutations) and the pending check (has_pending_mutations) both read from that list, so they can't drift apart.
  • assemble_block now flushes pending writes before computing state_root.
  • update_inactivity_scores only writes a score that actually changed.

Correctness

  • H256::is_basic_type (primitives.rs:790): forwarding it from [u8; 32] also changes how the lean state's H256 lists merkleize, not just the beacon fields. The new tests check this well:

    • SszList/SszVector of H256 against the same of [u8; 32].
    • A container holding both.
    • The lean historical_block_hashes root against a mirror list.
    • A model test that a basic-typed and a composite-typed hash collection give the same root.

    This is the highest-risk change in the PR. I'd still run the leanSpec STF and fork-choice spec tests and make test-beacon in CI, because any root mismatch would break consensus.

  • rebase_on skips historical_summaries (containers/mod.rs:630-680): every other tree field is rebased, but historical_summaries is not. It is in tree_fields! and is a tree-backed field from capella on. A decoded state therefore won't share that field with its resident parent, which costs memory and hash-cache reuse. It is probably rare to matter, since the list grows once per roughly 8192 slots, but it is inconsistent with the doc comment. Either add it, gated on the fork the way altair_validator_lists is, or note that it was left out on purpose. The new store test also doesn't assert historical_summaries sharing.

  • Missing flush in assemble_block (block_production.rs:347): the added apply_pending_mutations() is correct and matches state_transition.

  • process_slots pending writes: the new tests document that state_roots and block_roots writes stay buffered until the next slot or the final flush. That is fine because hash_tree_root stays correct with pending writes, per the existing doc. Any new code path that clones a state after process_slots but before the flush would lose cache sharing, though not correctness.

Minor

  • containers/mod.rs:563-605: the field lists in tree_fields! are hand-maintained per fork. every_tree_field_of_every_fork_is_flushed_and_checked catches omissions only for fields it writes to, so a newly tree-backed field still needs a case added to that test.
  • validator.rs:1198, beacon_state_delta.rs:1325: the iter_mut loops became index loops over 0..len(). Each write is a buffered map insert, which is fine for tests. Don't copy the pattern into production loops, since the per-element cost differs.
  • altair.rs: the skip-unchanged-score optimisation is correct. The IndexOutOfBounds check now uses get, so the error behaviour is preserved.

Verdict: approve once CI is green, in particular the lean spec tests, given the H256 change. Adding historical_summaries to rebase_on would be nice but isn't a blocker.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR which converts more beacon state fields to tree-backed (SSZ tree) structures for copy-on-write efficiency. This is a significant optimization touching consensus-critical data structures.

High-Level Assessment

The PR converts BlockRoots, StateRoots, HistoricalRoots, Eth1DataVotes, RandaoMixes, Slashings, InactivityScores, and HistoricalSummaries from SszVector/SszList to Vector/List from ethlambda_ssz_tree. This enables structural sharing between parent and child states, with buffered writes that are folded into the tree lazily.

Overall direction is correct, but I've identified several correctness and safety issues.


Critical Issues

1. Missing apply_pending_mutations() in process_slots final flush — Consensus Bug Risk

File: crates/blockchain/state_transition/src/beacon/stf/mod.rs (existing code, not changed but behavior-affected)

The PR adds tests asserting that process_slots leaves no pending mutations, but looking at the existing process_slots function (not fully shown in diff), the final step must call apply_pending_mutations(). The new test at line 353-361:

#[test]
fn process_slots_flushes_the_roots_it_wrote_in_its_last_slot() {
    for fork in BEACON_FORKS {
        let mut state = test_state::with_validators_at(fork, 4);
        let target_slot = state.slot() + 1;

        process_slots(&mut state, target_slot, &Config::mainnet()).unwrap();

        assert!(!state.has_pending_mutations(), "{fork:?}");
    }
}

This test passes, implying process_slots already flushes. However, I need to verify that process_slots properly handles the new tree-backed fields (block_roots, state_roots, etc.) in its final slot processing. The existing process_slot_leaves_no_buffered_registry_writes test was updated to acknowledge that state_roots and block_roots have pending updates after process_slot (lines 336-340), which is correct since they're written during slot processing but flushed at epoch boundaries or by apply_pending_mutations().

Verify: Ensure process_slots calls apply_pending_mutations() in its final iteration, not just for validators/balances.


2. rebase_on Incomplete for Cross-Fork Rebasing

File: crates/common/types/src/beacon/containers/mod.rs, lines 630-653

pub fn rebase_on(&mut self, base: &BeaconState) {
    // ...
    self.validators_mut().rebase_on(base.validators());
    self.balances_mut().rebase_on(base.balances());
    self.block_roots_mut().rebase_on(base.block_roots());
    self.state_roots_mut().rebase_on(base.state_roots());
    // ... more fields ...
    if let (Ok((_, _, scores)), Ok((_, _, base_scores))) = (
        self.altair_validator_lists_mut(),
        base.altair_validator_lists(),
    ) {
        scores.rebase_on(base_scores);
    }
}

Problem: The historical_summaries field is missing from rebase_on. Capella, Deneb, Electra, and Fulu states have this field, but it's not rebased. This means:

  1. Memory is not shared for historical_summaries between parent and child states
  2. More critically, if rebase_on is used for state reconstruction or fork choice, historical_summaries will diverge unnecessarily

Fix: Add after slashings rebase:

// For forks that have historical_summaries
if let (Ok(summaries), Ok(base_summaries)) = (
    self.historical_summaries_mut().ok(),
    base.historical_summaries().ok(),
) {
    summaries.rebase_on(base_summaries);
}

Wait — historical_summaries_mut() and historical_summaries() may not exist as methods. Check if they need to be added or if the existing macro-generated accessors handle this.

Actually, looking more carefully: historical_summaries is only in Capella+. The rebase_on method needs to handle this. The current code uses altair_validator_lists_mut() which returns a triplet including scores. For historical_summaries, you'd need similar conditional logic.

Line 653: The conditional for inactivity_scores uses altair_validator_lists_mut which is correct (scores are in Altair+), but historical_summaries needs its own conditional.


3. tree_fields! Macro Count Mismatch Risk

File: crates/common/types/src/beacon/containers/mod.rs, lines 420-468

The tree_fields! macro uses manual counting with tree_fields!(@count ...). This is fragile — if a field is added or removed, the count must match exactly or compilation fails with a confusing error.

The macro is correct as written, but consider using a more robust pattern. However, this is more of a maintainability note than a bug.

Actual issue: The tree_fields! invocations for capella, deneb, electra, fulu include historical_summaries on a separate line:

tree_fields!(
    capella::BeaconState => validators, balances, block_roots, state_roots, historical_roots, eth1_data_votes, randao_mixes, slashings, inactivity_scores,
    historical_summaries
);

The trailing comma placement is correct for the macro, but this is brittle. The tree_fields!(@count) recursive macro will count historical_summaries correctly.


4. H256 is_basic_type Forwarding — Correctness Verification

File: crates/common/types/src/primitives.rs, lines 37-49

impl libssz_merkle::HashTreeRoot for H256 {
    fn hash_tree_root(&self, hasher: &impl libssz_merkle::Sha256Hasher) -> libssz_merkle::Node {
        libssz_merkle::HashTreeRoot::hash_tree_root(&self.0, hasher)
    }

    fn is_basic_type() -> bool {
        <[u8; 32] as libssz_merkle::HashTreeRoot>::is_basic_type()
    }
}

This is correct and critical. The comment explains why: without is_basic_type() -> true, a collection of H256 would be treated as composite, causing the tree-backed list to cache a second copy of every element's root.

The tests at lines 199-293 thoroughly verify this. No issue here — this is well done.


5. inactivity_scores Indexing Change — Correctness

File: crates/blockchain/state_transition/src/beacon/stf/epoch/altair.rs, lines 130-147

// Before:
let score = inactivity_scores
    .get_mut(index)
    .ok_or(Error::IndexOutOfBounds { ... })?;
let updated = next_inactivity_score(*score, flags, leaking, config)?;
if updated != *score {
    *score = updated;
}

// After:
let score = *inactivity_scores
    .get(index)
    .ok_or(Error::IndexOutOfBounds { ... })?;
let updated = next_inactivity_score(score, flags, leaking, config)?;
if updated != score {
    inactivity_scores[index] = updated;
}

This change is correct and intentional. The old code used get_mut which, for the tree-backed List, would trigger a copy-on-write of the entire leaf/node even if the value didn't change. The new code:

  1. Reads via get (immutable borrow, no COW trigger)
  2. Computes updated
  3. Only writes via indexing if updated != score

This preserves structural sharing when scores don't change (which is the common case outside leaks). The test inactivity_updates_that_change_nothing_leave_the_tree_shared verifies this.


6. assemble_block apply_pending_mutations Placement

File: crates/blockchain/state_transition/src/beacon/block_production.rs, lines 347-351

post.apply_pending_mutations();
block.state_root = post.hash_tree_root();

This is correct. The state transition already applies pending mutations before checking state root (as noted in the comment). By applying here, the block producer ensures that hash_tree_root computes on the state's own cached nodes, not a throwaway copy. This is an optimization, not a correctness fix.

However, verify that state_transition itself also calls apply_pending_mutations() before its state root check — if it doesn't, there would be a mismatch between block production and validation. The comment says "as state_transition does", implying it does.


7. SSZ Encoding/Decoding Parity for New Tree Types

File: crates/common/ssz-tree/tests/model.rs, lines 524-744

The new tests extensively verify that List/Vector with BTreeMap update maps correctly:

  • Encode/decode parity with SszList/SszVector
  • Rebase operations preserve sharing
  • Buffered trait object dispatch works

The basic_and_composite_hash_collections_have_one_root test at lines 643-654 is particularly important — it verifies that BasicHash and CompositeHash with the same underlying bytes produce the same SSZ and root, which validates the H256 basic type forwarding.

No issues in test coverage.


8. JSON Test Fixture Updates — Missing historical_summaries in rebase_on Test?

File: crates/common/types/tests/beacon_json.rs

All state constructors updated to use tree_vector and ethlambda_ssz_tree::List. The inactivity_scores fields now use vec![...].try_into().expect(...) instead of SszList::try_from.

One concern: the tree_vector helper at lines 108-116:

pub fn tree_vector<T, const N: usize, U>(value: T) -> ethlambda_ssz_tree::Vector<T, N, U>
where
    T: ethlambda_ssz_tree::Value,
    U: ethlambda_ssz_tree::UpdateMap<T>,
{
    ethlambda_ssz_tree::Vector::try_from(vec![value; N])
        .expect("exactly N elements by construction")
}

This requires T: Value and U: UpdateMap<T>. For H256, Value is implemented. For u64 (slashings), Value is implemented. This is correct.


9. Storage Test — ptr_eq Assertions

File: crates/storage/src/store.rs, lines 6103-6155

The new test a_decoded_electra_state_shares_its_tree_fields_with_the_resident_parent verifies structural sharing after cold storage roundtrip. It checks ptr_eq for validators, scores, randao_mixes, slashings, historical_roots, eth1_data_votes.

Missing assertions: block_roots, state_roots, balances are not checked for ptr_eq. The test modifies balances[0] and block_roots[10], so those correctly should NOT share. But state_roots was not modified in the child, so it should share with parent.

Add:

assert!(decoded.state_roots().ptr_eq(resident.state_roots()));
assert!(decoded.block_roots().ptr_eq(resident.block_roots())); // Wait, block_roots[10] was modified

Actually, block_roots[10] was modified in child, so block_roots should NOT ptr_eq. But state_roots was not modified, so it should.

Line 6147-6154: Add state_roots ptr_eq check.

Also, historical_summaries is not checked — if the rebase_on issue from Point 2 is fixed, this should be added too.


10. eth1_data_votes Append Behavior

File: crates/common/types/src/beacon/containers/shared.rs, lines 51-53

pub type Eth1DataVotes =
    List<Eth1Data, { preset::SLOTS_PER_ETH1_VOTING_PERIOD }, BTreeMap<usize, Eth1Data>>;

SLOTS_PER_ETH1_VOTING_PERIOD is 2048 on mainnet. Eth1Data is a composite type (contains deposit_root: H256, deposit_count: u64, block_hash: H256).

With BTreeMap<usize, Eth1Data>, appending votes buffers the append. However, Eth1Data is not a basic type — it's a container. The List implementation must correctly handle composite element updates.

The tests don't specifically cover Eth1DataVotes append/rebase. Consider adding a test in model.rs or the beacon state tests.


Medium Issues

11. process_slots Test — Incomplete Fork Coverage Verification

File: crates/blockchain/state_transition/src/beacon/stf/mod.rs, lines 355-361

The test process_slots_flushes_the_roots_it_wrote_in_its_last_slot only tests target_slot = state.slot() + 1. For a more thorough test, also test epoch boundaries where more fields are modified. However, this is adequate for the immediate concern.

12. every_tree_field_of_every_fork_is_flushed_and_checked — Potential Panic on Lean State

File: crates/blockchain/state_transition/src/beacon/stf/mod.rs, lines 380-461

The test iterates over BEACON_FORKS which doesn't include Lean. The has_pending_mutations and apply_pending_mutations methods early-return for Lean states. This is correct since Lean is not in BEACON_FORKS.

However, the inactivity_scores case uses altair_validator_lists_mut() which returns Err for Phase0. The test handles this with Err(_) => false. This is correct.

13. push_historical_summary — Missing apply_pending_mutations for Push?

File: crates/blockchain/state_transition/src/beacon/stf/mod.rs, lines 372-383

fn push_historical_summary(state: &mut BeaconState) -> bool {
    let summary = containers::HistoricalSummary::default();
    match state {
        BeaconState::Capella(s) => s.historical_summaries.push(summary).unwrap(),
        // ...
    }
    true
}

List::push buffers the append. The test then checks has_pending_mutations() which should be true. Then apply_pending_mutations() flushes. This is correct.

But: HistoricalSummary contains two H256 fields. With the new H256 basic type forwarding, this should work correctly. Verify that HistoricalSummary derives or implements Value properly for the List.


Minor Issues / Style

14. Comment Typos

File: crates/blockchain/state_transition/src/beacon/stf/mod.rs, line 373

/// Writes one element of the field a case names, or answers `false` if the

Should be: "Writes one element of the field a case names" → "Writes one element of the field a case names, or answers false if the fork does not have it."

Actually reading again: "Writes one element of the field a case names" — this seems garbled. Should be "Writes one element of the field the case names" or "Performs one write to the field named by the case".

15. tree_fields! Macro — buffered and buffered_mut Return Arrays, Not Slices

The macro returns fixed-size arrays. This is fine for the macro's use, but means dispatch_state! must handle the array size correctly. The into_iter() on arrays (Rust 2021) moves elements, which is correct for the for_each usage.


Summary

Priority Issue Location Fix
Critical Missing historical_summaries in rebase_on containers/mod.rs:630-653 Add conditional rebase for historical_summaries
High Storage test missing state_roots ptr_eq check store.rs:6147-6154 Add assert!(decoded.state_roots().ptr_eq(resident.state_roots()))
Medium Comment clarity in test helper stf/mod.rs:373 Fix garbled comment
Low Consider adding historical_summaries ptr_eq to storage test store.rs After fixing rebase_on

The core logic is sound. The H256 basic type forwarding is critical and correctly implemented. The inactivity score optimization preserves sharing as intended. The main gap is historical_summaries not being included in rebase_on, which wastes memory and breaks the sharing contract for that field.


Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

@ilitteri ilitteri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low: historical_summaries is not rebased onto resident states.

BeaconState::rebase_on shares the other newly tree-backed fields but not historical_summaries (crates/common/types/src/beacon/containers/mod.rs:1011-1018), even though that field becomes tree-backed in crates/common/types/src/beacon/containers/shared.rs:120-121.

On Capella or later, a state reconstructed from storage therefore cannot reuse the resident state's unchanged summaries subtrees or cached hashes. This does not change contents or roots, but it loses the intended copy-on-write sharing and adds memory retention and rehashing.

Please conditionally rebase summaries when both states are Capella or later, and extend the storage sharing test with a non-empty summaries list and a ptr_eq assertion.

@pablodeymo
pablodeymo force-pushed the perf/beacon-more-state-fields-on-tree branch from 2495d66 to 6e9a3ac Compare October 27, 2026 19:35
@pablodeymo
pablodeymo changed the base branch from main to feat/prune-finalized-states October 27, 2026 19:35
@MegaRedHand
MegaRedHand added this pull request to stack #711 October 27, 2026 19:59
@MegaRedHand
MegaRedHand merged commit 7f4d243 into feat/prune-finalized-states Oct 27, 2026
12 checks passed
@MegaRedHand
MegaRedHand deleted the perf/beacon-more-state-fields-on-tree branch October 27, 2026 20:23
@MegaRedHand MegaRedHand added the performance Performance improvements or possible performance improvements label Nov 3, 2026
dicethedev pushed a commit to dicethedev/ethlambda that referenced this pull request Nov 4, 2026
…ambdaclass#651)

## Motivation

Beacon post-states are persisted by one background thread fed through a
bounded queue. `StateWriterHandle::send` blocks while the queue is full,
and `Store::insert_state` calls it from inside `fork_choice::on_block`,
which the chain actor times as the `stf` phase. If blocks import faster
than the writer persists them, the importer's wait for the writer is
reported as state-transition time. This PR reports the wait as its own
phase, so `stf` measures state-transition work only.

Measured on the import-replay benchmark (128 mainnet blocks, eth-5) with
this change merged into each leg:

| leg | `stf` p50 (now exclusive) | `writer_wait` p50 | block wall p50 |
|---|---|---|---|
| base (beacon-chain-integration) | 581 ms | ~0 | 777 ms |
| each single performance PR (lambdaclass#647-lambdaclass#650, lambdaclass#633) | 353-618 ms | ~0 (max 4
us) | 551-778 ms |
| all five performance PRs merged | 202 ms | **326 ms** | 559 ms |

With all five merged, blocks import faster than the writer persists
their states, so the queue fills and the importer waits about 326 ms per
block. Without this phase that wait was reported as `stf` (about 530 ms
instead of 202 ms), which hid most of the combined gain and pointed at
the wrong place. The state writer, not the state transition, is now the
replay's bottleneck.

## Changes

| Area | Change |
|---|---|
| `storage/state_writer.rs` | `StateWriterHandle::send` returns the
`(start, end)` instants around its blocking `send` |
| `storage/store.rs` | `insert_state` records the hand-off span (first
start, last end if repeated) in a shared field; new
`Store::take_state_handoff()` takes and clears it |
| `blockchain/import_timing.rs` | `writer_wait_start`/`writer_wait_end`
on `ImportTimings` and `StoreTimings`, copied in `absorb_store`; new
`writer_wait` row after `stf`; header documents the nesting |
| `blockchain/lib.rs` | Beacon arm takes the hand-off instants right
after `on_block` returns (both `Ok` and `Err`); clears any stale span
before the call |
| `blockchain/metrics.rs` | `writer_wait` added to `BLOCK_IMPORT_PHASES`
(the `phase` label list) |
| `docs/metrics.md`, `docs/benchmarking.md` | Phase documented; beacon
`stf` now excludes the wait |

The replay benchmark builds its `phases` map from `BLOCK_IMPORT_PHASES`
and the same histogram, so it picks up `writer_wait` and the exclusive
`stf` with no code change.

One beacon `on_block` makes one `insert_state` call. The store still
merges repeated calls (first start, last end), the module's convention
for repeated sections.

## Semantics change

Beacon `stf` (the report row and the
`lean_block_import_phase_seconds{phase="stf"}` series) now excludes the
writer wait: it is the `on_block` span minus `writer_wait`, saturating
at zero. The rows still sum to the end-to-end time. Lean blocks never
set the new instants, so their `stf` and report are unchanged. A new
`phase="writer_wait"` series appears on beacon only. Dashboards
comparing beacon `stf` across this change will see it drop by the wait.

## Testing

| Command | Result |
|---|---|
| `cargo fmt --all` | clean |
| `cargo clippy --workspace --all-targets --profile release-fast -- -D
warnings` | clean |
| `cargo test -p ethlambda-storage --profile release-fast --lib` | 139
passed, 1 ignored |
| `cargo test -p ethlambda-blockchain --profile release-fast --lib` |
210 passed, 0 ignored |
| `cargo check -p ethlambda --profile release-fast` | clean |

New tests: `stf` minus `writer_wait`, wait absent, wait longer than
`stf` saturating to zero, row order and presence (existing label-list
test also covers the new label), and a storage test that `insert_state`
records a start <= end pair and that taking it clears it. Spec suites
and the replay benchmark were not run.

Clippy ran without `--locked`: `Cargo.lock` on the base (125dce1) is
already stale (`zk_alloc` is listed under the wrong crate), so
`--locked` fails there with or without this change. The lockfile is not
part of this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beacon Ethereum Beacon Chain client performance Performance improvements or possible performance improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants