feat(specs,tests): Implement EIP-2780#2175
Conversation
…#2175) * refactor(bal): absence validators -> BalAccountAbsentValues * feat: raise for empty lists on BAL absence checks * refactor: changes from comments on PR ethereum#2175
…#2175) * refactor(bal): absence validators -> BalAccountAbsentValues * feat: raise for empty lists on BAL absence checks * refactor: changes from comments on PR ethereum#2175
SamWilsn
left a comment
There was a problem hiding this comment.
Quick, incomplete review
|
There's a possibility this becomes a blocker for Devnet-3 since it's listed as requirement for https://eips.ethereum.org/EIPS/eip-8037 which is included. Spencer checked with Maria, and it seems like it's not a hard requirement but it might be a good idea to get this ready to merge. |
To my mind, this needs further clarity, especially when it comes to the EIP-7702 interactions. @jochem-brouwer has nicely summarised some of the open questions in this eth magicians thread |
|
Raised a PR on the EIPs repo to clarify some interactions with EIP-7702 and EIP-7928. See here We can proceed to write more tests specific to these cases once we agree on these points. |
3f816ea to
0d197cc
Compare
044f9b3 to
4c9e3e4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## eips/amsterdam/eip-2780 #2175 +/- ##
===========================================================
+ Coverage 86.47% 88.40% +1.92%
===========================================================
Files 599 524 -75
Lines 37632 31721 -5911
Branches 3795 3036 -759
===========================================================
- Hits 32542 28042 -4500
+ Misses 4526 3164 -1362
+ Partials 564 515 -49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Converting back to draft so I can re-base this on the latest |
aa8045c to
f04a782
Compare
|
@danceratopz This got a bit out of hand in terms of PR size. If this is too much, I'd be happy to split this up into the following 2 PRs.
Do let me know what you think |
SamWilsn
left a comment
There was a problem hiding this comment.
From the spec side, my review is complete.
33a093e to
354ecc8
Compare
danceratopz
left a comment
There was a problem hiding this comment.
Hey @gurukamath, here's some initial feedback. I must admit, I need to go deeper and improve my understanding to do this justice in a second round. I haven't looked at the tests updated for interactions yet. Will come back to this.
8698153 to
1ae2024
Compare
That's a good idea, @gurukamath, I think splitting the older fork tests out would be a great idea to give more targeted reviews. |
danceratopz
left a comment
There was a problem hiding this comment.
Hey @gurukamath, here's something that stood out really quickly upon taking another look at these.
danceratopz
left a comment
There was a problem hiding this comment.
Hey @gurukamath, here's a second round after reviewing tests/amsterdam/eip7928_block_level_access_lists/.
| @pytest.mark.parametrize( | ||
| "fails_at_extcodesize", | ||
| [True, False], | ||
| ids=["oog_at_extcodesize", "successful_extcodesize"], |
There was a problem hiding this comment.
I think "successful_extcodesize" as-is deviates from the original test's intent; it should be renamed to oog_phase2_at_extcodesize, in which case, how about?
| ids=["oog_phase1_at_extcodesize", "oog_phase2_at_extcodesize"], |
The question is do we need to add a new test case here "successful_extcodesize", or is it covered elsewhere?
There was a problem hiding this comment.
I did not include a case here since it is covered specifically in tests/amsterdam/eip2780_reduce_intrinsic_tx_gas/test_account_access_opcodes.py::test_account_access_opcode. The original idea was to keep the implementation (and tests) of EIP-7928 and EIP-2780 as independent as possible. Not sure how essential this separation is anymore after the debates we've been having about merging stuff to forks/amsterdam
| Op.PUSH20(target_contract) # Target contract address | ||
| + Op.EXTCODESIZE(address_warm=False) # Check code size (cold access) | ||
| + Op.EXTCODESIZE( | ||
| address_warm=False, address_has_code=False |
There was a problem hiding this comment.
We only have an Op.STOP, but unless this changes too, we should rename "successful_extcodesize" to "phase2_oog_at_extcodesize". We could consider adding a third successful case but I think the happy case is covered here https://github.com/gurukamath/execution-specs/blob/eip-2780/implement-eip-final/tests/amsterdam/eip7928_block_level_access_lists/test_block_access_lists.py#L246.
| address_warm=False, address_has_code=False | |
| address_warm=False, address_has_code=True |
There was a problem hiding this comment.
I think the test ids are a bit misleading and might need to change. The test essentially looks at the target being in BAL vs target not being in BAL. Pre 2780 these two scenarios exactly coincide with Success vs OOG of the opcode. Post 2780, they don't. Which is also why there is a disconnect between the target having code and us using address_has_code=False.
danceratopz
left a comment
There was a problem hiding this comment.
I reviewed all other non-specific Amsterdam tests for hard-coded constants. Didn't add inline comments here, just Clauded these up.Each addresses a few missing hard-coded gas constants from a single fork (Cancun, Prague, Osaka). I still need to self-review them:
Squashes the previous EIP-2780 branch onto the current forks/amsterdam tip to account for the upstream GasCosts class refactor (module-level GAS_* constants consolidated into GasCosts.* class attributes). Spec changes (src/ethereum/forks/amsterdam/): - Add COLD_ACCOUNT_COST_CODE (2600), COLD_ACCOUNT_COST_NO_CODE (500), STATE_UPDATE (1000) to GasCosts - Reduce TX_BASE from 21000 to 4500 - Split cold account access in BALANCE/EXTCODESIZE/EXTCODECOPY/ EXTCODEHASH/CALL/CALLCODE/DELEGATECALL/STATICCALL by code presence - Restructure CALL value transfer: STATE_UPDATE + NEW_ACCOUNT for empty targets, 2 * STATE_UPDATE for non-empty - Add compute_recipient_cost helper in process_transaction Testing framework (packages/testing/): - Add RecipientType enum and propagate sends_value / recipient_type / recipient_is_warm / recipient_delegation_is_warm through the TransactionIntrinsicCostCalculator signature in EIP2, EIP2930, EIP7623, EIP7702 and Frontier base - Add EIP2780 class with gas_costs, transaction_intrinsic_cost_calculator, _calculate_call_gas and _with_account_access overrides - Extend GasCosts dataclass with COLD_ACCOUNT_COST_CODE, COLD_ACCOUNT_COST_NO_CODE, STATE_UPDATE (default 0 for pre-Amsterdam) Tests: - Add tests/amsterdam/eip2780_reduce_intrinsic_tx_gas/ covering value- moving transactions, calls, precompile transfers and account-access opcodes - Update blob, 7623, 7702, BAL, and tx-gas-limit tests to use the extended intrinsic cost calculator signature
3ca1df3 to
485232b
Compare
|
@danceratopz I have merged the changes you suggested in the 3 PRs as well as rebased this on the latest Next steps would be to write tests for more scenarios as well as fix the fails in the |
danceratopz
left a comment
There was a problem hiding this comment.
Thanks @gurukamath for the rebase and merging the test updates in.
Two comments below. Even if they're valid, I don't want to block a merge, so giving an approve! We can tackle other fixes in follow-up PRs. But as you like!
| call_value_cost = ( | ||
| Uint(0) if value == 0 else Uint(2) * GasCosts.STATE_UPDATE | ||
| ) |
There was a problem hiding this comment.
Wouldn't this count as "self-call" as caller == to in CALLCODE? So there should only be one state update:
| call_value_cost = ( | |
| Uint(0) if value == 0 else Uint(2) * GasCosts.STATE_UPDATE | |
| ) | |
| call_value_cost = Uint(0) if value == 0 else GasCosts.STATE_UPDATE |
If you agree, we should update the transaction_intrinsic_cost_calculator() and potentially tests this touches.
Also happy to do/discuss this as a follow-up!
| if call_target == EMPTY_ACCOUNT: | ||
| call_value_cost += GasCosts.STATE_UPDATE + GasCosts.NEW_ACCOUNT | ||
| else: | ||
| call_value_cost += Uint(2) * GasCosts.STATE_UPDATE |
There was a problem hiding this comment.
I think we need to check whether we're in a "self-call" context here and only charge GasCosts.STATE_UPDATE in that case.
Thanks for the review Dan. Yes. The changes are relevant. The self-call semantics weren't part of he EIP when we implemented this PR and have only been added to the EIP recently, along with the transfer log costs associated with 7708. I am planning to add both of those in a follow-up PR. So I will go ahead and merge this for now. |
8af3496
into
ethereum:eips/amsterdam/eip-2780
) Squashes the previous EIP-2780 branch onto the current forks/amsterdam tip to account for the upstream GasCosts class refactor (module-level GAS_* constants consolidated into GasCosts.* class attributes). Spec changes (src/ethereum/forks/amsterdam/): - Add COLD_ACCOUNT_COST_CODE (2600), COLD_ACCOUNT_COST_NO_CODE (500), STATE_UPDATE (1000) to GasCosts - Reduce TX_BASE from 21000 to 4500 - Split cold account access in BALANCE/EXTCODESIZE/EXTCODECOPY/ EXTCODEHASH/CALL/CALLCODE/DELEGATECALL/STATICCALL by code presence - Restructure CALL value transfer: STATE_UPDATE + NEW_ACCOUNT for empty targets, 2 * STATE_UPDATE for non-empty - Add compute_recipient_cost helper in process_transaction Testing framework (packages/testing/): - Add RecipientType enum and propagate sends_value / recipient_type / recipient_is_warm / recipient_delegation_is_warm through the TransactionIntrinsicCostCalculator signature in EIP2, EIP2930, EIP7623, EIP7702 and Frontier base - Add EIP2780 class with gas_costs, transaction_intrinsic_cost_calculator, _calculate_call_gas and _with_account_access overrides - Extend GasCosts dataclass with COLD_ACCOUNT_COST_CODE, COLD_ACCOUNT_COST_NO_CODE, STATE_UPDATE (default 0 for pre-Amsterdam) Tests: - Add tests/amsterdam/eip2780_reduce_intrinsic_tx_gas/ covering value- moving transactions, calls, precompile transfers and account-access opcodes - Update blob, 7623, 7702, BAL, and tx-gas-limit tests to use the extended intrinsic cost calculator signature
🗒️ Description
Implement EIP-2780 and some basic tests.
Note: This PR contains some testing framework changes which are not compatible with other Amsterdam tests. This PR however, does not focus on fixing those. Those should be fixed in a future PR.
As a result, the CI is not likely to pass for Amsterdam
🔗 Related Issues or PRs
issue #1940
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.Cute Animal Picture
TBD