feat(spec,tests): Implement EIP-8282 - #3070
Conversation
* feat(specs): Implement EIP-8282 * feat(test-forks): Implement EIP-8282 framework changes * feat(tests): Implement EIP-8282 tests * feat(tests): Add EIP-8282 to EIP-7685 tests * fix(test-forks): Update EIP-8282 deposit contract to 537b9c1 * fix(tests): Update EIP-8282 builder deposit contract target/max to 32/256 * fix(tests): Failing tests after update * refactor(test-forks): Introduce `minimum_block_gas_limit` * refactor(test-forks): Update EIP-8282 `empty_block_bal_item_count` * refactor(test-forks): Use pkgutil to load contract binaries * refactor(test-forks): Use pkgutil to load EIP-8282 contract binaries * refactor(test-forks): Remove pre_allocation_blockchain type: ignore comments * refactor(test-forks): Remove EIP-8282 pre_allocation_blockchain type: ignore comments * Update tests/amsterdam/eip8282_builder_execution_requests/__init__.py Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * Update tests/amsterdam/eip8282_builder_execution_requests/spec.py Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * chore(tests): Remove `dataclass` from `Spec` class EIP-8282 * chore(tests): Make test behavior more explicit Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * chore(spec,tests): set EIP-8282 builder addresses for glamsterdam-devnet-6 --------- Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Co-authored-by: spencer-tb <spencer.tb@ethereum.org>
34f05ea to
26c45dc
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3070 +/- ##
===================================================
+ Coverage 93.22% 93.23% +0.01%
===================================================
Files 624 624
Lines 36939 36936 -3
Branches 3377 3375 -2
===================================================
+ Hits 34436 34439 +3
+ Misses 1709 1704 -5
+ Partials 794 793 -1
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:
|
LouisTsai-Csie
left a comment
There was a problem hiding this comment.
Good from my side, some suggestion that could done in follow-up PRs:
- Add builder deposit / exit system contract to
eth_configrequest. - Do we need a
test_eip_mainnet.py/ fork transition test (so that we could test out )?
LouisTsai-Csie
left a comment
There was a problem hiding this comment.
I have one question related to requests_hash , otherwise it looks good! I like the design of SystemContractInteractionMeasuredOutOfGasContract, amazing!
| blocks=[ | ||
| Block( | ||
| txs=txs, | ||
| requests_hash=Requests(*requests_list), |
There was a problem hiding this comment.
| requests_hash=Requests(*requests_list), |
This looks like a no-op?
requests_hash on Block is a field inherited from Header. We can pass it into Block.requests_hash, but it is never verified against anything:
The header used for verification is rebuilt from the transition-tool (t8n) output.
execution-specs/packages/testing/src/execution_testing/specs/blockchain.py
Lines 891 to 909 in 2d68578
requests_hash is an output field, so it isn't a t8n input (it's not carried into the Environment) and it isn't merged into the re-built header either, that is, header.requests_hash always comes from the transition output.
execution-specs/packages/testing/src/execution_testing/specs/blockchain.py
Lines 933 to 942 in 2d68578
I've confirmed this by updating to Block(requests_hash=requests_hash=Requests(*[]),). It sill fills successfully and the fixture header keeps the real t8n value, so the field configuration is unused.
There was a problem hiding this comment.
Yep, and I can confirm that this is a copy-paste error because it's also in the source 7002 test file. We should fix this in the #3071.
spencer-tb
left a comment
There was a problem hiding this comment.
LGTM!
Feel free to add these before merge: #3090
* fix(tests): exceed per-block max in builder deposit carry-over test * chore(tests): clarify multi-type request test gas limit comment
🗒️ Description
Implements the current spec iteration of this PR: ethereum/EIPs#11760.
Matches glamsterdam-devnet-6 specs.
🔗 Related Issues or PRs
N/A.
✅ Checklist
just statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture