Conversation
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| fuzz_target: [deser_net_msg, deserialize_address, deserialize_amount, deserialize_block, deserialize_psbt, deserialize_script, deserialize_transaction, outpoint_string, uint128_fuzz] |
There was a problem hiding this comment.
If we can't generate this from the file list, can CI somewhere fail if this misses a fuzz target?
.github/workflows/rust.yml
Outdated
| steps: | ||
| - name: Install test dependencies | ||
|
|
||
| - name: Install fuzz dependencies |
There was a problem hiding this comment.
Can't this be dropped entirely now?
| run: ls -R | ||
| - run: find executed_* -type f -exec cat {} + | sort > executed | ||
| - run: ls fuzz/fuzz_targets | sort > expected | ||
| - run: diff expected executed |
There was a problem hiding this comment.
I think you want some kind of if statement that if there's a diff we fail the test.
There was a problem hiding this comment.
The if isn't necessary, diff returns not 0 if files are different. See my test where I explicitly removed one fuzz target https://github.com/RCasatta/rust-bitcoin/runs/3698414243?check_suite_focus=true
|
Why do we have such a big diff with all temporary files (which should never get into git) removed? |
Those files are fuzzing test cases stimulating different program paths. Fuzzers could recreate those by running and searching for various inputs, but it could take a long time. Providing those to the CI ensures many program paths are tested even if the fuzzing runs for just 30 seconds, that's why they are committed into git.
fuzz targets |
|
Using rust stable toolchain since honggfuzz is supporting it -> #667 (comment) |
dr-orlovsky
left a comment
There was a problem hiding this comment.
utACK 2bbf63c
Awaiting for @TheBlueMatt confirmation that he is satisfied on the answer to his comment and after that will merge (or he can merge) to get CI fixed and unlock further merging of other PRs
TheBlueMatt
left a comment
There was a problem hiding this comment.
Oops, sorry, yes, my comment was resolved.
My original plan was to run fuzz testing in CI separately with both x86_64 and i686 arch.
Unfortunately, I got issues with the i686 fuzzing https://github.com/RCasatta/rust-bitcoin/actions/runs/1265153484 and apparently specifying different targets is not supported https://github.com/rust-fuzz/honggfuzz-rs/blob/f45aef1b59736fcfc215eca09aca42e49bbf36db/src/bin/cargo-hfuzz.rs#L23
Nevertheless, I think there are some advantages in doing fuzzing in CI separately (faster, better report), however, there are some disadvantages (what launched from the CI is not exactly the same script launched locally). For this reason, I am not sure if 3e310d3 is desirable and if yes, if we want to remove fuzzing in the nightly test.
In case we keep the fuzzing in the nightly test I think 9049eef is desirable because it saves about 30 seconds for other tests that don't need those deps.