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

feat invariant (#5868) - configure calldata fuzzed addresses dictionary #7240

Merged
merged 7 commits into from
Mar 2, 2024

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Feb 26, 2024

Motivation

Atm there's no way to configure calldata fuzzed params behavior - this could in particular be an issue when fuzzing addresses as there's little to no chance the same address is reused.
For invariant testing this is a bigger issue as one would often want to randomly reuse addresses when calling different handler selectors (see #5868). Some projects maintains an array of addresses in handlers and fuzz the indexes / prank with users from array of addresses - that's a little bit hard to mantain and cannot scale for too many addresses.

Solution

This PR introduce an additional config for the dictionary of addresses used in calldata fuzzing.

[invariant]
max_calldata_fuzz_dictionary_addresses=100
  • added FuzzDictionaryConfig.max_calldata_fuzz_dictionary_addresses option to specify how many random addresses to generate and to randomly select from when fuzzing calldata. If option is not specified then current behavior applies
  • to narrow down number of runs / addresses involved in invariant test the CalldataFuzzDictionaryConfig is populated with random addresses plus all accounts from db (from EvmFuzzState)
  • added fuzz_calldata_with_config fn that accepts Option<CalldataFuzzDictionary> as param. Non invariants tests use existing fuzz_calldata fn and pass None as config arg

Further improvements:

  • add other options to generate fuzz dictionary address (like from external csv file, from inline config, etc)
  • extend config also for basic (non invariant) tests
  • extend calldata fuzz config with rules for other fuzzed param types

Todo:

  • add inline config
  • add tests

- added `FuzzDictionaryConfig.max_calldata_fuzz_dictionary_addresses` option to specify how many random addresses to generate and to randomly select from when fuzzing calldata. If option is not specified then current behavior applies
- to narrow down number of runs / addresses involved in invariant test the `CalldataFuzzDictionaryConfig` is populated with random addresses plus all accounts from db (from `EvmFuzzState`)
- added `fuzz_calldata_with_config` fn that accepts `Option<CalldataFuzzDictionary>` as param. Non invariants tests use existing `fuzz_calldata` fn and pass None as config arg
@grandizzy grandizzy changed the title feat invariant - configure calldata fuzzed addresses feat invariant - configure calldata fuzzed addresses dictionary Feb 26, 2024
@grandizzy grandizzy changed the title feat invariant - configure calldata fuzzed addresses dictionary feat invariant (#5868) - configure calldata fuzzed addresses dictionary Feb 27, 2024
@grandizzy grandizzy marked this pull request as ready for review February 27, 2024 08:48
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.

I think that makes sense

left some nits

}

impl CalldataFuzzDictionaryConfig {
pub fn new(config: &FuzzDictionaryConfig, state: EvmFuzzState) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

please add some docs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added in b77dcb6

use proptest::prelude::{BoxedStrategy, Strategy};
use std::{fmt, sync::Arc};

pub type CalldataFuzzDictionary = Arc<CalldataFuzzDictionaryConfig>;
Copy link
Member

Choose a reason for hiding this comment

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

I really disklike this alias style, this makes it much harder to navigate.

I know we have this in a few other places, but this isn't very helpful. I'd prefer a new wrapper type like

struct CalldataFuzzDictionary {
   inner: Arc<CalldataFuzzDictionaryConfig>
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done with b77dcb6 plus some code cleanup

@aviggiano
Copy link

Will this also allow users to specify the number of distinct senders?

AFAIK Foundry currently uses an unlimited number of msg.sender, which is not always ideal in some scenarios of invariant testing. Echidna/Medusa, for example, uses only 3 addresses by default.

It should be nice to configure that as well

@grandizzy
Copy link
Collaborator Author

Will this also allow users to specify the number of distinct senders?

AFAIK Foundry currently uses an unlimited number of msg.sender, which is not always ideal in some scenarios of invariant testing. Echidna/Medusa, for example, uses only 3 addresses by default.

It should be nice to configure that as well

correct, there is a new config max_calldata_fuzz_dictionary_addresses to specify the number of additional random addresses when fuzzing calldata.
Re msg.sender - that's already finite to the number of accounts from state and you can get to your desired set by using targetSender(address newTargetedSender_) see options here https://book.getfoundry.sh/forge/invariant-testing#invariant-test-helper-functions

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.

lgtm, pending conflicts

@grandizzy
Copy link
Collaborator Author

lgtm, pending conflicts

done

@mattsse mattsse merged commit 2d54c1f into foundry-rs:master Mar 2, 2024
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.

3 participants