Skip to content

feat(test-client-clis): tolerate missing trace file for rejected txs#2709

Merged
spencer-tb merged 1 commit intoethereum:forks/amsterdamfrom
leolara:leolara/bug-traces-rejected-tx
Apr 20, 2026
Merged

feat(test-client-clis): tolerate missing trace file for rejected txs#2709
spencer-tb merged 1 commit intoethereum:forks/amsterdamfrom
leolara:leolara/bug-traces-rejected-tx

Conversation

@leolara
Copy link
Copy Markdown
Member

@leolara leolara commented Apr 17, 2026

🗒️ Description

Fix FileNotFoundError in fill --traces when a transaction is rejected mid-processing (e.g. EIP-3607 collision).

When T8N's run_state_test catches an EthereumException from process_transaction, the tx is added to rejected_txs and state is rolled back — but a receipt entry is still produced by Result.updateget_receipts_from_output. Meanwhile the tracer's TransactionEnd event never fires, so no trace file is written. collect_traces then iterates receipts, assumes a matching trace file exists for each, and crashes on shutil.copy / TransactionTraces.from_file.

This patch makes collect_traces tolerant of a missing trace file: when the file isn't on disk, record an empty TransactionTraces and continue. This matches the implicit empty-trace case that already exists for tests with no transactions, and leaves downstream verify-traces comparison symmetric (both sides produce empty traces for rejected txs).

Reproducer (before this PR)

TMPDIR=./.tmp uv run fill \
  tests/ported_static/stCreateTest/test_transaction_collision_to_empty_but_code.py \
  tests/ported_static/stCreateTest/test_transaction_collision_to_empty_but_nonce.py \
  tests/ported_static/stEIP3607/test_init_colliding_with_non_empty_account.py \
  --traces -n 4 --output .tmp/fix --clean
# → 117 failed (FileNotFoundError on .../trace-0-0x<hash>.jsonl)

After this PR: 117 passed.

CI was not catching this because the just fill recipe does not pass --traces.

Scope

One function (TransitionTool.collect_traces in packages/testing/src/execution_testing/client_clis/transition_tool.py), ~6 added lines. No changes to T8N, tracer, or receipt generation — those still warrant a follow-up correctness fix (emit TransactionEnd from the exception path) but are out of scope here.

🔗 Related Issues or PRs

N/A.

✅ Checklist

  • All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    just static
  • 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).

Cute Animal Picture

Two red panda cubs in a tree

Two Red Panda Cubs in the Tree — Mathias Appel, CC0, via Wikimedia Commons

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.26%. Comparing base (40461a6) to head (6cad084).
⚠️ Report is 1 commits behind head on forks/amsterdam.

Additional details and impacted files
@@               Coverage Diff                @@
##           forks/amsterdam    #2709   +/-   ##
================================================
  Coverage            86.26%   86.26%           
================================================
  Files                  599      599           
  Lines                37038    37038           
  Branches              3795     3795           
================================================
  Hits                 31949    31949           
  Misses                4525     4525           
  Partials               564      564           
Flag Coverage Δ
unittests 86.26% <ø> (ø)

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.

@leolara leolara marked this pull request as ready for review April 20, 2026 10:15
Copy link
Copy Markdown
Contributor

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@spencer-tb spencer-tb added C-bug Category: this is a bug, deviation, or other problem C-feat Category: an improvement or new feature A-test-client-clis Area: execution_testing.client_clis labels Apr 20, 2026
@spencer-tb spencer-tb changed the title fix(plugins/transition_tool): tolerate missing trace file for rejected txs feat(test-client-clis): tolerate missing trace file for rejected txs Apr 20, 2026
@spencer-tb spencer-tb merged commit 464f61d into ethereum:forks/amsterdam Apr 20, 2026
21 checks passed
leolara added a commit that referenced this pull request Apr 26, 2026
Convert all ported static tests under `tests/ported_static/` from hard-coded addresses (literal `Address(0x…)` and `EOA(key=…)`) to dynamic allocation (`pre.fund_eoa()`, `pre.deploy_contract()` without `address=`). Tests now behave like hand-written EEST tests, where the framework picks addresses at fill time. 68 of 2151 tests become runnable under the `execute` plugin against live networks (no `pre_alloc_mutable` marker).

