search

privacy-scaling-explorations / chiquito

DSL for Halo2 circuits
https://docs.pecadorplonkish.xyz/
185 stars 39 forks source link

Compile to Halo2 middleware #263

Closed alxkzmn closed 2 months ago

alxkzmn commented 3 months ago

Switching to a more recent Halo2 version revealed a problem with our lookups - they don't pass a lookup-any-sanity-checks because they are using only the fixed query. This is why I had to disable the default features of all halo2 crates (that include lookup-any-sanity-checks) and only enable the rest. I suppose we can refactor our lookups to use lookup instead of lookup_any.

leolara commented 3 months ago

@alxkzmn something that for me is important is to check that we can use the "mock prover" for the new middleware that @ed255 mentioned. Can you check that we can use that and that we get the information we need to find bugs in circuits.

alxkzmn commented 3 months ago

Sure

alxkzmn commented 3 months ago

@leolara regarding the debug prover - according to Edu it is already doing the job. Here's the information he provided: the new check_witness function and the example. I'll wait until it's merged into main. I think it's safe to remove the fronted references already. Some of the remaining frontend code is preventing me from refactoring the changes in a better way.

leolara commented 2 months ago

@alxkzmn I know that it is impossible that two people name things the same, but I always use get_ to obtain things that are already calculated as something that is going to be called many times every time I want a value. not for something that is acually running things.

perhaps to_halo2() could be good, a get_preprocessing, just preprocessing. This is not important, really.

leolara commented 2 months ago

@alxkzmn why does setup needs args I thought one of the advantes of this is that we could get the setup without wittness and then pass the witness to the prover

alxkzmn commented 2 months ago

@leolara ready for review, fixed everything. Regarding inferring the k - there's no information on the number of rows in the plonkish compilation itself or the supercircuit. It only becomes available after we actually generate the witness, implying that we would need the witness for the setup phase. I can try to implement it if it sounds good to you.

leolara commented 2 months ago

@alxkzmn i guess the prints in the library you are going to remove them before merging, right? They are ok in the examples and tests, but a library cannot have prints

leolara commented 2 months ago

@alxkzmn num_rows is available in the compilation of plonkish but not put in the IR, it can be added to the IR trivially

leolara commented 2 months ago

@alxkzmn what you mean by refactor lookups in using lookup instead of lookup_any.

In plonkish IR the lookups are:

pub lookups: Vec<PolyLookup<F>>,

We should ignore 100% how the previous halo2 frontend was used. And create a new backend that goes to the halo2 middleware.

alxkzmn commented 2 months ago

K could be derived from the number of rows before this refactoring, so I created a separate issue for that: https://github.com/privacy-scaling-explorations/chiquito/issues/266 Also created a separate issue for the lookup refactoring: https://github.com/privacy-scaling-explorations/chiquito/issues/267

leolara commented 2 months ago

@alxkzmn I am not sure why you are not using the SuperAssignments type, but it is approved, don't worry about it