Conversation
mmagician
requested review from
Pratyush and
weikengchen
and removed request for
a team
weikengchen
left a comment
weikengchen
left a comment
There was a problem hiding this comment.
Do you want to check if it is equivalent to the original one?
|
We can probably remove the check for whether 2nd Miller loop is negative. The parameter X^2 - X - 1 > 0, unless |
|
@weikengchen They're indeed equivalent. You can check by substituting the right branches into: https://github.com/mmagician/bw6-comparison. |
Merge queue setting changed
|
Hm the tests seem to be failing right now |
|
@Pratyush The parameters for loop counts changed, so the tests will fail against the current master of arkworks/curves. I've updated both sides today though, and they are all green - I can add a temp patch to Cargo here to point to the compatible branch in curves. |
|
I think multi-pairings often with some frequency, especially for batch-verification. |
|
I ran some benchmarks, and it seems that the new version from this PR outperforms the current master, even for multi-pairings. Some numbers for BW6-767: Where the improvement % actually increases with number of pairs! Similar results for BW6-761: I'm not quite sure what to make of this given the notes in https://hackmd.io/@gnark/BW6-761-changes. I suppose it comes down to the implementation differences across the gnark & arkworks at some different level in the stack. In any case, I think the numbers suggest we can safely call this PR an improved version. |
ec/src/models/bw6/mod.rs
Outdated
|
|
||
| let f_u_inv; | ||
|
|
||
| // TODO: is it enough to get the inverse of f_1, or does f_u also need to get inverted? |
There was a problem hiding this comment.
Do we need to address these TODOs before merging?
There was a problem hiding this comment.
Ah right, I missed this. The TODO no longer makes much sense actually - now it's saying exactly the opposite of what's happening in the code. I think this might have been a leftover of some of my initial attempts to understand the original code, which was indeed inverting f_1 (although at that point, there was no such thing as f_u, so I'm not so sure of that theory now). Now the ATE_LOOP_COUNT_1 is only used for computing f_u, so if the count was negative, I think only f_u should be affected.
The good thing is that now we have two curves with opposite sign for ATE_LOOP_COUNT_1 - so at least both arms of this if/else are tested.
I'll remove the TODO.
| if bit == 1 { | ||
| f *= &f_u; | ||
| for &mut (p, ref mut coeffs) in pairs.iter_mut() { | ||
| BW6::<Self>::ell(&mut f, &coeffs.next().unwrap(), &p.0); | ||
| } | ||
| } else if bit == -1 { | ||
| f *= &f_u_inv; |
There was a problem hiding this comment.
The for loops in both cases are the same, so they can be extracted to outside the loop, no?
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
| ell_coeffs_1.push(r.add_in_place(&q)); | ||
| } | ||
| } | ||
| // TODO: this is probably the slowest part |
There was a problem hiding this comment.
I think we can leave this TODO in, it's potential for even more future optimizations
This reverts commit 26bbf90.
3 participants
Description
As per Algorithm 5 in https://eprint.iacr.org/2020/351.pdf.
TODO: check whether the inverse should apply to
f_uorf_1in case the loop count is negative.@yelhousni maybe you could help?
Benchmarks:
Before:
After:
full pairing is ~11% faster.
closes: #616
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pendingsection inCHANGELOG.mdFiles changedin the GitHub PR explorer