foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.31k stars 1.75k forks source link

Fuzz test gas in snapshot off by 1 gas #5689

Closed matYang closed 5 months ago

matYang commented 1 year ago

Component

Forge

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

What version of Foundry are you on?

forge 0.2.0 (b536f51 2023-08-21T00:25:51.452435000Z)

What command(s) is the bug in?

forge snapshot --check

Operating System

Linux

Describe the bug

In our CI workflow using Github actions:

      - name: Install Foundry
        uses: foundry-rs/foundry-toolchain@v1
        with:
          version: nightly

We check the present snapshot with forge snapshot --check {{snapshot}}. This step is failing due to 1 fuzz test result: Diff in "...(address,address,uint96)": consumed "(runs: 256, μ: 213572, ~: 213547)" gas, expected "(runs: 256, μ: 213573, ~: 213548)"

The gas measured during CI is consistently lower by 1 than our local machine, i.e. 213572 vs 213573, and 213547 vs 213548.

We are using default number of fuzz runs.

gakonst commented 1 year ago

Which repo can we use to reproduce this? Thanks for reporting!

matYang commented 1 year ago

The complete repo is not open sourced yet, the contract source (without tests) is here: https://www.npmjs.com/package/@chainlink/contracts-ccip?activeTab=readme

The specific test causing issues is a fuzz test again EVM2EVMOnRamp.sol: function testFuzz_ForwardFromRouterSuccess(address originalSender, address receiver, uint96 feeTokenAmount) public {...}

matYang commented 1 year ago

Nice, our ccip repo was made public today, the test causing the issue is here: https://github.com/smartcontractkit/ccip/blob/ccip-develop/contracts/src/v0.8/ccip/test/onRamp/EVM2EVMOnRamp.t.sol#L372

to reproduce it,

  1. export FOUNDRY_PROFILE=ccip
  2. remove the 32 run inline-comment
  3. run the snapshot under /contracts locally
  4. compare it against the result in gh actions
RensR commented 1 year ago

Any update on this? I'm hitting it more and more it seems. Made sure to foundryup and re-initialized the foundry dependencies locally but keep getting a diff with CI. This test isn't even off by one, but ~250 gas. CI run.

CI ran on commit 64faf451b95b939406d068d37eaa3bb034d35250 in this repo

How to reproduce

cd contracts
export FOUNDRY_PROFILE=ccip && forge snapshot --snap gas-snapshots/ccip.gas-snapshot

CI:

Ran 113 test suites: 432 tests passed, 0 failed, 0 skipped (432 total tests)
Diff in "EVM2EVMOnRamp_getTokenTransferCost::testFuzz_TokenTransferFeeDuplicateTokensSuccess(uint256,uint256)": consumed "(runs: 256, μ: 66371, ~: 66420)" gas, expected "(runs: 256, μ: 66623, ~: 66592)" gas 
Error: Process completed with exit code 1.
mattsse commented 1 year ago

fuzz gas tests can differ

but you can run with --tolerance 1 for 1% tolerance

perhaps we should add a --fuzz-tolerance flag explicitly for fuzz test tolerance.

the off by one is likely just an outliner the results in diff average/mean

are you using constant seeds?

for example seed = '0x3e8' (pick any hex number value) should result in reproduceable fuzz runs (not always due to platform-specific randomness)

mds1 commented 1 year ago

@mattsse forge snapshot and forge coverage automatically use a seed to ensure determinism, so it is odd that users report inconsistent gas usage with it, ref https://github.com/foundry-rs/foundry/issues/5199 and https://t.me/foundry_support/38736

https://github.com/foundry-rs/foundry/blob/0f530f2ae63342b136ad65e1c7d3b3231b939a6b/crates/forge/bin/cmd/snapshot.rs#L99-L101

matYang commented 1 year ago

I'm seeing the off-by-1 gas issue more frequently as well, this could be just a platform-specific rounding error.

mattsse commented 1 year ago

hmm, couldn't find anything wrong with the mean/avg math

could you perhaps try increasing/decreasing the fuzzruns, maybe there's an edge case with cross platform fuzzed values resulting in +256gas