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: avoid creating extra journal entries #7493

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Mar 26, 2024

Motivation

Closes #7481

The actual reason behind the code in issue failing is that there is an underflow happening after one of vm.transact calls. However, we should have just printed traces ending with [Revert] panic: arithmetic underflow or overflow (0x11) here.

The panic happens because current impl of apply_state_changeset updates state entries by doing state.remove() + load_account, which results in state.journal having JournalEntry::AccountLoaded item for each account in changeset.

Then, when call to test contract reverts due to underflow, revm firstly goes through AccountLoaded entires and removes those accounts from state: https://github.com/bluealloy/revm/blob/main/crates/revm/src/journaled_state.rs#L318

Because of that, on the next, for example BalanceTransfer entry, it will assume that account is still in the state, but it was just removed earlier and a panic will happen: https://github.com/bluealloy/revm/blob/main/crates/revm/src/journaled_state.rs#L351

Solution

Reuse update_state fn used by --isolate which updates state without touching journal

@klkvr klkvr requested review from DaniPopes and mattsse as code owners March 26, 2024 00:20
@klkvr
Copy link
Member Author

klkvr commented Mar 26, 2024

Just realized that what vm.transact is exactly what we are doing with isolation mode, it might make sense to merge logic for those at some point

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice find, one pedantic nit re error handling

you're right this is basically --isolate, so it would be beneficial if we could unify all of this eventually

crates/evm/core/src/backend/mod.rs Outdated Show resolved Hide resolved
@klkvr klkvr requested a review from mattsse March 26, 2024 15:11
@mattsse
Copy link
Member

mattsse commented Mar 26, 2024

test failures unrelated, but we have some new unrelated clippy issues, mind taking care of them @klkvr ?

also I think it's time to retire the goerli fork tests

@mattsse mattsse merged commit 157a253 into master Mar 26, 2024
16 of 19 checks passed
@mattsse mattsse deleted the klkvr/apply-state-changeset branch March 26, 2024 15:26
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.

Foundry panics after executing 3 transactions using transact on a forked state
2 participants