Repository navigation
fix(types): encode a beacon state without holding it twice - #741
MegaRedHand wants to merge 6 commits into
Conversation
libssz's container encoder writes each fixed-size field past the end of the buffer and then moves it into place, and its to_ssz reserves only the encoding's length. Fulu's state ends on a fixed-size field (proposer_lookahead) and so does gloas's (ptc_window), so encoding either regrows a buffer the size of the whole state to twice that for its last field, copying it. Encoding a 44 MB fulu state ended with an 88 MB buffer. BeaconState now reserves the encoding's length plus its largest fixed-size field (ssz_append_capacity) and appends into that, so to_ssz never regrows; the Beacon API's SSZ state response goes through it. The storage layer's encode_state_value, which runs on every import, appends behind its fork tag into one such buffer instead of copying a separately encoded state, which removed a second state-sized copy from the follower's per-slot memory spike.
🤖 Claude Code ReviewReview of PR 741: encode a beacon state without holding it twice No blocking issues. The change is small and the reasoning holds.
Nits, non-blocking:
I haven't built the PR or run its tests. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Kimi Code ReviewI'll review this PR which optimizes SSZ encoding for
|
| Severity | Issue | Location |
|---|---|---|
| Medium | capacity() equality assertions are too strict, may spuriously fail |
mod.rs:2239,2242, state_codec.rs:83 |
| Low | proposer_lookahead mentioned but not in largest_fixed_field array — verify if this is correct |
mod.rs:comment at 380 |
| Low | Typo in doc comment ("a fulu or gloas state") | mod.rs:370 |
| Low | ssz_append_capacity exposes libssz internals as public API |
mod.rs:388 |
Recommendations
-
Fix capacity assertions (most important):
// Instead of: assert_eq!(buf.capacity(), capacity, ...); // Use: assert!(buf.capacity() >= capacity, "{fork}: ssz_append regrew: got {}, expected {}", buf.capacity(), capacity);
-
Verify
proposer_lookahead: Confirm whether Fulu'sproposer_lookaheadis fixed-size and if so, whether it could exceed the currentlargest_fixed_field. If it's variable-size, update the comment to clarify. -
Consider making
ssz_append_capacitypub(crate)if external callers don't need it. -
Add a compile-time or macro-generated check for
largest_fixed_fieldthat fails when new forks are added with larger fixed fields, rather than relying on manual maintenance.
Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt
🤖 Codex Code ReviewReview
If you want, I can convert this into a concise ready-to-post GitHub review comment. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
libssz's container encoder writes each fixed-size field past the end of the buffer and then moves it into place, and its to_ssz reserves only the encoding's length. Fulu's state ends on a fixed-size field (proposer_lookahead) and so does gloas's (ptc_window), so encoding either regrows a buffer the size of the whole state to twice that for its last field, copying it. Encoding a 44 MB fulu state ended with an 88 MB buffer. BeaconState now reserves the encoding's length plus its largest fixed-size field (ssz_append_capacity) and appends into that, so to_ssz never regrows; the Beacon API's SSZ state response goes through it. The storage layer's encode_state_value, which runs on every import, appends behind its fork tag into one such buffer instead of copying a separately encoded state, which removed a second state-sized copy from the follower's per-slot memory spike. Cherry-picked from fix/types-state-encode-without-regrowth at 699f77a.
|
Looked at this against libssz at Two low-priority notes, neither blocking:
|
The test covered fulu and gloas only, so a later fork with a fixed-size field larger than any ssz_append_capacity counts would have lost the guarantee without failing anything. It now builds a small state of every fork, the hard case since little variable-size data follows each fixed field, and lives with the state-transition crate's test builders because ethlambda-types cannot build a state of every fork.
Each caller had to remember to size the buffer with ssz_append_capacity, and to add its own prefix to it. One that forgot (vec![tag] and then ssz_append, or a reserve of encoded_len only) regrew a buffer the size of the state and held two copies of it, the very spike this change set out to remove. ssz_append now reserves the room past the buffer's current length itself, so a prefix is covered, and to_ssz and the state codec no longer compute it.
Every variant reserved the largest fixed-size field of any beacon fork, 2 MiB on mainnet, including a lean state, whose widest fixed field is a block header. Each lean snapshot and to_ssz of a lean state allocated megabytes it never used, often more than the state itself. A lean state now reserves its own largest fixed field. The beacon forks keep one shared list, since every one of them has randao_mixes and a list per fork would save at most a kilobyte on minimal. The every-fork test now covers a lean genesis too; with no headroom for it, that case regrows.
plan_beacon_state_write cloned the encoded target at every snapshot, one copy for the States row and one for the writer's snapshot memo, so a snapshot write still held a mainnet state twice, plus RocksDB's own copy in the batch. The plan now hands over one buffer and a flag. The writer puts it into the batch from a borrow, through a new StorageWriteBatch::put that takes slices (RocksDB copies the value into its batch either way), and then moves the same buffer into the memo. The floor is now the target plus the batch's copy until commit.
|
@pablodeymo thanks for checking the bound against libssz.
Three more from a second review pass:
Fixing the overhang in libssz itself would also cover blocks and sidecars. I left that for a separate libssz PR, since it needs a rev bump. |
🗒️ Description / Motivation
libssz's container encoder writes each fixed-size field past the end of the buffer and then moves it into place, and its
to_sszreserves only the encoding's length. So when a state's last field is fixed-size, encoding it reallocates a buffer the size of the whole state to twice that, copying it, just for that last field. Fulu's state ends onproposer_lookahead, gloas's onptc_window. A 44 MB fulu state encoded with plainto_sszended in an 88 MB buffer.On a mainnet state (hundreds of MB) that is one extra state-sized copy per encode. Two places pay it:
encode_state_valueencoded the state, then copied it into a second buffer behind its fork tag. At a snapshot, the writer also cloned the result, one copy for theStatesrow and one for its snapshot memo./eth/v2/debug/beacon/states/{state_id}, on every request.What Changed
crates/common/types/src/beacon/containers/mod.rs:BeaconState::ssz_append_capacity(): the encoding's length plus the largest fixed-size field of that variant's state. The beacon forks share one list; a lean state has its own, since its widest fixed field is a block header, not 2 MiB.BeaconState::ssz_append(buf)reserves that much pastbuf's current length, then appends the encoding. A caller with a prefix (the fork tag) needs no sizing of its own.BeaconState::to_ssz()goes throughssz_appendinstead of libssz'sto_ssz. That fixes every caller of the enum, the API response included.crates/storage/src/state_codec.rs:encode_state_valuepushes the fork tag, then appends the state into the same buffer.crates/storage/src/state_writer.rs,api/traits.rs,backend/rocksdb.rs: a snapshot write no longer clones the encoded state. The plan hands over one buffer and amemoize_snapshotflag; the writer puts it into the batch from a borrow, through a newStorageWriteBatch::put(table, &key, &value), then moves the same buffer into the memo.Correctness / Behavior Guarantees
randao_mixeson mainnet, 2 MiB;ptc_windowon minimal). Any fixed-size field can then be written past the end, wherever it sits in the container, so the bound holds for every fork, including the earlier ones whose state also ends on a fixed-size field (phase0'sfinalized_checkpoint, altair'snext_sync_committee).Veccarries that headroom as spare capacity: a few MiB at most, next to a state of hundreds of MB.to_ssz. I found none outside tests.Tests Added / Run
ssz_append_never_regrows_a_buffer_with_its_capacity_to_spare, instate_transition'shelpers/test_state.rs(the one place a state of every fork is built): a small state of every fork, phase0 through gloas, plus a lean genesis with no validators.to_sszand an append after a one-byte prefix both reserve exactly the capacity and never regrow, and the encoding round-trips. Run on both presets. Verified to fail for the lean case without its own headroom.a_beacon_state_value_is_its_tag_then_its_ssz_in_one_buffer: the tag byte, then the state's SSZ, in one buffer of exactly the reserved capacity. The value round-trips throughdecode_state_value.putagainst RocksDB and the in-memory backend.cargo test --profile release-fast -p ethlambda-types --lib: 232 passed.-p ethlambda-storage --lib: 211 passed, 1 ignored.-p ethlambda-state-transition --lib ssz_append_never_regrows, with and withoutpreset-minimal: passed.Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — clean. Rancargo clippy --locked -p ethlambda-types -p ethlambda-storage -p ethlambda-state-transition --all-targets -- -D warnings(clean), not the full workspace targetmake test(test-consensusplustest-node, atrelease-fast) — all passing. Ran only the tests above