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

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

Closed
1 of 2 tasks
apbendi opened this issue Mar 20, 2024 · 6 comments
Closed
1 of 2 tasks
Assignees
Labels
A-testing Area: testing T-bug Type: bug
Milestone

Comments

@apbendi
Copy link

apbendi commented Mar 20, 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 (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 apbendi added the T-bug Type: bug label Mar 20, 2024
@grandizzy
Copy link
Collaborator

grandizzy commented Mar 21, 2024

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

let dictionary_weight = self.config.dictionary.dictionary_weight.min(100);
let strat = proptest::prop_oneof![
100 - dictionary_weight => fuzz_calldata(func.clone()),
dictionary_weight => fuzz_calldata_from_state(func.clone(), &state),
];

@mds1
Copy link
Collaborator

mds1 commented Mar 25, 2024

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:

let mut state = FuzzDictionary::default();
for (address, account) in db.accounts.iter() {
let address: Address = *address;
// Insert basic account information
state.values_mut().insert(address.into_word().into());

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

@grandizzy
Copy link
Collaborator

I was not able to reproduce this one anymore since #7552 fix, @apbendi would you mind retest? thanks

@zerosnacks zerosnacks added the A-testing Area: testing label Jul 12, 2024
@zerosnacks
Copy link
Member

zerosnacks commented Jul 12, 2024

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

@zerosnacks zerosnacks self-assigned this Jul 12, 2024
@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@apbendi
Copy link
Author

apbendi commented Jul 29, 2024

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.

@zerosnacks
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Area: testing T-bug Type: bug
Projects
None yet
Development

No branches or pull requests

4 participants