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

bug: state appears to be shared between tests when linked libraries are used #8639

Closed
2 tasks
mds1 opened this issue Aug 9, 2024 · 5 comments · Fixed by #9527
Closed
2 tasks

bug: state appears to be shared between tests when linked libraries are used #8639

mds1 opened this issue Aug 9, 2024 · 5 comments · Fixed by #9527
Assignees
Labels
A-testing Area: testing C-forge Command: forge Cmd-forge-test Command: forge test P-normal Priority: normal T-bug Type: bug
Milestone

Comments

@mds1
Copy link
Collaborator

mds1 commented Aug 9, 2024

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (1197fbe 2024-08-09T00:25:08.996768000Z)

What command(s) is the bug in?

forge test

Operating System

None

Describe the bug

  1. Clone the optimism monorepo and check out this branch: [DO NOT MERGE]: testing CI flakes ethereum-optimism/optimism#11423
  2. Run just test, and you'll notice testDebug fails. That is hardcoded data from a failed CI run
  3. Now, only run the failed test: forge test --mt testDebug -vvv. It will pass. If you run the full file containing that test it will also still pass.
  4. Compare the passing and failing traces (see below screenshots). Notice how in the passing one, the call is to an unlabeled address, and in the failing one it's to an address labeled ForgeArtifacts. Based on the hardcoded calldata in testDebug, we can see that address is 0x5F65cD7D792E9746EF82929D60de9a1C526f93A5.

So what appears to be happening here is that when the full suite is run, contracts deployed from another test (1) leak into the fuzzer dictionary of this test, and (2) the state leaks into this test as well.

image image
@mattsse
Copy link
Member

mattsse commented Aug 9, 2024

@grandizzy I wonder if the dictionary is unique per test contract and not TestContract::test ?

@mds1
Copy link
Collaborator Author

mds1 commented Aug 9, 2024

@mattsse I don't think so because forge test --mp test/libraries/SafeCall.t.sol also passes

It seems likely the issue is actually related to library deployments and linking: ethereum-optimism/optimism#11426 (comment). Once I remove all public lib methods in ethereum-optimism/optimism#11426, the test passes

@grandizzy
Copy link
Collaborator

grandizzy commented Aug 9, 2024

@mattsse I actually think there's a bug introduced with #8497
here, as it seems the executor for fuzz tests is not cloned anymore?
4a41367#diff-98c09a5d1cb595f37109d7b1c0109ee32dd74dc1d253cdda9a63e4ce32bf49d5R628-R671

Le: actually Cow::Borrowed().into_owned() is cloning it, and rechecked test is failing even with that feature reverted, so probably not this

@mds1
Copy link
Collaborator Author

mds1 commented Aug 9, 2024

@mattsse I updated my above comment, but have confirmed the cause of the bug was library linking: once we remove all public methods from libs the issue goes away

@mds1 mds1 changed the title bug: state appears to be shared between tests bug: state appears to be shared between tests when linked libraries are used Aug 9, 2024
@klkvr
Copy link
Member

klkvr commented Aug 10, 2024

Since #8034 when running test suites requiring library linking, forge predeploys all libraries which are required by at least one test contract and reuses resulted state. This likely causes some of those libraries being used as fuzz dictionary values, which could've resulted in this failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Area: testing C-forge Command: forge Cmd-forge-test Command: forge test P-normal Priority: normal T-bug Type: bug
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

5 participants