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

chore: fix all lints #100

Closed
wants to merge 11 commits into from
Closed
5 changes: 1 addition & 4 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,4 @@ jobs:
run: cargo fmt -- --check

# - name: Lint (clippy)
# uses: actions-rs/cargo@v1
# with:
# command: clippy
# args: --all-features -- -D warnings
# run: cargo clippy --all-features -- -D warnings
1 change: 0 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 Down
29 changes: 16 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 @@ -104,7 +104,9 @@ pub struct KimchiVesta {
impl Witness {
/// kimchi uses a transposed witness
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 @@ -124,7 +126,7 @@ impl Witness {

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(PrettyField::pretty).join(" | ");
println!("{row} - {values}");
}
}
Expand Down Expand Up @@ -191,7 +193,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 +294,7 @@ impl Backend for KimchiVesta {

let zero = VestaField::zero();

let _ = &self.add_generic_gate(
self.add_generic_gate(
label.unwrap_or("hardcode a constant"),
vec![Some(var)],
vec![VestaField::one(), zero, zero, zero, value.neg()],
Expand Down Expand Up @@ -322,7 +324,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 +351,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 +491,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 +519,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
4 changes: 2 additions & 2 deletions src/backends/kimchi/prover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,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
9 changes: 4 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 Down Expand Up @@ -57,8 +56,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 +121,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
59 changes: 33 additions & 26 deletions src/backends/r1cs/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// TODO: There is a bunch of places where there are unused vars.
// Remove this lint allowance when fixed.
#![allow(unused_variables)]
Copy link
Contributor

Choose a reason for hiding this comment

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

imo this is dangerous, as these unused variables might be bugs and we should address that instead of silencing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #101


pub mod builtin;
pub mod snarkjs;

Expand All @@ -10,8 +14,6 @@ use serde::{Deserialize, Serialize};

use crate::constants::Span;
use crate::error::{Error, ErrorKind, Result};
use crate::helpers::PrettyField;
use crate::parser::FunctionDef;
use crate::{circuit_writer::DebugInfo, var::Value};

use super::{Backend, BackendField, BackendVar};
Expand Down Expand Up @@ -39,10 +41,10 @@ impl CellVar {
impl<F: BackendField> BackendVar for LinearCombination<F> {}

/// Linear combination of variables and constants.
/// For example, the linear combination is represented as a * f_a + b * f_b + f_c
/// f_a and f_b are the coefficients of a and b respectively.
/// a and b are represented by CellVar.
/// The constant f_c is represented by the constant field, will always multiply with the variable at index 0 which is always 1.
/// For example, the linear combination is represented as a * `f_a` + b * `f_b` + `f_c`
/// `f_a` and `f_b` are the coefficients of a and b respectively.
/// a and b are represented by `CellVar`.
/// The constant `f_c` is represented by the constant field, will always multiply with the variable at index 0 which is always 1.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct LinearCombination<F>
where
Expand All @@ -57,7 +59,7 @@ impl<F> LinearCombination<F>
where
F: BackendField,
{
/// Convert to a CellVar.
/// Convert to a `CellVar`.
/// It should
/// - be used when the linear combination is a single variable.
/// - panic if the linear combination is not a single variable or has a non-zero constant.
Expand Down Expand Up @@ -133,8 +135,8 @@ where
}
}

/// Enforces a constraint for the multiplication of two CellVars.
/// The constraint reduces the multiplication to a new CellVar variable,
/// Enforces a constraint for the multiplication of two `CellVars`.
/// The constraint reduces the multiplication to a new `CellVar` variable,
/// which represents: self * other = res.
fn mul(&self, cs: &mut R1CS<F>, other: &Self, span: Span) -> Self {
let res = cs.new_internal_var(Value::Mul(self.clone(), other.clone()), span);
Expand All @@ -143,12 +145,12 @@ where
res
}

/// Enforces a constraint for the equality of two CellVars.
/// Enforces a constraint for the equality of two `CellVars`.
/// It needs to constraint: self * 1 = other.
fn assert_eq(&self, cs: &mut R1CS<F>, other: &Self, span: Span) {
let one_cvar = LinearCombination::one(span);

cs.enforce_constraint(self, &one_cvar, other, span)
cs.enforce_constraint(self, &one_cvar, other, span);
}
}

Expand Down Expand Up @@ -188,7 +190,7 @@ where
}
}

/// R1CS backend with bls12_381 field.
/// R1CS backend with `bls12_381` field.
#[derive(Clone)]
pub struct R1CS<F>
where
Expand All @@ -207,6 +209,15 @@ where
finalized: bool,
}

impl<F> Default for R1CS<F>
where
F: BackendField,
{
fn default() -> Self {
Self::new()
}
}

impl<F> R1CS<F>
where
F: BackendField,
Expand Down Expand Up @@ -271,7 +282,7 @@ where
}

