[WIP] Rebase dynamic lookups #715

therealyingtong · 2023-01-02T15:51:33Z

Recommended: review commit-by-commit.

to run tests: cargo test --features unstable-dynamic-lookups dyn
to run example: cargo run --features unstable-dynamic-lookups --example dynamic-lookup

(This PR is a rebased version of #660.)

halo2_proofs/src/circuit/layouter.rs

halo2_proofs/src/plonk/keygen.rs

halo2_proofs/src/plonk/prover.rs

halo2_proofs/src/plonk/verifier.rs

daira · 2023-02-03T21:49:19Z

halo2_proofs/src/dev.rs

@@ -476,7 +491,7 @@ impl<F: Field> Assignment<F> for MockProver<F> {
    }
 }

-impl<F: Field + Ord> MockProver<F> {
+impl<F: PrimeField + Ord> MockProver<F> {


Logically this shouldn't need to be a prime field because, for example, we might want to support proving systems that are not group-based, such as FRI. (For a group-based proving system, a non-prime scalar field would be insecure because discrete log on the group could be broken by Pohlig–Hellman.)

CPerezz

1st pass! :)

halo2_proofs/src/dev.rs

CPerezz · 2023-02-09T09:05:00Z

halo2_proofs/src/dev.rs

+            }
+        }
+
+        /// Just like dynamic_lookup, but breaks the table into single row regions.


I don't see how we break the table into single row regions. Am I missing anything?

halo2_proofs/src/dev/failure.rs

CPerezz · 2023-02-09T09:17:46Z

halo2_proofs/src/dev/failure.rs

            &|_| panic!("no instance columns in table expressions"),
            &|_| panic!("no negations in table expressions"),
            &|_, _| panic!("no sums in table expressions"),
-            &|_, _| panic!("no products in table expressions"),
+            &|a, b| format!("{} * {}", a, b),


Why are products now required? I presume because the Advice assigned into the DynamicLookupTable can come from any expression result.

If so, summ negation and scale should be supported too.

CPerezz · 2023-02-09T09:19:25Z

halo2_proofs/src/dev/gates.rs

+                            &|virtual_col| {
+                                iter::once(format!("V{}", virtual_col.0.index())).collect()
+                            },


Maybe not in this PR. But if we plan to support Instance cols too, we should change V for virtual_col.0.col_type()

CPerezz · 2023-02-09T09:20:13Z

halo2_proofs/src/dev/graph/layout.rs

@@ -82,7 +82,7 @@ impl CircuitLayout {
    }

    /// Renders the given circuit on the given drawing area.
-    pub fn render<F: Field, ConcreteCircuit: Circuit<F>, DB: DrawingBackend>(
+    pub fn render<F: PrimeField, ConcreteCircuit: Circuit<F>, DB: DrawingBackend>(


I saw above. Nor sure neither. It doesn't seem that anything requires any function from the PrimeField trait specifically right?

halo2_proofs/src/plonk/circuit.rs

halo2_proofs/src/dev/failure.rs

halo2_proofs/src/dev/failure/emitter.rs

halo2_proofs/src/dev/util.rs

halo2_proofs/src/plonk/lookup/prover.rs

halo2_proofs/src/plonk/lookup/verifier.rs

halo2_proofs/src/circuit.rs

halo2_proofs/src/dev.rs

halo2_proofs/src/dev/gates.rs

halo2_proofs/src/plonk/circuit/compress_selectors.rs

halo2_proofs/src/plonk/circuit.rs

therealyingtong · 2023-04-04T21:16:02Z

A book entry for the dynamic lookups feature has been added in 1de300f21ec84b119f82f04c3e7a2fabe4d40c82.

Co-authored-by: Avi Dessauer <[email protected]>

…ble() Co-authored-by: Avi Dessauer <[email protected]>

Co-authored-by: Avi Dessauer <[email protected]>

therealyingtong · 2023-04-17T11:22:23Z

This feature currently does not seem to work when input columns are reused to assign dynamic table columns; similarly, when inputs/tables across multiple arguments share the same columns. (see 9ce8858#diff-faaf1a95220be5b9b73f935e6659140eac1bd000fbbccb6e102589b4c3ec49c5R1869)

This seems like unexpected behaviour.

Co-authored-by: Avi Dessauer <[email protected]>

therealyingtong · 2023-05-04T08:35:40Z

As it stands now, this PR requires inputs and tables to be perfectly aligned by looking up these tuples:

(selector.clone() * input, selector.clone() * table_query)

https://github.com/Orbis-Tertius/halo2/blob/8bcc3f2349d926e4fddeee7cce09202913e10b8e/halo2_proofs/src/plonk/circuit.rs#L1226

Instead, we could either:

remove the selector.clone() from before table_query, which would require introducing a tag = 0, table_values = 0 in each table; or
introduce a table_selector complex selector, which would make the table tags redundant.

therealyingtong mentioned this pull request Jan 2, 2023

Implement Dynamic lookups #660

Closed

2 tasks

str4d added the F-nightly Issue or PR about a nightly feature label Jan 20, 2023

str4d requested changes Feb 3, 2023

View reviewed changes

daira reviewed Feb 3, 2023

View reviewed changes

CPerezz reviewed Feb 9, 2023

View reviewed changes

therealyingtong force-pushed the rebase-dynamic-lookups branch 4 times, most recently from dba5db0 to 0fb457e Compare February 23, 2023 04:22