feat(spec-spec, tests): Implement eip-8246 and testing scenario by LouisTsai-Csie · Pull Request #2842 · ethereum/execution-specs

LouisTsai-Csie · 2026-05-12T12:56:37Z

🗒️ Description

Implement EIP-8246: https://forkcast.org/eips/8246

🔗 Related Issues or PRs

issue #2821

✅ Checklist

All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
```
just static
```
All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
All: Considered updating the online docs in the ./docs/ directory.
All: Set appropriate labels for the changes (only maintainers can apply labels).
Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

Cute Animal Picture

codecov · 2026-05-12T13:06:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.01%. Comparing base (87c52b4) to head (b510ef9).

Additional details and impacted files

@@                   Coverage Diff                    @@
##           eips/amsterdam/eip-8246    #2842   +/-   ##
========================================================
  Coverage                    90.01%   90.01%           
========================================================
  Files                          539      539           
  Lines                        32618    32623    +5     
  Branches                      3030     3030           
========================================================
+ Hits                         29361    29366    +5     
  Misses                        2699     2699           
  Partials                       558      558

Flag	Coverage Δ
unittests	`90.01% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

marioevz

Looks good overall. I think we need to take care of failing tests in ./tests/cancun/eip6780_selfdestruct before eventually merging into forks/amsterdam (if this gets included in a devnet).

gurukamath · 2026-05-15T08:44:28Z

    for address in tx_output.accounts_to_delete:
-        destroy_account(tx_state, address)
+        destroy_storage(tx_state, address)
+        modify_state(tx_state, address, preserve_account_balance)


If modify_state ends up calling destroy_account (zero-balance case), destroy_storage runs twice. Not a huge issue but something to be kept in mind

I will keep it as is, but let me know if you have any suggestion!

Why not just call destroy_storage in convert_to_balance_only_account like destroy_account currently does? That way, the diff in fork.py looks cleaner as well. Otherwise, this looks good to me.

Great idea, i will add a new commit.

marioevz · 2026-05-18T22:38:04Z

Checking the list of failing tests for the current fill in Amsterdam, they all look like expected failures because of the behavior change.

I recommend we track these in the EIP tracking issue #2821, tackle them in separate PRs to merge to eips/amsterdam/eip-8246, and merge this PR as is after addressing the spec comments.

LouisTsai-Csie · 2026-05-20T14:25:43Z

@marioevz @gurukamath I've updated according to the comment. Now the PR contains spec & test for eip-8246 and refactor the failing legacy tests. Please take a look!

I will stop adding new tests here, and wait for the PR to merge first.

SamWilsn

A quick review of only the spec related changes.

SamWilsn · 2026-05-25T16:55:16Z

+    Invoked by ``modify_state`` (and the coinbase fee-credit path) to
+    clean up an account that has become empty (zero nonce, empty
+    code, and zero balance) so it does not appear in the post-state.


There's something funky about this. I've opened #2908.

SamWilsn · 2026-05-25T16:58:57Z

    set_account(tx_state, address, None)


+def convert_to_balance_only_account(account: Account) -> None:


This function name needs work. The word "convert" implies that something is changing about the kind of account, not about its contents. For example, you "convert a gas-powered car to electric", but you don't "convert a dirty car to clean" when you go through a car wash.

convert_to_balance_only_account implies that the account is changing "kinds" from a regular account to one that can only have a balance. As far as I am aware, there's nothing special about a balance only account. It's equivalent to an account that has received ether but has not sent any transactions.

Some ideas:

reset_account (pairs nicely with destroy_account, but also might be confusing)

make_account_deployable (this is more semantically meaningful I think, but a mouthful)

clear_account_nonce_and_code_hash (says what it does, but not what it intends; might make future refactors more difficult)

I applied Guru's feedback by moving destroy_storage into the convert_to_balance_only_account function, and your suggestion here. Now the refactored function becomes:

def convert_to_balance_only_account( tx_state: TransactionState, address: Address ) -> None: def clear_account(account: Account) -> None: account.nonce = Uint(0) account.code_hash = EMPTY_CODE_HASH destroy_storage(tx_state, address) modify_state(tx_state, address, clear_account)

Since it now clears storage, nonce, and code hash, the name clear_account_nonce_and_code_hash no longer fits, while reset_accounts is a bit confusing?

I would support make_account_deployable compared to other two suggestions, i also come up with:

nullify_account_code_storage_nonce

strip_to_balance_only

What do you think?

SamWilsn · 2026-05-25T17:04:34Z

    set_account(tx_state, address, None)


+def convert_to_balance_only_account(account: Account) -> None:


The pattern for other functions in this file is to create a function that operates on a frozen account, that defines an nested function to apply the transformation. Is there a reason this function breaks from that pattern?

If not, I'd suggest something like:

def convert_to_balance_only_account(tx_state: TransactionState, address: Address) -> None: def clear_account(account: Account) -> None: account.nonce = Uint(0) account.code_hash = EMPTY_CODE_HASH modify_state(tx_state, address, clear_account)

Carsons-Eels

I don't see anything wrong with the spec code that hasn't been commented on, will review the tests tomorrow. 🚀

Carsons-Eels · 2026-05-28T07:07:45Z

+        destroy_storage(tx_state, address)
+        modify_state(tx_state, address, convert_to_balance_only_account)


Might be worth a comment here noting that the explicit destroy_storage exists for EIP-7928 block-access-list completeness...

I dont fully understand, could you expand more on this?

I applied Guru and Sam's feedback on the refactor, would this break anything?

LouisTsai-Csie added 2 commits May 12, 2026 20:33

feat: implement eip-8624

e3c7ae5

tests: eip-8264 basic case

0ee0f02

marioevz requested a review from gurukamath May 13, 2026 21:52

marioevz reviewed May 13, 2026

View reviewed changes

Comment thread tests/amsterdam/eip8246_selfdestruct_no_burn/test_selfdestruct_no_burn.py Outdated

Comment thread tests/amsterdam/eip8246_selfdestruct_no_burn/test_selfdestruct_no_burn.py Outdated

Comment thread src/ethereum/forks/amsterdam/state_tracker.py Outdated

refactor: add more test parametrizatino

68d5c14

LouisTsai-Csie marked this pull request as ready for review May 14, 2026 16:08

LouisTsai-Csie requested review from Carsons-Eels and marioevz May 14, 2026 16:12

gurukamath reviewed May 15, 2026

View reviewed changes

danceratopz mentioned this pull request May 18, 2026

All Core Devs - Testing (ACDT) #80, May 18, 2026 ethereum/pm#2049

Closed

4 tasks

LouisTsai-Csie force-pushed the implement-eip-8246 branch from f42defc to 36db57d Compare May 19, 2026 08:43

LouisTsai-Csie marked this pull request as draft May 19, 2026 09:02

marioevz reviewed May 19, 2026

View reviewed changes

Comment thread tests/cancun/eip6780_selfdestruct/test_selfdestruct.py Outdated

fix: failing legacy consensus test

7bde788

LouisTsai-Csie force-pushed the implement-eip-8246 branch from 36db57d to 7bde788 Compare May 20, 2026 14:06

chore: apply suggested feedback

c128370

LouisTsai-Csie marked this pull request as ready for review May 20, 2026 14:24

LouisTsai-Csie requested review from gurukamath and marioevz May 20, 2026 14:25

SamWilsn requested changes May 25, 2026

View reviewed changes

Carsons-Eels reviewed May 28, 2026

View reviewed changes

refactor: strip to balance only function

b510ef9

		set_account(tx_state, address, None)


		def convert_to_balance_only_account(account: Account) -> None:

		destroy_storage(tx_state, address)
		modify_state(tx_state, address, convert_to_balance_only_account)

Conversation

LouisTsai-Csie commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🗒️ Description

🔗 Related Issues or PRs

✅ Checklist

Cute Animal Picture

Uh oh!

codecov Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marioevz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gurukamath May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marioevz commented May 18, 2026

Uh oh!

Uh oh!

LouisTsai-Csie commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamWilsn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carsons-Eels left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LouisTsai-Csie commented May 12, 2026 •

edited

Loading

codecov Bot commented May 12, 2026 •

edited

Loading

gurukamath May 20, 2026 •

edited

Loading

LouisTsai-Csie commented May 20, 2026 •

edited

Loading