Skip to content

feat(l1): add full-sync throughput regression watch - #7114

Open
ElFantasma wants to merge 14 commits into
mainfrom
feat/l1-fullsync-bench-watch
Open

feat(l1): add full-sync throughput regression watch#7114
ElFantasma wants to merge 14 commits into
mainfrom
feat/l1-fullsync-bench-watch

Conversation

@ElFantasma

Copy link
Copy Markdown
Contributor

Motivation

We have no continuous signal for execution throughput on real mainnet blocks. The per-PR CI benchmark runs a synthetic dense-ERC20 import, and multisync validates snap sync completion — so a change that lands on main and slows mainnet full sync stays invisible until someone runs a manual benchmark. We did that manually for #7008/#7023 and it cost days of setup each time.

Implements the design in #7111. Sibling manual A/B tool is #7112; both are meant to share the leg primitive added here.

Description

tooling/sync/fullsync_bench.py keeps a node a fixed distance behind head and, per cycle per network:

restore base → run M blocks           (MEASURE; resulting state discarded)
restore base → advance to head − GAP  (becomes the new base)

The base creeps forward at chain rate, so the node never ages and there is no anchor or reference commit to maintain. M is fixed and independent of the base's movement, so consecutive measurements overlap heavily — that overlap is what keeps day-to-day workload variation small enough for the series to be meaningful.

The advance targets head − GAP rather than a fixed block count, so it self-calibrates to each network's real block production and absorbs missed slots (relevant on testnets).

run_leg is deliberately pure — it never decides what happens to the resulting state, never assumes a particular ref, and never rotates anything. That is what lets #7112 reuse it unchanged.

Reuses multisync's per-network service pairs via a compose override (--syncmode full, mem_limit, published beacon APIs) rather than duplicating them, and its Slack webhook conventions.

Scope of this PR

Observe-only. It measures, records one JSON per leg, and posts a Slack summary with the delta against the trailing median. Alerting and step detection are deliberately not here: the real day-to-day σ is unknown, and thresholds should be derived from 2–3 weeks of collected data rather than guessed. Multi-network config is in place; the default is mainnet only.

Testing

The metric extraction is split into fullsync_metrics.py precisely so it can be tested without a node, a box or Docker — it is also the part most likely to be silently wrong. make fullsync-bench-test runs 7 tests against real log lines captured from the #7008/#7023 runs, covering: batch throughput mean/samples, the (unified pipeline) wording variant, blocks/s, gas-normalised phase timings, phase lines with no preceding block header (a truncated log must not skew the ratio), state-regeneration timing, and — importantly — that a run with no batches yields None rather than 0 Ggas/s, so a dead run is reported invalid instead of as a catastrophic regression.

The orchestration itself needs a box with a synced node and cannot be exercised in CI; make fullsync-bench-once is the intended first real run.

Operational invariants encoded in the code

These come from the manual benchmarking, not from preference:

  • graceful docker stop -t 300 only — repeated abrupt stops previously left the canonical head ahead of any durably-flushed state, after which the node could not regenerate and needed a full re-sync
  • the stop condition reads eth_blockNumber, never eth_syncing.currentBlock, which goes stale during catch-up and once let a leg overshoot its target badly
  • base rotation is health-gated: a new base is promoted only after the node demonstrably restarts on it with state available, otherwise the previous generation is kept
  • hardlink (rsync --link-dest) base generations, so rollback costs deltas rather than full copies
  • a box-wide lockfile: networks run serially, because concurrent legs contend and both results are junk

Follow-ups (not in this PR)

Checklist

  • Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync. — N/A: tooling only, no client code touched.

@ElFantasma
ElFantasma requested a review from a team as a code owner August 5, 2026 18:16
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Stateless (zkEVM) Amsterdam+ EF tests skipped

Where: tooling/ef_tests/blockchain/test_runner.rsparse_and_execute skips
fixtures with network >= Fork::Amsterdam when running with a stateless backend.
Affects make test-stateless (the vectors_zkevm/ run); make test-levm is
unaffected.

Why: The stateless run uses the tests-zkevm@v0.5.0 bundle, filled against
glamsterdam-devnet v6.1.0, which predeploys the EIP-8282 builder deposit/exit
contracts at the OLD addresses (0x0000884d…d9008282 / 0x000014574a…0f008282).
This client uses the devnet-7 addresses (0x0000bff4…300d8282 /
0x000064d6…800e8282, matching the live tests-glamsterdam-devnet@v7.2.0 bundle
used by make test-levm). Every Amsterdam+ block runs the end-of-block EIP-8282
builder system call; with the new addresses absent from the v0.5.0 bundle, each
stateless Amsterdam+ block fails with
SystemContractCallFailed("System contract: 0x0000…8282 has no code after deployment").
The skip is by fork rather than by test name, since cross-fork directories such as
for_amsterdam/prague/... still execute at the Amsterdam fork.

Removal: Delete the skip_stateless_amsterdam branch in parse_and_execute
once a tests-zkevm bundle filled with the devnet-7 builder predeploy addresses is
released and .fixtures_url_zkevm is bumped to it.

@github-actions github-actions Bot added the L1 Ethereum client label Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

Findings:

  1. High: tooling/sync/fullsync_bench.py:200-208 can parse a truncated log. stop_node() waits for the container, but then the code immediately terminate()s docker logs -f and parses the file without waiting for the log follower to flush/exit. That can drop the final [METRICS] batch lines and produce false no_batches / undercounted samples, which directly corrupts the throughput series. The log subprocess should be drained and the file handle closed before parse_run_file() runs.

  2. Medium: tooling/sync/fullsync_bench.py:113-115 and 253-257 assume the corresponding consensus-* service is already healthy, but start_node() brings up only ethrex-* with --no-deps. In this tool, both full sync progress and cl_head() depend on Lighthouse being available. If the BN dies or is absent, measure legs just burn until timeout and advance silently stops rotating the base. The runner should either start/health-check consensus-{net} itself or fail fast before beginning a cycle.

  3. Medium: tooling/sync/fullsync_bench.py:433-445 uses a plain create-once lockfile with no stale-lock recovery. Any crash, host reboot, or kill -9 leaves /tmp/ethrex-fullsync-bench.lock behind and blocks all future runs indefinitely. For a long-lived watchdog, this needs PID/liveness validation or flock-style locking.

  4. Low: tooling/sync/docker-compose.fullsync-bench.yaml:15-25 publishes the beacon HTTP APIs on all host interfaces. The runner only reads them via localhost, so binding them as 127.0.0.1:505x:5052 would avoid unnecessarily exposing BN HTTP endpoints on the box.

  5. Low: tooling/sync/fullsync_bench.py:400-403 labels the comparison as a median, but for an even number of historical samples it takes the upper middle element instead of the true median. That skews the reported delta and is easy to fix by reusing the existing _median helper pattern from fullsync_metrics.py.

No EVM/consensus-state correctness issues surfaced here since the PR is tooling-only. python3 tooling/sync/test_fullsync_metrics.py passes locally.


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

@greptile-apps

greptile-apps Bot commented Aug 5, 2026

Copy link
Copy Markdown

Greptile Summary

Adds a continuous full-sync benchmark that maintains rolling per-network bases, extracts execution metrics, persists leg results, and posts observe-only Slack summaries.

  • Adds measure-and-advance orchestration with serialized execution and retained hardlink generations.
  • Adds a Docker Compose override for full-sync mode, memory limits, and published consensus APIs.
  • Adds metric parsing and captured-log tests for throughput, phase timings, and state regeneration.
  • Documents bootstrap, usage, metric semantics, and operational invariants.

Confidence Score: 4/5

The base-promotion ordering needs to be fixed before merging because a routine snapshot failure can terminate the continuous watch and require manual base recovery.

The implementation moves the only active base to a retained generation before fallible snapshot and metadata operations have completed, leaving no automatic recovery path when either operation fails.

Files Needing Attention: tooling/sync/fullsync_bench.py

Important Files Changed

Filename Overview
tooling/sync/fullsync_bench.py Implements the benchmark lifecycle and reporting, but rotates away the active base before the replacement is durably created.
tooling/sync/fullsync_metrics.py Extracts batch throughput, block rate, normalized phase timings, and regeneration duration from node logs.
tooling/sync/test_fullsync_metrics.py Exercises metric extraction against representative captured log lines and invalid-run behavior.
tooling/sync/docker-compose.fullsync-bench.yaml Overrides multisync services for full-sync execution, bounded memory, and exposed beacon APIs.
tooling/sync/Makefile Adds watch, one-cycle, and metric-test entry points.
tooling/sync/README.md Documents operation, bootstrap requirements, metrics, and safety invariants.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  B[Restore base.0] --> M[Measure fixed block window]
  M --> D[Discard measured state]
  D --> R[Restore base.0]
  R --> A[Advance to consensus head minus gap]
  A --> H{Restart health check passes?}
  H -- No --> K[Keep previous base]
  H -- Yes --> G[Rotate base generations]
  G --> S[Snapshot live state as new base.0]
  S --> W[Record base head]
  W --> B
Loading
Prompt To Fix All With AI
### Issue 1
tooling/sync/fullsync_bench.py:279-281
**Rotation removes the active base**

When the snapshot or metadata write fails after generation rotation, `base.0` remains absent because the previous base has already moved to `base.1`. The next cycle raises `SystemExit`, terminating the continuous benchmark watch until an operator manually restores or re-bootstraps the base.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "feat(l1): add full-sync throughput regre..." | Re-trigger Greptile

Comment on lines +279 to +281
rotate_generations(state_root, net)
snapshot(net, base_dir(state_root, net, 0), link_dest=base_dir(state_root, net, 1))
write_base_head(base_dir(state_root, net, 0), advanced["reached_block"])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Rotation removes the active base

