feat(metrics): add Prometheus metrics for Engine API and payload builder#97
Merged
feat(metrics): add Prometheus metrics for Engine API and payload builder#97
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds comprehensive Prometheus metrics instrumentation to the Morph L2 Reth engine API and payload builder. It tracks latency (duration histograms) and failures (counters) for engine methods and builder operations, plus includes local devnet testing infrastructure with Prometheus/Grafana monitoring and extensive Python test/audit tooling. Changes
Sequence Diagram(s)No sequence diagrams are generated because the changes do not introduce new inter-component interactions—they add observability instrumentation (metrics recording) to existing flows without altering control flow or adding interactions between distinct components. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
etc/prometheus/prometheus.yml (1)
6-6: Avoid mixing mutually exclusive scrape targets in one job.This job will scrape all three targets, which can create persistent
DOWNnoise (and sometimes duplicate data paths) depending on runtime topology. Prefer one environment-specific target (or separate jobs/profiles).Suggested config adjustment
scrape_configs: - job_name: morph-reth metrics_path: "/" scrape_interval: 5s static_configs: - - targets: ['morph-reth:9001', 'localhost:9001', 'host.docker.internal:9001'] + - targets: ['morph-reth:9001']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etc/prometheus/prometheus.yml` at line 6, The scrape job currently lists three mutually exclusive targets in the same "targets" array ('morph-reth:9001', 'localhost:9001', 'host.docker.internal:9001'), causing DOWN noise and duplicate metrics; update the Prometheus config so each environment has a single selected target or split into separate jobs/profiles (or use environment-specific relabeling/files_sd) instead of one combined "targets" entry—modify the job that contains the "targets" list to either pick the appropriate environment-specific target or create distinct jobs named per environment to avoid scraping multiple conflicting endpoints.crates/payload/builder/src/metrics.rs (1)
86-94: Consider adding documentation about naming convention difference.The
inc_pool_tx_skippedmethod usesmorph_payload_builder_*(underscores) while the struct uses scopemorph.payload_builder(dots) which gets converted to underscores. The manualcounter!macro here is necessary for labeled metrics, but the naming convention differs slightly from the auto-generated metrics (no scope prefix transformation). This is acceptable but worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/payload/builder/src/metrics.rs` around lines 86 - 94, The inc_pool_tx_skipped method in MorphPayloadBuilderMetrics uses a manually constructed metric name via metrics::counter!("morph_payload_builder_pool_transactions_skipped_total", ...) which differs from the struct's scope style (morph.payload_builder -> converted to underscores by auto-metrics), so add a short doc-comment on the impl (or above inc_pool_tx_skipped) noting the naming convention difference and why the manual counter! is used (to attach a reason label), clarifying that the manual name uses underscores and is intentionally chosen to match the labeled metric rather than the auto-generated scoped name.etc/generate-jwt.sh (1)
9-9: Redundantteeusage with output redirection.The
tee > filepattern is confusing and inefficient.teewrites to both stdout and its file argument, but piping throughteeand then redirecting to the same file is redundant. Either useteealone or just redirect.♻️ Proposed fix
- openssl rand -hex 32 | tr -d "\n" | tee > "${SCRIPT_DIR}/jwttoken/jwt.hex" + openssl rand -hex 32 | tr -d "\n" > "${SCRIPT_DIR}/jwttoken/jwt.hex"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etc/generate-jwt.sh` at line 9, The pipeline uses a redundant "tee > \"${SCRIPT_DIR}/jwttoken/jwt.hex\"" pattern; replace it with a single, correct redirection such as using tee with the file argument ("tee \"${SCRIPT_DIR}/jwttoken/jwt.hex\"") or simply redirecting stdout to the file ("> \"${SCRIPT_DIR}/jwttoken/jwt.hex\"") so the line that currently contains openssl rand -hex 32 | tr -d "\n" | tee > "${SCRIPT_DIR}/jwttoken/jwt.hex" is simplified to one of those forms to remove the unnecessary extra redirection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/engine-api/src/builder.rs`:
- Around line 1365-1366: The test imports `MorphEngineApiMetrics` directly but
also has an unused `use super::*;` which causes the CI failure; remove the `use
super::*;` import from the test in `builder.rs` so only the needed `use
crate::metrics::MorphEngineApiMetrics;` remains (ensure no other symbols from
the parent module were relied upon after removal).
In `@crates/engine-api/src/metrics.rs`:
- Around line 18-21: The docs mention a non-existent gauge `head_block_number`;
update the comment to reflect the actual metric(s) defined—replace
`head_block_number` with the correct metric name or remove it so the "Chain head
health" section only lists `head_block_timegap_seconds`; locate the comment near
the `head_block_timegap_seconds` declaration in metrics.rs and ensure the
two-line list accurately matches the struct/metrics currently implemented.
In `@crates/payload/builder/src/metrics.rs`:
- Around line 16-23: The docs table in metrics.rs references a non-existent
metric commit_tx_duration_seconds; either remove that table row to avoid stale
documentation or add the missing metric to your metrics struct and registration
logic (e.g., add a commit_tx_duration_seconds histogram field and its
initialization where other metrics are created) so the documentation matches the
actual symbols; update the table row or the struct/registration for
commit_tx_duration_seconds accordingly.
---
Nitpick comments:
In `@crates/payload/builder/src/metrics.rs`:
- Around line 86-94: The inc_pool_tx_skipped method in
MorphPayloadBuilderMetrics uses a manually constructed metric name via
metrics::counter!("morph_payload_builder_pool_transactions_skipped_total", ...)
which differs from the struct's scope style (morph.payload_builder -> converted
to underscores by auto-metrics), so add a short doc-comment on the impl (or
above inc_pool_tx_skipped) noting the naming convention difference and why the
manual counter! is used (to attach a reason label), clarifying that the manual
name uses underscores and is intentionally chosen to match the labeled metric
rather than the auto-generated scoped name.
In `@etc/generate-jwt.sh`:
- Line 9: The pipeline uses a redundant "tee >
\"${SCRIPT_DIR}/jwttoken/jwt.hex\"" pattern; replace it with a single, correct
redirection such as using tee with the file argument ("tee
\"${SCRIPT_DIR}/jwttoken/jwt.hex\"") or simply redirecting stdout to the file
("> \"${SCRIPT_DIR}/jwttoken/jwt.hex\"") so the line that currently contains
openssl rand -hex 32 | tr -d "\n" | tee > "${SCRIPT_DIR}/jwttoken/jwt.hex" is
simplified to one of those forms to remove the unnecessary extra redirection.
In `@etc/prometheus/prometheus.yml`:
- Line 6: The scrape job currently lists three mutually exclusive targets in the
same "targets" array ('morph-reth:9001', 'localhost:9001',
'host.docker.internal:9001'), causing DOWN noise and duplicate metrics; update
the Prometheus config so each environment has a single selected target or split
into separate jobs/profiles (or use environment-specific relabeling/files_sd)
instead of one combined "targets" entry—modify the job that contains the
"targets" list to either pick the appropriate environment-specific target or
create distinct jobs named per environment to avoid scraping multiple
conflicting endpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03e0d0c4-6be4-42cf-b1b7-65ef1560cb4a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlcrates/engine-api/Cargo.tomlcrates/engine-api/src/builder.rscrates/engine-api/src/lib.rscrates/engine-api/src/metrics.rscrates/payload/builder/Cargo.tomlcrates/payload/builder/src/builder.rscrates/payload/builder/src/lib.rscrates/payload/builder/src/metrics.rsetc/.gitignoreetc/README.mdetc/docker-compose.ymletc/generate-jwt.shetc/grafana/dashboards/dashboard.ymletc/grafana/dashboards/morph-engine.jsonetc/grafana/dashboards/overview.jsonetc/grafana/dashboards/reth-discovery.jsonetc/grafana/dashboards/reth-mempool.jsonetc/grafana/dashboards/reth-state-growth.jsonetc/grafana/datasources/prometheus.ymletc/prometheus/prometheus.yml
Adds custom Morph-specific Prometheus metrics covering gaps that reth's
built-in metrics don't address:
**Engine API metrics** (`morph.engine` scope):
- Per-method latency histograms for all 5 custom RPC endpoints:
assembleL2Block, newL2Block, validateL2Block, newSafeL2Block, setBlockTags
- Per-method failure counters for each endpoint
- `head_block_timegap_seconds` gauge — seconds since last imported block
timestamp, analogous to geth's `chain/head/timegap`
**Payload builder metrics** (`morph.payload_builder` scope):
- `l1_tx_gas_limit_exceeded_total` / `l1_tx_strange_err_total` — L1
message skip counters, analogous to geth's `miner/skipped_txs/l1/*`
- `commit_txs_all_duration_seconds` — total tx execution time per block
- `commit_tx_apply_duration_seconds` — per-tx EVM execution time
- `payload_build_duration_seconds` — end-to-end build time (state setup
+ execution + finalize), inspired by tempo's payload builder metrics
- `block_transactions` gauge — tx count in last built block, analogous
to geth's `processor/block/transactions`
- `pool_transactions_skipped_total{reason=...}` — pool tx skip counter
with reason label (invalid_type / over_limits / nonce_too_low /
invalid_tx / validation_error), inspired by tempo
**Observability infrastructure**:
- `etc/` directory with docker-compose (morph-reth + Prometheus + Grafana),
Prometheus scrape config, and all reth upstream Grafana dashboards
- `etc/grafana/dashboards/morph-engine.json` — new Grafana dashboard with
14 panels covering chain head health, Engine API latency/failures, payload
builder timing, and L1 message processing
**Tests**:
- `test_all_engine_api_metrics` — verifies all engine metrics register and
record correctly using `metrics_util::DebuggingRecorder`
- `test_all_payload_builder_metrics` — verifies all payload builder metrics
including labeled counters
panos-xyz
force-pushed
the
worktree-feat+engine-api-metrics
branch
from
bda42a0 to
87c80a4
Compare
- Remove stale doc references to deleted metrics (head_block_number, commit_tx_duration_seconds) - Add payload_build_duration_seconds to metrics doc table - Document naming convention for labeled pool_tx_skipped counter - Fix redundant tee usage in generate-jwt.sh - Use single Prometheus scrape target to avoid DOWN noise
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
etc/README.md (2)
72-72: Consider more concise wording.The phrase "in order to" can be simplified to "to" for more concise documentation.
📝 Suggested refinement
-1. If updating an existing dashboard, you will need to change the name and UID of the imported dashboard in order to +1. If updating an existing dashboard, you will need to change the name and UID of the imported dashboard to avoid conflict🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etc/README.md` at line 72, Replace the phrase "in order to" with the more concise "to" in the sentence that begins "If updating an existing dashboard, you will need to change the name and UID of the imported dashboard in order to" (update the trailing text accordingly) so the sentence reads using "to" instead of "in order to" for clearer, more concise wording.
5-5: Consider more concise wording.The phrase "a lot of" could be replaced with "many" for more concise documentation.
📝 Suggested refinement
-The files in this directory may undergo a lot of changes while reth is unstable, so do not expect them to necessarily be +The files in this directory may undergo many changes while reth is unstable, so do not expect them to necessarily be🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etc/README.md` at line 5, Replace the phrase "a lot of" in the README sentence with the more concise "many" (i.e., change "may undergo a lot of changes" to "may undergo many changes") to tighten wording while preserving meaning; update the sentence that currently contains "a lot of" so the README reads more concise and professional.crates/payload/builder/src/builder.rs (1)
606-606: Record build duration on non-success exits too.
payload_build_duration_secondsis only emitted forBuildOutcomeKind::Better. Cancelled, aborted, and error paths still consume builder time but never hit this histogram, so the latency signal will skew low under contention. A smallDropguard aroundbuild_startedwould make this metric complete.Also applies to: 673-676, 697-700, 781-783
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/payload/builder/src/builder.rs` at line 606, The histogram payload_build_duration_seconds is only recorded for BuildOutcomeKind::Better; wrap the build timer (the Instant stored in build_started) in a small RAII/Drop guard that records the elapsed time to payload_build_duration_seconds on drop so every exit path (Cancelled, Aborted, Error, etc.) records duration; implement a struct (e.g., BuildDurationGuard) that takes build_started and the relevant labels/outcome (or records raw duration) and ensure an instance is created where build_started is currently set so it runs for the code regions around the build (places referring to build_started and BuildOutcomeKind::Better) instead of only in the Better-success branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/payload/builder/src/builder.rs`:
- Around line 688-694: The gauge ctx.metrics.block_transactions is being set too
early (before the is_better_payload abort and before fallible finalization) and
can be overwritten by discarded/failed candidates; move the
ctx.metrics.block_transactions.set(info.transaction_count as f64) call so it
only runs after the successful finalization (i.e., after
MorphBuiltPayload::new(...) completes without error and the payload is accepted
by is_better_payload), leaving the commit_txs_all_duration_seconds.record call
where it is or also moving it to the final success path if you want timing only
for successful commits; specifically, update the code around
MorphBuiltPayload::new, is_better_payload, and the commit path to set
block_transactions only on the final success branch.
In `@etc/README.md`:
- Around line 22-60: The README references a missing docs link
"../docs/design/metrics.md#creating-metrics" which breaks the documentation;
update the README entry in the metrics setup section by either (A) replacing
that broken reference with the correct internal path to the metrics doc if it
exists in this repo, (B) linking to the upstream Reth metrics guide URL, or (C)
removing the anchor and adding a short inline summary of "creating metrics"
steps; ensure you update the exact reference string
"../docs/design/metrics.md#creating-metrics" and keep the rest of the
Grafana/dashboards instructions (mentions of grafana/dashboards and Export/Save
to file) unchanged.
- Line 16: Update the README link to provide morph-reth-specific Docker Compose
guidance: replace or supplement the upstream link
(https://reth.rs/installation/docker#using-docker-compose) with a local
reference to ./docker-compose.yml and explicit instructions that the compose
file uses the morph-reth service with the --chain morph flag, and remove/clarify
any mention of lighthouse.yml since that file does not exist in this repo;
ensure references to "morph-reth", "--chain morph", and "./docker-compose.yml"
are added so users follow the correct, repo-specific setup.
---
Nitpick comments:
In `@crates/payload/builder/src/builder.rs`:
- Line 606: The histogram payload_build_duration_seconds is only recorded for
BuildOutcomeKind::Better; wrap the build timer (the Instant stored in
build_started) in a small RAII/Drop guard that records the elapsed time to
payload_build_duration_seconds on drop so every exit path (Cancelled, Aborted,
Error, etc.) records duration; implement a struct (e.g., BuildDurationGuard)
that takes build_started and the relevant labels/outcome (or records raw
duration) and ensure an instance is created where build_started is currently set
so it runs for the code regions around the build (places referring to
build_started and BuildOutcomeKind::Better) instead of only in the
Better-success branches.
In `@etc/README.md`:
- Line 72: Replace the phrase "in order to" with the more concise "to" in the
sentence that begins "If updating an existing dashboard, you will need to change
the name and UID of the imported dashboard in order to" (update the trailing
text accordingly) so the sentence reads using "to" instead of "in order to" for
clearer, more concise wording.
- Line 5: Replace the phrase "a lot of" in the README sentence with the more
concise "many" (i.e., change "may undergo a lot of changes" to "may undergo many
changes") to tighten wording while preserving meaning; update the sentence that
currently contains "a lot of" so the README reads more concise and professional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 005c93e9-d751-451d-bc3d-ee88c6b73c67
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
crates/engine-api/Cargo.tomlcrates/engine-api/src/builder.rscrates/engine-api/src/lib.rscrates/engine-api/src/metrics.rscrates/payload/builder/Cargo.tomlcrates/payload/builder/src/builder.rscrates/payload/builder/src/lib.rscrates/payload/builder/src/metrics.rsdeny.tomletc/.gitignoreetc/README.mdetc/docker-compose.ymletc/generate-jwt.shetc/grafana/dashboards/dashboard.ymletc/grafana/dashboards/morph-engine.jsonetc/grafana/dashboards/overview.jsonetc/grafana/dashboards/reth-discovery.jsonetc/grafana/dashboards/reth-mempool.jsonetc/grafana/dashboards/reth-state-growth.jsonetc/grafana/datasources/prometheus.ymletc/prometheus/prometheus.yml
✅ Files skipped from review due to trivial changes (14)
- etc/.gitignore
- crates/engine-api/src/lib.rs
- crates/payload/builder/src/lib.rs
- etc/grafana/dashboards/dashboard.yml
- deny.toml
- crates/payload/builder/Cargo.toml
- etc/generate-jwt.sh
- etc/grafana/datasources/prometheus.yml
- crates/engine-api/Cargo.toml
- etc/grafana/dashboards/morph-engine.json
- crates/engine-api/src/metrics.rs
- etc/docker-compose.yml
- etc/grafana/dashboards/reth-discovery.json
- etc/grafana/dashboards/reth-state-growth.json
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/payload/builder/src/metrics.rs
- etc/prometheus/prometheus.yml
Comment on lines
+688
to
+694
| // Record total transaction execution time and per-block tx count. | ||
| ctx.metrics | ||
| .commit_txs_all_duration_seconds | ||
| .record(txs_all_started.elapsed()); | ||
| ctx.metrics | ||
| .block_transactions | ||
| .set(info.transaction_count as f64); |
There was a problem hiding this comment.
Move block_transactions to the final success path.
This gauge is set before the is_better_payload abort and before the later fallible finalization steps, so a discarded or failed candidate can overwrite the “most recently built block” value. Please update it only after MorphBuiltPayload::new(...) succeeds.
Suggested change
ctx.metrics
.commit_txs_all_duration_seconds
.record(txs_all_started.elapsed());
- ctx.metrics
- .block_transactions
- .set(info.transaction_count as f64);
// Check if this payload is better than the previous one
if !ctx.is_better_payload(info.total_fees) {
return Ok(BuildOutcomeKind::Aborted {
fees: info.total_fees,
@@
let payload = MorphBuiltPayload::new(
ctx.payload_id(),
sealed_block,
info.total_fees,
executable_data,
Some(executed),
);
+ ctx.metrics
+ .block_transactions
+ .set(info.transaction_count as f64);
ctx.metrics
.payload_build_duration_seconds
.record(build_started.elapsed());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/payload/builder/src/builder.rs` around lines 688 - 694, The gauge
ctx.metrics.block_transactions is being set too early (before the
is_better_payload abort and before fallible finalization) and can be overwritten
by discarded/failed candidates; move the
ctx.metrics.block_transactions.set(info.transaction_count as f64) call so it
only runs after the successful finalization (i.e., after
MorphBuiltPayload::new(...) completes without error and the payload is accepted
by is_better_payload), leaving the commit_txs_all_duration_seconds.record call
where it is or also moving it to the final success path if you want timing only
for successful commits; specifically, update the code around
MorphBuiltPayload::new, is_better_payload, and the commit path to set
block_transactions only on the final success branch.
High: fix docker-compose paths to match Dockerfile USER morph-reth - Change /root/... to /var/lib/morph-reth/... for data, logs, and JWT - Add explicit --datadir flag Medium: record metrics on all return paths in validate_l2_block/new_l2_block - validate_l2_block: add duration+failure recording on discontinuous block number and wrong parent hash early-return paths - new_l2_block: add duration recording on past-block Ok(()) return path Medium: fix block_transactions gauge semantic - Move set() to after is_better_payload() check so Aborted builds do not update the gauge; only successful BuildOutcomeKind::Better updates it - Update doc comment to clarify Aborted/Cancelled do not update the gauge Low: replace broken ../docs/design/metrics.md link in etc/README.md - Point to actual examples in the codebase instead
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/payload/builder/src/builder.rs (2)
328-373:⚠️ Potential issue | 🟠 MajorRecord
commit_tx_apply_duration_secondsfor failedexecute_transactioncalls too.The timer starts before
execute_transaction, but Line 344, Line 359, Line 368, Line 507, Line 519, and Line 523 exit before the histogram update. Right now the metric only captures successful executions, so slow validation or fatal failures disappear from the latency distribution.🔧 Suggested change
- let apply_started = Instant::now(); - let gas_used = match builder.execute_transaction(recovered_tx.clone()) { + let apply_started = Instant::now(); + let exec_result = builder.execute_transaction(recovered_tx.clone()); + self.metrics + .commit_tx_apply_duration_seconds + .record(apply_started.elapsed()); + let gas_used = match exec_result { Ok(gas_used) => gas_used, @@ - self.metrics - .commit_tx_apply_duration_seconds - .record(apply_started.elapsed()); @@ - let apply_started = Instant::now(); - let gas_used = match builder.execute_transaction(tx.clone()) { + let apply_started = Instant::now(); + let exec_result = builder.execute_transaction(tx.clone()); + self.metrics + .commit_tx_apply_duration_seconds + .record(apply_started.elapsed()); + let gas_used = match exec_result { Ok(gas_used) => gas_used, @@ - self.metrics - .commit_tx_apply_duration_seconds - .record(apply_started.elapsed());Also applies to: 478-528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/payload/builder/src/builder.rs` around lines 328 - 373, The timer started in apply_started before calling builder.execute_transaction is only recorded on the success path; update the error-return branches in the execute_transaction handling (the match in which execute_transaction, BlockExecutionError::Validation, and Err(err) are matched) to always record commit_tx_apply_duration_seconds before any early return — e.g., compute apply_elapsed = apply_started.elapsed() or call self.metrics.commit_tx_apply_duration_seconds.record(apply_started.elapsed()) in each error arm (or use a scope guard) so validation failures and fatal errors also contribute to the histogram; changes should be applied around the builder.execute_transaction match and any other similar blocks (same pattern around lines handling execute_transaction/commit_tx_apply_duration_seconds).
318-373:⚠️ Potential issue | 🟡 Minor
l1_tx_strange_err_totalstill misses the non-EVM invalid exits.Line 309 (blob tx rejection) and Line 381 (queue-index mismatch) both return
PayloadBuilderErrorwithout incrementingl1_tx_strange_err_total, so malformed payload attributes will be undercounted.🔧 Suggested change
if recovered_tx.is_eip4844() { + self.metrics.l1_tx_strange_err_total.increment(1); return Err(PayloadBuilderError::other( MorphPayloadBuilderError::BlobTransactionRejected, )); } @@ if queue_index != info.next_l1_message_index { + self.metrics.l1_tx_strange_err_total.increment(1); return Err(PayloadBuilderError::other( MorphPayloadBuilderError::InvalidSequencerTransaction { error: format!(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/payload/builder/src/builder.rs` around lines 318 - 373, Some non-EVM invalid exits (e.g., the BlockGasLimitExceededBySequencerTransactions path and the blob/queue-index rejection paths) return a PayloadBuilderError without incrementing the malformed-L1 metric; update those return sites to call self.metrics.l1_tx_strange_err_total.increment(1) immediately before returning. Specifically, add the increment before the Err(PayloadBuilderError::other(MorphPayloadBuilderError::BlockGasLimitExceededBySequencerTransactions { .. })) return and likewise before the returns that produce PayloadBuilderError::other for the blob tx rejection / queue-index mismatch so all malformed payload-attribute error paths report via metrics.l1_tx_strange_err_total.
♻️ Duplicate comments (1)
etc/README.md (1)
15-16:⚠️ Potential issue | 🟡 MinorUse morph-reth-specific Docker Compose guidance instead of upstream-only docs.
This still points users to upstream Docker instructions, which may not match this repo’s compose setup. Please add a local reference (for example
./docker-compose.yml) and a short morph-reth-specific command/example alongside the upstream link.Suggested doc tweak
-To run Reth, Grafana or Prometheus with Docker Compose, refer to -the [docker docs](https://reth.rs/installation/docker#using-docker-compose). +To run morph-reth, Grafana, and Prometheus with Docker Compose, use +[`./docker-compose.yml`](./docker-compose.yml) in this repository. +For general upstream container docs, see +[reth docker docs](https://reth.rs/installation/docker#using-docker-compose).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etc/README.md` around lines 15 - 16, The README currently points only to upstream Docker docs; update the sentence to reference the repository's local compose file (e.g., ./docker-compose.yml) and add a short morph-reth-specific example command (for example a one-liner like "docker compose -f ./docker-compose.yml up -d reth grafana prometheus") alongside the existing upstream link so users have both upstream docs and an immediate local usage example; modify the README.md line that currently links to the upstream docs to include the local file reference and the example command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/payload/builder/src/builder.rs`:
- Around line 328-373: The timer started in apply_started before calling
builder.execute_transaction is only recorded on the success path; update the
error-return branches in the execute_transaction handling (the match in which
execute_transaction, BlockExecutionError::Validation, and Err(err) are matched)
to always record commit_tx_apply_duration_seconds before any early return —
e.g., compute apply_elapsed = apply_started.elapsed() or call
self.metrics.commit_tx_apply_duration_seconds.record(apply_started.elapsed()) in
each error arm (or use a scope guard) so validation failures and fatal errors
also contribute to the histogram; changes should be applied around the
builder.execute_transaction match and any other similar blocks (same pattern
around lines handling execute_transaction/commit_tx_apply_duration_seconds).
- Around line 318-373: Some non-EVM invalid exits (e.g., the
BlockGasLimitExceededBySequencerTransactions path and the blob/queue-index
rejection paths) return a PayloadBuilderError without incrementing the
malformed-L1 metric; update those return sites to call
self.metrics.l1_tx_strange_err_total.increment(1) immediately before returning.
Specifically, add the increment before the
Err(PayloadBuilderError::other(MorphPayloadBuilderError::BlockGasLimitExceededBySequencerTransactions
{ .. })) return and likewise before the returns that produce
PayloadBuilderError::other for the blob tx rejection / queue-index mismatch so
all malformed payload-attribute error paths report via
metrics.l1_tx_strange_err_total.
---
Duplicate comments:
In `@etc/README.md`:
- Around line 15-16: The README currently points only to upstream Docker docs;
update the sentence to reference the repository's local compose file (e.g.,
./docker-compose.yml) and add a short morph-reth-specific example command (for
example a one-liner like "docker compose -f ./docker-compose.yml up -d reth
grafana prometheus") alongside the existing upstream link so users have both
upstream docs and an immediate local usage example; modify the README.md line
that currently links to the upstream docs to include the local file reference
and the example command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8ca2fa9-bf3f-463a-8fc1-6f52dafe997e
📒 Files selected for processing (5)
crates/engine-api/src/builder.rscrates/payload/builder/src/builder.rscrates/payload/builder/src/metrics.rsetc/README.mdetc/docker-compose.yml
✅ Files skipped from review due to trivial changes (1)
- crates/engine-api/src/builder.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- etc/docker-compose.yml
- crates/payload/builder/src/metrics.rs
Rebuild the dashboard generator around domain-based rows so the unified dashboard stops duplicating sections and the default instance filter resolves to real metrics instead of an empty All selection. Constraint: Preserve source panel queries and field configs wherever possible Confidence: medium Scope-risk: narrow Not-tested: Pixel-level browser inspection of the rendered dashboard
- Merge morph-engine.json, overview.json, reth-mempool.json, reth-discovery.json, reth-state-growth.json into a single morph-reth.json with 10 domain-first rows and 131 panels - Remove standard beacon Engine API panels (FCU, newPayload, getBlobs, blob counters) and ExEx panels not used by Morph - Fix morph-engine.json Overview gridPos overlap (two panels at x=0,y=1); rearrange into proper 3-column layout - Add $interval variable to dashboard for state-growth panels - Rewrite merge-dashboards.py with declarative ROW_SOURCES, layout_panels(), and 6 structural assertions (no overlaps, unique IDs, unique row titles, correct row order) local-test/: - devnet.json: custom chainId=1337 genesis with 4 pre-funded test accounts (10k ETH each) + all 85 Morph system contracts - create-devnet.py: generates devnet.json from hoodi template - start-devnet.sh: launches 3-node P2P devnet (sequencer + 2 sync nodes) with trusted-peer wiring - produce-blocks.py: simulates morphnode driving sequencer engine API with real ETH-transfer transactions - docker-compose.monitoring.yml + prometheus.yml: local Prometheus+Grafana stack pointing at all 3 nodes with service/role/chain labels
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
local-test/merge-dashboards.py (1)
622-624: Specifyencoding="utf-8"for consistent file writing.The
load_jsonfunction correctly usesencoding="utf-8", but the output file write does not specify an encoding. For cross-platform consistency and to match the read behavior, specify the encoding explicitly.♻️ Suggested fix
- with OUTPUT_PATH.open("w") as handle: + with OUTPUT_PATH.open("w", encoding="utf-8") as handle: json.dump(dashboard, handle, indent=2) handle.write("\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-test/merge-dashboards.py` around lines 622 - 624, The output file is opened without specifying encoding which can cause cross-platform inconsistencies; update the write call that uses OUTPUT_PATH.open("w") to explicitly pass encoding="utf-8" (i.e., OUTPUT_PATH.open("w", encoding="utf-8")) so the file is written with the same UTF-8 encoding used by load_json and ensure json.dump(dashboard, handle, indent=2) and handle.write("\n") remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-test/produce-blocks.py`:
- Around line 205-219: The test is hand-rolling an invalid payload with fields
withdrawTrieRoot and nextL1MessageIndex which are not part of the
executable_l2_data schema, causing param decoding to fail before
engine_validateL2Block runs; instead build the negative test from the existing
data variable (use its structure and only schema fields) and send that via
rpc(ENGINE_URL, "engine_validateL2Block", ...) so the handler executes and
increments reth_morph_engine_validate_l2_block_failures_total; remove the
invalid keys (withdrawTrieRoot, nextL1MessageIndex) from the request and derive
the failing/invalid parts from data to produce a syntactically valid but
semantically failing test.
In `@local-test/start-devnet.sh`:
- Around line 198-202: The current command "docker compose -f
docker-compose.monitoring.yml up -d 2>&1 | grep -E \"Started|Running|Error\" ||
true" hides failures; remove the pipe to grep and the "|| true", run "docker
compose -f docker-compose.monitoring.yml up -d" directly, capture its exit code
($?), and if non-zero log the compose output/error and exit non-zero so the
script does not print the succeeding ok lines; only print the "ok" messages for
Grafana/Prometheus when the compose command succeeded.
- Around line 99-111: The PID variables (NODE1_PID/NODE2_PID/NODE3_PID) are
capturing the final pipeline command (tee) instead of the morph-reth process;
change the launch blocks for the node processes (the lines that run "$BINARY"
node ... 2>&1 | tee "$DATA_ROOT/nodeX/morph-reth.log" &) to use stdout/stderr
process substitution so $! refers to the morph-reth process (e.g., run "$BINARY"
node ... 2>&1 > >(tee "$DATA_ROOT/nodeX/morph-reth.log") & and then set
NODEX_PID=$!, and update the corresponding writes to $PID_FILE and
"$DATA_ROOT/nodeX/morph-reth.pid"); apply the same fix to the node2/node3 launch
blocks and ensure show_status()/stop_all() continue to use those PIDs; also
remove the "|| true" that follows the docker compose up -d | grep ... pipeline
so that docker compose failures are not masked and non-zero exits propagate.
In `@local-test/trigger-metrics.py`:
- Around line 81-86: The show helper currently only treats JSON-RPC errors as
failures; update show(label: str, result: dict) to also consider
result.get("result", {}).get("success") == False as a failure case so deliberate
handler-level failures (e.g., from engine_validateL2Block) print the red ✗ and
the failure detail instead of a green ok; locate the show function and augment
the conditional to check the nested "result.success" flag and include that
failure info in the error print path.
- Around line 154-169: The synthetic failure block `wrong_block` uses incorrect
ExecutableL2Data fields (`withdrawTrieRoot` and `nextL1MessageIndex`) so
decoding fails before reaching `engine_validateL2Block`/`engine_newL2Block`; fix
by using the correct shape: replace `withdrawTrieRoot` with `withdrawalsRoot`
(matching ExecutableL2Data) and remove `nextL1MessageIndex` (or rename it if
your runtime expects a different field), ensure `number`, `timestamp`,
`baseFeePerGas`, and other hex/string types match the real payload types, or
better yet obtain a real `engine_assembleL2Block` result and mutate only the
specific field you want to invalidate so the request decodes and the failure
counters are exercised.
---
Nitpick comments:
In `@local-test/merge-dashboards.py`:
- Around line 622-624: The output file is opened without specifying encoding
which can cause cross-platform inconsistencies; update the write call that uses
OUTPUT_PATH.open("w") to explicitly pass encoding="utf-8" (i.e.,
OUTPUT_PATH.open("w", encoding="utf-8")) so the file is written with the same
UTF-8 encoding used by load_json and ensure json.dump(dashboard, handle,
indent=2) and handle.write("\n") remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd9993a8-fd74-40b7-b5b2-99cd06f26828
📒 Files selected for processing (15)
.gitignoreetc/grafana/dashboards/morph-engine.jsonetc/grafana/dashboards/morph-reth.jsonlocal-test/accounts.jsonlocal-test/create-devnet.pylocal-test/devnet.jsonlocal-test/docker-compose.monitoring.ymllocal-test/grafana-provisioning/dashboards/default.ymllocal-test/grafana-provisioning/datasources/prometheus.ymllocal-test/merge-dashboards.pylocal-test/produce-blocks.pylocal-test/prometheus.ymllocal-test/run-test.shlocal-test/start-devnet.shlocal-test/trigger-metrics.py
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- local-test/accounts.json
- local-test/grafana-provisioning/dashboards/default.yml
- local-test/grafana-provisioning/datasources/prometheus.yml
- local-test/prometheus.yml
- local-test/docker-compose.monitoring.yml
- etc/grafana/dashboards/morph-engine.json
Comment on lines
+205
to
+219
| rpc(ENGINE_URL, "engine_validateL2Block", [{ | ||
| "parentHash":"0x" + "00" * 32, | ||
| "miner": "0x" + "00" * 20, | ||
| "number": hex(cur + 1), | ||
| "gasLimit": "0x1c9c380", | ||
| "timestamp": hex(int(time.time())), | ||
| "transactions": [], | ||
| "stateRoot": "0x" + "00" * 32, | ||
| "gasUsed": "0x0", | ||
| "receiptsRoot":"0x" + "00" * 32, | ||
| "logsBloom": "0x" + "00" * 256, | ||
| "withdrawTrieRoot":"0x" + "00" * 32, | ||
| "nextL1MessageIndex": 0, | ||
| "hash": "0x" + "00" * 31 + "01", | ||
| }], J()) |
There was a problem hiding this comment.
Build the negative test from data instead of hand-rolling an invalid schema.
withdrawTrieRoot and nextL1MessageIndex are not fields of crates/payload/types/src/executable_l2_data.rs:1-80. If this request fails during param decoding, the engine_validateL2Block handler never runs and reth_morph_engine_validate_l2_block_failures_total may not move, even though this loop claims it is triggering the failure counter.
Suggested fix
- rpc(ENGINE_URL, "engine_validateL2Block", [{
- "parentHash":"0x" + "00" * 32,
- "miner": "0x" + "00" * 20,
- "number": hex(cur + 1),
- "gasLimit": "0x1c9c380",
- "timestamp": hex(int(time.time())),
- "transactions": [],
- "stateRoot": "0x" + "00" * 32,
- "gasUsed": "0x0",
- "receiptsRoot":"0x" + "00" * 32,
- "logsBloom": "0x" + "00" * 256,
- "withdrawTrieRoot":"0x" + "00" * 32,
- "nextL1MessageIndex": 0,
- "hash": "0x" + "00" * 31 + "01",
- }], J())
+ bad_validate = {
+ **data,
+ "parentHash": "0x" + "00" * 32,
+ "hash": "0x" + "00" * 31 + "01",
+ }
+ rpc(ENGINE_URL, "engine_validateL2Block", [bad_validate], J())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rpc(ENGINE_URL, "engine_validateL2Block", [{ | |
| "parentHash":"0x" + "00" * 32, | |
| "miner": "0x" + "00" * 20, | |
| "number": hex(cur + 1), | |
| "gasLimit": "0x1c9c380", | |
| "timestamp": hex(int(time.time())), | |
| "transactions": [], | |
| "stateRoot": "0x" + "00" * 32, | |
| "gasUsed": "0x0", | |
| "receiptsRoot":"0x" + "00" * 32, | |
| "logsBloom": "0x" + "00" * 256, | |
| "withdrawTrieRoot":"0x" + "00" * 32, | |
| "nextL1MessageIndex": 0, | |
| "hash": "0x" + "00" * 31 + "01", | |
| }], J()) | |
| bad_validate = { | |
| **data, | |
| "parentHash": "0x" + "00" * 32, | |
| "hash": "0x" + "00" * 31 + "01", | |
| } | |
| rpc(ENGINE_URL, "engine_validateL2Block", [bad_validate], J()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/produce-blocks.py` around lines 205 - 219, The test is
hand-rolling an invalid payload with fields withdrawTrieRoot and
nextL1MessageIndex which are not part of the executable_l2_data schema, causing
param decoding to fail before engine_validateL2Block runs; instead build the
negative test from the existing data variable (use its structure and only schema
fields) and send that via rpc(ENGINE_URL, "engine_validateL2Block", ...) so the
handler executes and increments
reth_morph_engine_validate_l2_block_failures_total; remove the invalid keys
(withdrawTrieRoot, nextL1MessageIndex) from the request and derive the
failing/invalid parts from data to produce a syntactically valid but
semantically failing test.
Comment on lines
+99
to
+111
| "$BINARY" node \ | ||
| --chain "$GENESIS" \ | ||
| --datadir "$DATA_ROOT/node1" \ | ||
| --metrics 127.0.0.1:9001 \ | ||
| --authrpc.addr 127.0.0.1 --authrpc.port 8551 --authrpc.jwtsecret "$JWT_FILE" \ | ||
| --http --http.addr 127.0.0.1 --http.port 8545 \ | ||
| --http.api "eth,net,web3,txpool,debug" \ | ||
| --port 30303 \ | ||
| --log.stdout.format terminal \ | ||
| 2>&1 | tee "$DATA_ROOT/node1/morph-reth.log" & | ||
| NODE1_PID=$! | ||
| echo "$NODE1_PID" >> "$PID_FILE" | ||
| echo "$NODE1_PID" > "$DATA_ROOT/node1/morph-reth.pid" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Relevant launch lines:"
rg -n -C2 'tee "\$DATA_ROOT/node[123]/morph-reth\.log"|NODE[123]_PID=\$!' local-test/start-devnet.sh
echo
echo "Bash pipeline demo:"
sleep 30 | cat >/dev/null &
pid=$!
ps -o pid=,ppid=,comm=,args= -p "$pid"
kill "$pid" 2>/dev/null || true
wait "$pid" 2>/dev/null || true
echo
echo "Expected result: the printed command is the last pipeline stage (here: cat),"
echo "which is why storing \$! after '| tee ... &' does not give you the morph-reth PID."Repository: morph-l2/morph-reth
Length of output: 870
🏁 Script executed:
#!/bin/bash
echo "=== Examining stop_all() function ==="
rg -A 20 'stop_all\(\)' local-test/start-devnet.sh
echo
echo "=== Examining show_status() function ==="
rg -A 20 'show_status\(\)' local-test/start-devnet.sh
echo
echo "=== Checking all NODE*_PID references ==="
rg -n 'NODE[123]_PID' local-test/start-devnet.shRepository: morph-l2/morph-reth
Length of output: 1836
🏁 Script executed:
#!/bin/bash
echo "=== Docker Compose startup section ==="
rg -n -B 5 -A 5 'docker compose up' local-test/start-devnet.sh
echo
echo "=== Full cleanup/teardown context ==="
rg -n -B 3 -A 10 'cleanup\|teardown' local-test/start-devnet.shRepository: morph-l2/morph-reth
Length of output: 140
🏁 Script executed:
#!/bin/bash
echo "=== Content around lines 190-210 ==="
sed -n '190,210p' local-test/start-devnet.sh
echo
echo "=== Search for any docker command ==="
rg -n 'docker' local-test/start-devnet.sh
echo
echo "=== Search for docker-compose ==="
rg -n 'docker-compose' local-test/start-devnet.sh
echo
echo "=== Full file length ==="
wc -l local-test/start-devnet.shRepository: morph-l2/morph-reth
Length of output: 1311
Capture the morph-reth PID, not the tee pipeline PID.
These assignments persist $! after a backgrounded pipeline, so NODE*_PID tracks tee (the last pipeline command) instead of the node process. The stored PIDs are written to both $PID_FILE and individual .pid files. While stop_all() has a pkill fallback to catch actual processes, show_status() relies solely on checking those stored PIDs with kill -0—once tee exits (which happens immediately), the function will report nodes as "dead" even though morph-reth instances are still running.
Suggested fix
"$BINARY" node \
--chain "$GENESIS" \
--datadir "$DATA_ROOT/node1" \
--metrics 127.0.0.1:9001 \
--authrpc.addr 127.0.0.1 --authrpc.port 8551 --authrpc.jwtsecret "$JWT_FILE" \
--http --http.addr 127.0.0.1 --http.port 8545 \
--http.api "eth,net,web3,txpool,debug" \
--port 30303 \
--log.stdout.format terminal \
- 2>&1 | tee "$DATA_ROOT/node1/morph-reth.log" &
+ > "$DATA_ROOT/node1/morph-reth.log" 2>&1 &
NODE1_PID=$!Apply the same change to the node2/node3 launch blocks at lines 152–166 and 170–184.
Avoid masking docker compose startup failures. Line 200 pipes docker compose up -d through grep with || true, which masks startup failures when the output doesn't match the expected keywords. Remove the || true so that compose failures are visible and prevent the unconditional success messages that follow from being printed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "$BINARY" node \ | |
| --chain "$GENESIS" \ | |
| --datadir "$DATA_ROOT/node1" \ | |
| --metrics 127.0.0.1:9001 \ | |
| --authrpc.addr 127.0.0.1 --authrpc.port 8551 --authrpc.jwtsecret "$JWT_FILE" \ | |
| --http --http.addr 127.0.0.1 --http.port 8545 \ | |
| --http.api "eth,net,web3,txpool,debug" \ | |
| --port 30303 \ | |
| --log.stdout.format terminal \ | |
| 2>&1 | tee "$DATA_ROOT/node1/morph-reth.log" & | |
| NODE1_PID=$! | |
| echo "$NODE1_PID" >> "$PID_FILE" | |
| echo "$NODE1_PID" > "$DATA_ROOT/node1/morph-reth.pid" | |
| "$BINARY" node \ | |
| --chain "$GENESIS" \ | |
| --datadir "$DATA_ROOT/node1" \ | |
| --metrics 127.0.0.1:9001 \ | |
| --authrpc.addr 127.0.0.1 --authrpc.port 8551 --authrpc.jwtsecret "$JWT_FILE" \ | |
| --http --http.addr 127.0.0.1 --http.port 8545 \ | |
| --http.api "eth,net,web3,txpool,debug" \ | |
| --port 30303 \ | |
| --log.stdout.format terminal \ | |
| > "$DATA_ROOT/node1/morph-reth.log" 2>&1 & | |
| NODE1_PID=$! | |
| echo "$NODE1_PID" >> "$PID_FILE" | |
| echo "$NODE1_PID" > "$DATA_ROOT/node1/morph-reth.pid" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/start-devnet.sh` around lines 99 - 111, The PID variables
(NODE1_PID/NODE2_PID/NODE3_PID) are capturing the final pipeline command (tee)
instead of the morph-reth process; change the launch blocks for the node
processes (the lines that run "$BINARY" node ... 2>&1 | tee
"$DATA_ROOT/nodeX/morph-reth.log" &) to use stdout/stderr process substitution
so $! refers to the morph-reth process (e.g., run "$BINARY" node ... 2>&1 >
>(tee "$DATA_ROOT/nodeX/morph-reth.log") & and then set NODEX_PID=$!, and update
the corresponding writes to $PID_FILE and "$DATA_ROOT/nodeX/morph-reth.pid");
apply the same fix to the node2/node3 launch blocks and ensure
show_status()/stop_all() continue to use those PIDs; also remove the "|| true"
that follows the docker compose up -d | grep ... pipeline so that docker compose
failures are not masked and non-zero exits propagate.
Comment on lines
+81
to
+86
| def show(label: str, result: dict): | ||
| if "error" in result and "result" not in result: | ||
| print(f" {RED}✗ {label}: {result['error']}{RESET}") | ||
| else: | ||
| print(f" {GREEN}✓ {label}: ok{RESET}") | ||
| return result |
There was a problem hiding this comment.
Treat result.success == false as a failed smoke-test step.
engine_validateL2Block reports handler-level failure via {"result":{"success":false}}, not necessarily via a JSON-RPC error. With the current helper, the deliberate failure paths at Lines 152-173 still print a green ok, so the script can look healthy even when the failure-path instrumentation is not.
Suggested fix
-def show(label: str, result: dict):
- if "error" in result and "result" not in result:
- print(f" {RED}✗ {label}: {result['error']}{RESET}")
- else:
- print(f" {GREEN}✓ {label}: ok{RESET}")
+def show(label: str, result: dict):
+ failed = "error" in result and "result" not in result
+ if not failed and isinstance(result.get("result"), dict):
+ failed = result["result"].get("success") is False
+
+ if failed:
+ print(f" {RED}✗ {label}: {result.get('error', result.get('result'))}{RESET}")
+ else:
+ print(f" {GREEN}✓ {label}: ok{RESET}")
return result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def show(label: str, result: dict): | |
| if "error" in result and "result" not in result: | |
| print(f" {RED}✗ {label}: {result['error']}{RESET}") | |
| else: | |
| print(f" {GREEN}✓ {label}: ok{RESET}") | |
| return result | |
| def show(label: str, result: dict): | |
| failed = "error" in result and "result" not in result | |
| if not failed and isinstance(result.get("result"), dict): | |
| failed = result["result"].get("success") is False | |
| if failed: | |
| print(f" {RED}✗ {label}: {result.get('error', result.get('result'))}{RESET}") | |
| else: | |
| print(f" {GREEN}✓ {label}: ok{RESET}") | |
| return result |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/trigger-metrics.py` around lines 81 - 86, The show helper
currently only treats JSON-RPC errors as failures; update show(label: str,
result: dict) to also consider result.get("result", {}).get("success") == False
as a failure case so deliberate handler-level failures (e.g., from
engine_validateL2Block) print the red ✗ and the failure detail instead of a
green ok; locate the show function and augment the conditional to check the
nested "result.success" flag and include that failure info in the error print
path.
Comment on lines
+154
to
+169
| wrong_block = { | ||
| "parentHash": "0x0000000000000000000000000000000000000000000000000000000000000000", | ||
| "miner": "0x0000000000000000000000000000000000000000", | ||
| "number": hex(next_block), | ||
| "gasLimit": "0x5f5e100", | ||
| "baseFeePerGas": "0x1", | ||
| "timestamp": hex(int(time.time())), | ||
| "transactions": [], | ||
| "stateRoot": "0x0000000000000000000000000000000000000000000000000000000000000000", | ||
| "gasUsed": "0x0", | ||
| "receiptsRoot": "0x0000000000000000000000000000000000000000000000000000000000000000", | ||
| "logsBloom": "0x" + "00" * 256, | ||
| "withdrawTrieRoot": "0x0000000000000000000000000000000000000000000000000000000000000000", | ||
| "nextL1MessageIndex": 0, | ||
| "hash": "0x0000000000000000000000000000000000000000000000000000000000000001", | ||
| } |
There was a problem hiding this comment.
Use a valid ExecutableL2Data shape for the synthetic failure block.
This payload includes withdrawTrieRoot and nextL1MessageIndex, but crates/payload/types/src/executable_l2_data.rs:1-80 exposes withdrawalsRoot and no nextL1MessageIndex. If request decoding fails here, engine_validateL2Block and the downstream engine_newL2Block call at Line 179 never reach the instrumented handlers, so the *_failures_total counters this script is trying to exercise stay unchanged.
Suggested fix
wrong_block = {
"parentHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"miner": "0x0000000000000000000000000000000000000000",
"number": hex(next_block),
"gasLimit": "0x5f5e100",
"baseFeePerGas": "0x1",
"timestamp": hex(int(time.time())),
"transactions": [],
"stateRoot": "0x0000000000000000000000000000000000000000000000000000000000000000",
"gasUsed": "0x0",
"receiptsRoot": "0x0000000000000000000000000000000000000000000000000000000000000000",
"logsBloom": "0x" + "00" * 256,
- "withdrawTrieRoot": "0x0000000000000000000000000000000000000000000000000000000000000000",
- "nextL1MessageIndex": 0,
+ "withdrawalsRoot": "0x0000000000000000000000000000000000000000000000000000000000000000",
"hash": "0x0000000000000000000000000000000000000000000000000000000000000001",
}Even better: start from a real engine_assembleL2Block result and mutate only the field you want to invalidate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/trigger-metrics.py` around lines 154 - 169, The synthetic failure
block `wrong_block` uses incorrect ExecutableL2Data fields (`withdrawTrieRoot`
and `nextL1MessageIndex`) so decoding fails before reaching
`engine_validateL2Block`/`engine_newL2Block`; fix by using the correct shape:
replace `withdrawTrieRoot` with `withdrawalsRoot` (matching ExecutableL2Data)
and remove `nextL1MessageIndex` (or rename it if your runtime expects a
different field), ensure `number`, `timestamp`, `baseFeePerGas`, and other
hex/string types match the real payload types, or better yet obtain a real
`engine_assembleL2Block` result and mutate only the specific field you want to
invalidate so the request decodes and the failure counters are exercised.
…ole source
Drop the Python-based dashboard merge pipeline and all five upstream reth
source dashboards. morph-reth.json is now the single hand-maintained
dashboard; no build step sits between source and deploy.
Dashboard fixes (applied directly to morph-reth.json, 131 -> 120 panels):
- Remove panels that are architecturally inapplicable to Morph:
- Cargo Features (reth_info.cargo_features is empty in release builds)
- Stage checkpoints, Sync progress (%), Sync progress (block number)
(Morph drives blocks through engine API; reth staged P2P sync never runs)
- Reth Builder Active/Initiated/Failed Jobs (duplicated by Morph Engine API
metrics; active/failed are always ~0 on Morph's synchronous build path)
- Discv5 Peers / Peer Churn / Advertised Network Stacks (Morph runs with
bootnodes + Discv4; `--enable-discv5-discovery` is never set)
- Merge 5 redundant build-info stat panels into one Build Info strip at
the top of the Overview row. Avoids the Grafana stat-at-w=5 text
alignment glitch and survives multi-node Instance=All without showing
N copies of the same value.
- Split Storage size into 4 independent stat panels (Database / Freelist
/ Static Files / Total) with manual gridPos. Grafana's stat-panel
orientation auto-detect refused to keep them side-by-side when packed
into one panel at w=12, so per-tile gridPos is the only reliable layout.
- Convert Connected peers from gauge to timeseries. Gauges render one
semicircle per node and overflow the panel with raw label strings;
timeseries + `{{service}}` legend scales cleanly to any node count.
- Enable `instance` template variable multi-select + All by rewriting
all `$instance_label="$instance"` exact matches to regex (`=~`). Stat
build-info panels now aggregate with `max by (<label>)` so identical
values across nodes collapse to one series.
- Repack Overview row gridPos so every grid row fills all 24 columns.
Fix the TxPool & P2P y=388 row tail gap left by the Discv5 removal.
- Fix remaining stat-panel text alignment: justifyMode=center,
wideLayout=false, graphMode=none on the panels that had the upstream
auto-right-align default.
Dev-time tooling (local-test/, not part of runtime deploy):
- audit-dashboards.py: structural audit of morph-reth.json
(unique panel ids, no grid overlap, custom-metric coverage vs
crates/engine-api/src/metrics.rs + crates/payload/builder/src/metrics.rs)
- per-panel-audit.py: full 120-panel status matrix, runs every expr
against Prometheus to classify OK / Empty / Error
- query-panels.py: PromQL batch verification with All-mode variable
substitution
- stress-multiproof.py: stress driver that sends 500 tx/block to random
addresses and mirrors each built block to full nodes via engine
API, simulating what morph-node does in production. Exercises the
`reth_tree_root_multiproof_task_*` / StateRootTask metric family
which only populates on block-validation paths.
- metrics-test-report.md: end-to-end test findings + per-issue decisions
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
local-test/query-panels.py (1)
27-31: Minor: Inconsistent Prometheus URL handling across scripts.This script uses
PROMETHEUSenv var whileper-panel-audit.pyhardcodes the URL. Consider aligning the approach—either both configurable via the same env var, or both hardcoded for local-only use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-test/query-panels.py` around lines 27 - 31, The Prometheus URL handling is inconsistent: this file reads PROMETHEUS env var into PROM (variable PROM) while per-panel-audit.py hardcodes the URL; make them consistent by switching per-panel-audit.py to read os.environ.get("PROMETHEUS", "http://127.0.0.1:9090") (or conversely change this file to hardcode the same literal), and ensure both scripts use the same env var name "PROMETHEUS" and default value so callers can configure PROM via environment consistently.local-test/audit-dashboards.py (1)
169-177: Consider usingCounterfor O(n) duplicate detection.The current approach using
list.count()in a comprehension is O(n²). For the scale of this dashboard (131 panels), it's fine, butCounterwould be more efficient and idiomatic.Suggested improvement
+from collections import Counter, defaultdict -from collections import defaultdict - dup_ids = [x for x in set(all_ids) if all_ids.count(x) > 1] + id_counts = Counter(all_ids) + dup_ids = [x for x, c in id_counts.items() if c > 1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-test/audit-dashboards.py` around lines 169 - 177, Replace the O(n²) duplicate detection that uses list.count() with a Counter-based O(n) approach: use collections.Counter on all_ids to find keys with count>1 and build dup_ids, and similarly use Counter on titles inside the loop over rows to compute dup_titles; update the blocks that set dup_ids and dup_titles (referencing variables all_ids, dup_ids, rows, titles, dup_titles) and keep appending the same errors/warnings to errors and warnings respectively.local-test/per-panel-audit.py (1)
33-50: Consider extracting shared utilities to reduce duplication.
VAR_SUBShere is nearly identical toVAR_REPLACEMENTSinquery-panels.py. Both scripts also share similar panel iteration and Prometheus query logic. For maintainability, consider extracting common utilities to a shared module.For example, create
local-test/dashboard_utils.pywith:
- Variable substitution dictionary and function
- Panel iteration helpers
- Prometheus query wrapper
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-test/per-panel-audit.py` around lines 33 - 50, Extract the duplicated variable substitutions and panel/query helpers into a new module (e.g., local-test/dashboard_utils.py): move VAR_SUBS into that module (rename to VAR_REPLACEMENTS or SHARED_VAR_SUBS) and export a substitution function, a panel-iteration helper used by both local-test/per-panel-audit.py and query-panels.py, and a thin Prometheus query wrapper; then update per-panel-audit.py and query-panels.py to import the shared VAR_REPLACEMENTS/substitute_vars function, the panel iteration helper, and the query wrapper instead of duplicating the VAR_SUBS dict and query logic so both scripts call the same utilities (reference symbols: VAR_SUBS, VAR_REPLACEMENTS, per-panel-audit.py panel loop, query-panels.py).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-test/audit-dashboards.py`:
- Around line 108-116: The builtins set contains a duplicated "__name" entry;
update the builtins set definition (the builtins variable) to remove the
redundant "__name" so each literal appears only once, then run tests/lint to
confirm the duplicate warning is resolved; verify the rest of the set members
remain unchanged and that the missing computation (used - declared - builtins)
still behaves as before.
In `@local-test/per-panel-audit.py`:
- Around line 160-176: The second value returned by classify_panel() is assigned
to n but never used; change the unpacking in the loop over panels (for p in
panels:) to prefix that unused variable with an underscore (e.g., replace
"status, n, note = classify_panel(p)" with "status, _n, note =
classify_panel(p)" or "status, _, note = classify_panel(p)") so linters know
it’s intentionally ignored while preserving the existing status and note
variables and the rest of the logic that uses p, g, summary_ok/empty/error,
icon, and out.
In `@local-test/query-panels.py`:
- Around line 78-91: The local variable `rows` in function extract_row_of_panel
is unused and should be removed; edit the extract_row_of_panel function to
delete the `rows = []` declaration so only `current` and the iteration logic
remain (preserving yields and behavior for row-type panels and nested panels).
In `@local-test/stress-multiproof.py`:
- Around line 200-201: The code currently assumes accounts =
[Account.from_key(a["key"]) for a in json.load(open(ACCOUNTS_FILE))] will
produce a non-empty list and then uses sender = accounts[0]; guard against an
empty or malformed ACCOUNTS_FILE by validating the JSON load and the resulting
list before indexing. Specifically, catch JSONDecodeError when loading
ACCOUNTS_FILE, ensure the loaded value is a list and not empty, and if it is
empty or invalid raise/exit with a clear message (e.g., "no accounts found in
ACCOUNTS_FILE" or "invalid accounts.json") rather than allowing an IndexError;
update the code paths around Account.from_key and sender to run only after the
validation succeeds.
- Around line 97-109: The helper functions get_head, get_chain_id, and get_nonce
currently default to hardcoded hex strings when rpc(...) returns no "result",
which hides RPC failures; modify each function to check the rpc response for an
error or missing "result" and raise an exception (e.g., RuntimeError) containing
the full rpc response and a descriptive message instead of returning a default
value, so callers immediately see connectivity/auth errors; keep the existing
int(..., 16) parsing for successful responses and reference the functions
get_head, get_chain_id, and get_nonce when making the change.
- Around line 83-90: Validate and restrict URL schemes to only http and https
before opening or requesting any configured endpoint: in rpc(), parse the
provided url (and similarly in metrics_snapshot()) and raise or skip if its
scheme is not "http" or "https"; also validate each FULL_NODES entry's "url" and
"metrics" fields the same way before using them. Use urllib.parse.urlparse to
inspect scheme and enforce an allowlist {"http","https"}, returning a clear
error (or logging and ignoring invalid entries) instead of passing unsafe
schemes into urllib.request.urlopen. Ensure this validation is applied where
ENGINE_URL, HTTP_URL, METRICS_URL or node.url/node.metrics are consumed so no
non-http(s) schemes reach network-open calls.
---
Nitpick comments:
In `@local-test/audit-dashboards.py`:
- Around line 169-177: Replace the O(n²) duplicate detection that uses
list.count() with a Counter-based O(n) approach: use collections.Counter on
all_ids to find keys with count>1 and build dup_ids, and similarly use Counter
on titles inside the loop over rows to compute dup_titles; update the blocks
that set dup_ids and dup_titles (referencing variables all_ids, dup_ids, rows,
titles, dup_titles) and keep appending the same errors/warnings to errors and
warnings respectively.
In `@local-test/per-panel-audit.py`:
- Around line 33-50: Extract the duplicated variable substitutions and
panel/query helpers into a new module (e.g., local-test/dashboard_utils.py):
move VAR_SUBS into that module (rename to VAR_REPLACEMENTS or SHARED_VAR_SUBS)
and export a substitution function, a panel-iteration helper used by both
local-test/per-panel-audit.py and query-panels.py, and a thin Prometheus query
wrapper; then update per-panel-audit.py and query-panels.py to import the shared
VAR_REPLACEMENTS/substitute_vars function, the panel iteration helper, and the
query wrapper instead of duplicating the VAR_SUBS dict and query logic so both
scripts call the same utilities (reference symbols: VAR_SUBS, VAR_REPLACEMENTS,
per-panel-audit.py panel loop, query-panels.py).
In `@local-test/query-panels.py`:
- Around line 27-31: The Prometheus URL handling is inconsistent: this file
reads PROMETHEUS env var into PROM (variable PROM) while per-panel-audit.py
hardcodes the URL; make them consistent by switching per-panel-audit.py to read
os.environ.get("PROMETHEUS", "http://127.0.0.1:9090") (or conversely change this
file to hardcode the same literal), and ensure both scripts use the same env var
name "PROMETHEUS" and default value so callers can configure PROM via
environment consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c468fcd-8cb6-4f94-a1c9-8503192888b9
📒 Files selected for processing (7)
etc/grafana/dashboards/morph-reth.jsonlocal-test/.gitignorelocal-test/audit-dashboards.pylocal-test/metrics-test-report.mdlocal-test/per-panel-audit.pylocal-test/query-panels.pylocal-test/stress-multiproof.py
✅ Files skipped from review due to trivial changes (1)
- local-test/.gitignore
Comment on lines
+108
to
+116
| builtins = { | ||
| "__interval", "__interval_ms", "__range", "__range_s", | ||
| "__range_ms", "__rate_interval", "__timeFilter", | ||
| "__auto_interval", "__from", "__to", "__name", | ||
| "__user", "__org", "__dashboard", "__user_id", | ||
| "__auto", "__name", "__data", | ||
| } | ||
| missing = sorted(used - declared - builtins) | ||
| return missing |
There was a problem hiding this comment.
Remove duplicate "__name" entry in builtins set.
The set contains "__name" twice (lines 111 and 113), which is redundant and flagged by static analysis.
Proposed fix
builtins = {
"__interval", "__interval_ms", "__range", "__range_s",
"__range_ms", "__rate_interval", "__timeFilter",
"__auto_interval", "__from", "__to", "__name",
"__user", "__org", "__dashboard", "__user_id",
- "__auto", "__name", "__data",
+ "__auto", "__data",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| builtins = { | |
| "__interval", "__interval_ms", "__range", "__range_s", | |
| "__range_ms", "__rate_interval", "__timeFilter", | |
| "__auto_interval", "__from", "__to", "__name", | |
| "__user", "__org", "__dashboard", "__user_id", | |
| "__auto", "__name", "__data", | |
| } | |
| missing = sorted(used - declared - builtins) | |
| return missing | |
| builtins = { | |
| "__interval", "__interval_ms", "__range", "__range_s", | |
| "__range_ms", "__rate_interval", "__timeFilter", | |
| "__auto_interval", "__from", "__to", "__name", | |
| "__user", "__org", "__dashboard", "__user_id", | |
| "__auto", "__data", | |
| } | |
| missing = sorted(used - declared - builtins) | |
| return missing |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 113-113: Sets should not contain duplicate item "__name"
Remove duplicate item
(B033)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/audit-dashboards.py` around lines 108 - 116, The builtins set
contains a duplicated "__name" entry; update the builtins set definition (the
builtins variable) to remove the redundant "__name" so each literal appears only
once, then run tests/lint to confirm the duplicate warning is resolved; verify
the rest of the set members remain unchanged and that the missing computation
(used - declared - builtins) still behaves as before.
Comment on lines
+160
to
+176
| for p in panels: | ||
| g = p["gridPos"] | ||
| status, n, note = classify_panel(p) | ||
| if status == "OK": | ||
| summary_ok += 1 | ||
| icon = "✅" | ||
| elif status == "EMPTY": | ||
| summary_empty += 1 | ||
| icon = "⭕" | ||
| else: | ||
| summary_error += 1 | ||
| icon = "❌" | ||
| out.append( | ||
| f"| {p['id']} | {p['title']} | {p['type']} | " | ||
| f"x={g['x']},y={g['y']} | {g['w']}×{g['h']} | " | ||
| f"{icon} {status} | {note} |" | ||
| ) |
There was a problem hiding this comment.
Prefix unused variable with underscore.
The n variable from classify_panel() is unpacked but never used. Prefix it with _ to indicate it's intentionally ignored.
Proposed fix
- status, n, note = classify_panel(p)
+ status, _n, note = classify_panel(p)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for p in panels: | |
| g = p["gridPos"] | |
| status, n, note = classify_panel(p) | |
| if status == "OK": | |
| summary_ok += 1 | |
| icon = "✅" | |
| elif status == "EMPTY": | |
| summary_empty += 1 | |
| icon = "⭕" | |
| else: | |
| summary_error += 1 | |
| icon = "❌" | |
| out.append( | |
| f"| {p['id']} | {p['title']} | {p['type']} | " | |
| f"x={g['x']},y={g['y']} | {g['w']}×{g['h']} | " | |
| f"{icon} {status} | {note} |" | |
| ) | |
| for p in panels: | |
| g = p["gridPos"] | |
| status, _n, note = classify_panel(p) | |
| if status == "OK": | |
| summary_ok += 1 | |
| icon = "✅" | |
| elif status == "EMPTY": | |
| summary_empty += 1 | |
| icon = "⭕" | |
| else: | |
| summary_error += 1 | |
| icon = "❌" | |
| out.append( | |
| f"| {p['id']} | {p['title']} | {p['type']} | " | |
| f"x={g['x']},y={g['y']} | {g['w']}×{g['h']} | " | |
| f"{icon} {status} | {note} |" | |
| ) |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 162-162: Unpacked variable n is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 174-174: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/per-panel-audit.py` around lines 160 - 176, The second value
returned by classify_panel() is assigned to n but never used; change the
unpacking in the loop over panels (for p in panels:) to prefix that unused
variable with an underscore (e.g., replace "status, n, note = classify_panel(p)"
with "status, _n, note = classify_panel(p)" or "status, _, note =
classify_panel(p)") so linters know it’s intentionally ignored while preserving
the existing status and note variables and the rest of the logic that uses p, g,
summary_ok/empty/error, icon, and out.
Comment on lines
+78
to
+91
| def extract_row_of_panel(dashboard): | ||
| """Yield (row_title, panel_dict) for every non-row panel in order.""" | ||
| rows = [] | ||
| current = None | ||
| for p in dashboard["panels"]: | ||
| if p.get("type") == "row": | ||
| current = p["title"] | ||
| for nested in p.get("panels", []): | ||
| if nested.get("type") != "row": | ||
| yield (current, nested) | ||
| continue | ||
| if current is not None: | ||
| yield (current, p) | ||
|
|
There was a problem hiding this comment.
Remove unused rows variable.
The rows list is defined at line 80 but never used. This appears to be leftover code.
Proposed fix
def extract_row_of_panel(dashboard):
"""Yield (row_title, panel_dict) for every non-row panel in order."""
- rows = []
current = None
for p in dashboard["panels"]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def extract_row_of_panel(dashboard): | |
| """Yield (row_title, panel_dict) for every non-row panel in order.""" | |
| rows = [] | |
| current = None | |
| for p in dashboard["panels"]: | |
| if p.get("type") == "row": | |
| current = p["title"] | |
| for nested in p.get("panels", []): | |
| if nested.get("type") != "row": | |
| yield (current, nested) | |
| continue | |
| if current is not None: | |
| yield (current, p) | |
| def extract_row_of_panel(dashboard): | |
| """Yield (row_title, panel_dict) for every non-row panel in order.""" | |
| current = None | |
| for p in dashboard["panels"]: | |
| if p.get("type") == "row": | |
| current = p["title"] | |
| for nested in p.get("panels", []): | |
| if nested.get("type") != "row": | |
| yield (current, nested) | |
| continue | |
| if current is not None: | |
| yield (current, p) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/query-panels.py` around lines 78 - 91, The local variable `rows`
in function extract_row_of_panel is unused and should be removed; edit the
extract_row_of_panel function to delete the `rows = []` declaration so only
`current` and the iteration logic remain (preserving yields and behavior for
row-type panels and nested panels).
Comment on lines
+83
to
+90
| def rpc(url: str, method: str, params: list, jwt: str | None = None) -> dict: | ||
| body = json.dumps({"jsonrpc": "2.0", "id": 1, "method": method, "params": params}).encode() | ||
| headers = {"Content-Type": "application/json"} | ||
| if jwt: | ||
| headers["Authorization"] = f"Bearer {jwt}" | ||
| req = urllib.request.Request(url, data=body, headers=headers) | ||
| try: | ||
| return json.loads(urllib.request.urlopen(req, timeout=30).read()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify URL sources and network open callsites.
rg -n 'ENGINE_URL|HTTP_URL|METRICS_URL|FULL_NODES|urlopen|Request\(' local-test/stress-multiproof.py
# Verify whether scheme allowlisting exists.
rg -n 'urlparse|allowed_schemes|scheme not in|startswith\("http://"\)|startswith\("https://"\)' local-test/stress-multiproof.py || trueRepository: morph-l2/morph-reth
Length of output: 1351
🏁 Script executed:
cd local-test && cat -n stress-multiproof.py | head -100Repository: morph-l2/morph-reth
Length of output: 4488
🏁 Script executed:
cd local-test && sed -n '130,145p' stress-multiproof.pyRepository: morph-l2/morph-reth
Length of output: 585
Restrict endpoint schemes to http/https before opening URLs.
ENGINE_URL, HTTP_URL, METRICS_URL are environment-configurable and passed to urllib without scheme validation at lines 83-90 (rpc()) and 137-139 (metrics_snapshot()). Additionally, FULL_NODES entries contain unvalidated url and metrics fields. Add an allowlist check to prevent unexpected schemes (e.g., file:, ftp:).
Proposed hardening
import urllib.error
import urllib.request
+from urllib.parse import urlparse
@@
+def _validate_http_url(url: str, label: str) -> None:
+ scheme = urlparse(url).scheme.lower()
+ if scheme not in {"http", "https"}:
+ raise ValueError(f"{label} must use http/https, got: {url}")
+
@@
def main() -> None:
@@
+ _validate_http_url(ENGINE_URL, "ENGINE_URL")
+ _validate_http_url(HTTP_URL, "HTTP_URL")
+ _validate_http_url(METRICS_URL, "METRICS_URL")
+ for f in FULL_NODES:
+ _validate_http_url(f["url"], f'{f["name"]}.url')
+ _validate_http_url(f["metrics"], f'{f["name"]}.metrics')🧰 Tools
🪛 Ruff (0.15.10)
[error] 88-88: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 90-90: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/stress-multiproof.py` around lines 83 - 90, Validate and restrict
URL schemes to only http and https before opening or requesting any configured
endpoint: in rpc(), parse the provided url (and similarly in metrics_snapshot())
and raise or skip if its scheme is not "http" or "https"; also validate each
FULL_NODES entry's "url" and "metrics" fields the same way before using them.
Use urllib.parse.urlparse to inspect scheme and enforce an allowlist
{"http","https"}, returning a clear error (or logging and ignoring invalid
entries) instead of passing unsafe schemes into urllib.request.urlopen. Ensure
this validation is applied where ENGINE_URL, HTTP_URL, METRICS_URL or
node.url/node.metrics are consumed so no non-http(s) schemes reach network-open
calls.
Comment on lines
+97
to
+109
| def get_head() -> int: | ||
| r = rpc(HTTP_URL, "eth_blockNumber", []) | ||
| return int(r.get("result", "0x0"), 16) | ||
|
|
||
|
|
||
| def get_chain_id() -> int: | ||
| r = rpc(HTTP_URL, "eth_chainId", []) | ||
| return int(r.get("result", "0x539"), 16) | ||
|
|
||
|
|
||
| def get_nonce(address: str) -> int: | ||
| r = rpc(HTTP_URL, "eth_getTransactionCount", [address, "latest"]) | ||
| return int(r.get("result", "0x0"), 16) |
There was a problem hiding this comment.
Fail fast on RPC errors instead of silently defaulting chain/head/nonce.
get_head(), get_chain_id(), and get_nonce() convert missing/failed RPC results into defaults (0x0 / 0x539). This masks connectivity/auth errors and can make stress results invalid while appearing successful.
Proposed fix
+def _require_hex_result(resp: dict, method: str) -> int:
+ if "error" in resp:
+ raise RuntimeError(f"{method} failed: {resp['error']}")
+ if "result" not in resp:
+ raise RuntimeError(f"{method} failed: missing result")
+ return int(resp["result"], 16)
+
def get_head() -> int:
- r = rpc(HTTP_URL, "eth_blockNumber", [])
- return int(r.get("result", "0x0"), 16)
+ return _require_hex_result(rpc(HTTP_URL, "eth_blockNumber", []), "eth_blockNumber")
def get_chain_id() -> int:
- r = rpc(HTTP_URL, "eth_chainId", [])
- return int(r.get("result", "0x539"), 16)
+ return _require_hex_result(rpc(HTTP_URL, "eth_chainId", []), "eth_chainId")
def get_nonce(address: str) -> int:
- r = rpc(HTTP_URL, "eth_getTransactionCount", [address, "latest"])
- return int(r.get("result", "0x0"), 16)
+ return _require_hex_result(
+ rpc(HTTP_URL, "eth_getTransactionCount", [address, "latest"]),
+ "eth_getTransactionCount",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def get_head() -> int: | |
| r = rpc(HTTP_URL, "eth_blockNumber", []) | |
| return int(r.get("result", "0x0"), 16) | |
| def get_chain_id() -> int: | |
| r = rpc(HTTP_URL, "eth_chainId", []) | |
| return int(r.get("result", "0x539"), 16) | |
| def get_nonce(address: str) -> int: | |
| r = rpc(HTTP_URL, "eth_getTransactionCount", [address, "latest"]) | |
| return int(r.get("result", "0x0"), 16) | |
| def _require_hex_result(resp: dict, method: str) -> int: | |
| if "error" in resp: | |
| raise RuntimeError(f"{method} failed: {resp['error']}") | |
| if "result" not in resp: | |
| raise RuntimeError(f"{method} failed: missing result") | |
| return int(resp["result"], 16) | |
| def get_head() -> int: | |
| return _require_hex_result(rpc(HTTP_URL, "eth_blockNumber", []), "eth_blockNumber") | |
| def get_chain_id() -> int: | |
| return _require_hex_result(rpc(HTTP_URL, "eth_chainId", []), "eth_chainId") | |
| def get_nonce(address: str) -> int: | |
| return _require_hex_result( | |
| rpc(HTTP_URL, "eth_getTransactionCount", [address, "latest"]), | |
| "eth_getTransactionCount", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/stress-multiproof.py` around lines 97 - 109, The helper functions
get_head, get_chain_id, and get_nonce currently default to hardcoded hex strings
when rpc(...) returns no "result", which hides RPC failures; modify each
function to check the rpc response for an error or missing "result" and raise an
exception (e.g., RuntimeError) containing the full rpc response and a
descriptive message instead of returning a default value, so callers immediately
see connectivity/auth errors; keep the existing int(..., 16) parsing for
successful responses and reference the functions get_head, get_chain_id, and
get_nonce when making the change.
Comment on lines
+200
to
+201
| accounts = [Account.from_key(a["key"]) for a in json.load(open(ACCOUNTS_FILE))] | ||
| sender = accounts[0] |
There was a problem hiding this comment.
Guard against empty accounts.json before indexing accounts[0].
If accounts.json is empty/malformed, this crashes with IndexError and no actionable message.
Proposed fix
- accounts = [Account.from_key(a["key"]) for a in json.load(open(ACCOUNTS_FILE))]
- sender = accounts[0]
+ accounts = [Account.from_key(a["key"]) for a in json.load(open(ACCOUNTS_FILE))]
+ if not accounts:
+ raise RuntimeError(f"No accounts found in {ACCOUNTS_FILE}")
+ sender = accounts[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| accounts = [Account.from_key(a["key"]) for a in json.load(open(ACCOUNTS_FILE))] | |
| sender = accounts[0] | |
| accounts = [Account.from_key(a["key"]) for a in json.load(open(ACCOUNTS_FILE))] | |
| if not accounts: | |
| raise RuntimeError(f"No accounts found in {ACCOUNTS_FILE}") | |
| sender = accounts[0] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-test/stress-multiproof.py` around lines 200 - 201, The code currently
assumes accounts = [Account.from_key(a["key"]) for a in
json.load(open(ACCOUNTS_FILE))] will produce a non-empty list and then uses
sender = accounts[0]; guard against an empty or malformed ACCOUNTS_FILE by
validating the JSON load and the resulting list before indexing. Specifically,
catch JSONDecodeError when loading ACCOUNTS_FILE, ensure the loaded value is a
list and not empty, and if it is empty or invalid raise/exit with a clear
message (e.g., "no accounts found in ACCOUNTS_FILE" or "invalid accounts.json")
rather than allowing an IndexError; update the code paths around
Account.from_key and sender to run only after the validation succeeds.
Remove three event-style counters in favor of structured logs:
- reth_morph_payload_builder_l1_tx_gas_limit_exceeded_total
- reth_morph_payload_builder_l1_tx_strange_err_total
- reth_morph_payload_builder_pool_transactions_skipped_total
Rationale: L1 skip events are too rare to warrant a timeseries tile
(panels were a flat zero line), and pool skip events are too noisy to
plot usefully (nonce_too_low fires on the fast path every build).
In both cases a log line carries more context per event than a bare
counter, so we lean on logging and delete the metrics entirely.
Changes by code path:
L1 message execution (all paths now log):
- gas-limit exceeded: new `warn!` with tx_gas / cumulative / block_gas_limit
- InvalidTx: pre-existing `warn!` (preserved)
- Validation error: pre-existing `warn!` (preserved)
- Fatal EVM error: new `error!` with tx_index / signer / error
Pool transaction sweep:
- blob or L1-msg in pool: new `warn!` (pool-filter bug indicator)
- over block limits: new `debug!` with gas/da/size context
- nonce_too_low: existing `trace!` kept (fast-path, noisy)
- invalid_tx: existing `trace!` kept (fast-path, noisy)
- validation_error: existing `trace!` kept (fast-path, noisy)
- Fatal EVM error: new `error!` before propagating
The three trace-level sites stay at trace because they fire on every
pool sweep under load; enable `RUST_LOG=morph_payload_builder=trace`
to diagnose pool-skip rates when needed.
Dashboard:
- Remove Morph L1 Tx Skip Counters and Morph Pool Tx Skipped By Role
panels from the Morph Payload Builder row (now a 2x2 grid of 4
duration/count panels)
- Shift all downstream row gridPos up by 9 to reclaim the vacated slot
Dev tooling:
- audit-dashboards.py: clear INTENTIONALLY_UNCOVERED_METRICS — all
remaining custom metrics now have dashboard panels, and there are
no whitelist exemptions needed.
…ty queries
Dashboard polish pass driven by visual review of every row. All changes
are made directly to the hand-maintained `morph-reth.json`.
Row / variable simplification:
- Drop `Morph` prefix on Engine API / Payload Builder row titles and
the four corresponding Payload Builder panel titles — the dashboard
itself is morph-reth, the prefix was redundant.
- Remove `instance` and `instance_label` template variables — `service`
(which is role-filtered) already covers every use. All
`$instance_label=~"$instance"` usages are rewritten to
`service=~"$service"`.
Delete panels whose metric no longer exists or was never exposed
(repacked the two affected rows so no layout gaps remain):
Sync & Downloaders:
- Block body response sizes (reth_downloaders_bodies_response_* removed upstream)
Process & Maintenance:
- Jemalloc Memory (requires `jemalloc` cargo feature, not built)
- Pruner duration, per segment (reth only exposes total, not per-segment)
- Highest pruned block, per segment (same)
Fix queries that returned `No data` even though the underlying metric
exists — both are upstream reth dashboard bugs:
- Summary `quantile="1"` literal mismatch: Prometheus normalizes summary
quantile labels to `"1.0"` (and `"0.0"`), but reth's panels query
bare `"1"`. Three panels never matched: `DB Max Transaction Open
Time`, `DB Max Insertion Operation Time`, `Static Files Max Writer
Commit Time`. Replaced the literal.
- `> 0` filter eats zeros under low load: six panels wrap expressions
in `... > 0` so "inactive" values are hidden. On devnet-ish traffic
everything is zero, so the filter empties the panel. Dropped the
filter so zero-value series still plot as a flat baseline:
DB Max Transaction Open Time
DB Max Insertion Operation Time
Static Files Max Writer Commit Time
Request Latency time
Maximum call latency per method
Call Latency time
RPC Throughput
Overview row layout fixes:
- Merge 5 upstream build-info stat panels (Version / Build Timestamp /
Git SHA / Build Profile / Target Triple) into a single `Build Info`
stat strip (w=24, h=5, vertical orientation). The 5 original narrow
tiles had a Grafana text-alignment glitch at w=5.
- Split `Storage size` into 4 independent stat panels
(Database / Freelist / Static Files / Total), each w=3, h=6. The
packed form didn't honor `orientation=horizontal`.
- Convert `Connected peers` from gauge to stacked bar chart with
`{{service}}` legend — scales to any number of nodes and renders
clearly when values are constant (the line form vanishes into the
grid lines).
- Repack every Overview grid row to fill 24 columns (previously 4
rows left 4-8 column gaps each after the earlier Cargo Features /
Stage checkpoints / Sync progress / Discv5 removals).
Dev tooling:
- audit-dashboards.py: update `TARGET_ROWS` to the new row names
(`Engine API` / `Payload Builder`). `INTENTIONALLY_UNCOVERED_METRICS`
whitelist is now empty.
- per-panel-audit.py: same row rename.
Under low load `rate(..._count[$__rate_interval])` is 0, so the outer `rate(sum) / rate(count)` is 0/0 = NaN and the panel renders as 'No data'. Clamp the denominator to a minimum of 1 so the expression gracefully falls to 0 while `count` is flat. The shape of the result when count > 0 is unchanged (clamp_min is a no-op once count actually grows).
- h: 8 -> 6 so Connected peers matches the 4 Storage stat panels on the same grid row. The previous h=8 overflowed into y=12..14 and overlapped with Execution throughput and Block Import Rate (y=12 x=0..24). - max: 50 -> 10 so the bargauge fill is actually visible at devnet peer counts (1-2). Still has headroom for typical production scale (color turns green at 3, saturates near 10).
…polish
Dashboard cleanup driven by walking every panel in Chrome DevTools MCP
against both an idle 3-node mainnet setup and a loaded 4-node devnet.
## Row structure (10 → 11 rows)
Split the old TxPool & P2P row into two independent rows:
- TxPool (19 panels, pool-internal metrics)
- P2P (5 panels, networking)
They have different width patterns and mixing them made react-grid-layout's
verticalCompact re-flow panels across sections, creating visual gaps.
Final P2P layout: Discovery + Connections, Errors + Peer Disconnect Reasons,
Total Dial Success full width.
## Chain / role / service filtering (245/245 targets)
Inject `chain="$chain"` and `role=~"$role"` into every PromQL target so
switching the dropdowns actually filters. Previously only `service=` was
threaded into queries and Prometheus's 5-minute staleness window kept
old devnet series visible after swapping chains.
## Idle-state normalization
- Wrap summary-quantile panels (newL2Block / assembleL2Block /
validateL2Block latency, Payload Builder, Execution & State Root,
Pending MultiProof, DB/Pruner average-time) in `> 0` so idle nodes
show "No data" instead of a flat line with 16 series in the legend.
- Keep 0 lines on rate counters (Engine API Failures, P2P Errors,
Task Executor) because 0 is a meaningful healthy baseline there.
- Redundant multiproof / Head Block Timegap By Role are gauges; 0 is
healthy so they stay without `> 0`.
- Hard-set `fieldConfig.defaults.min = 0` on every timeseries panel so
rate charts no longer auto-scale Y to negative.
- Head Block Timegap By Role gets `axisSoftMax: 5` so the 0 s baseline
is visible (Grafana was auto-scaling to 1.67 min of empty space).
## Per-service legends
Every target's legendFormat starts with `{{service}}` so lines can be
distinguished across the 3-4 node topology. `palette-classic-by-name`
ensures colors stay stable per service regardless of ordering.
## Time range
- Lift default time range to `now-30m` so `interval = 5m` Growth panels
have enough window to render.
- Shrink the interval variable default from 10m → 5m and add 1m.
- Uncollapse all 7 rows so a full sweep doesn't require manual expansion.
## Supporting config
- `etc/prometheus/prometheus.yml` labels the default single-node scrape
with `chain=morph / role=sequencer / service=morph-reth` so the
dashboard works out of the box for users running `etc/docker-compose.yml`.
- `etc/docker-compose.yml` drops the `${VAR_INSTANCE_LABEL}` sed (the
dashboard no longer references that variable).
- `etc/README.md` Docker Compose section rewritten with morph-reth
specifics (local `docker-compose.yml` reference + `--chain morph`).
## Verification
- Idle (3 mainnet reth, no CL driver): every panel either shows its
static metric or "No data" — no empty plots with crowded legends.
- Under load (4 reth + tendermint + stress.py): Block Import Rate,
Engine API latency, Payload Builder, TxPool, Proof calculation,
History Growth all render real data; nothing regressed to No data.
- Under mainnet catch-up (3 reth + morphnode pairs pulling from mainnet
seeds): newL2Block p95 ≈ 1.2 ms, 20 blocks/s per node, Block Import
Rate / Execution throughput / Chain Tree Canonical Height all update
live; follower-only No-data panels (assembleL2Block, Payload Build
Duration, newSafeL2Block) remain No data as expected.
Remove unnecessary entries from .gitignore
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
chengwenxi
approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
morph.engine): per-method latency histograms + failure counters for all 5 custom RPC endpoints, plushead_block_timegap_secondsgauge (geth'schain/head/timegap)morph.payload_builder): L1 message skip counters (geth'sminer/skipped_txs/l1/*), per-tx and per-block timing (geth'sminer/commit/*), end-to-end build duration (tempo-inspired), pool tx skip counter with reason labels (tempo-inspired)etc/directory with Prometheus + Grafana + docker-compose setup, including a custommorph-engine.jsonGrafana dashboard with 14 panelsMetrics Added
morph.engine(11 metrics){assemble,new,validate,new_safe}_l2_block_duration_secondsset_block_tags_duration_seconds{assemble,new,validate,new_safe}_l2_block_failures_totalset_block_tags_failures_totalhead_block_timegap_secondsmorph.payload_builder(8 metrics)l1_tx_gas_limit_exceeded_totall1_tx_strange_err_totalcommit_txs_all_duration_secondscommit_tx_apply_duration_secondspayload_build_duration_secondsblock_transactionspool_transactions_skipped_total{reason=...}Test plan
cargo clippy -p morph-engine-api -p morph-payload-builder -- -D warningspassescargo test -p morph-engine-api— 36 tests pass (includingtest_all_engine_api_metrics)cargo test -p morph-payload-builder— 36 tests pass (includingtest_all_payload_builder_metrics)metrics_util::DebuggingRecorderto verify all counters, gauges, histograms, and labeled counters register and record correctlySummary by CodeRabbit
Release Notes
New Features
Documentation