Repository navigation
docs: update roadmap with extended steps - #6
Merged
Merged
Conversation
pablodeymo
added a commit
that referenced
this pull request
Mar 13, 2026
…ttestation processing Three defensive fixes in process_attestations / try_finalize: 1. aggregation_bits OOB (finding #6): A malicious attestation could carry aggregation_bits longer than the validator set, causing votes[validator_id] to panic on out-of-bounds access. Now filter bits >= validator_count before indexing. 2. root_to_slot missing key (finding #7): try_finalize used direct HashMap index root_to_slot[root] which panics if a justification root is absent from historical_block_hashes (e.g. carried over from a prior finalization window). Replaced with .get() — missing roots are conservatively retained. 3. justifications[root] (finding #8): Reviewed and confirmed safe — the roots come from justifications.keys() so the key is always present. No change needed.
pablodeymo
added a commit
that referenced
this pull request
Mar 16, 2026
…on data (#228) ## Motivation Security audit findings #6, #7, #8: three sites in `process_attestations` / `try_finalize` use direct indexing on data structures whose keys or indices come from network-controlled attestations. A malicious peer can craft attestations that trigger panics, crashing the node. ## Description ### Fix 1 — Reject attestations with OOB `aggregation_bits` (finding #6) **File:** `crates/blockchain/state_transition/src/lib.rs` **Problem:** `aggregation_bits` is a `BitList<ValidatorRegistryLimit>` (max 4096 bits) deserialized from the network. `votes` is a `Vec<bool>` of length `validator_count` (actual validators in state). If a malicious attestation carries `aggregation_bits` with bits set beyond `validator_count`, direct indexing panics. **Spec and cross-client behavior:** | Implementation | Behavior | |---|---| | **Spec** (state.py:503-505) | Direct list access `justifications[target.root][validator_id]` — crashes on OOB (IndexError) | | **Zeam** (Zig) | Rejects attestation with error | | **Lantern** (C) | Rejects attestation with `return -1` | | **gean** (Go) | `Bitlist.Get()` returns false for OOB (silent skip) | **Fix:** Pre-loop length check that rejects the entire attestation when `aggregation_bits.len() > validator_count`, matching the spec (crash = implicit reject) and the majority of other clients (Zeam, Lantern). Direct indexing `votes[validator_id] = true` is safe after the bounds check. --- ### Fix 2 — Prune justification roots missing from `root_to_slot` (finding #7) **File:** `crates/blockchain/state_transition/src/lib.rs` (in `try_finalize`) **Problem:** `root_to_slot` is built from `historical_block_hashes` for slots after finalization. But `justifications` can contain roots carried over from a previous finalization window that no longer appear in `historical_block_hashes`. Direct HashMap index panics on missing key. **Spec and cross-client behavior:** | Implementation | Behavior | |---|---| | **Spec** (state.py:560-562) | `assert all(root in root_to_slot for root in justifications)` — crashes (invariant violation) | | **Lantern** (C) | Conservative retention | The spec treats missing roots as an invariant violation. A root absent from `root_to_slot` means its slot is at or below `finalized_slot` (already finalized). Conservatively retaining such roots (previous approach with `is_none_or`) would cause them to accumulate forever — they never reappear in `root_to_slot`, and `is_valid_vote` rejects new votes for them. **Fix:** Prune missing roots (return `false` in `retain`), matching the spec's filter intent: `root_to_slot[root] > finalized_slot` would be `false` for such roots. A warning is logged when this happens. --- ### Finding #8 — `justifications[root]` (no change needed) **File:** `crates/blockchain/state_transition/src/lib.rs` (in `serialize_justifications`) ```rust let justification_roots: Vec<H256> = justifications.keys().cloned().collect(); // ... .flat_map(|root| justifications[root].iter()) ``` Reviewed and confirmed safe: `justification_roots` is built from `justifications.keys()`, so every root used as index is guaranteed to be present in the map. No change needed. ## How to Test ```bash make fmt make lint cargo test --workspace --release ``` All 120 tests pass unchanged — valid attestations always have `aggregation_bits.len() <= validator_count` and all justification roots are in `root_to_slot`, so these guards only trigger on malformed/adversarial data not present in test fixtures.
tcoratger
pushed a commit
to tcoratger/ethlambda
that referenced
this pull request
Oct 30, 2026
* docs: design the unified lean/beacon storage layer
The beacon fork choice holds its whole world in memory, including three
unbounded maps of ~350 MB beacon states, so a mainnet follower can
neither survive a restart nor bound its memory. This designs the move
onto the DB-backed store the lean chain already runs on: one set of
methods over one block enum, one runtime config persisted for both
chains, and a byte-domain state delta that makes recovering an arbitrary
beacon state bounded.
The diff layer is the one deliberate exception to sharing. Lean's
field-shaped StateDiff depends on validators never changing, which a
beacon registry breaks every epoch, and it is the better algorithm for
lean, so beacon gets xdelta3 byte deltas beside it rather than instead
of it.
Working artifacts, not reference docs: both are removed once the work
lands, the way the beacon STF design document was.
* build: check debug assertions under the test profile
Tests run under release-fast, which inherits release and therefore had
debug assertions off, so a debug_assert! in any crate ran nowhere. A dev
build is not a fallback: signature verification and aggregation
stack-overflow without release-grade opt-level.
overflow-checks is pinned rather than inherited so arithmetic behaviour
stays identical to release.
Verified both directions: debug_assertions appears in `cargo rustc
--profile release-fast -- --print cfg` and not under release.
* test(types): pin the genesis state root
`State.config` is hashed into every lean state root and is the field
leanSpec declares as `config: GenesisConfig`. Naming it after the spec was
this commit's original job, and main has since done exactly that under the
name `StateConfig`, keeping `ChainConfig` for the node's runtime
configuration: a different type for a different job.
What is left to add is the guard the rename came with. This pins the
genesis state root, so a field reorder, an added field, or a type change
disguised as a rename fails here rather than in cross-client interop.
* feat(types): let the runtime config persist, and give lean one
Config becomes the store-level configuration for both chains, so it needs
an encoding it did not have and a genesis_time it did not carry.
min_genesis_time could not serve as the clock: it is the earliest the
deposit-driven rules permit, 23 seconds before mainnet actually started.
Config::lean fills every beacon field with a placeholder chosen to fail
closed, FAR_FUTURE_EPOCH for every fork epoch, so a beacon-shaped gate
reading a lean config never sees an activated fork where epoch 0 would
have activated all seven.
blob_schedule becomes a bounded SSZ list because SSZ has no unbounded
one; the bound is a storage limit, which is what the database format
version exists to let us change.
* feat(storage): tag a data directory with its chain and format version
A directory is one chain for its whole life and one on-disk format, and
neither is recoverable when wrong: there is no migration, because the
States value layout is about to change and every state already written
would decode as the wrong shape.
Both checks come after the "has this ever held a chain" reads, so a fresh
directory is still Ok(None) rather than a version mismatch.
Also folds four Store struct literals into one constructor, which is how
the next few fields avoid drifting between the bootstrap paths.
* feat(storage): hold the runtime config, not the in-state one
Metadata["config"] now holds the unified runtime Config for both chains
rather than the two-field container that lives inside a lean State. The
in-state one is still what a state root commits to; this is what the node
runs on, and a beacon directory needs the fork schedule in it.
Returned behind an Arc because every beacon fork-choice call takes
&mut Store alongside the config, so a caller has to hold it across a
mutable borrow of the store it came from.
The config row's layout changes without a DB_VERSION bump on purpose:
the version was introduced one commit ago on this same branch and has
never shipped, so there is no released format to be compatible with.
* feat(types): give the block enum a lean variant
BeaconState already carries Lean; blocks now do too, so the storage layer
can take one block type and split inside its methods instead of growing a
method per chain.
The accessors are macro-generated, so the arm lands in the macros: the
existing message:/outer: split turns out to be exactly the lean boundary,
since lean's Block declares all four message fields under the same names
and has no BLS signature to answer the one outer accessor with.
The enum is not renamed. It names 156 sites across 19 files, eight of
them the concrete per-fork structs that share the identifier, and
BeaconState already carries Lean under a beacon-shaped name.
Adding the variant also required PartialEq on SignedBlock, Block,
BlockBody and AggregatedAttestation, since SignedBeaconBlock derives it
and now has to compare a lean payload too; and a lean_block_unreachable
boundary in ethlambda-state-transition's beacon module, mirroring the one
ethlambda-types already has, since the crate's own beacon process_block
dispatcher matched the enum exhaustively.
* docs: withdraw the block-enum rename, and record how the Lean arm lands
Two things the design got wrong about blocks, found while sizing the work.
The rename to SignedBlock is withdrawn. SignedBeaconBlock names 156 sites
across 19 files and eight of those are the concrete per-fork structs that
share the identifier, so a rename is a large edit with a real corruption
mode and no functional gain. BeaconState already carries Lean under a
beacon-shaped name, which is the precedent that makes the rename
unnecessary rather than merely risky.
The plan also assumed the block accessors were hand-written match arms.
They are macro-generated, and there is a fifth accessor the plan never
accounted for: signature, returning a BlsSignature that a lean block has
no equivalent of. The state side already solved this with a pair of
dispatch macros, one panicking on Lean and one running every arm, so
blocks mirror it. The happy part is that the existing message:/outer:
split in the accessor macro turns out to be exactly the lean boundary,
since lean's Block declares all four message fields under the same names
and cannot answer the one outer accessor.
Also corrects the claim that current_slot() reads genesis_time. It
divides the store clock by INTERVALS_PER_SLOT and never reads the config,
so it is lean-shaped as written and the beacon path needs its own slot
derivation from the Unix clock.
* feat(storage): tag States values with the fork that shapes them
One table now has to hold a lean State and seven beacon shapes, and SSZ
carries no type tag, so the reader cannot recover the shape from the
bytes. A leading ForkName::selector byte is what makes one table serve
both without the caller having to already know which chain it opened.
Safe to change the value layout only because the format version and
chain tag landed first: an older directory is rejected outright rather
than misdecoded, and there is deliberately no migration.
The selector is not a variant index, so it always goes back through
from_selector: Lean takes 255 so beacon forks after fulu can keep taking
the next free value.
* feat(storage): the beacon fork-choice table and its scratch
Splits the beacon fork-choice store's fields by whether they are worth
persisting. Only the unrealized justification is: get_voting_source reads
it for every block from a prior epoch, and recomputing an entry means
replaying epoch processing on a copy of that block's post-state, so it
gets the ninth table.
The rest are per-slot or per-epoch scratch and go in memory. Proposer
boost resets every slot, equivocators come back from replaying attester
slashings, latest messages from one epoch of attestations, and PoW blocks
stand in for an execution-client call a restarted node makes again.
The latest-message read takes a closure because the map is behind a
mutex, and it filters equivocators there because get_weight has to
exclude such a vote entirely rather than let it count for either side of
the fork it created.
* feat(storage): one block insert for both chains
insert_signed_block takes the fork-generic enum and splits inside it,
with both arms spelled out rather than delegating to a helper per chain:
the two write different tables, and that difference is what a reader
needs to see. Because a beacon Root is already H256 there is one key type
and one value codec, so nothing here needs a bridging generic.
BlockRoots and BlockProof stay unwritten on the beacon arm: a slot holds
several blocks once a fork exists, so only the parent links say which is
canonical, and a range request walks those from the fork-choice head.
The lean arm keeps its post-commit attestation-vote recording, which the
beacon arm has nothing to do: a beacon block carries no lean
attestations, so the match hands that decision back out rather than
running it unconditionally.
* feat(storage): the beacon checkpoints, head, and an empty beacon directory
Both bootstrap paths write every metadata key they will later read, so an
absent key is a bug reaching the wrong chain's accessor rather than a
condition a caller could recover from. The reader now panics naming the
key and the chain, which is what says which mistake was made, and the
helpers stop returning a Result they could never populate.
Beacon's accessors return values directly: their callers' error type
lives in ethlambda-types and cannot carry a storage error, so a Result
here would buy a map_err at every call site and nothing else.
The fork-choice head is the one row deliberately left unseeded, since
absent means no head has been computed yet, which is a real state.
Metadata["time"] is shared between the chains with different units, which
is safe exactly because a directory holds one chain for its whole life.
from_db_state's existence gate previously relied on KEY_LATEST_FINALIZED,
a lean-only key init_beacon never writes; a fully-formed beacon directory
would otherwise read as empty and let the lean bootstrap write into it.
The gate now consults the chain tag first.
* feat(storage): read a block back as the fork it was written as
insert_signed_block already took the fork-generic enum, but the read side
still returned the lean type, so a beacon block could be written and
never read back. The fork choice migration needs to read blocks, so the
asymmetry had to go.
Dispatches on the cached chain tag rather than trial-decoding: a data
directory holds one chain for its whole life, so the tag is authoritative
and guessing would be wrong as well as slower.
The lean arm keeps the two synthesis behaviours its callers depend on,
the empty proof for the slot-0 anchor and the empty body for a header
whose body_root says there is none.
* feat(storage): persist and read a beacon post-state
The fork choice migration has post-states to keep, so insert_state and
get_state take the fork-generic state type and split inside, the way the
block methods already do.
The beacon arm writes a full snapshot per block on purpose. That is
wasteful at mainnet scale and is a placeholder: the delta layer and the
bounded cache land in later tasks, and at the fixture presets a
standalone snapshot per block is entirely adequate. Beacon deliberately
does not borrow lean's StateDiff, which omits validators on the
assumption they never change; a beacon registry changes every epoch.
The lean arm is unchanged: same snapshot interval, same diff chain, same
reconstruction, same cache.
* feat(beacon): put the fork choice on the DB-backed store
The spec-shaped in-memory Store held three unbounded maps of whole
beacon states, so a mainnet follower could neither survive a restart nor
bound its memory. Its fields move to the storage layer the lean chain
already runs on: blocks to the block tables, checkpoints and the clock to
Metadata, unrealized justifications to their own table, and the per-slot
scratch to an in-memory struct that is cheap to rebuild.
The children scan takes a block index built once per tree walk rather
than a lookup per hop, since get_weight calls get_ancestor once per
active validator.
checkpoint_state is derived on every call for now; a later task restores
a bounded cache. That also removes the file's only outright whole-state
clone, which existed because two maps each needed their own copy of the
anchor.
The fixture suite passes with an unchanged case count at both presets.
* feat(beacon): record the head fork choice computed, and narrow the rest
set_beacon_head existed with tests but had no caller, so a restarted
node had to recompute a whole weighted tree walk before it could answer
what its head was. get_head now writes the head it just computed, every
time rather than only on change: a value written once and left alone is
a second source of truth a bug can let drift.
That also earns get_head's &mut Store. The six read-only helpers around
it go back to &Store: checkpoint_state was given &mut on the theory that
a future cache would need it, but the cache accessors are &self by
design and use interior mutability precisely so a read-only helper can
record a derived value. The module's invariant is restored, so which
functions can mutate consensus state is visible again at a glance.
Fixture suite unchanged at both presets.
* feat(storage): byte-domain state deltas for the beacon chain
VCDIFF rather than xor-plus-compress: an SSZ list that grows shifts every
offset after it, which an xor delta cannot survive and a COPY/ADD stream
handles natively. That is what lets the three big arrays stay untouched
instead of needing length-aware handling of their own.
Beacon needs its own algorithm because lean's StateDiff omits validators
on the assumption they never change, which a beacon registry breaks every
epoch. Lean keeps its own, which is smaller and cheaper there and keeps a
C dependency off the path every devnet runs.
Sized through the *_with_output_len variants rather than the crate's
convenience wrappers, which ask for (input + src) * 2: at beacon scale
that is a ~1.4 GB transient per call and the u32 cast wraps above a
~1.07 GB source. The frame carries the target length so a decode can size
its output exactly.
default-features = false because the crate's stream feature does not
compile at this rev; the one-shot API used here is outside that module.
* docs: replace the delta layer's guessed costs with measured ones
A standalone probe of the codec, before wiring it in, answered two of
this design's open questions and corrected a third.
The dependency does not build as specified: its default `stream` feature
pulls an async module that fails to compile at the pinned rev, so
`default-features = false` is mandatory rather than tidy. The one-shot
in-memory API this design uses is outside that module.
The source-window risk is deleted. xdelta3's 64 MiB window belongs to its
streaming API; this crate wraps the one-shot memory API, which takes the
whole source as one buffer. A one-byte change 79 MB into an 80 MB base
still yields a tiny delta.
And the cost estimates were pessimistic by roughly eight times. Measured
on a 64 MiB buffer with three quarters of its bytes rewritten to
index-derived values: delta 1 MiB, encode 229 ms, decode 6 ms. Scaled to
a mainnet state that puts encode near 1.3 s, inside a slot, and a
31-delta fold near one second rather than eight. The epoch-sized
snapshot interval survives.
The benchmark task still earns its place: the extrapolation is linear and
xdelta3's hash table need not be, and the probe's change pattern is more
structured than a real reward distribution, so it flatters the delta size
more than the timings.
* feat(storage): wire the byte-domain delta codec into beacon state storage
A mainnet BeaconState is on the order of 350 MB, so a snapshot per block
(the placeholder insert_state/get_state shipped with) is not viable past
the minimal preset the fixture suites run against. insert_state now
snapshots only at fork-scoped intervals (one epoch for beacon, unchanged
for lean) and diffs everything in between with the byte-domain VCDIFF
codec added in the previous commit; get_state folds the resulting delta
chain in the byte domain and SSZ-decodes exactly once, at the end, since
folding bytes is memcpy-speed while a mainnet-scale SSZ decode is not.
ForkName::snapshot_interval centralizes the interval so lean's storage
layer and the new beacon path share one source of truth instead of each
picking its own constant.
* refactor(storage): one bounded state cache for both chains
The beacon fork choice used to hold whole states in maps with no cap,
which is not a tuning question at a few hundred megabytes each. It now
shares the LRU the lean chain already had, so one capacity bounds
everything.
Behind an Arc because a hit must not copy: returning an owned state would
hand back much of what the cache saves, and the fixture presets are too
small for a test to notice.
Block and checkpoint states share one bound through a key enum rather
than getting a map each, since two separate caps overshoot the total. A
checkpoint is keyed by epoch as well as root, because a checkpoint's root
is the last block at or before its boundary slot and the same root can
serve different epochs.
The accessors take &self so the read-only fork-choice helpers can record
what they derive on a miss; that is what lets checkpoint_state stay
&Store instead of widening its callers.
* test(storage): measure beacon delta cost on a real mainnet-scale state
The design's encode-cost risk was marked "largely retired by measurement"
on a synthetic 64 MiB probe whose 75%-rewritten, single-pass change
pattern is far more structured than a real epoch transition, and whose
timings were extrapolated linearly past a hash-table codec that need not
scale that way. Both assumptions mattered: measuring a real 2,000,000-
validator electra state (280.7 MB) shows encode cost tracks how
fragmented a change is, not just buffer size.
One ordinary slot: 924 B delta, 375 ms encode, 26 ms decode, comfortably
inside a 4 s slot. Across an epoch boundary: 26.0 MB delta, 3.04 s
encode, 111 ms decode, roughly 2.3x the earlier linear guess and three
quarters of a slot on its own. The epoch-sized snapshot interval still
survives, but only because `is_anchor` already routes the expensive
shape around xdelta3 entirely (always a full snapshot at that boundary,
never a delta): its safety is contingent on staying epoch-aligned, not
on the codec being fast for any shape.
Recorded in the design doc alongside the probe for contrast.
* docs: price the delta measurements against a beacon slot, not a lean one
The measurement section compared both encode times to a 4 second slot,
which is the lean chain's slot time. Mainnet's seconds_per_slot is 12, so
the ordinary-slot delta is about 3% of a slot rather than "most of it
free", and the epoch-boundary shape is about a quarter of a slot rather
than three quarters.
The conclusion is unchanged, and so is the reason: the epoch-transition
shape never reaches the codec, because is_anchor writes that state as a
raw snapshot. But a quarter of a slot and three quarters read very
differently to whoever has to decide whether the contingency is
acceptable, so the arithmetic is worth getting right.
* chore: remove implementation plan
* refactor(storage): the unrealized justification is scratch, not a table
`get_voting_source` reads a block's unrealized justification for every
block from a prior epoch, and recomputing a missing entry means replaying
epoch processing on a copy of that block's post-state. That is what earned
it a column family of its own, but it does not make the map chain history:
no beacon path resumes from a data directory today, and the one that
eventually will re-imports the unfinalized window from its anchor, refilling
the map as it goes.
So move it into `BeaconScratch` beside the other beacon fork-choice maps and
drop `Table::BeaconForkChoice`, putting the table count back at eight. The
accessors keep their signatures, so fork choice is untouched; a read is now a
mutex rather than a backend read plus an SSZ decode, and a write no longer
commits a batch per block.
The struct's bound claim changes with it. `block_timeliness` and the
justifications are both one entry per block imported and neither is pruned,
so "bounded by the unfinalized window" was already loose; say what is
actually true of each map instead.
A data directory written by an earlier commit of this branch will no longer
open, since RocksDB requires every on-disk column family to be listed.
Nothing outside this branch ever had the table, so no lean directory is
affected.
* refactor(types): peel a lean value off the shared enum in one place
`Store::get_state` and `get_signed_block` hand back the fork-generic
`BeaconState`/`SignedBeaconBlock` for both chains, so every lean-only
caller narrowed the result again by hand: thirteen copies of
`let BeaconState::Lean(x) = y else { unreachable!(..) }` across five
crates, each carrying its own wording for the same impossibility, and one
that logged and skipped rather than panicking at all.
Give both enums an `expect_lean` accessor and put its panic behind
`beacon_value_unreachable`, beside the `lean_state_unreachable` pair that
already answers the opposite direction. The invariant that a data
directory holds one chain for its whole life is now asserted in one place
instead of thirteen, and a new lean caller cannot word it differently or
weaken it into a silent skip.
* refactor(storage): one head and checkpoint writer for both chains
The beacon arm kept its own checkpoint rows because the two chains'
`Checkpoint` types carry different fields, lean's `{root, slot}` against
beacon's `{epoch, root}`. That reason does not survive contact with the
fact that an epoch names its own start slot: storing it that way round
trips exactly through a division, so one slot-denominated row serves
either chain and `Store::update_checkpoints` becomes the single writer of
the realized pair. `KEY_BEACON_JUSTIFIED` and `KEY_BEACON_FINALIZED` go
away, and with them the four setters that fed them.
`KEY_BEACON_HEAD` goes too. The head is `KEY_HEAD` on both chains now, so
`beacon_head` derives its slot from the head's own header row rather than
keeping a second `slot || root` value that could drift from the first.
Recording a head through the shared writer also keeps the canonical
`BlockRoots` index in step on a beacon directory, which it never was:
that index holds one block per slot because it holds only the branch fork
choice picked, so forks are no more an obstacle here than on lean.
What the unification actually needed was a chain-agnostic reader for a
stored block's slot and parent root. `block_root_index_changes` decoded a
lean `BlockHeader`, so it could not have run on a beacon directory at
all; `block_entry` now dispatches on the chain tag and both walks share
it. The beacon fork choice reads through it too, in place of the seven
sites that fully decoded a `SignedBeaconBlock`, execution payload
included, to reach two integers.
Pruning stays lean-only, deliberately. The gossip-signature and payload
buffers hold lean attestations a beacon directory never has, and
`prune_live_chain` would be worse than a no-op: `filter_block_tree` asks
`get_checkpoint_block` for the ancestor at the finalized epoch's start
slot, and `get_ancestor` keeps walking while a parent's slot exceeds the
one asked for, so an empty start slot lands below the horizon. A row
missing there is a hard `SpecAssert`, not a degraded read, so beacon
needs its own horizon before it prunes anything.
Also here, from the same read-through: `block_index` was a byte-for-byte
copy of `get_live_chain` and now delegates; `get_weight` takes the block
index instead of rescanning the whole table once per candidate child at
every level of `get_head`'s walk; `set_metadata_batch` puts related
metadata rows under one commit; and the index diff short-circuits when
the head has not moved, which is what a checkpoint-only advance looks
like on the beacon arm.
* refactor(storage): keep a beacon block in one row, not two
Lean splits a block across `BlockHeaders` and `BlockBodies` for two
reasons: a header-only query need not pay for the body, and a block whose
body is empty can leave the body row out entirely. Neither reason holds
for a beacon block. It has no empty-body case, and nothing reads a beacon
header without wanting the block behind it, so the split bought a second
write per import and a second way for the two rows to disagree.
`BlockHeaders` now holds the whole `SignedBeaconBlock`, fork-selector
tagged the way a `States` value is, and the beacon arm writes no
`BlockBodies` row at all. `BeaconBlockEntry`, the two-field struct that
stood in for a header, goes away with it, and the tag-then-decode that
`get_signed_block` spelled out inline is now `encode_beacon_block_value`
and its inverse, beside the pair that already does this for states.
The cost is that `block_entry` decodes a whole block, execution payload
included, to answer a slot and a parent root. Its own documentation now
says so and points a caller walking a chain of them at
`Store::block_index`, which reads the same links out of `LiveChain`.
Also corrects the comment on why `BlockRoots` is not written during
import. It claimed a slot can hold several blocks once a fork exists, but
that index only ever holds the branch fork choice picked, so it holds one
block per slot on either chain; what makes it wrong to write here is that
import order does not determine which branch is canonical.
`update_checkpoints` maintains it as the head moves.
* refactor(beacon): read a block's slot from the caller that already has it
Three of the seven places the beacon fork choice reached into the store
for a block's slot were asking for a value the caller already held. Now
that a beacon `BlockHeaders` row holds the whole block, each of those was
a full SSZ decode, execution payload included, to recover one integer.
`get_voting_source` was the worst of them: `filter_block_tree` calls it
once per leaf on every `get_head`, and one frame up it already has that
block's `(slot, parent_root)` out of the block index it is walking. It
now takes the index, like `get_ancestor`, `get_checkpoint_block` and
`get_weight` do, which is also the closer reading of the specification,
whose `store.blocks[block_root].slot` this index stands in for.
`compute_pulled_up_tip` is called only by `on_block`, which has the block
itself in hand, so the slot is passed in. `validate_on_attestation` built
the index a few lines below its own point lookup; building it first lets
the known-block check and the LMD walk share one scan.
The four that remain, in `get_proposer_head` and
`should_override_forkchoice_update`, are proposal-time and have no index
in scope, where a whole-table scan to serve two lookups would be the
worse trade.
* chore(storage): drop a comment for an import that is gone
`BeaconBlockEntry` was the only thing in `store.rs` deriving SSZ, and it
took the `libssz_derive` import with it when 5a8f8be1 folded a beacon
block into one row. The comment above that import stayed.
It explained a real hazard: `libssz` exports traits named `SszDecode`
and `SszEncode`, `libssz_derive` exports derive macros of the same two
names, so a file wanting both has to bind one pair under another name.
Nothing here wants both any more, and a comment describing a collision
that no longer exists reads as a warning against a change that is
already made.
* refactor(storage): drop the beacon import watermark
`beacon_highest_imported_slot` is the highest slot any beacon block was
imported at: monotone, advanced only forward, and deliberately not the
fork-choice head, so that a reorg lowering the head does not make
forward sync re-fetch slots it already has.
Nothing on this branch reads it. Its three callers live on
feat/mainnet-network, which forked from lambdaclass#591 and keeps its own copy in
`beacon_store.rs`, the file this branch folded into `store.rs`: the
range-sync origin, the `Status` head fallback before the first head
computation, and the `lean_sync_local_head_slot` metric. So what stood
here was the storage half of a feature whose other half is on a sibling
line, kept alive by tests written for it and by nothing else.
Carrying it costs a metadata key seeded at every beacon bootstrap, a
point read on every `insert_signed_block` including the lean arm that
has no use for it, and a conditional write. Whichever line lands second
has to reconcile the two copies anyway; there is no version of that
merge made harder by this side being empty.
Also drops half of the `batch.commit()` rationale, which justified the
single commit partly by the watermark never getting ahead of the block
it counts. The other half, that a half-written block is visible to the
`LiveChain` scan without being decodable, is the whole reason now.
* feat(storage): one store clock in seconds, plus a preset tag on the directory
`Metadata["time"]` carried two different units under one key and one accessor:
intervals-since-genesis on a lean directory, Unix seconds on a beacon one.
`Store::time`'s own documentation asserted the lean meaning unconditionally,
and `Store::current_slot` divided by `INTERVALS_PER_SLOT`, so it answered a
number that meant nothing on a beacon store. Nothing read it there yet, which
is the only reason that was not already a bug.
`time` is now a Unix second on both chains, which is what the beacon
specification's `Store.time` is, and `current_slot` derives the slot from it
for either chain. Lean keeps its interval clock under a second row,
`intervals_since_genesis`, because it cannot be derived from the first: with
`INTERVALS_PER_SLOT` intervals to a slot most interval boundaries fall
strictly between two whole seconds, and `on_tick` steps that grid one interval
at a time, running a duty per step. Both advance from the same tick, the
shared one forward-only so that `tick_to_slot` handing back an earlier
timestamp cannot rewind it.
Every lean site that meant "which interval are we in" now says so: the
attestation and block future-slot guards, the tick idempotency guard, the
arrival-metrics bound, the early-aggregation gate, the fork-choice fixtures'
`checks.time` and the Hive driver snapshot. The two block/attestation guards
in particular are documented as mirrors of each other, so both read the clock
`on_tick` rewinds to replay a slot rather than one of each.
`get_slots_since_genesis` now goes through `Store::current_slot` instead of
keeping a second copy of the arithmetic. It had been reading `genesis_time`
off the store and `seconds_per_slot` off its `config` parameter, and those are
not the same value: `init_beacon` overwrites `genesis_time` from the anchor
state, so a caller holding a config with a placeholder there got a split
clock.
Separately, a data directory now records which SSZ preset the build that wrote
it used, beside the chain tag and for the same reason: a raw byte, readable
before anything else in the directory is decoded, by a build that would decode
the rest into the wrong shape. `db_version` cannot stand in for it, because
both presets write the same layout at the same version while bounding every
SSZ container differently; a `preset-minimal` build opening a mainnet
directory passed both existing gates and then misread it. `from_db_state`
refuses a mismatch, and refuses a directory with no preset recorded, since the
whole point of the row is that nothing else in the directory says which shapes
it holds.
* refactor(storage): one clock row in milliseconds, every other grid derived
The previous commit gave the two chains one `Metadata["time"]` in seconds and
handed lean a second row for its intervals, because four of every five
interval boundaries fall between two whole seconds and a seconds row cannot
name them. Two rows is the wrong answer to that: they are the same clock, and
anything that must be kept in step can fall out of step.
The row is milliseconds instead, which is fine enough for the finest grid
either chain schedules on, and the intervals row is gone. `intervals_since_
genesis` now derives from the row rather than holding its own copy of it, as
`current_slot` already did, and both go through one `ms_since_genesis` that
owns the genesis subtraction and its saturation. Nothing stores a derived
reading, so no two readings can disagree.
`on_tick` walks that one row forward an interval at a time, which is what it
was already doing to the intervals row, and leaves it on the boundary it
processed rather than on the tick's own timestamp: the leftover milliseconds
carry no duty that has run. The forward-only guard the previous commit needed
for the second row goes away with it, since the only backwards write left is
the deliberate rewind that replays a slot.
The beacon specification denominates its `Store.time` in seconds, so
`on_tick_per_slot` and `get_forkchoice_store` multiply on the way in and the
fixture harness divides on the way out. That is three call sites against a
second stored unit, and it is why the accessor is `time_ms` and not `time`:
the two quantities are one unmarked factor of a thousand apart, which is the
confusion this whole change exists to remove.
`is_proposing_on_time` and `on_block`'s timeliness check get more precise for
free. Both were computing a whole-second difference and multiplying it back up
to compare against a basis-point deadline within the slot; both now read
`ms_since_genesis` directly. The fixtures drive those through whole seconds,
so the suites see no change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.