Implement Dynamic lookups #660
Conversation
moreSocratic
force-pushed
the
dynamic-lookups
branch
from
29cab54
to
184d7a4
Compare
|
Could I get a review on this? |
| @@ -516,6 +591,7 @@ impl<F: Field> Expression<F> { | |||
| &self, | |||
| constant: &impl Fn(F) -> T, | |||
| selector_column: &impl Fn(Selector) -> T, | |||
| virtual_column: &impl Fn(VirtualColumn) -> T, | |||
There was a problem hiding this comment.
Since Expression::evaluate is public adding this argument is a breaking change.
Should I break out it into an extend variant of the function Expression::evaluate_ext, or is a major release planned so this breakage is fine?
| // Enable the lookup on rows | ||
| if i % 2 == 0 { | ||
| config.is_even.enable(&mut region, 0)?; | ||
| } else { | ||
| config.is_odd.enable(&mut region, 0)?; | ||
| }; | ||
|
|
||
| region.assign_advice(|| "", config.a, 0, || Value::known(Fp::from(i as u64))) |
There was a problem hiding this comment.
Do we always mean to be calling enable and assign_advice at offset 0?
| @@ -286,6 +287,8 @@ pub struct MockProver<F: Group + Field> { | |||
| instance: Vec<Vec<InstanceValue<F>>>, | |||
|
|
|||
| selectors: Vec<Vec<bool>>, | |||
| /// A map between DynamicTable.index, and rows included. | |||
There was a problem hiding this comment.
| /// A map between DynamicTable.index, and rows included. | |
| /// A map from the index of a DynamicTable, to the rows included in it. |
| @@ -374,6 +399,7 @@ fn render_constraint_not_satisfied<F: Field>( | |||
| } | |||
| } | |||
|
|
|||
| // TODO handle dynamic lookups | |||
There was a problem hiding this comment.
Is this TODO meaning to say that dynamic lookups should be handled separately from fixed lookups? The current PR seems to handle them in the same render_lookup function.
| @@ -119,6 +120,7 @@ impl CircuitGates { | |||
| expression: constraint.evaluate( | |||
| &util::format_value, | |||
| &|selector| format!("S{}", selector.0), | |||
| &|virtual_col| format!("T{}", virtual_col.0.index()), | |||
There was a problem hiding this comment.
| &|virtual_col| format!("T{}", virtual_col.0.index()), | |
| &|tag_col| format!("T{}", virtual_col.0.index()), |
| &|virtual_col| { | ||
| iter::once(format!("V{}", virtual_col.0.index())).collect() | ||
| }, |
There was a problem hiding this comment.
| &|virtual_col| { | |
| iter::once(format!("V{}", virtual_col.0.index())).collect() | |
| }, | |
| &|tag_col| { | |
| iter::once(format!("T{}", tag_col.0.index())).collect() | |
| }, |
|
Closing in favour of #715. |
| } | ||
|
|
||
| let table_query = cells.query_any(table, Rotation::cur()); | ||
| (selector.clone() * input, selector.clone() * table_query) |
There was a problem hiding this comment.
This would require inputs and tables to be perfectly aligned.
● Supports using Advice and Fixed columns as a lookup table.
● Supports interspersed/non-contiguous tables.
● Checks for unassigned cells using the mock prover (like gates do).
● Performs tag combining when tables do not include the same circuit row.
I'd still like to removeDynamicTable.columns, since the info is also inConstraintSystem.DynamicTableMap(ideas welcome), but the core of this PR is ready for reviews.