evmmax: Add inversion method#1142
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1142 +/- ##
==========================================
+ Coverage 94.56% 94.57% +0.01%
==========================================
Files 168 168
Lines 18283 18320 +37
==========================================
+ Hits 17290 17327 +37
Misses 993 993
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
pdobacz
left a comment
There was a problem hiding this comment.
I didn't dig into the details of the algo yet, if you want me to, lmk.
Checking the linked paper would be more than welcome as we plan to apply more optimizations from it. But this is not urgent. |
pdobacz
left a comment
There was a problem hiding this comment.
still one topic to maybe discuss, but otherwise still LGTM
| // Bézout's coefficients are originally initialized to 1 and 0. But because the input x | ||
| // is in Montgomery form XR the algorithm would compute X⁻¹R⁻¹. To get the expected X⁻¹R, | ||
| // we need to multiply the result by R². Do this by initializing u to R². | ||
| UintT u = m_r_squared; |
There was a problem hiding this comment.
out of curiosity - not easier to correct for this at the end? Any external source to cite that this variation is equivalent?
There are a lot of modification to the linked classical algorithm, could do away with at least one. The others are kinda easier to warp head around.
There was a problem hiding this comment.
We borrowed the idea from zksync implementation of some precompiles. Maybe it would be equivalent to do this in the end, but that would be additional multiplication (here this is free). But I will try :)
There was a problem hiding this comment.
"Fixing" v after works, but it requires 2 multiplications by R² (because of the performed reduction):
v = mul(v, m_r_squared);
v = mul(v, m_r_squared);
return v;There was a problem hiding this comment.
Or in other words: v = mul(v, to_mont(m_r_squared));.
There was a problem hiding this comment.
I'm OK with keeping as is, if we have a solid reference to cite and confidence all the edge cases are also correctly handled (and later tested).
Add method for computing modular inversion using extended binary Euclidean algorithm. This algorithm, even in a simple form, is significantly faster (~5x) than computing the inversion as pow(x, p-2, p) by using a dedicated addchain of MULs.
Add method for computing modular inversion using extended binary Euclidean algorithm. This algorithm, even in a simple form, is significantly faster (~5x) than computing the inversion as pow(x, p-2, p) by using a dedicated addchain of MULs.
Add method for computing modular inversion using extended binary Euclidean algorithm. This algorithm, even in a simple form, is significantly faster (~5x) than computing the inversion as pow(x, p-2, p) by using a dedicated addchain of MULs.