Merge bitcoin#831: Safegcd inverses, drop Jacobi symbols, remove libgmp

24ad04f Make scalar_inverse{,_var} benchmark scale with SECP256K1_BENCH_ITERS (Pieter Wuille)
ebc1af7 Optimization: track f,g limb count and pass to new variable-time update_fg_var (Peter Dettman)
b306935 Optimization: use formulas instead of lookup tables for cancelling g bits (Peter Dettman)
9164a1b Optimization: special-case zero modulus limbs in modinv64 (Pieter Wuille)
1f233b3 Remove num/gmp support (Pieter Wuille)
20448b8 Remove unused Jacobi symbol support (Pieter Wuille)
5437e7b Remove unused scalar_sqr (Pieter Wuille)
aa9cc52 Improve field/scalar inverse tests (Pieter Wuille)
1e0e885 Make field/scalar code use the new modinv modules for inverses (Pieter Wuille)
436281a Move secp256k1_fe_inverse{_var} to per-impl files (Pieter Wuille)
aa404d5 Move secp256k1_scalar_{inverse{_var},is_even} to per-impl files (Pieter Wuille)
08d5496 Improve bounds checks in modinv modules (Pieter Wuille)
151aac0 Add tests for modinv modules (Pieter Wuille)
d8a92fc Add extensive comments on the safegcd algorithm and implementation (Pieter Wuille)
8e415ac Add safegcd based modular inverse modules (Peter Dettman)
de0a643 Add secp256k1_ctz{32,64}_var functions (Pieter Wuille)

Pull request description:

  This is a rebased and squashed version of bitcoin#767, adding safegcd-based implementations of constant-time and variable-time modular inverses for scalars and field elements, by Peter Dettman. The PR is organized as follows:
  * **Add secp256k1_ctz{32,64}_var functions** Introduction of ctz functions to util.h (which use `__builtin_ctz` on recent GCC and Clang, but fall back to using a software emulation using de Bruijn on other platforms). This isn't used anywhere in this commit, but does include tests.
  * **Add safegcd based modular inverse modules** Add Peter Dettman's safegcd code from bitcoin#767 (without some of his optimizations, which are moved to later commits), turned into separate modules by me.
  * **Add extensive comments on the safegcd algorithm and implementation** Add a long description of the algorithm and optimizations to `doc/safegcd_implementation.md`, as well as additional comments to the code itself. It is probably best to review this together with the previous commit (they're separated to keep authorship).
  * **Add tests for modinv modules** Adds tests on the modinv interface directly, for arbitrary moduli.
  * **Improve bounds checks in modinv modules** Adds a lot of sanity checking to the modinv modules.
  * **Move secp256k1_scalar_{inverse{_var},is_even} to per-impl files** A pure refactor to prepare for switching the field and scalar code to modinv.
  * **Make field/scalar code use the new modinv modules for inverses** Actually switch over.
  * **Add extra modular inverse tests** This adds modular inverse tests through the field/scalar interface, now that those use modinv.
  * **Remove unused Jacobi symbol support** No longer needed.
  * **Remove num/gmp support** Bye-bye.
  * 3 commits with further optimizations.

ACKs for top commit:
  gmaxwell:
    ACK 24ad04f
  sanket1729:
    ACK 24ad04f
  real-or-random:
    ACK 24ad04f careful code review, some testing

Tree-SHA512: 732fe29315965e43ec9a10ee8c71eceeb983c43fe443da9dc5380a5a11b5e40b06e98d6abf67b773b1de74571fd2014973c6376f3a0caeac85e0cf163ba2144b

.cirrus.yml

@@ -1,6 +1,5 @@
 env:
   WIDEMUL: auto
-  BIGNUM: auto
   STATICPRECOMPUTATION: yes
   ECMULTGENPRECISION: auto
   ASM: no
@@ -59,17 +58,15 @@ task:
     - env: {WIDEMUL: int128,  RECOVERY: yes,            EXPERIMENTAL: yes, SCHNORRSIG: yes}
     - env: {WIDEMUL: int128,                 ECDH: yes, EXPERIMENTAL: yes, SCHNORRSIG: yes}
     - env: {WIDEMUL: int128,  ASM: x86_64}
-    - env: {BIGNUM: no}
-    - env: {BIGNUM: no,       RECOVERY: yes,            EXPERIMENTAL: yes, SCHNORRSIG: yes}
-    - env: {BIGNUM: no,       STATICPRECOMPUTATION: no}
+    - env: {                  RECOVERY: yes,            EXPERIMENTAL: yes, SCHNORRSIG: yes}
+    - env: {                  STATICPRECOMPUTATION: no}
     - env: {BUILD: distcheck, WITH_VALGRIND: no, CTIMETEST: no, BENCH: no}
     - env: {CPPFLAGS: -DDETERMINISTIC}
     - env: {CFLAGS: -O0, CTIMETEST: no}
     - env:
         CFLAGS:  "-fsanitize=undefined -fno-omit-frame-pointer"
         LDFLAGS: "-fsanitize=undefined -fno-omit-frame-pointer"
         UBSAN_OPTIONS: "print_stacktrace=1:halt_on_error=1"
-        BIGNUM: no
         ASM: x86_64
         ECDH: yes
         RECOVERY: yes
@@ -80,7 +77,6 @@ task:
     - env: { ECMULTGENPRECISION: 8 }
     - env:
         RUN_VALGRIND: yes
-        BIGNUM: no
         ASM: x86_64
         ECDH: yes
         RECOVERY: yes
@@ -115,12 +111,6 @@ task:
         CC: i686-linux-gnu-gcc
     - env:
         CC: clang --target=i686-pc-linux-gnu -isystem /usr/i686-linux-gnu/include
-  matrix:
-    - env:
-        BIGNUM: gmp
-    - env:
-        BIGNUM: no
-  << : *MERGE_BASE
   test_script:
     - ./ci/cirrus.sh
   << : *CAT_LOGS
@@ -178,7 +168,7 @@ task:
     # If we haven't restored from cached (and just run brew install), this is a no-op.
     - brew link valgrind
   brew_script:
-    - brew install automake libtool gmp gcc@9
+    - brew install automake libtool gcc@9
   << : *MERGE_BASE
   test_script:
     - ./ci/cirrus.sh
@@ -195,7 +185,6 @@ task:
     HOST: s390x-linux-gnu
     BUILD:
     WITH_VALGRIND: no
-    BIGNUM: no
     ECDH: yes
     RECOVERY: yes
     EXPERIMENTAL: yes

Makefile.am

@@ -14,8 +14,6 @@ noinst_HEADERS += src/scalar_8x32_impl.h
 noinst_HEADERS += src/scalar_low_impl.h
 noinst_HEADERS += src/group.h
 noinst_HEADERS += src/group_impl.h
-noinst_HEADERS += src/num_gmp.h
-noinst_HEADERS += src/num_gmp_impl.h
 noinst_HEADERS += src/ecdsa.h
 noinst_HEADERS += src/ecdsa_impl.h
 noinst_HEADERS += src/eckey.h
@@ -26,14 +24,16 @@ noinst_HEADERS += src/ecmult_const.h
 noinst_HEADERS += src/ecmult_const_impl.h
 noinst_HEADERS += src/ecmult_gen.h
 noinst_HEADERS += src/ecmult_gen_impl.h
-noinst_HEADERS += src/num.h
-noinst_HEADERS += src/num_impl.h
 noinst_HEADERS += src/field_10x26.h
 noinst_HEADERS += src/field_10x26_impl.h
 noinst_HEADERS += src/field_5x52.h
 noinst_HEADERS += src/field_5x52_impl.h
 noinst_HEADERS += src/field_5x52_int128_impl.h
 noinst_HEADERS += src/field_5x52_asm_impl.h
+noinst_HEADERS += src/modinv32.h
+noinst_HEADERS += src/modinv32_impl.h
+noinst_HEADERS += src/modinv64.h
+noinst_HEADERS += src/modinv64_impl.h
 noinst_HEADERS += src/assumptions.h
 noinst_HEADERS += src/util.h
 noinst_HEADERS += src/scratch.h

README.md

@@ -34,11 +34,11 @@ Implementation details
   * Optimized implementation of arithmetic modulo the curve's field size (2^256 - 0x1000003D1).
     * Using 5 52-bit limbs (including hand-optimized assembly for x86_64, by Diederik Huys).
     * Using 10 26-bit limbs (including hand-optimized assembly for 32-bit ARM, by Wladimir J. van der Laan).
-  * Field inverses and square roots using a sliding window over blocks of 1s (by Peter Dettman).
 * Scalar operations
   * Optimized implementation without data-dependent branches of arithmetic modulo the curve's order.
     * Using 4 64-bit limbs (relying on __int128 support in the compiler).
     * Using 8 32-bit limbs.
+* Modular inverses (both field elements and scalars) based on [safegcd](https://gcd.cr.yp.to/index.html) with some modifications, and a variable-time variant (by Peter Dettman).
 * Group operations
   * Point addition formula specifically simplified for the curve equation (y^2 = x^3 + 7).
   * Use addition between points in Jacobian and affine coordinates where possible.

build-aux/m4/bitcoin_secp.m4

@@ -75,19 +75,6 @@ if test x"$has_libcrypto" = x"yes" && test x"$has_openssl_ec" = x; then
 fi
 ])
 
-dnl
-AC_DEFUN([SECP_GMP_CHECK],[
-if test x"$has_gmp" != x"yes"; then
-  CPPFLAGS_TEMP="$CPPFLAGS"
-  CPPFLAGS="$GMP_CPPFLAGS $CPPFLAGS"
-  LIBS_TEMP="$LIBS"
-  LIBS="$GMP_LIBS $LIBS"
-  AC_CHECK_HEADER(gmp.h,[AC_CHECK_LIB(gmp, __gmpz_init,[has_gmp=yes; GMP_LIBS="$GMP_LIBS -lgmp"; AC_DEFINE(HAVE_LIBGMP,1,[Define this symbol if libgmp is installed])])])
-  CPPFLAGS="$CPPFLAGS_TEMP"
-  LIBS="$LIBS_TEMP"
-fi
-])
-
 AC_DEFUN([SECP_VALGRIND_CHECK],[
 if test x"$has_valgrind" != x"yes"; then
   CPPFLAGS_TEMP="$CPPFLAGS"

ci/cirrus.sh

@@ -14,7 +14,7 @@ valgrind --version || true
 
 ./configure \
     --enable-experimental="$EXPERIMENTAL" \
-    --with-test-override-wide-multiply="$WIDEMUL" --with-bignum="$BIGNUM" --with-asm="$ASM" \
+    --with-test-override-wide-multiply="$WIDEMUL" --with-asm="$ASM" \
     --enable-ecmult-static-precomputation="$STATICPRECOMPUTATION" --with-ecmult-gen-precision="$ECMULTGENPRECISION" \
     --enable-module-ecdh="$ECDH" --enable-module-recovery="$RECOVERY" \
     --enable-module-schnorrsig="$SCHNORRSIG" \

ci/linux-debian.Dockerfile

@@ -8,6 +8,6 @@ RUN apt-get update
 RUN apt-get install --no-install-recommends --no-upgrade -y \
         git ca-certificates \
         make automake libtool pkg-config dpkg-dev valgrind qemu-user \
-        gcc clang libc6-dbg libgmp-dev \
-        gcc-i686-linux-gnu libc6-dev-i386-cross libc6-dbg:i386 libgmp-dev:i386 \
+        gcc clang libc6-dbg \
+        gcc-i686-linux-gnu libc6-dev-i386-cross libc6-dbg:i386 \
         gcc-s390x-linux-gnu libc6-dev-s390x-cross libc6-dbg:s390x

configure.ac

@@ -48,17 +48,12 @@ case $host_os in
          # in expected paths because they may conflict with system files. Ask
          # Homebrew where each one is located, then adjust paths accordingly.
          openssl_prefix=`$BREW --prefix openssl 2>/dev/null`
-         gmp_prefix=`$BREW --prefix gmp 2>/dev/null`
          valgrind_prefix=`$BREW --prefix valgrind 2>/dev/null`
          if test x$openssl_prefix != x; then
            PKG_CONFIG_PATH="$openssl_prefix/lib/pkgconfig:$PKG_CONFIG_PATH"
            export PKG_CONFIG_PATH
            CRYPTO_CPPFLAGS="-I$openssl_prefix/include"
          fi
-         if test x$gmp_prefix != x; then
-           GMP_CPPFLAGS="-I$gmp_prefix/include"
-           GMP_LIBS="-L$gmp_prefix/lib"
-         fi
          if test x$valgrind_prefix != x; then
            VALGRIND_CPPFLAGS="-I$valgrind_prefix/include"
          fi
@@ -164,9 +159,6 @@ AC_ARG_ENABLE(external_default_callbacks,
 # Legal values are int64 (for [u]int64_t), int128 (for [unsigned] __int128), and auto (the default).
 AC_ARG_WITH([test-override-wide-multiply], [] ,[set_widemul=$withval], [set_widemul=auto])
 
-AC_ARG_WITH([bignum], [AS_HELP_STRING([--with-bignum=gmp|no|auto],
-[bignum implementation to use [default=auto]])],[req_bignum=$withval], [req_bignum=auto])
-
 AC_ARG_WITH([asm], [AS_HELP_STRING([--with-asm=x86_64|arm|no|auto],
 [assembly optimizations to use (experimental: arm) [default=auto]])],[req_asm=$withval], [req_asm=auto])
 
@@ -245,32 +237,6 @@ else
   esac
 fi
 
-if test x"$req_bignum" = x"auto"; then
-  SECP_GMP_CHECK
-  if test x"$has_gmp" = x"yes"; then
-    set_bignum=gmp
-  fi
-
-  if test x"$set_bignum" = x; then
-    set_bignum=no
-  fi
-else
-  set_bignum=$req_bignum
-  case $set_bignum in
-  gmp)
-    SECP_GMP_CHECK
-    if test x"$has_gmp" != x"yes"; then
-      AC_MSG_ERROR([gmp bignum explicitly requested but libgmp not available])
-    fi
-    ;;
-  no)
-    ;;
-  *)
-    AC_MSG_ERROR([invalid bignum implementation selection])
-    ;;
-  esac
-fi
-
 # Select assembly optimization
 use_external_asm=no
 
@@ -308,24 +274,6 @@ auto)
   ;;
 esac
 
-# Select bignum implementation
-case $set_bignum in
-gmp)
-  AC_DEFINE(HAVE_LIBGMP, 1, [Define this symbol if libgmp is installed])
-  AC_DEFINE(USE_NUM_GMP, 1, [Define this symbol to use the gmp implementation for num])
-  AC_DEFINE(USE_FIELD_INV_NUM, 1, [Define this symbol to use the num-based field inverse implementation])
-  AC_DEFINE(USE_SCALAR_INV_NUM, 1, [Define this symbol to use the num-based scalar inverse implementation])
-  ;;
-no)
-  AC_DEFINE(USE_NUM_NONE, 1, [Define this symbol to use no num implementation])
-  AC_DEFINE(USE_FIELD_INV_BUILTIN, 1, [Define this symbol to use the native field inverse implementation])
-  AC_DEFINE(USE_SCALAR_INV_BUILTIN, 1, [Define this symbol to use the native scalar inverse implementation])
-  ;;
-*)
-  AC_MSG_ERROR([invalid bignum implementation])
-  ;;
-esac
-
 # Set ecmult window size
 if test x"$req_ecmult_window" = x"auto"; then
   set_ecmult_window=15
@@ -390,11 +338,6 @@ else
   enable_openssl_tests=no
 fi
 
-if test x"$set_bignum" = x"gmp"; then
-  SECP_LIBS="$SECP_LIBS $GMP_LIBS"
-  SECP_INCLUDES="$SECP_INCLUDES $GMP_CPPFLAGS"
-fi
-
 if test x"$enable_valgrind" = x"yes"; then
   SECP_INCLUDES="$SECP_INCLUDES $VALGRIND_CPPFLAGS"
 fi
@@ -571,7 +514,6 @@ echo "  module extrakeys        = $enable_module_extrakeys"
 echo "  module schnorrsig       = $enable_module_schnorrsig"
 echo
 echo "  asm                     = $set_asm"
-echo "  bignum                  = $set_bignum"
 echo "  ecmult window size      = $set_ecmult_window"
 echo "  ecmult gen prec. bits   = $set_ecmult_gen_precision"
 # Hide test-only options unless they're used.