Optimize & fix Edwards XYZT operations #1693
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR combines a few improvements to the EdwardsXYZT point operations, in both the Gallina & Bedrock versions.
Eliminates an addition by halving the precomputed coordinates
This PR switches to the following precomputed coordinate representation:
With this representation, we no longer need to compute
D = 2*Z1
in the mixed addition function (https://hyperelliptic.org/EFD/g1p/data/twisted/extended-1/addition/madd-2008-hwcd-3)This optimization is sourced from Section 3.2 of https://www.shiftleft.org/papers/fff/fff.pdf
Sometimes eliminates a multiplication by storing the components of T individually
Quoting from Section 3.2 of https://www.shiftleft.org/papers/fff/fff.pdf:
In this code, I call the components
Ta
andTb
instead, since we already useT1
andT2
(orX1
andX2
, etc) to refer to theT
coordinates of the two inputs to addition.Fixes a bug in the calculation of T in m1add_precomputed_coordinates
Previously, we calculated `T = (A-B)(A+B) by:
However, we used the
X3
andY3
variables for both these intermediate values and the output values. So before the lineT3 := X3*Y3
, we were reassigning them:Which led to us getting the wrong result for T. This wasn't caught by the proof, because m1add_precomputed_coordinates doesn't use the
point
sigma type, and its spec didn't specify anything about T in the output.This PR fixes this by using new different intermediate & output variables, rather than reusing
X3
andY3
.Note that this bug is not present in BoringSSL, because it has a separate output struct
r
rather than reusingX3
andY3
: https://boringssl.googlesource.com/boringssl/+/HEAD/third_party/fiat/curve25519_64_adx.h#599