🤖 Kimi Code Review

Critical Issue: Unawaited Async Operation

Line 278: self.propose_block(slot, validator_id); is not awaited, yet the containing function is async (evidenced by .await on line 319). If propose_block returns a Future or Result, this is a fire-and-forget bug that could cause the block to not be published before attestations are generated, leading to consensus failures.

Verification: Check if propose_block is synchronous (void return). If async, change to self.propose_block(slot, validator_id).await?; and handle the Result.

Consensus Correctness: Interval Ordering

The reordering of interval logic is correct and fixes a subtle sequencing bug:

• Before: Interval 2 (aggregation) was checked before Interval 0 (block proposal)
• After: Follows chronological order: Interval 4 → 0 → 1 → 2 → 3

This ensures aggregation sessions (Interval 2) start after attestations are produced (Interval 1), which is required for meaningful aggregation.

Interval 4 Snapshot Timing

Lines 259-262: The comment correctly identifies the race condition between snapshotting new_payloads and store::on_tick. Ensure that store::on_tick is actually invoked after the snapshot code (line 259: "below") and not elsewhere in the call stack. If on_tick is called inside propose_block, the snapshot would be invalid.

Minor Observations

1. Error Handling: propose_block appears to discard any error results. Consider propagating or logging failures explicitly rather than silent drops.

2. Metrics Consistency: Line 326 (update_safe_target_slot) relies on state updated by store::on_tick at Interval 3. Verify this call ordering remains correct after the refactoring.

Verdict: The structural refactoring improves readability and fixes the temporal ordering of consensus operations. Address the potential unawaited Future before merging.

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

🤖 Codex Code Review

No findings.

The change in crates/blockchain/src/lib.rs looks non-semantic: the interval-4 new_payloads snapshot still stays correctly before store::on_tick, and the interval-2 aggregation path at lib.rs:313 remains mutually exclusive with the interval-0/1 branches, so the reordering does not change fork-choice, attestation handling, justification/finalization, or aggregation behavior.

Residual gap: I could not complete a cargo check -p ethlambda-blockchain in this environment because rustup attempted to write under /home/runner/.rustup, which is read-only here.

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

🤖 Claude Code Review

Here is the review output:

PR Review: refactor(blockchain): group on_tick conditionals by interval

Overview: A pure readability refactor of BlockChainServer::on_tick. The interval-2 aggregation block was previously interleaved before interval-0 and interval-1 blocks; this PR moves it to its natural ascending position (0 → 1 → 2) and adds // ==== interval N ==== section markers. No behavioral change.

Correctness

The reorder is safe. The interval-2 block (start_aggregation_session) and interval-1 block (produce_attestations) are guarded by mutually exclusive interval == 2 / interval == 1 predicates, so swapping their source positions has no runtime effect. The claim "pure reorder, behavior unchanged" holds.

The one genuine ordering constraint — the new_payloads snapshot must precede store::on_tick because the tick drains new_payloads — is correctly preserved and is now documented with an explicit MUST comment (lines 254–258). This is a meaningful improvement over the original implicit ordering.

Minor Issues

1. The // ==== interval 4 (pre-tick) ==== section header implicitly covers store::on_tick

store::on_tick runs on every interval (0–4), but it sits immediately inside the ==== interval 4 (pre-tick) ==== section. A reader scanning section headers top-to-bottom could mistakenly infer that the tick call is interval-4-only. The inline comment says "pre-tick" clearly, but the section label still creates a small mental-model hazard.

A lighter fix would be to separate the header:

// ==== interval 4 (pre-tick snapshot) ====
if interval == 4 && let Some(snapshot) = ... { ... }

// Tick the store (runs every interval) ...
store::on_tick(...);

// ==== interval 0 ====

This isn't blocking, but it removes the one ambiguous grouping in an otherwise clean layout.

2. Metrics at the end run every interval, not just interval 3

update_safe_target_slot and update_head_slot execute unconditionally at the bottom, but they follow the // ==== interval 3 ==== comment, which could suggest they're interval-3-specific. The per-line comments on each call do explain when the underlying values change, so this is fine as-is — just worth noting.

Cross-PR Note

The PR description correctly flags that commit 7474c15 in PR #445 moves the new_payloads snapshot into the post-tick interval-4 group, which would silently break the "timely" cohort in the coverage report (observability-only regression, wouldn't surface in devnet). The note that #445 should adopt this PR's placement when it rebases is accurate and important.

Summary

The refactor achieves its stated goal cleanly. The MUST-comment documenting the snapshot ordering constraint is a net improvement over implicit ordering. The only change worth considering is separating the ==== interval 4 (pre-tick) ==== label from store::on_tick to avoid implying the tick call is interval-4-gated. Everything else looks good.

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

Greptile Summary

This PR reorders the per-interval blocks inside BlockChainServer::on_tick so they appear in ascending slot-timeline order (0 → 1 → 2 → 3), and adds ==== interval N ==== section markers. The behavior is unchanged because every block is already guarded by its own interval == N predicate.

• The interval-4 new_payloads snapshot is intentionally kept ahead of store::on_tick and now has an extended comment explaining the ordering constraint; the PR correctly calls this out as the one action that cannot be moved to its natural position.
• The aggregation block (previously ordered before the interval-0 propose and interval-1 attestation code) is now correctly placed after both, with no functional impact.

Confidence Score: 4/5

Safe to merge — the reorder is purely cosmetic because every block is guarded by its own interval predicate, and the one ordering constraint that matters (interval-4 snapshot before store::on_tick) is correctly preserved and now explicitly documented.

The only actionable finding is that unconditional metrics calls (update_safe_target_slot, update_head_slot, advance_keys_to) land under the // ==== interval 3 ==== banner, which could mislead a future developer into treating them as interval-3-specific. This is a readability concern with no current behavioral impact.

crates/blockchain/src/lib.rs — the tail of on_tick after the interval-2 block mixes unconditional per-tick calls under the interval-3 section label.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant T as on_tick
    participant S as store::on_tick
    participant C as coverage
    participant M as metrics

    Note over T: interval 4 (pre-tick)
    T->>C: "snapshot_new_payloads() [if interval==4]"

    T->>S: store::on_tick(timestamp_ms, has_proposer)
    Note over S: accepts attestations (int 0)<br/>updates safe-target (int 3)<br/>promotes new_payloads (int 4)

    Note over T: interval 0
    T->>T: propose_block() [if proposer_validator_id]

    Note over T: interval 1
    T->>C: "emit_post_block_coverage() [if slot>0]"
    T->>T: produce_attestations()

    Note over T: interval 2
    T->>C: emit_agg_start_new_coverage() [if is_aggregator]
    T->>T: start_aggregation_session()

    Note over T: interval 3
    Note over S: (safe-target already updated inside store::on_tick)

    T->>M: update_safe_target_slot()
    T->>M: update_head_slot()
    T->>T: advance_keys_to(slot+1)

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant T as on_tick
    participant S as store::on_tick
    participant C as coverage
    participant M as metrics

    Note over T: interval 4 (pre-tick)
    T->>C: "snapshot_new_payloads() [if interval==4]"

    T->>S: store::on_tick(timestamp_ms, has_proposer)
    Note over S: accepts attestations (int 0)<br/>updates safe-target (int 3)<br/>promotes new_payloads (int 4)

    Note over T: interval 0
    T->>T: propose_block() [if proposer_validator_id]

    Note over T: interval 1
    T->>C: "emit_post_block_coverage() [if slot>0]"
    T->>T: produce_attestations()

    Note over T: interval 2
    T->>C: emit_agg_start_new_coverage() [if is_aggregator]
    T->>T: start_aggregation_session()

    Note over T: interval 3
    Note over S: (safe-target already updated inside store::on_tick)

    T->>M: update_safe_target_slot()
    T->>M: update_head_slot()
    T->>T: advance_keys_to(slot+1)

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/blockchain/src/lib.rs:325-332
**Unconditional calls grouped under an interval-3 banner**

`metrics::update_safe_target_slot` and `metrics::update_head_slot` run on **every** tick, not just at interval 3. Placing them under `// ==== interval 3 ====` might lead a future developer to assume they are interval-3-gated when refactoring, especially since the head-slot comment explicitly mentions "intervals 0/4". Consider pulling these (and `key_manager.advance_keys_to`) into a trailing `// ==== every tick ====` section so the interval labels stay consistent in meaning.

_{Reviews (1): Last reviewed commit: "refactor(blockchain): group on_tick cond..." | Re-trigger Greptile}