search

AleoNet / snarkVM

A Virtual Machine for Zero-Knowledge Executions
https://snarkvm.org
Apache License 2.0
1.08k stars 1.5k forks source link

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

Open ljedrz opened 3 months ago

ljedrz commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 1 month ago

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.