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

leading_coefficient() may unexpectedly be zero #796

Closed
moCello opened this issue Dec 14, 2023 · 2 comments · Fixed by #800
Closed

leading_coefficient() may unexpectedly be zero #796

moCello opened this issue Dec 14, 2023 · 2 comments · Fixed by #800

Comments

@moCello
Copy link
Member

moCello commented Dec 14, 2023

Summary

Polynomials are stored as a vector of BlsScalars. A polynomial is the zero polynomial if 1) it is empty or 2) all coefficients in the vector are zero. We don’t want to store extra zero coefficients if we don’t need to, so almost every function on polynomials makes sure to call self.truncate_leading_zeros() after each operation.

However, there are two exceptions: in add_assign and in sub_assign, if two polynomials have the same degree, then truncate_leading_zeros() is not called. It’s possible for the highest coefficients to cancel out, leaving us with a non-zero polynomial that still has a “zero” leading coefficient.

In 'fft/polynomial.rs', the leading_coefficient() function should return the largest non-zero coefficient of the polynomial. Currently, this function is marked as dead_code and not used anywhere in the repository, but future developers of this library could easily make a mistake here.
Recommendation: Either fix leading_coefficient() to ignore leading zeros, or change add_assign and sub_assign to call truncate_leading_zeros in the case of two polynomials with the same degree.

Relevant Context (optional)

Finding 1 of the plonk audit.

@moCello
Copy link
Member Author

moCello commented Dec 14, 2023

I would even go for both fixes:

  • Change add_assign and sub_assign to truncate leading zeros before returning
  • And also fix leading_coefficient to not return an unexpected zero coefficient

@moCello
Copy link
Member Author

moCello commented Dec 14, 2023

Additionally I propose to also truncate the leading zeros before returning a Polynomial::from_slice in 'fft/polynomial.rs' line 47.

moCello added a commit that referenced this issue Dec 14, 2023
moCello added a commit that referenced this issue Dec 14, 2023
moCello added a commit that referenced this issue Dec 14, 2023
moCello added a commit that referenced this issue Dec 18, 2023
moCello added a commit that referenced this issue Dec 18, 2023
moCello added a commit that referenced this issue Dec 18, 2023
moCello added a commit that referenced this issue Dec 18, 2023
moCello added a commit that referenced this issue Dec 18, 2023
moCello added a commit that referenced this issue Dec 18, 2023
moCello added a commit that referenced this issue Dec 19, 2023
@moCello moCello mentioned this issue Jan 3, 2024
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 a pull request may close this issue.

1 participant