-
Notifications
You must be signed in to change notification settings - Fork 131
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
feat: optionally unblinded advice columns #220
feat: optionally unblinded advice columns #220
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.
Overall LGTM, thanks for adding the feature. Some parts would need to be fixed.
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 really like the feature. But so far I think that this PR is just an "introduction" to it.
-
I think that (as @han0110 proposed in one of the suggestions) we should have this as a feature. This means that either we just add the feature for the check or for all the structs etc...
Any suggestions on this @han0110 ?? -
I'd like to see an example of this inside an
examples
folder or similar. Where the user of the lib can see how to use that. -
It's also important to add docs in the readme about this feature and what is it intended to be used for.
halo2_proofs/src/plonk/circuit.rs
Outdated
/// Allocate a new advice column at `FirstPhase` | ||
pub fn advice_column(&mut self) -> Column<Advice> { | ||
self.advice_column_in(FirstPhase) | ||
} | ||
|
||
/// Allocate a new advice column in given phase |
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.
Could you extend a bit more the docs to actually mention what does it imply to have this type of column and what is designed to be useful for?
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.
have added 2 extra sentences to provide a bit more context. We have some longer form description of the utility of this in two articles:
(creds also due to @han0110 for helping sanity check these)
Lmk if you think we should link to these explicitly in the docs or if we should instead incorporate fragments of them
Co-authored-by: Han <[email protected]>
Co-authored-by: Han <[email protected]>
Co-authored-by: Han <[email protected]>
Co-authored-by: Han <[email protected]>
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.
All LGTM now!
No preference to have a feature flag for this or not, since it doesn't break existing circuits .
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.
@alexander-camuto thanks for addressing the PR comments.
I just would appreciate if you could add back the example you added and later deleted using this feature revealing how to use and why is useful to have this feature.
Also, try to make the example simpler if possible. So that the focus is on the feature rather than on the Chips & understanding of other non-relevant stuff.
Sorry to be nitpicky. But if we don't document our features correctly and put examples for them. Is difficult for others to use them.
This reverts commit ed0498b.
@CPerezz added an example which showcases how to leverage unblinded columns to match commitments across multiple circuits / proofs in Here we create two simple circuits, one for vector mul, the other for addition. // Instantiate the mul circuit with the inputs.
let mul_circuit = MulCircuit {
a: a.iter().map(|&x| Value::known(x)).collect(),
b: b.iter().map(|&x| Value::known(x)).collect(),
};
// Instantiate the add circuit with the inputs.
let add_circuit = AddCircuit {
a: a.iter().map(|&x| Value::known(x)).collect(),
b: b.iter().map(|&x| Value::known(x)).collect(),
}; Both leverage unblinded columns for their inputs. For eg. fn configure(meta: &mut ConstraintSystem<F>) -> Self::Config {
// We create the three advice columns that FieldChip uses for I/O.
let advice = [
meta.unblinded_advice_column(),
meta.unblinded_advice_column(),
meta.unblinded_advice_column(),
];
// We also need an instance column to store public inputs.
let instance = meta.instance_column();
MultChip::configure(meta, advice, instance)
} Thus when comparing the proof bytes, the first set of bytes, corresponding to the commitments to these columns, should be the same for both proofs: // the commitments will be the first columns of the proof transcript so we can compare them easily
let proof_1 = test_prover::<halo2curves::pasta::EqAffine>(k, mul_circuit, true, c_mul);
// the commitments will be the first columns of the proof transcript so we can compare them easily
let proof_2 = test_prover::<halo2curves::pasta::EqAffine>(k, add_circuit, true, c_add);
// the commitments will be the first columns of the proof transcript so we can compare them easily
// here we compare the first 10 bytes of the commitments
println!(
"Commitments are equal: {:?}",
proof_1[..10] == proof_2[..10]
); |
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.
LGTM. Just one typo!
Once fixed we can merge!
Thanks for adding the example!
Co-authored-by: Carlos Pérez <[email protected]>
done ! @CPerezz |
Includes the vector-mul example with unblinded columns