[MegaRedHand]

🗒️ Description / Motivation

The import replay reported stf as one number. That is most of a mainnet block's import, but it does not say which step of the transition to attack, nor whether an epoch-crossing block, a block after skipped slots and an ordinary block differ. The beacon STF had no internal timing at all.

What Changed

• crates/blockchain/state_transition/src/metrics.rs, stf/*.rs, fork_choice.rs: lean_beacon_stf_phase_seconds{context,phase} is observed around the steps of the transition: process_slots (excluding epoch work), process_epoch, the block signature, header (including the body hash it commits to), randao, eth1 data, execution payload (gloas: parent payload, withdrawals, bid), each operation list, the sync aggregate and the post-state root check. One Instant pair per step, none per attestation; an empty list observes nothing. The phase list is generated with the enum the code observes through, so the two cannot disagree.
• The context label: the epoch precompute worker runs process_epoch on another thread while an import runs, and the replay reads the histogram before and after each block, so one series would charge the worker to whichever block was importing. Only fork_choice's transition_block sets import (a thread-local guard, since the steps are observed deep inside the per-fork block processing); every other caller writes other. The replay reads only import.
• bin/ethlambda/src/benchmark/: the replay report (schema_version 2) gains per block the STF steps, the stf time outside any step, a class (epoch_boundary, after_skip, ordinary), process CPU seconds and RSS; and per run p95 and p99, a per-class summary and the deltas of the cache lookup counters, so precompute hit rates appear in it.
• docs/benchmarking.md, docs/metrics.md.

Correctness / Behavior Guarantees

• Consensus behavior is unchanged: the new code reads clocks around existing steps and never changes what is verified or in what order.

Tests Added / Run

On this branch (based on main): cargo fmt --all -- --check, make lint and cargo test --profile release-fast -p ethlambda-state-transition -p ethlambda --lib --bins are running; results will be posted as a comment.

On the adaptation of this change onto nightly (which carries the BLS stack):

• cargo fmt --all -- --check: clean; make lint: clean
• cargo test --profile release-fast -p ethlambda-state-transition -p ethlambda-blockchain -p ethlambda-crypto --lib --bins: 649 + 292 + 47 passed, 0 failed
• cargo test --profile release-fast -p ethlambda --bins: 287 passed, 4 ignored
• beacon_spec_tests (mainnet, v1.7.0-beta.2): sanity/ 595, /attestation/ 356, operations/ 2141 passed, 0 failed

Not run locally: make test (spec suites) and the minimal-preset beacon spec tests; CI covers them.

Related Issues / PRs

Part of a beacon import perf batch, each PR independent and based on main: #752, #753, #754, #755. Overlaps: #755 also edits docs/benchmarking.md and fork_choice.rs (different regions); most batch PRs edit docs/metrics.md.

The BLS stack (#738) moves block signature checks into a deferred batch, so whichever of the two merges second adapts the phases to it. The nightly adaptation does it this way: a new phase batch_signatures (observed in SignatureBatch::verify, including the failure re-check loop); randao, the operation lists and sync_aggregate then cover set building and state changes only; block_signature is the proposer signature alone, which #738 still checks immediately.

✅ 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]

🤖 Claude Code Review

Review of PR 756: split the beacon import's STF phase into steps, classes and resources

The PR is benchmark-only plus one metric label, so there's no consensus-path risk. I read the diff and the surrounding metrics.rs but did not build it or run the tests. The design is sound. The context label stops the epoch precompute worker's process_epoch from being charged to whichever block was importing. Each new reading is documented with its caveat and has a test. The unsafe in process.rs has a correct SAFETY comment.

Points worth addressing

1. Possible threading gap in the context guard (fork_choice.rs:1066).

   • import_context() is a thread-local, and its doc says every observation is made on the calling thread, never inside a rayon task. That is the one assumption the whole attribution rests on.
   • If any STF phase timer (state_root, attestations or block_signature with BLS batches) is dropped on a rayon or spawn_blocking thread, it lands in context="other".
   • The import's stf_phases would then be silently low, and stf_unattributed inflated by the same amount, with nothing in the report to flag it.
   • A debug assertion or a test that runs a block import and checks that all expected phases appear under import would guard this.

2. stf_unattributed hides over-attribution (replay.rs, stf_unattributed).

   • The .max(0.0) clamp is justified for microsecond clock skew. It also hides a real double-count if one StfPhase is ever nested inside another, for example a state_root timer opened inside process_slots.
   • The doc promises that steps "cannot exceed" stf. Consider logging a warning when the overshoot exceeds a small tolerance such as 1 ms, so a future nesting bug surfaces instead of reading as 0.

3. CPU delta includes the metrics gather (replay.rs).

   • The comment correctly orders the CPU read before the histogram gathers.
   • PhaseTimer::start and start_filtered, which call gather(), run before cpu_before, so they are excluded. That is right.
   • eprintln! and load_block also sit outside the window. Fine.
   • One caveat for the docs: RUSAGE_SELF CPU is whole-process. Idle background threads such as libp2p, if any run in the replay, add noise. The module doc says as much.

Minor

• Doc wording (report/common.rs). The comment "up to 51 samples for the p99 and 11 for the p95" is easy to misread. Say instead that p99 equals max for n ≤ 51 and p95 equals max for n ≤ 11, with the nearest-rank formula round((n-1)·q).
• Cast in read_counter. metric.get_counter().value() as u64 is fine for non-negative counters. It saturates on NaN, which Prometheus counters never produce.
• process.rs casts. tv_sec as f64 and tv_usec as f64 are fine. ru_maxrss is i64 on Linux, and u64::try_from correctly rejects negatives.
• rss_bytes cost. It reads /proc/self/status once per block. That is negligible next to an import.
• Schema bump. SCHEMA_VERSION goes 1 to 2 and is documented. Check that no consumer script or CI comparison still hard-codes 1.
• BlockClass::classify. It divides by slots_per_epoch, which is non-zero for any real preset. The epoch-crossing-over-skip precedence is sensible and documented.
• LookupCounts::from_results. hit + miss can't realistically overflow u64.

Verdict

No correctness or security issues found. I'd approve after considering Point 1 (a guard test for the thread-local assumption) and Point 2 (warn on a non-trivial overshoot).

---

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

[github-actions]

🤖 Codex Code Review

I’m done reading. Here are the review findings I’d leave on the PR.

• No consensus-critical bug jumped out in fork choice, attestation processing, justification/finalization, state transition ordering, XMSS/BLS paths, or SSZ handling from the touched hunks. The STF instrumentation is mostly clean and well-scoped.

• Potential correctness / portability bug — bin/ethlambda/src/benchmark/import/process.rs:34

  • peak_rss_bytes() assumes every non-macOS target reports ru_maxrss in KiB and multiplies by 1024.
  • That is correct for Linux, but not generally for every non-macOS Unix target. Since this code is already OS-conditional elsewhere, it would be safer to gate explicitly on target_os = "linux" and return None on unknown targets rather than silently misreporting memory.

• Potential correctness / maintainability bug — bin/ethlambda/src/benchmark/import/replay.rs:242

  • PhaseTimer::start(IMPORT_PHASE_HISTOGRAM) and PhaseTimer::start_filtered(STF_PHASE_HISTOGRAM, ("context", STF_CONTEXT_IMPORT)) are started before import(...), but I don’t see any guard here ensuring the STF context="import" is actually set for the importing thread during the whole transition.
  • If that context is only set deeper inside a subset of the import path, the replay can silently undercount stf_phases and inflate stf_unattributed_seconds.
  • I would confirm that fork_choice::on_block wraps the full transition in import_context(). If not, add it there rather than in the benchmark path so production metrics stay correct too.

• Possible observability skew — crates/blockchain/state_transition/src/beacon/stf/mod.rs:271

  • process_slots() records ProcessSlots only on success. That matches the comment, but it means failed transitions disappear from this metric entirely while still contributing to wall-clock import failure time elsewhere.
  • If you intend this histogram to explain all import STF time, failed blocks will create a systematic gap. Consider whether you want a separate failure series/counter, or to record partial elapsed time before returning errors.

• Performance concern — bin/ethlambda/src/benchmark/import/process.rs:46

  • rss_bytes() re-reads and scans /proc/self/status once per sampled block. For long replays this is small but non-zero overhead added to every sample, and it is Linux-specific string parsing in the hot measurement path.
  • If the benchmark aims for minimal observer effect, consider making RSS sampling optional, or sampling every N blocks while keeping peak_rss_bytes() per block.

• Performance concern — crates/blockchain/state_transition/src/beacon/stf/operations.rs:276

  • slash_attesting_index_intersection() builds a HashSet for attesting_indices_2, even though both attesting index lists are already sorted and unique by precondition.
  • A two-pointer merge walk would avoid allocation and hashing, and is more idiomatic for consensus code where deterministic low-overhead set intersection matters. This is not a new consensus bug, but it is worth tightening.

• Minor API robustness nit — bin/ethlambda/src/benchmark/import/replay.rs:250

  • cpu_seconds is computed as (after - before).max(0.0). Clamping hides anomalous readings instead of surfacing them.
  • For a benchmark/reporting tool that may be fine, but adding a debug log when after < before would make clock/platform issues easier to diagnose.

Overall this looks solid: the STF phase split, per-thread import context, and replay-side cache/resource reporting are thoughtfully designed and don’t appear to disturb consensus behavior. If you want, I can turn this into ready-to-post GitHub review comments with one comment per file/line.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds detailed beacon state transition phase timing, resource monitoring, and enhanced reporting to the benchmark import replay. Let me analyze each component.

Overall Assessment

This is a well-structured benchmarking/metrics PR with generally good code quality. The core logic around STF phase timing, resource monitoring, and report generation is sound. However, I've identified several issues ranging from minor correctness concerns to potential bugs in edge cases.

---

Detailed Findings

1. Potential Integer Overflow in parse_vm_rss — process.rs:71

fn parse_vm_rss(status: &str) -> Option<u64> {
    let line = status
        .lines()
        .find_map(|line| line.strip_prefix("VmRSS:"))?;
    let kibibytes: u64 = line.trim().strip_suffix("kB")?.trim().parse().ok()?;
    kibibytes.checked_mul(1024)  // Line 71
}

Issue: The strip_suffix("kB") will fail on kernels that report VmRSS in other units or with different spacing. Some kernels report VmRSS: 2048 kB with variable whitespace, but the suffix strip requires exact "kB". More critically, if a kernel ever reports VmRSS: 2048 KB (uppercase), this silently returns None.

Suggestion: Make the suffix matching case-insensitive or more lenient:

let kibibytes_str = line.trim().trim_end_matches(|c: char| c.is_alphabetic() || c.is_whitespace()).trim();

---

2. Race Condition in cpu_seconds Delta — replay.rs:257-260

let cpu_seconds = cpu_before
    .zip(process::cpu_seconds())
    .map(|(before, after)| (after - before).max(0.0));

Issue: cpu_before is captured before import() starts, but process::cpu_seconds() is called after import() completes. Between these two calls, the code reads histogram metrics (timer.finish_at_most_once(), stf_timer.finish_summed()). While the comment at lines 251-254 acknowledges this ordering is deliberate to exclude histogram read overhead from CPU measurement, there's a subtle issue: if the process is heavily loaded, the histogram reads could themselves consume meaningful CPU time that gets attributed to the block.

This is actually correct per the comment's intent, but the max(0.0) at line 260 is a silent clamp that could hide clock non-monotonicity bugs. Consider logging at debug! level when clamping occurs.

---

3. Thread-Local Guard Not Send — metrics.rs:287-301

pub struct ImportContextGuard {
    previous: StfContext,
}

// No explicit Send/Sync impls — defaults apply

Issue: ImportContextGuard contains StfContext (a simple enum), so it's Send by default. However, the thread-local semantics mean sending this guard across threads would be logically incorrect — the guard restores context on drop, but that restoration would happen on a different thread.

The code is correct because #[must_use] encourages proper scoping and the typical usage pattern is RAII within one function. However, consider adding:

impl !Send for ImportContextGuard {}
impl !Sync for ImportContextGuard {}

(Requires nightly #![feature(negative_impls)] or just document the constraint.)

Actually, since StfContext is Copy, the guard is trivially Send. The real protection is that STF_CONTEXT is thread-local. No action needed, but worth noting for review.

---

4. try_for_each_timed Empty Iterator Handling — metrics.rs:393-413

pub fn try_for_each_timed<I, E>(
    phase: StfPhase,
    items: I,
    mut step: impl FnMut(I::Item) -> Result<(), E>,
) -> Result<(), E>
where
    I: IntoIterator,
{
    let mut started = None;
    let mut outcome = Ok(());
    for item in items {
        started.get_or_insert_with(std::time::Instant::now);
        outcome = step(item);
        if outcome.is_err() {
            break;
        }
    }
    if let Some(started) = started {
        observe_stf_phase(phase, started.elapsed());
    }
    outcome
}

Issue: When items is empty, this returns Ok(()) without observation — this is documented and correct behavior. However, when items is non-empty and all items succeed, the observation includes the loop overhead between iterations. For large operation lists (e.g., thousands of attestations), this overhead is negligible; for very fast operations, it could slightly inflate the measurement.

Acceptable for benchmarking purposes. The alternative (per-item timing) would have different overhead characteristics.

---

5. Missing SyncAggregate Timing in Phase0 — block.rs:141-157

pub fn process_block_phase0(
    // ...
) -> Result<()> {
    timed(StfPhase::BlockHeader, || { ... })?;
    process_randao(state, &block.body.randao_reveal, options.randao)?;  // timed internally
    process_eth1_data(state, &block.body.eth1_data)?;  // timed internally
    crate::beacon::stf::operations::process_operations(
        state,
        &block.body,
        config,
        committees,
        active_balances,
    )?;
    // NO sync_aggregate timing — phase0 has no sync aggregate
    Ok(())
}

This is correct — Phase0 predates sync committees. The StfPhase::SyncAggregate is only observed in altair.rs::process_sync_aggregate.

---

6. Gloas Double-Counting ExecutionPayload — gloas.rs:111-125

// [New in Gloas:EIP7732]
timed(StfPhase::ExecutionPayload, || {
    process_parent_execution_payload(state, block, config)
})?;
// ...
timed(StfPhase::ExecutionPayload, || -> Result<()> {
    process_withdrawals(state)?;
    process_execution_payload_bid(state, &block.body.signed_execution_payload_bid, config)
})?;

Issue: Gloas observes ExecutionPayload twice per block — once for the parent payload and once for withdrawals+bid. The comment at line 112-114 explicitly acknowledges this:

"Gloas has no execution payload step of its own: the parent's payload, the withdrawals and the bid are its equivalents, so they are observed as execution_payload, which therefore reaches the histogram twice per block."

This is intentional and documented, but consumers of the data need to understand that ExecutionPayload sums in gloas may represent two distinct operations. The replay's finish_summed correctly handles this by summing all observations.

---

7. BlockClass::classify Edge Case — import.rs:47-58

pub(crate) const fn classify(parent_slot: u64, slot: u64, slots_per_epoch: u64) -> Self {
    if parent_slot / slots_per_epoch < slot / slots_per_epoch {
        BlockClass::EpochBoundary
    } else if slot > parent_slot + 1 {
        BlockClass::AfterSkip
    } else {
        BlockClass::Ordinary
    }
}

Issue: When parent_slot == slot (same slot, which shouldn't happen in valid chain but could in test scenarios), this returns Ordinary. When slot < parent_slot (also invalid), it returns Ordinary due to the else branch.

The slot < parent_slot case should probably panic or be explicitly handled since it violates the precondition that blocks advance the chain. However, the function is const and used in contexts where preconditions are assumed.

Consider adding debug_assert!(slot > parent_slot, "block slot must advance"); at the top, though this requires std in const context which is available.

---

8. Percentile Calculation Off-By-One Risk — common.rs:78-85

fn percentile(sorted: &[f64], q: f64) -> f64 {
    let count = sorted.len();
    if count == 0 {
        return 0.0;
    }
    let index = ((count - 1) as f64 * q).round() as usize;
    sorted[index.min(count - 1)]
}

Issue: The comment in tests says "nearest-rank of (n - 1) * q, rounded" but this is actually nearest-rank on zero-indexed array with rounding, not standard nearest-rank percentile. For the test case with 200 elements:

• index = round(199 * 0.95) = round(189.05) = 189 → sorted[189] = 190.0 ✓
• index = round(199 * 0.99) = round(197.01) = 197 → sorted[197] = 198.0 ✓

The test passes, but this is not standard nearest-rank percentile (which would use ceil(q * n) without subtracting 1). The documentation should clarify this is "linear interpolation of the nearest-rank method on zero-indexed arrays" or similar.

Actually, re-reading: The comment says "nearest-rank, like the others, so a short run reports its maximum for the tail". The behavior is intentional — for 3 samples, p95 = round(2 * 0.95) = round(1.9) = 2 = maximum. This is a deliberate choice for small-sample behavior.

No bug, but document the formula explicitly in Stats or percentile doc comment.

---

9. read_counter Panic Risk — mod.rs:477-493

fn read_counter(counter: &str) -> BTreeMap<String, u64> {
    ethlambda_metrics::gather()
        .iter()
        .filter(|family| family.name() == counter)
        .flat_map(|family| family.get_metric())
        .filter_map(|metric| {
            let result = metric
                .get_label()
                .iter()
                .find(|label| label.name() == "result")?
                .value()
                .to_string();
            Some((result, metric.get_counter().value() as u64))  // Line 491
        })
        .collect()
}

Issue: metric.get_counter().value() as u64 — if the counter value exceeds u64::MAX, this truncates silently. Prometheus counters are f64 internally. For a long-running process with high-frequency events, this could theoretically wrap.

Practical impact: Very low for benchmark replays. But for correctness:

metric.get_counter().value().round() as u64  // or use to_bits() check

Or better, use f64::to_u64 from num_traits or manual clamping.

Actually, as u64 on f64 truncates toward zero. For positive values, value().round() as u64 is safer. But Prometheus counters should be integral in practice.

---

10. Missing #[cfg(target_os = "linux")] on rss_bytes — process.rs:56-62

pub(crate) fn rss_bytes() -> Option<u64> {
    let status = std::fs::read_to_string("/proc/self/status").ok()?;
    parse_vm_rss(&status)
}

Issue: This will always return None on non-Linux, but it attempts the file read anyway. The read_to_string will fail quickly on macOS/Windows, but on systems with a /proc filesystem that doesn't have self/status (e.g., some BSDs with Linux compat), behavior is undefined.

The None return is correct and documented, but consider adding:

#[cfg(target_os = "linux")]
pub(crate) fn rss_bytes() -> Option<u64> { ... }

#[cfg(not(target_os = "linux"))]
pub(crate) fn rss_bytes() -> Option<u64> {
    None
}

This avoids the filesystem attempt entirely on non-Linux and makes the platform dependency explicit at compile time.

---

11. delta Function Name Shadowing — replay.rs:113-122

fn delta(before: &BTreeMap<String, u64>, after: &BTreeMap<String, u64>) -> BTreeMap<String, u64> {
    after
        .iter()
        .map(|(result, value)| {
            let start = before.get(result).copied().unwrap_or(0);
            (result.clone(), value.saturating_sub(start))
        })
        .collect()
}

Issue: The function name delta shadows the concept but is used as a module-private helper. However, in replay.rs tests (line 416), there's a test named a_counter_delta_excludes_what_was_counted_before_and_starts_new_results_at_zero that calls delta(&before, &after) — this is the function, not a method.

No actual bug, but the naming is slightly confusing with CacheReading::deltas_since_start. Consider renaming to counter_delta for clarity.

---

12. LookupCounts::from_results Division by Zero Protection — import.rs:162-175

pub(crate) fn from_results(results: &BTreeMap<String, u64>, keyless: bool) -> Self {
    let get = |result: &str| results.get(result).copied().unwrap_or(0);
    let (hit, miss) = (get("hit"), get("miss"));
    let hit_rate = (hit + miss > 0).then(|| hit as f64 / (hit + miss) as f64);
    Self {
        hit,
        miss,
        unkeyable: keyless.then(|| get("unkeyable")),
        hit_rate,
    }
}

Correct: The hit + miss > 0 guard prevents division by zero. When both are zero, hit_rate is None, which serializes as absent due to #[serde(skip_serializing_if = "Option::is_none")].

---

13. Critical: process_slots Epoch Timing Double-Count Risk — mod.rs:239-268

pub fn process_slots(state: &mut BeaconState, slot: Slot, config: &Config) -> Result<()> {
    // ...
    let started = Instant::now();
    let mut epoch_work = Duration::ZERO;

    while state.slot() < slot {
        process_slot(state)?;
        if (state.slot() + 1).is_multiple_of(preset::SLOTS_PER_EPOCH) {
            let epoch_started = Instant::now();
            epoch::process_epoch(state, config)?;
            let elapsed = epoch_started.elapsed();
            observe_stf_phase(StfPhase::ProcessEpoch, elapsed);
            epoch_work += elapsed;
        }
        *state.slot_mut() += 1;
        upgrade_at_fork_boundary(state, config)?;
    }

    // ...
    state.apply_pending_mutations();

    observe_stf_phase(
        StfPhase::ProcessSlots,
        started.elapsed().saturating_sub(epoch_work),
    );
    // ...
}

Issue: The process_slot(state)? call at line 248 can fail with ?. If it fails, the function returns early without observing ProcessSlots at all. This is documented in the comment at lines 264-266: "Only a call that got this far is observed: a rejected target slot or a failed epoch leaves no state worth attributing time to."

However, if process_slot fails after some epoch work was done, that epoch work was already observed (line 253). This creates an asymmetry: failed mid-epoch transitions observe partial epoch work but no ProcessSlots time.

This is intentional per the comment, but consider whether failed transitions should observe anything. For benchmarking, excluding failures is correct; for production monitoring, it might miss interesting data. Given this is benchmark-focused, acceptable.

---

14. Histogram Bucket Choice — metrics.rs:349-354

vec![
    0.0001, 0.0005, 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1.0, 2.0, 4.0
]

Issue: The largest bucket is 4.0 seconds. For process_epoch on a large state or state_root hashing, this could be exceeded. Prometheus histograms put +Inf observations in the last bucket, so data isn't lost, but precision is.

Suggestion: Add 8.0 and 16.0 buckets, or verify through testing that 4.0s captures >99% of observations. Mainnet epoch transitions can take multiple seconds on slower hardware.

---

15. Documentation Inconsistency — benchmarking.md

The documentation says (line 332): "the p99 is the maximum up to 51 samples, the p95 up to 11." But percentile uses round((n-1)*q), so:

• For 11 samples: p95 index = round(10 * 0.95) = round(9.5) = 10 = maximum (index 10 of 0..10)
• For 51 samples: p99 index = round(50 * 0.99) = round(49.5) = 50 = maximum (index 50 of 0..50)

The documentation is correct but the threshold is actually 10 for p95 and 50 for p99 (0-indexed), making it "up to 11" and "up to 51" inclusive. Minor off-by-one in explanation vs. implementation, but the behavior matches the intent.

---

Summary

Priority	Item	File	Line(s)	Issue
Low	1	process.rs	71	VmRSS parsing too strict
Low	2	replay.rs	257-260	max(0.0) silent clamp
Info	3	metrics.rs	287-301	ImportContextGuard Send semantics
Low	4	metrics.rs	393-413	Loop overhead in try_for_each_timed
—	5	block.rs	141-157	Correctly no sync aggregate in phase0
—	6	gloas.rs	111-125

---

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

[MegaRedHand]

Checks on this branch (based on main, c891134):

• cargo fmt --all -- --check: clean
• make lint: clean
• cargo test --profile release-fast -p ethlambda-state-transition -p ethlambda --lib --bins: state-transition 635 passed (15 ignored), ethlambda bins 285 passed (4 ignored), 0 failed