Skip to content

feat(test-consume): initial implementation of the enginex simulator#1964

Merged
danceratopz merged 10 commits into
ethereum:forks/amsterdamfrom
danceratopz:consume-enginex-sequential
Mar 10, 2026
Merged

feat(test-consume): initial implementation of the enginex simulator#1964
danceratopz merged 10 commits into
ethereum:forks/amsterdamfrom
danceratopz:consume-enginex-sequential

Conversation

@danceratopz

@danceratopz danceratopz commented Jan 5, 2026

Copy link
Copy Markdown
Member

🗒️ Description

Adds a consume enginex simulator that reuses client instances across tests sharing the same pre-allocation state. This simulator significantly reduces test execution time compared to consume engine by avoiding per-test client startup overhead.

Includes basic pytest-xdist support via --dist=loadgroup, with further optimizations planned in subsequent PRs.

Architecture

The enginex simulator groups tests by pre_hash (hash of genesis + pre-state). Tests in the same group run against a single client instance instead of starting/stopping per test.

New Files

File Purpose
simulators/enginex/conftest.py Client fixture with reuse logic, test sorting by group
simulators/multi_test_client.py MultiTestClientManager for client lifecycle
simulators/helpers/test_tracker.py Tracks test completion per group for cleanup

Modified

File Change
consume.py Adds xdist_group marker based on pre_hash for enginex fixtures
test_via_engine.py Unified test logic supporting both engine and enginex modes
pytest_hive.py Adds multi_test_hive_test fixture for cross-test client sharing
processors.py Adds --dist=loadgroup default for enginex with xdist

Execution Flow

  1. Collection: pytest_collection_modifyitems counts tests per group, sorts by pre_hash.
  2. First test in group: Starts client, registers with manager.
  3. Subsequent tests: Reuses existing client (skips FCU to keep at genesis).
  4. Last test in group: mark_test_completed triggers client shutdown.
  5. Session end: Cleanup any remaining clients.

Fixture Chain

flowchart LR
    subgraph "Per Group (cached)"
        PAG[pre_alloc_group] --> CG[client_genesis]
        PAG --> GH[genesis_header]
        CG --> BG[buffered_genesis]
        BG --> CF[client_files]
    end

    subgraph "Client (reused)"
        CF --> C[client]
        C --> ETH[eth_rpc]
        C --> ENG[engine_rpc]
    end

    subgraph "Per Test"
        TC[test_case] --> F[fixture]
        F --> P[payloads]
    end

    subgraph "Test Execution"
        GH --> TEST[test_blockchain_via_engine]
        ENG --> TEST
        P --> TEST
    end
Loading

Testing

Due to the pre-alloc optimizations from #2140 and the fact there's no release generated with this change yet (except bal@v5*), it's easiest to fill locally and test using those fixtures.

For example, fill the benchmark tests:

uv run fill --no-html --fork=Osaka --gas-benchmark-values 1 -m benchmark --output=fixtures-gas-values -n 8 --clean --evm-bin=evmone-t8n --generate-all-formats

Then consume them:

uv run consume enginex --input=fixtures-gas-values/ -v 

🔗 Related Issues or PRs

N/A.

✅ Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx tox -e static
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • [ ] All: Considered adding an entry to CHANGELOG.md. skipped
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).

Cute Animal Picture

image

@danceratopz danceratopz added C-feat Category: an improvement or new feature A-test-consume Area: execution_testing.cli.pytest_commands.plugins.consume labels Jan 5, 2026
@danceratopz danceratopz requested a review from spencer-tb January 5, 2026 09:04
@codecov

codecov Bot commented Jan 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.97%. Comparing base (3c1b009) to head (82a06a3).
⚠️ Report is 44 commits behind head on forks/amsterdam.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           forks/amsterdam    #1964      +/-   ##
===================================================
+ Coverage            84.93%   85.97%   +1.04%     
===================================================
  Files                  599      599              
  Lines                36916    36916              
  Branches              3771     3771              
===================================================
+ Hits                 31354    31738     +384     
+ Misses                4931     4560     -371     
+ Partials               631      618      -13     
Flag Coverage Δ
unittests 85.97% <ø> (+1.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Carsons-Eels pushed a commit to Carsons-Eels/execution-specs that referenced this pull request Jan 6, 2026
* chore(fw): remove fill warnings.

* chore(fw): refactor.
@danceratopz danceratopz force-pushed the consume-enginex-sequential branch 6 times, most recently from 953c821 to 97cf5e7 Compare March 5, 2026 09:10
@danceratopz danceratopz force-pushed the consume-enginex-sequential branch 3 times, most recently from b5a1811 to 0e87751 Compare March 6, 2026 09:03
@danceratopz

danceratopz commented Mar 6, 2026

Copy link
Copy Markdown
Member Author

As-is this branch can be tested against hive master, but hiveview does not report the tests correctly.

If you'd like to test end-to-end with hiveview, use this execution-specs branch (which adds two small commits to this PR's branch) and hive branch:

The hive branch needs a little clean-up, so not ready for review yet.

But to test, need to checkout the hive add-enginex-simulator branch and run this command (update path to clients-file as appropriate):

go build .
./hive --sim ethereum/eels/consume-enginex --client go-ethereum --client-file ../execution-specs/.github/configs/hive/master.yaml --sim.limit='.*' --sim.timelimit 3m --sim.buildarg "branch=consume-enginex-multi-test-reporting" --sim.buildarg "fixtures=https://github.com/danceratopz/execution-specs/releases/download/v0.1.0a2/fixtures.tar.gz" --docker.buildoutput --sim.loglevel=5 --docker.nocache consume-enginex

The release is a minimal release just for testing, you can also use "fixtures=benchmark@v0.0.7" but it's larger/slower (it was built without #2139). If it looks like it's hanging, it's calculating the buffered genesis (one-off), which is much smaller after #2139!

Then rebuild hiveview; each client log link now only highlights the relevant part of the log from that test:

go build ./cmd/hiveview; ./hiveview --serve

Example test summary and client logs in screenshots.

Summary screenshot:

  • Red boxes = first test in a pre-alloc group w/client initialization;
  • Green boxes = subsequent pre-alloc groups w/out client initialization.

Log screenshot; only the lines corresponding to that test are show.
image
image

@spencer-tb spencer-tb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review — consume enginex simulator

Reviewed by Claude (Opus 4.6) + Codex (GPT-5.3). Overall architecture is solid — the grouping-by-pre_hash, test sorting, xdist loadgroup integration, and engine_rpc extraction are all well done. Below are the findings, ordered by severity.


🔴 High

1. Cross-client reuse bug when multiple client types are parametrized (Codex)

MultiTestClientManager keys clients by pre_hash alone (enginex/conftest.py:154, multi_test_client.py:34). Since client_type is parametrized independently in consume.py:628-634, tests for client B could reuse client A's instance if they share the same pre_hash.

In practice Hive CI typically runs one client, but the code doesn't enforce that. Suggested fix:

# enginex/conftest.py:154
group_identifier = f"{client_type.name}:{fixture.pre_hash}"

2. multi_test_hive_test always reports test_pass=True to Hive (Both)

pytest_hive.py:296-300 unconditionally sends test_pass=True at module teardown. The existing hive_test fixture inspects result_call.passed — this module-scoped fixture should track failure state similarly (e.g., via a pytest_runtest_makereport hook that sets a flag on the test object). <- Seems fine as is!


🟡 Medium

3. No resilience to client crash mid-group (Both)

get_client() (multi_test_client.py:46) returns the cached Client object without any liveness check. If a client dies, all remaining tests in that group cascade-fail with opaque connection errors. A lightweight TCP connect check (or even just a try/except around the first RPC call with a clear error message) would improve debuggability.

4. Unbounded session caches (Both)

pre_alloc_group_cache, client_genesis_cache, environment_cache (multi_test_client.py:158-173) are session-scoped dicts with no eviction. For large fixture sets, these retain heavy PreAllocGroup objects until session end. Could evict entries in mark_test_completed when a group completes.

5. assert used for runtime validation of external data (Both)

Several assert statements guard external fixture data — these are stripped by python -O:

  • consume.py:612pre_hash presence check
  • enginex/conftest.py:190 — client startup check
  • multi_test_client.py:271 — ruleset key check

Suggest replacing with if not ...: raise ValueError(...) / raise RuntimeError(...).


🟢 Low / Nits

6. HIVE_GROUP_ID uses truncated hash (Both)

format_group_identifier() truncates to 16 chars (test_tracker.py:14), which is fine for log messages but is also used for the HIVE_GROUP_ID env var (multi_test_client.py:280). If this is used downstream as a unique identifier, consider using the full hash for the env var.

7. PR description says shared_hive_test, code says multi_test_hive_test (Claude)

Minor doc inconsistency in the PR description's "Modified" table.

8. Dual-yield in client fixture (Claude)

The client fixture (enginex/conftest.py:137-207) has two yield paths (reuse vs new). Works correctly but a single-yield refactor would be clearer — extract the resolution to a local variable, then have one try/yield/finally block.

9. isinstance branching in unified test function (Claude)

test_via_engine.py:78 and :217 use isinstance(fixture, BlockchainEngineFixture) to branch FCU behavior. A fixture-provided perform_fcu: bool flag would decouple the test logic from concrete fixture types. Not blocking, but worth considering if more modes are added.

10. Emoji in log messages (Claude)

♻️, 🚀, 🛑, in log messages (enginex/conftest.py, multi_test_client.py, test_tracker.py) can cause encoding issues in some CI log collectors. Consider plain text markers like [reuse], [start], [stop], [done].

@spencer-tb

Copy link
Copy Markdown
Contributor

I wanted to try claude/codex review skill here (thought it was a good candidate, not sure if I like it 😓)!

I ran consume enginex with --client go-ethereum,nethermind against the v0.1.0a2 fixtures using the 2 branches you listed for hive and EELS. I got the following result: 445 tests, 0 failures. It seems every test (both clients) shared a single node and nethermind tests ran against geth.

So the simple fix for 1) above wont work I believe. The grouping is by pre_hash throughout (marker, collection counts, client cleanup), so it can't tell between client types. Multi client runs would need grouping by client_type & pre_hash through the whole flow, which looks quite tricky to add. This can be a follow up imo though as single client is what we need for now!

- Build combined `test_case`×`client_type` parametrization in
  `pytest_generate_tests`, attaching `xdist_group` marks inline so
  xdist's `loadgroup` scheduler reads them natively.
- Simplify enginex `pytest_collection_modifyitems` to only read
  existing markers for counting and sorting (largest groups first).
- Add `make_group_identifier()` helper for composite key construction
  (`pre_hash` + `client_name`).
