Resurrecting lookup PR: Fix lookup has_zero_entry bug

book/src/specs/kimchi.md

@@ -1641,8 +1641,8 @@ If lookup is used, the following values are added to the common index:
 To create the index, follow these steps:
 
 1. If no lookup is used in the circuit, do not create a lookup index
-2. Get the lookup selectors and lookup tables (TODO: how?)
-3. Concatenate runtime lookup tables with the ones used by gates
+2. Get the lookup selectors and lookup tables that are specified implicitly
+3. Concatenate explicit runtime lookup tables with the ones (implicitly) used by gates
 4. Get the highest number of columns `max_table_width`
    that a lookup table can have.
 5. Create the concatenated table of all the fixed lookup tables.

kimchi/src/circuits/lookup/index.rs

@@ -217,11 +217,11 @@ impl<F: PrimeField + SquareRootField> LookupConstraintSystem<F> {
                 // product is 1, we cannot use those rows to store any values.
                 let max_num_entries = d1_size - zk_rows - 1;
 
-                //~ 2. Get the lookup selectors and lookup tables (TODO: how?)
+                //~ 2. Get the lookup selectors and lookup tables that are specified implicitly
                 let (lookup_selectors, gate_lookup_tables) =
                     lookup_info.selector_polynomials_and_tables(domain, gates);
 
-                //~ 3. Concatenate runtime lookup tables with the ones used by gates
+                //~ 3. Concatenate explicit runtime lookup tables with the ones (implicitly) used by gates
                 let mut lookup_tables: Vec<_> = gate_lookup_tables
                     .into_iter()
                     .chain(lookup_tables)

kimchi/src/circuits/lookup/tables/mod.rs

@@ -78,15 +78,19 @@ impl<F> LookupTable<F>
 where
     F: FftField,
 {
-    /// Return true if the table has an entry containing all zeros.
+    /// Return true if the table has an entry (row) containing all zeros.
     pub fn has_zero_entry(&self) -> bool {
         // reminder: a table is written as a list of columns,
         // not as a list of row entries.
         for row in 0..self.len() {
+            let mut row_zero = true;
             for col in &self.data {
                 if !col[row].is_zero() {
-                    continue;
+                    row_zero = false;
+                    break;
                 }
+            }
+            if row_zero {
                 return true;
             }
         }

kimchi/src/precomputed_srs.rs

@@ -33,6 +33,9 @@ where
     let file =
         File::open(srs_path.clone()).unwrap_or_else(|_| panic!("missing SRS file: {srs_path:?}"));
     let reader = BufReader::new(file);
+    // Note: In tests, this read takes significant amount of time (2 min) due
+    // to -O0 optimisation level. Compile tests with -O1 or --release flag.
+    // See: https://github.com/o1-labs/proof-systems/blob/develop/CONTRIBUTING.md#development
     rmp_serde::from_read(reader).unwrap()
 }
 

kimchi/src/prover_index.rs

@@ -210,6 +210,7 @@ pub mod testing {
             override_srs_size,
             |d1: D<G::ScalarField>, size: usize| {
                 let log2_size = size.ilog2();
+                // Run test_srs_serialization test to generate test SRS & enable this
                 let mut srs = if log2_size <= precomputed_srs::SERIALIZED_SRS_SIZE {
                     // TODO: we should trim it if it's smaller
                     precomputed_srs::get_srs()

kimchi/src/tests/lookup.rs

@@ -23,10 +23,12 @@ type BaseSponge = DefaultFqSponge<VestaParameters, SpongeParams>;
 type ScalarSponge = DefaultFrSponge<Fp, SpongeParams>;
 
 fn setup_lookup_proof(use_values_from_table: bool, num_lookups: usize, table_sizes: Vec<usize>) {
-    let lookup_table_values: Vec<Vec<_>> = table_sizes
+    let mut lookup_table_values: Vec<Vec<_>> = table_sizes
         .iter()
         .map(|size| (0..*size).map(|_| rand::random()).collect())
         .collect();
+    // Zero table must have a zero row
+    lookup_table_values[0][0] = From::from(0);
     let lookup_tables = lookup_table_values
         .iter()
         .enumerate()
@@ -205,7 +207,7 @@ fn test_runtime_table() {
     let len = first_column.len();
 
     let mut runtime_tables_setup = vec![];
-    for table_id in 0..num {
+    for table_id in 1..num + 1 {
         let cfg = RuntimeTableCfg {
             id: table_id,
             first_column: first_column.into_iter().map(Into::into).collect(),
@@ -241,7 +243,7 @@ fn test_runtime_table() {
 
         for row in 0..20 {
             // the first register is the table id. We pick one random table.
-            lookup_cols[0][row] = (rng.gen_range(0..num) as u32).into();
+            lookup_cols[0][row] = (rng.gen_range(1..num + 1) as u32).into();
 
             // create queries into our runtime lookup table.
             // We will set [w1, w2], [w3, w4] and [w5, w6] to randon indexes and
@@ -598,13 +600,21 @@ fn test_lookup_with_a_table_with_id_zero_but_no_zero_entry() {
 
     // Non zero-length table
     let len = 1u32 + rng.gen_range(0u32..max_len);
+    // Table id is 0
     let table_id: i32 = 0;
-    // No index 0 in the table.
+    // Always include index 0 in the table. Maybe even a few.
     let indices: Vec<Fp> = (0..len)
-        .map(|_| 1 + rng.gen_range(0u32..max_len))
+        .map(|i| {
+            if i == 0 {
+                0u32
+            } else {
+                rng.gen_range(0u32..max_len)
+            }
+        })
         .map(Into::into)
         .collect();
-    // No zero value
+    // But no zero values!
+    // So we'll get rows with zeroes that are not full-zero-rows.
     let values: Vec<Fp> = (0..len)
         .map(|_| rng.gen_range(1u32..max_len))
         .map(Into::into)