feat(spec-specs, spec-tests): add EIP-7708 ETH transfers emit a log#2844
Conversation
fix(spec-specs): correct CR issues, fix formatting fix(spec-specs): inline `execute_code()` to `process_message()` chore(spec-specs): backport changes fix(spec-specs): trim out whitespace in topic hash to match tests feat(spec-specs): add selfdestruct event topic and logging function feat(spec-specs): selfdestruct to self emits selfdestruct event feat(spec-specs): define call success constant feat(spec-specs): emit selfdestruct finalization log for remaining balance
test(test-tests): add selfdestruct topic and use empty account test(test-tests): add nested calls log ordering test feat(test-tests): add selfdestruct finalization test fix(test-tests): use spaces in event signature to match spec
…e charges (#2059) * fix(spec-specs): Move account closure log emission before priority fee charges * fix(spec-specs): formatting and spelling tweaks * fix(spec-specs): remove duplicate WriteInStaticContext check * refactor(spec-specs): align memory expansion with other opcodes * fix(testing/test): Fix unit test expectation * refactor(spec-specs): move post-mining coinbase balance calculation --------- Co-authored-by: Mario Vega <marioevz@gmail.com>
* feat(test-specs): add/refactor tests, add mainnet marked, checklist, coverage check * feat(test-specs): add fork transition test for selfdestruct logs * fix: tests * chore(test-specs): fix fork transition tests * test(test-specs): add code deposit oog test case --------- Co-authored-by: carsons-eels <carson@ethereum.org> Co-authored-by: Mario Vega <marioevz@gmail.com>
* fix(spec,text): Updates to EIP-7708 spec for bal-devnet-2 - fix(spec,test): EIP-7708 emit selfdestruct logs only in same tx - fix(spec,test): EIP-7708, only emit on transfers to other accounts - Add more tests that match these edge cases * feat(test): tests for finalization selfdest + bal transfer in lexicographic order * fix: changes from comments on PR #2086 * add more variants to test_selfdestruct_same_tx_via_call * fix: docstring * refactor: improvements from comments on PR #2086 * Update test to use dynamic addresses --------- Co-authored-by: Mario Vega <marioevz@gmail.com>
- Add CREATE2 support via with_all_create_opcodes marker - Add tests for SELFDESTRUCT to coinbase - revealed a change needed since the last update was made to the EIP that should be included in the currect refspec (miner fees paid before finalization LOG2).
Co-authored-by: raxhvl <raxhvl@users.noreply.github.com>
* ♻️ refactor: Rename selfdestruct log to burn * 🥢 nit: * chore(tests): fix stale selfdestruct references and rename to burn --------- Co-authored-by: raxhvl <raxhvl@users.noreply.github.com> Co-authored-by: spencer-tb <spencer.tb@ethereum.org>
… no-log (#2717) * feat(tests): EIP-7708 - multi-account finalization burn log ordering Adds a dedicated test that proves finalization burn logs are emitted in lexicographical address order when multiple accounts are marked for deletion in the same transaction. Parametrized over N in {2, 5}. N accounts are created and SELFDESTRUCT'd in the same tx, then funded via payer contracts called in REVERSE sorted address order with distinct nonzero amounts. Each destroyed account ends with a unique nonzero balance at finalization, so ordering by address vs. by call order is always distinguishable. Addresses issue #2691. * feat(tests): EIP-7708 - coinbase priority fee must not emit transfer log Adds a dedicated test proving the coinbase priority fee payment does not produce a Transfer log. A contract CALLs the coinbase address with nonzero value while the tx pays a nonzero priority fee to that same coinbase. Only the CALL-with-value must produce a Transfer log; the priority fee credit happens outside the EVM as a protocol-level balance change. An implementation that hooks every balance addition (instead of only CALL / SELFDESTRUCT / tx-level value transfers) would emit an extra Transfer log for the fee and fail the exact-log assertion. Addresses issue #2692. * feat(tests): add single account multi transfer test * fix(tests): minor nit --------- Co-authored-by: marioevz <marioevz@gmail.com>
Align EIP-7708 selfdestruct finalization test with the gas constant rename on forks/amsterdam.
* feat(tests): Add EIP-7708 checks to 6780 tests * fix: add another call and properly accumulate so that burn logs are validated * fix(tests): Review fixes --------- Co-authored-by: carsons-eels <carson@ethereum.org>
* feat(tests): cover EIP-7708 CREATE log rollback on outer revert Mirror `test_inner_call_succeeds_outer_reverts_no_log` for the bal-devnet-5 update that brings CREATE/CREATE2 under EIP-7708. A factory CREATEs a child with value (the deployment succeeds and a `factory -> created` log is emitted in the child frame) and then REVERTs; the receipt must record no logs because the outer revert discards the factory's frame along with every log it produced. * feat(tests): Review comments --------- Co-authored-by: marioevz <marioevz@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2844 +/- ##
===================================================
- Coverage 88.17% 86.94% -1.23%
===================================================
Files 577 586 +9
Lines 35659 35764 +105
Branches 3490 3362 -128
===================================================
- Hits 31442 31095 -347
- Misses 3654 4010 +356
- Partials 563 659 +96
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:
|
marioevz
left a comment
There was a problem hiding this comment.
Partial review of the specs only, I'll continue with the tests review in a bit.
| # EIP-7708: Emit appropriate log based on whether ETH is burned | ||
| # or transferred to a different account |
There was a problem hiding this comment.
It doesn't seem like we mention EIP numbers in comments anywhere else in the spec, and the comment itself seems too verbose when compared to the rest of the code in this file.
Perhaps something simpler like:
| # EIP-7708: Emit appropriate log based on whether ETH is burned | |
| # or transferred to a different account | |
| # Emit logs |
| # EIP-7708: Emit appropriate log based on whether ETH is burned | ||
| # or transferred to a different account | ||
| if originator in tx_state.created_accounts and beneficiary == originator: | ||
| # Self-destruct to self in same tx burns the balance |
There was a problem hiding this comment.
Similar comment to first one, it might be better to just delete the comment since the code itself is readable enough for the reader to infer why is this operation happening.
| # Self-destruct to self in same tx burns the balance | ||
| emit_burn_log(evm, originator, originator_balance) | ||
| elif beneficiary != originator: | ||
| # Transfer to different beneficiary |
There was a problem hiding this comment.
Same idea:
| # Transfer to different beneficiary |
| message.current_target, | ||
| message.value, | ||
| ) | ||
| # EIP-7708: Only emit transfer log to a different account |
There was a problem hiding this comment.
Same idea, the code is very self-explanatory.
| # EIP-7708: Only emit transfer log to a different account |
marioevz
left a comment
There was a problem hiding this comment.
Partial review of the tests, I'll continue later today!
| ], | ||
| ) | ||
| @pytest.mark.with_all_create_opcodes | ||
| def test_selfdestruct_to_self_same_tx( |
There was a problem hiding this comment.
New Test (follow-up PR): Self-destruct to self but the contract is being created via a contract-creating-tx (to=None). Invalidated by EIP-8246.
| + Op.TSTORE( | ||
| 0, Op.CREATE(value=create_value, offset=0, size=initcode_len) | ||
| ) | ||
| + Op.CALL(gas=100_000, address=Op.TLOAD(0), value=first_call_value) |
There was a problem hiding this comment.
Returning values of the Op.CREATE and Op.CALL could be SSTORE'd and verified to confirm that the contract is working properly.
| runtime = ( | ||
| Op.TLOAD(0) | ||
| + Op.ISZERO | ||
| + Op.PUSH1(8) | ||
| + Op.JUMPI | ||
| + Op.STOP | ||
| + Op.JUMPDEST | ||
| + Op.TSTORE(0, 1) | ||
| + Op.SELFDESTRUCT(target) | ||
| ) |
There was a problem hiding this comment.
The Conditional helper class makes this more readable.
| runtime = ( | |
| Op.TLOAD(0) | |
| + Op.ISZERO | |
| + Op.PUSH1(8) | |
| + Op.JUMPI | |
| + Op.STOP | |
| + Op.JUMPDEST | |
| + Op.TSTORE(0, 1) | |
| + Op.SELFDESTRUCT(target) | |
| ) | |
| runtime = Conditional( | |
| condition=Op.ISZERO(Op.TLOAD(0)), | |
| if_true=Op.TSTORE(0, 1) + Op.SELFDESTRUCT(target), | |
| if_false=Op.STOP, | |
| ) |
| + Op.TSTORE( | ||
| 0, Op.CREATE(value=create_balance, offset=0, size=initcode_len) | ||
| ) | ||
| + Op.CALL(gas=Op.GAS, address=Op.TLOAD(0), value=0) |
There was a problem hiding this comment.
Same as comment above, these values can be SSTORE'd.
|
I'll work to resolve these today and merge tonight |
danceratopz
left a comment
There was a problem hiding this comment.
Thanks for getting on this so quickly!
This was just a fly-by and excuse me being pedantic but could you please add a link to the spec this PR targets to the description. Thanks!
I think it should be your latest commit that you made with the interop breakout discussion updates?
ethereum/EIPs@100615c
This was an AI fly-by, if it's still here tomorrow morning I'll take a more human look 🙂
marioevz
left a comment
There was a problem hiding this comment.
Last few comments on the test. Thanks!
| # finalization burn log | ||
| if fork.is_eip_enabled(8037): | ||
| # TODO: Fix calculation of the exact expected gas usage | ||
| finalization_balance = None |
There was a problem hiding this comment.
| finalization_balance = None | |
| raise Exception("Test needs update") |
Otherwise we might miss fixing this test when 8037 is merged.
|
|
||
|
|
||
| @pytest.mark.with_all_call_opcodes | ||
| def test_call_opcodes_transfer_log_behavior( |
There was a problem hiding this comment.
Test: call opcodes attempting to transfer more value than what the caller has, no log expected.
| # Build chain: contracts[0] -> contracts[1] -> ... -> final_recipient | ||
| final_recipient = pre.nonexistent_account() | ||
| contracts: list[Address] = [] | ||
| expected_logs: list[TransactionLog] = [] | ||
|
|
||
| # Build contracts in reverse order (deepest first) | ||
| next_target = final_recipient | ||
| for _ in range(call_depth): | ||
| contract_code = Op.CALL( | ||
| gas=500_000, address=next_target, value=transfer_value | ||
| ) | ||
| # Each contract needs enough balance for its transfer | ||
| contract = pre.deploy_contract(contract_code, balance=transfer_value) | ||
| contracts.insert(0, contract) | ||
| next_target = contract | ||
|
|
||
| # First contract is the tx target | ||
| entry_contract = contracts[0] | ||
|
|
||
| # Build expected logs in chronological order | ||
| # First: tx-level transfer (sender -> entry_contract) | ||
| expected_logs.append(transfer_log(sender, entry_contract, tx_value)) | ||
|
|
||
| # Then: each CALL in order | ||
| for i in range(call_depth): | ||
| from_addr = contracts[i] | ||
| to_addr = contracts[i + 1] if i + 1 < call_depth else final_recipient | ||
| expected_logs.append(transfer_log(from_addr, to_addr, transfer_value)) |
There was a problem hiding this comment.
| # Build chain: contracts[0] -> contracts[1] -> ... -> final_recipient | |
| final_recipient = pre.nonexistent_account() | |
| contracts: list[Address] = [] | |
| expected_logs: list[TransactionLog] = [] | |
| # Build contracts in reverse order (deepest first) | |
| next_target = final_recipient | |
| for _ in range(call_depth): | |
| contract_code = Op.CALL( | |
| gas=500_000, address=next_target, value=transfer_value | |
| ) | |
| # Each contract needs enough balance for its transfer | |
| contract = pre.deploy_contract(contract_code, balance=transfer_value) | |
| contracts.insert(0, contract) | |
| next_target = contract | |
| # First contract is the tx target | |
| entry_contract = contracts[0] | |
| # Build expected logs in chronological order | |
| # First: tx-level transfer (sender -> entry_contract) | |
| expected_logs.append(transfer_log(sender, entry_contract, tx_value)) | |
| # Then: each CALL in order | |
| for i in range(call_depth): | |
| from_addr = contracts[i] | |
| to_addr = contracts[i + 1] if i + 1 < call_depth else final_recipient | |
| expected_logs.append(transfer_log(from_addr, to_addr, transfer_value)) | |
| accounts: list[Address] = [pre.nonexistent_account()] | |
| for _ in range(call_depth): | |
| contract_code = Op.CALL( | |
| gas=500_000, address=accounts[0], value=transfer_value | |
| ) | |
| accounts.insert( | |
| 0, pre.deploy_contract(contract_code, balance=transfer_value) | |
| ) | |
| entry_contract = accounts[0] | |
| final_recipient = accounts[-1] | |
| expected_logs: list[TransactionLog] = [ | |
| transfer_log(sender, entry_contract, tx_value) | |
| ] | |
| for i in range(call_depth): | |
| expected_logs.append( | |
| transfer_log(accounts[i], accounts[i + 1], transfer_value) | |
| ) |
Make logic a bit more readable.
| pytest.param(Op.MSTORE(2**256 - 1, 0), id="out_of_gas"), | ||
| ], | ||
| ) | ||
| def test_reverted_transaction_no_log( |
There was a problem hiding this comment.
Test: Contract-creating tx address collision: attempt to deploy to an address that already has code.
|
Also please add to the CHANGELOG like in this other PR: https://github.com/ethereum/execution-specs/pull/2840/changes#diff-2bfab3db5cc1baf4c6d3ff6b19901926e3bdf4411ec685dac973e5fcff1c723b |
ccef2bc to
8e7231c
Compare
|
Okay, that should take care of all of them except the question about the disabled BPO transitions test |
Wait, didn't this get removed by #2810 ? |
bd4494a to
8e7231c
Compare
The CHANGELOG got removed recently in #2810 Justification TLDR - it was only partially maintained and often misleading; happy to add back more dedicated changelogs as proposed by #2036. cc @marioevz |
🗒️ Description
Merge changes necessary for implementing (pre 8246) EIP-7708 into the forks/amsterdam branch.
This PR implements the spec from ethereum/EIPs@100615c.
🔗 Related Issues or PRs
forks/amsterdam#2846✅ Checklist
just statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.Cute Animal Picture
CC0 Public Domain