Achieved without changing test semantics — verified by 39,002 / 39,002 trace comparisons equivalent under `exact-no-stack` against pre-conversion baseline traces.

### What changed

**`scripts/filler_to_python/analyzer.py`** — bulk of the work. New analyzer logic:

- Topological sort of contracts by bytecode address references; substitute known addresses with variable names (`addr_to_var`) in emitted Op expressions.
- Resolve CREATE-derived addresses in post-state storage and account references via `compute_create_address(...)` (depth 2, nonce 0–15).
- Coinbase pinning: any account whose address equals `env.current_coinbase` is kept hardcoded so the env's `fee_recipient` and the pre-state entry agree.
- Sender pinning when post-state has unresolvable addresses or address-like storage values (>2³² ints) — keeps CREATE addresses derivable from sender consistent with baseline.
- Short-PUSH detection: contracts referenced via `PUSH<20` (i.e. addresses with leading-zero bytes) are pinned to baseline addresses.
- Same for EOAs (precompile-touch case): EOAs referenced via short PUSH or hint-overridden into the precompile range (`<eoa:0x...01>`) are pinned literally at `0x01–0x10`.
- Computed call targets (`address=Op.ADD/MLOAD/CALLDATALOAD/...`) and address arithmetic trigger a global hardcoded fallback.
- `FORCE_HARDCODED_TESTS` allowlist (144 entries) for tests that fundamentally don't survive dynamic addresses (gas-precise tests, keccak-derived storage keys, cryptographic signatures in tx-data, CREATE2 collisions, structural pre-execution rejections).
- `needs_mutable_pre` flag — emitted only when `assert_mutable()` would actually fire (sender or any account hardcoded, sender funded with non-default nonce, or any contract emitted with `nonce=0`).

**`scripts/filler_to_python/ir.py`** — `use_dynamic` on `AccountIR`/`SenderIR`/`AccessListEntryIR`, `nonce` on `SenderIR`, `needs_mutable_pre` on `IntermediateTestModel`.

**`scripts/filler_to_python/templates/state_test.py.j2`** — conditional rendering: `pre.fund_eoa(amount=...)` / `pre.deploy_contract(...)` for dynamic tests, `EOA(key=...)` / `pre[var] = Account(...)` / `pre.deploy_contract(..., address=Address(...))` for hardcoded ones. `@pytest.mark.pre_alloc_mutable` is only emitted when `needs_mutable_pre`.

**`scripts/filler_to_python/render.py`** — `format_storage` handles string variable references; new context fields piped to the template.

**`tests/ported_static/`** — all 2151 test files regenerated. 14 `@manually-enhanced` files preserved as-is via the existing skip mechanism.

### Verification

| Check | Result |
|---|---|
| Regeneration | 2137 / 2151 OK + 14 skipped (manually-enhanced) |
| `just static` | clean |
| Full fill (CI mode) | 60,476 / 60,476 passed |
| Full fill `--traces` | 60,476 / 60,476 passed (depends on PR #2709 in `forks/amsterdam`) |
| `--verify-traces` `exact-no-stack` | **39,002 / 39,002 equivalent** |

> **Note:** The trace report shows 78 additional "divergences" on 3 STRUCTURAL tests (CREATE-collision / EIP-3607). These are spurious and tracked in #2758 — an asymmetry between in-memory and disk-loaded traces in the verify-traces machinery (extends #2709). The tests themselves pass, fixtures are byte-identical to baseline, and the comparison artifact is independent of this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-test-client-clis Area: execution_testing.client_clis C-bug Category: this is a bug, deviation, or other problem C-feat Category: an improvement or new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants