The magnitude of the f and g variables generally goes down as the algorithm
progresses. Make use of this by keeping tracking how many limbs are used, and
when the number becomes small enough, make use of this to reduce the complexity
of arithmetic on them.
Refactored by: Pieter Wuille <pieter@wuille.net>
Both the field and scalar modulus can be written in signed{30,62} notation
with one or more zero limbs. Make use of this in the update_de function to
avoid a few wide multiplications when that is the case.
This doesn't appear to be a win in the 32-bit implementation, so only
do it for the 64-bit one.
Add a new run_inverse_tests that replaces all existing field/scalar inverse tests,
and tests a few identities for fixed inputs, small numbers (-999...999), random
inputs (structured and unstructured), as well as comparing with the output of
secp256k1_fe_inv_all_var.
This commit adds functions to verify and compare numbers in signed{30,62} notation,
and uses that to do more extensive bounds checking on various variables in the modinv
code.
This adds tests for the modinv{32,64}_impl.h directly (before the functions are used
inside the field/scalar code). It uses a naive implementation of modular multiplication
and gcds in order to verify the modular inverses themselves.
This adds a long comment explaining the algorithm and implementation choices by building
it up step by step in Python.
Comments in the code are also reworked/added, with references to the long explanation.
This was detected while running the tests with the `-Wconditional-uninitialized` flag
```
./autogen.sh
CC=clang CFLAGS="-Wconditional-uninitialized" ./configure
make check
```
The resulting warning is a false positive, but setting the value to -1
ensures that the CHECK below will fail if recid is never written to.
b6f649889ae78573f1959f04172a8e1fe15beab7 Add parens around ROUND_TO_ALIGN's parameter. This makes the macro robust against a hypothetical ROUND_TO_ALIGN(foo ? sizeA : size B) invocation. (Russell O'Connor)
Pull request description:
This makes the macro robust against a hypothetical `ROUND_TO_ALIGN(foo ? sizeA : size B)` invocation.
See also <https://wiki.sei.cmu.edu/confluence/display/c/PRE01-C.+Use+parentheses+within+macros+around+parameter+names>.
ACKs for top commit:
sipa:
ACK b6f649889ae78573f1959f04172a8e1fe15beab7. This is the way.
jonasnick:
utACK b6f649889ae78573f1959f04172a8e1fe15beab7
real-or-random:
utACK b6f649889ae78573f1959f04172a8e1fe15beab7
Tree-SHA512: 6a2685f959e8ae472259e5ea75fe12e8e6213f56f5aec7603a896c294e6a8833caae25c412607d9c9a3125370a7765a3e506127b101a1b87203f95e326f6c6c6
fb390c5299e999e06b7dff9e77e373600fae9fdf Remove underscores from header defs. This makes them consistent with other files and avoids reserved identifiers. (Russell O'Connor)
Pull request description:
ACKs for top commit:
real-or-random:
utACK fb390c5299e999e06b7dff9e77e373600fae9fdf
jonasnick:
ACK fb390c5299e999e06b7dff9e77e373600fae9fdf
Tree-SHA512: f49da79c0a90d1e82494821e7cf6f61c66bc377a3f37b2d4787ef19d2126e000627bfe4a76aa1c5bfffeb1382054aa824a7e9ab5d73c19d876b0828722c73854
3c15130709da26a6d2f25a483aa45e14bf1e4feb Improve CC_FOR_BUILD detection (Tim Ruffing)
47802a476246b67360bc24df78fe5fad6b93c296 Restructure and tidy configure.ac (Tim Ruffing)
252c19dfc654dbb10a35579fa36edb3466904758 Ask brew for valgrind include path (Tim Ruffing)
Pull request description:
See individual commit messages. These are improvements in preparation of the switch to Cirrus CI. (Maybe I'll just open a PR on top of this one.)
The first commit made the difference between successful build https://cirrus-ci.com/task/6740575057608704 and unsuccessful build https://cirrus-ci.com/task/4909571074424832.
I've tested the second commit without cross-compilation and with cross-compilation for android (https://github.com/bitcoin-core/secp256k1/issues/621#issuecomment-495703399)
When working on the autoconf stuff, I noticed two things that I just want to write down here:
- At some point we should update [build-aux/m4/ax_prog_cc_for_build.m4](https://www.gnu.org/software/autoconf-archive/ax_prog_cc_for_build.html). This is outdated, and [there have been a lot of fixes](https://github.com/autoconf-archive/autoconf-archive/pull/207) But the latest version is [broken](https://lists.gnu.org/archive/html/autoconf-archive-maintainers/2020-06/msg00002.html), so now is probably not the time.
- The latest autoconf 2.70 deprecates `AC_PROG_CC_C89`. It's not needed anymore because `AC_PROG_CC` cares about testing for version support. This makes autoconf 2.70 output a warning that we should probably just ignore. We don't want to force users onto 2.70...
ACKs for top commit:
sipa:
utACK 3c15130709da26a6d2f25a483aa45e14bf1e4feb
jonasnick:
utACK 3c15130 makes sense (with my very basic understanding of autoconf)
Tree-SHA512: 595b9de316374c2213f1340cddaa22eb3190b01fa99aa6ae26e77804df41e7ecf96a09e03c28e8f8b9fd04e211e4ee2f78f1e5a7995143c84f99d2e16d4f0260
33cb3c2b1fc3f3fe46c6d0eab118248ea86c1f06 Add secret key extraction from keypair to constant time tests (Elichai Turkel)
36d9dc1e8e6e3b15d805f04c973a8784a78880f6 Add seckey extraction from keypair to the extrakeys tests (Elichai Turkel)
fc96aa73f5c7f62452847a31821890ff1f72a5a4 Add a function to extract the secretkey from a keypair (Elichai Turkel)
Pull request description:
With schnorrsig if you need to tweak the secret key (for BIP32) you must use the keypair API to get compatible secret/public keys which you do by calling `secp256k1_keypair_xonly_tweak_add()`, but after that there's no currently a way to extract the secret key back for storage.
so I added a `secp256k1_keypair_seckey` function to extract the key
ACKs for top commit:
jonasnick:
ACK 33cb3c2b1fc3f3fe46c6d0eab118248ea86c1f06
real-or-random:
ACK 33cb3c2b1fc3f3fe46c6d0eab118248ea86c1f06 code inspection, tests pass
Tree-SHA512: 11212db38c8b87a87e2dc35c4d6993716867b45215b94b20522b1b3164ca63d4c6bf5192a6bff0e9267b333779cc8164844c56669a94e9be72df9ef025ffcfd4
This commits simply uses CC as CC_FOR_BUILD and the same for
corresponding flags if we're not cross-compiling. This has a number of
benefits in this common case:
- It avoids strange cases where very old compilers are used (#768).
- Flags are consistently set for CC and CC_FOR_BUILD.
- ./configure is faster.
- You get compiler x consistently if you set CC=x; we got this wrong
in CI in the past.
./configure warns if a _FOR_BUILD variable is set but ignored because
we're not cross-compiling.
The change exposed that //-style comments are used in gen_context.c,
which is also fixed by this commit.
This commit also reorganizes code in configure.ac to have a cleaner
separation of sections.
1f4dd0383807bfb7fef884601357b4c629dfb566 Typedef (u)int128_t only when they're not provided by the compiler (Tim Ruffing)
e89278f211a526062745c391d48a7baf782b4b2b Don't use reserved identifiers memczero and benchmark_verify_t (Tim Ruffing)
Pull request description:
As identified in #829 and #833. Fixes#829.
Since we touch this anyway, this commit additionally makes the
identifiers in the benchmark files a little bit more consistent.
This is necessary before we can merge #833. I preferred a separate PR because it makes it easier to see the results of Travis in #833.
ACKs for top commit:
sipa:
utACK 1f4dd0383807bfb7fef884601357b4c629dfb566
jonasnick:
ACK 1f4dd0383807bfb7fef884601357b4c629dfb566
Tree-SHA512: c0ec92798f3c94f3ef6ac69b3f0f39a39257a32be9d9a068832cece1ebe64c89848b70e44652fc397004b8b240883ac4bc0c8f95abbe4ba4b028de120e6734bf
29a299e373d5f0e326be74c514c7c70ddf50cce1 Run the undefined behaviour sanitizer on Travis (Fabien)
7506e064d791e529d2e57bb52c156deb33b897ef Prevent arithmetic on NULL pointer if the scratch space is too small (Fabien)
Pull request description:
ACKs for top commit:
sipa:
ACK 29a299e373d5f0e326be74c514c7c70ddf50cce1. Reviewed the code changes and verified that building with these sanitizer flags catches the existing error, as well as a signed integer overflow if introduced.
real-or-random:
ACK 29a299e373d5f0e326be74c514c7c70ddf50cce1 code inspection
jonasnick:
utACK 29a299e373d5f0e326be74c514c7c70ddf50cce1
Tree-SHA512: 4d788f12f3d7b48018e884910adb9b530a05d88f504de83dadeab8a22d75da83c05a1518f7317de5f536c4dd243ea7b347b1eaddb2ca1d804c663e41b85db69d
If the user passes invalid flags to _context_create, and the default
illegal callback does not abort the program (which is possible), then we
work with the result of malloc(0), which may be undefined behavior. This
violates the promise that a library function won't crash after the
illegal callback has been called.
This commit fixes this issue by returning NULL early in _context_create
in that case.
If the scratch space is too small when calling
`secp256k1_ecmult_strauss_batch()`, the `state.pre_a` allocation will
fail and the pointer will be `NULL`. This causes `state.pre_a_lam` to be
computed from the `NULL` pointer.
It is also possible that the first allocation to fail is for `state.ps`,
which will cause the failure to occur when in
`secp256k1_ecmult_strauss_wnaf()`.
The issue has been detected by UBSAN using Clang 10:
```
CC=clang \
CFLAGS="-fsanitize=undefined -fno-omit-frame-pointer" \
LDFLAGS="-fsanitize=undefined -fno-omit-frame-pointer" \
../configure
UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=1 make check
```
As identified in #829 and #833. Fixes#829.
Since we touch this anyway, this commit additionally makes the
identifiers in the benchmark files a little bit more consistent.
