fix!: roll back the `compress_selectors` logic #322

guorong009 · 2024-05-06T14:08:45Z

Description

Roll back the logic of compressing selectors in compile_circuit

Related issue

Broken selector compression #321

Changes

remove compile_circuit_cs
roll back the implementation of compressing selectors in compile_circuit
remove the create_proof_custom_with_engine
correct the related codes
add unit test about compressing selectors
add a new CI check of running halo2_proofs/examples(additional)

ed255

Overall looks good! I just have an important comment about the change you did in the "serialization" example.

ed255 · 2024-05-07T10:02:35Z

halo2_proofs/examples/serialization.rs

+        // TEST for "compress_selectors" logic correctness
+        let s = meta.selector();
+        meta.create_gate("selector gate", |meta| {
+            let s = meta.query_selector(s);
+            let a = meta.query_advice(a, Rotation::cur());
+            let b = meta.query_advice(b, Rotation::cur());
+            vec![s * (a - b)]
+        });
+


I would consider removing this. The reason is that this circuit is called StandardPlonk, and this gate is not part of the standard plonk; so as an example it can be confusing.

Moreover, I don't see any update on the example that assigns values to the new selector s.

I think it's better to have a unit test for selector to fixed checks (both compression and without, different number of selectors, different expression degree, etc.)

@ed255
I add the unit tests for compress_selectors.
Pls check this file if it is able to cover all possible cases.

ed255 · 2024-05-07T10:08:15Z

halo2_proofs/src/plonk.rs

-        #[cfg(feature = "circuit-params")]
-        params,
-    );
+    let (_, _, cs) = compile_circuit(k, circuit, compress_selectors)


