zkevm: add SELFDESTRUCT coverage #1678
Merged
Conversation
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
jsign
added
scope:tests
jsign
commented
May 29, 2025
|
@jochem-brouwer, do you want to do a review of this draft PR (which should be ready) before I open up for other reviewers? |
|
On it 😄 👍 |
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 😄
|
@jochem-brouwer, thanks a lot for your fantastic review. I'm now opening for formal review! |
chfast
reviewed
Jun 2, 2025
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>
jsign
force-pushed
the
jsign-zkevm-selfdestruct
branch
from
b152eca to
364aad1
Compare
|
Rebased. |
jochem-brouwer
requested changes
Jun 4, 2025
jsign
commented
Jun 4, 2025
|
Done changes after the last review, ready for a final round @jochem-brouwer! |
|
Still not sure why it says it's waiting for changes when I re-asked for review to get the final check. 🤷 |
|
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
codeofcarson
pushed a commit
to codeofcarson/execution-spec-tests
that referenced
this pull request
Jul 1, 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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