feat(test-fill): Optimistic grouping flag by marioevz · Pull Request #3390 · ethereum/execution-specs

marioevz · 2026-08-18T00:31:56Z

Description

Makes pre-allocation group packing optional, and replaces the untyped string hashes used across the pre-alloc group and fixture index machinery with fixed-size types.

--disable-optimistic-pre-alloc-grouping

New fill flag. Phase 1 skips the packing pass so groups stay fine-grained, and phase 2 then resolves a test's group by recomputing its hash directly instead of looking it up in the packed index. Packing stays the default.

Typed group hashes

New AllocGroupHash (8-byte truncated sha256) replaces the str/int hashes that were assembled by hand in several places.
Environment.__hash__ is replaced by Environment.canonical_json(), so the grouping pre-image is an explicit canonical JSON encoding rather than a process-stable __hash__ whose value was persisted as a folder name.
PreAllocGroupBuilder carries its own group_hash, and GroupPreAlloc.get_pre_alloc_grouping_hash() lets an allocation report the group it belongs to.
GroupIndexEntry/GroupIndexEntries are pydantic models; the test → group index is read and written through them instead of hand-rolled JSON.

Engine X fixtures

pre_hash is set when the fixture is constructed, from the allocation's own group hash, rather than patched onto the model afterwards.
postStateDiff was never populated: post_state is not a field of BlockchainEngineXFixture, so the filler's hasattr(fixture, "post_state") guard never fired. It is now computed in BlockchainTest.make_hive_fixture through the new Alloc.calculate_diff() (moved out of filler.py) and is a required field.

Fixture index

TestCaseBase.fixture_hash and IndexFile.root_hash are Hash instead of HexNumber; hasher and compare_fixtures follow.
FixtureCollector writes index entries through TestCaseIndexFile instead of an ad-hoc dict.
HashableItem.from_folder no longer strips the first two characters of a hash unconditionally, which mis-parsed unprefixed generatedTestHash values into 31 bytes.

Compatibility

Group hashes change, so existing pre_alloc folders must be regenerated. Engine X fixtures and .meta/index.json files written by earlier versions no longer validate, since postStateDiff and fixture_hash are now required.

Related Issues or PRs

N/A.

Checklist

Ran fast static checks to avoid CI fails, see Code Standards & Verifying Changes: just static
PR title has the form <type>(<area>): <title>, where <type> and <area> come from an appropriate C-<type>, respectively A-<area>, label. The title should match the target squash commit message.

Cute Animal Picture

codecov · 2026-08-18T00:35:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.33%. Comparing base (3d473e8) to head (e20e161).

Additional details and impacted files

@@                 Coverage Diff                 @@
##           forks/amsterdam    #3390      +/-   ##
===================================================
- Coverage            93.53%   91.33%   -2.21%     
===================================================
  Files                  624      624              
  Lines                37074    37074              
  Branches              3394     3394              
===================================================
- Hits                 34679    33863     -816     
- Misses                1645     2758    +1113     
+ Partials               750      453     -297

Flag	Coverage Δ
unittests	`91.33% <ø> (-2.21%)`	⬇️

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

☔ View full report in Codecov by Harness.
📢 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.

marioevz · 2026-08-18T15:39:47Z

        config = MockConfig(use_pre_alloc_groups=True)

        # Mock the file system operations
+        group_hash = AllocGroupHash.from_hash("test_hash")


For reviewers: Maybe from_hash is not the best name for this function. It's supposed to mean "calculate the AllocGroupHash from sha56'ing the input, string or bytes, and then truncate".

danceratopz

Thanks a lot for cleaning up the types here Mario 🙏

This looks good to me, just a few comments below. But I think we should align on a release strategy before merge.

If you're busy, I can PR the changes below; as you like.

danceratopz · 2026-08-19T14:38:14Z

+    pre_hash: AllocGroupHash
    """Hash of the pre-allocation group this test belongs to."""

-    post_state_diff: Alloc | None = None


