Repository navigation
fix(timing): take slot duration from the beacon node (#642) - #685
Conversation
Coverage ReportTotal: 94.40% (98253 / 104080 lines)
|
6a24bd3 to
31c1d5a
Compare
Doppelganger epoch, liveness sleep, the index-resolver tick, the import hold, the default exit epoch, the slashing-monitor interval, and the Gloas payload deadline use the slot duration from the beacon node spec. Slot boundaries stay in milliseconds, so a 1500 ms slot starts slot 2 at genesis+3000 ms. Responding beacon nodes must agree on the duration, SLOTS_PER_EPOCH, and genesis time. A missing, zero, conflicting, out-of-range, or disagreeing value fails closed. A named network skips a beacon node that does not respond, requires at least one response, and must match the preset duration and genesis. Custom still requires every configured beacon node. An explicit genesis_time cannot override a named-network preset. Epoch 0 marks keys Safe only on a named network after that preset match. The keymanager exit uses the startup duration and genesis time. The CLI exit rejects a genesis time that is not the named-network preset. Fixes #642 Co-authored-by: Joonkyo Kim <rootwarp@gmail.com>
31c1d5a to
7643540
Compare
rootwarp
left a comment
There was a problem hiding this comment.
Formal review — PR #685 R6 (head 7643540, base develop d47e230, 1 ahead / 0 behind)
Delta reviewed: R5 31c1d5a → 7643540 (7 files, +236/−27).
CI
All 9 required checks pass: Check, Coverage, Scripts, Secret Scan, Architecture Gates, Spec Vectors, Trace continuity, GitGuardian, Test (all features).
Local (cargo 1.99.0, 8 affected crates)
- fmt: clean. clippy
-D warnings --all-targets --all-features: clean. - nextest: 2994/2997 pass. The 3 failures are known box flakes (
refresh_clean_shutdown,async_heartbeat, clistart_helptimeout). - gitleaks (sse.rs, error.rs): no leaks.
- Fork-hazard inventory, checked with my own parser: 60/60 needles sit on their pinned lines and match the audit table in both directions. The re-pins are client.rs 548→547, 1247→1246 and 2124→2123.
- Spawn census: 17/17 rows hold. Rows that name the task through a constant were checked by hand; R6 does not touch those files.
R5 items
- B1 — resolved. The SSE admission fallback (
sse.rs:311-318) now scans(1..len).map(|o| (current_idx + o) % len), so it searches forward and wraps. The new three-node testsse_failover_reaches_a_later_admitted_nodeuses[failing, unadmitted, healthy]. Mutation M5, which restores the scan from index 0, is caught. - S1 — resolved.
sse_drops_chunks_after_admission_is_revokedcovers the per-chunk check. M4, which makes that check (if !endpoint_admitted(current_idx) { break; }) a no-op, is now caught. Both new tests passed 20/20 under--stress-count 20. - Doc fix — done. The
subscribe_events_admitteddoc now says "per chunk", which matches the code. - S3 — resolved. New
BeaconError::GenesisValidatorsRootMismatch { root }. Both producers use it:ensure_genesis_validators_root(client.rs:213) andconfirm_genesis_validators_root(manager.rs:2834).startup::validate_genesis_rootmatches on the variant, and the string match is gone.- M6 (startup matches a different variant) is caught by test_validate_genesis_root_mismatch and test_connect_beacon_rejects_genesis_root_mismatch.
- M7 (manager returns HttpError) is caught by test_connect_beacon_rejects_genesis_root_mismatch.
- S2, S4: Ash has deferred both to a follow-up. Both fail closed.
- R4 and R5 checks still hold. The R5 mutations M1–M3 are unaffected by this delta.
Soft
- S5 (Codex P3, kept as P3 soft):
sse_drops_chunks_after_admission_is_revoked(sse.rs:~1726) relies on a fixed 400 ms sleep. It does not confirm that chunk 3 was sent before asserting. On a heavily loaded runner it could pass without exercising the guard. Locally it does catch M4, and it passed 20/20 under stress. Suggest syncing on a server-side "chunk 3 written" signal. - Hygiene: the commit subject "fix(timing): take slot duration from the beacon node" does not describe this delta. Consider rewording it at squash time.
Blocking
None.
Codex gpt-6.1-sol (high, read-only) session 01a144da-2f26-7192-b4e2-5dcb6d9c2caf: one P3 (S5), no P1 or P2.
Formal verdict: APPROVE
rootwarp
left a comment
There was a problem hiding this comment.
Security re-audit (R6, scoped, Codex only) of #685 at head 7643540 (vs 31c1d5a, base develop @ d47e230)
Verdict: CLEAR-WITH-P3 (my rating). Stricter verdict: BLOCK (Codex).
- Codex gives BLOCK because two of the three requested fixes are incomplete, not because it found a new risk.
- Both gaps predate R6 and fail safe, so I rate them P3.
- No doppelganger or slashing risk, so I did not escalate to Claude.
Interdiff scope: sse.rs and its tests, plus the typed root-mismatch change: the beacon/src/error.rs variant, client.rs ensure_genesis_validators_root, a 4-line change in manager.rs confirm_genesis_validators_root, and the startup.rs match. The rest is fork-hazard line pins (client.rs 547/1246/2123), which resolve correctly. No dependency changes.
Items
- SSE failover: FIXED. The scan is forward-circular over admitted endpoints (
bn-manager/src/sse.rs:315), so there's no more starvation. With none admitted it sleeps 50 ms and selects on shutdown (sse.rs:324-331), so there's no busy-spin. - Chunks dropped after revocation: PARTIAL.
- Admission is checked before awaiting
resp.chunk()(sse.rs:454). There's no re-check after the await, before framing and forwarding (:465). Queued callbacks don't check admission either (sse.rs:283). - The new test only rejects the second post-revocation frame (slot "3") and lets the first through (slot "2") (
sse.rs:1724,:1734). - The doc at
sse.rs:259("read before each connection and per chunk") is accurate, but it isn't a delivery guarantee. - Nothing marks a node unadmitted after boot today, so this can't currently trigger. P3.
- Fix: re-check after the await, and carry endpoint identity through the callback queue.
- Admission is checked before awaiting
- Typed root mismatch: PARTIAL.
- The typed variant
BeaconError::GenesisValidatorsRootMismatchreplaces the string match. The startup match is now exact (startup.rs:123), and other errors still go toerr.into()and fail closed. - No classification regression.
is_unreachableand failover classification never saw this error, because the raw client path doesn't produce it (manager.rs:70,:2663,client.rs:484).confirm_genesis_validators_rootstill skips wrong-root responders and returns the typed error only when none match (manager.rs:2815). Admission rejects before setting the flag (manager.rs:2750). - Not fixed (R5 P3): in
agreed_spec_and_genesis, a node'sgenesis_timeis folded into agreement before its root is checked (manager.rs:2605then:2606). UnderResponding, a wrong-root node with a different genesis_time still stops boot instead of being skipped. This fails safe; it's an availability problem. P3. - Fix: check the root first, then fold in that node's values.
- The typed variant
Checks
- fmt OK. clippy
-D warningsOK. - Scoped nextest (rvc-bn-manager, rvc, rvc-architecture-tests): 1977 run, 1975 passed, 2 failed.
config_url::refresh_clean_shutdown: known flake.import_budget::async_heartbeat_has_no_gap_over_50ms_while_100_keystores_import: a timing test that passed on rerun.
- No dependency changes, so no cargo audit.
- CI: all green (Check, Test all-features, Architecture Gates, Spec Vectors, Coverage, Scripts, Trace continuity, Secret Scan, GitGuardian).
Fixes #642 (parent #638). Rebased onto develop
d47e2302.Slot duration for the doppelganger epoch, liveness sleep, index-resolver tick, import hold, keymanager and CLI default exit epoch, slashing-monitor interval, and the Gloas payload deadline comes from
GET /eth/v1/config/spec. The unusedLivenessChecker/DoppelgangerStatus/DoppelgangerResult/BeaconLivenessAdaptersurface is removed.ValidatorLivenessDatastays.Security fixes:
SLOT_DURATION_MSorSECONDS_PER_SLOT, onSLOTS_PER_EPOCHmatching the network preset, and on genesis time matching the configured genesis. A missing, zero, conflicting, or disagreeing value fails closed.Network::slot_duration_ms). Custom may accept a bounded beacon-node value. There is no silent 12 s fallback.SLOTS_PER_EPOCH, genesis time, and genesis validators root match that boot agreement. Recovering/eth/v1/node/syncingdoes not admit it.ApiError.genesis_timethat differs from the preset. Beacon genesis must match the configured genesis.VoluntaryExitManagerAdapter::new. There is no zero default and nowith_genesis_time. It does not re-fetch spec or genesis from whichever pool node answers first.SLOTS_PER_EPOCHthat is not the preset, and a genesis time that is not the named-network preset.wall_clock_epochhelper, which assumed a 12 s slot, is removed.Duration::MAX. The slashing-monitor interval uses the startup duration.BlockService::newandKeystoreManagerAdapter::newstill start from 12 s; production replaces those, anddocs/releases/UNRELEASED.mdsays so.BeaconError::GenesisValidatorsRootMismatch. Startup matches that variant.fork_hazard_inventoryand.gitleaksignoreare re-pinned where lines moved.Q-188 and B-026 are tracked in the external review notes.
Verification on
7643540439916ba51c393e037d9be3c99a7a6c49:cargo nextest run --workspace --locked(6061 passed, 9 skipped),cargo clippy --workspace --all-targets --locked -- -D warnings, andcargo llvm-cov nextest --workspace --locked(6061 passed, 9 skipped).