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 CirclePointIndex bug in debug mode for 32-bit archs #613

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented May 3, 2024

This change is Reviewable

Copy link
Contributor Author

@andrewmilson andrewmilson left a 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-commenter
Copy link

codecov-commenter commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.41%. Comparing base (403c45c) to head (f070d14).

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.
📢 Have feedback on the report? Share it here.

@andrewmilson andrewmilson force-pushed the 05-02-Change_in_benchmarks_script branch from a553947 to d96abee Compare May 11, 2024 02:43
@andrewmilson andrewmilson force-pushed the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch from 2455d53 to 279a980 Compare May 11, 2024 02:44
@andrewmilson andrewmilson force-pushed the 05-02-Change_in_benchmarks_script branch from d96abee to 79d4eb3 Compare May 15, 2024 04:35
@andrewmilson andrewmilson force-pushed the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch from 279a980 to e6948f4 Compare May 15, 2024 04:35
@andrewmilson andrewmilson force-pushed the 05-02-Change_in_benchmarks_script branch from 79d4eb3 to 99939af Compare May 15, 2024 19:38
@andrewmilson andrewmilson force-pushed the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch from e6948f4 to d98f633 Compare May 15, 2024 19:38
@andrewmilson andrewmilson force-pushed the 05-02-Change_in_benchmarks_script branch from 99939af to 793628e Compare May 16, 2024 03:56
@andrewmilson andrewmilson force-pushed the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch from d98f633 to dbbd950 Compare May 16, 2024 03:56
@andrewmilson andrewmilson force-pushed the 05-02-Change_in_benchmarks_script branch from 793628e to 6c233c2 Compare May 16, 2024 15:24
@andrewmilson andrewmilson force-pushed the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch from dbbd950 to 05762bd Compare May 16, 2024 15:25
@andrewmilson andrewmilson force-pushed the 05-02-Change_in_benchmarks_script branch from 6c233c2 to 1ad5eef Compare May 16, 2024 16:47
@andrewmilson andrewmilson force-pushed the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch from 05762bd to 7e1ac4e Compare May 16, 2024 16:47
@andrewmilson andrewmilson force-pushed the 05-02-Change_in_benchmarks_script branch from 1ad5eef to 86303f4 Compare May 19, 2024 14:34
@andrewmilson andrewmilson force-pushed the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch from 7e1ac4e to 6e736a5 Compare May 19, 2024 14:34
Base automatically changed from 05-02-Change_in_benchmarks_script to dev May 19, 2024 14:38
@andrewmilson andrewmilson force-pushed the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch from 6e736a5 to f070d14 Compare May 19, 2024 14:40
@andrewmilson andrewmilson enabled auto-merge (squash) May 19, 2024 14:47
Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a 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?

Copy link
Contributor Author

@andrewmilson andrewmilson left a 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

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alonh5)

@andrewmilson andrewmilson merged commit 2f84421 into dev May 20, 2024
20 of 21 checks passed
Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: 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)

@andrewmilson andrewmilson deleted the 05-02-Fix_CirclePointIndex_bug_in_debug_mode_for_32-bit_archs branch May 20, 2024 14:00
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.

None yet

3 participants