-
Notifications
You must be signed in to change notification settings - Fork 8
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
Sumcheck circuit #28
base: main
Are you sure you want to change the base?
Sumcheck circuit #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much bellman so my review is not very useful I'm afraid... I left a few comments though!
src/gadgets/sumcheck.rs
Outdated
); | ||
eval = eval_aux; | ||
} | ||
let allocated_eval: AllocatedNum<Scalar> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the allocated_eval
from the loop above here? Or do you need to reallocate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The allocated_eval
from the loop before has a different value than the eval
once the loop ends. But we can use allocated_eval_aux
which has the desired value, although then the code gets a bit less clear.
Added this change in the next commit among other updates related to the comments.
{ | ||
let mut eval: Num<Scalar> = Num::<Scalar>::from(coeffs[0].clone()); | ||
let mut power = r.clone(); | ||
for (i, coeff) in coeffs.iter().enumerate().skip(1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions get quite complicated even though they are supposed to be doing simple things. This might just be how bellman is, or maybe you can define two helper methods here?
For example one could be multiply_and_add()
and the other would be multiply_with_r()
and both of them would return their aux
variable? This way you could simplify this func, and you could also separately test the helper functions as well?
assert_eq!(res.get_value().unwrap(), p.evaluate(&r)); | ||
|
||
let (inst, witness) = cs.r1cs_instance_and_witness(&shape, &ck).unwrap(); | ||
assert!(shape.is_sat(&ck, &inst, &witness).is_ok()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we have some basic negative tests here, to make sure that not everything is satisfied?
Can we make a shape that is obviously not satisfied and then check that it is indeed not satisfied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uni_evaluate
gadget does not check that two inputs satisfy the checks, it internally computes the evaluation of the univariate polynomial and outputs the result, while internally adding constraints for the operations of the evaluation.
But there is no 'wrong inputs' for that, what we can check is that the output of the gadget is the expected result of the evaluation (which is what the test does).
Is like a gadget that we can name "add" that outputs the result of a + b
, we can not input 'wrong' values that will not satisfy the shape, as it is just computing a+b and adding a constraint that ensures that the output is equal to a+b, but to test that gadget we can instanciate it and assert that the outputted value is the expected one.
src/gadgets/sumcheck.rs
Outdated
use crate::spartan::polynomial::MultilinearPolynomial; | ||
use crate::spartan::sumcheck::SumcheckProof; | ||
|
||
fn synthetize_uni_evaluate<Scalar, CS>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the role of this function? Are the commented stuff needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role is the same as in other synthesize_
methods in the tests of other gadgets such as
https://github.com/privacy-scaling-explorations/Nova/tree/b53229d07c50cd135006b319995bd29799cfc2e7/src/gadgets/ecc.rs#L1020
which is to prepare the inputs and run the gadget, which is later used in the test first to get the r1cs shape, and after that to run the gadget with real values, getting it's output.
The commented stuff will not be needed, leaving it for the moment meanwhile the PR is in draft mode, in order to check stuff while the PR is in progress.
Note about typo: in the sumcheck.rs code the word synthetize
should be synthesize
as in the rest of methods with that word in the other Nova files, I'll update this in the next commit.
103b1d6
to
8183971
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! Would it be possible to include some reference comments in the code to what you are basing the the algorithms etc on? It'd make it easier to check for correctness and to better understand the flow. E.g. if it is HyperNova paper, or Thaler book, or anything else used as a reference.
8183971
to
b53229d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
src/gadgets/sumcheck.rs
Outdated
); | ||
eval = eval_aux; | ||
} | ||
let allocated_eval: AllocatedNum<Scalar> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you copy constrain the value at least? So that there's no way to cheat values here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by 'copy constrain'? (in the context of R1CS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that we need a way to ensure that the inner loop allocated value is the same as the one created later.
Otherwise, there's an equality not being checked.
let allocated_eval: AllocatedNum<Scalar> = | ||
AllocatedNum::<Scalar>::alloc(&mut cs, || Ok(eval.get_value().unwrap()))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Can't you copy constrain this? Otherwise it looks like there's nothing that constraints this.
src/lib.rs
Outdated
@@ -5,7 +5,7 @@ | |||
future_incompatible, | |||
nonstandard_style, | |||
rust_2018_idioms, | |||
missing_docs | |||
// missing_docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd re-enable this and add all the required comments before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
a6fe8d7
to
49f38d9
Compare
spartan::SumCheckProof's didn't had a generic prover, added its implementation to be used for the sumcheck-verifier-circuit tests and probably for the HyperNova's Multifolding step 3.
Add test to SumCheck verify gadget, for the SumCheck example of g(X_0, X_1, X_2) = 2 X_0^3 + X_0 X_2 + X_1 X_2.
49f38d9
to
4b5a606
Compare
@@ -0,0 +1,336 @@ | |||
#![allow(unused)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some basic documentation to this gadget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, agree. @CPerezz also mentioned it (#28 (comment)) and is planned once the impl is more solid (and before removing it's draft status).
Hi @arnaucube Notice you implemente sumcheck verify circuit in r1cs-bellperson, may I know your following up plan, such like do you plan to upstream this feature to Nova repo or not and when? |
Hi @hero78119 , so this was a line of work started with the objective to end up having the HyperNova multifolding verifier in-circuit, but recently we stopped that line of work. |
…rivacy-scaling-explorations#28) - Modified `SuperNovaAugmentedCircuitParams` to be passed by reference in `test_recursive_circuit_with` function and associated instances in `test.rs`. - Updated `SuperNovaAugmentedCircuit::new` method to match changes in the `test_recursive_circuit_with` function. - Revamped arguments handling in `synthesize_non_base_case` and `synthesize` functions in `circuit.rs`, changing from owned types to references to reduce unnecessary cloning. - Altered function body to accommodate the changes for referenced values in `circuit.rs`.
Add SumCheck verification r1cs-bellperson gadget.
This PR:
To do after this PR:
once the transcript gadget is ready, we can then compare the number of constraints with the arkworks sumcheck-verify gadget number of constraints.
resolves #25