fix(evm_tools): avoid cloning unchanged fork template in ForkCache by JackCC703 · Pull Request #2738 · ethereum/execution-specs

JackCC703 · 2026-04-21T02:55:22Z

🗒️ Description

Optimized ForkCache.get(...) to avoid creating a temporary fork clone when requested overrides are identical to the template.
Added _ForkOverrides to organize temporary override values internally.
Added _template_matches(...) to compare requested values against template defaults before cloning.
Kept conservative fallback behavior: if vm.gas or GasCosts cannot be read, the request is treated as non-identical and still clones.
Preserved existing behavior for changed values: still clone and cache as before.
Expanded test coverage in tests/evm_tools/test_fork_cache.py to include:
- no-override fast path returns template
- fork criteria change triggers clone
- per-override identical vs changed behavior (parameterized)
- repeated changed request reuses cached clone
- missing vm.gas / GasCosts falls back to cloning

🔗 Related Issues or PRs

Fixes #1864.

✅ Checklist

All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
```
just static
```
(Executed the equivalent just static command set directly via uv run ... because just is not available in local environment.)
All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
All: Considered updating the online docs in the ./docs/ directory.
All: Set appropriate labels for the changes (only maintainers can apply labels).
Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

Cute Animal Picture

SamWilsn

How does this affect test runs? Worthwhile speedup I assume?

SamWilsn · 2026-04-21T14:47:13Z

+
+        gas_costs = getattr(gas_mod, "GasCosts", None)
+
+        def gas_default(


My naive implementation would be to do something like:

GasCosts = template.module("vm.gas").GasCosts if gas_per_blob is not None: if gas_per_blob != getattr(GasCosts, "per_blob", None): return False ...

I assume this is insufficient for some reason. Why?

Yeah,after I re-checking the current fork layout, comparing against vm.gas.GasCosts directly was enough here.

I kept the conservative fallback of cloning if vm.gas / GasCosts cannot be read, and added coverage for that case too.

SamWilsn · 2026-04-21T14:48:44Z

+            blob_base_fee_update_fraction=blob_base_fee_update_fraction,
+            max_blob_gas_per_block=max_blob_gas_per_block,
+            blob_schedule_target=blob_schedule_target,
+            blob_schedule_max=blob_schedule_max,


I think it's probably time to introduce a dataclass to organize this pile of arguments 🤣

Thanks. I wrapped the override values in _ForkOverrides so the internal plumbing is a bit less noisy without changing the external get(...) signature.😺

SamWilsn · 2026-04-21T14:50:34Z

+            return Unscheduled(order_index=1)
+        return zero_order
+
+    raise AssertionError(f"unsupported fork criteria type: {type(criteria)}")


Should use assert_never here if possible.

Fixed, this now uses assert_never.

SamWilsn · 2026-04-21T14:51:37Z

+    """Return a gas default from module-level or `GasCosts` attributes."""
+    value = getattr(gas_mod, module_attr, None)
+    if value is not None:
+        return cast(U64 | Uint, value)


Should we use asserts here instead of casts?

Fixed, I replaced the casts here with assert isinstance(...) checks.

codecov · 2026-04-21T15:04:19Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.17%. Comparing base (e8e9c38) to head (a3b8317).
⚠️ Report is 5 commits behind head on forks/amsterdam.

Additional details and impacted files

@@                 Coverage Diff                 @@
##           forks/amsterdam    #2738      +/-   ##
===================================================
+ Coverage            86.47%   88.17%   +1.69%     
===================================================
  Files                  599      577      -22     
  Lines                37632    35659    -1973     
  Branches              3795     3490     -305     
===================================================
- Hits                 32542    31442    -1100     
+ Misses                4526     3654     -872     
+ Partials               564      563       -1

Flag	Coverage Δ
unittests	`88.17% <ø> (+1.69%)`	⬆️

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.

JackCC703 · 2026-04-22T03:57:31Z

How does this affect test runs? Worthwhile speedup I assume?

Yes, looks worthwhile for the case this PR is targeting. When the requested blob overrides are identical to the template defaults, the old path still paid the full Hardfork.clone() cost. On my macbook that was about 23.1-23.3s per fresh cache, versus about 0.37s warm (0.67s cold) now since it just returns the template.

For actually changed overrides, both versions still clone, and I did not see a meaningful regression there (23.70s vs 23.16s).

SamWilsn

I wonder if we should move the _ForkOverrides dataclass into ethereum_spec_tools.forks and use it for Hardfork.clone directly. The parameter list for that function is getting unwieldy.

JackCC703 · 2026-04-23T03:08:56Z

I wonder if we should move the _ForkOverrides dataclass into ethereum_spec_tools.forks and use it for Hardfork.clone directly. The parameter list for that function is getting unwieldy.

That makes sense. I can open a follow-up PR for this.

fix(evm_tools): handle GasCosts defaults in template match

7bd4beb

SamWilsn reviewed Apr 21, 2026

View reviewed changes

fix(evm_tools): avoid cloning identical fork overrides

99d18e9

simplify fork cache template stuff

a3b8317

SamWilsn approved these changes Apr 22, 2026

View reviewed changes

SamWilsn merged commit d4c95a4 into ethereum:forks/amsterdam Apr 22, 2026
18 checks passed

JackCC703 deleted the jack/fork-cache-skip-identical-clone branch April 23, 2026 02:59

JackCC703 mentioned this pull request Apr 28, 2026

refactor(forks): pass fork overrides to clone #2769

Open

7 tasks


		gas_costs = getattr(gas_mod, "GasCosts", None)

		def gas_default(

Conversation

JackCC703 commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🗒️ Description

🔗 Related Issues or PRs

✅ Checklist

Cute Animal Picture

Uh oh!

SamWilsn 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!

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

Uh oh!

Codecov Report

Uh oh!

JackCC703 commented Apr 22, 2026

Uh oh!

SamWilsn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JackCC703 commented Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JackCC703 commented Apr 21, 2026 •

edited

Loading

codecov Bot commented Apr 21, 2026 •

edited

Loading