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

create transient storage snapshots for process_create_message #918

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

gurukamath
Copy link
Collaborator

(closes #917 )

What was wrong?

process_create_message did not create the right snapshots for rollback in case something went wrong.

Related to Issue #917

How was it fixed?

Add transient_storage to Environment and create appropriate snapshots.

Cute Animal Picture

Cute Animals - 1 of 1

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.72%. Comparing base (bf47143) to head (d663e77).
Report is 65 commits behind head on forks/cancun.

Additional details and impacted files
@@               Coverage Diff                @@
##           forks/cancun     #918      +/-   ##
================================================
+ Coverage         69.96%   74.72%   +4.76%     
================================================
  Files               610      611       +1     
  Lines             34295    34897     +602     
================================================
+ Hits              23993    26076    +2083     
+ Misses            10302     8821    -1481     
Flag Coverage Δ
unittests 74.72% <100.00%> (+4.76%) ⬆️

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.

@gurukamath gurukamath marked this pull request as ready for review April 3, 2024 09:42
Copy link
Collaborator

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'm not super familiar with this part of the code.

@holiman
Copy link
Contributor

holiman commented Apr 4, 2024

@SamWilsn once you rebase the statetest branch to include this, I can fuzz it again and see if it holds up

@petertdavies
Copy link
Collaborator

LGTM!

@gurukamath gurukamath merged commit 9c24cd7 into forks/cancun Apr 4, 2024
5 checks passed
@SamWilsn SamWilsn deleted the issue-917 branch April 4, 2024 15:39
@holiman
Copy link
Contributor

holiman commented Apr 8, 2024

@gurukamath could you explain a bit about the circumstances here? The original testcase was pretty messy, building a very deeply nested calltree.
I'm trying to make a canonical testcase for this (holiman/goevmlab#140) , but would appreciate some guidance.

@gurukamath
Copy link
Collaborator Author

@gurukamath could you explain a bit about the circumstances here? The original testcase was pretty messy, building a very deeply nested calltree.
I'm trying to make a canonical testcase for this (holiman/goevmlab#140) , but would appreciate some guidance.

I believe the case is of the following

  1. A does a tstore a value and and tries to create B using CREATE/CREATE2
  2. The creation of B fails because len(contract_code) > MAX_CODE_SIZE
  3. A then tries to tload the same value as in step 1.
  4. sstore then stores this value from tload

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.

5 participants