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

feat(evm): use completely separated storage sections in multifork #2301

Merged

Conversation

mattsse
Copy link
Member

@mattsse mattsse commented Jul 13, 2022

Motivation

with #1715 we added multifork support, with some assumptions/limitations:

  • only one local memory area, which meant that write operations were performed in the same memory area regardless of forking

this has turned out to be an undesired and potential pitfall.

This rewrites how storage is tracked:

  • each fork is completely separate, write operations are performed only in the fork's own separate memory
  • the storage of the account of the sender and the contract remains persistent across swaps: e.g: changes to the test contract's own variables are persistent when selecting another fork

Solution

  • store multiple forkId -> ForkDB pairs
  • also keep track and update Subroutine when selecting forks
  • drastically simplify the FuzzWrapper which is now a Cow<Backend>

@mattsse mattsse added the A-cheatcodes Area: cheatcodes label Jul 13, 2022
@mattsse mattsse changed the title feat(evm): use completely separate storage sections in multifork feat(evm): use completely separated storage sections in multifork Jul 13, 2022
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

clean, anything else we should test beyond storage values?

cc @hexonaut, @aureliusbtc & @mds1 for any user testing we can get before merging in case we're introducing any edge cases here.

evm/src/executor/backend/fuzz.rs Outdated Show resolved Hide resolved
@onbjerg onbjerg added the T-feature Type: feature label Jul 14, 2022
mattsse and others added 2 commits July 14, 2022 11:58
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
@mattsse mattsse force-pushed the matt/use-separate-memory-sections-for-forks branch from 5392d84 to 55a4d6d Compare July 14, 2022 10:09
@hexonaut
Copy link
Contributor

hexonaut commented Jul 17, 2022

I'm getting a non-descript "revert" error in this test at this line with this PR (previously working): https://github.com/makerdao/xdomain/blob/add-optimism/packages/dss-bridge/src/tests/domains/optimism/OptimismIntegration.t.sol#L292

@mattsse mattsse mentioned this pull request Jul 26, 2022
22 tasks
@mattsse
Copy link
Member Author

mattsse commented Jul 26, 2022

@gakonst I think this is good to merge, but maybe we should wait until I updated all the docs, also need another round of revm debugging to figure out how to enable diagnostics for calls on non-existent errors.

But from a functionality point of view it is complete and can be merged

@gakonst
Copy link
Member

gakonst commented Jul 26, 2022

Ack - reviewing today. Want to give some thinking on whether we can do this without the persist cheats

@mattsse
Copy link
Member Author

mattsse commented Jul 27, 2022

@gakonst figured out the problem with diagnostics,

now we return meaningful revert reasons if someone calls a contract that only exists on another fork:

assert_eq!(
result.reason.unwrap(),
"Contract 0xCe71065D4017F316EC606Fe4422e11eB2c47c246 does not exists on active fork with id `1`\n But exists on non active forks: `[0]`"
);

testdata/cheats/Fork2.t.sol Outdated Show resolved Hide resolved
@mattsse
Copy link
Member Author

mattsse commented Jul 28, 2022

I'm going to revise all multifork docs next. Should we hold off on merging until tonight's night is built so we can ship it with tomorrow's nightly?

@gakonst
Copy link
Member

gakonst commented Jul 28, 2022

Yep - let's give this one final pass and merge/ship tomorrow. I'd try to clarify to the reader how to think about all the forkign cheatcodes, which ones are the "most common" ones and which ones require deeper understanding.

@joshieDo joshieDo mentioned this pull request Jul 29, 2022
@gakonst gakonst merged commit af3c9d3 into foundry-rs:master Jul 30, 2022
@gakonst
Copy link
Member

gakonst commented Jul 30, 2022

@hexonaut @mds1 merged, see how to use here. This should allow us to have contracts that persist across forks beyond the default test contract and msg.sender.

iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
…undry-rs#2301)

* refactor: completely separate fork states

* refactor: turn fuzz wrapper into cow

* refactor: add subroutine to trait

* feat: track subroutine

* copy sender and receiver

* test: extend fork test

* fix: initialize accounts on setup

* test: add create select test

* Update evm/src/executor/backend/fuzz.rs

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* update docs

* fix: clone cheat code address and add traces

* test: add another test

* introduce persistent accounts

* feat: add persistent cheatcodes

* add persistent tests

* test: add persistent test

* feat: add revert error multifork diagnostic

* feat: better diagnostic

* docs

* feat: fork revert diagnostic

* test: remove uncommented left over

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants