simplify equal_cells

[mimoo]

/// Returns a new variable set to 1 if x1 is equal to x2, 0 otherwise.
fn equal_cells<B: Backend>(
    compiler: &mut CircuitWriter<B>,
    x1: &ConstOrCell<B::Field>,
    x2: &ConstOrCell<B::Field>,
    span: Span,
) -> Var<B::Field> {

    let zero = B::Field::zero();
    let one = B::Field::one();
    match (x1, x2) {
        // two constants
        (ConstOrCell::Const(x1), ConstOrCell::Const(x2)) => {
            let res = if x1 == x2 { one } else { B::Field::zero() };
            Var::new_constant(res, span)
        }
        (x1, x2) => {
            let x1 = match x1 {
                ConstOrCell::Const(cst) => compiler.backend.add_constant(
                    Some("encode the lhs constant of the equality check in the circuit"),
                    *cst,
                    span,
                ),
                ConstOrCell::Cell(cvar) => *cvar,
            };
            let x2 = match x2 {
                ConstOrCell::Const(cst) => compiler.backend.add_constant(
                    Some("encode the rhs constant of the equality check in the circuit"),
                    *cst,
                    span,
                ),
                ConstOrCell::Cell(cvar) => *cvar,
            };
            // compute the result
            let res = compiler.backend.new_internal_var(
                Value::Hint(Arc::new(move |backend, env| {
                    let x1 = backend.compute_var(env, x1)?;
                    let x2 = backend.compute_var(env, x2)?;
                    if x1 == x2 {
                        Ok(B::Field::one())
                    } else {
                        Ok(B::Field::zero())
                    }
                })),
                span,
            );

            // 1. diff = x2 - x1
            let neg_x1 = compiler.backend.neg(&x1, span);
            let diff = compiler.backend.add(&x2, &neg_x1, span);

^ in this code, it doesn't make sense to call add_constant. I think we should write something like this directly:

/// Returns a new variable set to 1 if x1 is equal to x2, 0 otherwise.
fn equal_cells<B: Backend>(
    compiler: &mut CircuitWriter<B>,
    x1: &ConstOrCell<B::Field>,
    x2: &ConstOrCell<B::Field>,
    span: Span,
) -> Var<B::Field> {

    let zero = B::Field::zero();
    let one = B::Field::one();

            // compute the result
            let res = compiler.backend.new_internal_var(
                Value::Hint(Arc::new(move |backend, env| {
                    let x1 = backend.compute_var(env, x1)?;
                    let x2 = backend.compute_var(env, x2)?;
                    if x1 == x2 {
                        Ok(B::Field::one())
                    } else {
                        Ok(B::Field::zero())
                    }
                })),
                span,
            );

            // 1. diff = x2 - x1
            let diff = compiler.sub(&x2, &x1, span);

[lognorman20]

Is noname shifting away from using hints and Arc? I'm having trouble compiling the code provided in the snippet as well as reproducing this code due to the following lifetime error (which could be my misunderstanding):

fn equal_cells<B: Backend>(
    compiler: &mut CircuitWriter<B>,
    x1: &ConstOrCell<B::Field, B::Var>,
    x2: &ConstOrCell<B::Field, B::Var>,
    span: Span,
) -> Var<B::Field, B::Var> where {
    let zero = B::Field::zero();
    let one = B::Field::one();

    match (x1, x2) {
        // two constants
        (ConstOrCell::Const(x1), ConstOrCell::Const(x2)) => {
            let res = if x1 == x2 { one } else { zero };
            Var::new_constant(res, span)
        }

        (x1, x2) => {
            let x1 = match x1 {
                ConstOrCell::Const(cst) => {
                    let cst = Value::Constant(*cst);
                    compiler.backend.new_internal_var(cst, span)
                },
                ConstOrCell::Cell(cvar) => cvar.clone(),
            };

            let x2 = match x2 {
                ConstOrCell::Const(cst) => {
                    let cst = Value::Constant(*cst);
                    compiler.backend.new_internal_var(cst, span)
                },
                ConstOrCell::Cell(cvar) => cvar.clone(),
            };

            let res = compiler.backend.new_internal_var(
                Value::Hint(Arc::new(move |backend, env| {
                    let x1 = backend.compute_var(env, &x1)?;
                    let x2 = backend.compute_var(env, &x2)?;
                    if x1 == x2 {
                        Ok(B::Field::one())
                    } else {
                        Ok(B::Field::zero())
                    }
                })),
                span,
            );

            Var::new_var(res, span)
        }
    }
}

error[E0310]: the associated type `<B as Backend>::Var` may not live long enough
   --> src/constraints/field.rs:171:29
    |
171 |                   Value::Hint(Arc::new(move |backend, env| {
    |  _____________________________^
172 | |                     let x1 = backend.compute_var(env, &x1)?;
173 | |                     let x2 = backend.compute_var(env, &x2)?;
174 | |                     if x1 == x2 {
...   |
178 | |                     }
179 | |                 })),
    | |                  ^
    | |                  |
    | |__________________the associated type `<B as Backend>::Var` must be valid for the static lifetime...
    |                    ...so that the type `<B as Backend>::Var` will meet its required lifetime bounds
    |
help: consider adding an explicit lifetime bound
    |
142 | ) -> Var<B::Field, B::Var> where <B as Backend>::Var: 'static {

[mimoo]

we are moving away from hints yeah, see https://github.com/zksecurity/noname/issues/38

[katat]

I think the motivation of using the add_constant in the following code was to avoid duplicating the same logic for different backend interfaces for constant value or backend var.

https://github.com/zksecurity/noname/blob/77e41565a5f431eced8acd2f1299bc2ac90a049f/src/constraints/field.rs#L170-L192

Because the Backend::add_constant can wrap the constant as a backend var, it only needs to consider passing it to the var version of the backend function, such as Backend::mul instead of Backend::scale.

Maybe it makes sense to encapsulate the handling of ConstOrVar in the backend functions, meaning the backend functions accept the ConstOrVar as arguments instead of either constant value or backend var?

If so, we can remove the Backend::add_constant while the "frontend" code can be simplified as it doesn't need to handle the match for const or var.

[mimoo]

I think either the backend always accepts constants or vars in its functions OR it exposes different functions for vars and constants. I believe we have done the latter so far, but the former might be much cleaner indeed. WDYT? This could be a nice first task for a new contributor