[MegaRedHand]

🗒️ Description / Motivation

Dedicated validation workers, bounded queues or CPU budgets are only worth building if gossip validation actually delays block import or the epoch precompute. Nothing measured that: the validation permit pools had no metric, the existing arrival-to-verdict histogram cannot separate blocking-pool queueing from run time, and the precompute histogram gives a duration but no overlap with import latency.

Instrumentation only: no pool size, scheduling or verdict changes.

What Changed

• crates/net/p2p/src/metrics.rs: new metrics
  • lean_beacon_validation_permits_in_use{pool} and lean_beacon_validation_permit_exhausted_total{pool}
  • lean_beacon_validation_queue_seconds{kind} (spawn_blocking to closure start) and lean_beacon_validation_run_seconds{kind}, for gossip stateful checks and for the fetched column and envelope checks
  • lean_beacon_column_check_permit_wait_seconds{kind}
• crates/blockchain/src/metrics.rs, epoch_precompute.rs: lean_beacon_epoch_precompute_running, 1 while a worker runs
• p2p/src/beacon/{verdict,column_checks,envelope_checks}.rs: observation sites
• docs/metrics.md: each metric, plus the PromQL that answers "does validation compete with import"

Block import already exposes the mailbox wait as lean_block_import_phase_seconds{phase="queue"}, so nothing is added there.

Correctness / Behavior Guarantees

• No behavior change: every verdict, permit count and task placement is as before; the new code only reads clocks and gauges around existing sections.

Tests Added / Run

• cargo fmt --all -- --check: clean
• make lint: clean
• cargo test --profile release-fast -p ethlambda-p2p -p ethlambda-blockchain --lib --bins: p2p 364 passed (1 ignored), blockchain 292 passed (43 ignored)
• Not run locally: make test (spec suites); CI covers them.

Related Issues / PRs

Part of a beacon import perf batch, each PR independent and based on main. Overlap: this PR and the other batch PRs (STF phase timing, column KZG batching, engine/STF overlap) all edit docs/metrics.md; the column KZG batching PR also edits p2p/src/beacon/column_checks.rs.

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — not run locally (spec suites); unit tests of the touched crates pass

[github-actions]

🤖 Codex Code Review

Looks good overall: this PR is observability-only and does not change fork choice, attestation validation, justification/finalization, STF, XMSS, or SSZ logic. I did spot one correctness/readability issue worth fixing.

• crates/net/p2p/src/beacon/envelope_checks.rs:114

  • PoolInUse::new("column_check") is probably intentional because envelopes share column_check_permits, but it is easy to misread alongside BlockingTask::new("envelope_check") on the next line.
  • Suggestion: add a tiny helper like const COLUMN_CHECK_POOL: &str = "column_check"; or reuse a shared function for pool labels, so the pool label and task-kind label cannot be confused in future edits.
  • Reason: this code currently records permit wait under kind="envelope_check", queue/run time under kind="envelope_check", but in-use occupancy under pool="column_check". That is correct per docs, but brittle.

• crates/net/p2p/src/metrics.rs:478

  • VALIDATION_POOLS includes "column_check", while lean_beacon_validation_permit_exhausted_total is documented as gossip-only and never incremented for column_check.
  • Suggestion: either pre-initialize only the pools that can exhaust (gossip, attestation, builder, sync) or explicitly document in code that "column_check" is present only for label prewarming / future-proofing.
  • Reason: as written, dashboards may show a permanent zero-series for pool="column_check" on an “exhausted” metric that can never fire, which is mildly misleading.

• crates/blockchain/src/metrics.rs:130

  • The EpochPrecomputeRunning gauge assumes exclusivity from epoch_precompute_in_flight. If that invariant is ever violated by a future refactor, set(1)/set(0) becomes inaccurate.
  • Suggestion: consider an IntGauge inc/dec guard instead of binary set/unset, or at least reference the invariant closer to the spawn site in crates/blockchain/src/epoch_precompute.rs:223.
  • Reason: not a current bug if the invariant holds, but this kind of metric silently goes wrong under concurrency regressions.

No security concerns stood out from this diff. Performance impact should be negligible: a few metric updates and Instant::now() calls around already-expensive blocking validation paths. Rust patterns are otherwise solid: RAII guards for panic-safe cleanup are idiomatic here.

---

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

[github-actions]

🤖 Claude Code Review

I've reviewed the metrics changes and have no blocking issues. The commit is a single squashed import of the whole repo, so I couldn't diff against a base. I read the touched code directly: p2p/src/metrics.rs, verdict.rs, column_checks.rs, envelope_checks.rs, and the blockchain metrics.rs and epoch_precompute.rs. I haven't built the PR or run its tests.

What looks good

• Instrumentation only. Pool sizes, scheduling and verdicts are unchanged, as the commit message says.
• Panic safety. PoolInUse, BlockingRun and EpochPrecomputeRunning are RAII guards. The gauges fall and the run-time histogram is observed even on a panic unwind. This also holds if spawn_blocking is dropped without running, because the guards are moved into the closure.
• Where the timers start.
  • BlockingTask::new is created immediately before spawn_blocking and .start() is the first call in the closure, so the queue histogram measures only blocking-pool queueing.
  • In verdict.rs, PoolInUse::new is created after try_acquire_owned succeeds, so the exhausted path never inflates the gauge.
  • In column_checks.rs, the permit-wait histogram covers only the acquire_owned().await.
• Cardinality. Labels come from fixed &'static str sets: five pools, and the kinds. VALIDATION_POOLS is used at metrics.rs:453 to pre-initialize series, which keeps the PromQL from reading as absent before the first event.
• EpochPrecomputeRunning. A plain 0/1 gauge is exact because of the epoch_precompute_in_flight single-worker invariant. The comment says so.
• Naming. The lean_ prefix, LazyLock registration and guard idioms match the surrounding code.

Minor, non-blocking

1. EpochPrecomputeRunning (crates/blockchain/src/metrics.rs:135) is pub while the p2p guards are pub(crate). Use pub(crate) unless something outside the crate needs it. Its only use is epoch_precompute.rs:224.
2. The column_check pool tracks PoolInUse after the permit is acquired, so the gauge counts blocking tasks, not tasks waiting on a permit. That matches the help text ("one per blocking task queued or running"). Queued waiters show up only in column_check_permit_wait_seconds, so the docs should say that, so nobody reads a gauge below capacity as "no contention".
3. column_check is both a pool name and a kind value, and envelope_check shares that pool. That is fine, but the docs/metrics.md PromQL should avoid joining the pool and kind labels directly.

Overall this is a clean, low-risk observability change. I'd approve it, with the small cleanups in Item 1 and Point 2 optional.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds metrics for validation contention and epoch precompute monitoring. Let me analyze each file carefully.

crates/net/p2p/src/beacon/envelope_checks.rs

Bug: Incorrect pool label for envelope checks

Line 113: let in_use = metrics::PoolInUse::new("column_check");

This should be "envelope_check" or "column_check" consistently. Looking at the metrics definition, VALIDATION_POOLS includes "column_check" but not "envelope_check". However, the BlockingTask::new("envelope_check") on line 114 uses "envelope_check" as the kind label, while PoolInUse uses "column_check".

The PoolInUse guard counts permits held, but envelope checks and column checks share the same semaphore (column_check_permits). The pool label for PoolInUse should indeed be "column_check" since they share that pool, but this is inconsistent with how the code reads. The comment in metrics.rs line 476-478 clarifies this, but the mismatch between PoolInUse("column_check") and BlockingTask("envelope_check") is confusing.

Actually, re-reading: this is correct but fragile. The PoolInUse tracks the permit pool (column_check), while BlockingTask tracks the task kind (envelope_check). The metrics serve different purposes. However, there's a real issue:

Line 113: The in_use variable is named generically but uses "column_check" — this is correct for the shared pool, but the naming in column_checks.rs uses the same pattern. Consider renaming to pool_in_use for clarity, or adding a comment.

crates/net/p2p/src/beacon/verdict.rs

Logic error in pool_of function — Validated::Envelope mapping

Lines 615-621: The pool_of function maps Validated::Envelope(_) to "gossip", but envelope_checks.rs uses column_check_permits (via check_and_forward_then), not gossip_validation_permits.

Wait — let me re-check. verdict.rs handles gossip validation (via spawn_stateful_checks), while envelope_checks.rs handles fetched envelopes in check_and_forward_then. These are different code paths for different envelope sources.

Looking more carefully at verdict.rs line 615: Validated::Envelope(_) in the gossip path uses "gossip" pool. But envelope_checks.rs is for fetched/replayed envelopes, not gossip. This seems correct — gossip envelopes go through gossip validation permits, fetched envelopes go through column_check_permits.

However, line 615-616: Validated::Block { .. } | Validated::Column(_) | Validated::Envelope(_) => "gossip" — is Envelope actually validated through gossip? Let me check if there's a discrepancy in how envelopes flow.

Actually, looking at the original permits_for function before this PR, Envelope was in the gossip pool. So this preserves existing behavior. The PR just extracts the string matching.

