Conversation
jeanmon
left a comment
There was a problem hiding this comment.
Very nice work! Please address/look at my feedback before merging.
bberg/src/circuit_builder.rs
Outdated
| return false; | ||
| }}", | ||
| permutation_name = permutation_name | ||
| lookup_name = lookup_name |
There was a problem hiding this comment.
nitpick: I would prefer two different identifiers, unless this is Rust standard practice.
There was a problem hiding this comment.
This can just be replaced with the single identifier, as it is just for string templating - will edit
bberg/src/lookup_builder.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug)] | ||
| /// PermSide |
There was a problem hiding this comment.
Copy-paste leftover? Probably should be "LookupSide"
There was a problem hiding this comment.
Actually good point here- they are more or less the same i can unify the type
There was a problem hiding this comment.
Actually i probably wont, even though it is repeated it is much more readable when they are defined beside each other
| ) -> Vec<Lookup>; | ||
| } | ||
|
|
||
| impl LookupBuilder for BBFiles { |
There was a problem hiding this comment.
In this function and subsequent ones, replace variables like "perm", "perm_settings" by "lookup", "lookup_settings".
bberg/src/lookup_builder.rs
Outdated
| * | ||
| * @details To create your own lookup: | ||
| * 1) Create a copy of this class and rename it | ||
| * 2) Update all the values with the ones needed for your permutation |
There was a problem hiding this comment.
| * 2) Update all the values with the ones needed for your permutation | |
| * 2) Update all the values with the ones needed for your lookup |
| static constexpr size_t READ_TERM_TYPES[READ_TERMS] = {read_term_types}; | ||
|
|
||
| /** | ||
| * @brief They type of WRITE_TERM used for each write index |
There was a problem hiding this comment.
| * @brief They type of WRITE_TERM used for each write index | |
| * @brief The type of WRITE_TERM used for each write index |
bberg/src/lookup_builder.rs
Outdated
| }}") | ||
| } | ||
|
|
||
| fn get_perm_side<F: FieldElement>(def: &SelectedExpressions<AlgebraicExpression<F>>) -> LookupSide { |
There was a problem hiding this comment.
Rename function to "get_lookup_side"
| {compute_inverse_exists} | ||
|
|
||
| /** | ||
| * @brief Get all the entities for the lookup when need to update them |
There was a problem hiding this comment.
| * @brief Get all the entities for the lookup when need to update them | |
| * @brief Get all the entities for the lookup when we need to update them |
Co-authored-by: Jean M <132435771+jeanmon@users.noreply.github.com>
see AztecProtocol/aztec-packages#3880 for implementation example
Lookups allow us to assert that values in a set of columns equal some values in another set of columns.
This approach uses the log derivative method to perform lookups, where the lookup relation requires four extra columns ( as well as the read and write columns ) to function.
The selector columns will need to be defined manually - as in the
ToyAvmexample.The inverse column and the counts columns are automatically included by the code gen. ( this means there are two extra columns per lookup ( which is a little bit inefficient; but will do for now ).
The name of the inverse column is decided by the attribute tag
#[attribute]above the lookup - which is required.The name of the counts column is
${attribute}_counts.Note: that in the circuit builder you are currently required to set the counts column value manually.
syntax: