fix(metrics): discard block-processing time for duplicate blocks

[MegaRedHand]

Problem

lean_fork_choice_block_processing_time_seconds is sampled by a TimingGuard created at the top of on_block_core, before the idempotent duplicate-block check returns early. A duplicate block does no processing, so it recorded a near-zero sample that skews the histogram low. Duplicates are common during range sync, so the effect is not negligible.

This is the same class of issue as the recently fixed tick-interval metric (#514): the sample boundary didn't line up with the unit of work — there it inflated the tail, here it deflates the percentiles.

Change

• Add TimingGuard::discard() to cancel a measurement before the guard drops (a disarmed flag checked in Drop).
• Call it on the duplicate-block early return in on_block_core.

The block hashing (hash_tree_root) stays inside the timed span since it is real work; only the sample for a no-op duplicate is dropped. All other early returns (validation rejections) still record, as they represent real processing time.

Verification

make fmt, make lint (clean), full workspace build. Two new unit tests in timing.rs cover the guard: records_a_sample_on_drop and discard_suppresses_the_sample (both pass). cargo test -p ethlambda-blockchain 46 unit tests pass. Fork-choice/signature spec tests require downloaded fixtures (absent in this env).

[github-actions]

🤖 Kimi Code Review

The PR is well-crafted and addresses a real metrics accuracy issue. The implementation is correct, safe, and idiomatic.

Minor suggestions:

crates/common/metrics/src/timing.rs

1. Naming consistency (optional): Consider renaming disarmed to discarded to match the method name (discard), or rename the method to disarm(). Current naming is clear enough but consistency reduces cognitive load.

   • Line 16: discarded: bool,

2. Test helper: The Box::leak in unregistered_histogram() is acceptable for test isolation, but add a brief comment acknowledging the intentional leak since it creates unrecoverable memory.

   • Line 44: // Intentionally leak to obtain a 'static reference; acceptable for test scope

crates/blockchain/src/store.rs

3. Variable mutability: The change from let _timing to let mut timing on line 572 is correct and necessary for the discard() call.

Consensus/Security note:
This change is purely instrumentation-related and does not affect consensus-critical paths (fork choice, state transition, or validation). The early return for duplicate blocks (line 580-585) preserves existing idempotency behavior; only the metrics recording is modified.

Summary: No blockers. The PR correctly prevents skewed histogram measurements during range sync while maintaining the existing performance characteristics (single bool check on drop).

---

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

[github-actions]

🤖 Claude Code Review

Everything checks out. This is a clean, minimal, well-tested fix. Here's the review.

Review: fix(metrics): discard block-processing time for duplicate blocks

Overall: Clean, minimal, well-targeted fix. The disarmed flag on TimingGuard is a sound, idiomatic pattern (mirrors the standard "cancel a Drop-based guard" idiom used elsewhere in Rust, e.g. mem::forget-style guards but scoped and safer since the histogram is still dropped normally). Good test coverage for both the normal-record and discard paths. No correctness or safety issues found.

Observations (non-blocking):

1. crates/blockchain/src/store.rs:573 — block_start (a separate, un-timed-guard Instant) is computed unconditionally right after the new timing guard, and is presumably used later (block_total at line 699) for a different metric/log. Worth double-checking that metric/log doesn't also get thrown off by duplicate blocks returning early right after block_start is set — but that's pre-existing behavior untouched by this PR, so out of scope here. Just flagging in case a follow-up wants the same treatment.

2. crates/common/metrics/src/timing.rs:33 — discard(&mut self) requires callers to bind the guard as mut (as done in store.rs:572). That's correct and matches the intent, but note it does mean a future accidental omission of mut at a new call site would be a compile error, not a silent bug — good, that's the desired failure mode.

3. Test naming/style (records_a_sample_on_drop, discard_suppresses_the_sample) and the unregistered_histogram() helper using Box::leak are appropriate for a 'static-borrowing API and avoid polluting the global Prometheus registry — nice touch.

4. Doc comments are clear about why (not just what), consistent with the codebase's stated commenting philosophy (explains the histogram-skew rationale, not just "sets disarmed to true").

No security, correctness, or fork-choice/consensus concerns — this only affects a Prometheus histogram sample, not any state-transition or fork-choice logic. Comment in store.rs correctly notes that other early returns (validation rejections) are intentionally left recording, which is the right call since those represent real processing cost.

---

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

[github-actions]

🤖 Codex Code Review

1. Low: crates/blockchain/src/store.rs changes the semantics of lean_fork_choice_block_processing_time_seconds_count, not just its latency distribution. Duplicate block arrivals are now omitted from the histogram entirely via timing.discard(), so any dashboard or alert using the histogram count as a proxy for inbound block load or duplicate-flood pressure will underreport. If that count is consumed operationally, add a dedicated duplicate-block counter or document the contract change.

No blocking correctness, safety, or consensus-layer issues stood out in this diff. The change is metrics-only, TimingGuard::discard is straightforward and memory-safe, and the new unit tests cover both the normal drop path and the discarded path.

Testing gap: I could not run the Rust tests in this environment because cargo/rustup needed writable home directories and network access to resolve the pinned toolchain/dependencies.

---

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

[greptile-apps]

Greptile Summary

This PR excludes duplicate blocks from the block-processing timing histogram. The main changes are:

• Adds TimingGuard::discard() to suppress an observation before drop.
• Discards the timing sample on the duplicate-block early return.
• Adds tests for normal and discarded guard behavior.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.
• Existing timing behavior remains unchanged unless a caller explicitly discards the sample.
• Other block-validation returns still record their processing time.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Discards the timing sample only when the exact block root already has stored state.
crates/common/metrics/src/timing.rs	Adds an explicit disarmed state to the timing guard and tests both drop paths.

_{Reviews (1): Last reviewed commit: "fix(metrics): discard block-processing t..." | Re-trigger Greptile}