Add endo scalar decomposition#24
Conversation
kilic
force-pushed
the
update/pasta_0.5.0
branch
from
a62397a to
32f2d39
Compare
CPerezz
force-pushed
the
update/pasta_0.5.0
branch
2 times, most recently
from
694b451 to
89021ff
Compare
|
Where can we check the endomorfism docs from which you derived the impl? Is the endomorfism formalized somewhere? |
|
Documents:
Implementations that helped: |
| $acc += if $e.is_positive() { | ||
| $table[idx] | ||
| } else if $e.is_negative() { | ||
| -$table[idx] | ||
| } else { | ||
| C::identity() | ||
| }; |
There was a problem hiding this comment.
I'd try a match statement here. It maybe brings tiny bit more performance than if-else chains
There was a problem hiding this comment.
Should we bench the diff? Or not worth?
| .iter() | ||
| .rev() | ||
| .zip(k2_wnaf.iter().rev()) | ||
| .fold(C::identity(), |acc, (e1, e2)| { |
There was a problem hiding this comment.
| k1_wnaf | ||
| .iter() | ||
| .rev() | ||
| .zip(k2_wnaf.iter().rev()) |
There was a problem hiding this comment.
Why do we reverse k2 here?
There was a problem hiding this comment.
They are both reversed to apply the sliding window mul algorithm starting from the less significant bit.
| #[test] | ||
| fn test_wnaf_form() { |
There was a problem hiding this comment.
Maybe move this test inside of wnaf mod?
There was a problem hiding this comment.
I added glv multiplication as an example application of endo decomposition and whole thing is in test mod. And I think it can be removed. We can change ecc mul with another PR. However it seems like single multiplication hasn't been a bottlenck for us that much so I think it is better to keep the original and simple one unless such change is strictly necessary
davidnevadoc
left a comment
davidnevadoc
left a comment
There was a problem hiding this comment.
Overall LGTM! Great work :)
I have made some suggestions to add some comments and references to make the code more understandable.
| }; | ||
|
|
||
| let is_neg = |e: &$field| { | ||
| let e = to_limbs(e); |
There was a problem hiding this comment.
I am unsure of the criteria of this function. Could you give some further explanation please? :)
Do we get is_neg(e) when e[3] >0?
Aren't the 3 first assignments of borrow always 0?
There was a problem hiding this comment.
This trick actually reveals whether a element is ~128 bit or ~F::NUM_BITS. if latter we see that element is negative and subtract it from modulus and get the actual ~128 bit element. It was originally like below
let (_, borrow) = sbb(0xffffffffffffffff, e[0], 0);
let (_, borrow) = sbb(0xffffffffffffffff, e[1], borrow);
let (_, borrow) = sbb(0x00, e[2], borrow);
let (_, borrow) = sbb(0x00, e[3], borrow);
However in some fields we hit scalars decomposed into 129 bits so I just also made third control limb sparse�
There was a problem hiding this comment.
I see, thanks for the explanation!
| ]), | ||
| }; | ||
|
|
||
| const ENDO_PARAMS: EndoParameters = EndoParameters { |
There was a problem hiding this comment.
Are these derived directly or taken form some implementation/ article? I think it would be good to add some reference in any case.
I also think it would be good to add a reference to some resource that explains what these are. Maybe something like:
// https://www.iacr.org/archive/crypto2001/21390189.pdf
// Vectors v1, v2 described in Section 4.
Without some indication I know future me will forget what these are x)
| macro_rules! endo { | ||
| ($name:ident, $field:ident, $params:expr) => { | ||
| impl CurveEndo for $name { | ||
| fn decompose_scalar(k: &$field) -> (u128, bool, u128, bool) { |
There was a problem hiding this comment.
Looks good! However there are some non-obvious optimizations that make the algorithm quite hard to follow.
I gave some explanation of what I believe is going on in this note
I think some comments should be added so that working in this code is easier in the future.
|
Can we fix the changes and merge the PR @kilic ? |
davidnevadoc
left a comment
davidnevadoc
left a comment
There was a problem hiding this comment.
LGTM! Thanks a lot 👍
| k1_wnaf | ||
| .iter() | ||
| .rev() | ||
| .zip(k2_wnaf.iter().rev()) |
There was a problem hiding this comment.
They are both reversed to apply the sliding window mul algorithm starting from the less significant bit.
| }; | ||
|
|
||
| let is_neg = |e: &$field| { | ||
| let e = to_limbs(e); |
There was a problem hiding this comment.
I see, thanks for the explanation!
davidnevadoc
left a comment
davidnevadoc
left a comment
There was a problem hiding this comment.
LGTM! Thanks a lot 👍
3 participants
Adds scalar decomposition for endomorphism for pasta and bn curves. Now we should be able to rebase privacy-ethereum/halo2#40. Also in tests there is a naive glv multiplication applilcation. I think it should not be replaced with the present multiplication function since that one also applies constant time multiplication. Closes #17