ethereum / ethereum/execution-specs

Refactor EL request helpers to avoid shared param mutation and add cross-format fixture regression checks

Open
#2,534 1 comment 0 reactions 0 assignees View on GitHub
stale
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.