-
Notifications
You must be signed in to change notification settings - Fork 174
chore(execute): add gas validation for benchmark tests in execute mode #2219
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
chore(execute): add gas validation for benchmark tests in execute mode #2219
Conversation
Just unsure if tests should be added to prove that this works or not. I haven't seen many tests for the core of the lib. So LMK if you want me to add them and where (as I locally tested with a python script). |
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 fix, it would be nice if we align the behavior between fill
and execute
mode, i left some suggested changes.
Previously, execute mode was not validating that transactions consumed the expected amount of gas when expected_benchmark_gas_used was set. This could cause benchmark tests to incorrectly pass even when consuming significantly less gas than expected (e.g., due to missing factory contracts). This feature is needed by benchmark tests like the ones in ethereum#2186 in order to make sure that the benchmarks are indeed consuming all gas available or causing a failure otherwise when the flag is set. Changes: - Add expected_benchmark_gas_used and skip_gas_used_validation fields to TransactionPost - Implement gas validation logic in TransactionPost.execute() using transaction receipts - Pass gas validation parameters from StateTest and BlockchainTest to TransactionPost - Add eth_getTransactionReceipt RPC method to fetch gas used from receipts This ensures benchmark tests fail appropriately when gas consumption doesn't match expectations, preventing false positives in performance testing.
9d894e2
to
5e4fe91
Compare
Unsure why I needed to force-push. Anyways, I refactored the code to mimic BTW, nice review. Your suggestions indeed were nice! |
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.
I left a comment for some refactoring, inside the base_test_parametrizer_func
function in execute.py
file, we should configure default value for expected_benchmark_gas_used
, not gas_benchmark_value
.
Based on this, we do not need to pass the gas_benchmark_value
from BlockchainTest
and StateTest
to the TransactionPost object, as it is already configured during test env configuration phase.
Please let me know if it is not clear.
Addresses review comment to make execute mode gas validation cleaner: - Set expected_benchmark_gas_used to gas_benchmark_value as default in execute parametrizer - Remove gas_benchmark_value parameter from TransactionPost, StateTest, BlockchainTest, and BaseTest - Simplify gas validation logic in TransactionPost This ensures consistent gas validation behavior between fill and execute modes with a cleaner implementation that sets defaults at the parametrizer level.
@LouisTsai-Csie thanks so much for the tips. I missunderstood the approach at first. I hope this iteration is closer to what you wanted! |
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.
LGTM! Thanks for the fix. I've testes it locally using Anvil
, and it works as expected.
@LouisTsai-Csie should we merge? All checks passing and your test locally succeeds. |
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.
LGTM from my side! Thanks :)
🗒️ Description
Previously, execute mode was not validating that transactions consumed the expected amount of gas when expected_benchmark_gas_used was set. This could cause benchmark tests to incorrectly pass even when consuming significantly less gas than expected (e.g., due to missing factory contracts).
This feature is needed by benchmark tests like the ones in #2186 in order to make sure that the benchmarks are indeed consuming all gas available or causing a failure otherwise when the flag is set.
Changes:
This ensures benchmark tests fail appropriately when gas consumption doesn't match expectations, preventing false positives in performance testing.
🔗 Related Issues or PRs
Se this discussion: #2186 (comment). for more context.
✅ Checklist
tox
checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
type(scope):
.mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_from
marker.