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

Reduce CI fuzz iterations as we're now timing out #3691

Merged
merged 3 commits into from
Apr 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -262,11 +262,12 @@ jobs:
sudo apt-get -y install build-essential binutils-dev libunwind-dev
- name: Pin the regex dependency
run: |
cd fuzz && cargo update -p regex --precise "1.9.6" --verbose && cd ..
cd fuzz && cargo update -p regex --precise "1.9.6" --verbose
cd write-seeds && cargo update -p regex --precise "1.9.6" --verbose
- name: Sanity check fuzz targets on Rust ${{ env.TOOLCHAIN }}
run: |
cd fuzz
RUSTFLAGS="--cfg=fuzzing --cfg=secp256k1_fuzz --cfg=hashes_fuzz" cargo test --verbose --color always
RUSTFLAGS="--cfg=fuzzing --cfg=secp256k1_fuzz --cfg=hashes_fuzz" cargo test --verbose --color always --lib --bins
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these two additions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because RUSTFLAGS aren't applied to doctests, so we have to restrict tests to only run library and binary tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could add comments when making changes like this. Make life easier for future devs.

cargo clean
- name: Run fuzzers
run: cd fuzz && ./ci-fuzz.sh && cd ..
Expand Down
21 changes: 16 additions & 5 deletions fuzz/ci-fuzz.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,19 @@ rm *_target.rs
[ "$(git diff)" != "" ] && exit 1
popd

export RUSTFLAGS="--cfg=secp256k1_fuzz --cfg=hashes_fuzz"

mkdir -p hfuzz_workspace/full_stack_target/input
pushd write-seeds
RUSTFLAGS="$RUSTFLAGS --cfg=fuzzing" cargo run ../hfuzz_workspace/full_stack_target/input
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to verify that using the hardcoded seed indeed increases coverage, but it seems that hongfuzz reports in the log always

branch_coverage_percent:0

Not sure why that is? Otherwise it would be easy to compare with and without this hardcoded seed.

Or maybe there is another way to verify that it works indeed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fact that the full_stack_target got much slower is pretty good evidence :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, indirect evidence. But why is hongfuzz reporting 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not using coverage to direct fuzzing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, that I don't know, it certainly does locally and there's nothing different locally vs in CI. Also CI does turn up (fairly shallow) fuzz bugs as well, so I'd be surprised to learn it was able to do that with just general patterns and no instrumentation...

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it show you branch_coverage_percent as non-zero locally?

popd

cargo install --color always --force honggfuzz --no-default-features
# Because we're fuzzing relatively few iterations, the maximum possible
# compiler optimizations aren't necessary, so switch to defaults.
sed -i 's/lto = true//' Cargo.toml
sed -i 's/codegen-units = 1//' Cargo.toml

export RUSTFLAGS="--cfg=secp256k1_fuzz --cfg=hashes_fuzz"
export HFUZZ_BUILD_ARGS="--features honggfuzz_fuzz"

cargo --color always hfuzz build
Expand All @@ -25,11 +34,13 @@ for TARGET in src/bin/*.rs; do
FILE="${FILENAME%.*}"
HFUZZ_RUN_ARGS="--exit_upon_crash -v -n2"
if [ "$FILE" = "chanmon_consistency_target" ]; then
HFUZZ_RUN_ARGS="$HFUZZ_RUN_ARGS -F 64 -N100000"
elif [ "$FILE" = "full_stack_target" ]; then
HFUZZ_RUN_ARGS="$HFUZZ_RUN_ARGS -t0 -N1000000"
HFUZZ_RUN_ARGS="$HFUZZ_RUN_ARGS -F 64 -N5000"
elif [ "$FILE" = "process_network_graph_target" -o "$FILE" = "full_stack_target" -o "$FILE" = "router_target" ]; then
HFUZZ_RUN_ARGS="$HFUZZ_RUN_ARGS -N50000"
elif [ "$FILE" = "indexedmap_target" ]; then
HFUZZ_RUN_ARGS="$HFUZZ_RUN_ARGS -N500000"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reasoning behind the 10x, 10x and 2x reductions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. I probably could have benchmarked, but in general there's not a lot of value in full_stack_target in CI because its just too dense to make any real progress, and similar for chanmon_consistency_target.

else
HFUZZ_RUN_ARGS="$HFUZZ_RUN_ARGS -N1000000"
HFUZZ_RUN_ARGS="$HFUZZ_RUN_ARGS -N2500000"
fi
export HFUZZ_RUN_ARGS
cargo --color always hfuzz run $FILE
Expand Down
1,242 changes: 624 additions & 618 deletions fuzz/src/full_stack.rs

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions fuzz/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ extern crate bitcoin;
extern crate lightning;
extern crate lightning_rapid_gossip_sync;

#[cfg(not(fuzzing))]
compile_error!("Fuzz targets need cfg=fuzzing");

#[cfg(not(hashes_fuzz))]
compile_error!("Fuzz targets need cfg=hashes_fuzz");

#[cfg(not(secp256k1_fuzz))]
compile_error!("Fuzz targets need cfg=secp256k1_fuzz");

pub mod utils;

pub mod base32;
Expand Down
15 changes: 15 additions & 0 deletions fuzz/write-seeds/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
[package]
name = "lightning-fuzz-write-seeds"
version = "0.0.1"
authors = ["Automatically generated"]
publish = false
edition = "2021"

[features]

[dependencies]
lightning-fuzz = { path = "../" }

# Prevent this from interfering with workspaces
[workspace]
members = ["."]
6 changes: 6 additions & 0 deletions fuzz/write-seeds/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fn main() {
let mut iter = std::env::args();
iter.next().unwrap(); // program name
let path = iter.next().expect("Requires a path as the first argument");
lightning_fuzz::full_stack::write_fst_seeds(&path);
}
2 changes: 1 addition & 1 deletion lightning/src/routing/gossip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1786,7 +1786,7 @@ where
}

fn test_node_counter_consistency(&self) {
#[cfg(test)]
#[cfg(any(test, fuzzing))]
{
let channels = self.channels.read().unwrap();
let nodes = self.nodes.read().unwrap();
Expand Down
Loading