#[derive(Debug)]
/// An intermediate struct for SnarkjsExporter to reorder the witness and convert to snarkjs format.
/// An intermediate struct for `SnarkjsExporter` to reorder the witness and convert to snarkjs format.
pub struct GeneratedWitness<F>
where
F: BackendField,
Expand All @@ -297,8 +308,8 @@ where
self.new_internal_var(Value::Constant(F::one()), Span::default());
}

/// Create a new CellVar and record in witness_vector vector.
/// The underlying type of CellVar is always WitnessVar.
/// Create a new `CellVar` and record in `witness_vector` vector.
/// The underlying type of `CellVar` is always `WitnessVar`.
fn new_internal_var(
&mut self,
val: crate::var::Value<Self>,
Expand Down Expand Up @@ -329,7 +340,7 @@ where

/// Final checks for generating the circuit.
/// todo: we might need to rethink about this interface
/// - we might just need the returned_cells argument, as the backend can record the public outputs itself?
/// - we might just need the `returned_cells` argument, as the backend can record the public outputs itself?
fn finalize_circuit(
&mut self,
public_output: Option<crate::var::Var<Self::Field, Self::Var>>,
Expand Down Expand Up @@ -513,7 +524,7 @@ where
let (a, b, c) = (&fmt_lcs[0], &fmt_lcs[1], &fmt_lcs[2]);

// format an entire constraint
res.push_str(&format!("{} == ({}) * ({})\n", c, a, b));
res.push_str(&format!("{c} == ({a}) * ({b})\n"));

if debug {
// link the constraint to the source code
Expand Down Expand Up @@ -561,7 +572,7 @@ where
fn assert_eq_const(&mut self, x: &LinearCombination<F>, cst: F, span: Span) {
let c = LinearCombination::from_const(cst, span);

x.assert_eq(self, &c, span)
x.assert_eq(self, &c, span);
}

fn assert_eq_var(
Expand All @@ -570,7 +581,7 @@ where
rhs: &LinearCombination<F>,
span: Span,
) {
lhs.assert_eq(self, rhs, span)
lhs.assert_eq(self, rhs, span);
}

/// Adds the public input cell vars.
Expand Down Expand Up @@ -600,13 +611,9 @@ where

#[cfg(test)]
mod tests {
use crate::{
backends::{
r1cs::{R1csBls12381Field, R1CS},
Backend, BackendKind,
},
lexer::Token,
parser::{types::FnSig, FunctionDef, ParserCtx},
use crate::backends::{
r1cs::{R1csBls12381Field, R1CS},
Backend, BackendKind,
};
use ark_ff::One;
use rstest::rstest;
Expand Down
26 changes: 14 additions & 12 deletions src/backends/r1cs/snarkjs.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::backends::BackendField;
use constraint_writers::r1cs_writer::{ConstraintSection, HeaderData, R1CSWriter};
use miette::{miette, Diagnostic};
use miette::Diagnostic;
use thiserror::Error;

use std::collections::HashMap;
Expand Down Expand Up @@ -43,10 +43,11 @@ impl SnarkjsLinearCombination {
let mut terms = self.terms.clone();

// add the constant term with var indexed at 0
if terms.insert(0, self.constant.clone()).is_some() {
// sanity check
panic!("The first var should be preserved for constant term");
}
// sanity check
assert!(
terms.insert(0, self.constant.clone()).is_none(),
"The first var should be preserved for constant term"
);

terms
}
Expand Down Expand Up @@ -79,7 +80,7 @@ where
}

/// Restructure the linear combination to align with the snarkjs format.
/// - convert the factors to BigInt.
/// - convert the factors to `BigInt`.
fn restructure_lc(&self, lc: &LinearCombination<F>) -> SnarkjsLinearCombination {
let terms = lc
.terms
Expand Down Expand Up @@ -226,9 +227,10 @@ impl WitnessWriter {

// Write the file type (magic string) as bytes
let file_type_bytes = file_type.as_bytes();
if file_type_bytes.len() != 4 {
panic!("File type must be 4 characters long");
}
assert!(
file_type_bytes.len() == 4,
"File type must be 4 characters long"
);
writer.write_all(file_type_bytes)?;

// Write the version as a 32-bit unsigned integer in little endian
Expand Down Expand Up @@ -302,15 +304,15 @@ impl WitnessWriter {
// Start writing the second section
self.start_write_section(2)?;

/// Write the witness values to the file
/// Each witness value occupies the same number of bytes as the prime field
// Write the witness values to the file
// Each witness value occupies the same number of bytes as the prime field
for value in witness {
self.write_big_int(value.clone(), field_n_bytes)?;
}
self.end_write_section()
}

/// Write a BigInt to the file
/// Write a `BigInt` to the file
fn write_big_int(&mut self, value: BigInt, size: usize) -> Result<(), io::Error> {
let bytes = value.to_bytes_le().1;

Expand Down
Loading
Loading