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: handle duplicate forkids #5939

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

mattsse
Copy link
Member

@mattsse mattsse commented Sep 28, 2023

Closes #5935

we have an edge case with forkids internally that can lead to an out of bounds bug if the same URL is used twice and the local mapping is updated

https://github.com/foundry-rs/foundry/blob/master/crates/evm/src/executor/backend/mod.rs#L1624-L1634

@frank-lim-partior
Copy link

The expected behavior is that forkId1 != forkId2, is that correct? I had hoped to use this to test contracts across 2 different forks from the same state.

@mattsse
Copy link
Member Author

mattsse commented Sep 29, 2023

this is rather a bug how me map the assigned uint256 forkids internally, because both forking off the same endpoint at the same height, so we need to change how we keep track of those, will fix shortly

@DaniPopes
Copy link
Member

@mattsse ping

@mattsse mattsse marked this pull request as ready for review November 28, 2023 10:50
@mattsse
Copy link
Member Author

mattsse commented Nov 28, 2023

@DaniPopes whoops, totally forgot about this.

I took a page out of the cargo playbook and used an URL hack to make them unique, this was easier than rewriting the internal mappings...
This is not ideal but at least fixes the issue, the fact that we only got one report so far indicates that this is not a common pattern so the fix should be fine

@@ -198,3 +198,6 @@ test_repro!(6180);

// https://github.com/foundry-rs/foundry/issues/6437
test_repro!(6437);

// <https://github.com/foundry-rs/foundry/issues/5935>
test_repro!(5935);
Copy link
Member

Choose a reason for hiding this comment

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

order :)

@mattsse mattsse changed the title wip:fix: handle duplicate forkids fix: handle duplicate forkids Nov 28, 2023
@mattsse mattsse requested a review from DaniPopes November 28, 2023 12:27
@mattsse mattsse merged commit db39460 into foundry-rs:master Nov 28, 2023
19 checks passed
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.

Using vm.selectFork() to change fork causes an application panic
3 participants