From ecbc6b622395923903337838b357b06c59c50545 Mon Sep 17 00:00:00 2001 From: adria0 Date: Fri, 16 Feb 2024 11:21:35 +0100 Subject: [PATCH] fix: compilation, docs, CI & clippy --- .github/workflows/lints.yml | 72 +++++++++++++++++++++++++++++++++++++ .github/workflows/test.yml | 60 +++++++++++++++++++++++++++++++ Cargo.toml | 3 +- README.md | 2 +- benches/poseidon.rs | 2 +- benches/rm_primitives.rs | 42 +++------------------- rust-toolchain | 1 + src/lib.rs | 4 +-- src/poseidon.rs | 2 +- src/poseidon/pow5.rs | 24 ++++++------- src/poseidon/primitives.rs | 2 +- 11 files changed, 153 insertions(+), 61 deletions(-) create mode 100644 .github/workflows/lints.yml create mode 100644 .github/workflows/test.yml create mode 100644 rust-toolchain diff --git a/.github/workflows/lints.yml b/.github/workflows/lints.yml new file mode 100644 index 0000000..980f4e8 --- /dev/null +++ b/.github/workflows/lints.yml @@ -0,0 +1,72 @@ +name: Lints + +# We only run these lints on trial-merges of PRs to reduce noise. +on: + merge_group: + pull_request: + types: [synchronize, opened, reopened, ready_for_review] + push: + branches: + - main + +jobs: + skip_check: + runs-on: ubuntu-latest + outputs: + should_skip: ${{ steps.skip_check.outputs.should_skip }} + steps: + - id: skip_check + uses: fkirc/skip-duplicate-actions@v5 + with: + cancel_others: 'true' + concurrent_skipping: 'same_content_newer' + + lints: + needs: [skip_check] + if: | + github.event.pull_request.draft == false && + (github.event.action == 'ready_for_review' || needs.skip_check.outputs.should_skip != 'true') + + name: Various lints + timeout-minutes: 30 + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v2 + - uses: actions-rs/toolchain@v1 + with: + components: rustfmt, clippy + override: false + - name: Cargo cache + uses: actions/cache@v3 + with: + path: | + ~/.cargo/bin/ + ~/.cargo/registry/index/ + ~/.cargo/registry/cache/ + ~/.cargo/git/db/ + target/ + key: lint-${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} + - name: Cargo build + uses: actions-rs/cargo@v1 + with: + command: build + - name: Check code format + uses: actions-rs/cargo@v1 + with: + command: fmt + args: --all -- --check + + - name: Run clippy + uses: actions-rs/cargo@v1 + with: + command: clippy + args: --all-features --all-targets -- -D warnings + + # Ensure intra-documentation links all resolve correctly + # Requires #![deny(intra_doc_link_resolution_failure)] in crates. + - name: Check intra-doc links + uses: actions-rs/cargo@v1 + with: + command: doc + args: --no-deps --all --document-private-items \ No newline at end of file diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..b240c09 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,60 @@ +name: CI checks + +on: + merge_group: + pull_request: + push: + branches: + - main + +jobs: + test: + name: Test on ${{ matrix.os }} with ${{ matrix.feature_set }} features + runs-on: ${{ matrix.os }} + strategy: + matrix: + feature_set: [basic, all] + os: [ubuntu-latest, windows-latest, macOS-latest] + include: + - feature_set: all + features: test-dev-graph,circuit-params + + steps: + - uses: actions/checkout@v3 + - uses: actions-rs/toolchain@v1 + with: + override: false + - name: Run tests + uses: actions-rs/cargo@v1 + with: + command: test + args: --verbose --release --workspace --no-default-features --features "${{ matrix.features }}" + + steps: + - uses: actions/checkout@v3 + - uses: actions-rs/toolchain@v1 + with: + override: false + - name: Add target + run: rustup target add ${{ matrix.target }} + - name: cargo build + uses: actions-rs/cargo@v1 + with: + command: build + args: --no-default-features --target ${{ matrix.target }} + + bitrot: + name: Bitrot check + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + - uses: actions-rs/toolchain@v1 + with: + override: false + # Build benchmarks to prevent bitrot + - name: Build benchmarks + uses: actions-rs/cargo@v1 + with: + command: build + args: --benches --examples --all-features \ No newline at end of file diff --git a/Cargo.toml b/Cargo.toml index 55a5011..a7c4be2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "poseidon" +name = "halo2_poseidon" version = "0.2.0" authors = [ "Sean Bowe ", @@ -59,7 +59,6 @@ circuit-params = ["halo2_proofs/circuit-params"] test-dependencies = ["proptest"] unstable = [] - [[bench]] name = "poseidon" harness = false diff --git a/README.md b/README.md index 4281c42..b5e0a44 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [Poseidon hash function](https://eprint.iacr.org/2019/458.pdf) gadget for Halo2. -Requires Rust 1.56.1+. +Requires Rust 1.72.0+. ## Documentation diff --git a/benches/poseidon.rs b/benches/poseidon.rs index 4175318..b06efdc 100644 --- a/benches/poseidon.rs +++ b/benches/poseidon.rs @@ -20,7 +20,7 @@ use halo2_proofs::{ }; use halo2curves::pasta::{pallas, vesta, EqAffine, Fp}; -use halo2_gadgets::poseidon::{ +use halo2_poseidon::poseidon::{ primitives::{self as poseidon, generate_constants, ConstantLength, Mds, Spec}, Hash, Pow5Chip, Pow5Config, }; diff --git a/benches/rm_primitives.rs b/benches/rm_primitives.rs index 5397efc..f24e304 100644 --- a/benches/rm_primitives.rs +++ b/benches/rm_primitives.rs @@ -1,17 +1,14 @@ -use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion}; +use criterion::{criterion_group, criterion_main, Criterion}; use ff::Field; -use halo2_gadgets::{ - poseidon::primitives::{self as poseidon, ConstantLength, P128Pow5T3}, - sinsemilla::primitives as sinsemilla, -}; +use halo2_poseidon::poseidon::primitives::{self as poseidon, ConstantLength, P128Pow5T3}; use halo2curves::pasta::pallas; #[cfg(unix)] use pprof::criterion::{Output, PProfProfiler}; -use rand::{rngs::OsRng, Rng}; +use rand::rngs::OsRng; fn bench_primitives(c: &mut Criterion) { - let mut rng = OsRng; + let rng = OsRng; { let mut group = c.benchmark_group("Poseidon"); @@ -24,37 +21,6 @@ fn bench_primitives(c: &mut Criterion) { }) }); } - - { - let mut group = c.benchmark_group("Sinsemilla"); - - let hasher = sinsemilla::HashDomain::new("hasher"); - let committer = sinsemilla::CommitDomain::new("committer"); - let bits: Vec = (0..1086).map(|_| rng.gen()).collect(); - let r = pallas::Scalar::random(rng); - - // Benchmark the input sizes we use in Orchard: - // - 510 bits for Commit^ivk - // - 520 bits for MerkleCRH - // - 1086 bits for NoteCommit - for size in [510, 520, 1086] { - group.bench_function(BenchmarkId::new("hash-to-point", size), |b| { - b.iter(|| hasher.hash_to_point(bits[..size].iter().cloned())) - }); - - group.bench_function(BenchmarkId::new("hash", size), |b| { - b.iter(|| hasher.hash(bits[..size].iter().cloned())) - }); - - group.bench_function(BenchmarkId::new("commit", size), |b| { - b.iter(|| committer.commit(bits[..size].iter().cloned(), &r)) - }); - - group.bench_function(BenchmarkId::new("short-commit", size), |b| { - b.iter(|| committer.commit(bits[..size].iter().cloned(), &r)) - }); - } - } } #[cfg(unix)] diff --git a/rust-toolchain b/rust-toolchain new file mode 100644 index 0000000..0834888 --- /dev/null +++ b/rust-toolchain @@ -0,0 +1 @@ +1.72.0 diff --git a/src/lib.rs b/src/lib.rs index 420a8b7..507d566 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,10 +1,8 @@ //! This crate provides the poseidon gadget for use with `halo2_proofs`. //! This gadget has been stracted from zcash's halo2_gadgets: -//! https://github.com/zcash/halo2/tree/main/halo2_gadgets +//! #![cfg_attr(docsrs, feature(doc_cfg))] -// Temporary until we have more of the crate implemented. -#![allow(dead_code)] // Catch documentation errors caused by code changes. #![deny(rustdoc::broken_intra_doc_links)] #![deny(missing_debug_implementations)] diff --git a/src/poseidon.rs b/src/poseidon.rs index bfd78f3..400e90f 100644 --- a/src/poseidon.rs +++ b/src/poseidon.rs @@ -288,7 +288,7 @@ impl< .enumerate() { self.sponge - .absorb(layouter.namespace(|| format!("absorb_{}", i)), value)?; + .absorb(layouter.namespace(|| format!("absorb_{i}")), value)?; } self.sponge .finish_absorbing(layouter.namespace(|| "finish absorbing"))? diff --git a/src/poseidon/pow5.rs b/src/poseidon/pow5.rs index 51c1f05..715ca45 100644 --- a/src/poseidon/pow5.rs +++ b/src/poseidon/pow5.rs @@ -32,6 +32,7 @@ pub struct Pow5Config { alpha: [u64; 4], round_constants: Vec<[F; WIDTH]>, m_reg: Mds, + #[allow(dead_code)] m_inv: Mds, } @@ -286,7 +287,7 @@ impl< let mut state = Vec::with_capacity(WIDTH); let mut load_state_word = |i: usize, value: F| -> Result<_, Error> { let var = region.assign_advice_from_constant( - || format!("state_{}", i), + || format!("state_{i}"), config.state[i], 0, value, @@ -325,7 +326,7 @@ impl< initial_state[i] .0 .copy_advice( - || format!("load state_{}", i), + || format!("load state_{i}"), &mut region, config.state[i], 0, @@ -341,7 +342,7 @@ impl< let constraint_var = match input.0[i].clone() { Some(PaddedWord::Message(word)) => word, Some(PaddedWord::Padding(padding_value)) => region.assign_fixed( - || format!("load pad_{}", i), + || format!("load pad_{i}"), config.rc_b[i], 1, || Value::known(padding_value), @@ -350,7 +351,7 @@ impl< }; constraint_var .copy_advice( - || format!("load input_{}", i), + || format!("load input_{i}"), &mut region, config.state[i], 1, @@ -369,12 +370,7 @@ impl< // The capacity element is never altered by the input. .unwrap_or_else(|| Value::known(F::ZERO)); region - .assign_advice( - || format!("load output_{}", i), - config.state[i], - 2, - || value, - ) + .assign_advice(|| format!("load output_{i}"), config.state[i], 2, || value) .map(StateWord) }; @@ -474,7 +470,7 @@ impl Pow5State { }); region.assign_advice( - || format!("round_{} partial_sbox", round), + || format!("round_{round} partial_sbox"), config.partial_sbox, offset, || r.as_ref().map(|r| r[0]), @@ -536,7 +532,7 @@ impl Pow5State { let load_state_word = |i: usize| { initial_state[i] .0 - .copy_advice(|| format!("load state_{}", i), region, config.state[i], 0) + .copy_advice(|| format!("load state_{i}"), region, config.state[i], 0) .map(StateWord) }; @@ -558,7 +554,7 @@ impl Pow5State { // Load the round constants. let mut load_round_constant = |i: usize| { region.assign_fixed( - || format!("round_{} rc_{}", round, i), + || format!("round_{round} rc_{i}"), config.rc_a[i], offset, || Value::known(config.round_constants[round][i]), @@ -574,7 +570,7 @@ impl Pow5State { let next_state_word = |i: usize| { let value = next_state[i]; let var = region.assign_advice( - || format!("round_{} state_{}", next_round, i), + || format!("round_{next_round} state_{i}"), config.state[i], offset + 1, || value, diff --git a/src/poseidon/primitives.rs b/src/poseidon/primitives.rs index c456c87..cc1e123 100644 --- a/src/poseidon/primitives.rs +++ b/src/poseidon/primitives.rs @@ -310,7 +310,7 @@ impl Domain for Const type Padding = iter::Take>; fn name() -> String { - format!("ConstantLength<{}>", L) + format!("ConstantLength<{L}>") } fn initial_capacity_element() -> F {