-
Notifications
You must be signed in to change notification settings - Fork 13
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
Adding merkle proof verification for Chunk crate #41
base: main
Are you sure you want to change the base?
Adding merkle proof verification for Chunk crate #41
Conversation
32d7b60
to
f22c71b
Compare
} | ||
|
||
// Reconstructs a hash from a list of field elements. | ||
fn reconstruct_hash<F: PrimeField + PrimeFieldBits, CS: ConstraintSystem<F>>( |
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.
PrimeField
should no longer be necessary: PrimeFieldBits
implies it.
} | ||
|
||
/// If condition return a otherwise b | ||
pub fn conditionally_select_vec<F: PrimeField, CS: ConstraintSystem<F>>( |
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.
This is from Arecibo, right? Considering argumentcomputer/arecibo#324 #43 let's put a TODO on this to mention that this should be removed once any of those issues close.
fn primary_circuit(&self, circuit_index: usize) -> Self::C1 { | ||
if circuit_index == 2 { | ||
Self::C1::CheckEquality(EqualityCircuit::new()) | ||
} else { | ||
if let Some(fold_step) = self.inner.circuits().get(circuit_index) { | ||
return Self::C1::IterStep(FoldStepWrapper::new(fold_step.clone())); | ||
} | ||
panic!("No circuit found for index {}", circuit_index) | ||
} | ||
} |
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.
This implementation reveals that primary_circuit
is either wrongly implemented locally here or (as I think I observe from looking around) being misused altogether in this ChunkStepCircuit
abstraction. Since I haven't had time yet to fully probe this, I will hedge and say that it's remotely conceivable that this is correct but just written in an extremely convoluted way. If that turns out to be the case, I will recommend simplifying.
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.
Part of the confusion here flows from the wrong interpretation of num_circuits
above. That method should return the number of alternative circuits SuperNova must manage when folding. It does not indicate the number of folding steps or anything related to the total size of the computation. It relates to the shape of the proof, which is insensitive to details of the actual computation trace that will be proved.
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.
You are 100% right, I got confused on the meaning of the API for Supernova
. I tried to refactor the examples based on what you said.
fn num_circuits(&self) -> usize { | ||
self.inner.num_fold_steps() + 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.
This seems quite wrong, but maybe I misunderstand the purpose of the chunk gadget. This seems to imply that a computation that is folded 100 times will have 101 distinct circuit shapes. If that were true there would be little benefit in folding or NIVC. You could just inline all the circuits into one giant flat circuit and use that. I very much suspect this is not what you intend.
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.
Let's discuss all this. I made a few comments which should be enough to explain generally why this looks wrong to me. It might be easier to combine general discussion of SuperNova with design around Chunk than for me to try to decipher the latter to review in a more fine-grained way.
19ba9a9
to
af0447a
Compare
Thanks for your inputs, I took some time to simplify the implementation of the crate while also trying to have better |
Goal
The goal of this PR is to implement an example of a usage of the Chunk gadget in the case of a merkle proof verification using Supernova.
Current progress
The overall architecture of the circuit and the Supernova pipeline is implemented.
The circuit is divided in two main parts, one for the chunk part (multiple
StepCircuit
generated based on the chunk parameters) and anEqualityCircuit
that checks that we computed a proper root at the end.Chunk API evolution ideas
For now I could implement the example without evolving the chunk API. An improvement the we could think of is allowing user to set any type as an input of a
FoldStep
as long as we have a way to convert them back and for intoAllocatedNum
to pass them as IO. Would be glad to have some feedback on this idea :)