foundry-rs / forge-std

Forge Standard Library is a collection of helpful contracts for use with forge and foundry. It leverages forge's cheatcodes to make writing tests easier and faster, while improving the UX of cheatcodes. For more in-depth usage examples checkout the tests.
Apache License 2.0
842 stars 333 forks source link

feat: add boundLog to enable log-uniform sampling #584

Closed guil-lambert closed 1 week ago

guil-lambert commented 3 months ago

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

mds1 commented 3 months ago

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 commented 3 months ago

cc @grandizzy @klkvr for thoughts

grandizzy commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 1 month ago

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 commented 1 week ago

Closing in favor of https://github.com/foundry-rs/foundry/pull/9154