feat: add clock module for slot/epoch timing (Zig 0.16)#301
feat: add clock module for slot/epoch timing (Zig 0.16)#301GrapeBaBa wants to merge 4 commits intoChainSafe:chore/zig-masterfrom
Conversation
Three-layer beacon clock architecture: - Layer 0 (slot_math): pure arithmetic, comptime-compatible - Layer 1 (SlotClock): stateful clock with pluggable time source - Layer 2 (EventClock): async event loop with listeners and waiters Ported from ChainSafe/clock-zig for Zig 0.16.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust, three-layer beacon clock architecture designed for Zig 0.16. By separating pure arithmetic, stateful time tracking, and asynchronous event handling, the new module provides a reliable foundation for slot and epoch timing within the project. The implementation includes comprehensive testing and seamless integration into the existing build system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a three-layer beacon clock architecture for Ethereum consensus, comprising pure arithmetic (slot_math), stateful tracking (SlotClock), and an async event loop (EventClock). The reviewer identified several violations of the project's safety-first style guide, specifically regarding the lack of mandatory assertions, the need for in-place initialization of large structs to manage stack growth, and various formatting issues such as line length limits and the requirement for braces in multi-line conditional statements.
| /// Initialise in-place. Takes `*EventClock` (not returning by value) | ||
| /// because `TimeSource.fromIo` stores a pointer into `self.io`, making | ||
| /// the struct self-referential. | ||
| pub fn init(self: *EventClock, allocator: Allocator, config: Config, io_handle: std.Io) Error!void { |
There was a problem hiding this comment.
This function, and many others in this module, are missing assertions for arguments and invariants. The repository style guide requires that all function arguments be asserted and that the assertion density averages at least two assertions per function. This is critical for the safety-first design goals of the project.
References
- Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. (link)
| .clock = undefined, | ||
| .waiters = WaiterQueue.initContext({}), | ||
| }; | ||
| self.clock = SlotClock.init(config, TimeSource.fromIo(&self.io)) catch return error.InvalidConfig; |
There was a problem hiding this comment.
Following the style guide, SlotClock should be initialized in-place using an out pointer since it exceeds 16 bytes. This avoids unnecessary copies and stack growth.
try self.clock.init(config, TimeSource.fromIo(&self.io));
References
- Construct larger structs in-place by passing an out pointer during initialization. If the argument type is more than 16 bytes, pass the argument as *const. (link)
| if (self.next_listener_id == std.math.maxInt(ListenerId)) | ||
| return error.ListenerLimitReached; |
There was a problem hiding this comment.
The if statement should either be on a single line or use braces to adhere to the style guide and prevent potential bugs.
if (self.next_listener_id == std.math.maxInt(ListenerId)) return error.ListenerLimitReached;
References
- Add braces to the if statement unless it fits on a single line. (link)
| self.allocator.destroy(state); | ||
| return WaitForSlotResult.immediate(error.Aborted); | ||
| } | ||
| self.waiters.push(self.allocator, .{ .target = target, .state = state }) catch return error.OutOfMemory; |
There was a problem hiding this comment.
This line exceeds the 100-column limit specified in the style guide.
self.waiters.push(self.allocator, .{
.target = target,
.state = state,
}) catch return error.OutOfMemory;
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
| pub fn init(config: Config, time: TimeSource) error{InvalidConfig}!SlotClock { | ||
| try config.validate(); | ||
| var self = SlotClock{ | ||
| .config = config, | ||
| .time = time, | ||
| }; | ||
| self.current_slot = slot_math.slotAtMs(config, time.nowMs()); | ||
| return self; |
There was a problem hiding this comment.
Structs larger than 16 bytes should be initialized in-place using an out pointer to avoid stack growth and unnecessary copies.
pub fn init(self: *SlotClock, config: Config, time: TimeSource) error{InvalidConfig}!void {
try config.validate();
self.* = .{
.config = config,
.time = time,
.current_slot = slot_math.slotAtMs(config, time.nowMs()),
};
}
References
- Construct larger structs in-place by passing an out pointer during initialization. (link)
| const now_ms = self.time.nowMs(); | ||
| const next_slot = current + 1; | ||
| const next_slot_ms = slot_math.slotStartMs(self.config, next_slot) orelse return current; | ||
| return if (next_slot_ms -| now_ms < self.config.maximum_gossip_clock_disparity_ms) next_slot else current; |
There was a problem hiding this comment.
This line exceeds the 100-column limit. Additionally, the style guide prefers explicit branches over complex ternary-like expressions for clarity.
if (next_slot_ms -| now_ms < self.config.maximum_gossip_clock_disparity_ms) {
return next_slot;
}
return current;
References
- Hard limit all line lengths to 100 columns. Add braces to the if statement unless it fits on a single line. (link)
| if (self.seconds_per_slot == 0 or self.slots_per_epoch == 0) | ||
| return error.InvalidConfig; |
There was a problem hiding this comment.
Compound conditions should be split into simple conditions using nested branches or separate statements. Also, the if statement requires braces if it spans multiple lines.
if (self.seconds_per_slot == 0) return error.InvalidConfig;
if (self.slots_per_epoch == 0) return error.InvalidConfig;
References
- Split compound conditions into simple conditions using nested if/else branches. Add braces to the if statement unless it fits on a single line. (link)
There was a problem hiding this comment.
Pull request overview
Adds a new src/clock/ module providing slot/epoch timing utilities and an event-driven beacon clock, integrating it into the build as a standalone module with dedicated tests.
Changes:
- Introduces Layer 0 pure arithmetic helpers (
slot_math) and associated config validation. - Adds Layer 1
SlotClockwith pluggableTimeSourceplus unit tests. - Adds Layer 2
EventClockimplementing an async auto-advance loop, listener registration, andwaitForSlot, and wires clock tests intobuild.zig.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/clock/slot_math.zig | Pure slot/epoch arithmetic + config validation + tests. |
| src/clock/time_source.zig | Pluggable “now in ms” abstraction with std.Io-backed implementation. |
| src/clock/SlotClock.zig | Stateful slot clock built on slot_math, with gossip-disparity/tolerance helpers + tests. |
| src/clock/EventClock.zig | Async event loop with slot/epoch listeners and slot waiters + tests. |
| src/clock/root.zig | Public module root re-exporting clock layers/types. |
| build.zig | Registers clock module and adds test:clock build step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var maybe_future = self.loop_future; | ||
| self.loop_future = null; | ||
| if (maybe_future) |*future| { | ||
| future.cancel(self.io); |
There was a problem hiding this comment.
join() claims it “wait[s] for [the loop] to finish”, but it only calls future.cancel() and never awaits the future. Additionally, if a caller invokes join() without calling stop() first, runAutoLoop() currently treats sleep cancellation as a transient error and continues looping, so the canceled fiber can keep running after loop_future is dropped. Consider making join() set stopped = true (or call stop()), and then await the future to guarantee the loop has actually exited before returning.
| var maybe_future = self.loop_future; | |
| self.loop_future = null; | |
| if (maybe_future) |*future| { | |
| future.cancel(self.io); | |
| // Ensure the loop has been signaled to stop. | |
| self.stop(); | |
| var maybe_future = self.loop_future; | |
| if (maybe_future) |*future| { | |
| // Clear the stored handle before waiting, so a subsequent `start` | |
| // can safely create a new loop. | |
| self.loop_future = null; | |
| // Request cancellation and wait for the fiber to exit. | |
| future.cancel(self.io); | |
| _ = future.*.wait(self.io); |
| /// Create an immediately-resolved result (no async work needed). | ||
| /// Relies on `std.Io.Future.await` returning `.result` when `.any_future == null`. | ||
| fn immediate(result: Error!void) WaitForSlotResult { | ||
| return .{ .inner = .{ .any_future = null, .result = result }, .state = null, .clock = null }; | ||
| } |
There was a problem hiding this comment.
WaitForSlotResult.immediate constructs a std.Io.Future by manually setting internal fields (any_future = null, result = ...) and relies on a specific Future.await behavior when any_future == null. This couples the code to std.Io.Future representation/semantics and is brittle across Zig stdlib changes. Prefer using a documented helper/constructor for an already-resolved future, or adjust the API to avoid fabricating a Future when no async work is needed.
| pub fn await(self: *WaitForSlotResult, io: std.Io) Error!void { | ||
| const result = self.inner.await(io); | ||
| // Free AFTER await returns — workaround for Zig futex use-after-free | ||
| // where GCD still holds a reference to the event address after wake. | ||
| if (self.state) |s| s.allocator.destroy(s); | ||
| self.state = null; | ||
| self.clock = null; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
WaitForSlotResult.await requires the caller to pass a std.Io even though the result already captures the io used to create the future (via EventClock.io / WaitState.io). Passing a different io than the one used for std.Io.async is likely incorrect and can cause hard-to-debug runtime issues. Consider storing the std.Io inside WaitForSlotResult and using it internally (or at least documenting/enforcing that the same io must be passed).
There was a problem hiding this comment.
currently it seems this is the only way to do it
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d31d281e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.loop_future = null; | ||
| if (maybe_future) |*future| { | ||
| future.cancel(self.io); |
There was a problem hiding this comment.
join() clears loop_future and only calls future.cancel(self.io), but runAutoLoop explicitly treats canceled sleeps as recoverable (sleep(... ) catch { continue; }) unless stopped is set. In the start() -> join() path (without stop() first), the loop can keep running while loop_future is already null, so a later start() can spawn a second loop and duplicate slot/epoch dispatches. This is a correctness issue for any caller that follows the current join docstring and expects it to fully terminate the loop.
Useful? React with 👍 / 👎.
- TimeSource is now a tagged union (io: std.Io / fake: *FakeTime), eliminating the self-referential struct problem in EventClock.init - Add FakeTime struct with setMs/advanceMs/advanceSlot methods - Fix join(): call stop() and await future after cancel - Fix runAutoLoop: break on error.Canceled instead of continuing - Fix ArrayListUnmanaged init syntax for Zig 0.16 (.empty)
TimeSource variants are now both proper structs: - RealClock: wraps std.Io, encapsulates wall-clock reading - FakeTime: mutable test clock with set/advance methods
The clock module uses only std.Io APIs with no direct libc dependency.
|
|
||
| pub const Config = struct { | ||
| genesis_time_sec: UnixSec, | ||
| seconds_per_slot: u64, |
There was a problem hiding this comment.
Preferred ChainConfig value to use is slot_duration_ms (seconds_per_slot is deprecated and unused in lodestar).
Also just worth pointing out now, we are anticipating (maybe in h-fork) that we will move to 6 second slots. So might be worth thinking about how you would handle that (eg slotAtMs and slotAtSec would need to consume ChainConfig at a minimum)
See lodekeeper/lodestar@44a4048
Addresses Copilot reviews on PR #301: 1. `immediate` no longer pokes `std.Io.Future` field layout — the union's `.immediate` variant carries the result directly, no more `.{ .any_future = null, .result = ... }` struct literal that depended on stdlib internal field names. 2. `await` drops the `io: std.Io` parameter — the pending variant stores `state.io` (the io that created the future), so we no longer rely on the caller passing the same io and can't get io-mismatch bugs. State/clock fields move from `?*WaitState`/`?*EventClock` (sentinel-null) into the `.pending` payload as non-optional pointers; `.immediate` carries no extra state. After `await` / `cancel` we transition self to `.immediate = result` so subsequent calls are idempotent no-ops. 9 test sites updated: `fut.await(io_handle)` → `fut.await()`. `zig fmt --check` passes. (Pre-existing `b.modules.put` build error in build.zig is unrelated.)
|
Closing in favor of #354 — replanted onto current main. PR #301 had drifted 40 commits behind with 33 unrelated Zig 0.16 migration commits that obscured review (see codex feedback citing build.zig / metrics_stub / fuzz extractor / etc., none of which are clock-related). #354 is +1596 lines of pure clock work on top of zbuild. |
Addresses the second half of wemeetagain's review on PR #301: TS lodestar is preparing for EIP-7782 (consensus-specs#4484, anticipated in h-fork) which switches from 12-second to 6-second slots mid-chain. A single `slot_duration_ms` can't express that. Replaces `Config.slot_duration_ms: u64` with `Config.slot_durations: []const SlotDuration`, where each `SlotDuration` records the first slot at which a duration applies. The first entry must have `start_slot = 0`. Forks that change duration append an entry whose `start_slot = fork_epoch * slots_per_epoch`. `Config.constantDuration(genesis, ms, slots_per_epoch)` is the single-segment convenience for chains without any duration transition (borrows a static-lifetime schedule slice). `Config.slotDurationMsAt(s)` walks the schedule backwards. `slotAtMs` / `slotStartMs` / `msUntilNextSlot` walk the schedule cumulatively, summing per-segment ms until they find the segment that contains the timestamp / slot. Validation enforces non-empty schedule, first entry at slot 0, ascending `start_slot`, and non-zero durations. New tests cover an EIP-7782-shape config (12s slots up to slot 1024, 6s thereafter) and exercise `slotDurationMsAt`, `slotStartMs`, `slotAtMs`, and `msUntilNextSlot` across the boundary. `SlotClock` and `EventClock` test fixtures migrated to the schedule form. Module API itself is unchanged — `slot_durations` is the only config-shape change.
Match the original PR #301 shape that the user asked to revert to: - `WaitForSlotResult` reverts from tagged union (`.immediate` / `.pending` switch) to a plain struct that uses `Future{.any_future = null, .result = ...}` for the immediate case. `Future.await` returns `.result` when `any_future` is null, so the callsite is uniform whether the wait was immediate or pending — no per-call discriminant. - `await(io)` now takes the `io` parameter explicitly. Removes the `state.io` indirection and the awkwardness of needing `state` to exist for an immediate result. - `cancel()` collapses to an `orelse return` early-out followed by the cleanup path; no more two-arm switch. - `advanceAndDispatch` reverts to module-private `fn` (still callable from in-file tests). Was made `pub` only to keep the outside-of-test API surface stable through earlier rewrites. - `runAutoLoop` reverts to the simpler `sleep → dispatch` order with `msUntilNextSlot(now_ms)` and `.awake` clock. The codex P2 findings (catch-up before sleep, suspend-aware sleep) were strict improvements but added complexity that the rest of the lodestar TS reference does not have either; matching TS behavior is preferred here. - Doc comment trimmed back to the original short version. Kept the `slot_duration_ms` Config and fork-aware schedule in slot_math — unrelated to the EventClock surgery. Tests adjust to call `fut.await(io_handle)` and use `slot_duration_ms = N_000` instead of `seconds_per_slot = N`.
Summary
clockmodule undersrc/clock/ported from ChainSafe/clock-zig for Zig 0.16slot_math) – pure arithmetic, comptime-compatibleSlotClock) – stateful clock with pluggable time sourceEventClock) – async event loop with listeners and waitersbuild.zigwithtest:clockstep and included intest(run-all)