zkevm: add SELFDESTRUCT coverage#1678
Merged
Merged
Conversation
jsign
commented
May 29, 2025
Collaborator
Author
|
@jochem-brouwer, do you want to do a review of this draft PR (which should be ready) before I open up for other reviewers? |
Member
|
On it 😄 👍 |
jochem-brouwer
left a comment
Member
There was a problem hiding this comment.
This was a fun one 😄 👍
I am sure I missed some things though. I would also suggest to run these tests inside some EVM with traces on, because upon reading these tests I think these might not behave as we might expect 😄 👍
I think when looking over this again, I might find some other optimizations for the gas here 😄
Collaborator
Author
|
@jochem-brouwer, thanks a lot for your fantastic review. I'm now opening for formal review! |
chfast
reviewed
Jun 2, 2025
Collaborator
Author
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
b152eca to
364aad1
Compare
Collaborator
Author
|
Rebased. |
jochem-brouwer
requested changes
Jun 4, 2025
jochem-brouwer
left a comment
Member
There was a problem hiding this comment.
Some style comments and I believe one of the tests is doing unnecessary extra behavior 😄 👍
jsign
commented
Jun 4, 2025
Collaborator
Author
|
Done changes after the last review, ready for a final round @jochem-brouwer! |
Collaborator
Author
|
Still not sure why it says it's waiting for changes when I re-asked for review to get the final check. 🤷 |
Member
|
Sorry, I forgot to approve, which I wanted to do after my last comment. Will do now! |
jochem-brouwer
approved these changes
Jun 6, 2025
jochem-brouwer
left a comment
Member
There was a problem hiding this comment.
LGTM, great work, beautiful tests!
kclowes
pushed a commit
to kclowes/execution-spec-tests
that referenced
this pull request
Oct 20, 2025
* zkevm: add selfdestruct existing contracts bench Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * zkevm: add selfdestruct of contracts deployed in tx Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * improvements Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * Update tests/zkevm/test_worst_stateful_opcodes.py Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com> * improvements Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * adjust gas prices Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * Update tests/zkevm/test_worst_stateful_opcodes.py Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com> * improvements Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * improvement Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * fix bug Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * selfdestruct in initcode Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * feedback Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds coverage for
SELFDESTRUCTcovering two main scenarios:since both cases are relevant for the current way
SELFDESTRUCTworks.Each case covers both value-bearing and non-value-bearing destructions.
Cycles:
Note that
*createdscenarios are very costly to execute since they involve deploying the transactions we want to SELFDESTRUCT. This means that each attempt requires a lot of gas, and most of it isn't coming from SELFDESTRUCT. While this was known before doing it, I decided, for completeness, to cover the scenario anyway.Targets #1657