Skip to content

Commit

Permalink
chore: fix all lints
Browse files Browse the repository at this point in the history
There's a lot of changes - I'll briefly summarize what I did:

I ran, as much as possible, the following:

```rust
cargo fix --all --allow-dirty
cargo fmt
cargo clippy --fix --all --allow--dirty
```

Some parts, I had to manually fix, or was out of scope of the current PR,
so I added lint allowances + TODOs instead. An example of such 'out of scope'
fixes include the `unused_must_use`s in `r1cs/snarkjs.rs` - these would require
additional work on error handling which should probably belong in its own PR.
For these cases, I prefer lint allowances since they allow us to easily find the spots
where we want to fix later on.

Note that some of the lint allowances seem sane, so I didn't add TODOs for those,
but only for those I felt would improve code quality if removed.
  • Loading branch information
bing committed Jun 5, 2024
1 parent 0607a00 commit b522b33
Show file tree
Hide file tree
Showing 43 changed files with 405 additions and 273 deletions.
4 changes: 3 additions & 1 deletion src/backends/kimchi/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
//!

use std::collections::{HashMap, HashSet};
use std::fmt::Write;
use std::hash::Hash;

use crate::helpers::PrettyField;
Expand All @@ -38,6 +37,7 @@ pub fn extract_vars_from_coeffs(vars: &mut OrderedHashSet<VestaField>, coeffs: &
}
}

#[must_use]
pub fn parse_coeffs(vars: &OrderedHashSet<VestaField>, coeffs: &[VestaField]) -> Vec<String> {
let mut coeffs: Vec<_> = coeffs
.iter()
Expand Down Expand Up @@ -90,10 +90,12 @@ where
self.map[value]
}

#[must_use]
pub fn len(&self) -> usize {
self.ordered.len()
}

