-
Notifications
You must be signed in to change notification settings - Fork 391
feat(execute): Many Improvements to execute remote command
#1822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: forks/osaka
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/osaka #1822 +/- ##
============================================
Coverage 87.31% 87.31%
============================================
Files 541 541
Lines 32832 32832
Branches 3015 3015
============================================
Hits 28668 28668
Misses 3557 3557
Partials 607 607
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:
|
| deploy_gas_limit = min(deploy_gas_limit * 2, 30_000_000) | ||
| print(f"Deploying contract with gas limit: {deploy_gas_limit}") | ||
|
|
||
| self._refresh_sender_nonce() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this logic leads to issues for stateful benchmark testing and may also affect consensus testing.
We first construct a transaction (e.g., a contract deployment). This nonce assignment happens in the post-model initialization phase of the Pydantic model:
if "nonce" not in self.model_fields_set and self.sender is not None:
self.nonce = HexNumber(self.sender.get_nonce())The get_nonce function assumes that the nonce always increases sequentially. However, this assumption breaks in several scenarios:
- If two tests run sequentially, the framework assigns nonces 1 and 2. If the first transaction fails, the second transaction will remain pending indefinitely because its nonce will never become valid.
- If multiple workers submit transactions concurrently and accidentally reuse the same nonce, the duplicate nonce will invalidate one or more of those transactions.
Point 1 is more critical, because we currently cannot ensure that all benchmark tests will run successfully. However, we still want the test suite to continue running without any stop.
This concern was previously raised and solved in PR from @kamilchodola , but removing the logic here would reintroduce the same problems and may also impact his tooling.
One potential solution here is to update the nonce value based on the network's state:
if "nonce" not in self.model_fields_set and self.sender is not None:
self.nonce = HexNumber(elf._eth_rpc.get_transaction_count(self._sender, block_number="pending"))
I've tried this approach before, but i encounter a circular import issue back at that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, agreed, (1) can be addressed by updating the nonce just a single time before running the test function so the nonce is up to date when the test starts requesting contracts and EOAs.
Number (2) I don't see how can it happen since all workers use different senders, otherwise concurrency would fail instantly.
Will fix (1), thanks for pointing out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change! I've reviewed again and this should be resolved.
execute remote command
|
Thanks for the enhancement! Could we also update the related docs so I can provide a reference to Kamil and Jochem? |
ac90263 to
83d8717
Compare
🗒️ Description
Introduces many enhancements to the
execute remoteandexecute hiveby porting ethereum/execution-spec-tests#1566 to this repo and other fixes.Enhancements
Deferred EOA Funding
Changes the behavior of
pre.fund_eoaduring execute to, instead of immediately sending a the transaction to fund the given EOA with a fixed amount of Eth in all tests, save the incomplete funding transaction into a list, and then process the rest of the transactions included in the test that have the given EOA as sender, then calculate the exact amount of Eth that is required to send all of them given current network gas prices, and finally fund the EOA before submitting the test transactions.This behavior only kicks in if
pre.fund_eoais not called with theamountparameter explicitly set. I.e. onlypre.fund_eoa()orpre.fund_eoa(amount=None)use this deferred funding method.This results in:
Dynamic Gas Prices
Execute command now dynamically fetches gas prices before the execution of each test, with the information becoming stale every 12 seconds.
The default gas prices (
gas_price,max_priority_fee_per_gas,max_fee_per_gas,max_fee_per_blob_gas) are now automatically set to 1.5x the current network prices.The 1.5x multiplier is currently constant, but if we deem necessary we could introduce another flag to change this.
To achieve this, we still set a number to the default gas price (7) during transaction instantiation, but we remove
gas_price,max_priority_fee_per_gas,max_fee_per_gas,max_fee_per_blob_gasfrommodel_fields_setif the user did not explicitly set them. Thenexecutepeeks intomodel_fields_setand if the gas prices were not explicitly set, it updates them with the current network prices for all test transactions.Dry-run Mode
This mode is enabled by flag
--dry-runduringexecute remotein order to estimate how much Eth is going to be consumed when running these tests given the current network gas prices.It does not send any transaction to the network, rather it just collects all transactions it would send and adds up the gas used and the gas prices.
Better Nonce Refresh for Worker Senders
This PR also removes
_refresh_sender_noncefrom the execute'sAllocclass and moves this behavior to its own fixture. We now separate theworker_key(Prevsender_key) into two different fixtures:session_worker_key: Determines the key that will fund all contracts and EOAs for the current pytest worker. This fixture is "session" scoped and also cleans up the worker key by sending the funds back to the seed account.worker_key: This fixture takes the key fromsession_worker_keyand updates its nonce from the current value from the network, then it's passed directly to thepreso no update of the nonce has to be performed there.loadscope->loadforexecute remoteandexecute hiveThis is a minor change with the noticeable improvement when running tests from a single python file.
Using
loadscopeis convenient tofillbecause the module's fixtures are normally a bottleneck to test generation, and this way all the tests from the same file are assigned to the same pytest worker.This is not the case for
executesince the worst bottleneck is transaction inclusion in the chain. Now tests are assigned to whichever worker is available so it can run tests while the rest of the workers are waiting for their transactions to be included, even for tests in the same file.Example running command
uv run execute remote --fork=Osaka ./tests/frontier/opcodes/test_dup.py --rpc-seed-key $RPC_SEED_KEY --rpc-endpoint $RPC_ENDPOINT --chain-id $RPC_CHAIN_ID -n 8with a Kurtosis local network:loadscope: 16 passed, 9 warnings in 602.58s (0:10:02)load: 16 passed, 9 warnings in 97.46s (0:01:37)Note:
test_dup.pyis currently broken inforks/osakabecause it sets thenonceand thegas_price, so the command will definitely fail when trying to reproduce this command.Increased Logging
loggeris now used more frequently insender.py,pre_alloc.pyandexecute.py, to print information about the transaction execution on chain during test execution, and also more verbose errors.Blob Tests In Execute
All
blob_testtests can now be run withexecute, by skipping thegetBlobsVXcheck which requires the Engine API.This is a minor improvement mainly targeted to sending blob transactions on live networks, rather than attempting to test the
engine_getBlobsVXendpoints.The blob gas prices are also fetched from the network live during execution of the tests.
--eoa-fund-amount-defaultis removed.--sender-key-initial-balanceinexecute hiveis now--seed-key-initial-balance--default-gas-price,--default-max-fee-per-gasand--default-max-priority-fee-per-gasnow default toNoneand ideally should be omitted because, when unset, the command now defaults to fetch the value from the network, which is a more reliable behavior.🔗 Related Issues or PRs
N/A.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e 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