…P_ID`

- Remove `format_group_identifier()` which silently truncated the 18-char
  `pre_hash` (and lost the client name entirely from composite keys).
- Use full group identifiers in all log messages.
- Remove unused `HIVE_GROUP_ID` env var (nothing in hive reads it).
- Collapse single-value f-strings where possible.
- Consolidate dual-yield paths (reuse vs new) into a single
  resolve-or-create branch followed by one `try`/`yield`/`finally`.
The enginex conftest both imported `MultiTestClientManager` and
`TimingData` at module level and listed their modules in
`pytest_plugins`. Python's import ran before pytest could register
the modules for assert rewriting, producing:

  PytestAssertRewriteWarning: Module already imported so cannot
  be rewritten

Fix by moving the imports behind `TYPE_CHECKING` and using string
annotations in fixture signatures. Pytest resolves fixtures by
parameter name, not type annotation, so the string literals work
at runtime. Mypy/ruff still see the types via the `TYPE_CHECKING`
block.
@danceratopz

Copy link
Copy Markdown
Member Author

I wanted to try claude/codex review skill here (thought it was a good candidate, not sure if I like it 😓)!

I think it's quite helpful, just a little noisy. I addressed what I thought was worthwhile in this PR - rationale below! I think they should only augment reviews; human reviews still catch the most important issues atm, imo.

I additionally fixed the PytestAssertRewriteWarning warnings that were introduced in this PR in the last commit.

consume-enginex-multi-test-reporting is rebased on these fixes for hive end-to-end testing.

1. Cross-client reuse bug when multiple client types are parametrized - Fixed

Nice catch! The group identifier should have been a composition of pre-alloc group and client type. Fixed!

There is another fix in this commit. The tests were already ordered in the enginex simulator by pre-alloc group (this ordering is likely present anyway, but it's essential in sequential mode to prevent the simulator from trying to switch between pre-alloc groups - spoiler, it can't, so it would just hang), but this change also revealed that the scheduling was extremely poor in non-sequential mode. So the commit addressing this also ordered the tests so that the largest pre-alloc groups come first. This increases our chances of splitting larger groups across workers with loadgroup instead of say, with -n=2:

Worker 0: 1-test-group (geth); 1-test-group (reth)
Worker 1: 100-test-group (reth); 100-test-group (reth)

I.e., workers have imbalanced load leading to starvation.

With group size ordering:

Worker 0: 100-test-group (geth); 1-test-group (geth)
Worker 1: 100-test-group (reth); 1-test-group (reth)

This isn't the end solution though, as imbalances can still occur. xdist optimizations are coming in a follow-up PR.

2. multi_test_hive_test always reports test_pass=True to Hive (Both) - Retracted by reviewer

N/A

3. No resilience to client crash mid-group - won't fix

Could be more elegant, but the connection error from the first failed RPC call already points to the right cause. Let's see if this ever becomes an issue in practice.

4. Unbounded session caches - defer

Deferring as more xdist optimization will come in subsequent PRs. But I think it's a fair concern!

5. assert used for runtime validation of external data - Won't fix

We don't run simulators via Hive w/ python -O. But about the individual checks

  • pre_hash being None, this is behind an "is_enginex" check. I did wonder if we should add stricter typing/pydantic type checking by sublcassing TestCase to get a TestCaseEngineX.

These are pre-existing:

  • Client startup returning None is the existing pattern used by every other simulator in the codebase.
  • Ruleset key presence is also a pre-existing standard pattern.

6. HIVE_GROUP_ID uses truncated hash - Fixed

Removed (actually spotted while fixing 1.), hive doesn't use this. Someone hallucinated.

7. PR description says shared_hive_test, code says multi_test_hive_test - Updated

Updated.

8. Dual-yield in client fixture - Done

Fixed, more readable now, imo.

9. isinstance branching in unified test function - Deferred

test_via_engine() is definitely a bit unwieldly. There's a follow-up action here too: To add optional FCU calls post genesis and/or post-test. I think we should reconsider it when we add this logic.

10. Emoji in log messages - Wont fix (yet)

LLM's love to add emoji's but always complain about them in review lol. I don't think there's any legit issue here. Tbh, it's quite helpful for visually debugging logs.

@danceratopz danceratopz requested a review from spencer-tb March 10, 2026 11:06

@spencer-tb spencer-tb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Only thing I could spot! Previous issue looks solved from my side. Lets get this merged! 🚀

Comment thread docs/running_tests/running.md Outdated
Co-authored-by: spencer <spencer.tb@ethereum.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-test-consume Area: execution_testing.cli.pytest_commands.plugins.consume C-feat Category: an improvement or new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants