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 poly degree 0 underflow #10

Merged
merged 8 commits into from
Oct 22, 2023
Merged

fix poly degree 0 underflow #10

merged 8 commits into from
Oct 22, 2023

Conversation

smtmfft
Copy link

@smtmfft smtmfft commented Oct 13, 2023

substract underflow if poly's degree is 0, which happens if we have an empty/constant value gate.

Another issue is if I don't update the rust-toolchain to 1.66, I got:

error: package `constant_time_eq v0.3.0` cannot be built because it requires rustc 1.66.0 or newer, while the currently active rustc version is 1.64.0

@einar-taiko einar-taiko self-requested a review October 16, 2023 19:44
@einar-taiko
Copy link

Great work! I specially like the test case 💫

Copy link

@einar-taiko einar-taiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct.

Please treat my comments as suggestions.

halo2_proofs/src/plonk/evaluation.rs Outdated Show resolved Hide resolved
halo2_proofs/src/plonk/evaluation.rs Show resolved Hide resolved
halo2_proofs/src/fft/parallel.rs Show resolved Hide resolved
halo2_proofs/src/plonk/evaluation.rs Show resolved Hide resolved
rust-toolchain Show resolved Hide resolved
Copy link

@einar-taiko einar-taiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about running cargo clippy --fix on the whole thing as well?

let mut idx = 0;
if degree != 0 {
// same as unstable degree.ilog()
idx = (degree as f32).log2().floor() as usize;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch with the versioning.

I would prefer idx = (31 - (degree as u32).leading_zeros()) as usize over introducing floating-point math.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP, let's back to the origin code :)

@smtmfft
Copy link
Author

smtmfft commented Oct 20, 2023

What do you think about running cargo clippy --fix on the whole thing as well?

I guess it should be ok, let me try

@smtmfft
Copy link
Author

smtmfft commented Oct 22, 2023

All CI & lint issues are addressed!

@smtmfft smtmfft merged commit fb69aa2 into taiko/unstable Oct 22, 2023
12 checks passed
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.

2 participants