-
Notifications
You must be signed in to change notification settings - Fork 89
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 CirclePointIndex bug in debug mode for 32-bit archs #613
Fix CirclePointIndex bug in debug mode for 32-bit archs #613
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion
crates/prover/src/core/circle.rs
line 272 at r1 (raw file):
fn mul(self, rhs: usize) -> Self::Output { Self(self.0.wrapping_mul(rhs)).reduce()
Test examples::fibonacci::tests::test_mixed_degree_multi_fibonacci
caused an overflow here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #613 +/- ##
=======================================
Coverage 93.41% 93.41%
=======================================
Files 84 84
Lines 11909 11909
Branches 11909 11909
=======================================
Hits 11125 11125
Misses 702 702
Partials 82 82 ☔ View full report in Codecov by Sentry. |
ea76ec0
to
d1d1245
Compare
279a980
to
e6948f4
Compare
79d4eb3
to
99939af
Compare
e6948f4
to
d98f633
Compare
99939af
to
793628e
Compare
d98f633
to
dbbd950
Compare
793628e
to
6c233c2
Compare
dbbd950
to
05762bd
Compare
6c233c2
to
1ad5eef
Compare
05762bd
to
7e1ac4e
Compare
1ad5eef
to
86303f4
Compare
7e1ac4e
to
6e736a5
Compare
6e736a5
to
f070d14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @andrewmilson)
crates/prover/src/core/circle.rs
line 256 at r1 (raw file):
fn add(self, rhs: Self) -> Self::Output { Self(self.0 + rhs.0).reduce()
Do we need also wrapping_add here?
Code quote:
self.0 + rhs.0
crates/prover/src/core/circle.rs
line 272 at r1 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
Test
examples::fibonacci::tests::test_mixed_degree_multi_fibonacci
caused an overflow here.
how it didn't overflow up until now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @shaharsamocha7)
crates/prover/src/core/circle.rs
line 256 at r1 (raw file):
Previously, shaharsamocha7 wrote…
Do we need also wrapping_add here?
We don't because all values are in the range [0, 2^31]
crates/prover/src/core/circle.rs
line 272 at r1 (raw file):
Previously, shaharsamocha7 wrote…
how it didn't overflow up until now?
Because it's usize. All the platforms we had currently tested on where 64 bit. Since we now test Wasm (tests added to CI in the next PR) which is 32 bit it caused overflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5)
crates/prover/src/core/circle.rs
line 256 at r1 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
We don't because all values are in the range [0, 2^31]
*[0, 2^31)
This change is