This change will break loading v20.0.0/v20.0.1. It would be nice to be stricter with these fields going forward, although adding an empty Alloc has little value.

From our side, this is currently only relevant for the generic test flow, I can fix that by pinning the EELS branch until the next release.

For client infra: The easiest solution to avoid breaking their infra would be to build and release v20.0.2 immediately after merging this and telling clients to update.

I think we can do Alloc | None = None and then eventually remove the None once we've made enough releases with this field populated. Thanks for spotting this, I'll rollback the change!

After some checking I've discovered that the pre-alloc groups in v20.0.0/v20.0.1 are broken anyway due to a bug introduced in #2140: They contain the "builder" variant and therefore do not include the genesis header in them, and the only way to ingest them is to load them using the builder's model and then build them internally, which is fine for consume, but if anyone wants to use them out-of-the-box it's very inconvenient.

It's possible to include a ton of workarounds in this PR to try to load the broken variants but I feel we are just digging ourselves in a deeper tech debt hole. I suggest we merge this PR with the strict models and then make the v20.1.0 release, stating that there are breaking changes in the format.

cc @danceratopz

danceratopz · 2026-08-19T14:43:10Z

hasher still breaks by default with engine_x when ran as:

hasher compare <fill-a> <fill-b>

due to the presence of blockchain_tests_engine_x/pre_alloc/*.json. As we're touching hasher here anyway, we could consider excluding pre_alloc/. But very happy to do this as a follow-up!

Very nice idea, I'll give it a go while time boxing it 👍

danceratopz · 2026-08-19T14:43:58Z

            "groups, phase 2 generates all supported fixture formats."
        ),
    )
+    test_group.addoption(


We could consider adding a pytester test for invoking fill with this to pin the behavior down.

danceratopz · 2026-08-19T14:51:48Z

+   "testCount": 88,
+   "preAccountCount": 174,


These never got written to the JSON (pre-existing issue); they're only tracked internally in the in-memory PreAllocGroup. We should remove them here (consumers can use len(testIds) and len(pre).

Suggested change

"testCount": 88,

"preAccountCount": 174,

It's a bug, see #3390 (comment). Along with the genesis header, they are missing from v20.0.0/1.

danceratopz · 2026-08-19T14:53:44Z

+- **`testCount`**: Number of tests in this pre-allocation group
+- **`preAccountCount`**: Number of accounts in the pre-allocation group


They can be removed here, too.

Suggested change

- **`testCount`**: Number of tests in this pre-allocation group

- **`preAccountCount`**: Number of accounts in the pre-allocation group

See previous comment.

marioevz requested a review from spencer-tb August 18, 2026 00:31

danceratopz self-requested a review August 18, 2026 14:57

marioevz commented Aug 18, 2026

View reviewed changes

danceratopz reviewed Aug 19, 2026

View reviewed changes

marioevz added 11 commits August 21, 2026 18:32

feat(fill): Make pre-alloc groups packing optional

8494fa3

fix(bug): Bug due to untyped structs

2d3cab4

nit

511667c

claude findings

4cbe15c

claude updated docs

0636d23

fix: backwards compatible index reading

59a306d

fix: properly skip amsterdam skip list for engine x

bdc9d79

fix: bug saving pre-alloc group builder as final product

3630b4a

fix: hasher

b9258b0

fix: typo

852a70c

fix: chain ID typing

e20e161

marioevz force-pushed the optimistic-grouping-flag branch from 81629c4 to e20e161 Compare August 22, 2026 00:32

spencer-tb mentioned this pull request Aug 22, 2026

chore(test-fill): make pre-alloc grouping safe for ported_static and remove the Engine X drift #3419

Open

11 tasks

		- `testCount`: Number of tests in this pre-allocation group
		- `preAccountCount`: Number of accounts in the pre-allocation group

Uh oh!

Conversation

marioevz commented Aug 18, 2026

Description

Related Issues or PRs

Checklist

Cute Animal Picture

Uh oh!

codecov Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danceratopz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Aug 18, 2026 •

edited

Loading