crates/net/p2p/src/metrics.rs

Potential panic safety issue with IntGauge::dec()

Lines 526-533: PoolInUse::drop calls .dec() on the gauge. If there's a bug where dec() is called more times than inc(), the gauge goes negative. Prometheus IntGauge allows negative values, so this is fine mathematically, but semantically it indicates a bug.

More importantly: The Drop implementations are not unwind-safe in terms of ordering. If PoolInUse::drop panics (which it can't here, dec() doesn't panic), but more realistically, if the thread panics after BlockingRun::drop but during another drop, we could have issues. This is fine — Rust drop order is well-defined.

Thread safety concern with LazyLock and register_*! macros

Lines 482-503: The LazyLock initialization calls register_*! macros which use .unwrap(). If registration fails (e.g., duplicate metric name from a previous initialization in tests), this panics. This is pre-existing pattern in the file, but the new metrics follow the same pattern. Consider if tests that call metrics::init() multiple could cause issues — but init() is idempotent for Prometheus counters.

Histogram bucket allocation performance

Lines 505-520: VALIDATION_TASK_BUCKETS.to_vec() allocates a Vec on each LazyLock initialization. This is once per program lifetime, so negligible. But it's unnecessary — prometheus crate accepts Vec<f64>; could use Vec::from(VALIDATION_TASK_BUCKETS) or just pass the slice if the API allows. Check if register_histogram_vec! accepts &[f64] — if not, to_vec() is fine.

Actually, looking at the prometheus crate API, register_histogram_vec! likely takes Vec<f64> for buckets. The .to_vec() is correct.

Missing #[must_use] on guard types

Lines 524, 576, 600: PoolInUse, BlockingTask, BlockingRun, and EpochPrecomputeRunning are guard types meant to be held until scope end. Without #[must_use], they can be accidentally dropped immediately:

PoolInUse::new("gossip"); // immediately dropped, then dec() called!

This is a real bug risk. Add #[must_use] to all guard types:

