feat(config): read the slot duration from the network config file - #598
feat(config): read the slot duration from the network config file#598MegaRedHand wants to merge 3 commits into
Conversation
🤖 Kimi Code ReviewAutomated review by Kimi (Moonshot AI) |
🤖 Codex Code Review
No other blockers stood out in the touched timing/storage paths on static review. I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewReview: PR 598 — configurable slot durationThis is a well-scoped refactor: A few minor points worth considering, none blocking:
Nothing else stood out — the SSZ round-trip/legacy-decode tests for Automated review by Claude (Anthropic) · sonnet · custom prompt |
The slot duration was a compile-time constant, so trying a different cadence on a devnet meant rebuilding and reshipping every client. It is a network-wide parameter that belongs with the other things every node on a chain has to agree on, next to `GENESIS_TIME`. `config.yaml` gains an optional `MILLISECONDS_PER_SLOT`, defaulting to the 4000 the whole network already ran on. It must be a positive multiple of `INTERVALS_PER_SLOT`, rejected at parse time otherwise: the five intervals are cut from it and every duty is scheduled off those boundaries, so a value that does not divide evenly would drift the grid against the wall clock. `INTERVALS_PER_SLOT` stays a constant. Each interval carries a distinct duty, so the count is part of the protocol rather than a knob. The value cannot go into the config the state carries, which is merkleized into the state root and whose layout is fixed by the spec. That type is renamed `StateConfig`, and `ChainConfig` now names what the node itself persists under `Metadata["config"]`: genesis time plus slot duration. Every consumer reads it through `Store::config`, which already carried the genesis time. Values derived from the cadence now follow it: interval length, the aggregation deadline (one interval), the early-aggregation window, and gossipsub's duplicate cache, which the spec defines in slots. The early-aggregation window is now clamped to one interval rather than guarded by a const assert. It is subtracted from an interval offset in two places, and a short enough cadence would make the nominal 600 ms wider than an interval and underflow both. Compatibility: - A data directory written before this change holds a bare SSZ `StateConfig` under `Metadata["config"]`. It decodes as the 4-second default, which is the only cadence it could have run, so existing nodes resume rather than fail. - Resuming a data directory whose persisted cadence disagrees with the config file is refused with `GenesisMismatch::SlotDuration`. Its blocks are indexed against a different time grid, which makes it as foreign as another genesis, and the state cannot reveal this because the duration is deliberately absent from it. - Other clients hold the value at compile time and ignore unknown config keys, so setting it only takes effect on a network where every node reads it.
9a38f2d to
436b9c1
Compare
Brings in the offline block-building benchmark sub-command (#595). Conflicts and adaptations: - `NEW_PAYLOAD_CAP` (crates/storage/src/store.rs): main made it `pub` for the benchmark's pool seeding; this branch dropped the hardcoded "~4s" from its doc comment because the slot duration is now configurable. Kept both. - `benchmark::corpus` builds its synthetic store through `Store::from_anchor_state`, which now takes the slot duration; the harness derives tick timestamps from slot numbers rather than a clock, so it passes `DEFAULT_MILLISECONDS_PER_SLOT`.
The slot-duration knob exists to slow a network down, not to speed one up. A config file asking for a shorter slot than the spec's is now rejected when it is parsed, instead of silently reshaping the timings the client holds in milliseconds rather than as a fraction of the slot. That floor lets the early-aggregation window go back to being a plain constant. Scaling it with the interval only ever guarded the two subtractions it feeds from underflowing at cadences a config file can no longer ask for, and what the window buys is wall time for a leanVM proof, which costs the same however long the slot is. Its invariant returns to a const assert, now measured against the narrowest interval any network may configure rather than a compile-time one.
| let slot_start_ms = genesis_time_ms + slot * MILLISECONDS_PER_SLOT; | ||
| let time_config = *self.store.config(); | ||
| let slot_start_ms = | ||
| time_config.genesis_time_ms() + slot * time_config.milliseconds_per_slot; |
There was a problem hiding this comment.
Tiny dedup opportunity: this hand-rolls the slot-start formula that SlotInterval already centralizes — SlotInterval::BlockPublication.to_ms_since_genesis(slot, &time_config) is a value-identical drop-in (interval index 0), and it's what the adjacent t2_ms computation already uses via SlotInterval::Aggregation. Same applies to get_proposal_head in store.rs (store.config().genesis_time_ms() + slot * store.config().milliseconds_per_slot), where it would also drop the doubled store.config() call. Keeps the time grid encoded in exactly one place if its shape ever changes.
|
Not introduced by this PR (the multiply pre-dates it), but since these exact lines are re-plumbed here, noting for a follow-up: |
2 participants
What
config.yamlgains an optionalMILLISECONDS_PER_SLOT. It defaults to4000, the value the whole network already ran on, so an existing config file behaves exactly as before.The value must be a positive multiple of
INTERVALS_PER_SLOT, rejected at parse time otherwise: the five intervals are cut from it and every duty is scheduled off those boundaries, so a duration that does not divide evenly would leave the last interval short and drift the grid against the wall clock.INTERVALS_PER_SLOTstays a compile-time constant. Each interval carries a distinct validator duty, so the count is part of the protocol rather than a tuning knob.Why not put it in the state's
ConfigState.configis SSZ-merkleized into the state root, so its layout is fixed by the spec and cannot gain a field without changing every state root and breaking the SSZ fixtures.So there are now two config types:
StateConfigState.config, merkleizedgenesis_time, unchanged spec layout (this is the oldChainConfig, renamed)ChainConfigMetadata["config"]in the DBgenesis_time+milliseconds_per_slotConsumers read the second one through
Store::config, which already carried the genesis time, so most call sites changed from a constant to a field on something they already had in hand.What now follows the configured cadence
SlotInterval,ms_until_next_interval,store::on_tick).constassert. It is subtracted from an interval offset in two places, and a short enough cadence would make the nominal 600 ms wider than an interval and underflow both.SECONDS_PER_SLOT * JUSTIFICATION_LOOKBACK_SLOTS * 2and which was pinned at4 * 3 * 2seconds.GET /lean/v0/config/spec, which now reports what the node is actually running rather than what it was built with.Slot-count constants (
SNAPSHOT_ANCHOR_INTERVAL,MAX_RESUMABLE_DB_STATE_AGE, the payload buffer caps) are deliberately left alone: their cost is per-slot, not per-second. Their doc comments now say which wall-clock figure assumes the default cadence.Prometheus histogram buckets are also left alone, since Prometheus fixes them at registration. A network on a different cadence reads the arrival and tick histograms against the default grid; noted in
docs/metrics.mdand in the bucket definitions.Compatibility
StateConfigunderMetadata["config"]. It decodes as the 4-second default, which is the only cadence it could have run under.GenesisMismatch::SlotDuration. Its blocks are indexed against a different time grid, which makes it as foreign as another genesis, and the state cannot reveal this because the duration is deliberately absent from it.SECONDS_PER_SLOT) and their genesis parsers ignore unknown keys, so setting it only takes effect on a network where every node reads it. Until the spec adopts a config-file key, this is an ethlambda-only knob.Tests
ChainConfig: SSZ round trip, legacy blob decoding as the default cadence, garbage rejected.from_db_staterejects a slot-duration mismatch, and resumes a pre-MILLISECONDS_PER_SLOTdata directory.ms_until_next_intervalat and around a boundary, the early window never exceeds an interval at any cadence.store::on_tickadvances one interval per configured interval at 8 s, rolling the slot over after five.GET /lean/v0/config/specreports8000/1600for a store built at 8 s.