lambdaclass / lambdaclass/ethlambda

refactor(blockchain): return block-building phase timings instead of deltaing histogram sums

Open
#599 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

performance
Dominant language
Rust
Stars
82
Forks
28
Avg merge
1d 20h
Merged PRs (30d)
20

Description

Follow-up from review of #595, deferred there by agreement:

This is a really hacky way of recording phase times. We should record the duration of
each phase inside the relevant functions and return a report, which the metrics
consume. That way we don't have to do this here.

We can do this in another PR

— @MegaRedHand, https://github.com/lambdaclass/ethlambda/pull/595#discussion_r3884295715

Current approach

The benchmark reads the lean_block_proposal_attestation_build_phase_seconds histogram
from the default prometheus registry before and after each build, and takes the
difference of the per-label sample sums as the phase durations
(PhaseTimer in bin/ethlambda/src/benchmark/mod.rs).

It is exact — histogram sums accumulate raw f64 seconds, so bucket boundaries play no
part — and it needs no changes to the hot path, which is why it was the right call to get
the harness landed. But it is a roundabout way to obtain numbers the building code
already has:

  • Phase timings reach the benchmark only through a global registry, so the harness
    depends on process-wide state and on the metric's label set staying exactly as it is.
  • Correctness rests on each phase being observed exactly once per build. The harness has
    to assert that and fail the run when it does not hold, because it cannot otherwise tell
    a mis-attribution from a real measurement.
  • It works only single-threaded and single-configuration per process; two concurrent
    builds would interleave into the same counters.

Proposed change

Have the block-building phases measure themselves and return their durations as part of a
report, with the metrics layer as one consumer of that report rather than the channel
through which timings travel:

  • build_block (and the phases inside it) return the phase durations alongside their
    result.
  • The existing histogram observations are fed from that report, so dashboards are
    unchanged.
  • The benchmark consumes the same report directly and drops PhaseTimer, the registry
    read, and the once-per-build assertion.

Why it is worth doing

Beyond removing the indirection: the timings become available to anything that builds a
block, not just to a process that can read the global registry — which is what a
replay-from-datadir mode and any future per-build logging would want. It also removes the
only reason the benchmark is restricted to one configuration per process invocation.

Acceptance criteria

  • Phase durations come from the building code, not from a registry diff.
  • lean_block_proposal_attestation_build_phase_seconds keeps its current name, labels and
    values, so existing dashboards and alerts are unaffected.
  • PhaseTimer, the read() helper and the "observed exactly once" assertion are gone
    from bin/ethlambda/src/benchmark/mod.rs.
  • make bench reports the same phases, and a same-seed run produces the same
    per-iteration block roots as before.

Related: #465 (Optimize block building), #595 (the harness), #596 (report statistics).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in bin/ethlambda/src/benchmark/mod.rs with PhaseTimer, the registry read helper, and the once-per-build assertion, then trace build_block and its phase timing data. Verify that the existing histogram name, labels, and values remain unchanged, and run make bench with the same seed to compare per-iteration block roots.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
blockchain
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.