#[must_use]
pub(crate) struct PoolInUse(&'static str);

Same for EpochPrecomputeRunning in crates/blockchain/src/metrics.rs.

crates/blockchain/src/epoch_precompute.rs

Correctness: Gauge timing relative to actual work

Line 224: let _running = crate::metrics::EpochPrecomputeRunning::start(); is created before fork_choice::advance_to_epoch_start on line 227, but after entering the spawn_blocking closure. This is correct — it measures the actual computation time, not the queue wait.

However, the comment in metrics.rs says "Worker start / end" but the gauge is set inside the closure, not when the worker is spawned. This matches the intended semantics (overlap with other work), but the doc in metrics.md says "1 while an epoch precompute worker runs its advance" which is accurate.

crates/net/p2p/src/beacon/column_checks.rs

Correctness: Permit wait timing includes clone overhead

Lines 49-52:

let waiting_since = std::time::Instant::now();
let Ok(permit) = permits.clone().acquire_owned().await else {

The permits.clone() is an Arc::clone, very cheap. But the timing starts before it, so it includes the clone. Negligible, but for precision, move waiting_since after permits.clone(). Actually, clone() is before await, so it's synchronous and cheap. The timing is fine.

crates/net/p2p/src/metrics.rs — Test issues

Test pollution risk

Lines 838-874: Tests use "test_pool" and "test_kind" labels. These are unregistered in init(), so get_sample_count() may return 0 unexpectedly if the metric vec wasn't initialized with those labels. However, Prometheus HistogramVec and IntGaugeVec create labels lazily on first use, so this works.

But: Tests run concurrently and access global state. The test_pool label avoids collision with real pools, but multiple test threads running a_held_permit_is_counted_until_its_guard_drops would interfere with each other since they all use "test_pool".

The comment says "A pool label no real pool uses, so parallel tests cannot move it" — but parallel tests with the same test would collide. Rust runs tests in the same file concurrently by default. This is a test flake risk.

Use std::thread::current().id() or a UUID for the test label, or use #[serial] from serial_test crate, or use a test-specific module approach.

Actually, re-reading: the tests are in mod tests which is #[cfg(test)]. The comment claims parallel tests cannot move it, but they can — two threads both incrementing "test_pool" would see 2, then one drops and sees 1, but expected 0.

Wait — the test creates first and second, checks gauge() == 2, drops first, checks == 1, drops second, checks == 0. If another test is also manipulating "test_pool", this fails.

Fix: Use a unique label per test invocation. Since there's no easy way to do this statically, use std::sync::atomic::AtomicUsize or thread-local, or better, use a Mutex around the test section, or use distinct labels with a random/sequential component.

Simplest fix for a_held_permit_is_counted_until_its_guard_drops:

use std::sync::atomic::{AtomicUsize, Ordering};
static TEST_COUNTER: AtomicUsize = AtomicUsize::new(0);

let test_id = format!("test_pool_{}", TEST_COUNTER.fetch_add(1, Ordering::SeqCst));
// ... use &test_id

But PoolInUse::new takes &'static str, so String won't work. You'd need to leak the string: Box::leak(test_id.into_boxed_str()).

Alternatively, use a std::sync::Mutex in the test to serialize access to "test_pool".

For a_blocking_task_observes_queue_delay_and_run_time_once, the get_sample_count() is monotonically increasing, so concurrent tests would see higher counts. Use a unique kind label there too.

Documentation

docs/metrics.md — PromQL query correctness

Lines 482-486:

sum(lean_beacon_validation_permits_in_use)
histogram_quantile(0.99, sum by (le) (rate(lean_beacon_validation_queue_seconds_bucket[5m])))
sum by (pool) (rate(lean_beacon_validation_permit_exhausted_total[5m]))

This is three separate expressions. In Grafana/promql, this would need to be separate panels or combined with or (which doesn't make sense for different units). The doc says "Plot these on one panel" — but you can't plot a gauge (count), a histogram quantile (seconds), and a rate (count/sec) on the same Y-axis meaningfully. This is documentation guidance, not executable code, but it's misleading. Clarify that these are separate queries for the same dashboard panel group, or use | to separate them visually.

Summary of Required Changes

Priority	File	Line	Issue	Fix
High	crates/net/p2p/src/metrics.rs	524	Missing #[must_use] on PoolInUse	Add #[must_use]
High	crates/net/p2p/src/metrics.rs	576	Missing #[must_use] on BlockingTask	Add #[must_use]
High	crates/net/p2p/src/metrics.rs	600	Missing #[must_use] on BlockingRun	Add #[must_use]
High	crates/blockchain/src/metrics.rs	~121	Missing #[must_use] on EpochPrecomputeRunning	Add #[must_use]
Medium	crates/net/p2p/src/metrics.rs	838-874	Test race condition on global metrics	Use unique labels or serialization
Low	crates/net/p2p/src/beacon/envelope_checks.rs	113	PoolInUse("column_check") is confusing vs task kind	Consider comment or rename

The consensus-critical code (fork choice, attestation processing, etc.) is not modified in this PR — it's purely metrics instrumentation. The pool_of / permits_for refactoring in verdict.rs is a pure extraction and preserves semantics correctly.

The guard-based RAII pattern for metrics is well-designed and panic-safe. The main risks are the #[must_use] omissions (could lead to incorrect metrics) and test flakiness.

---

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

[MegaRedHand]

Block import benchmark (offline replay of mainnet blocks, steady state)

leg	STF per block	difference
main 012dc4f	157.7 ms
main + #752	157.4 ms	-0.3 ms (-0.2%), 95% CI [-0.8, +0.2]

No measurable overhead on import. The replay has no gossip, so most of these metrics are not exercised there; the leg also served as a noise control.

Method

• Offline ethlambda benchmark import replay on ethlambda-5 (Ryzen 7 9700X), with the node stopped.
• 3 consecutive mainnet windows fetched from that node: 173 measured blocks (slots 15567200 to 15567385), each window after one unmeasured warm-up epoch. --block-delay 1000.
• One leg per PR: the PR head merged onto main 012dc4f. The BLS PRs (perf(crypto): sum validated public keys through blst's batched affine addition #737, perf(beacon): verify a block's signatures in one random-weight batch #738, perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743) are stacked, so each of those legs includes its parents. --warm-committee-sums on the legs that have the flag (perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743, nightly).
• 2 rounds over all legs, leg order rotated per window. Every run of a window imported the same block roots.
• Numbers are per-block STF means, paired against the reference leg on the same blocks, with a bootstrap 95% CI of the mean difference.
• Steady state: 9 of the 173 blocks are dropped from every leg, because their cost exists only in a fresh process with no gossip. They are the first measured block of each window (about 31k attesters whose keys the process has never validated) and, per epoch, the first block carrying that epoch's attestations (it builds the committee shuffling inside the timed span; a live node's gossip validation builds it first). Including them makes the legs with --warm-committee-sums look about 3 points better than they are, since the flag does both before the timer starts.
• Noise floor: legs whose code the replay cannot exercise move about ±1.5% between rounds.