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: add boundLog to enable log-uniform sampling #584

Closed
wants to merge 1 commit into from

Conversation

guil-lambert
Copy link

Using bound to generate random inputs is heavily skewed towards the upper end of the range [see below]. To explore a more balanced distribution of values, users should have the option to sample in log space instead.

This PR adds boundLog(uint256 x, uint8 min, uint8 max), which generates a random number between [2**min, 2**(max+1)-1] that is uniformly distributed in log space (ie. data generated follows a log-uniform distribution)

Note: contrarily to bound, boundLog WILL NOT return x when the input x is within the provided [2**min, 2**(max+1)-1] bounds.

bound vs boundLog sampling between 1 and 2^32, N=10**6
image

@guil-lambert guil-lambert changed the title feat: add boundLog to enable uniform sampling in log space feat: add boundLog to enable log-uniform sampling Jul 22, 2024
@mds1
Copy link
Collaborator

mds1 commented Jul 24, 2024

For large ranges I agree logarithmic distribution is preferable

Note: contrarily to bound, boundLog WILL NOT return x when the input x is within the provided [2**min, 2**(max+1)-1] bounds.

This is a pretty big weakness, as it means you are purely fuzzing with random values and not using any of the foundry strategies, such as edge biasing or the dictionary. Purely random fuzzing is the least valuable strategy.

Also, is this actually the ideal solution, compared to updating random number generation in foundry to support this? If there is a way we can improve this upstream that is typically preferable to avoid forge-std bloat, so just want to make sure we think through the solution space

@mds1
Copy link
Collaborator

mds1 commented Jul 24, 2024

cc @grandizzy @klkvr for thoughts

@grandizzy
Copy link
Contributor

grandizzy commented Jul 26, 2024

@mds1 I think it makes sense to have such directly in foundry and have strategies generate values in bounded range. What I can see here as a potential issue is that we will need config to specify limits per fuzzed param so could lead to more complex config, e.g. for a fuzzed test like testFuzz(uint256 amount, uint256 b) we need to specify a min/max for amount and another one (or none) for b
Since we already have such per named param config for fixtures we could extend it to bounds too, maybe like

uint256[] public fixtureAmount = [5 * 1e18, 15 * 1e18, 25 * 1e18, 35 * 1e18];
uint8[2]  public boundAmount   = [5, 10];

(ref https://book.getfoundry.sh/forge/fuzz-testing#fuzz-test-fixtures)

@mds1
Copy link
Collaborator

mds1 commented Jul 28, 2024

Yea the UX for this was always the trickiest, in the past we also considered just using the inline config which could be a good approach:

/// forge-config: default.fuzz.range.x = [5, type(uint256).max];
/// forge-config: default.fuzz.distribution.x = logarithmic;
/// forge-config: default.fuzz.range.y = [1, 10];
/// forge-config: default.fuzz.distribution.x = uniform;
function testFoo(uint256 x, uint256 y) public {
  // --- snip ---
}

How would you specify distribution type with fixtures? Though, I am hesitant to expose distribution
type at all because it adds more complexity to fuzz config. I would prefer to add some logic to forge to detect when this is needed and just accordingly. For example if fuzzing over a really large space like uint256, logarithmic uniformity is preferable and should be the default. If fuzzing a uint8 standard uniform sampling is preferable.

@grandizzy
Copy link
Contributor

How would you specify distribution type with fixtures?

Yeah, wasn't thinking to use fixtures for distribution but maybe having a similar way to current fixtures for all configuration of named parameter in Solidity

struct Uint256ParamConfig {
    uint256 min;
    uint256 max;
    uint256[] fixtures;
    uint256[] excluded;
}

Uint256ParamConfig public configAmount;

(basically including fixtures in same config and also superseeding assume and bound cheatcodes per named param). Though probably inline config is better UX here considering that we need specific struct per type...

I would prefer to add some logic to forge to detect when this is needed and just accordingly. For example if fuzzing over a really large space like uint256, logarithmic uniformity is preferable and should be the default. If fuzzing a uint8 standard uniform sampling is preferable.

Yep, agree, maybe we should start an issue in foundry to track / implement these?

@mds1
Copy link
Collaborator

mds1 commented Jul 28, 2024

Yep, agree, maybe we should start an issue in foundry to track / implement these?

That sounds good to me, would you mind creating that? :)

@07Vaishnavi-Singh
Copy link

07Vaishnavi-Singh commented Sep 14, 2024

How would you specify distribution type with fixtures?

Yeah, wasn't thinking to use fixtures for distribution but maybe having a similar way to current fixtures for all configuration of named parameter in Solidity

struct Uint256ParamConfig {
    uint256 min;
    uint256 max;
    uint256[] fixtures;
    uint256[] excluded;
}

Uint256ParamConfig public configAmount;

(basically including fixtures in same config and also superseeding assume and bound cheatcodes per named param). Though probably inline config is better UX here considering that we need specific struct per type...

I would prefer to add some logic to forge to detect when this is needed and just accordingly. For example if fuzzing over a really large space like uint256, logarithmic uniformity is preferable and should be the default. If fuzzing a uint8 standard uniform sampling is preferable.

Yep, agree, maybe we should start an issue in foundry to track / implement these?

Hey I am working on this issue and trying to solve it using the struct

struct ParamConfig {
        uint256 min;
        uint256 max;
        DistributionType distributionType;
        uint256[] fixtures;
        uint256[] excluded;
    } 

and I am fetching the from another function that is checking the unit type of the value. Is it important to keep the exact same struct to solve the issue?
Uniform is preffered for uint8,uint16,uint32 and uint64 ? and logarithmic for rest ?
@grandizzy

@mds1
Copy link
Collaborator

mds1 commented Oct 24, 2024

Closing in favor of foundry-rs/foundry#9154

@mds1 mds1 closed this Oct 24, 2024
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.

4 participants