Test reservoir refunded at top level when parent halts after child sp…#2644
Conversation
…ills and reverts. Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
jochem-brouwer
left a comment
jochem-brouwer
left a comment
There was a problem hiding this comment.
Left some thoughts, mainly confused by the spec 😅
| # restored from the child refund is preserved for top-level refund. | ||
| # block_regular = gas_limit - sstore_state_gas (reservoir kept out) | ||
| # block_state = 0 (nothing actually grown) | ||
| expected_gas_used = gas_limit_cap - sstore_state_gas |
There was a problem hiding this comment.
I am confused by this (and also by spec). The regular gas here for Op.SSTORE(0,1) will cost 2900 (EIP-8037) + 2100 (cold access, EIP-2929) = 5000 gas, and the state gas is 32 * 1174 = 37568. If this is fully consumed by reservoir (and thus not eating into regular gas) then we now get "free refunds" it seems. If I would spam storage creation and then invalidate it in the end I think I could even end up with negative gas here (?)
There was a problem hiding this comment.
Never mind, I misunderstood the EIP
There was a problem hiding this comment.
No wait, in this test the reservoir is initialized to 0, right? So there is no storage gas available, it will all directly eat in regular gas.
| child = pre.deploy_contract(code=(Op.SSTORE(0, 1) + Op.REVERT(0, 0))) | ||
|
|
||
| parent = pre.deploy_contract( | ||
| code=(Op.POP(Op.CALL(gas=500_000, address=child)) + Op.INVALID), |
There was a problem hiding this comment.
Since the child frame reverts it will not consume all gas so the gas= param is technically not necessary here (we can send all gas).
Also it seems that this test is almost equal to the other test, the only difference seems if either REVERT/INVALID is used in the child contract, I think we should parametrize that.
|
|
||
| tx = Transaction( | ||
| to=parent, | ||
| gas_limit=gas_limit_cap, |
There was a problem hiding this comment.
| gas_limit=gas_limit_cap, | |
| gas_limit=gas_limit_cap + sstore_state_gas, |
This inits the reservoir to have state gas.
Note: this also means it can at most refund sstore_state_gas, so this might not cover all cases, I think we should add multiple gas limits, where we use sstore_state_gas - 1 and sstore_state_gas + 1 as extra gas on top of the max gas limit to take into account off-by-one errors.
|
|
||
| tx = Transaction( | ||
| to=parent, | ||
| gas_limit=gas_limit_cap, |
There was a problem hiding this comment.
Gas limit should be changed here too to make gas from reservoir available!
|
CIFO: #2644 Incorperated your comments @jochem-brouwer. Will add you as co-author @daniellehrner before merging! Thx |
Ports three tests from the closed PR ethereum#2639 that cover reservoir behavior paths not exercised by the merged ethereum#2689/ethereum#2704/ethereum#2707 tests. test_top_level_halt_preserves_restored_reservoir (parametrized reservoir_delta in {-1, 0, 1} x child_termination in {revert, halt}) Regression test for the bal-devnet-3 Besu bug (ethereum#2644). Child runs an SSTORE then fails, restoring state gas to the parent. Parent then INVALIDs, triggering the top-level failure refund. Expected `header.gas_used = gas_limit_cap + min(reservoir_delta, 0)` so the reservoir (including any spill-restore) is preserved across the halt. test_callcode_value_no_new_account_state_gas CALLCODE transfers value to the caller, not to the target, so no new-account state gas is ever charged regardless of whether the target exists. The reservoir stays intact for a subsequent SSTORE. test_create_oog_during_state_gas_charge Parent CALLs an inner with only 20k gas forwarded. The inner's CREATE charges GAS_NEW_ACCOUNT which exceeds the forwarded budget, OOGing before any state gas lands. Per PR ethereum#2704 the refund restores the parent's reservoir and the parent's subsequent SSTORE succeeds from it.
Ports three tests from the closed PR ethereum#2639 that cover reservoir behavior paths not exercised by the merged ethereum#2689/ethereum#2704/ethereum#2707 tests. test_top_level_halt_preserves_restored_reservoir (parametrized reservoir_delta in {-1, 0, 1} x child_termination in {revert, halt}) Regression test for the bal-devnet-3 Besu bug (ethereum#2644). Child runs an SSTORE then fails, restoring state gas to the parent. Parent then INVALIDs, triggering the top-level failure refund. Expected `header.gas_used = gas_limit_cap + min(reservoir_delta, 0)` so the reservoir (including any spill-restore) is preserved across the halt. test_callcode_value_no_new_account_state_gas CALLCODE transfers value to the caller, not to the target, so no new-account state gas is ever charged regardless of whether the target exists. The reservoir stays intact for a subsequent SSTORE. test_create_oog_during_state_gas_charge Parent CALLs an inner with only 20k gas forwarded. The inner's CREATE charges GAS_NEW_ACCOUNT which exceeds the forwarded budget, OOGing before any state gas lands. Per PR ethereum#2704 the refund restores the parent's reservoir and the parent's subsequent SSTORE succeeds from it.
3 participants
🗒️ Description
Adds a new test for EIP-8037 that checks that the state gas reservoir is refunded at top level when parent halts after child spills and reverts
🔗 Related Issues or PRs
N/A.
✅ Checklist
just 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