feat(l1): log block hash in per-block execution metric line - #6736
Conversation
|
🤖 Kimi Code ReviewReview Summary: This is a good refactoring that eliminates redundant block hash calculations and improves observability. No issues found. Detailed Feedback: Performance Optimization (Line 1936)
Code Quality (Lines 1951-1956)
Function Signature Change (Line 2045)
Observability Improvement (Lines 2133-2134)
Ethereum-Specific Verification
Suggestion (Minor): Verdict: LGTM. The changes improve performance, reduce code duplication, and enhance observability without introducing any safety issues. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewReview:
|
Greptile SummaryAdds the block hash to the per-block
Confidence Score: 5/5Safe to merge — the change is additive and purely affects log output. The change is small and well-scoped: it hoists a hash computation that was already happening in one or two conditional branches to just before the point where the block is moved (which made the pre-computation necessary anyway), then threads it through the logging function. No data path, storage operation, or error handling is affected. The format-string change is backward-compatible with the confirmed log consumers described in the PR. No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/blockchain/blockchain.rs | Moves block_hash computation before store_block (which consumes the block), passes it to print_add_block_pipeline_logs, and adds it to the METRIC log header. Also refactors the BAL if-let into a let-chain. No logic issues found. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[add_block_pipeline] --> B[execute_block_pipeline]
B --> C[Extract gas_used, gas_limit,\nblock_number, transactions_count]
C --> D["block_hash = block.hash()\n(computed once here)"]
D --> E{logger && accumulated_updates?}
E -- Yes --> F[generate_witness\nstore_witness using block_hash]
E -- No --> G{produced_bal?}
F --> G
G -- Yes --> H[store_block_access_list\nusing block_hash]
G -- No --> I[store_block\nconsumes block]
H --> I
I --> J{perf_logs_enabled?}
J -- Yes --> K["print_add_block_pipeline_logs\n[METRIC] BLOCK {num} {#x hash} | ..."]
J -- No --> L[return]
K --> L
Reviews (1): Last reviewed commit: "feat(l1): log block hash in per-block ex..." | Re-trigger Greptile
Lines of code reportTotal lines added: Detailed view |
🤖 Codex Code ReviewNo findings. The diff in blockchain.rs looks safe: it hoists I couldn’t complete a compile check in this environment: the Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
3bb6e79 to
7df4937
Compare
## What
Implements the ethrex block-log parser in `pkg/blocklog` (replacing the
stub), so ethrex runs produce per-block metrics like the other clients.
It parses ethrex's per-block execution metric header (emitted on the
`engine_newPayload` path) into the shared `{ "block": { "hash": ... },
... }` shape:
```
[METRIC] BLOCK 24358000 0x<hash> | 1.234 Ggas/s | 567.00 ms | 150 txs | 700 Mgas (93%)
```
Captured fields: `block.number`, `block.hash`, `block.gas_used_mgas`,
`block.gas_used_pct`, `block.tx_count`, `throughput.ggas_per_sec`,
`timing.total_ms`. ANSI is stripped; the hash is optional in the regex
so the line still parses on builds that don't log it (it just won't
match a test without `block.hash`).
## Dependency
Matching a log to a test requires `block.hash`, and ethrex does not yet
include the hash on this line. That is addressed upstream in
**lambdaclass/ethrex#6736**, which adds the hash exactly where this
parser expects it (`BLOCK <num> 0x<hash> | ...`). Once that lands and
reaches the ethrex image used here, ethrex runs will start attaching
per-block metrics.
Marking this draft until the ethrex change is merged and released.
## Notes
- ethrex follows this header with separate `|-
validate/exec/merkle/store` lines; since the collector matches one
payload per line by hash, only the header is captured. Folding in the
phase breakdown would need ethrex to emit a single structured line
(follow-up).
- Test coverage mirrors `reth_test.go`: hash / no-hash / zero-gas / ANSI
/ non-matching lines. `gofmt`, `go vet`, and `go test ./pkg/blocklog/`
pass.
The per-block execution metric line emitted on the `engine_newPayload` / `add_block_pipeline` path logs the block number but not the block hash: ``` [METRIC] BLOCK 24358000 | 1.234 Ggas/s | 567.00 ms | 150 txs | 700 Mgas (93%) ``` The block number alone is not a stable key for correlating these metrics with a specific block: benchmarking and replay harnesses commonly roll back and re-import at the same height, so the same block number maps to many distinct blocks. The block hash is the only content-unique identifier. This also helps [ethpandaops/benchmarkoor](https://github.com/ethpandaops/benchmarkoor), which captures client block logs and associates them with tests strictly by block hash (`{ "block": { "hash": "0x..." } }`). Without the hash on this line, ethrex runs there can't attach per-block metrics; with it, a log parser can match them. (benchmarkoor drives blocks via `engine_newPayload`, which goes through this exact pipeline log.) Thread the already-available `block.hash()` into `print_add_block_pipeline_logs` and include it in the metric header: ``` [METRIC] BLOCK 24358000 0x<hash> | 1.234 Ggas/s | 567.00 ms | 150 txs | 700 Mgas (93%) ``` The hash was already computed in this function on the witness/BAL branches; it is now computed once before the block is moved into `store_block` and reused, so there is no extra hashing on the common path. Uses the existing `{:#x}` block-hash convention (see `dev/block_producer.rs`). Verified the existing in-tree log parsers are unaffected: - `tooling/log_analysis/log_analysis.ipynb` — regex is hardcoded to the `BLOCK EXECUTION THROUGHPUT (N): ...` line (`print_add_block_logs`), a different line that this PR does not touch. - `tooling/import_benchmark/parse_bench.py` — keys off the first `)` in the line; the hash is inserted before the trailing `(...%)`, so its parsing is unchanged. No other consumers of this line exist in the repo (Grafana panels read Prometheus metrics, not logs).
4 participants
Motivation
The per-block execution metric line emitted on the
engine_newPayload/add_block_pipelinepath logs the block number but not the block hash:The block number alone is not a stable key for correlating these metrics with a specific block: benchmarking and replay harnesses commonly roll back and re-import at the same height, so the same block number maps to many distinct blocks. The block hash is the only content-unique identifier.
This also helps ethpandaops/benchmarkoor, which captures client block logs and associates them with tests strictly by block hash (
{ "block": { "hash": "0x..." } }). Without the hash on this line, ethrex runs there can't attach per-block metrics; with it, a log parser can match them. (benchmarkoor drives blocks viaengine_newPayload, which goes through this exact pipeline log.)Change
Thread the already-available
block.hash()intoprint_add_block_pipeline_logsand include it in the metric header:The hash was already computed in this function on the witness/BAL branches; it is now computed once before the block is moved into
store_blockand reused, so there is no extra hashing on the common path. Uses the existing{:#x}block-hash convention (seedev/block_producer.rs).Log-consumer check
Verified the existing in-tree log parsers are unaffected:
tooling/log_analysis/log_analysis.ipynb— regex is hardcoded to theBLOCK EXECUTION THROUGHPUT (N): ...line (print_add_block_logs), a different line that this PR does not touch.tooling/import_benchmark/parse_bench.py— keys off the first)in the line; the hash is inserted before the trailing(...%), so its parsing is unchanged.No other consumers of this line exist in the repo (Grafana panels read Prometheus metrics, not logs).