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

fix: collect push bytes from test setup in fuzzer #2929

Merged
merged 3 commits into from
Aug 25, 2022

Conversation

onbjerg
Copy link
Member

@onbjerg onbjerg commented Aug 24, 2022

Motivation

There's an old report that immutables were not collected in the fuzzer. Immutables are sections in the bytecode of contracts that are replaced by constant values, so we should be collecting them (since we collect push bytes), but it turns out that we were not collecting push bytes in the initial fuzzer DB (i.e. from the state right after setUp).

See #1168

Solution

Also collect push bytes after setUp. Closes #1168

Note: The state dict after setUp for the test in #1168 contains about ~120 values. It seems that either one of the tests fail every time (which is what we want), but not super reliably. Increasing the fuzzer runs to 512 makes them both fail reliably, though.

@onbjerg onbjerg added the T-bug Type: bug label Aug 24, 2022
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.

smol nit

evm/src/fuzz/strategies/state.rs Outdated Show resolved Hide resolved
@mattsse
Copy link
Member

mattsse commented Aug 24, 2022

unsure if this is related but this now results in

Failing tests:
Encountered 2 failing tests in src/test/StdAssertions.t.sol:StdAssertionsTest
[FAIL. Reason: Too many global rejects] testAssertEq_BytesErr_Pass(bytes,bytes) (runs: 31, μ: 20023, ~: 21439)
[FAIL. Reason: Too many global rejects] testAssertEq_Bytes_Pass(bytes,bytes) (runs: 24, μ: 19965, ~: 20697)

when running forge test on forge-std, I could reproduce this locally

@joshieDo
Copy link
Collaborator

We had this discussion here: #2724 (comment)

We decided to revert the push collection on build_initial_state. But I'm all for keeping this and change the forge-std test instead.

@rkrasiuk rkrasiuk added the A-testing Area: testing label Aug 25, 2022
@onbjerg
Copy link
Member Author

onbjerg commented Aug 25, 2022

The Forge Std assume call is just way too narrow so I'd expect it to reach the limit, and the fact that the reject limit hasn't been reached before is not ideal. Either they can increase the reject limit in Forge Std or just fix the test, obviously if the assume call makes it so a is always b then they should just take a single parameter.

@mds1
Copy link
Collaborator

mds1 commented Aug 25, 2022

Just merged foundry-rs/forge-std#160 which should prevent foundry CI from failing on those now

@gakonst gakonst force-pushed the onbjerg/fuzz-push-bytes-setup branch from 51f034b to 54c6c04 Compare August 25, 2022 23:39
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Rebased, merging per the forge-std fixes. Flagging for us to keep this PR in mind if people come to us with complaints about the fuzzer error'ing too much.

@gakonst gakonst merged commit 56dc746 into master Aug 25, 2022
@gakonst gakonst deleted the onbjerg/fuzz-push-bytes-setup branch August 25, 2022 23:42
iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
* fix: collect push bytes from test setup in fuzzer

* Update evm/src/fuzz/strategies/state.rs

* chore: rustfmt

Co-authored-by: Matthias Seitz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Area: testing T-bug Type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: fuzzer dict does not contain immutables
6 participants