Lookups: Improve assertions and tests regarding empty rows and table duplicates #1263

volhovm · 2023-10-02T20:18:50Z

Introduces test for lookup table creation function here:
https://github.com/o1-labs/proof-systems/pull/1263/files#diff-c712332ec88dcf26c81cb945fa135596e8816276fbec24228af3922546cc5443R384
Fix the bug in checking the existence of the zero row (the check was not actually done properly)
Move a check on inconsistent table lengths into the table implementation itself.
Add a check on duplicate table IDs and fix several tests that were mistakenly double-adding the tables.

This resolves MinaProtocol/mina#14070 and MinaProtocol/mina#14097.

volhovm · 2023-10-02T20:24:30Z

@mimoo Question: test_zero_table_zero_row will return true if at least one element of the row is zero. While the description of the function implies it should return true when all the elements of the row are zero. Is this a bug or a documentation issue?

https://github.com/o1-labs/proof-systems/blob/develop/kimchi/src/circuits/lookup/tables/mod.rs#L35

(You were in git blame for this line, feel free to redirect me to someone else if you're not the author)

…#create()

kimchi/src/circuits/lookup/index.rs

kimchi/src/prover_index.rs

mrmr1993 · 2023-10-30T14:00:03Z

kimchi/src/circuits/lookup/index.rs

@@ -411,6 +433,8 @@ impl<F: PrimeField + SquareRootField> LookupConstraintSystem<F> {
                    lookup_table8.push(eval);
                }

+                // @volhovm: Do we need to enforce that there is at least one table


We should actually ensure that either we have a table with ID 0 or that there was space for us to insert a 0 value.

"space to insert zero value" is checking that the total number of lookup table rows is one less than d1 domain minus zk rows?

mrmr1993 · 2023-10-30T14:00:50Z

kimchi/src/circuits/lookup/lookups.rs

@@ -204,7 +204,7 @@ impl LookupInfo {
        &self,
        domain: &EvaluationDomains<F>,
        gates: &[CircuitGate<F>],
-    ) -> (LookupSelectors<Evaluations<F>>, Vec<LookupTable<F>>) {
+    ) -> (LookupSelectors<Evaluations<F>>, HashSet<LookupTable<F>>) {


HashSet ordering is non-deterministic (and we probably shouldn't be hashing a whole lookup table). Revert.

Hashset order being nondeterministic is true, but the problem is deeper then: we build tables right now on the result of this hashset converted to a vector: we return gate_tables.iter().convert(), where gate_tables is a hashset. What I did instead is to return it directly without converting to vec, because it might be more convenient to check for duplicates.

The question is: how do you want tables sorted between each other, and is there any difference at all?

mrmr1993 · 2023-10-30T14:01:47Z

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

 /// A table of values that can be used for a lookup, along with the ID for the table.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]


Don't hash/check equality on a Vec<Vec<F>>. It could be large!

It is absolutely true and I realise it, but otherwise we can't really check if there are any truly duplicate tables in our setup. In that case we would only be able to check table id collisions. I guess it's fine.

kimchi/src/prover_index.rs

kimchi/src/tests/lookup.rs

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

kimchi/src/circuits/constraints.rs

kimchi/src/circuits/lookup/index.rs

dannywillems · 2023-11-13T15:09:50Z

Opened PR: MinaProtocol/mina#14549.
Checking if it does not break anything in Mina.
Feel free to push on this branch and update the PR.

dannywillems · 2023-11-13T15:13:05Z

Changes have to be done in https://github.com/o1-labs/o1js-bindings/tree/main/kimchi/wasm.

error[E0451]: field `id` of struct `kimchi::circuits::lookup::tables::LookupTable` is private
  --> src/pasta_fp_plonk_index.rs:47:13
   |
47 |             id: wasm_lt.id.into(),
   |             ^^^^^^^^^^^^^^^^^^^^^ private field

error[E0451]: field `data` of struct `kimchi::circuits::lookup::tables::LookupTable` is private
  --> src/pasta_fp_plonk_index.rs:48:13
   |
48 |             data: wasm_lt.data.0,
   |             ^^^^^^^^^^^^^^^^^^^^ private field

error[E0451]: field `id` of struct `kimchi::circuits::lookup::tables::LookupTable` is private
  --> src/pasta_fq_plonk_index.rs:44:13
   |
44 |             id: wasm_lt.id.into(),
   |             ^^^^^^^^^^^^^^^^^^^^^ private field

error[E0451]: field `data` of struct `kimchi::circuits::lookup::tables::LookupTable` is private
  --> src/pasta_fq_plonk_index.rs:45:13
   |
45 |             data: wasm_lt.data.0,
   |             ^^^^^^^^^^^^^^^^^^^^ private field

dannywillems · 2023-11-13T15:14:28Z

Please git merge develop and check again against mina.

mrmr1993 · 2023-12-04T20:08:30Z

kimchi/src/circuits/lookup/index.rs

+                            runtime_tables_ids
+                                .iter()
+                                .chain(fixed_lookup_tables_ids.iter()),
+                            "duplicates between runtime and fixed tables",


We explicitly don't want this. This should never have landed without backing this out.

…-id-zero-row" This reverts commit 7f6eba0, reversing changes made to 88c1776.

Revert bad PR #1263

volhovm changed the base branch from master to develop October 2, 2023 20:19

volhovm force-pushed the volhovm/mina14070-table-id-zero-row branch from 09684d6 to fa03ec4 Compare October 2, 2023 20:21

volhovm force-pushed the volhovm/mina14070-table-id-zero-row branch 2 times, most recently from 798f83f to 77fbfc1 Compare October 4, 2023 21:56

volhovm force-pushed the volhovm/mina14070-table-id-zero-row branch from c799649 to f9b2ce7 Compare October 12, 2023 11:59

volhovm added 2 commits October 12, 2023 12:59

[#14070] Add a unit check for the table id zero value condition

488ad18

[#14070] Fix the table dummy rows bug

74b5b0c

volhovm force-pushed the volhovm/mina14070-table-id-zero-row branch 2 times, most recently from 442a784 to 670bf26 Compare October 12, 2023 12:00

[#14097,#14070] Move lookup table consistency checks into LookupTable…

eaf3184

…#create()

volhovm force-pushed the volhovm/mina14070-table-id-zero-row branch from 670bf26 to eaf3184 Compare October 12, 2023 12:05

volhovm force-pushed the volhovm/mina14070-table-id-zero-row branch from e7a23cc to f004e8b Compare October 30, 2023 09:27

volhovm changed the title ~~Add a unit test for lookup table zero table zero row condition~~ Lookups: Improve assertions and tests regarding empty rows and table duplicates Oct 30, 2023

volhovm force-pushed the volhovm/mina14070-table-id-zero-row branch from f004e8b to 0e772a2 Compare October 30, 2023 09:42