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

Fuzz test gas in snapshot off by 1 gas #5689

Closed
2 tasks done
matYang opened this issue Aug 21, 2023 · 8 comments · Fixed by #7951
Closed
2 tasks done

Fuzz test gas in snapshot off by 1 gas #5689

matYang opened this issue Aug 21, 2023 · 8 comments · Fixed by #7951
Labels
T-bug Type: bug

Comments

@matYang
Copy link

matYang commented Aug 21, 2023

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 (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.

@matYang matYang added the T-bug Type: bug label Aug 21, 2023
@gakonst gakonst added this to Foundry Aug 21, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Aug 21, 2023
@matYang matYang changed the title Fuzz test gas cost in snapshot off by 1 gas Fuzz test gas in snapshot off by 1 gas Aug 21, 2023
@gakonst
Copy link
Member

gakonst commented Aug 22, 2023

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

@matYang
Copy link
Author

matYang commented Aug 22, 2023

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
Copy link
Author

matYang commented Aug 23, 2023

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
Copy link

RensR commented Sep 4, 2023

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
Copy link
Member

mattsse commented Sep 4, 2023

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
Copy link
Collaborator

mds1 commented Sep 5, 2023

@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 #5199 and https://t.me/foundry_support/38736

pub async fn run(mut self) -> Result<()> {
// Set fuzz seed so gas snapshots are deterministic
self.test.fuzz_seed = Some(U256::from_big_endian(&STATIC_FUZZ_SEED));

@matYang
Copy link
Author

matYang commented Sep 7, 2023

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

@mattsse
Copy link
Member

mattsse commented Sep 7, 2023

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

hyunchel added a commit to hyunchel/v4-core that referenced this issue Oct 16, 2023
The previous attempt to assert identical gas usage in CI has been
reverted because a snapshot generated in CI was different from a
snapshot generated locally.

The diff still exist as of today, but amounts are minuscule. The amounts
range from 1 to tens. And these diffs exist in fuzz tests only.

This is a known bug in Foundry: foundry-rs/foundry#5689

This commit adds back gas snapshot, and check for a minimum diff in CI.
This way, snapshot diff tool can be used while waiting for the Foundry
issue to be fixed.

Resolves issue Uniswap#216
hyunchel added a commit to hyunchel/v4-core that referenced this issue Oct 16, 2023
The previous attempt to assert identical gas usage in CI has been
reverted because a snapshot generated in CI was different from a
snapshot generated locally.

The diff still exist as of today, but amounts are minuscule. The amounts
range from 1 to tens. And these diffs exist in fuzz tests only.
This is a known bug in Foundry: foundry-rs/foundry#5689

This commit adds back gas snapshot, and check for a minimum diff in CI.
This way, snapshot diff tool can be used while waiting for the Foundry
issue to be fixed.

Resolves issue Uniswap#216
hyunchel added a commit to hyunchel/v4-core that referenced this issue Oct 16, 2023
The previous attempt to assert identical gas usage in CI has been
reverted because a snapshot generated in CI was different from a
snapshot generated locally.

The diff still exist as of today, but amounts are minuscule. The amounts
range from 1 to tens. And these diffs exist in fuzz tests only.
This is a known bug in Foundry: foundry-rs/foundry#5689

This commit adds back gas snapshot, and check for a small diff in CI.
By allowing just one percent of diff, forge snapshot diff tool can be
utilized while waiting for the Foundry issue to be fixed.

Resolves issue Uniswap#216
@jenpaff jenpaff moved this from Todo to Completed in Foundry Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants