Add new unchecked #1024
Add new unchecked #1024
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1024 +/- ##
==========================================
- Coverage 70.17% 70.16% -0.01%
==========================================
Files 165 165
Lines 32770 32773 +3
==========================================
- Hits 22996 22995 -1
- Misses 9774 9778 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Do we have a benchmark? |
I've run the Pedersen benchmark locally (MacOS, Apple M3 Pro, 18 GB of RAM): Command Results
|
crates/math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs
Show resolved
Hide resolved
| // SAFETY: The values `x_hex, y_hex`` should be constants valid for the curve. | ||
| ShortWeierstrassProjectivePoint::new_unchecked([ |
There was a problem hiding this comment.
There's nothing in the method stating that. The name doesn't confer that this might silently give wrong results. I don't this is correct without also adding _unchecked to the method's name.
There was a problem hiding this comment.
You're right, renamed the function in f411abd also improved the SAFETY comment
| /// Creates an elliptic curve point giving the projective [x: y: z] coordinates without | ||
| /// checking that the point satisfies the curve equation. | ||
| pub const fn new_unchecked(value: [FieldElement<E::BaseField>; 3]) -> Self { | ||
| Self(ProjectivePoint::new(value)) | ||
| } | ||
|
|
||
| /// Changes the point coordinates without checking that it satisfies the curve equation. | ||
| pub fn set_unchecked(&mut self, value: [FieldElement<E::BaseField>; 3]) { | ||
| self.0.value = value | ||
| } | ||
|
|
There was a problem hiding this comment.
These should also have the following (or similar) SAFETY comment:
// SAFETY: the caller MUST be sure the arguments correspond to a point in the curve.
// Failing to do so will result in silently wrong values.There was a problem hiding this comment.
Added SAFETY comment in f411abd
| /// Creates an elliptic curve point giving the projective [x: y: z] coordinates without | ||
| /// checking that the point satisfies the curve equation. | ||
| pub const fn new_unchecked(value: [FieldElement<E::BaseField>; 3]) -> Self { | ||
| Self(JacobianPoint::new(value)) | ||
| } |
There was a problem hiding this comment.
This also needs a SAFETY comment explaining the requirements.
There was a problem hiding this comment.
Added SAFETY comment in f411abd
Use new_unchecked
Description
This PR replaces the use of
newwithnew_uncheckedin cases where the points are already known to be valid.It also applies
new_uncheckedto the generator point inStarkCurve, since it is a verified valid point