Switch to homogeneous coordinates + Add complete formulae #19
Conversation
- Pasta curves adapted to use homogenous instead of jacobian coordinates. - Efficient complete formulas for add and dbl.
|
NOTE for reviewers: The diff up to the line 400 is just a indentation correction and can be ignored. |
davidnevadoc
changed the title
There was a problem hiding this comment.
LGTM! I have double checked that the algorithms are identical with the zkcrypto/bls12_381 ones, except the mul_by_3b part (not sure how much performance it affects, might be cool to have some benchmark to see in the future).
Also tried to learn some stuff about different coordinates representation and couldn't realize why the conversion from/to jacobian is like that, so also tried to leave some thoughts here (they might not make sense at all, so appreciate any further resource about them!).
Co-authored-by: Han <tinghan0110@gmail.com>
Co-authored-by: Han <tinghan0110@gmail.com>
There was a problem hiding this comment.
Double checked against zkcrypto/bls12_381 the formulas as @han0110 did. (2 pairs of eyes might miss less than 1 🤣 )
Everything matches AFAIS. Just added a note regarding the 3mulb fn that I think could be extrapolated as a const so that we get some extra perf maybe (at least more than as a fn).
Finally, thanks @han0110 for your extensive and detailed review!! This thinks take a lot of time to do and you catched everything AFAIS.
Good work @davidnevadoc !!!!!! 👍
src/derive/curve.rs
Outdated
| #[inline] | ||
| fn curve_constant_3b() -> $base { | ||
| $constant_b + $constant_b + $constant_b | ||
| } |
There was a problem hiding this comment.
Why not having this as a const?? It's much better than compute 3 times in 3 places this value.
There was a problem hiding this comment.
I went for this approach since it is the one taken for b(). But I agree 100%, I think it would be better to store the value directly so it need not be computed with each call.
However I haven't find a way to do so without introducing a new argument in the macro. I'll look into it.
There was a problem hiding this comment.
Ohh yeye, there's no way. Unless you introduce a lazy_static call inside of the macro which creates the constant. This could be one approach.
You can also leave an issue filled for this and we can explore it later!
| let p = $name { | ||
| x:p_x, | ||
| y:$base::conditional_select(&p_y, &$base::one(), z.is_zero()), | ||
| z | ||
| }; |
There was a problem hiding this comment.
Just re-run the workflow, everything looking good.
The error it gave was Illegal instruction so I guess it is related with something else.
There was a problem hiding this comment.
LGTM.
See my suggestion in #19 (comment) and address it if you consider it
…aling-explorations#19) * Use homogenous coordinates - Pasta curves adapted to use homogenous instead of jacobian coordinates. - Efficient complete formulas for add and dbl. * Improve Homogeneous to Jacobian Co-authored-by: Han <tinghan0110@gmail.com> * Improve Jacobian to homogeneous Co-authored-by: Han <tinghan0110@gmail.com> * Remove leftover * Fix homogeneous coordinates for inf * Add static 3b --------- Co-authored-by: Han <tinghan0110@gmail.com>
This PR contains two main changes:
The reference algorithms can be found here: https://eprint.iacr.org/2015/1060.pdf
The implementations for the complete formulas were directly taken from here: https://github.com/zkcrypto/bls12_381/blob/11617d9dd1b67cc6e102c08d507e5e5e11b9fbdf/src/g1.rs#L670
Close #15