-
Notifications
You must be signed in to change notification settings - Fork 217
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
Finalize #334
Finalize #334
Changes from 3 commits
27f8b80
31a16cd
4c4ad1c
afd19bb
974ef4f
32e5206
a31f000
7f9ef99
b58393a
9de6b0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -47,6 +47,10 @@ pub struct ConstraintSystem<F: Field> { | |||||||||||||||||||||
/// The number of linear combinations | ||||||||||||||||||||||
pub num_linear_combinations: usize, | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// The parameter we aim to minimize in this constraint system (either the | ||||||||||||||||||||||
/// number of constraints or their total weight). | ||||||||||||||||||||||
pub optimization_goal: OptimizationGoal, | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Assignments to the public input variables. This is empty if `self.mode | ||||||||||||||||||||||
/// == SynthesisMode::Setup`. | ||||||||||||||||||||||
pub instance_assignment: Vec<F>, | ||||||||||||||||||||||
|
@@ -91,6 +95,16 @@ pub enum SynthesisMode { | |||||||||||||||||||||
}, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Defines the parameter to optimize for a `ConstraintSystem`. | ||||||||||||||||||||||
#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] | ||||||||||||||||||||||
pub enum OptimizationGoal { | ||||||||||||||||||||||
/// Minimize the number of constraints. | ||||||||||||||||||||||
Constraints, | ||||||||||||||||||||||
/// Minimize the total weight of the constraints (the number of nonzero | ||||||||||||||||||||||
/// entries across all constraints). | ||||||||||||||||||||||
Weight, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
impl<F: Field> ConstraintSystem<F> { | ||||||||||||||||||||||
#[inline] | ||||||||||||||||||||||
fn make_row(&self, l: &LinearCombination<F>) -> Vec<(F, usize)> { | ||||||||||||||||||||||
|
@@ -109,7 +123,7 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
.collect() | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Construct an ampty `ConstraintSystem`. | ||||||||||||||||||||||
/// Construct an empty `ConstraintSystem`. | ||||||||||||||||||||||
pub fn new() -> Self { | ||||||||||||||||||||||
Self { | ||||||||||||||||||||||
num_instance_variables: 1, | ||||||||||||||||||||||
|
@@ -131,6 +145,8 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
mode: SynthesisMode::Prove { | ||||||||||||||||||||||
construct_matrices: true, | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
|
||||||||||||||||||||||
optimization_goal: OptimizationGoal::Constraints, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -149,6 +165,18 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
self.mode == SynthesisMode::Setup | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Check whether this constraint system aims to optimize weight or | ||||||||||||||||||||||
/// number of constraints. | ||||||||||||||||||||||
pub fn get_optimization_goal(&self) -> OptimizationGoal { | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
self.optimization_goal | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Specify whether this constraint system should aim to optimize weight | ||||||||||||||||||||||
/// or number of constraints. | ||||||||||||||||||||||
pub fn set_optimization_goal(&mut self, goal: OptimizationGoal) { | ||||||||||||||||||||||
self.optimization_goal = goal; | ||||||||||||||||||||||
weikengchen marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Check whether or not `self` will construct matrices. | ||||||||||||||||||||||
pub fn should_construct_matrices(&self) -> bool { | ||||||||||||||||||||||
match self.mode { | ||||||||||||||||||||||
|
@@ -238,7 +266,8 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Count the number of times each LC is used within other LCs in the constraint system | ||||||||||||||||||||||
/// Count the number of times each LC is used within other LCs in the | ||||||||||||||||||||||
/// constraint system | ||||||||||||||||||||||
fn lc_num_times_used(&self, count_sinks: bool) -> Vec<usize> { | ||||||||||||||||||||||
let mut num_times_used = vec![0; self.lc_map.len()]; | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -260,16 +289,17 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
/// Transform the map of linear combinations. | ||||||||||||||||||||||
/// Specifically, allow the creation of additional witness assignments. | ||||||||||||||||||||||
/// | ||||||||||||||||||||||
/// This method is used as a subroutine of `inline_all_lcs` and `outline_lcs`. | ||||||||||||||||||||||
/// | ||||||||||||||||||||||
/// The transformer function is given a references of this constraint system (&self), | ||||||||||||||||||||||
/// number of times used, and a mutable reference of the linear combination to be transformed. | ||||||||||||||||||||||
/// (&ConstraintSystem<F>, usize, &mut LinearCombination<F>) | ||||||||||||||||||||||
/// This method is used as a subroutine of `inline_all_lcs` and | ||||||||||||||||||||||
/// `outline_lcs`. | ||||||||||||||||||||||
/// | ||||||||||||||||||||||
/// The transformer function returns the number of new witness variables needed | ||||||||||||||||||||||
/// and a vector of new witness assignments (if not in the setup mode). | ||||||||||||||||||||||
/// (usize, Option<Vec<F>>) | ||||||||||||||||||||||
/// The transformer function is given a references of this constraint system | ||||||||||||||||||||||
/// (&self), number of times used, and a mutable reference of the linear | ||||||||||||||||||||||
/// combination to be transformed. (&ConstraintSystem<F>, usize, | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Were these comments re-ordered by a linter? Its kinda weird imo that the tuple is split across a new line |
||||||||||||||||||||||
/// &mut LinearCombination<F>) | ||||||||||||||||||||||
/// | ||||||||||||||||||||||
/// The transformer function returns the number of new witness variables | ||||||||||||||||||||||
/// needed and a vector of new witness assignments (if not in the setup | ||||||||||||||||||||||
/// mode). (usize, Option<Vec<F>>) | ||||||||||||||||||||||
pub fn transform_lc_map( | ||||||||||||||||||||||
&mut self, | ||||||||||||||||||||||
transformer: &mut dyn FnMut( | ||||||||||||||||||||||
|
@@ -309,14 +339,14 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
// Delete linear combinations that are no longer used. | ||||||||||||||||||||||
// | ||||||||||||||||||||||
// Deletion is safe for both outlining and inlining: | ||||||||||||||||||||||
// * Inlining: the LC is substituted directly into all use | ||||||||||||||||||||||
// sites, and so once it is fully inlined, it is redundant. | ||||||||||||||||||||||
// * Inlining: the LC is substituted directly into all use sites, and so once it | ||||||||||||||||||||||
// is fully inlined, it is redundant. | ||||||||||||||||||||||
// | ||||||||||||||||||||||
// * Outlining: the LC is associated with a new variable `w`, | ||||||||||||||||||||||
// and a new constraint of the form `lc_data * 1 = w`, where | ||||||||||||||||||||||
// `lc_data` is the actual data in the linear combination. | ||||||||||||||||||||||
// Furthermore, we replace its entry in `new_lc_map` with `(1, w)`. | ||||||||||||||||||||||
// Once `w` is fully inlined, then we can delete the entry from `new_lc_map` | ||||||||||||||||||||||
// * Outlining: the LC is associated with a new variable `w`, and a new | ||||||||||||||||||||||
// constraint of the form `lc_data * 1 = w`, where `lc_data` is the actual | ||||||||||||||||||||||
// data in the linear combination. Furthermore, we replace its entry in | ||||||||||||||||||||||
// `new_lc_map` with `(1, w)`. Once `w` is fully inlined, then we can delete | ||||||||||||||||||||||
// the entry from `new_lc_map` | ||||||||||||||||||||||
// | ||||||||||||||||||||||
num_times_used[lc_index.0] -= 1; | ||||||||||||||||||||||
if num_times_used[lc_index.0] == 0 { | ||||||||||||||||||||||
|
@@ -404,8 +434,9 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
// If true, the LC is replaced with 1 * this witness variable. | ||||||||||||||||||||||
// Otherwise, the LC is inlined. | ||||||||||||||||||||||
// | ||||||||||||||||||||||
// Each iteration first updates the LC according to outlinings in prior iterations, | ||||||||||||||||||||||
// and then sees if it should be outlined, and if so adds the outlining to the map. | ||||||||||||||||||||||
// Each iteration first updates the LC according to outlinings in prior | ||||||||||||||||||||||
// iterations, and then sees if it should be outlined, and if so adds | ||||||||||||||||||||||
// the outlining to the map. | ||||||||||||||||||||||
// | ||||||||||||||||||||||
self.transform_lc_map(&mut |cs, num_times_used, inlined_lc| { | ||||||||||||||||||||||
let mut should_dedicate_a_witness_variable = false; | ||||||||||||||||||||||
|
@@ -481,10 +512,19 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
/// Useful for SNARKs like [\[Marlin\]](https://eprint.iacr.org/2019/1047) or | ||||||||||||||||||||||
/// [\[Fractal\]](https://eprint.iacr.org/2019/1076), where addition gates | ||||||||||||||||||||||
/// are not cheap. | ||||||||||||||||||||||
pub fn reduce_constraint_weight(&mut self) { | ||||||||||||||||||||||
fn reduce_constraint_weight(&mut self) { | ||||||||||||||||||||||
self.outline_lcs(); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove this now, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't the idea to add other weight-reducing optimizations, apart from just outlining? (That's what the comment here says.) In which case we'd still want this as a separate function |
||||||||||||||||||||||
|
||||||||||||||||||||||
/// Finalize the constraint system (either by outlining or inlining, | ||||||||||||||||||||||
/// depending on the set optimization mode). | ||||||||||||||||||||||
pub fn finalize(&mut self) { | ||||||||||||||||||||||
match self.optimization_goal { | ||||||||||||||||||||||
OptimizationGoal::Constraints => self.inline_all_lcs(), | ||||||||||||||||||||||
OptimizationGoal::Weight => self.outline_lcs(), | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// This step must be called after constraint generation has completed, and | ||||||||||||||||||||||
/// after all symbolic LCs have been inlined into the places that they | ||||||||||||||||||||||
/// are used. | ||||||||||||||||||||||
|
@@ -604,7 +644,7 @@ impl<F: Field> ConstraintSystem<F> { | |||||||||||||||||||||
self.lc_assignment_cache.borrow_mut().insert(idx, value); | ||||||||||||||||||||||
Some(value) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
@@ -782,6 +822,23 @@ impl<F: Field> ConstraintSystemRef<F> { | |||||||||||||||||||||
.map_or(0, |cs| cs.borrow().num_witness_variables) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Check whether this constraint system aims to optimize weight or | ||||||||||||||||||||||
/// number of constraints. | ||||||||||||||||||||||
#[inline] | ||||||||||||||||||||||
pub fn get_optimization_goal(&self) -> OptimizationGoal { | ||||||||||||||||||||||
self.inner().map_or(OptimizationGoal::Constraints, |cs| { | ||||||||||||||||||||||
cs.borrow().get_optimization_goal() | ||||||||||||||||||||||
}) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
|
||||||||||||||||||||||
/// Specify whether this constraint system should aim to optimize weight | ||||||||||||||||||||||
/// or number of constraints. | ||||||||||||||||||||||
#[inline] | ||||||||||||||||||||||
pub fn set_optimization_goal(&self, goal: OptimizationGoal) { | ||||||||||||||||||||||
self.inner() | ||||||||||||||||||||||
.map_or((), |cs| cs.borrow_mut().set_optimization_goal(goal)) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Check whether or not `self` will construct matrices. | ||||||||||||||||||||||
#[inline] | ||||||||||||||||||||||
pub fn should_construct_matrices(&self) -> bool { | ||||||||||||||||||||||
|
@@ -875,6 +932,14 @@ impl<F: Field> ConstraintSystemRef<F> { | |||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Finalize the constraint system (either by outlining or inlining, | ||||||||||||||||||||||
/// depending on the set optimization mode). | ||||||||||||||||||||||
pub fn finalize(&self) { | ||||||||||||||||||||||
if let Some(cs) = self.inner() { | ||||||||||||||||||||||
cs.borrow_mut().finalize() | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// This step must be called after constraint generation has completed, and | ||||||||||||||||||||||
/// after all symbolic LCs have been inlined into the places that they | ||||||||||||||||||||||
/// are used. | ||||||||||||||||||||||
|
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.
Should we have a
None
here, which will do no optimization passes?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.
That makes sense. Do you think
None
, orConstraints
, should be the default?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 think constraints should be the default
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 are the semantics of calling finalize with None? In the end we have to get rid of all SymbolicLC before converting to matrices
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 think
None
would still need to inline Symbolic LC (self.inline_all_lcs()
, without whichto_matrices
would fail.Dev, what do you think? Basically, at this moment,
None
would be the same asConstraints
, and it is uncertain if in the future we will treat them differently.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 think given that no specific use cases pop up for
None
, and we currently do not have a good idea of faster inlining (without optimization to reduce constraints), maybe we can omitNone
and add it later. Dev, what do you think?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.
Sounds good to me!
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.
Actually, I do think
None
is still useful, in that it could just make far fewer symbolic LC's for memory reduction. E.g. when multiplying by a constant, we don't need to make a new symbolic LC if we don't care about optimizationsThere 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.
And we could have drop semantics to symbolic LC's to remove intermediate variables from the map as soon as they're dropped from memory. It is a significant engineering effort though, so maybe not worth having it now. (Its non breaking to add it later)
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.
That makes sense. Then let us keep
None
and, for now, letNone
also doinline_all_lcs
. More changes may be coming in the future.