fix(tests,test-fill): fix Engine X fills on BAL forks - #3219
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3219 +/- ##
===================================================
+ Coverage 93.39% 93.41% +0.01%
===================================================
Files 624 624
Lines 37014 37014
Branches 3386 3386
===================================================
+ Hits 34570 34576 +6
+ Misses 1671 1668 -3
+ Partials 773 770 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
spencer-tb
added
C-bug
|
Forced pushed these commits to |
marioevz
left a comment
marioevz
left a comment
There was a problem hiding this comment.
My main takeaway from this review is that #3122 broke the soundness of the pre-allocation grouping, and it tried to side-step this issue with packages/testing/src/execution_testing/fixtures/engine_x_checks.py. This is the second time in two weeks that this file breaks (#3176), and it's going to keep on breaking in the future.
I don't think this PR is incorrect, is just that we are patching a patch that is going to grow in complexity and bring us more issues in the future.
| params = entry.get("params") | ||
| if params and isinstance(params[0], dict): | ||
| payload = params[0] | ||
| bal = payload.get("blockAccessList") |
There was a problem hiding this comment.
The problem with this is that we are passing fixture as a black-boxed python dictionary. We have the definitions of all the fixture types so we can parse them (yes it's another round of json parsing, but that's a problem with the approach this file takes to try to make its verifications).
| return [_masked(child, parent_hash) for child in node] | ||
|
|
||
|
|
||
| def _scrubbed_bal(bal_hex: str, parent_hash_hex: str) -> Any: |
There was a problem hiding this comment.
It's not great that we need this in the first place, this highlights how brittle this file is.
There was a problem hiding this comment.
This file is just a nightmare to maintain: it introduced a post-fact verification and provides zero indication about the cause when there's a failure.
|
Let me create an issue or PR to try to make the file less brittle |
2 participants
Description
The
tests-glamsterdam-devnet@v7.2.1release fill failed withEngineXExecutionDriftErroron 18049 of 24683 Engine X fixtures.On BAL forks the EIP-2935 system call writes the parent hash (the group genesis hash at payload 0) into every payloads
blockAccessList, so the check flagged an expected state root derived difference. Decode the BAL and mask the (leading zero-trimmed) parent hash instead of comparing it verbatim, keeping the rest of the BAL in the comparison so leaked accounts are still caught.The ported static Amsterdam skip list never matched Engine X variants (format token stripping mangled
blockchain_test_engine_xids into_x_from_state_test), andtest_gas_cost_returnblind calls undeclared0x1000/0x2000, which pre alloc packing can populate (isolated as in #3176).Validated with a full
tests/ported_staticfill at Amsterdam with--generate-all-formats: 25144 passed, 0 failed, 6286 Engine X fixtures execute identically against their packed group's genesis.Related Issues or PRs
Fixes the
fill (amsterdam)failure in the glamsterdam-devnet@v7.2.1 release dispatch, same failure class as the Benchmarking CI red since #3187.Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture