feat(test-benchmark): add deployment script - #3304
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## benchmarks/amsterdam #3304 +/- ##
=====================================================
Coverage 93.41% 93.41%
=====================================================
Files 625 625
Lines 37035 37035
Branches 3386 3386
=====================================================
Hits 34597 34597
Misses 1668 1668
Partials 770 770
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:
|
jochem-brouwer
left a comment
jochem-brouwer
left a comment
There was a problem hiding this comment.
Some comments.
For filling, we have to keep in mind if we are filling under Osaka or under Amsterdam.
I also have the feeling that the gas calculations might be off. Note that we "only" want to deploy prestate here (which we should only do once, because then we take the snapshot) so we do not need to optimize it by for instance minimizing the amount of transactions or blocks.
The test_setup_contracts is not really a test, I think we should move this somewhere out of the tests/ folder (maybe in some helpers folder?)
| intrinsic = fork.transaction_intrinsic_cost_calculator()( | ||
| calldata=b"\xff" * 32 + initcode | ||
| ) | ||
| create_cost = Op.CREATE2( |
There was a problem hiding this comment.
Nit: we could take the code of the deterministic deployment factory (EIP-7997) and use that to calculate the actual gas cost
The code is:
https://eips.ethereum.org/EIPS/eip-7997#specification
0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe03601600081602082378035828234f58015156039578182fd5b8082525050506014600cf3
(pretty sure we have this as constant in EELS somewhere also)
There was a problem hiding this comment.
Added in the new commit.
| calldata=b"\xff" * 32 + initcode | ||
| ) | ||
| create_cost = Op.CREATE2( | ||
| value=0, |
There was a problem hiding this comment.
This is not the behavior of the CREATE2 factory, it will forward CALLVALUE
There was a problem hiding this comment.
This is only for calculating creation cost (test_deploy_existing_contracts), and since there's no value transfer when deploying max code size contracts, it should reflect the exact deployment behavior.
| ).initcode | ||
|
|
||
| base_gas = intrinsic(authorization_list_or_count=0) | ||
| per_auth_gas = intrinsic(authorization_list_or_count=1) - base_gas |
There was a problem hiding this comment.
Doesn´t this also count the base tx gas again?
Does this include that we are creating a new account? (or are we not creating a new account?)
There was a problem hiding this comment.
Fixed in a follow-up commit.
| per_auth_gas = intrinsic(authorization_list_or_count=1) - base_gas | ||
| gas_buffer = 100_000 | ||
| auths_per_tx = max( | ||
| 1, (tx_gas_limit - gas_buffer - base_gas) // per_auth_gas |
There was a problem hiding this comment.
Note that tx_gas_limit is the EIP-7825 limit. If the fork is Amsterdam we can spend state gas on the to-be-created (?) accounts.
We don't have to optimize this situation though (my simple approach was just one delegation per tx 😂 )
There was a problem hiding this comment.
Fix in a follow-up commit.
d2546e6 to
328611c
Compare
Brings test_setup_contracts.py (the CREATE2 receiver + EIP-7702 delegation deployer), its transaction-packing helper, and fill-stateful's --no-reset-between-tests flag, which is what lets a prestate persist rather than being rewound to start_block after each test. This is the branch PR ethereum#3304 targets, based on benchmarks/amsterdam. Deliberately NOT feat-bloatnet-deploy-script: that is the same three commits (patch-id identical) replayed onto an earlier, forks/amsterdam-based version of this branch, where TransactionWithCost had been renamed regular_cost -> execution_cost. Filling that combination dies at construction with a pydantic ValidationError (executionCost missing / regular_cost extra_forbidden); benchmarks/amsterdam still uses regular_cost, which is what this test passes.
2 participants
Description
Related Issues or PRs
N/A.
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