Change elliptic curve new point function #970
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #970 +/- ##
==========================================
+ Coverage 71.49% 71.65% +0.15%
==========================================
Files 156 156
Lines 33969 34235 +266
==========================================
+ Hits 24287 24531 +244
- Misses 9682 9704 +22 ☔ View full report in Codecov by Sentry. |
| @@ -15,4 +17,16 @@ pub trait IsEdwards: IsEllipticCurve + Clone + Debug { | |||
| - FieldElement::<Self::BaseField>::one() | |||
| - Self::d() * x.pow(2_u16) * y.pow(2_u16) | |||
| } | |||
|
|
|||
| // Edwards equation in projective coordinates. | |||
| // a * x^2 * z^2 + y^2 * z^2 - z^4 = d * x^2 * y^2 | |||
There was a problem hiding this comment.
This could be more efficient just by factoring out z^2. We can update that in future PR
There was a problem hiding this comment.
Overall it looks correct, however:
- Did we check that the use of
.unwrap_unchecked()actually provides any advantage over regular.unwrap()? - I would suggest, regardless of which kind of
unwrapwe use, that we do something like the following:
let val = Curve::new(...);
debug_assert!(val.is_some());
val.unwrap()That way we can make sure the condition holds in debug builds, without introducing any checking overhead in release builds.
| let a_g1 = BN254Curve::generator().operate_with_self(a_val).to_affine(); | ||
| let b_g1 = BN254Curve::generator().operate_with_self(b_val).to_affine(); | ||
|
|
||
| let a_g2 = BN254TwistCurve::generator() | ||
| .operate_with_self(b_val) | ||
| .to_affine(); | ||
| let b_g2 = BN254TwistCurve::generator() | ||
| .operate_with_self(b_val) | ||
| .to_affine(); |
There was a problem hiding this comment.
I believe this changes the whole meaning of the benchmark. E.g., the first two bench runs are precisely for converting to affine, which becomes trivial with the change.
| // - `unwrap_unchecked()` avoids unnecessary checks, as we guarantee | ||
| // correctness based on external verification. |
There was a problem hiding this comment.
| // - `unwrap_unchecked()` avoids unnecessary checks, as we guarantee | |
| // correctness based on external verification. |
No longer relevant, we now use the regular unwrap.
| /// | ||
| /// - The generator values are taken from the [Arkworks implementation](https://github.com/arkworks-rs/curves/blob/5a41d7f27a703a7ea9c48512a4148443ec6c747e/ed_on_bls12_381_bandersnatch/src/curves/mod.rs#L120) | ||
| /// and have been converted to hexadecimal. | ||
| /// - `unwrap_unchecked()` is safe because: |
There was a problem hiding this comment.
| /// - `unwrap_unchecked()` is safe because: | |
| /// - `unwrap()` does not panic because: |
| FieldElement::<Self::BaseField>::new_base( | ||
| "29C132CC2C0B34C5743711777BBE42F32B79C022AD998465E1E71866A252AE18", | ||
| ), | ||
| FieldElement::<Self::BaseField>::new_base( | ||
| "2A6C669EDA123E0F157D8B50BADCD586358CAD81EEE464605E3167B6CC974166", | ||
| ), | ||
| FieldElement::one(), | ||
| ]) | ||
| ]); | ||
| debug_assert!(point.is_ok()); |
There was a problem hiding this comment.
| debug_assert!(point.is_ok()); |
No longer necessary, .unwrap() has the exact same effect.
Change elliptic curve new point function
Description
This PR changes the function
newfor every elliptic curve in Lambdaworks. In the previous version, this function creates a new curve point but doesn't check if it satisfies the curve equation. In this PR the function checks if the point is valid returning aResult.