fix(blockchain): sample tick interval in handle_tick so skipped ticks don't inflate it

[MegaRedHand]

Alternative to #513 (same bug, different approach).

Problem

lean_tick_interval_duration_seconds reported a spurious ~1.6s tail on every proposer slot. It measures wall-clock time between consecutive recorded ticks, recorded inside on_tick after the idempotency guard. A guard-skipped tick records no sample and doesn't advance last_tick_instant. On a proposer slot the block is built during interval 4 of the previous slot, which advances the store clock to the next slot's interval 0, so the real interval-0 tick is skipped — and the following interval-1 tick then measured across two intervals (~1.6s) even though ticks were firing on their 800ms cadence.

Approach

Move the sample to handle_tick, the scheduler-level handler that runs for every tick (including guard-skipped ones). One sample per tick regardless of whether on_tick drops the duty, so a skipped interval no longer inflates the next sample.

Versus #513 (which records from the proposer path after the publish-alignment sleep): this needs no proposer special-casing and covers any skip uniformly. It matches how Zeam records the same metric (at the clock layer, decoupled from duty execution).

Tradeoff: wall-clock-drift duplicate ticks (which the guard also swallows) are now sampled too, adding occasional sub-interval samples. Harmless for a metric meant to surface late ticks; noted in the comment.

Verification

Confirmed on a single-node local devnet (0 gossip imports isolate the proposer path). Behavior is equivalent to #513: the skipped interval-0 tick is now sampled (sample count rises to ~5/slot), no pinned ~1.6s artifact remains, and the large samples track the real (variable) block-build duration rather than a fixed two-interval value. Total recorded time is unchanged (_sum matches #513's run).

[github-actions]

🤖 Kimi Code Review

The change correctly fixes a metrics blind spot where skipped ticks (due to the idempotency guard in on_tick) caused the next sample to span two intervals, creating false spikes in the tick_interval_duration metric.

Consensus & Safety: This is purely instrumentation code with no impact on consensus logic, state transitions, or cryptography. The Instant usage is monotonic and safe.

Correctness: The logic is sound. By moving the observation to handle_tick (lines 1182–1200), you capture the wall-clock interval between scheduler invocations regardless of whether on_tick processes or skips the tick. The #[send_handler] attribute ensures sequential access to last_tick_instant, preventing race conditions.

Minor observations:

• The extensive comment (lines 1183–1192) appropriately explains the subtle timing interaction between the proposer early-build logic and the idempotency guard.
• Consider whether metrics::observe_tick_interval_duration should handle the Duration directly or if you need sub-millisecond precision, though Instant::elapsed() is sufficient for ~800ms interval monitoring.

Verdict: Good fix. No changes required.

---

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

[github-actions]

🤖 Codex Code Review

No findings.

The change in crates/blockchain/src/lib.rs is observability-only: it moves lean_tick_interval_duration_seconds sampling from the on_tick post-idempotency path to the scheduler entrypoint, which correctly accounts for skipped interval-0 ticks after early proposal builds described in crates/blockchain/src/lib.rs. I don’t see any impact on fork choice, attestation validation, justification/finalization, state transition, XMSS handling, SSZ, or memory-safety/error-handling behavior.

Residual risk: there does not appear to be targeted test coverage for the skipped-tick metric path around the idempotency guard in crates/blockchain/src/lib.rs, so if this metric is operationally important, a focused regression test would be worthwhile.

I could not run cargo check -p ethlambda-blockchain here because rustup tried to write under read-only /home/runner/.rustup.

---

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

[greptile-apps]

Greptile Summary

This PR moves tick interval sampling to the scheduler-level tick handler.

• Records lean_tick_interval_duration_seconds in handle_tick.
• Removes the same sampling from on_tick after the idempotency guard.
• Keeps skipped ticks represented in the metric before duty execution.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.
• The moved state only feeds the tick interval histogram.
• Slot processing and tick rescheduling remain on the existing path.

Important Files Changed

Filename	Overview
crates/blockchain/src/lib.rs	Moves tick interval metric observation from guarded duty processing into the outer tick handler.

_{Reviews (1): Last reviewed commit: "fix(blockchain): sample tick interval in..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Review — PR 514

Small, well-scoped fix that moves the lean_tick_interval_duration_seconds sampling point from inside on_tick (after the idempotency guard, crates/blockchain/src/lib.rs:293-299 old code) to handle_tick (crates/blockchain/src/lib.rs:1195-1198), the scheduler-level entry point that runs unconditionally for every fired tick.

Correctness

• The root-cause analysis is accurate: since a proposer's interval-4 duty advances store.time() to the next slot's interval 0, the subsequent real interval-0 tick hits the guard at lib.rs:278 and returns before the old sampling code ever ran — inflating the next sample to ~2 intervals. Sampling in handle_tick instead, before on_tick is invoked, fixes this cleanly since it runs regardless of what on_tick decides to do.
• Verified there's only one call site for observe_tick_interval_duration (metrics.rs:882), so no other code path needs updating.
• The self-rescheduling loop in handle_tick (lib.rs:1208-1220, the issue Tick scheduler silently skips intervals when a duty overruns 800ms (proposer never attests on its own slot) #413 catch-up mechanism) can invoke handle_tick again immediately with ms_to_next_interval = 0 when on_tick overruns the interval boundary. This still produces a meaningful sample (interval between actual handler invocations, including any overrun), not a spurious one — consistent with the metric's purpose of surfacing late/slow ticks.
• First-ever tick: last_tick_instant starts None (lib.rs:165), so no bogus zero/garbage sample is emitted on the initial call — same as before.
• The actor model here is single-threaded per instance (GenServer-style mailbox), so there's no risk of concurrent access to last_tick_instant racing between the read/observe and the write.
• No orphaned comments or dead code left behind at the old call site; the diff context around lib.rs:296-298 reads cleanly after removal.

Documentation

• The new comment at lib.rs:1182-1194 clearly explains both the interval-0 skip case and the acknowledged tradeoff (wall-clock-drift duplicate ticks now also produce a sub-interval sample). This is good — the tradeoff is explicitly called out rather than silently introduced.

Minor / non-blocking observations

• No test accompanies this change (a unit test simulating a guard-skipped tick and asserting the histogram doesn't record an inflated sample would be nice), but given this is a metrics-only fix with no consensus-relevant logic, that's a reasonable simplification aligned with the low blast radius mentioned in the description.
• The PR body notes this was verified manually on a local devnet with concrete before/after histogram behavior — good verification practice for a metrics fix that's otherwise hard to unit test deterministically (depends on scheduler timing).

Overall: correct, minimal, well-reasoned fix with no consensus-logic or state-transition impact (metrics-only). No blocking issues found.

---

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