Skip to content

Conversation

@fselmo
Copy link
Contributor

@fselmo fselmo commented Jan 27, 2026

🗒️ Description

SELFDESTRUCT

CALL and Transactions

Also added some more test cases for coverage emitting transfer logs when not in the same tx, self destruct logs when in same tx, combo of transfer and selfdestruct logs across fork transition, update tests to reflect above changes

closes #2088

✅ Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx tox -e 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

Screenshot 2026-01-28 at 11 06 45

@codecov
Copy link

codecov bot commented Jan 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.14%. Comparing base (bf16746) to head (41d9159).
⚠️ Report is 23 commits behind head on eips/amsterdam/eip-7708.

Additional details and impacted files
@@                     Coverage Diff                     @@
##           eips/amsterdam/eip-7708    #2086      +/-   ##
===========================================================
- Coverage                    86.14%   84.14%   -2.01%     
===========================================================
  Files                          599      642      +43     
  Lines                        39491    42267    +2776     
  Branches                      3782     4066     +284     
===========================================================
+ Hits                         34021    35566    +1545     
- Misses                        4848     5971    +1123     
- Partials                       622      730     +108     
Flag Coverage Δ
unittests 84.14% <100.00%> (-2.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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fselmo fselmo force-pushed the fix/selfdestruct-log-only-in-same-tx branch 3 times, most recently from 8fa8d42 to a76f905 Compare January 28, 2026 00:21
@fselmo fselmo linked an issue Jan 28, 2026 that may be closed by this pull request
@fselmo fselmo changed the title fix(spec,test): EIP-7708 emit selfdestruct logs only in same tx fix(spec,test): EIP-7708 spec updates for self as beneficiary Jan 28, 2026
@fselmo fselmo changed the title fix(spec,test): EIP-7708 spec updates for self as beneficiary fix(spec,test): EIP-7708 spec updates for self as target Jan 28, 2026
@fselmo fselmo force-pushed the fix/selfdestruct-log-only-in-same-tx branch from a76f905 to 1213282 Compare January 28, 2026 17:04
@fselmo fselmo added A-spec-specs Area: Specification—The Ethereum specification itself (eg. `src/ethereum/*`) C-feat Category: an improvement or new feature C-test Category: test labels Jan 28, 2026
@fselmo fselmo changed the title fix(spec,test): EIP-7708 spec updates for self as target feat(spec,test): EIP-7708 spec updates for self as target Jan 28, 2026
@fselmo fselmo force-pushed the fix/selfdestruct-log-only-in-same-tx branch from 1213282 to 70d5a42 Compare January 28, 2026 18:11
@fselmo fselmo marked this pull request as ready for review January 28, 2026 18:11
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked tests, they look good but left a few nit comments and commented on one test where I think it is not tested what is intended.

Here are some extra cases to add:

  • Ensure CALLCODE does not emit any logs
  • Ensure SELFDESTRUCT to COINBASE emits the correct logs
    This specific case will: 1. Invoke selfdestruct (address is coinbase), this could burn eth if coinbase has funds already (the contract created is thus sending to Op.ADDRESS, but the block.coinbase is set such that this matches the created address). 2. Pay the priority fees to coinbase and 3. Destroy coinbase.
  • Ensure that in all 3 create situations (tx create, CREATE, CREATE2) the logs are emitted correctly
  • Invoking selfdestruct in the same contract multiple times (parametrize sending to self (burn), other addresses, and then burn again). This should either burn or emit transfer logs.
  • Send via selfdestruct funds to a contract already marked for selfdestruct.
  • Ensure that a transaction which 7702-delegates an account, which calls that account, which itself does SELFDESTRUCT, does not do the burn logs (as it does not burn since contract is not created)
  • Ensure that if any action done in current tx, which reverts, that the logs are also deleted

Let me know if I can assist 😄 👍

@fselmo
Copy link
Contributor Author

fselmo commented Jan 29, 2026

Here are some extra cases to add:

This is quite helpful, thank you 👍🏼. Imo, we should track these cases in the tracker for this EIP here. Or feel free to push here if you have permissions or PR to this branch. My goal here is to update the spec and provide enough tests to get a new release out and get clients to agree. Anything beyond this I think we can make subsequent PRs with more test cases. Wdyt?

Let me know if I can assist 😄 👍

The more the merrier 🙂! Thank you for the review! I will look into updating this tomorrow. Much appreciated 🙏🏼

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, thanks!

@marioevz marioevz force-pushed the eips/amsterdam/eip-7708 branch from f3d0672 to 6b9199e Compare January 29, 2026 20:31
fselmo added a commit to fselmo/execution-specs that referenced this pull request Jan 29, 2026
- 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
@fselmo fselmo force-pushed the fix/selfdestruct-log-only-in-same-tx branch from db5b412 to c069c11 Compare January 29, 2026 20:48
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these tests! And many thanks to @jochem-brouwer for your review!

I think we can go ahead and merge as-is, and add any remaining tests to the tracker #1875, in order to (a) be able to release soon, and (b) keep this PR from growing out of proportion.

@fselmo
Copy link
Contributor Author

fselmo commented Jan 29, 2026

add any remaining tests to the tracker

done #1875 (comment) 👍🏼

@fselmo fselmo merged commit fb17a68 into ethereum:eips/amsterdam/eip-7708 Jan 29, 2026
14 of 17 checks passed
@fselmo fselmo deleted the fix/selfdestruct-log-only-in-same-tx branch January 29, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-spec-specs Area: Specification—The Ethereum specification itself (eg. `src/ethereum/*`) C-feat Category: an improvement or new feature C-test Category: test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Small update to 7708 EIP requires minor spec update

3 participants