refactor(tests): EIP-8037 test gas calculation logic and post state verification - #3383
refactor(tests): EIP-8037 test gas calculation logic and post state verification#3383LouisTsai-Csie wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3383 +/- ##
================================================
Coverage 93.53% 93.53%
================================================
Files 624 624
Lines 37070 37074 +4
Branches 3394 3394
================================================
+ Hits 34675 34679 +4
Misses 1645 1645
Partials 750 750
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:
|
| Block( | ||
| txs=txs, | ||
| header_verify=Header( | ||
| gas_used=max(block_execution, block_state) |
There was a problem hiding this comment.
Could we make the intended state dominance explicit here and pin the header directly to block_state?
assert block_state > block_execution, "requires state gas to dominate"
...
header_verify=Header(gas_used=block_state)| stop_execution = intrinsic_calc() | ||
|
|
||
| expected = max(create_execution + stop_execution, create_state_gas) | ||
| expected = max(create_execution + stop_execution, create_state) |
There was a problem hiding this comment.
Could we assert that state dominates and pin expected = create_state? To ensure this test specifically validates the CREATE state gas charges
| execution gas plus the account-creation state gas, and not the | ||
| legacy combined execution account-creation cost. | ||
| """ | ||
| # TODO: Modify to subcall scenario |
There was a problem hiding this comment.
Shall we add this to the testnet tracker issue?
| @pytest.mark.parametrize( | ||
| "num_txs,num_sstores", | ||
| [ | ||
| pytest.param(1, 1, id="single_sstore_single_tx"), |
There was a problem hiding this comment.
Should this be dropped?
Description
While extending EIP-8037 coverage I reviewed the existing suite and noticed that a number of tests do not restrict the behavior the test names and docstring intended to do. This PR marks those tests and starts tightening them.
Case 1: set
state_gas_reserviorto 0 does not mean no state gas chargeTake
test_state_gas_selfdestruct.py::test_selfdestruct_existing_beneficiary_no_state_gasas a representative example:In my opinion, some of the verification is not strict enough, below is an example for unmodified
test_state_gas_selfdestruct:The test intends to show that a
SELFDESTRUCTsweep to an existing beneficiary charges no state gas. It has two independent problems.It exercises the opposite scenario.
pre.fund_eoa(amount=0)returns a fresh address but writes nothing to the pre-allocation, so the selfdestruting actually create a new account, charging theNEW_ACCOUNTcost. However, the test implementation does not catch the error at all.In this test, the
tx.gas_limitis not configured, while thestate_gas_reserviorconfigure to 0. In this case, the transaction gas limit case would be configured to transaction gas limit cap. So it is possible that a misimplemented client draw gas fromgas_leftand increasestate_gas_from_gas_left, and the transaction is still possible to pass.Transactions are constructed via
BlockchainTest::generate_block_data, which callswith_gas_limit()to set gas limits:If
gas_limitis not already set,with_gas_limit()calculates it implicitly:In
_calculate_implicit_gas_limit(), whenstate_gas_reservoir_enabledis True, the transaction gas limit becomes:To actually constrain the state dimension, a test needs one of: (1) Header verification, and (2) A transaction gas limit that excludes the state cost.
Case 2: If the test is intended to validate state gas calculation, the header gas cost should not be derived from
max(execution_gas, state_gas)Take
test_create_selfdestruct_code_deposit_no_refund_header_checkas an example. This test verifies that code deposit costs are not refunded (state gas cost dimension). Currently, usingmax(state_gas, execution_gas)works fine because state gas always dominates in Amsterdam.However, consider future upgrades where: (1) execution gas costs for certain operations increase significantly, or (2) state gas costs drop substantially. If
execution_gasever exceedsstate_gas, the test would still pass -> but it would no longer validate the state gas for "no refund" scenario. The test would be measuring a different code path without catching the regression.In other words: the test's intention (validating no-refund behavior) now depends on which gas component is larger. Using max() masks this dependency and creates a brittle test that silently fails to catch the intended behavior change.
There are several instances in the test suite, this PR adds stricter verification to tighten it.
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