EspressoSystems / jellyfish

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

fix: correct lagrange coefficient computation #611

Closed alxiong closed 1 month ago

alxiong commented 1 month ago

closes: #605

This PR:

Executed this plan: https://github.com/EspressoSystems/jellyfish/issues/605#issuecomment-2180773751 and updated all previously lagrange coeffs computation

This PR does not:

Update anything about recursive circuit, since the occurrence of the challenge zeta being one of the domain elements is negligibly small, not sure if we want to burden our recursion circuit to include the extra branch condition.


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 1 month ago

@chancharles92 one thing I need to get your opinion:

in verifier.rs::evaluate_pi_poly()

        // TODO: (alex) deal with merged circuit later

        if circuit_is_merged {
            let n = self.domain.size();
            for (i, val) in pub_input.iter().skip(len).enumerate() {
                let lagrange_n_minus_i = vanish_eval_div_n * self.domain.element(n - i - 1)
                    / (*z - self.domain.element(n - i - 1));
                result += lagrange_n_minus_i * val;
            }
        }

I have left a todo, where the current trait LagrangeCoeffs doesn't handle this logic blackboxly. So I left it untouched. Do you have any suggestion?

chancharles92 commented 1 month ago

@chancharles92 one thing I need to get your opinion:

in verifier.rs::evaluate_pi_poly()

        // TODO: (alex) deal with merged circuit later

        if circuit_is_merged {
            let n = self.domain.size();
            for (i, val) in pub_input.iter().skip(len).enumerate() {
                let lagrange_n_minus_i = vanish_eval_div_n * self.domain.element(n - i - 1)
                    / (*z - self.domain.element(n - i - 1));
                result += lagrange_n_minus_i * val;
            }
        }

I have left a todo, where the current trait LagrangeCoeffs doesn't handle this logic blackboxly. So I left it untouched. Do you have any suggestion?

How about just adding a simple handle when *z = self.domain.element(n - i - 1)?

alxiong commented 1 month ago

How about just adding a simple handle when *z = self.domain.element(n - i - 1)?

just realized that we can use lagrange_for_range for this, see f8f2d5f