ethereum / ethereum/execution-specs
Refactor EL request helpers to avoid shared param mutation and add cross-format fixture regression checks
- Dominant language
- Python
- Stars
- 1.2k
- Forks
- 505
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 116
Description
Felipe wrote the [blueprint](https://github.com/ethereum/execution-specs/pull/2532#issuecomment-4093291174) for this issue. I added wishes of my own and had an LLM summarize the situation so we get:
## Context
#2532 fixed a bug in branch `eips/amsterdam/eip-8037` where `WithdrawalRequestContract.tx_gas_limit` was double-accumulated across fixture format runs because pytest parameter objects are shared by reference. The immediate fix moved the gas adjustment into `transactions()` as a pure function.
That addressed the concrete bug, but the broader anti-pattern still exists: the EL request helper objects used in the request tests are still mutated in `update_pre()`. Today these mutations are idempotent assignments, so they do not currently produce incorrect fixtures, but the pattern is fragile. A future `+=`, conditional mutation, or other stateful update could silently reintroduce divergence between `blockchain_test` and `blockchain_test_engine`.
There is also a separate footgun now: some behavior depends on callers remembering to pass `fork`, and a missed `fork` would not be caught by the mutation warning hook because nothing is being mutated in that case.
## Scope of what to do
Refactor the EL request helper flows in:
* `tests/prague/eip6110_deposits`
* `tests/prague/eip7002_el_triggerable_withdrawals`
* `tests/prague/eip7251_consolidations`
so that `update_pre()` no longer mutates shared parametrized helper instances. Instead, it should return derived/prepared values, and downstream methods such as `transactions()` and `valid_requests()` should consume those values explicitly.
## Guardrails
1. Add a warning-based guard first:
* Keep the filler hook that detects mutation of parametrized values during a test run.
* Use it to identify and clean up all remaining EL request helper mutation sites.
2. Add an explicit end-to-end regression check:
* Add a CI workflow script that compares relevant hashes between generated `blockchain_test` and `blockchain_test_engine` fixtures.
This should catch cross-format divergence directly, including cases that would not be detected by the mutation hook alone (for example, if a required fork is not passed somewhere).
3. Tighten the guard once the tree is clean:
* After the remaining mutation warnings are eliminated, flip the filler hook from warning to hard failure.
## Why both checks (mutation hook + explicit hash comparisons) are needed
* The mutation hook is a good guardrail against shared-object mutation bugs.
* The explicit hash comparison is the end-to-end proof that both fixture formats remain consistent. It also catches non-mutation regressions, including missed fork plumbing.
## Summary of what the PR that closes this issue should do
* Make it so that `update_pre()` no longer mutates shared helper instances in the three EL request modules.
* The current mutation warnings for those helpers are eliminated.
* Adds a regression check that compares `blockchain_test` vs `blockchain_test_engine` outputs for representative EL request fixtures.
* The filler hook is switched from warning to failure once we have addressed all occurrences
Contributor guide
Assessment
This issue has not been assessed yet.