refactor(clock): single public Clock; internalize SlotClock - #463
Merged
Conversation
EventClock's current* accessors called catchUp() (one wall read, advance, emit) and then took a SECOND wall read to compute the returned value. If a listener callback inside the catch-up dispatch burns wall time across a slot boundary, the second read lands in a newer slot than the events the first read flushed — the returned slot runs ahead of the emitted events, violating the module's read-flushes-events invariant. Fork choice's FUTURE_SLOT check is the load-bearing consumer: a slot returned ahead of its emitted events can misclassify an on-time block as future. catchUp() now returns its single wall reading and every accessor derives its result from that reading alone. SlotClock's gossip-disparity methods take the reading as a parameter (isCurrentSlotGivenGossipDisparity via an options struct — it would otherwise take two bare u64s) so both the base slot and the disparity window come from the same snapshot (also closing the u64-underflow paths a fresher second read would open in the window arithmetic). runAutoLoop's duplicated catch-up body is folded into catchUp(). waitForSlot already read only the cache after catch-up and is unchanged beyond discarding the new return value. Verified against lodestar clock.ts getter semantics (read the wall once, return that same reading). TS history: the getter went wall-fresh in fe5b91ad45; catchup-emit was added in PR #2417 (daf87974c3).
emitSlot/emitEpoch copied listeners into reused member buffers (slot_snapshot/epoch_snapshot) and iterated those. Since catchUp() became reachable from inside a listener callback, a querying callback during backlog dispatch triggers a nested advanceAndDispatch, and the nested emit's clear()+re-push clobbers the buffer the outer emit is mid-iterating: entries shift under the captured slice, so remaining outer iterations invoke wrong or duplicate (callback, ctx) pairs — a listener removed in the nested frame loses the in-flight event while its neighbor receives it twice. Each emit now iterates a by-value stack copy of the listener list (BoundedArray has inline storage; 16 entries fit trivially on the stack). Nested emits snapshot into their own frame, and on/off during dispatch mutate only the member list. This is the port of Node EventEmitter's clone-handlers-per-emit semantics that the TS reference (Lodestar's clock on EventEmitter) implicitly relies on: removal mid-emit does not affect the emit in flight, and a listener added mid-emit does not receive it. New test pins the exact per-listener sequences under a reentrant listener that burns wall time across a slot boundary, removes a peer, registers a new listener, and queries the clock mid-dispatch: the removed peer still receives the in-flight slot exactly once, and the listener added mid-dispatch misses the in-flight slot but receives the nested one — pinning both halves of the per-emit-snapshot contract. The test fails against the member-buffer implementation (the removed peer loses the in-flight slot; its neighbor receives it twice).
Policy: from a listener callback it is safe to call onSlot, offSlot, onEpoch, offEpoch, stop, all current* accessors, and waitForSlot (awaiting its result still falls under the no-yield rule). A current* query while the cached slot is behind the wall (a backlog) triggers a nested dispatch: later slots and epochs are delivered to all listeners before the in-flight event's remaining deliveries complete, but every (listener, event) pair is delivered exactly once — including an outer iterator's pending epoch, which drains after the nested cascade, so epochs can arrive out of order (a later epoch before the in-flight one). Each nested level consumes at least one slot of the pre-existing backlog, so nesting depth is bounded by the backlog size — a backlog can legitimately accrue while the process is descheduled; levels beyond that require the wall to cross another slot boundary mid-cascade under the unchanged no-yield rule. A reentrancy guard (rejecting or deferring queries from callbacks) was considered and rejected: the Lodestar TS clock allows nested advance, and a guard would have to answer mid-dispatch queries from the cached slot, producing snapshots incoherent with the single wall reading the accessors are defined against (commit 20eddf2). The enabling code landed in the previous two commits (single wall reading; per-emit stack snapshots); this commit is comments and tests only. Five tests pin the policy: a backlog query delivers every (listener, slot) exactly once in the derived interleaving; a non-backlog query is a no-op returning the in-flight slot; nested dispatch delivers epochs out of order ([2, 1]) but exactly once; waitForSlot called from a callback is resolved by the ongoing dispatch; and epoch-listener add/remove mid-emit preserves the per-emit snapshot contract on the epoch path.
Deliver the issue-457 consolidation: the module now exposes ONE public clock type. EventClock is renamed to Clock and becomes the module's public clock; SlotClock stays as a file but is demoted to an internal implementation detail imported by Clock (root.zig no longer exports it, and runs its tests via a direct test-block import since a transitive import compiles but does not run them). Slot/Epoch re-exports retarget slot_math, their source of truth; ListenerId/Error retarget Clock. Clock gains the four pure-read helpers (slotWithFutureToleranceMs, slotWithPastToleranceMs, secFromSlot, msFromSlot) as *const forwards to the internal slot clock, completing the TS IClock surface. Verified against TS clock.ts: none of the four route through the catching-up getter there, so the forwards deliberately skip catchUp(); a new test pins that property (backlog present, listener sees zero events, cache unchanged) alongside the ported value expectations. Header fold-ins from the previous review cycle: the safety list now names the whole current*/isCurrent* accessor family, and the waitForSlot parenthetical covers cancel() (it awaits internally) as well as await().
The gossip-disparity and tolerance/from-slot reads are pure functions of (config, now_ms) — they belong in slot_math with the rest of the slot arithmetic, not on the stateful SlotClock. Bodies move verbatim (self.config becomes a config parameter; currentSlotWithGossipDisparity drops the "current" prefix since a free function has no clock). Clock's public API is unchanged: the catchUp-backed disparity accessors and the four pure-read forwards call slot_math directly, the forwards absorbing the null→now defaulting. Free functions taking two u64-family parameters (now_ms + tolerance_ms, slot + end time) take options structs per the styleguide rule. One behavior addition (design-gate endorsed): slotWithPastToleranceMs subtracts with `-|`. tolerance_ms is caller-supplied runtime data, not program-controlled config, so slot_math's plain-operator convention does not cover it; a tolerance larger than now_ms saturates to 0 ms, which is pre-genesis, which takes the orelse-0 path to slot 0 — instead of trapping. Covered by a new test. The 8 affected SlotClock tests relocate to slot_math as FakeClockIo-free literal-ms tests with identical boundary values and assertion counts; 58/58 (57 before + 1 saturation test). Also folds in: retired Layer 0/1 header taxonomy, stale EventClock reference, root.zig public-surface list gains config/ClockConfig, and Clock's header notes the pure-read helpers are trivially callback-safe.
After the pure reads moved to slot_math, SlotClock was a ~60-line
pass-through layer (fields + init + advance iterator) with exactly one
consumer. Fold it into Clock: Clock now owns the config and the
current_slot cursor directly, and init performs the validate + cache
snapshot itself.
AdvanceIterator is re-pointed at the tight capability it actually
uses — {config, *current_slot} — rather than *Clock, so "the iterator
cannot dispatch" is enforced by construction, not convention. The
stopped-check ordering in advanceAndDispatch and the post-push
dispatchWaiters in waitForSlot move byte-identical.
SlotClock's four production-dead read accessors (test-only since the
catchUp fix) die with the file; Clock's public accessors already
provide the semantics. Its FakeClockIo copy dies too — Clock.zig has
its own (resolves the recorded duplication nit). runAutoLoop's sleep
computation now reads time via self.io, removing the cosmetic
self.clock.io/self.io split (same handle).
SlotClock's 7 tests re-home into Clock.zig: the 4 advanceTo tests
drive the now-private iterator verbatim; the 3 read tests re-target
Clock's public accessors (asserted values unchanged; the cache now
advances with each read, which no assertion depended on).
test:clock stays at 58/58 (-7 SlotClock, +7 re-homed).
Also folds carried comment nits: slot_math header saturation
overclaim, "wrapping subtraction" -> "plain `-`" in the saturation
test, "four forwards" count drop, and stale SlotClock mentions in
root.zig/config.zig headers.
The pure slot_math helpers (isCurrentSlotGivenGossipDisparity, slotWithFutureToleranceMs, slotWithPastToleranceMs, secFromSlot, msFromSlot) take positional params — their names make the argument order self-evident. The Clock forwards and the relocated tests are updated to match. Adds one order-discriminating assertion to the "tolerance helpers" test: slotWithPastToleranceMs is the only order-sensitive helper, and positional params drop the compiler name-check, so a bare operand swap would otherwise go uncaught (the existing saturation/tolerance assertions land pre-genesis either way and don't discriminate). No behavior change.
The single-read invariant comments had accreted qualifying clauses over review and read like committee prose. Trim to the load-bearing WHY: - accessor block: state the single-read discipline (each derives from catchUp's one reading, not a second skewable read) instead of an ordering claim the disparity accessors and post-stop path falsify. - catchUp doc: one causal sentence for why a second read is wrong; keep the precise "Emits nothing" scoping (not "No-op"). - slotWithGossipDisparity: two lines instead of four for the single-snapshot rationale. No behavior change; 58/58.
- Drop the listener-registration block comment (it restated the header's callback-safety list); the emitSlot copy-vs-reference note shrinks to one line. - Delete the five reentrancy-test derivation comments; the test names and pinned literals stand on their own. - Recast module-header invariant #1: the callback-safe set becomes a bullet list and the nested-dispatch / depth-bound prose is tightened, with no invariant dropped (the backlog-bounded depth wording is unchanged in substance; the nesting trigger reads "a query", not just "current*", since waitForSlot also catches up). Comment-only; 58/58.
The standalone slotWithPastToleranceMs-saturates test was one pure-data assertion of a function already exercised by "tolerance helpers"; move it in as a fourth case (future / past / operand-order / saturation) so the tolerance family lives under one name. No for-loop table: these clusters have no shared setup to amortize and are already legible as sequential assertions. 57/57 (one fewer decl, zero assertions lost).
Replace the spelled-out magic-number arithmetic in the three gossip- disparity tests with one terse framing line each that maps the opaque literals to the disparity boundary, and drop the per-assertion restatement comments (the expectEqual(null/0/1, ...) calls already say what they assert). Test 3 keeps its regression WHY (slot_duration <= disparity guards against slotAtMs-orelse-0 clamping pre-genesis to slot 0), tightened to two lines. Collapse the expect(...) calls that were only multi-line as a leftover of the old options-struct width onto single lines. No assertion, literal, or coverage change.
wemeetagain
previously approved these changes
Jul 6, 2026
wemeetagain
left a comment
wemeetagain
left a comment
Member
There was a problem hiding this comment.
I think this is much more readable, lgtm
nazarhussain
self-requested a review
July 7, 2026 16:00
nazarhussain
requested changes
Jul 9, 2026
| /// Accessors must derive from this reading, not a fresh clock read: a slow | ||
| /// callback can cross a slot boundary mid-dispatch, so a second read could | ||
| /// name a slot the just-emitted events haven't reached. | ||
| fn catchUp(self: *Clock) u64 { |
Contributor
There was a problem hiding this comment.
Most of the usage of this function result in recomputing the slot from the given now_ms. Why not return the both from this function to avoid recomputation on multiple places?
| /// Accessors must derive from this reading, not a fresh clock read: a slow | ||
| /// callback can cross a slot boundary mid-dispatch, so a second read could | ||
| /// name a slot the just-emitted events haven't reached. | ||
| fn catchUp(self: *Clock) u64 { |
Contributor
There was a problem hiding this comment.
Possible stack overflow when an onSlot callback calls clock.currentSlot() during a backlog.
currentSlot() calls catchUp(), and catchUp() is also the thing that runs listener callbacks. So while my callback is being called by catchUp for slot N, my call to currentSlot() starts a second catchUp inside it....
With a normal backlog of 1 slot this is harmless. But after the process is paused for a while (laptop sleep, SIGSTOP, VM pause), the backlog can be thousands of slots.
Suggested fix, never dispatch from a nested call. Add a dispatching flag: if currentSlot() is called from inside a callback, catchUp just records the newest wall slot in pending_target and returns.
fn catchUp(self: *Clock) u64 {
const now_ms = time.nowMs(self.io);
const target = slot_math.slotAtMs(self.config, now_ms) orelse return now_ms;
if (self.dispatching) {
// Called from inside a callback: just note the target, don't recurse.
self.pending_target = @max(self.pending_target orelse target, target);
return now_ms;
}
self.dispatching = true;
defer self.dispatching = false;
self.pending_target = target;
defer self.pending_target = null;
while (!self.stopped) {
const t = self.pending_target orelse break;
self.pending_target = null;
self.advanceAndDispatch(t);
}
return now_ms;
}Nice side effect, listeners now always see events in order (1, 2, 3 instead of 2, 3, 1 when a callback triggers a mid-backlog catch-up), so the "out of order but exactly once" caveat in the docs and tests goes away.
Contributor
Author
There was a problem hiding this comment.
Cool, nice refactoring suggestion fixed both stack overflow and make the events in order
waitForSlot returned a WaitForSlotResult wrapping a std.Io.concurrent future over a heap-allocated WaitState. Replace it with a direct suspend: `waitForSlot(target) Error!void` parks the calling fiber on a stack-local WaitState until dispatch or stop() wakes it. Motivation from the Lodestar TS reference (verified against lodestar checkout 551b8b0a3d): all 9 call sites across api/impl/validator/index.ts either immediately await the promise (8 of 9) or race it against a deadline (the lone Promise.race at index.ts:327, better served here by a sleep arm). None hold the handle to cancel a specific wait later, and TS carries no per-wait cancellation and zero waitForSlot unit coverage. The promise return is a JavaScript artifact, not a semantic requirement, so returning a future in Zig bought a heap alloc + concurrent-fiber bridge for no caller. Consequences and decisions: - The wait is uncancelable: an external fiber-cancel takes effect only once the wait resolves via dispatch or stop(); a wait on a never-started, never-read clock blocks until stop(). This matches TS (no per-wait cancel) and removes the cancel/await ownership contract. - The post-push dispatchWaiters(current_slot) call is deleted. In the legal call space (waitForSlot never invoked from a callback) it is a no-op: the reached-check guarantees the just-pushed target exceeds the cursor, and all other queued targets already exceed it, so it pops nothing. The prior "load-bearing for the property test" claim was folklore; it is replaced by an assert (queue head > cursor) as a partial tripwire for a forbidden callback-call. - The waiter queue's capacity is pre-reserved at init (max_waiters), so the enqueue cannot allocate and the suspend path stays off the error track (reachable errors: Aborted, WaiterLimitReached). - Broadcast-wake (single event, all waiters re-check) was considered and rejected: per-waiter events keep the O(reached) dispatch and avoid thundering-herd re-checks. Tests reworked in place: waiting callers now spawn as zio fibers and the driver rendezvouses on waiters.count() before advancing; the cancel test and the waitForSlot-from-callback test are removed; tests added for the stopped-clock pre-check, the mandatory post-catchUp re-check, and the reached-beats-stop ordering (a stop() from a catchUp callback at the target slot still resolves the wait with success, synchronously).
A listener that queries the clock (currentSlot/currentEpoch/disparity) while the cache lags the wall would, per query, recurse a fresh advanceAndDispatch: each backlogged slot emitted one level deeper. A deep-enough backlog (a long stall, SIGSTOP, or host suspend) recursed once per slot and faulted the zio fiber stack — an uncatchable stack-overflow fault, unlike V8's catchable RangeError. The TypeScript clock carries the same latent recursion; it is only test-gated in sync.ts, never exercised in production. Replace the recursion with a guard + iterative drain: a reentrant query records the furthest wall target in pending_target (coalesced via @max so an NTP step-back cannot regress it) and returns; the owning frame drains the pending target after the current emit finishes. Stack is O(1) regardless of backlog. The three emit/dispatch helpers assert `dispatching` to pin that they only run inside the owning frame. This also fixes event delivery order. Under the old nesting, later slots reached listeners before the in-flight slot's remaining listeners — TS shows the same interleaving, but that ordering is an untested accident of its getter recursing mid-emit, not a contract. The drain delivers every (listener, event) exactly once, in order. Behavior split, deliberately: the value a mid-emit query RETURNS still names the wall slot (matching TS's getter, which gossip validation and waitForSlot rely on). Only delivery TIMING diverges — TS nests delivery inside the getter; we defer the remaining events until the callback returns. Fold in the accessor cleanup: catchUp returns the Reading { now_ms, slot } it already computes, and each accessor derives from that single reading instead of a second, possibly skewed clock read. slot_math is left untouched. Road not taken: dispatching only from the loop fiber (queries would just read, never emit). Rejected — it breaks the getter/event coherence contract that gossip validation and waitForSlot depend on. Tests pin the drain from both sides: a 32k-slot backlog with a query per callback (flat-loop stack), a mid-emit query's run-ahead reading with trailing in-order delivery, three mid-emit queries whose targets rise then step back (@max must neither keep the first target nor take the last), a query-then-stop callback whose recorded pending must not outlive the dispatch frame (the exit backstop), and a stop-mid-drain wait proving the reached-check consults the cursor rather than the run-ahead reading. Addresses PR #463 review comments r3549917709 and r3549490564.
The slot_math header called its non-arithmetic half "policy helpers" — a coinage nobody outside the original design discussion could decode. Name the concrete things instead: gossip-disparity/tolerance helpers. Trim the slotWithGossipDisparity doc to the reviewer's suggested form: the single-snapshot paragraph restated what a pure function of now_ms already guarantees (there is no second time source in scope), the spec formula and spelled-out boundary case are one-step re-derivable from the kept strict-`<` anchor, and the unsupported-config sentence was the contrapositive of the kept assumption. The cross-reference on isCurrentSlotGivenGossipDisparity drops its dangling single-snapshot mention. Drop the AdvanceIterator capability comment: the four-field struct cannot reach listeners, waiters, or io by construction, and no reader would hand an iterator dispatch capability unprompted — a regression guard is not a reading aid. Addresses PR #463 review comments r3551839944, r3551971005, and r3551978245.
Member
There was a problem hiding this comment.
i'm concerned with the amount of doc comment and test noise that is generated, a lot of it is verbose and using terms that are not really obvious at first glance, we should strive to review the generated doc comments as much as possible to optimize for human readability
i'm being a bit of a stickler about this because it's easy to generate tons of comments and for others to give it an approval since technically it doesn't affect code quality, but imo after a few more of such PRs it makes it hard to maintain and understand a codebase
| } | ||
|
|
||
| test "gossip disparity: exact threshold (500ms) applies inclusively" { | ||
| // 111_500 is 500 ms before slot 1's 112_000 start — inclusive edge; 111_499 is 1 ms past. |
Member
There was a problem hiding this comment.
imo we need more similar comments or var names similar to this for all the other tests so it's less like magic numbers, otherwise the next reader (me) might have a hard time understanding why these numbers were used
Comment on lines
+111
to
+124
| /// Returns the slot the network may be advancing to, accounting for gossip | ||
| /// clock disparity, or null pre-genesis when no slot is current yet. | ||
| /// | ||
| /// Base slot and disparity window come from the same `now_ms`, so they can't | ||
| /// disagree — a fresh read could sit past the boundary being compared. | ||
| /// | ||
| /// Per phase0/p2p-interface.md, gossip validation rejects future messages with | ||
| /// strict `<` (`current_time + MAXIMUM_GOSSIP_CLOCK_DISPARITY < message_time`), | ||
| /// so the boundary case (exactly equal) is accepted — hence `<=` here. | ||
| /// | ||
| /// Assumes the disparity window reaches at most the adjacent slot — true | ||
| /// for every real config (500 ms disparity vs seconds-long slots). A | ||
| /// config where disparity approaches or exceeds the slot duration is not | ||
| /// supported. |
Member
There was a problem hiding this comment.
this can really be a lot more concise with the same meaning
Suggested change
| /// Returns the slot the network may be advancing to, accounting for gossip | |
| /// clock disparity, or null pre-genesis when no slot is current yet. | |
| /// | |
| /// Base slot and disparity window come from the same `now_ms`, so they can't | |
| /// disagree — a fresh read could sit past the boundary being compared. | |
| /// | |
| /// Per phase0/p2p-interface.md, gossip validation rejects future messages with | |
| /// strict `<` (`current_time + MAXIMUM_GOSSIP_CLOCK_DISPARITY < message_time`), | |
| /// so the boundary case (exactly equal) is accepted — hence `<=` here. | |
| /// | |
| /// Assumes the disparity window reaches at most the adjacent slot — true | |
| /// for every real config (500 ms disparity vs seconds-long slots). A | |
| /// config where disparity approaches or exceeds the slot duration is not | |
| /// supported. | |
| /// Returns the slot the network may be advancing to, accounting for gossip | |
| /// clock disparity, or null pre-genesis when no slot is current yet. | |
| /// | |
| /// Per phase0/p2p-interface.md, gossip validation rejects future messages with | |
| /// strict `<`, hence `<=` here. | |
| /// | |
| /// Assumes the disparity window reaches at most the adjacent slot — true | |
| /// for every real config (500 ms disparity vs seconds-long slots). |
Contributor
Author
There was a problem hiding this comment.
/// Per phase0/p2p-interface.md, gossip validation rejects future messages with
/// strict `<` (`current_time + MAXIMUM_GOSSIP_CLOCK_DISPARITY < message_time`),
/// so the boundary case (exactly equal) is accepted — hence `<=` here.
This is generated by @lodekeeper 😂 in ts side
Contributor
There was a problem hiding this comment.
Guilty 😄 — that's ported from the TS slotWithGossipDisparity doc comment. @spiral-ladder's trim above is the right call: I over-explained the <=/boundary and adjacent-slot caveat on the TS side, and the port shouldn't inherit that verbosity. Please trim it freely.
Member
There was a problem hiding this comment.
@lodekeeper please open a PR to trim this on lodestar ts side
Contributor
There was a problem hiding this comment.
Done — ChainSafe/lodestar#9631 trims both TS-side comments (currentSlotWithGossipDisparity + isCurrentSlotGivenGossipDisparity) down to the spec pointer.
Comment on lines
+433
to
+435
| // Holds only what advancing needs — config and the slot cursor — so the | ||
| // iterator cannot dispatch (no listeners, waiters, or io in reach). | ||
| const AdvanceIterator = struct { |
Member
There was a problem hiding this comment.
iterators are not meant to dispatch anyway, so i think this comment is kind of redundant
The disparity/tolerance/fromSlot tests read as a bank of six-digit literals (111_499, 112_300, ...) whose meaning lived in the reader's head-arithmetic against test_cfg. Derive every input from test_genesis_ms / test_disparity_ms / test_slot_1_start_ms (matching the named-anchor style the mainnet and fork-aware tests already use), so each value states its own distance from the boundary it probes. The framing comments the derivations replace are dropped. The saturation case becomes the minimal underflow (tolerance = now + 1), pinning the trap boundary exactly; the old inputs' 10s excess was not load-bearing. Every other old literal is reproduced exactly by its named derivation (verified input-by-input). Addresses PR #463 review comment r3551947057.
Full-module comment pass judged from a zero-context reader's seat, both directions. De-jargon: "draining frame" / "owning frame" / "the guard" named nothing in the code — replaced with concrete phrasing (the dispatch already on the stack; the frame that set `dispatching`; the `pending_target == null` assert). root.zig's header narrated Clock's internals with the undefined "slot cursor"; the surface list carries everything the re-export file needs. Accuracy: the degenerate-config test comment cited a `slotAtMs orelse 0` shape that lives in slotWithPastToleranceMs, not in the functions under test — reworded in scenario terms. The waitForSlot tripwire is "best-effort": the illegal push itself always passes the check (the reached-check guarantees target > cursor), so only a coincidentally ripe queue head trips it. Trims: runAutoLoop's exit narration to the one non-obvious fact (join() stops before cancelling); the waiter-reserve rationale to one sentence; two test walkthroughs to mechanism only (the assertions carry the outcomes); duplicated pre-genesis comment now stated once with a cross-ref; what-comment labels and a redundant idempotence clause deleted. Additions (under-explanation is a finding too): why listener IDs start at 1 (0 stays usable as an unset sentinel); max_waiters and max_duration_transitions labeled as headroom bets, not derived limits. Comment-only diff; 63/63 tests unchanged.
The limit comments added in the comment sweep labeled the constants
with derivation meta-language ("headroom bet, not a derived limit") —
vocabulary from the review process, not something a reader of this
code needs or would write. Say it plainly: a generous cap, and what
it bounds.
Contributor
Author
Yes I agree that, actually I had a specific skill and step to handle code comments in each loop. But probably still need edit by hand |
Contributor
|
Owning this one — a lot of the verbose doc comments and test scaffolding here were ported from my TS side, and terms like "policy helpers" or the bare-number tests read okay with TS context but are opaque standing alone. Please don't carry the TS verbosity into the Zig port: trim aggressively, rename unclear terms, and add named constants where tests use magic numbers. I'll keep the TS-side comments leaner too. Happy to propose concise rewrites for any specific comment if that helps. |
Member
|
@lodekeeper <3
|
Adopt the review-suggested shape for the reentrancy machinery: the guard, the pending-target coalescing, and the drain loop move from advanceAndDispatch into catchUp, where reentrant entry actually happens. The walk that remains is renamed dispatchTo; it asserts `dispatching`, so every dispatch provably flows through the guarded frame. Semantics are unchanged: delivery order, @max coalescing, the exit backstop, stop suppression, and waitForSlot behavior all keep their pinning tests. Driving events through the public read path required a steerable clock that can still park fibers: FakeClockIo gains an optional `inner` io whose two futex entries forward to a real zio runtime, while every other entry still crashes, preserving the no-suspension tripwire for synchronous tests. The nine tests that drove advanceAndDispatch directly now steer fake time and query instead. A new test pins the wall-step-back path: the returned slot follows the wall down, the cursor holds, nothing re-emits. catchUp returns WallTime { now_ms, slot }, the single wall reading accessors must derive from. The type name and the doc vocabulary say "wall" so the value cannot be confused with the delivery cursor (current_slot), which waitForSlot's reached-check deliberately consults instead: a stop during the drain suppresses events past the cursor, and a wait for a suppressed slot must abort, not resolve. join() drops the await after future.cancel: std.Io documents cancel as "equivalent to `await` but places a cancelation request", and it consumes the future, so the await was a no-op since birth. It entered in cc5301f as a belt alongside the two real fixes for the April join hang (stop-before-cancel and break-on-Canceled); the original clock.zig repo (PR #2, future-cancel-linux) and the initial port were cancel-only. The cancel-only join was soaked 25x on macOS and 30x on Linux with no hang. init's parameters reorder to (allocator, io, config): environment handles first, data last. Comments across the module were reworked against a cold-reader standard. Each fact now has one home: the module header carries the reentrancy contract with a timeline example, catchUp's doc carries the return-value guarantees and their two exceptions, and each call site carries its local why. Assert comments state the rule they enforce rather than the derivation. A max-effort multi-agent review over the refactor found no production defects; its five test and doc findings are folded in here. Addresses PR #463 review threads r3549917709 and r3549490564.
Rename the six test names and the comments that still said "reading" to the wall-time vocabulary the code uses. Add the missing positive assertion to the immediate-resolution test: a reached target resolves without ever enqueuing a waiter.
Overlap audit results, applied with zero coverage loss (every distinct mutant keeps a killer): - the backlog query-from-callback test folds into the run-ahead interleave test whose event log subsumes its assertions; - the lifecycle smoke and the two real-time slot tests merge into one auto-loop test asserting timing, ordering, and depth together; - slot_math's two forward-disparity tests merge into one covering inside, at, and past the threshold; - QueryThenStopCtx becomes a second named callback on QueryAtSlotCtx. New test: init's only allocation failing yields a clean error.OutOfMemory with nothing leaked. After init the module is allocation-free by design, so this closes the module's entire OOM surface. Closing full-module review fixes: - runAutoLoop sleeps on .boot again. The deadline is anchored to the wall (the next slot boundary), so suspended host time must count toward it and the loop must wake promptly on resume. zio currently folds .awake and .boot into one monotonic clock, so this restores intent first fixed in ccad20d and regressed in a later rewrite. - The sleep retry branch is deleted: the error set holds only error.Canceled, so the retry was unreachable. - start() uses try: the concurrent error set already matches ours. - slot_math: the four undocumented public helpers get one-line docs, restoring the past-tolerance clamp contract lost in an earlier trim, and the module header states the validate() precondition once. Comments across the module are ASCII-only now: em-dashes, en-dashes, arrows, and the multiplication sign replaced with typeable equivalents.
Replace the '>= not ==' meta-phrasing with the reason itself: the wait spans two boundaries so two slots arrive, the 3 s budget is two 1 s boundaries plus scheduler headroom.
Audit of all 60 tests against the spec-vs-implementation line, applied with zero mutant-coverage loss: - Internal-state peeks (clock.current_slot, clock.stopped) are gone from eight tests whose observable assertions already carry the same mutants, and replaced by observable assertions in two more: the step-back test now drives the wall forward and asserts delivery continues 4, 5 without re-emission, and the pure-reads test follows up with a read that delivers the intact backlog. - Two advanceTo iterator tests were subsumed by delivery-level tests and are deleted; the other two carried unique coverage and are converted to observable delivery tests: first delivery from a pre-genesis start begins at slot 0, and an epoch arrives after its boundary slot and before the next slot (pinned through one tagged log fed by both listeners - previously only the private iterator pinned that ordering). - The two remaining white-box touches carry one-line justifications: WaiterLimitReached's queue fill is a setup shortcut sparing 1024 real fibers, and the fast-path count peek guards a waiter that would dangle past its stack frame. - Five names dropped internal vocabulary (cursor, pending target) for contract language. 59 tests, down from 61 by the two subsumed deletions.
…white-box test The dummy-fill shortcut existed to spare max_waiters real fibers, but measurement voids the justification: 1024 concurrently suspended waitForSlot fibers leave MaxRSS unchanged at 34M (stacks are lazily committed) and the runtime inside the suite's noise band. Fill the queue through the public API instead: spawn the fibers, rendezvous, assert the (limit+1)th call rejects, then stop() and reap all 1024. With the private-WaitState dependency gone, both limit tests move to clock_test.zig beside the waiter battery. Clock.zig keeps only the four sanity tests; the suite now contains no white-box test at all.
Three moves that finish the suite's layering: - realtime_test.zig takes the four tests whose subject is the auto loop under real zio scheduling and real wall time (assertions are bounds there, not exact values). They are the only tests that prove the clock ticks by itself, that join() cancels a genuinely sleeping fiber, and that a zio or std.Io regression would be noticed at all; everything else runs on fake time, which is also how the Lodestar TS suite tests this clock (vi.useFakeTimers throughout). - Four incidentally-real-io tests (fast-path resolve, stopped-clock reject, listener limit, stop/join idempotence) are synchronous and time-independent, so they drop their zio runtime for FakeClockIo and gain the no-suspension tripwire. - The offSlot/offEpoch test now removes listeners mid-stream: both event kinds provably flow before removal (slots 0..4, epoch 1) and stay frozen after it while the read advances on - removal stopping an active stream, not an unused registration. - Every wall-time input across clock_test.zig, property_test.zig and the Clock.zig sanity tests is now derived from its test's config (slot_math.slotStartMs(cfg, n), genesis offsets, slot-duration multiples) instead of a bare millisecond literal; mid-slot points keep their meaningful remainder (slotStartMs + 300). Exact-value assertions make the passing suite the value-preservation proof.
Zig callbacks need a context struct, but a single-use one can live in the test block instead of the file's helper inventory. Drop the record indirection and the bounds guard while at it: overflowing the 8-entry log in a 4-event test should panic, not truncate.
Register the listener before the pre-genesis read so the 'nothing is delivered' claim has an observer, then assert the trace is still empty alongside the null return.
Move the type to the declaration and initialize with .{} across the
clock sources and tests (81 sites). The six ClockConfig temporaries
inside expectError keep their explicit type: an inline literal that is
immediately method-called has no declaration to infer from.
The scenario tests were driven by contexts invented to kill mutants (QueryAtSlotCtx, SlowCallbackCtx, MutateAndQueryCtx, ...). Rebuild them around fixtures shaped like the services that will actually drive this clock, so each test reads as something a running node does: - ForkChoiceTicker - a synchronous tick that never reads the clock. - AttnetsService - a slot tick that calls a pure read (secFromSlot) and an epoch tick that reads the current epoch. - SyncService - an epoch tick that recomputes sync state by reading the clock from inside its own callback. - SaturatedChainListener - a tick whose work runs past the slot boundary, so the wall has moved when it returns. - ForkChoiceFailure - an irrecoverable error that shuts the node down from inside the tick. - PrepareNextSlot - a synchronous prefix plus a yield-free handoff to a worker fiber, the shape any listener needs when its work must await. New test: work that must await runs on its own fiber. A callback may not await (it runs on the emitting fiber's stack, so suspending it stalls the remaining listeners and the whole drain) and may not spawn (task registration can reschedule-yield). The listener hands the slot to a pre-spawned worker and returns; the test proves the other listeners and the drain keep going while that worker sleeps. The shutdown-from-a-tick test now unsubscribes the OTHER services, the shape a synchronous shutdown takes - which is also what pins the per-emit snapshot: a service removed mid-emit still receives the slot in flight, and no one receives it twice. Deleted for having no production trigger: the 400 ms sub-disparity config test (the function documents that it assumes the disparity window reaches at most the adjacent slot, and no preset is below 5 s); the past-tolerance saturation arm (now_ms is a Unix-ms epoch and callers pass sub-second tolerances, so the underflow is unreachable - the saturating operator stays as cheap insurance and slot_math's header no longer claims otherwise); and the mid-emit epoch-listener mutation test (no consumer mutates listeners from a callback on either axis). Clock.zig's callback-safety list now states the await and spawn bans and names the two shapes that work: a worker woken by a yield-free handoff, or a fiber that loops on waitForSlot. 56 tests.
Every consumer-shaped fixture now carries a commit-pinned permalink to the Lodestar code it stands in for, so a reviewer can compare the model against the original directly. Line numbers verified against the pinned commit.
The saturated-tick fixture is not one consumer's shape - any listener whose work runs past the slot boundary produces it. Drop the Chain from its name and say so; its permalink already points at the phenomenon (the clock's own saturation comment) rather than a caller.
The framing comment now states only the scenario; the epoch-before- slot-5 ordering that the assertion pins is visible in the expected slice.
nazarhussain
previously approved these changes
Jul 13, 2026
Contributor
Author
|
@nazarhussain @spiral-ladder eliminate the magic number in the tests and remove some useless tests |
nazarhussain
approved these changes
Jul 14, 2026
Merged
nflaig
pushed a commit
to ChainSafe/lodestar
that referenced
this pull request
Aug 6, 2026
## Summary Trims two over-explained gossip-disparity doc comments in `packages/beacon-node/src/util/clock.ts` down to the essential spec reference: - `Clock.currentSlotWithGossipDisparity` getter - `Clock.isCurrentSlotGivenGossipDisparity` Both restated the `<=` vs strict-`<` boundary at length. The trimmed form keeps the spec pointer (`phase0/p2p-interface.md` rejects future messages with strict `<`, hence `<=`) and drops the redundant re-explanation. ## Motivation These comments were ported verbatim into the lodestar-z Zig clock ([ChainSafe/lodestar-z#463](ChainSafe/lodestar-z#463)), where reviewers flagged them as verbose/noisy. Trimming the TS source keeps future ports lean. Comment-only change — no behavior change.
wemeetagain
pushed a commit
that referenced
this pull request
Aug 19, 2026
🤖 I have created a release *beep* *boop* --- ## [1.0.0](v0.1.2...v1.0.0) (2026-08-19) ### Features * add `state.getBuildersLength()` binding ([#472](#472)) ([be2b5ab](be2b5ab)) * **beacon-node:** add block state cache and checkpoint datastore ([#452](#452)) ([2145faa](2145faa)) * bindings to `getExpectedWithdrawals` and native tweaks ([#350](#350)) ([f47bc66](f47bc66)) * **bindings:** add pubkey cache syncPubkeys ([#537](#537)) ([542779f](542779f)) * **bindings:** aggregate cached public keys by validator index ([#397](#397)) ([2f90603](2f90603)) * **bindings:** align `BeaconStateView` with `IBeaconStateView` ([#347](#347)) ([b8ec273](b8ec273)) * **bindings:** configurable pubkey cache growth step ([#481](#481)) ([133ef24](133ef24)) * **bindings:** expose more APIs for STF ([#444](#444)) ([7fe2609](7fe2609)) * **bls:** add small MSM for npoints < 32 ([#393](#393)) ([b430638](b430638)) * **blst:** use external buffers for blst operations ([#358](#358)) ([78e4678](78e4678)) * **ci:** conditionally publish bindings with tag ([#355](#355)) ([ea77919](ea77919)) * **clock:** add clock module for slot/epoch timing ([#354](#354)) ([385b077](385b077)) * **fork_choice:** add Prometheus metrics module ([#309](#309)) ([cbc9d8d](cbc9d8d)) * **forkchoice:** implement the forkchoice module ([#246](#246)) ([7c62a9b](7c62a9b)) * getSyncCommitteesWitness ([#367](#367)) ([ef77649](ef77649)) * implement `loadState` API and binding ([#165](#165)) ([f903519](f903519)), closes [#159](#159) * **metrics:** metrics bindings ([#455](#455)) ([dd41999](dd41999)) * migrate blst,pubkeys to use zapi js dsl ([#331](#331)) ([fcd26ca](fcd26ca)) * **pubkeys:** add getPubkeyBytes binding ([#555](#555)) ([4ca51cf](4ca51cf)) * publish ARM64 musl bindings ([#482](#482)) ([ac764c9](ac764c9)) * **shuffle:** add swap-or-not shuffling module and binding ([#559](#559)) ([c2db37c](c2db37c)) * split nextValue fn ([#464](#464)) ([b47faeb](b47faeb)) * support getLatestWeakSubjectivityCheckpointEpoch ([#366](#366)) ([dcf3883](dcf3883)) * update fulu deposit processing ([#442](#442)) ([064335c](064335c)) ### Bug Fixes * avoid set ([#484](#484)) ([2e25d97](2e25d97)) * better generation of rand scalar ([#388](#388)) ([74dce77](74dce77)) * **bindings:** accept `dontTransferCache` in processSlots for backward compatibility ([#460](#460)) ([65df5af](65df5af)) * **bindings:** check signature infinity by default ([#509](#509)) ([2f5f281](2f5f281)) * **bindings:** clean up failed async BLS work ([#527](#527)) ([1111b00](1111b00)) * **bindings:** free metrics writer on scrape failure ([#529](#529)) ([4c8d94a](4c8d94a)) * **bindings:** harden random aggregate scalars ([#528](#528)) ([8e89a63](8e89a63)) * **bindings:** log level for missing fields ([#435](#435)) ([08faf41](08faf41)) * **bindings:** misordering of print for cpu count ([#381](#381)) ([752a972](752a972)) * **bindings:** populate epoch participation for test fixtures ([#436](#436)) ([8dbdd2e](8dbdd2e)) * **bindings:** refcount Pool to fix teardown panic ([#352](#352)) ([23b2f68](23b2f68)) * **bindings:** roll back partial N-API initialization ([#491](#491)) ([31c5ebb](31c5ebb)) * **bindings:** size BLS thread pool by cgroup-aware CPU count ([#386](#386)) ([3ae9522](3ae9522)) * **bindings:** validate class types before unwrap ([#514](#514)) ([2fd2ad5](2fd2ad5)) * **bindings:** validate secret key hex length ([#517](#517)) ([136e415](136e415)) * **bls:** align PublicKey.uncompress validation with Signature.uncompress ([#508](#508)) ([5a8dbe9](5a8dbe9)) * **bls:** bound randomized aggregation inputs ([#548](#548)) ([779d0bf](779d0bf)), closes [#542](#542) * **bls:** clean up partial thread pool initialization ([#490](#490)) ([d55e598](d55e598)) * **bls:** convert pippenger scratch bytes to element counts ([#513](#513)) ([a12ca92](a12ca92)) * **bls:** enforce 32-byte signing roots ([#545](#545)) ([72fd308](72fd308)) * **bls:** make batch cardinality structural ([#547](#547)) ([a06d8b2](a06d8b2)) * **bls:** preserve aggregate outputs on failure ([#521](#521)) ([e0b6dd1](e0b6dd1)) * **bls:** reject empty keygen salts ([#524](#524)) ([d2a9c86](d2a9c86)) * **bls:** reject unknown BLST error codes ([#525](#525)) ([9e4a6ad](9e4a6ad)) * **bls:** size pairing buffers for 32-bit targets ([#531](#531)) ([dc64a27](dc64a27)) * **blst:** default signature infinity check to true if not provided ([#387](#387)) ([021cdcb](021cdcb)) * **build:** remove `zig-out` from `files` ([#360](#360)) ([c52af09](c52af09)) * **ci:** fix caching spec test version ([#439](#439)) ([96885a1](96885a1)) * dangling state pointer in loadOtherState ([#450](#450)) ([81cbd5f](81cbd5f)) * **epoch_cache:** compute missing `next_proposers` ([#447](#447)) ([0088a29](0088a29)) * **epoch_cache:** populate decision roots in afterProcessEpoch ([#453](#453)) ([4b70a5e](4b70a5e)) * export asyncAggregateWithRandomness through napi binding ([#371](#371)) ([1d04c2b](1d04c2b)) * harden memory safety across PMT, SSZ tree views, and state transition ([#377](#377)) ([d6f5897](d6f5897)) * improve atomic ordering in ThreadPool and NAPI init ([#310](#310)) ([4b0a1cc](4b0a1cc)) * interface compatbility with NativeBeaconStateView ([#445](#445)) ([89e13d1](89e13d1)) * missing deinits in loadOtherState ([#459](#459)) ([094d278](094d278)) * missing state commits ([#454](#454)) ([a432b55](a432b55)) * no-op when syncPubkeys run on a pk cache with shrinking validator set ([#432](#432)) ([ed05a99](ed05a99)) * param order in BeaconBlockBody ([#348](#348)) ([d8b9c06](d8b9c06)) * pendingConsolidations bindings ([#449](#449)) ([b9c497e](b9c497e)) * **pmt,ssz:** harden chunked-leaf and zero-copy tree-view memory safety ([#400](#400)) ([de50c53](de50c53)) * populate cache balances during rewards/penalties processing ([#474](#474)) ([5bf23dc](5bf23dc)) * re-expose sizes ([#369](#369)) ([64b81f3](64b81f3)) * remove `slashValidator` gating on active status ([#448](#448)) ([d319a0d](d319a0d)) * **ssz:** drop redundant default-init pass in fixed-list decode ([#468](#468)) ([0c757be](0c757be)) * **ssz:** publish child cache entries after lookup ([#565](#565)) ([21e78c9](21e78c9)) * state transition binding exports ([#456](#456)) ([895982c](895982c)) * **state-transition:** group-check signature sets ([#515](#515)) ([42774e9](42774e9)), closes [#502](#502) * **state-transition:** isolate epoch step cache mutations ([#535](#535)) ([a83741a](a83741a)) * **state-transition:** repair Pool.init call broken by [#346](https://github.com/ChainSafe/lodestar-z/issues/346)×[#367](https://github.com/ChainSafe/lodestar-z/issues/367) merge skew ([#394](#394)) ([b42944f](b42944f)) * various fixes around config ([#433](#433)) ([c4f082c](c4f082c)) ### Performance Improvements * **bindings:** drop TS BLS comparison benches and report benchmarks on PRs ([#552](#552)) ([c909c6f](c909c6f)) * **bls:** add cache-aware signature verifier ([#562](#562)) ([063857e](063857e)) * **bls:** bypass worker queue for small batches ([#553](#553)) ([3f8a6df](3f8a6df)) * **epoch:** replace AutoHashMap with array lookup in reward/penalty caches ([#286](#286)) ([e4e181b](e4e181b)), closes [#243](#243) * **pmt:** chunked-leaf packing for basic lists and container_struct ([#346](#346)) ([ba156c4](ba156c4)) ### Code Refactoring * allocate `AsyncAggRandData` in one obj ([#384](#384)) ([459750f](459750f)) * **bindings/pubkeys:** simplify allocation strategy for aggregate ([#518](#518)) ([b82750f](b82750f)) * **bindings:** rename blst Lifecycle to State ([#516](#516)) ([0a9c179](0a9c179)) * **bindings:** use zapi js.io() instead of local io module ([#469](#469)) ([2b34cc0](2b34cc0)) * **bindings:** wake only required number of workers ([#383](#383)) ([1db57f1](1db57f1)) * **bls:** allocations around VMAS ([#395](#395)) ([dfda58c](dfda58c)) * **bls:** clean up bls ([#398](#398)) ([e0f3b9b](e0f3b9b)) * **bls:** remove need for tracking results for verifyMultipleAggregateSignatures ([#389](#389)) ([6fe5c3f](6fe5c3f)) * **bls:** remove single-threaded fallback ([#390](#390)) ([e057713](e057713)) * **clock:** single public Clock; internalize SlotClock ([#463](#463)) ([fbab1fa](fbab1fa)) * make XXXDecisionRoot fns return `js.String` ([#342](#342)) ([aef4420](aef4420)) * move shuffle into swap_or_not_shuffle module ([#558](#558)) ([e56efb2](e56efb2)) * **pubkeys:** centralize the process-wide cache ([#522](#522)) ([dc9669d](dc9669d)) ### Miscellaneous Chores * avoid slow tests in AGENTS.md ([#546](#546)) ([c60f2a9](c60f2a9)) * bump zapi to include musl build ([#485](#485)) ([0b488cc](0b488cc)) * **ci:** pin github actions with sha hashes ([#507](#507)) ([167b8f5](167b8f5)) * deprecate unused blst APIs ([#575](#575)) ([7b547fa](7b547fa)) * **deps:** bump zapi v2.1.0 -> v2.2.0 ([#376](#376)) ([0c240d8](0c240d8)) * **deps:** bump zbuild ([#403](#403)) ([e2545de](e2545de)) * **deps:** compile blst with ReleaseFast ([#391](#391)) ([753a896](753a896)) * **deps:** update zapi to 3.1.0 ([#483](#483)) ([f3e5827](f3e5827)) * **deps:** use zapi v2.1.0 ([#372](#372)) ([88f403a](88f403a)) * disable gemini auto code review ([#382](#382)) ([63e42a4](63e42a4)), closes [#380](#380) * **docs:** add comments section in AGENTS.md ([#566](#566)) ([0c09750](0c09750)) * move state clones out of benchmark run functions ([#324](#324)) ([e4035de](e4035de)) * prepare 1.0.0 release ([#576](#576)) ([20b657b](20b657b)) * release v0.1.2-rc.3 ([#370](#370)) ([e4fc551](e4fc551)) * **release:** 0.1.2-rc.2 ([#365](#365)) ([7046128](7046128)) * **release:** v0.1.2-rc.10 ([#477](#477)) ([9a4fad5](9a4fad5)) * **release:** v0.1.2-rc.4 ([#373](#373)) ([09468f1](09468f1)) * **release:** v0.1.2-rc.5 ([#374](#374)) ([f344efa](f344efa)) * **release:** v0.1.2-rc.6 ([#375](#375)) ([bdf5b67](bdf5b67)) * **release:** v0.1.2-rc.8 ([#401](#401)) ([06f91c2](06f91c2)) * **release:** v0.1.2-rc.9 ([#404](#404)) ([6024800](6024800)) * remove merge transition code ([#359](#359)) ([09b175d](09b175d)) * remove stale epoch cache TODOs ([#534](#534)) ([27a547a](27a547a)) * rename era shortHistoricalRoot to shortEraRoot ([#473](#473)) ([c75a4d3](c75a4d3)) * **scripts:** build bindings with preset ([#434](#434)) ([a1b5ef7](a1b5ef7)) * silence debug log when used in release builds ([#486](#486)) ([c5377d7](c5377d7)) * support dev workflow ([#364](#364)) ([fcb9a78](fcb9a78)) * update gloas types to align with the latest specs ([#431](#431)) ([1f065b5](1f065b5)) * update spec test version to v1.7.0-alpha.11 ([#451](#451)) ([5875660](5875660)) * update spec-test-version: v1.6.0-beta.2 -> v1.7.0-alpha.10 ([#441](#441)) ([f932b1c](f932b1c)) * update zapi to 4.0.0 ([#571](#571)) ([de8e3fd](de8e3fd)) ### Documentation * document security threat model ([#557](#557)) ([e678b87](e678b87)) * more comprehensive AGENTS.md ([#520](#520)) ([c74b386](c74b386)) * **pkix:** document load provenance requirement ([#556](#556)) ([37e0aa2](37e0aa2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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
7 participants
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.
Follow-up to #354 (which added the clock module) and the design feedback in #457.
Consolidates the clock module to a single public
Clock:EventClockis renamed toClock,SlotClockis dissolved into it, and the pure slot reads move intoslot_math.