From 665f8645a4fe350f84be661c52d48214dfeba629 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 14 Sep 2024 19:14:31 +0000 Subject: [PATCH 1/4] clippy: shorten a doccomment's first line The original doccomment was a self-contained single-sentence description of this iterator. But it was really long and hard to follow. Better to separate out some details into a separate paragraph. --- src/primitives/encode.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/primitives/encode.rs b/src/primitives/encode.rs index c0d3cf1bf..65f2f8521 100644 --- a/src/primitives/encode.rs +++ b/src/primitives/encode.rs @@ -169,9 +169,11 @@ where } } -/// Iterator adaptor which takes a stream of field elements, converts it to characters prefixed by -/// an HRP (and separator), and suffixed by the checksum i.e., converts the data in a stream of -/// field elements into stream of characters representing the encoded bech32 string. +/// Iterator adaptor which converts a stream of field elements to an iterator over the +/// characters of an HRP-string. +/// +/// Does so by converting the field elements to characters, prefixing an HRP, and suffixing +/// a checksum. pub struct CharIter<'hrp, I, Ck> where I: Iterator, From 53664cf25547233bc5f8abc4319a5f2263fdca8a Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 30 Mar 2024 16:03:25 +0000 Subject: [PATCH 2/4] fuzz: remove `afl` feature, clean up fuzztests a bit Substantially rewrite the `encode_decode` fuzztest, which had some ugly/awkward control flow. For the other tests, remove the AFL feature which has not been used in years and the now-unneeded `extern crate` and `macro_use` calls. Now you can fuzz things by just running `cargo hfuzz` directly without needing to set rustflags etc. Also run shellcheck on fuzz.sh, add a bunch of quotes to it, and change the runtime from 100000 iterations (which seems to run in less than a second on my system for all the existing tests) to a fixed 10 seconds. This does **not** pull the full rust-bitcoin fuzztest scripts over. I would like to wait until the "move fuzztests to cron" PR merges in rust-bitcoin, and consider trying to move all the scripts into a shared repo rather than copy/pasting them in, before doing so. --- fuzz/Cargo.toml | 10 ++---- fuzz/fuzz.sh | 14 ++++----- fuzz/fuzz_targets/decode_rnd.rs | 22 +++----------- fuzz/fuzz_targets/encode_decode.rs | 49 +++++++++++------------------- fuzz/fuzz_targets/parse_hrp.rs | 22 +++----------- 5 files changed, 36 insertions(+), 81 deletions(-) diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 7f50403b1..b3d21a63b 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -1,5 +1,7 @@ [package] name = "bech32-fuzz" +edition = "2021" +rust-version = "1.56.1" version = "0.0.1" authors = ["Automatically generated"] publish = false @@ -7,14 +9,8 @@ publish = false [package.metadata] cargo-fuzz = true -[features] -afl_fuzz = ["afl"] -honggfuzz_fuzz = ["honggfuzz"] - [dependencies] -honggfuzz = { version = "0.5", default-features = false, optional = true } -afl = { version = "0.3", optional = true } -libc = "0.2" +honggfuzz = { version = "0.5.55", default-features = false } bech32 = { path = ".." } # Prevent this from interfering with workspaces diff --git a/fuzz/fuzz.sh b/fuzz/fuzz.sh index db1b9eb38..d12891409 100755 --- a/fuzz/fuzz.sh +++ b/fuzz/fuzz.sh @@ -2,17 +2,17 @@ set -e cargo install --force honggfuzz --no-default-features for TARGET in fuzz_targets/*; do - FILENAME=$(basename $TARGET) + FILENAME=$(basename "$TARGET") FILE="${FILENAME%.*}" - if [ -d hfuzz_input/$FILE ]; then + if [ -d "hfuzz_input/$FILE" ]; then HFUZZ_INPUT_ARGS="-f hfuzz_input/$FILE/input" fi - HFUZZ_BUILD_ARGS="--features honggfuzz_fuzz" HFUZZ_RUN_ARGS="-N1000000 --exit_upon_crash -v $HFUZZ_INPUT_ARGS" cargo hfuzz run $FILE + HFUZZ_RUN_ARGS="--run_time 10 --exit_upon_crash -v $HFUZZ_INPUT_ARGS" cargo hfuzz run "$FILE" - if [ -f hfuzz_workspace/$FILE/HONGGFUZZ.REPORT.TXT ]; then - cat hfuzz_workspace/$FILE/HONGGFUZZ.REPORT.TXT - for CASE in hfuzz_workspace/$FILE/SIG*; do - cat $CASE | xxd -p + if [ -f "hfuzz_workspace/$FILE/HONGGFUZZ.REPORT.TXT" ]; then + cat "hfuzz_workspace/$FILE/HONGGFUZZ.REPORT.TXT" + for CASE in "hfuzz_workspace/$FILE/SIG"*; do + xxd -p < "$CASE" done exit 1 fi diff --git a/fuzz/fuzz_targets/decode_rnd.rs b/fuzz/fuzz_targets/decode_rnd.rs index f39629e58..0205df23e 100644 --- a/fuzz/fuzz_targets/decode_rnd.rs +++ b/fuzz/fuzz_targets/decode_rnd.rs @@ -1,7 +1,6 @@ -extern crate bech32; - use bech32::primitives::decode::{CheckedHrpstring, SegwitHrpstring, UncheckedHrpstring}; use bech32::Bech32m; +use honggfuzz::fuzz; // Checks that we do not crash if passed random data while decoding. fn do_test(data: &[u8]) { @@ -11,19 +10,6 @@ fn do_test(data: &[u8]) { let _ = SegwitHrpstring::new(&data_str); } -#[cfg(feature = "afl")] -extern crate afl; -#[cfg(feature = "afl")] -fn main() { - afl::read_stdio_bytes(|data| { - do_test(&data); - }); -} - -#[cfg(feature = "honggfuzz")] -#[macro_use] -extern crate honggfuzz; -#[cfg(feature = "honggfuzz")] fn main() { loop { fuzz!(|data| { @@ -39,9 +25,9 @@ mod tests { for (idx, c) in hex.as_bytes().iter().enumerate() { b <<= 4; match *c { - b'A'...b'F' => b |= c - b'A' + 10, - b'a'...b'f' => b |= c - b'a' + 10, - b'0'...b'9' => b |= c - b'0', + b'A'..=b'F' => b |= c - b'A' + 10, + b'a'..=b'f' => b |= c - b'a' + 10, + b'0'..=b'9' => b |= c - b'0', _ => panic!("Bad hex"), } if (idx & 1) == 1 { diff --git a/fuzz/fuzz_targets/encode_decode.rs b/fuzz/fuzz_targets/encode_decode.rs index bba32ca45..cfe37deac 100644 --- a/fuzz/fuzz_targets/encode_decode.rs +++ b/fuzz/fuzz_targets/encode_decode.rs @@ -1,11 +1,10 @@ -extern crate bech32; - use std::str; use bech32::{Bech32m, Hrp}; +use honggfuzz::fuzz; fn do_test(data: &[u8]) { - if data.len() < 1 { + if data.is_empty() { return; } @@ -17,35 +16,23 @@ fn do_test(data: &[u8]) { let dp = &data[hrp_end..]; - match str::from_utf8(&data[1..hrp_end]) { + let s = match str::from_utf8(&data[1..hrp_end]) { + Ok(s) => s, Err(_) => return, - Ok(s) => { - match Hrp::parse(&s) { - Err(_) => return, - Ok(hrp) => { - if let Ok(address) = bech32::encode::(hrp, dp) { - let (hrp, data) = bech32::decode(&address).expect("should be able to decode own encoding"); - assert_eq!(bech32::encode::(hrp, &data).unwrap(), address); - } - } - } - } - } -} + }; + let hrp = match Hrp::parse(s) { + Ok(hrp) => hrp, + Err(_) => return, + }; + let address = match bech32::encode::(hrp, dp) { + Ok(addr) => addr, + Err(_) => return, + }; -#[cfg(feature = "afl")] -extern crate afl; -#[cfg(feature = "afl")] -fn main() { - afl::read_stdio_bytes(|data| { - do_test(&data); - }); + let (hrp, data) = bech32::decode(&address).expect("should be able to decode own encoding"); + assert_eq!(bech32::encode::(hrp, &data).unwrap(), address); } -#[cfg(feature = "honggfuzz")] -#[macro_use] -extern crate honggfuzz; -#[cfg(feature = "honggfuzz")] fn main() { loop { fuzz!(|data| { @@ -61,9 +48,9 @@ mod tests { for (idx, c) in hex.as_bytes().iter().filter(|&&c| c != b'\n').enumerate() { b <<= 4; match *c { - b'A'...b'F' => b |= c - b'A' + 10, - b'a'...b'f' => b |= c - b'a' + 10, - b'0'...b'9' => b |= c - b'0', + b'A'..=b'F' => b |= c - b'A' + 10, + b'a'..=b'f' => b |= c - b'a' + 10, + b'0'..=b'9' => b |= c - b'0', _ => panic!("Bad hex"), } if (idx & 1) == 1 { diff --git a/fuzz/fuzz_targets/parse_hrp.rs b/fuzz/fuzz_targets/parse_hrp.rs index bebf56ba3..c497f822d 100644 --- a/fuzz/fuzz_targets/parse_hrp.rs +++ b/fuzz/fuzz_targets/parse_hrp.rs @@ -1,6 +1,5 @@ -extern crate bech32; - use bech32::Hrp; +use honggfuzz::fuzz; fn do_test(data: &[u8]) { let s = String::from_utf8_lossy(data); @@ -10,19 +9,6 @@ fn do_test(data: &[u8]) { let _ = Hrp::parse(&s); } -#[cfg(feature = "afl")] -extern crate afl; -#[cfg(feature = "afl")] -fn main() { - afl::read_stdio_bytes(|data| { - do_test(&data); - }); -} - -#[cfg(feature = "honggfuzz")] -#[macro_use] -extern crate honggfuzz; -#[cfg(feature = "honggfuzz")] fn main() { loop { fuzz!(|data| { @@ -38,9 +24,9 @@ mod tests { for (idx, c) in hex.as_bytes().iter().filter(|&&c| c != b'\n').enumerate() { b <<= 4; match *c { - b'A'...b'F' => b |= c - b'A' + 10, - b'a'...b'f' => b |= c - b'a' + 10, - b'0'...b'9' => b |= c - b'0', + b'A'..=b'F' => b |= c - b'A' + 10, + b'a'..=b'f' => b |= c - b'a' + 10, + b'0'..=b'9' => b |= c - b'0', _ => panic!("Bad hex"), } if (idx & 1) == 1 { From 207c6dd17501ac091dd06a9a84f630ef75eaec71 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 29 Mar 2024 17:54:08 +0000 Subject: [PATCH 3/4] field: add Sum trait Also tighten the ops::Neg bound to specify that the output needs to be Self. Otherwise this bound is not really that useful. --- src/primitives/field.rs | 19 +++++++++++++++++-- src/primitives/polynomial.rs | 3 +-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/primitives/field.rs b/src/primitives/field.rs index d7da8c794..5f8636dce 100644 --- a/src/primitives/field.rs +++ b/src/primitives/field.rs @@ -2,7 +2,7 @@ //! Generic Field Traits -use core::{fmt, hash, ops}; +use core::{fmt, hash, iter, ops}; /// A generic field. pub trait Field: @@ -13,6 +13,8 @@ pub trait Field: + hash::Hash + fmt::Debug + fmt::Display + + iter::Sum + + for<'a> iter::Sum<&'a Self> + ops::Add + ops::Sub + ops::AddAssign @@ -29,7 +31,7 @@ pub trait Field: + for<'a> ops::MulAssign<&'a Self> + for<'a> ops::Div<&'a Self, Output = Self> + for<'a> ops::DivAssign<&'a Self> - + ops::Neg + + ops::Neg { /// The zero constant of the field. const ZERO: Self; @@ -362,6 +364,19 @@ macro_rules! impl_ops_for_fe { self._neg() } } + + // sum + impl core::iter::Sum for $op { + fn sum>(iter: I) -> Self { + iter.fold(crate::primitives::Field::ZERO, |i, acc| i + acc) + } + } + + impl<'s> core::iter::Sum<&'s Self> for $op { + fn sum>(iter: I) -> Self { + iter.fold(crate::primitives::Field::ZERO, |i, acc| i + acc) + } + } }; } pub(super) use impl_ops_for_fe; diff --git a/src/primitives/polynomial.rs b/src/primitives/polynomial.rs index a09750522..3ef65b4f0 100644 --- a/src/primitives/polynomial.rs +++ b/src/primitives/polynomial.rs @@ -90,8 +90,7 @@ impl Polynomial { let mut cand = F::ONE; let mut eval = self.clone(); for _ in 0..F::MULTIPLICATIVE_ORDER { - let sum = eval.inner.iter().cloned().fold(F::ZERO, F::add); - if sum == F::ZERO { + if eval.inner.iter().sum::() == F::ZERO { ret.push(cand.clone()); } From b2252b7e5f68fbd49b1dbe6026baf447278f61cf Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 14 Sep 2024 19:22:57 +0000 Subject: [PATCH 4/4] ci: upgrade upload-artifact and download-artifact to v4 Apparently v2 is deprecated and no longer supported. v3 works for us and v4 has some new limit on the size of uploads, but those shouldn't be a problem here, so jump all the way to v4. --- .github/workflows/fuzz.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml index f75d565cc..4a0d12611 100644 --- a/.github/workflows/fuzz.yml +++ b/.github/workflows/fuzz.yml @@ -28,7 +28,7 @@ jobs: - name: fuzz run: cd fuzz && ./fuzz.sh "${{ matrix.fuzz_target }}" - run: echo "${{ matrix.fuzz_target }}.rs" >executed_${{ matrix.fuzz_target }} - - uses: actions/upload-artifact@v2 + - uses: actions/upload-artifact@v4 with: name: executed_${{ matrix.fuzz_target }} path: executed_${{ matrix.fuzz_target }} @@ -39,7 +39,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - - uses: actions/download-artifact@v2 + - uses: actions/download-artifact@v4 - name: Display structure of downloaded files run: ls -R - run: find executed_* -type f -exec cat {} + | sort > executed