This enables testing overflow is correctly encoded in the recid, and
likely triggers more edge cases.
Also introduce a Sage script to generate the parameters.
47a7b8382fd6f1458d859b315cf3bcd3b9790b68 Clear field elements when writing infinity (Elichai Turkel)
61d1ecb02847be9d65ffe9df2d2408d85f3a0711 Added test with additions resulting in infinity (Elichai Turkel)
Pull request description:
Currently if `secp256k1_gej_add_var` / `secp256k1_gej_add_ge_var` /` secp256k1_gej_add_zinv_var` receive `P + (-P)` it will set `gej->infinity = 1` but doesn't call initialize the field elements.
Notice that this is the only branch in the function that results in an uninitialized output.
By using `secp256k1_gej_set_infinity()` it will set the field elements to zero while also setting the infinity flag.
I also added a test that fails with valgrind on current master but passes with the fix.
EDIT: This isn't a bug or something necessary, I just personally found this helpful.
ACKs for top commit:
real-or-random:
ACK 47a7b8382fd6f1458d859b315cf3bcd3b9790b68
Tree-SHA512: cdc2efc242a1b04b4f081183c07d4b2602cdba705e6b30b548df4e115e54fb97691f4b1a28f142f02d5e523c020721337a297b17d732acde147b910f5c53bd0a
ECMULT_CONST_TABLE_GET_GE was branching on its secret input.
Also makes secp256k1_gej_double_var implemented as a wrapper
on secp256k1_gej_double_nonzero instead of the other way
around. This wasn't a constant time bug but it was fragile
and could easily become one in the future if the double_var
algorithm is changed.
9bd89c8 Optimize secp256k1_fe_normalize_weak calls. Move secp256k1_fe_normalize_weak calls out of ECMULT_TABLE_GET_GE and ECMULT_TABLE_GET_GE_STORAGE and into secp256k1_ge_globalz_set_table_gej instead. (Russell O'Connor)
Pull request description:
Move secp256k1_fe_normalize_weak calls out of ECMULT_TABLE_GET_GE and ECMULT_TABLE_GET_GE_STORAGE and into secp256k1_ge_globalz_set_table_gej instead.
Tree-SHA512: 7bbb1aca8e37a268a26d7061bd1f390db129e697792f1d5ddd10ea34927616edc26ef118b500c3e5e14d1d463196033ef64e4d34b765380325c24835458b7a9b
Identifiers starting with an underscore and followed immediately by a capital letter are reserved by the C++ standard.
The only header guards not fixed are those in the headers auto-generated from java.
If you compile without ./configure --enable-exhaustive-tests=no,
this will create a binary ./exhaustive_tests which will execute
every function possible on a group of small order obtained by
moving to a twist of our curve and locating a generator of small
order.
Currently defaults to order 13, though by changing some #ifdefs
you can get a couple other ones. (Currently 199, which will take
forever to run, and 14, which won't work because it's composite.)
TODO exhaustive tests for the various modules
We observe that when changing the b-value in the elliptic curve formula
`y^2 = x^3 + ax + b`, the group law is unchanged. Therefore our functions
for secp256k1 will be correct if and only if they are correct when applied
to the curve defined by `y^2 = x^3 + 4` defined over the same field. This
curve has a point P of order 199.
This commit adds a test which computes the subgroup generated by P and
exhaustively checks that addition of every pair of points gives the correct
result.
Unfortunately we cannot test const-time scalar multiplication by the same
mechanism. The reason is that these ecmult functions both compute a wNAF
representation of the scalar, and this representation is tied to the order
of the group.
Testing with the incomplete version of gej_add_ge (found in 5de4c5dff^)
shows that this detects the incompleteness when adding P - 106P, which
is exactly what we expected since 106 is a cube root of 1 mod 199.
This makes it more clear that a null check is intended. Avoiding the
use of a pointer as a test condition alse increases the type-safety
of the comparisons.
(This is also MISRA C 2012 rules 14.4 and 11.9)
Designed with clear separation of the wNAF conversion, precomputation
and exponentiation (since the precomp at least we will probably want
to separate in the API for users who reuse points a lot.
Future work:
- actually separate precomp in the API
- do multiexp rather than single exponentiation
If two points (x1, y1) and (x2, y2) are given to gej_add_ge with
x1 != x2 but y1 = -y2, the function gives a wrong answer since
this causes it to compute "lambda = 0/0" during an intermediate
step. (Here lambda refers to an auxiallary variable in the point
addition formula, not the cube-root of 1 used by the endomorphism
optimization.)
This commit catches the 0/0 and replaces it with an alternate
expression for lambda, cmov'ing it in place if necessary.
There is zero functionality or opcount changes here; I need to do
this to make sure both R and M are computed before they are used,
since a future patch will replace either none or both of them.
Also compute r->y directly in terms of r->x, which again will be
used in a future patch.
* Make secp256k1_gej_add_var and secp256k1_gej_double return the
Z ratio to go from a.z to r.z.
* Use these Z ratios to speed up batch point conversion to affine
coordinates, and to speed up batch conversion of points to a
common Z coordinate.
* Add a point addition function that takes a point with a known
Z inverse.
* Due to secp256k1's endomorphism, all additions in the EC
multiplication code can work on affine coordinate (with an
implicit common Z coordinate), correcting the Z coordinate of
the result afterwards.
Refactoring by Pieter Wuille:
* Move more global-z logic into the group code.
* Separate code for computing the odd multiples from the code to bring it
to either storage or globalz format.
* Rename functions.
* Make all addition operations return Z ratios, and test them.
* Make the zr table format compatible with future batch chaining
(the first entry in zr becomes the ratio between the input and the
first output).
Original idea and code by Peter Dettman.
