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

Adding merkle proof verification for Chunk crate #41

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

tchataigner
Copy link
Contributor

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 an EqualityCircuit 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 into AllocatedNum to pass them as IO. Would be glad to have some feedback on this idea :)

@tchataigner tchataigner force-pushed the feature/chunk-merkle-example branch from 32d7b60 to f22c71b Compare February 26, 2024 16:00
@tchataigner tchataigner marked this pull request as ready for review February 26, 2024 17:45
}

// Reconstructs a hash from a list of field elements.
fn reconstruct_hash<F: PrimeField + PrimeFieldBits, CS: ConstraintSystem<F>>(
Copy link
Contributor

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>>(
Copy link
Contributor

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 lurk-lang/arecibo#324 #43 let's put a TODO on this to mention that this should be removed once any of those issues close.

@porcuquine porcuquine self-requested a review February 27, 2024 17:05
Comment on lines 148 to 163
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)
}
}
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines 144 to 155
fn num_circuits(&self) -> usize {
self.inner.num_fold_steps() + 1
}
Copy link
Contributor

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.

Copy link
Contributor

@porcuquine porcuquine left a 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.

@tchataigner tchataigner force-pushed the feature/chunk-merkle-example branch from 19ba9a9 to af0447a Compare February 29, 2024 12:14
@tchataigner
Copy link
Contributor Author

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.

Thanks for your inputs, I took some time to simplify the implementation of the crate while also trying to have better NonUniformCircuit and StepCircuit implementations based on your comments. I will iterate over the implementation again after tonight's discussion.

@tchataigner tchataigner requested a review from huitseeker March 14, 2024 14:21
@huitseeker huitseeker removed their request for review June 5, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants