Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct lagrange coefficient computation #611

Merged
merged 9 commits into from
Jun 26, 2024
Merged

Conversation

alxiong
Copy link
Contributor

@alxiong alxiong commented Jun 21, 2024

closes: #605

This PR:

Executed this plan: #605 (comment)
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.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added relevant changelog entries to the CHANGELOG.md of touched crates.
  • Re-reviewed Files changed in the GitHub PR explorer

@alxiong alxiong requested a review from chancharles92 June 21, 2024 14:28
@alxiong alxiong self-assigned this Jun 21, 2024
@alxiong alxiong requested a review from mrain as a code owner June 21, 2024 14:28
@alxiong alxiong requested a review from philippecamacho June 21, 2024 14:42
@alxiong alxiong changed the title bugfix: correct lagrange coefficient computation fix: correct lagrange coefficient computation Jun 21, 2024
@alxiong
Copy link
Contributor Author

alxiong commented Jun 21, 2024

@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?

plonk/src/lagrange.rs Outdated Show resolved Hide resolved
plonk/src/lagrange.rs Outdated Show resolved Hide resolved
plonk/src/lagrange.rs Outdated Show resolved Hide resolved
plonk/src/lagrange.rs Outdated Show resolved Hide resolved
plonk/src/lagrange.rs Outdated Show resolved Hide resolved
plonk/src/proof_system/prover.rs Show resolved Hide resolved
plonk/src/lagrange.rs Show resolved Hide resolved
@chancharles92
Copy link
Contributor

@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
Copy link
Contributor Author

alxiong commented Jun 24, 2024

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

@alxiong alxiong requested a review from chancharles92 June 24, 2024 16:04
plonk/src/lagrange.rs Outdated Show resolved Hide resolved
plonk/src/lagrange.rs Outdated Show resolved Hide resolved
plonk/src/lagrange.rs Show resolved Hide resolved
plonk/src/proof_system/verifier.rs Show resolved Hide resolved
@alxiong alxiong requested a review from mrain June 25, 2024 03:45
@alxiong alxiong merged commit 4bb3a2c into main Jun 26, 2024
5 checks passed
@alxiong alxiong deleted the alex/fix-lagrange-eval branch June 26, 2024 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: incorrect lagrange evaluation when zeta is roots of unity
3 participants