-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add endo scalar decomposition #24
Conversation
a62397a
to
32f2d39
Compare
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: |
src/arithmetic.rs
Outdated
$acc += if $e.is_positive() { | ||
$table[idx] | ||
} else if $e.is_negative() { | ||
-$table[idx] | ||
} else { | ||
C::identity() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we bench the diff? Or not worth?
src/arithmetic.rs
Outdated
.iter() | ||
.rev() | ||
.zip(k2_wnaf.iter().rev()) | ||
.fold(C::identity(), |acc, (e1, e2)| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/arithmetic.rs
Outdated
k1_wnaf | ||
.iter() | ||
.rev() | ||
.zip(k2_wnaf.iter().rev()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we reverse k2 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are both reversed to apply the sliding window mul algorithm starting from the less significant bit.
src/arithmetic.rs
Outdated
#[test] | ||
fn test_wnaf_form() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this test inside of wnaf
mod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the explanation!
@@ -109,6 +115,20 @@ const G2_GENERATOR_Y: Fq2 = Fq2 { | |||
]), | |||
}; | |||
|
|||
const ENDO_PARAMS: EndoParameters = EndoParameters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is resolved with de71b18
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot 👍
src/arithmetic.rs
Outdated
k1_wnaf | ||
.iter() | ||
.rev() | ||
.zip(k2_wnaf.iter().rev()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot 👍
Adds scalar decomposition for endomorphism for pasta and bn curves. Now we should be able to rebase privacy-scaling-explorations/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