Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix revert account destroyed with nonexistent target #1282

Conversation

LindaGuiga
Copy link
Contributor

We noticed that the create2collisionSelfdestructedRevert tests from the evm-tests test suite were failing with a Kernel panic. This is because in revert_account_destroyed, we access the target's balance to undo the balance change carried out in SELF_DESTRUCT. But in the tests, the target account had to be created when carrying out a SELF_DESTRUCT, and the revert therefore already deleted it. In that case, mpt_read_state_trie returns 0 (corresponding to a nonexistent account), and we should not try to change the target's balance.

This PR therefore ensures that when a target account is nonexistent, we do not try to change its balance.

@Nashtare Nashtare requested a review from wborgeaud October 9, 2023 22:09
Copy link
Contributor

@wborgeaud wborgeaud left a comment

Choose a reason for hiding this comment

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

Good catch!
Your fix should work, but I think the underlying issue here is that in sys_selfdestruct we do %journal_add_account_destroyed before doing %journal_add_account_created (in %add_eth).
It should be the other way around and looking at the revm implementation, that's what they do too.
I've pushed this change here https://github.com/0xPolygonZero/plonky2/tree/wb/testing_selfdestruct and it makes the create2collisionSelfdestructedRevert tests pass, although I haven't run the full suite with it yet.
What do you think?

@LindaGuiga
Copy link
Contributor Author

@wborgeaud Thank you! Your solution does seem better! I ran it on 6 tests that were failing due to this issue, and they all pass with your solution as well (all the create2collisionSelfdestructedRevert and create2collisionSelfdestructedOOG). I also haven't run the full suite with it, but it looks good to me

@wborgeaud
Copy link
Contributor

@LindaGuiga Great! Just opened #1287 with the fix.

@LindaGuiga LindaGuiga closed this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants