[MegaRedHand]

🗒️ Description / Motivation

Store::latest_messages (the beacon fork choice's latest vote per validator) is a dense table indexed by validator index. set_latest_message grew it with resize, which at least doubles the capacity whenever an index lands past it. A registry that keeps gaining validators crosses that edge soon after the table is first sized. So on our mainnet follower the table had room for about twice the registry, and roughly half of its 256 MB (heap profile) was never used.

Exact growth has its own trap: an indexed attestation's attesters are sorted ascending. Growing the table one index at a time, each index past the headroom reallocated and copied the whole table, so the first aggregate spanning the registry after a start copied a registry-sized table hundreds of times while holding the beacon lock.

What Changed

• crates/storage/src/store.rs:
  • When a new index is past the capacity, the table grows to that index plus LATEST_MESSAGES_HEADROOM (4096) spare entries with reserve_exact, then resizes as before. The capacity arithmetic is checked, and the growth lives in one helper, reserve_latest_message_slot.
  • New Store::reserve_latest_messages(highest): makes room for every index up to highest in one reallocation at most.
• crates/blockchain/state_transition/src/beacon/fork_choice.rs: update_latest_messages reserves for the batch's highest index before recording it. The highest is taken by max, not by position, so an unsorted caller is still covered.

Correctness / Behavior Guarantees

• Same contents and indexing; only the allocation size changes.
• One attestation reallocates the table once at most, whatever order its indices come in.
• A registry growing by a few validators an epoch still reallocates rarely, once per headroom's worth of new indices.
• Expected saving on the mainnet follower: about 120 MB.

Tests Added / Run

• the_latest_message_table_grows_with_headroom_rather_than_doubling:
  • the first index sizes the table to at most that index plus the headroom;
  • the highest index inside the headroom leaves the capacity unchanged;
  • an index past the headroom grows the table to at most the new index plus the headroom.
• a_reserved_batch_of_ascending_votes_never_moves_the_table: indices spaced wider than the headroom, recorded in ascending order after one reservation, never move the table's buffer or change its capacity.
• cargo test --profile release-fast -p ethlambda-storage --lib: 212 passed, 1 ignored.
• cargo test --profile release-fast -p ethlambda-state-transition --lib fork_choice: 93 passed.

Related Issues / PRs

• One of three memory fixes from the same profile: fix(p2p): disconnect peers whose gossipsub send queue never drains #739 (slow gossipsub peers), fix(types): encode a beacon state without holding it twice #741 (state encoding).

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean. Ran cargo clippy --locked -p ethlambda-storage -p ethlambda-state-transition --all-targets -- -D warnings (clean), not the full workspace target
• Ran make test (test-consensus plus test-node, at release-fast) — all passing. Ran only the tests above

[github-actions]

🤖 Claude Code Review

Review of PR 740: grow the latest-message table exactly rather than by doubling

The change looks correct and I found nothing blocking.

Correctness

• In set_latest_message, reserve_exact(slot + 1 + HEADROOM - len) guarantees capacity >= slot + 1 + HEADROOM. The resize(slot + 1, None) after it therefore never reallocates.
• The capacity() <= slot guard is the right condition. Indices that land inside leftover capacity skip the reserve call, so no reallocation happens.
• The subtraction slot + 1 + HEADROOM - messages.len() can't underflow, because the outer check already gives len <= slot.
• MAX_PLAUSIBLE_VALIDATOR_INDEX (2^26) bounds slot above this code, so the addition can't overflow.
• The lock is still held across the whole operation, and taking messages as &mut beacon.latest_messages avoids repeated field access.

Performance

• The tradeoff is sound: for a roughly 1M-entry registry it saves about half the table's memory, and the cost is more frequent reallocation.
• The cost shows up when indices first arrive in ascending order across a cold start (checkpoint sync or restart). Each time an index passes the headroom, the whole table is copied. With a 4096-entry headroom that is about N/4096 copies, roughly 250 for 1M validators.
• That is acceptable. In practice a high index usually arrives early and sizes the table in one step.
• Nit: if this ever shows up in profiles, make the headroom proportional, for example a few percent of slot, instead of a fixed 1 << 12. I wouldn't do it now.

Tests

• the_latest_message_table_grows_exactly_rather_than_doubling covers three cases: first sizing, a higher index inside the headroom (no reallocation), and growth past the headroom (grown < 2 * sized).
• The assertions on capacity() are slightly allocator-dependent: reserve_exact guarantees only >=. The upper-bound checks do pass in practice, since Vec honors reserve_exact exactly. This is fine.

Style

• The comment explains why the code departs from resize's doubling, which fits the surrounding code.
• The constant's doc comment links Store::set_latest_message correctly.

Approve.

---

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

[github-actions]

🤖 Codex Code Review

I found one correctness concern.

• crates/storage/src/store.rs:4290 — reserve_exact does not guarantee the final capacity will be slot + 1 + LATEST_MESSAGES_HEADROOM; allocators are free to over-allocate. The new test at crates/storage/src/store.rs:9148 and crates/storage/src/store.rs:9158 assumes an upper bound on capacity, so it can be allocator/platform dependent and flaky even if the production code is fine.
• crates/storage/src/store.rs:9133 — because the test asserts capacity() <= target + headroom, it is checking a property Rust’s Vec API does not promise. A safer test would assert the behavioral contract only: no reallocation within the headroom window, and growth occurs once crossing it.
• crates/storage/src/store.rs:4291 — arithmetic is currently safe under today’s MAX_PLAUSIBLE_VALIDATOR_INDEX, but the expression is brittle if that bound changes. Using named locals plus saturating_sub/checked math would make this consensus-adjacent storage path easier to audit.

No fork-choice / attestation / justification / SSZ logic changes are in this PR; the production change is a localized memory-usage optimization and otherwise looks reasonable.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which optimizes memory growth for the latest_messages vector in the Beacon Chain store.

Overall Assessment

This is a well-scoped optimization PR that addresses a real memory inefficiency. The logic is mostly sound, but I have several concerns ranging from edge cases to code clarity.

---

Detailed Findings

1. Potential Panic on Integer Overflow (Lines 4288-4290)

if messages.capacity() <= slot {
    messages.reserve_exact(slot + 1 + LATEST_MESSAGES_HEADROOM - messages.len());
}

Bug: slot + 1 + LATEST_MESSAGES_HEADROOM can overflow usize on 32-bit systems. slot is derived from a u64 validator index (capped at 1 << 26 ≈ 67M), and LATEST_MESSAGES_HEADROOM is 1 << 12 = 4096. While this won't overflow on 64-bit, Rust's usize is 32-bit on some targets. The expect at line 4287 already assumes usize >= u64::MAX behavior isn't hit, but this addition is unchecked.

Fix: Use checked_add or saturating_add:

let new_cap = slot
    .checked_add(1 + LATEST_MESSAGES_HEADROOM)
    .expect("validator index + headroom fits in usize");
messages.reserve_exact(new_cap.saturating_sub(messages.len()));

---

2. reserve_exact Followed by resize is Redundant (Lines 4289-4291)

messages.reserve_exact(slot + 1 + LATEST_MESSAGES_HEADROOM - messages.len());
messages.resize(slot + 1, None);

Issue: reserve_exact allocates capacity, then resize both grows capacity and initializes elements. But resize may reallocate if the capacity isn't sufficient. The intent is to separate allocation from initialization, but resize will do its own capacity check.

Actually, re-reading: reserve_exact ensures capacity() >= len() + additional, so resize won't reallocate. However, this is subtle. A comment explaining why reserve_exact + resize is used instead of just resize would help.

Suggestion: Add comment: // resize won't reallocate due to reserve_exact above.

---

3. Test Name Mismatch with Behavior (Line 9132)

fn the_latest_message_table_grows_exactly_rather_than_doubling()

Issue: The function name claims "grows exactly," but the implementation uses reserve_exact which allocates at least the requested capacity—the allocator may round up. The test assertion allows for this:

assert!(sized <= usize::try_from(first).unwrap() + 1 + LATEST_MESSAGES_HEADROOM);

The name oversells the guarantee. Consider grows_with_headroom_rather_than_doubling.

---

4. Test Fragility: 2 * sized Assertion (Line 9159)

assert!(grown < 2 * sized);

Issue: This relies on sized being large enough that 2 * sized exceeds the headroom-based allocation. If first were small (e.g., 10), and headroom is 4096, then sized ≈ 4107, and grown for higher = 10 + 4096 + 1000 = 5106 would be ~5107, which is not < 2 * 4107 = 8214. The test passes with first = 100_000 by coincidence of scale.

Actually, let me recalculate: sized ≈ 100_000 + 1 + 4096 = 104_097. higher = 100_000 + 4096 + 1000 = 105_096. grown ≈ 105_096 + 1 + 4096 = 109_193. 2 * sized = 208_194. Yes, 109_193 < 208_194. The test works but is structurally fragile.

Suggestion: Make the anti-doubling assertion more robust by computing the expected "doubled" value explicitly:

let doubled = sized.checked_mul(2).unwrap_or(usize::MAX);
assert!(grown < doubled, "grown={grown} should be less than doubled={doubled}");

Or better, assert the exact expected growth formula:

let expected_grown = usize::try_from(higher).unwrap() + 1 + LATEST_MESSAGES_HEADROOM;
assert_eq!(grown, expected_grown, "allocator may round up, so use <= check");

---

5. Missing Edge Case: Validator Index Near MAX_PLAUSIBLE_VALIDATOR_INDEX (Line 683)

The constant MAX_PLAUSIBLE_VALIDATOR_INDEX is 1 << 26, but set_latest_message doesn't enforce this. The old code and new code both allow slot up to usize::MAX (via the expect). If someone calls with index 1 << 26 - 1, the allocation is ~67M + 4096 entries. At Option<LatestMessage> size (likely 40+ bytes), that's ~2.7GB.

Question: Is this a DoS vector? Gossip validation should filter these, but set_latest_message is called from multiple paths. Not a new issue in this PR, but the headroom makes it slightly worse (4096 extra entries ≈ 160KB, negligible).

---

6. Comment Grammar (Line 4283)

// Exact growth plus headroom rather than `resize`'s amortized
// doubling: attestations name validators in no particular order,

"amortized doubling" is misleading—Vec::resize doesn't double, Vec growth strategy on push does. resize to a larger size will reallocate to exactly the requested size if current capacity is insufficient.

Wait, let me re-check: Vec::resize calls extend_with, which calls self.buf.reserve(len - self.len), and RawVec::reserve uses amortized_double_size. So yes, resize can trigger amortized doubling!

But actually, the comment says "resize's amortized doubling" which is correct. However, the old code was resize(slot + 1, None) which would indeed potentially double. The new code avoids this.

Minor: "attestations name validators" → "attestations reference validators" or "name" is jargon. Acceptable.

---

7. Lock Contention Pattern Unchanged (Line 4282)

let mut beacon = self.beacon.lock().unwrap();

Not new in this PR, but holding a mutex across allocation-heavy operations is a performance concern. The latest_messages resize could be large. Consider if this can be optimized separately.

---

Correctness Verifications (Good)

Aspect	Status
latest_message read path unchanged	✓ No diff
None initialization preserves "no vote" semantics	✓
slot bounds check (messages.len() <= slot) preserved	✓
MAX_PLAUSIBLE_VALIDATOR_INDEX still enforced elsewhere	✓ (unchanged)

---

Recommendations Summary

Priority	Item	Location
Medium	Fix potential usize overflow in capacity calculation	store.rs:4289
Low	Clarify reserve_exact + resize interaction with comment	store.rs:4289-4291
Low	Rename test for accuracy	store.rs:9132
Low	Strengthen test assertions against fragility	store.rs:9154-9159
Trivial	Fix comment grammar ("name" → "reference")	store.rs:4285

The core optimization is sound and the test coverage is appreciated. Address the overflow check (Item 1) before merge.

---

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

[pablodeymo]

@MegaRedHand could you address the Kimi review before this goes in? The items
on the current diff:

1. store.rs:4289 — make the capacity arithmetic explicit and checked, e.g.
   a named target = slot.checked_add(1 + LATEST_MESSAGES_HEADROOM) with an
   expect, and reserve_exact(target - messages.len()). The debug assert
   bounds index, but this path is consensus-adjacent and should read as
   safe on its own.
2. store.rs:4289-4291 — add the one-line comment that resize cannot
   reallocate after the reserve_exact.
3. store.rs:9132 — rename the test; reserve_exact guarantees at least,
   not exactly, so something like
   the_latest_message_table_grows_with_headroom_rather_than_doubling.
4. store.rs:9154-9159 — assert the behavioural contract rather than
   grown < 2 * sized: no reallocation inside the headroom, and growth past
   it lands at the new index plus headroom (use <= for the upper bound).
   Codex flagged the same allocator-dependence.
5. store.rs:4285 — "attestations name validators" → "reference validators".

Items 5 and 7 in Kimi's list (index bound, mutex held across the allocation)
are pre-existing and can stay out of this PR.

[MegaRedHand]

@pablodeymo all five are in, one commit each:

1. 451b9ca04: a named target = slot.checked_add(1 + LATEST_MESSAGES_HEADROOM) with an expect, then reserve_exact(target - messages.len()).
2. 3a49d1073: the comment that resize cannot reallocate after the reserve.
3. 275ef362d: renamed to the_latest_message_table_grows_with_headroom_rather_than_doubling.
4. 276cf37be: the test asserts the contract now. The highest index inside the headroom leaves the capacity unchanged, and growth past it lands at most at the new index plus the headroom.
5. 56faefb74: "reference validators".

Kimi's items 5 and 7 stay out, as you said.

One more from a second review pass, be3748538. An indexed attestation's indices are sorted ascending, so with exact growth each index past the headroom reallocated and copied the whole table. The first aggregate spanning the registry after a start copied it hundreds of times under the beacon lock. The bot review's "a high index usually arrives early" doesn't hold inside one aggregate. update_latest_messages now calls a new Store::reserve_latest_messages for the batch's highest index first, so one attestation reallocates once at most. a_reserved_batch_of_ascending_votes_never_moves_the_table covers it.