EspressoSystems / jellyfish

A Rust Implementation of the PLONK ZKP System and Extensions
https://jellyfish.docs.espressosys.com
MIT License
408 stars 106 forks source link

refactor: use BigEndian for SolidityTranscript #619

Closed alxiong closed 4 months ago

alxiong commented 5 months ago

closes: #618

This PR:

Changes behavior of SolidityTranscript, see the linked issue for description

This PR does not:

Question: should we also optionally append tau for circuits that does support lookup? currently in contract, we have to do a useless get_and_append_challenge call for this unused challenge.

@chancharles92 @philippecamacho


Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why.

alxiong commented 5 months ago

Somehow there's still one test failed after ce7cff8.

after some digging, I'm unable to locate the cause. @chancharles92 you are probably more familiar with this gadget, do you have any clue where did i forget to update?


update: fixed in 44b7c42

alxiong commented 5 months ago

reminder for myself after I address comments for this PR:

philippecamacho commented 4 months ago

Should not we add an if statement to raise an error here if the plonk instance does not support lookups?

alxiong commented 4 months ago

because anyway it's not used to construct the hasher.

yes, but previously, we were using it. in this PR, we removed it.

Same field also exists in the RescueTranscript, we should consider removing that as well.

that would require refactoring of the recursion circuit, since we are not using it atm, maybe we leave it as it is for now.

cc @mrain

alxiong commented 4 months ago

Should not we add an if statement to raise an error here if the plonk instance does not support lookups?

we have already does the protection from the caller side: in snark.rs

            let plookup_evals = if circuits[i].support_lookup() {
                let evals = prover.compute_plookup_evaluations(
                    prove_keys[i],
                    &challenges,
                    &online_oracles[i],
                )?;
                transcript.append_plookup_evaluations::<E>(&evals)?;
                Some(evals)
            } else {
                None
            };

we only invoke it if .supports_lookup() evaluates to true. cc @philippecamacho

alxiong commented 4 months ago

maintainer check: @mrain are you happy with jf-plonk-only bump in 4c421e7?

(if so, i'll create a tag named jf-plonk-v0.5.0 later)