This is why I tried to implement compile_circuit_cs originally. So that we could do vk_read without having to call synthesize on the circuit :(
But unfortunately it's not possible; the cs needs the conversion of selector expressions; and that requires selector assignments, which we obtain by calling synthesize in compile_circuit.

As a result the methods vk_read and pk_read do a lot of work.

Nevertheless the frontend-backend split API methods for reading vk and pk don't suffer from this:

halo2/halo2_backend/src/plonk.rs

Line 99 in 9eedeb5

pub fn read<R: io::Read>(

So I think this is acceptable :)

adria0 · 2024-05-07T10:46:00Z

@guorong009 Roll back the logic of compressing selectors in compile_circuit

There's a specific commit (where do you get the old logic) to check?

ed255 · 2024-05-07T10:48:33Z

@guorong009 Roll back the logic of compressing selectors in compile_circuit

There's a specific commit (where do you get the old logic) to check?

The specific commit is referenced in #321
Basically it's PR #290 and within that PR this commit eddbe95

ed255

LGTM!

ed255 · 2024-05-10T11:25:49Z

halo2_proofs/src/plonk.rs

@@ -44,17 +44,15 @@ use std::io;
 pub fn vk_read<C: SerdeCurveAffine, R: io::Read, ConcreteCircuit: Circuit<C::Scalar>>(
    reader: &mut R,
    format: SerdeFormat,
+    k: u32,


It's unfortunate that we need to pass k because the serialized key already has k. https://github.com/privacy-scaling-explorations/halo2/blob/9eedeb5d6eb9a119d34307697e670b0bb043a5a5/halo2_backend/src/plonk.rs#L115C9-L115C38

But anyway, this is for the legacy API so I think it's OK. But it makes me wonder, should we deprecate the legacy API at some point? It would make our lives easier.

Yes, I believe that we would need to deprecate the legacy API in near future.
But, I also believe that it should come after discussion with community.
In addition, I think we don't need more work with legacy API, later.

ed255 · 2024-05-10T11:30:55Z

halo2_proofs/tests/compress_selectors.rs

+    assert!(test_mycircuit(false, false).is_ok());
+}
+
+#[should_panic]


Does this test panic from an out of bounds array access?

Yes, exactly.
Fyi, the test panics with following error message.

thread '<unnamed>' panicked at /.../halo2/halo2_backend/src/plonk/evaluation.rs:81:17: index out of bounds: the len is 2 but the index is 2

ed255 · 2024-05-10T11:48:22Z

Before merging, could you add this piece of documentation in the new test? I think it will help understand what's happening for the compress true/false case:

# compress = false

selector `s_add` -> fixed `s_add`
- 1 when `s_add` enabled, 0 otherwise
selector `s_mul` -> fixed `s_mul`
- 1 when `s_mul` enabled, 0 otherwise
selector `s_cubed` -> fixed `s_cubed`
- 1 when `s_cubed` enabled, 0 otherwise

Selector queries in expressions become the corresponding fixed column queries
at rotation 0.

# compress = true

selector `s_add`, `s_mul` -> fixed `s_add_mul`
- 0 when `s_add` disabled and `s_mul` disabled
- 1 when only `s_add` enabled
- 2 when only `s_mul` enabled
selector `s_cubed` -> fixed `s_cubed`
- 1 when `s_cubed` enabled, 0 otherwise
- NOTE: `s_cubed` is not compressed to avoid growing the max degree which is 3

Selector query for `s_add` becomes (`s_add_mul`)*(2 - `s_add_mul`)
Selector query for `s_mul` becomes (`s_add_mul`)*(1 - `s_add_mul`)
Selector query for `s_cubed` becomes the corresponding fixed column query
at rotation 0.

Maybe this could go as a comment in the test test_success. Or as a comment before the tests (wherever you see fit).

guorong009 added 17 commits April 29, 2024 14:50

fix: correct the logic of convert selectors to fixed column

05c9292

refactor: remove the "create_proof_custom_with_engine"

a002524

chore: clippy

0152297

chore: add ci for running examples

5627fce

chore: fix the ci

41e4da2

chore: clean the ci check script

8f38f79

fix: roll back wrong updates

47157ea

fix!: correct the "compile_circuit" process

151112e

chore: update import

61d2d05

chore: fmt

0469a7b

feat: roll back "compress_selectors" logic

ba65fd8

chore: update the .gitignore

0125d5f

fix: update the "layout.rs"

2034660

feat: remove the "compress_selectors" from several utils

09f63a2

fix: remove the "selectors_to_fixed" field from "ConstraintSystem"

ef7573d

feat: remove the "compress_selectors" option & default to "true"

1688754

feat: update the "vk_read" & "pk_read"

0fb9792

guorong009 requested a review from ed255 May 6, 2024 14:08

guorong009 added 3 commits May 6, 2024 22:12

chore: remove leftovers

33b6dbd

fix: re-enable the "compress_selectors" option

bbc773b

chore: roll back some needless update

f614ddd

ed255 reviewed May 7, 2024

View reviewed changes

ed255 linked an issue May 7, 2024 that may be closed by this pull request

Broken selector compression #321

Closed

guorong009 mentioned this pull request May 7, 2024

fix!: correct the compile_circuit #320

Closed

guorong009 changed the title ~~[draft]: experiment about compress_selectors~~ fix!: roll back the compress_selectors logic May 7, 2024

guorong009 added 3 commits May 8, 2024 13:19

test: add integration test for "compress_selectors"

2ef0805

chore: fmt + clippy

6ad6b4b

test: use the custom circuit for integration test

df25fee

guorong009 added 5 commits May 8, 2024 23:34

chore: fix clippy

deed874

chore: fix error mapping

2bcb970

chore: some renamings + some comments

379491e

fix: remove wrong attr & comments

b8aa324

fix!: remove the "create_proof_custom_with_engine"

3190c57

guorong009 requested a review from ed255 May 9, 2024 14:43

guorong009 self-assigned this May 9, 2024

guorong009 marked this pull request as ready for review May 9, 2024 14:43

chore: some housekeeping

46d1a85

ed255 approved these changes May 10, 2024

View reviewed changes

chore: add detailed comments

95c6bac

guorong009 force-pushed the gr@fix-compile-circuit-cs-experiment branch from b39d0bd to 95c6bac Compare May 10, 2024 12:42

guorong009 merged commit 54a8fd8 into main May 11, 2024
16 checks passed

guorong009 deleted the gr@fix-compile-circuit-cs-experiment branch May 11, 2024 08:03

ed255 mentioned this pull request May 14, 2024

soundness bug: enabled selector returns a zero Expression #325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: roll back the `compress_selectors` logic #322

fix!: roll back the `compress_selectors` logic #322

guorong009 commented May 6, 2024 •

edited

Loading

ed255 left a comment

ed255 May 7, 2024

guorong009 May 8, 2024

ed255 May 7, 2024

adria0 commented May 7, 2024

ed255 commented May 7, 2024

ed255 left a comment

ed255 May 10, 2024

guorong009 May 10, 2024

ed255 May 10, 2024

guorong009 May 10, 2024 •

edited

Loading

ed255 commented May 10, 2024

fix!: roll back the compress_selectors logic #322

fix!: roll back the compress_selectors logic #322

Conversation

guorong009 commented May 6, 2024 • edited Loading

Description

Related issue

Changes

ed255 left a comment

Choose a reason for hiding this comment

ed255 May 7, 2024

Choose a reason for hiding this comment

guorong009 May 8, 2024

Choose a reason for hiding this comment

ed255 May 7, 2024

Choose a reason for hiding this comment

adria0 commented May 7, 2024

ed255 commented May 7, 2024

ed255 left a comment

Choose a reason for hiding this comment

ed255 May 10, 2024

Choose a reason for hiding this comment

guorong009 May 10, 2024

Choose a reason for hiding this comment

ed255 May 10, 2024

Choose a reason for hiding this comment

guorong009 May 10, 2024 • edited Loading

Choose a reason for hiding this comment

ed255 commented May 10, 2024

fix!: roll back the `compress_selectors` logic #322

fix!: roll back the `compress_selectors` logic #322

guorong009 commented May 6, 2024 •

edited

Loading

guorong009 May 10, 2024 •

edited

Loading