-
Notifications
You must be signed in to change notification settings - Fork 73
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
Continue CompositeBackend
#1469
Conversation
@@ -95,7 +119,7 @@ pub trait BackendFactory<F: FieldElement> { | |||
&self, | |||
pil: &'a Analyzed<F>, | |||
fixed: &'a [(String, Vec<F>)], | |||
output_dir: Option<&'a Path>, |
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 made it hard to change the path in the CompositeBackend
. I don't see a good reason to pass a reference with a lifetime here anyway.
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.
sgtm
With 8f472bd, the slowest CI job now takes 16min (compared to 14min before this PR), which I think is reasonable. |
@@ -95,7 +119,7 @@ pub trait BackendFactory<F: FieldElement> { | |||
&self, | |||
pil: &'a Analyzed<F>, | |||
fixed: &'a [(String, Vec<F>)], | |||
output_dir: Option<&'a Path>, |
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.
sgtm
pipeline/src/test_util.rs
Outdated
#[cfg(not(feature = "halo2"))] | ||
pub fn test_halo2(_file_name: &str, _inputs: Vec<Bn254Field>) {} | ||
|
||
#[cfg(feature = "halo2")] | ||
pub fn gen_halo2_proof(file_name: &str, inputs: Vec<Bn254Field>) { | ||
pub fn gen_halo2_proof(file_name: &str, inputs: Vec<Bn254Field>, composite: bool) { |
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.
Not a fan of boolean arguments like that, if it can't be two different functions can you at least introduce an enum for the {use_}composite
argument?
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 split the function.
A few incremental changes towards implementing VadCop (#424):
CompositeBackend
now explicitly panics for stuff that I'm planning to defer the implementation of, like exporting verification keys."main"
.