Greptile Summary

This PR centralizes conversion of the persisted store clock into the current slot without changing runtime behavior.

• Adds Store::current_slot() using the shared INTERVALS_PER_SLOT constant.
• Replaces three open-coded slot calculations in tick processing, block import, and attestation handling.
• Updates the Store::time() documentation to direct slot callers to the new accessor.

Confidence Score: 5/5

The PR appears safe to merge with no actionable behavioral defects identified.

The new accessor performs the same metadata read and integer division as all replaced expressions, and it is available through existing unconditional crate dependencies.

_{Reviews (1): Last reviewed commit: "refactor(storage): add Store::current_sl..." | Re-trigger Greptile}

🤖 Kimi Code Review

This is a clean refactoring that properly centralizes slot calculation logic. The changes are correct and improve maintainability.

Approval with minor observations:

1. Code Quality (crates/storage/src/store.rs:832) — Good encapsulation of the time / INTERVALS_PER_SLOT calculation. The documentation update correctly guides callers to use the new helper.

2. Performance Note (crates/blockchain/src/store.rs:348-349) — Not introduced by this PR, but on_tick now performs three separate storage reads for time (one in set_time, one in current_slot, one for interval). Consider caching store.time() in a local variable to reduce redundant DB/metadata lookups:

   let new_time = store.time().unwrap() + 1;
   store.set_time(new_time).expect("set_time should succeed");
   let slot = new_time / INTERVALS_PER_SLOT; // or keep using current_slot() if preferred

   This is minor since metadata reads are typically cached in memory, but worth noting if this path is hot.

3. Panic Consistency — The change from expect("DB read should succeed") to expect("store time exists") (in on_block_core) is actually an improvement; the new message better reflects the invariant being asserted.

4. Consensus Correctness — The slot horizon check (current_slot + 1) and attestation timing logic remain semantically identical. The INTERVALS_PER_SLOT division is the correct spec-derived calculation.

Verdict: LGTM. The refactoring reduces duplication without changing consensus-critical behavior.

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

🤖 Claude Code Review

This is a clean, correctly scoped refactor. Verified the behavior claim holds — time() internally calls get_metadata, which already panics (via .expect) on any read failure, so current_slot()'s own .expect("store time exists") is unreachable in practice; the three call sites' panic behavior is preserved as described.

Findings

Stale doc comment now duplicates the formula current_slot was added to centralize (crates/blockchain/src/store.rs:328-331)

The doc comment directly above on_tick still spells out slot = store.time() / INTERVALS_PER_SLOT, even though the function body right below it now calls store.current_slot() (line 351). Since the PR's stated purpose is eliminating the open-coded division at call sites, this comment is now the last place restating the formula outside Store::current_slot/Store::time's own doc. Worth updating to say "see Store::current_slot" instead, so there's a single source of truth for the derivation.

Everything else is solid:

• Store::current_slot() (crates/storage/src/store.rs:832-835) is a straightforward, correctly-placed accessor — good spot next to time()/set_time().
• The time() doc comment update at crates/storage/src/store.rs:819-822 appropriately delegates to the new method rather than re-deriving the formula.
• All three call-site replacements (lib.rs:1416, store.rs:351, store.rs:640) are behavior-preserving; the two expect/unwrap panic-message differences are cosmetic and called out honestly in the PR description.
• Import of INTERVALS_PER_SLOT in crates/storage/src/store.rs is now actually used, and the pre-existing uses of INTERVALS_PER_SLOT for interval derivation, tick skip-check, and test set_time calls were correctly left untouched — the PR didn't overreach into unrelated INTERVALS_PER_SLOT usages.
• No SSZ, fork-choice, or attestation-validation logic touched; scope is exactly what's advertised.

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

🤖 Codex Code Review

No blocking findings in the diff.

The refactor preserves the existing slot math at the changed call sites in crates/blockchain/src/lib.rs and crates/blockchain/src/store.rs, crates/blockchain/src/store.rs. Moving INTERVALS_PER_SLOT and the millisecond timing constants into crates/common/types/src/constants.rs is a net improvement for consensus safety because it removes duplicate definitions across crates.

Residual risk only: crates/storage/src/store.rs introduces Store::current_slot() as an infallible wrapper around time().expect(...). That matches the previous unwrap/expect behavior at the current call sites, so I do not see a regression here, but it does bake panic semantics into the storage API. If the intent is to keep backend corruption/read failures recoverable, returning Result<u64, Error> would be cleaner.

I could not run cargo check in this environment because rustup tried to create temp files under a read-only /home/runner/.rustup, so this review is based on the diff and source inspection rather than a local build.

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