Refactor for basic constraints #48
Conversation
src/backends/mod.rs
Outdated
| lhs: &ConstOrCell<Self::Field>, | ||
| rhs: &ConstOrCell<Self::Field>, | ||
| span: Span, | ||
| ) -> ConstOrCell<Self::Field>; |
There was a problem hiding this comment.
I would call these neg, add, and mul as they might not create a constraint eventually.
There was a problem hiding this comment.
also as I hint later, I think they should not receive ConstOrCell but CellVar instead
There was a problem hiding this comment.
It now uses CellVar at the backend layer
src/constraints/boolean.rs
Outdated
| ), | ||
| ConstOrCell::Cell(_) => { | ||
| // x * (x - 1) | ||
| let x_1 = field::sub(compiler, xx, &ConstOrCell::Const(one.neg()), span); |
src/constraints/field.rs
Outdated
| ); | ||
| let res_mul_diff = mul(compiler, &ConstOrCell::Cell(res), &diff, span)[0]; | ||
| // ensure that res * diff = 0 | ||
| sub(compiler, &res_mul_diff, &ConstOrCell::Const(zero), span); |
There was a problem hiding this comment.
this is a no-op, instead what we want to do here is assert_eq(res_mul_diff, zero), where assert_eq is also exposed by the backend
There was a problem hiding this comment.
it is now enforced by assert_eq
src/constraints/field.rs
Outdated
| span, | ||
| ); | ||
| let diff_inv_mul_diff = mul(compiler, &ConstOrCell::Cell(diff_inv), &diff, span)[0]; | ||
| sub(compiler, &diff_inv_mul_diff, &one_minus_res, span); |
There was a problem hiding this comment.
same here, either assert_eq(diff_inv_mul_diff, one_minus_res) or assert that the result of sub is equal to zero.
BTW we also should add #[must_use] to these functions (sub, neg, etc.) as well as trait functions (the one you added to do sub, neg, mul, etc.) so that the compiler yells at us if we don't use a var or a cvar from an operation
|
|
||
| match (lhs, rhs) { | ||
| // 2 constants | ||
| (ConstOrCell::Const(lhs), ConstOrCell::Const(rhs)) => Var::new_constant(*lhs + *rhs, span), |
There was a problem hiding this comment.
so my feeling is that it would be more elegant to leave the code handling constants here. Perhaps this means that in the backend we would also have: add_const(const: F, cvar: CellVar<F>) and scale(coeff: F, cvar: CellVar<F>)
this way the handling of pure constants is not done in the backend
There was a problem hiding this comment.
this one should be fixed
katat
force-pushed
the
refactor/basic-constraints
branch
from
0c45057 to
e4cd10f
Compare
| @@ -133,58 +136,11 @@ impl KimchiVesta { | |||
| pending_generic_gate: None, | |||
| debug_info: vec![], | |||
| finalized: false, | |||
| public_input_size: 0, | |||
There was a problem hiding this comment.
would it make sense to pass that as argument in new()?
There was a problem hiding this comment.
actually it doesn't make sense, rereading things, as it gets calculated when we call add_public_input and add_public_output
There was a problem hiding this comment.
yeah, I was wondering this fact also
| @@ -546,4 +551,159 @@ impl Backend for KimchiVesta { | |||
|
|
|||
| res | |||
| } | |||
|
|
|||
| fn constraint_neg(&mut self, var: &CellVar, span: Span) -> CellVar { | |||
There was a problem hiding this comment.
remove constraint from the name (so just neg) as it wrongly imply that we will create a constraint here (same for others)
| fn constraint_eq_const(&mut self, var: &CellVar, cst: Self::Field, span: Span); | ||
|
|
||
| /// add a constraint to assert a var equals another var | ||
| fn constraint_eq_var(&mut self, lhs: &CellVar, rhs: &CellVar, span: Span); |
There was a problem hiding this comment.
none of these functions have to add a constraint, so constraint should be removed from the name and the doc
neg/add/mul/assert_eq