foundry icon indicating copy to clipboard operation
foundry copied to clipboard

Setting for `include_storage` in invariant tests seems to leak into fuzz tests settings

Open apbendi opened this issue 11 months ago • 3 comments

Component

Forge

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

  • [X] Foundry
  • [ ] Foundryup

What version of Foundry are you on?

forge 0.2.0 (1a4960d 2024-03-20T00:19:54.542150000Z)

What command(s) is the bug in?

No response

Operating System

macOS (Apple Silicon)

Describe the bug

Our code in the UniStaker repo has unit tests that will fail if certain addresses are selected from storage by the fuzzer so we turned off include_storage in our foundry toml. This has worked fine for a long time. When we added invariant tests, we turned it on for those tests as the same restriction didn't exists. This worked fine across many thousands of fuzz runs locally and in CI.

Sometime recently, something seems to have changed. Now, we see CI failures where the fuzzer is clearly selecting values from storage for our unit tests.

Here's how you can reproduce.

  1. Clone the UniStaker repo
  2. Copy the .env.template to .env and put in a valid RPC URL (we can avoid actually hitting it by not running integration tests) and deployer private key (it can be the default anvil account)
  3. Edit your foundry.toml, under the [profile.default] section add fuzz = { runs = 100000 }
  4. Run locally forge test --mt testFuzz_CalculatesCorrectEarningsForFourUsersWhenTwoStakeMorePartiallyThroughTheDurationAndOneBeneficiary
  5. You should see a test failures. It may take a bit to run. If you don't see failures, you got "lucky", run the command again

Using older values of Foundry this didn't occur.

Edit: Updated this to remove some comments about the settings in the invariant section impacting the fuzz runs. This seems not to be the case. It simply seems like we're getting values picked from storage now (despite the setting) when previously we weren't.

apbendi avatar Mar 20 '24 21:03 apbendi

you should not get those if you set dictionary_weight=0, that is fuzzed calldata won't use state

https://github.com/foundry-rs/foundry/blob/e5318c3054e5f883d1467da9fae5d29567a03d43/crates/evm/evm/src/executors/fuzz/mod.rs#L85-L89

grandizzy avatar Mar 21 '24 03:03 grandizzy

I believe include_storage is still being respected, but the address is being added to the dictionary simply because it exists in the vm database (outside of contract storage), see here: https://github.com/foundry-rs/foundry/blob/e5acbcfe71d5d6687fcc649e91776454f5c3eb73/crates/evm/fuzz/src/strategies/state.rs#L95-L100

However, I don't think that has changed recently so I'm unsure why the behavior appears to have changed. cc @DaniPopes @klkvr

mds1 avatar Mar 25 '24 18:03 mds1

I was not able to reproduce this one anymore since https://github.com/foundry-rs/foundry/pull/7552 fix, @apbendi would you mind retest? thanks

grandizzy avatar Apr 16 '24 18:04 grandizzy

ping @apbendi, would be great if you could retest this if this still occurs, if not we can mark this as resolved

zerosnacks avatar Jul 12 '24 13:07 zerosnacks

Hey @zerosnacks, we ended up working around this in the UniStaker codebase, but I just went back and checked out the commit before that fix was merged and ran 1,000,000 fuzz runs with version forge 0.2.0 (6822860 2024-07-29T00:23:34.055980000Z). All passed without issue. So it seems the specific issue we were encountering may be resolved. Always a little hard to say for sure because these things are non-deterministic. But as far as I can tell this problem is fixed for now.

apbendi avatar Jul 29 '24 12:07 apbendi

Thanks for retesting @apbendi!

Optimistically marking this ticket as resolved, feel free to re-open if you do end up encountering it in the future

zerosnacks avatar Jul 29 '24 12:07 zerosnacks