When the snapshot or metadata write fails after generation rotation, base.0 remains absent because the previous base has already moved to base.1. The next cycle raises SystemExit, terminating the continuous benchmark watch until an operator manually restores or re-bootstraps the base.

Prompt To Fix With AI
This is a comment left during a code review.
Path: tooling/sync/fullsync_bench.py
Line: 279-281

Comment:
**Rotation removes the active base**

When the snapshot or metadata write fails after generation rotation, `base.0` remains absent because the previous base has already moved to `base.1`. The next cycle raises `SystemExit`, terminating the continuous benchmark watch until an operator manually restores or re-bootstraps the base.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Code Review — PR #7114 (full-sync throughput regression watch)

Scope: this is pure Python tooling (tooling/sync/*) for an ops benchmarking daemon — no Rust/EVM/consensus code is touched, so the Ethereum-specific correctness concerns (gas accounting, RLP, trie ops) don't apply here. Review focuses on the automation logic itself.

Findings

1. Lock is held for the entire --watch daemon lifetime, not per leg (design/correctness)
tooling/sync/fullsync_bench.py:39 documents the lock's purpose: "two legs running at once contend for CPU and disk and both results are junk... This also excludes the A/B tool (#7112), which must take the same lock." But take_lock() is called once before the while True: loop and only released in the outer finally (fullsync_bench.py:466-482). In continuous fullsync-bench-watch mode, the lock is held for the process's entire lifetime — including the time.sleep(3600) idle windows between cycles where there's no actual CPU/disk contention. Since this daemon is meant to run continuously in production, the sibling A/B tool (#7112) can effectively never acquire the lock while the watch daemon is active, which seems to defeat the stated intent of leg-level (not daemon-level) mutual exclusion. Consider taking/releasing the lock inside cycle()/run_leg() per leg instead of around the whole main() loop.

Related minor gap: there's no stale-lock recovery — if the process is SIGKILLed, LOCK_PATH persists forever and requires manual cleanup (no PID liveness check in take_lock(), fullsync_bench.py:638-644).

2. drop_page_cache() drops the entire host's page cache, not just this container's (fullsync_bench.py:129)
This writes 3 to /proc/sys/vm/drop_caches, which is a host-wide operation affecting every process on the box, called before every leg. Given the box is explicitly meant to be shared with the sibling A/B tool and potentially other services, this is a fairly invasive side effect for what's framed as an isolated benchmark. It may be an intentional tradeoff for cold-start parity (and is called out as deliberate), but it's worth flagging explicitly as a host-wide blast radius, not container-scoped.

3. logger.terminate() without wait()/communicate() (fullsync_bench.py:202)
docker logs -f child processes are terminated but never reaped. In the long-running --watch mode (potentially running for months across many cycles), this can accumulate zombie processes. Cheap fix: logger.wait(timeout=...) after terminate().

4. phase_ms_per_mgas silently reports 0.0 instead of None for a phase that's missing from the log (fullsync_metrics.py:99, :143)
phase_ms is initialized as {name: 0.0 for name in TRACKED_PHASES} and only gated on block_mgas > 0 as a whole. If a future log format drops or renames one of validate/exec/merkle/store while still emitting block headers, that phase would silently report as ~0ms/Mgas (a phantom "instant" phase) rather than None/invalid — inconsistent with the careful "no batches → None, not 0" invariant that's explicitly tested elsewhere (test_run_with_no_batches_yields_none_not_zero). Not currently exploitable since all four phases are always logged together, but it's a latent gap in the same class of bug the tests otherwise guard against.

5. Minor statistical nits (fullsync_metrics.py:79, fullsync_bench.py:401-408)

  • _stdev computes population stdev (÷n) rather than sample stdev (÷n-1). Fine for descriptive purposes but worth a one-line comment since it's a deliberate choice.
  • summarise()'s median picks sorted(previous)[len//2] — for even-length history this is the upper-middle value, not the textbook average-of-two-middles median. Inconsequential at these sample sizes, just noting the approximation.

What's solid

  • Clean separation of run_leg (pure) from cycle/rotation logic, exactly as described — this is good design and genuinely testable.
  • rotate_generations/snapshot hardlink logic is correct (verified the rename ordering avoids clobbering, and the health-gate correctly runs before promoting a new base).
  • No injection risk: all subprocess calls use argument lists (no shell=True), and network names are validated against the NETWORKS allowlist before being interpolated into container/service names.
  • The "no batches → None, never 0" invariant is well-reasoned and well-tested — this is exactly the kind of silent-corruption bug that's easy to miss in a metrics pipeline.
  • Test suite uses real captured log lines rather than synthetic ones, and covers the right edge cases (truncated logs, wording variants, empty runs).
  • Operational invariants (graceful-stop-only, eth_blockNumber over eth_syncing, health-gated rotation) are documented with the concrete incidents that motivated them — good practice for an ops tool that will be maintained by someone else later.

Overall: solid, well-tested tooling addition with one substantive design issue (Point 1) worth resolving before this and #7112 are expected to coexist on the same box, and a few minor robustness nits.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant