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

Implement Dynamic lookups #660

Closed
wants to merge 7 commits into from

Conversation

moreSocratic
Copy link

@moreSocratic moreSocratic commented Sep 16, 2022

● 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 remove DynamicTable.columns, since the info is also in ConstraintSystem.
  • And maybe improve DynamicTableMap (ideas welcome), but the core of this PR is ready for reviews.

@moreSocratic
Copy link
Author

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,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First-pass review; this needs a rebase.

Comment on lines +82 to +89
// 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)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&|virtual_col| format!("T{}", virtual_col.0.index()),
&|tag_col| format!("T{}", virtual_col.0.index()),

Comment on lines +158 to +160
&|virtual_col| {
iter::once(format!("V{}", virtual_col.0.index())).collect()
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&|virtual_col| {
iter::once(format!("V{}", virtual_col.0.index())).collect()
},
&|tag_col| {
iter::once(format!("T{}", tag_col.0.index())).collect()
},

@therealyingtong
Copy link
Collaborator

I've rebased this at #715. Most merge conflicts were from #697.

@therealyingtong
Copy link
Collaborator

Closing in favour of #715.

}

let table_query = cells.query_any(table, Rotation::cur());
(selector.clone() * input, selector.clone() * table_query)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require inputs and tables to be perfectly aligned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-nightly Issue or PR about a nightly feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants