No actionable comments were generated in the recent review. 🎉

sequenceDiagram
    participant Executor
    participant BlockProducer
    participant Timer
    Executor->>Timer: wait for next tick
    Timer-->>Executor: tick
    Executor->>Executor: record startTime
    Executor->>BlockProducer: ProduceBlock(ctx)
    BlockProducer-->>Executor: block produced
    Executor->>Executor: elapsed = now - startTime
    Executor->>Timer: reset(duration = blockTime - elapsed)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Claude finished @chatton's task in 1m 18s —— View job

Code Review

• Read changed files
• Analyze code quality and correctness
• Check tests
• Post review feedback

The fix is correct and well-targeted. The core change in executionLoop properly accounts for block production time when resetting the timer. Here are my observations:

Correctness

The fix in executor.go:418-429 is sound:

start := time.Now()
if err := e.blockProducer.ProduceBlock(e.ctx); err != nil {
    e.logger.Error().Err(err).Msg("failed to produce block")
}
// ...
elapsed := time.Since(start)
remaining := e.config.Node.BlockTime.Duration - elapsed
if remaining <= 0 {
    remaining = 0
}
blockTimer.Reset(remaining)

• Elapsed time is subtracted correctly, even on error paths (we still spent time attempting production).
• Clamping to 0 avoids negative durations.

Minor Issues

1. Lazy mode path is inconsistent — block/internal/executing/executor.go:433-437

The lazyTimerCh case still resets to the full LazyBlockInterval.Duration without accounting for execution time. If lazy mode blocks are expected to have accurate intervals too, this should be addressed consistently. If the semantics intentionally differ ("fire again after a full idle period"), a comment would clarify the intent.

2. Missing edge case test — The new TestExecutionLoop_BlockInterval test only covers the normal case where execution takes less than blockTime. The remaining <= 0 branch (execution time ≥ block time) is not tested, meaning that code path has 0 coverage (which aligns with the codecov report showing 2 missing lines). A test with executionTime >= blockTime would cover this and verify the timer fires immediately without blocking. Fix this →

3. Behavior when execution consistently exceeds block time — When remaining = 0, the timer fires immediately, meaning if block production is always slower than blockTime, the node will produce blocks back-to-back with no pause. This may be the desired "catch up" behavior, but a short comment clarifying this would improve readability:

if remaining <= 0 {
    remaining = 0 // block production exceeded target; fire immediately to catch up
}

Test Quality

The stubBlockProducer is well-implemented:

• Mutex protection on callTimes is correct.
• Uses testing/synctest for fake-time determinism — the interval assertions are exact because time.Now() advances by precise amounts in fake time.
• defer timer.Stop() in ProduceBlock prevents goroutine/timer leaks.
• Context cancellation is respected.

Performance Benchmarks

The benchmark regressions (EVM Contract Roundtrip: 1.68x allocs/op, SpamoorSmoke: ~3x latency) look unrelated to this change — they affect the execution layer, not the timer logic. These are likely environmental noise or pre-existing flakiness rather than a regression introduced here.

Summary

The fix is correct and addresses a real production issue. The two main improvements worth considering are: (1) covering the remaining <= 0 branch in tests, and (2) clarifying the lazy mode path. Neither is a blocker. This is a clean, focused bug fix with good test coverage for the happy path.

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.00%. Comparing base (5a07bc3) to head (45db47b).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3137      +/-   ##
==========================================
+ Coverage   60.82%   61.00%   +0.17%     
==========================================
  Files         113      113              
  Lines       11695    11700       +5     
==========================================
+ Hits         7114     7137      +23     
+ Misses       3777     3753      -24     
- Partials      804      810       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.