Remove zk

[spherel]

Summary:

To remove zero-knowledge, we need to

1. change the blinding_factors to be 0. So the column structure should be [usable_rows, last_row].
2. remove randomness from the end of each witness column and expression (but the last row is still out of usable rows so sometimes padding a zero is needed).
3. remove the random_poly and its commitment in the vanishing argument.

examples/simple-example-4.rs constructs a circuit with 2^k - 1 rows.

[kilic]

First look: I'd be rather in favor of trait level support for the zk property rather than compile time feature. Mostly because in the same session some circuits might need zk while for others zk can be turned off. Especially thinking about recursion graphs that we want to support in future probably developer would want to turn on zk for some layers and turn off for other levels.

[spherel]

First look: I'd be rather in favor of trait level support for the zk property rather than compile time feature. Mostly because in the same session some circuits might need zk while for others zk can be turned off. Especially thinking about recursion graphs that we want to support in future probably developer would want to turn on zk for some layers and turn off for other levels.

Would that be better if we set a flag in Circuit and do the work related to zk (such as calculating blinding factor & extend randomness to the end of polynomials) in the ConstraintSystem of which the behaviors are adjusted by this flag?

[kilic]

I'm not completely sure whether we should define the behavior by a boolean flag or by traits. However traits sounds to me a bit more useful than passing flags around. There also might be more customization in future so there would be many flags and conditionals around.

[spherel]

Make sense to me. I will change that.

[spherel]

By the way, I just found Plonky2 just had the same idea as me: they add a boolean flag in a circuit config and use this to change behaviors of the constraint builder. Here is their code FYI: https://github.com/mir-protocol/plonky2/blob/e10e103933b76d22dcc76b75a4065008d3ce39c0/plonky2/src/plonk/circuit_data.rs#L44

[CPerezz]

I like this approach of the feature quite more than the one of the flag.

At the end, there's no use case where we might want to have a circuit compiled with and without zk properties. And if so, we can compile it twice.

So I'd rather take the feature approach as done here and not add any changes to the halo2 API.

LGTM. WDYT @kilic ??

[CPerezz]

First look: I'd be rather in favor of trait level support for the zk property rather than compile time feature. Mostly because in the same session some circuits might need zk while for others zk can be turned off. Especially thinking about recursion graphs that we want to support in future probably developer would want to turn on zk for some layers and turn off for other levels.

What would be the benefit of a trait level ZK property? To define it by circuit instead of binary level? Which can be the use case for the trait-based ZK property??

Do it with a trait is viable of course. It's just that I don't really see the use case for it.

[kilic]

@CPerezz I feel like this fork in the medium run can be a community preference to develop plonk based circuits with various customizations. I agree that in our case (zkevm) we can even remove whole zk stuff. It's not of course crucial to make it trait optional for now and maybe I'm just overthinking. However I've imagined bundle of circuits which have some relationship between them and some of them requires zk some of them not. So for example while synthesizing aggregation circuit it would probably be required know which circuit is zk which is not.

Here we have three choices. 1. compile time flag (this PR) 2. boolean flag that @spherel suggest 3. trait level support that I'd think would be the best practice but I can be wrong. I think in circuit definition what customization is followed somehow should be known like option 2 and 3. So I'm also open for booleans.

[spherel]

I have a question: since for circuits with or without zero-knowledge property will have different usable rows, can they still be aggregated together? I am not quite familiar with the aggregation, will it generate a proof of the verification circuit as a part of the whole proof or will it use polynomial commitment to link two circuits?

[kilic]

@spherel Yes they can be aggregated together. We mostly process commitments to the columns (witness or fixed) in aggregation side which are actually some part of the verification algorithm. So content of rows or number of rows shouldn't effect verification algorithm.

will it generate a proof of the verification circuit as a part of the whole proof

And yes aggregation circuit is basically shifting some of verification work of bundle of circuits to prover side

[lispc]

is it better to use brace to put them together? like

#[cfg(feature = "zero-knowledge")]
{
  let random_eval = eval_polynomial(&self.committed.random_poly, *x);
  transcript.write_scalar(random_eval)?;
}

more easy to read code

[CPerezz]

@CPerezz I feel like this fork in the medium run can be a community preference to develop plonk based circuits with various customizations. I agree that in our case (zkevm) we can even remove whole zk stuff. It's not of course crucial to make it trait optional for now and maybe I'm just overthinking. However I've imagined bundle of circuits which have some relationship between them and some of them requires zk some of them not. So for example while synthesizing aggregation circuit it would probably be required know which circuit is zk which is not.

Here we have three choices. 1. compile time flag (this PR) 2. boolean flag that @spherel suggest 3. trait level support that I'd think would be the best practice but I can be wrong. I think in circuit definition what customization is followed somehow should be known like option 2 and 3. So I'm also open for booleans.

I see @kilic .
IMO, anyhting that is not bool flags works for me.
I'm not a fan of the flags as they will really change the API and also, we can end up with multiple bool flags which is a mess..

So we can go with the trait if you're more in favour of it. I like it too. But before changing this PR, would be nice to see a design first so that we can all check it and comment on it before is implemented.

WDYT @kilic @spherel ?

[kilic]

Closing due to #76