#[must_use]
pub fn is_empty(&self) -> bool {
self.ordered.is_empty()
}
Expand Down
2 changes: 1 addition & 1 deletion src/backends/kimchi/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ pub fn poseidon(
let vars = final_row
.iter()
.flatten()
.cloned()
.copied()
.map(ConstOrCell::Cell)
.collect();

Expand Down
37 changes: 24 additions & 13 deletions src/backends/kimchi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ pub struct Witness(Vec<[VestaField; NUM_REGISTERS]>);
pub struct GeneratedWitness {
/// contains all the witness values
pub all_witness: Witness,
/// contains the public inputs, which are also part of the all_witness
/// contains the public inputs, which are also part of the `all_witness`
pub full_public_inputs: Vec<VestaField>,
/// contains the public outputs, which are also part of the all_witness
/// contains the public outputs, which are also part of the `all_witness`
pub public_outputs: Vec<VestaField>,
}

Expand Down Expand Up @@ -103,8 +103,11 @@ pub struct KimchiVesta {

impl Witness {
/// kimchi uses a transposed witness
#[must_use]
pub fn to_kimchi_witness(&self) -> [Vec<VestaField>; NUM_REGISTERS] {
let transposed = vec![Vec::with_capacity(self.0.len()); NUM_REGISTERS];
let transposed = (0..NUM_REGISTERS)
.map(|_| Vec::with_capacity(self.0.len()))
.collect::<Vec<_>>();
let mut transposed: [_; NUM_REGISTERS] = transposed.try_into().unwrap();
for row in &self.0 {
for (col, field) in row.iter().enumerate() {
Expand All @@ -114,23 +117,29 @@ impl Witness {
transposed
}

#[must_use]
pub fn len(&self) -> usize {
self.0.len()
}

#[must_use]
pub fn is_empty(&self) -> bool {
self.0.is_empty()
}

pub fn debug(&self) {
for (row, values) in self.0.iter().enumerate() {
let values = values.iter().map(|v| v.pretty()).join(" | ");
let values = values
.iter()
.map(super::super::helpers::PrettyField::pretty)
.join(" | ");
println!("{row} - {values}");
}
}
}

impl KimchiVesta {
#[must_use]
pub fn new(double_generic_gate_optimization: bool) -> Self {
Self {
next_variable: 0,
Expand Down Expand Up @@ -191,7 +200,7 @@ impl KimchiVesta {
.entry(var.index)
.and_modify(|w| match w {
Wiring::NotWired(old_cell) => {
*w = Wiring::Wired(vec![old_cell.clone(), annotated_cell.clone()])
*w = Wiring::Wired(vec![old_cell.clone(), annotated_cell.clone()]);
}
Wiring::Wired(ref mut cells) => {
cells.push(annotated_cell.clone());
Expand Down Expand Up @@ -292,7 +301,7 @@ impl Backend for KimchiVesta {

let zero = VestaField::zero();

let _ = &self.add_generic_gate(
let () = &self.add_generic_gate(
label.unwrap_or("hardcode a constant"),
vec![Some(var)],
vec![VestaField::one(), zero, zero, zero, value.neg()],
Expand All @@ -302,6 +311,7 @@ impl Backend for KimchiVesta {
var
}

#[allow(unused_variables)]
fn finalize_circuit(
&mut self,
public_output: Option<Var<Self::Field, Self::Var>>,
Expand All @@ -322,7 +332,7 @@ impl Backend for KimchiVesta {

// for sanity check, we make sure that every cellvar created has ended up in a gate
let mut written_vars = HashSet::new();
for row in self.witness_table.iter() {
for row in &self.witness_table {
row.iter().flatten().for_each(|cvar| {
written_vars.insert(cvar.index);
});
Expand All @@ -349,9 +359,10 @@ impl Backend for KimchiVesta {
}

// kimchi hack
if self.gates.len() <= 2 {
panic!("the circuit is either too small or does not constrain anything (TODO: better error)");
}
assert!(
self.gates.len() > 2,
"the circuit is either too small or does not constrain anything (TODO: better error)"
);

// store the return value in the public input that was created for that ^
if let Some(public_output) = public_output {
Expand Down Expand Up @@ -488,15 +499,15 @@ impl Backend for KimchiVesta {
}

fn generate_asm(&self, sources: &Sources, debug: bool) -> String {
let mut res = "".to_string();
let mut res = String::new();

// version
res.push_str(&crate::utils::noname_version());

// vars
let mut vars: OrderedHashSet<VestaField> = OrderedHashSet::default();

for Gate { coeffs, .. } in self.gates.iter() {
for Gate { coeffs, .. } in &self.gates {
extract_vars_from_coeffs(&mut vars, coeffs);
}

Expand All @@ -516,7 +527,7 @@ impl Backend for KimchiVesta {
for (row, (Gate { typ, coeffs }, debug_info)) in
self.gates.iter().zip(&self.debug_info).enumerate()
{
println!("gate {:?}", row);
println!("gate {row:?}");
// gate #
if debug {
writeln!(res, "╭{s}", s = "─".repeat(80)).unwrap();
Expand Down
5 changes: 3 additions & 2 deletions src/backends/kimchi/prover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ pub struct VerifierIndex {
// Setup
//

#[allow(clippy::type_complexity)]
impl KimchiVesta {
pub fn compile_to_indexes(
&self,
Expand Down Expand Up @@ -247,12 +248,12 @@ mod tests {

#[test]
fn test_public_output_constraint() -> miette::Result<()> {
let code = r#"fn main(pub public_input: Field, private_input: Field) -> Field {
let code = r"fn main(pub public_input: Field, private_input: Field) -> Field {
let xx = private_input + public_input;
assert_eq(xx, 2);
let yy = xx + 6;
return yy;
}"#;
}";

let mut sources = Sources::new();
let mut tast = TypeChecker::new();
Expand Down
14 changes: 9 additions & 5 deletions src/backends/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{fmt::Debug, hash::Hash, str::FromStr};
use std::{fmt::Debug, str::FromStr};

use ark_ff::{Field, Zero};
use num_bigint::BigUint;
Expand All @@ -9,7 +9,6 @@ use crate::{
error::{Error, ErrorKind, Result},
helpers::PrettyField,
imports::FnHandle,
parser::FunctionDef,
var::{Value, Var},
witness::WitnessEnv,
};
Expand All @@ -32,21 +31,26 @@ pub trait BackendField:
/// It is intended to make it opaque to the frondend.
pub trait BackendVar: Clone + Debug + PartialEq + Eq {}

// TODO: Fix this by `Box`ing `KimchiVesta`.
#[allow(clippy::large_enum_variant)]
pub enum BackendKind {
KimchiVesta(KimchiVesta),
R1csBls12_381(R1CS<R1csBls12381Field>),
R1csBn254(R1CS<R1csBn254Field>),
}

impl BackendKind {
#[must_use]
pub fn new_kimchi_vesta(use_double_generic: bool) -> Self {
Self::KimchiVesta(KimchiVesta::new(use_double_generic))
}

#[must_use]
pub fn new_r1cs_bls12_381() -> Self {
Self::R1csBls12_381(R1CS::new())
}

#[must_use]
pub fn new_r1cs_bn254() -> Self {
Self::R1csBn254(R1CS::new())
}
Expand All @@ -57,8 +61,8 @@ pub trait Backend: Clone {
/// The circuit field / scalar field that the circuit is written on.
type Field: BackendField;

/// The CellVar type for the backend.
/// Different backend is allowed to have different CellVar types.
/// The `CellVar` type for the backend.
/// Different backend is allowed to have different `CellVar` types.
type Var: BackendVar;

/// The generated witness type for the backend. Each backend may define its own witness format to be generated.
Expand Down Expand Up @@ -122,7 +126,7 @@ pub trait Backend: Clone {
span: Span,
) -> Self::Var;

/// Backends should implement this function to load and compute the value of a CellVar.
/// Backends should implement this function to load and compute the value of a `CellVar`.
fn compute_var(
&self,
env: &mut WitnessEnv<Self::Field>,
Expand Down
Loading

0 comments on commit b522b33

Please sign in to comment.