ethereum / ethereum/execution-specs
Forbid Global Mutable State in Environment Defaults
- Dominant language
- Python
- Stars
- 1.2k
- Forks
- 505
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 116
Description
PR #1711 fixes a flaky test (`test_model_copy`) that was failing ~50% of the time due to race conditions in how `Environment.gas_limit` defaults are handled. While the PR successfully resolves the immediate bug, it highlights a deeper architectural issue with global mutable state that could cause similar problems in the future.
## Current Architecture
### How It Works
```python
# block_types.py
DEFAULT_BLOCK_GAS_LIMIT = 90_000_000
class EnvironmentDefaults:
"""Default environment values."""
gas_limit: int = DEFAULT_BLOCK_GAS_LIMIT # ← Mutable class attribute
class EnvironmentGeneric(CamelModel, Generic[NumberBoundTypeVar]):
gas_limit: NumberBoundTypeVar = Field(
default_factory=lambda: EnvironmentDefaults.gas_limit, # ← Late binding
alias="currentGasLimit",
)
```
Pytest plugins modify this global state during configuration:
```python
# filler.py:699 and execute.py:152
@pytest.hookimpl(tryfirst=True)
def pytest_configure(config: pytest.Config) -> None:
if config.getoption("block_gas_limit"):
EnvironmentDefaults.gas_limit = config.getoption("block_gas_limit")
```
### Why It Exists
**Design Intent:**
1. **Convenience**: Single place to configure gas limits for all tests
2. **Flexibility**: Different pytest commands (fill, execute, benchmark) need different gas limits
3. **CLI Control**: Users can override defaults with `--block-gas-limit` or `--transaction-gas-limit`
4. **Backwards Compatibility**: Avoids refactoring hundreds of test files
**Usage Pattern:**
- 480 `Environment()` instantiations across 163 test files
- Only 7% explicitly set `gas_limit`
- 93% rely on the dynamic default from `EnvironmentDefaults.gas_limit`
## Problems with Current Architecture
### 1. Non-Deterministic Plugin Ordering
**Issue**: Two plugins (`filler.py` and `execute.py`) both use `@pytest.hookimpl(tryfirst=True)`, giving them identical priority. Pytest doesn't guarantee execution order when hooks have the same priority.
**Impact**: If both plugins try to set `EnvironmentDefaults.gas_limit`, the result is non-deterministic.
### 2. Race Conditions in Parallel Execution
**Issue**: Tests run with `pytest -n auto --maxprocesses 6`, creating multiple worker processes that can have different timing for:
- When plugins load and modify globals
- When test modules are imported
- When `Environment()` objects are created
**Impact**: Objects created at different times can have different `gas_limit` values, even though they use the same code (`Environment()`).
### 3. Late-Binding Default Factory
**Issue**: The `default_factory=lambda: EnvironmentDefaults.gas_limit` captures a **reference** to the class attribute, not its value. Every time an `Environment()` is created, the lambda re-evaluates.
**Impact**:
- `Environment()` at time T₁ → `gas_limit = 90_000_000`
- Plugin modifies `EnvironmentDefaults.gas_limit = 1000`
- `Environment()` at time T₂ → `gas_limit = 1000`
- Old `.copy()` implementation re-ran the factory → got new value!
### 4. Hidden Dependencies
**Issue**: The behavior of `Environment()` depends on invisible state:
- Whether pytest plugins have loaded yet
- Which CLI flags were passed
- Whether other tests have run that might affect globals
**Impact**: Violates the Principle of Least Surprise—test behavior is not locally reasonable.
### 5. Copy Semantics Violation
**Issue**: The old `.copy()` implementation broke the fundamental contract: `obj.copy() == obj`
**PR #1711 Fix**: Captures actual field values instead of re-running `default_factory`
Contributor guide
Assessment
This issue has not been assessed yet.