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

[refactor] Use explicit wrapping addition in Fp*::mul_assign #2533

Draft
wants to merge 1 commit into
base: mainnet-staging
Choose a base branch
from

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Aug 22, 2024

While overflowing addition will not panic in --release mode (it will instead wrap around), it will cause a crash under --debug. This PR switches to explicit wrapping addition for the mul_assign method for Fp256 and Fp384.

Found via fuzzing.

@vicsn
Copy link
Contributor

vicsn commented Aug 22, 2024

Can you share the error you saw, was it "Failed to eject scalar value: The scalar is greater than or equal to the modulus." by any chance?

@ljedrz
Copy link
Collaborator Author

ljedrz commented Aug 26, 2024

@vicsn sadly, this has only occurred in one of my initial runs, where I built with --debug by accident (since it is not the intended way of building snarkVM), so I no longer have those artifacts. That being said, I consider this a cleanup (as opposed to a fix), as this is what we are already de facto doing except for some of the tests (which is what the linked PR noted).

@vicsn
Copy link
Contributor

vicsn commented Oct 1, 2024

Currently I'd recommend to close this PR. Perhaps for now you can draft it.

Reason is that IF the wrapping behaviour is not something we actually want, it would be nice if it will at least show up in a panicking test.

I didn't evaluate whether the wrapping is acceptable or reachable under normal conditions. One day that part of the code can be re-tested and re-reviewed.

@ljedrz ljedrz marked this pull request as draft October 1, 2024 15:16
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