📄 docs(tests): Update EIP-7928 test case document to include no-op and OOG #2116
Conversation
raxhvl
added
scope:tests
| | `test_bal_noop_storage_write` | Ensure BAL includes address with storage read for no-op writes where pre-state equals post-state | Contract with pre-existing storage value `0x42` in slot `0x01`; transaction executes `SSTORE(0x01, 0x42)` | BAL **MUST** include the contract address with `storage_reads` for the slot since it was accessed, but **MUST NOT** include it in `storage_changes` | 🟡 Planned | | ||
| | `test_bal_oog_intrinsic` | Ensure BAL handles intrinsic out-of-gas (OOG) failure correctly | Transaction with gas limit below intrinsic cost | BAL **MUST** be empty for transactions that fail intrinsic gas validation | 🟡 Planned | | ||
| | `test_bal_oog_sstore` | Ensure BAL handles OOG before `SSTORE` execution correctly | Alice calls `StorageContract` that executes `SSTORE` to cold slot `0x01` with insufficient gas for cold storage write | BAL **MUST** include `StorageContract` in `account_changes` but **MUST NOT** include it in `storage_changes` for slot `0x01` | 🟡 Planned | | ||
| | `test_bal_oog_sload` | Ensure BAL handles OOG before `SLOAD` execution correctly | Alice calls `StorageContract` that executes `SLOAD` from cold slot `0x01` with insufficient gas for cold storage read | BAL **MUST** include `StorageContract` in `account_changes` but **MUST NOT** include it in `storage_reads` for slot `0x01` | 🟡 Planned | |
There was a problem hiding this comment.
BAL **MUST** include StorageContract in account_changes If by StorageContract it is meant bytecode, it is already included as it is target for the CALL/CREATE/etc, maybe I am missing something but this check is unneeded.
There was a problem hiding this comment.
The StorageContract is deployed with an SLOAD operation:
storage_contract = pre.deploy_contract(
code=Op.SLOAD(0x01)
)We verify two things in the generated BAL:
- The
StorageContractis recorded (since it's the target of a CALL). storage_readsfor this account entry does not include slot 0x01. ( infact its empty in this case)
This comes across as verbose, in practice it is implied when we check for the following subset in generated BAL:
[
....
{
'address': storage_contract,
'storage_reads': []
}
]maybe I am missing something but this check is unneeded.
(1) is a sanity check that ensures that StorageContract was infact called. I have reworded the test case for clarity:
> BAL MUST NOT contain slot 0x01 in the storage_reads of StorageContract
|
Delegated accounts (EIP-7702) are not covered, when CALL/DELEGATECALL/STATICCALL/CALLCODE is called for delegated account there are edge case where first account get loaded but we dont have gas for second load etc. There are two variants:
|
| | `test_bal_system_dequeue_withdrawals_eip7002` | Ensure BAL tracks post-exec system dequeues for withdrawals | Pre-populate EIP-7002 withdrawal requests; produce a block where dequeues occur | BAL **MUST** include the 7002 system contract with `storage_changes` (queue head/tail slots 0–3) using `tx_index = len(txs)` and balance changes for withdrawal recipients | 🟡 Planned | | ||
| | `test_bal_system_dequeue_consolidations_eip7251` | Ensure BAL tracks post-exec system dequeues for consolidations | Pre-populate EIP-7251 consolidation requests; produce a block where dequeues occur | BAL **MUST** include the 7251 system contract with `storage_changes` (queue slots 0–3) using `tx_index = len(txs)` | 🟡 Planned | | ||
| | `test_bal_noop_storage_write` | Ensure BAL includes address with storage read for no-op writes where pre-state equals post-state | Contract with pre-existing storage value `0x42` in slot `0x01`; transaction executes `SSTORE(0x01, 0x42)` | BAL **MUST** include the contract address with `storage_reads` for the slot since it was accessed, but **MUST NOT** include it in `storage_changes` | 🟡 Planned | | ||
| | `test_bal_oog_intrinsic` | Ensure BAL handles intrinsic out-of-gas (OOG) failure correctly | Transaction with gas limit below intrinsic cost | BAL **MUST** be empty for transactions that fail intrinsic gas validation | 🟡 Planned | |
There was a problem hiding this comment.
I'd add other situations where the transaction is aborted too, i.e. running into an invalid opcode (Op.INVALID), or reverting (Op.REVERT).
| | `test_bal_system_dequeue_withdrawals_eip7002` | Ensure BAL tracks post-exec system dequeues for withdrawals | Pre-populate EIP-7002 withdrawal requests; produce a block where dequeues occur | BAL **MUST** include the 7002 system contract with `storage_changes` (queue head/tail slots 0–3) using `tx_index = len(txs)` and balance changes for withdrawal recipients | 🟡 Planned | | ||
| | `test_bal_system_dequeue_consolidations_eip7251` | Ensure BAL tracks post-exec system dequeues for consolidations | Pre-populate EIP-7251 consolidation requests; produce a block where dequeues occur | BAL **MUST** include the 7251 system contract with `storage_changes` (queue slots 0–3) using `tx_index = len(txs)` | 🟡 Planned | | ||
| | `test_bal_noop_storage_write` | Ensure BAL includes address with storage read for no-op writes where pre-state equals post-state | Contract with pre-existing storage value `0x42` in slot `0x01`; transaction executes `SSTORE(0x01, 0x42)` | BAL **MUST** include the contract address with `storage_reads` for the slot since it was accessed, but **MUST NOT** include it in `storage_changes` | 🟡 Planned | | ||
| | `test_bal_oog_intrinsic` | Ensure BAL handles intrinsic out-of-gas (OOG) failure correctly | Transaction with gas limit below intrinsic cost | BAL **MUST** be empty for transactions that fail intrinsic gas validation | 🟡 Planned | |
There was a problem hiding this comment.
Also as I mentioned in the other PR, perhaps insufficient-intrinsic-gas is not the correct approach to test since, normally, exception tests obscure other checks, so any block-level access list check will be irrelevant when another verification failed earlier during the block verification process (i.e. we won't even get to the part where the BAL is checked because the block contained an invalid transaction).
I think the approach should be to test OOG, reverts, invalid-opcode, or other types of EVM execution aborts, and verify that the BAL, when something like that happens during the EVM execution of a transaction that was successfully included in the block, is still correctly calculated.
There was a problem hiding this comment.
Make sense. I have replaced intrinsic gas failure test with EVM INVALID and REVERT.
raxhvl
force-pushed
the
feat/eip-7928/test-cases
branch
from
2104098 to
e7367f8
Compare
| | `test_bal_system_dequeue_consolidations_eip7251` | Ensure BAL tracks post-exec system dequeues for consolidations | Pre-populate EIP-7251 consolidation requests; produce a block where dequeues occur | BAL **MUST** include the 7251 system contract with `storage_changes` (queue slots 0–3) using `tx_index = len(txs)` | 🟡 Planned | | ||
| | `test_bal_noop_storage_write` | Ensure BAL includes address with storage read for no-op writes where pre-state equals post-state | Contract with pre-existing storage value `0x42` in slot `0x01`; transaction executes `SSTORE(0x01, 0x42)` | BAL **MUST** include the contract address with `storage_reads` for the slot since it was accessed, but **MUST NOT** include it in `storage_changes` | 🟡 Planned | | ||
| | `test_bal_aborted_transaction` | Ensure BAL captures aborted transactions correctly | Alice calls `AbortContract` that reads slot `0x01`, writes to slot `0x02`, sends 100 wei to Bob, calls `TargetContract`, deploys `NewContract` via `CREATE`, then aborts via `INVALID` or `REVERT` | BAL **MUST** include Alice, Bob, `AbortContract`, and `TargetContract` addresses in `account_changes` but **MUST NOT** include any `storage_reads`, `storage_changes`, `balance_changes`, `code_changes`, or `nonce_changes` except for Alice's `nonce_changes` and `balance_changes` for gas | 🟡 Planned | | ||
| | `test_bal_oog_sstore` | Ensure BAL handles OOG before `SSTORE` execution correctly | Alice calls `StorageContract` that executes `SSTORE` to cold slot `0x01` with insufficient gas for cold storage write | BAL **MUST NOT** contain slot `0x01` in the `storage_changes` of `StorageContract` | 🟡 Planned | |
There was a problem hiding this comment.
This "before" here is a very important keyword IMO.
There's an important distinction and I feel we need to add a test for each case:
- OOG before an SSTORE/SLOAD/BALANCE/etc opcode is reached: Should not affect the BAL since the storage was not accessed because we didn't even get to that part of the code.
- OOG exactly at SSTORE/SLOAD/BALANCE/etc, i.e. we had exactly the gas left required to execute the opcode minus 1: (Maybe underspecified?) Should (IMO) not affect the BAL since we didn't get to access the storage.
- OOG after SSTORE/SLOAD/BALANCE/etc, i.e. we had exactly the gas left required to execute the opcode and therefore it was correctly executed: Should affect the BAL because we needed to access the storage and we ran OOG afterwards.
On 1 and 3, the OOG can also be Op.INVALID for example, or Op.REVERT.
cc @fselmo these are really nice scenarios we should consider.
There was a problem hiding this comment.
As I understand, 1 = right before target opcode, 2 = during, 3 = right after.
1 and 3 (hopefully) should not have side effect on the target opcode since either we execute successfully and BAL is updated or not.
case 2 is interesting because clients rely on evm journaling mechanism under the hood for generating BAL. so if an SSTORE fails because of OOG, its possible a residual dirty journal entry can make its way into BAL and cause a consensus issue. The goal of the test case is to catch it. See this revm bug bluealloy/revm#2903 which inspired this test case.
Do you think 1 & 3 is worthwhile? If so we could parametrize this test for all three use case to cover one opcode before and after the target opcode (SSTORE/SLOAD/BALANCE/etc)
🗒️ Description
New OOG test cases proposed by @rakita have been added to the
test_cases.mdfor EIP-7928. The no-op test case, which was missed during the rebase of the framework changes, has been patched in.Related Reth PR: paradigmxyz/reth#17765 and bluealloy/revm#2903
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlinttype(scope):.