Skip to content

Commit 26de4df

Browse files
committed
Merge #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 #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 #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
2 parents 4c3ba88 + 24ad04f commit 26de4df

34 files changed

+3049
-1650
lines changed

.cirrus.yml

+3-14
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
env:
22
WIDEMUL: auto
3-
BIGNUM: auto
43
STATICPRECOMPUTATION: yes
54
ECMULTGENPRECISION: auto
65
ASM: no
@@ -59,17 +58,15 @@ task:
5958
- env: {WIDEMUL: int128, RECOVERY: yes, EXPERIMENTAL: yes, SCHNORRSIG: yes}
6059
- env: {WIDEMUL: int128, ECDH: yes, EXPERIMENTAL: yes, SCHNORRSIG: yes}
6160
- env: {WIDEMUL: int128, ASM: x86_64}
62-
- env: {BIGNUM: no}
63-
- env: {BIGNUM: no, RECOVERY: yes, EXPERIMENTAL: yes, SCHNORRSIG: yes}
64-
- env: {BIGNUM: no, STATICPRECOMPUTATION: no}
61+
- env: { RECOVERY: yes, EXPERIMENTAL: yes, SCHNORRSIG: yes}
62+
- env: { STATICPRECOMPUTATION: no}
6563
- env: {BUILD: distcheck, WITH_VALGRIND: no, CTIMETEST: no, BENCH: no}
6664
- env: {CPPFLAGS: -DDETERMINISTIC}
6765
- env: {CFLAGS: -O0, CTIMETEST: no}
6866
- env:
6967
CFLAGS: "-fsanitize=undefined -fno-omit-frame-pointer"
7068
LDFLAGS: "-fsanitize=undefined -fno-omit-frame-pointer"
7169
UBSAN_OPTIONS: "print_stacktrace=1:halt_on_error=1"
72-
BIGNUM: no
7370
ASM: x86_64
7471
ECDH: yes
7572
RECOVERY: yes
@@ -80,7 +77,6 @@ task:
8077
- env: { ECMULTGENPRECISION: 8 }
8178
- env:
8279
RUN_VALGRIND: yes
83-
BIGNUM: no
8480
ASM: x86_64
8581
ECDH: yes
8682
RECOVERY: yes
@@ -115,12 +111,6 @@ task:
115111
CC: i686-linux-gnu-gcc
116112
- env:
117113
CC: clang --target=i686-pc-linux-gnu -isystem /usr/i686-linux-gnu/include
118-
matrix:
119-
- env:
120-
BIGNUM: gmp
121-
- env:
122-
BIGNUM: no
123-
<< : *MERGE_BASE
124114
test_script:
125115
- ./ci/cirrus.sh
126116
<< : *CAT_LOGS
@@ -178,7 +168,7 @@ task:
178168
# If we haven't restored from cached (and just run brew install), this is a no-op.
179169
- brew link valgrind
180170
brew_script:
181-
- brew install automake libtool gmp gcc@9
171+
- brew install automake libtool gcc@9
182172
<< : *MERGE_BASE
183173
test_script:
184174
- ./ci/cirrus.sh
@@ -195,7 +185,6 @@ task:
195185
HOST: s390x-linux-gnu
196186
BUILD:
197187
WITH_VALGRIND: no
198-
BIGNUM: no
199188
ECDH: yes
200189
RECOVERY: yes
201190
EXPERIMENTAL: yes

Makefile.am

+4-4
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@ noinst_HEADERS += src/scalar_8x32_impl.h
1414
noinst_HEADERS += src/scalar_low_impl.h
1515
noinst_HEADERS += src/group.h
1616
noinst_HEADERS += src/group_impl.h
17-
noinst_HEADERS += src/num_gmp.h
18-
noinst_HEADERS += src/num_gmp_impl.h
1917
noinst_HEADERS += src/ecdsa.h
2018
noinst_HEADERS += src/ecdsa_impl.h
2119
noinst_HEADERS += src/eckey.h
@@ -26,14 +24,16 @@ noinst_HEADERS += src/ecmult_const.h
2624
noinst_HEADERS += src/ecmult_const_impl.h
2725
noinst_HEADERS += src/ecmult_gen.h
2826
noinst_HEADERS += src/ecmult_gen_impl.h
29-
noinst_HEADERS += src/num.h
30-
noinst_HEADERS += src/num_impl.h
3127
noinst_HEADERS += src/field_10x26.h
3228
noinst_HEADERS += src/field_10x26_impl.h
3329
noinst_HEADERS += src/field_5x52.h
3430
noinst_HEADERS += src/field_5x52_impl.h
3531
noinst_HEADERS += src/field_5x52_int128_impl.h
3632
noinst_HEADERS += src/field_5x52_asm_impl.h
33+
noinst_HEADERS += src/modinv32.h
34+
noinst_HEADERS += src/modinv32_impl.h
35+
noinst_HEADERS += src/modinv64.h
36+
noinst_HEADERS += src/modinv64_impl.h
3737
noinst_HEADERS += src/assumptions.h
3838
noinst_HEADERS += src/util.h
3939
noinst_HEADERS += src/scratch.h

README.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,11 @@ Implementation details
3434
* Optimized implementation of arithmetic modulo the curve's field size (2^256 - 0x1000003D1).
3535
* Using 5 52-bit limbs (including hand-optimized assembly for x86_64, by Diederik Huys).
3636
* Using 10 26-bit limbs (including hand-optimized assembly for 32-bit ARM, by Wladimir J. van der Laan).
37-
* Field inverses and square roots using a sliding window over blocks of 1s (by Peter Dettman).
3837
* Scalar operations
3938
* Optimized implementation without data-dependent branches of arithmetic modulo the curve's order.
4039
* Using 4 64-bit limbs (relying on __int128 support in the compiler).
4140
* Using 8 32-bit limbs.
41+
* 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).
4242
* Group operations
4343
* Point addition formula specifically simplified for the curve equation (y^2 = x^3 + 7).
4444
* Use addition between points in Jacobian and affine coordinates where possible.

build-aux/m4/bitcoin_secp.m4

-13
Original file line numberDiff line numberDiff line change
@@ -75,19 +75,6 @@ if test x"$has_libcrypto" = x"yes" && test x"$has_openssl_ec" = x; then
7575
fi
7676
])
7777

78-
dnl
79-
AC_DEFUN([SECP_GMP_CHECK],[
80-
if test x"$has_gmp" != x"yes"; then
81-
CPPFLAGS_TEMP="$CPPFLAGS"
82-
CPPFLAGS="$GMP_CPPFLAGS $CPPFLAGS"
83-
LIBS_TEMP="$LIBS"
84-
LIBS="$GMP_LIBS $LIBS"
85-
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])])])
86-
CPPFLAGS="$CPPFLAGS_TEMP"
87-
LIBS="$LIBS_TEMP"
88-
fi
89-
])
90-
9178
AC_DEFUN([SECP_VALGRIND_CHECK],[
9279
if test x"$has_valgrind" != x"yes"; then
9380
CPPFLAGS_TEMP="$CPPFLAGS"

ci/cirrus.sh

+1-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ valgrind --version || true
1414

1515
./configure \
1616
--enable-experimental="$EXPERIMENTAL" \
17-
--with-test-override-wide-multiply="$WIDEMUL" --with-bignum="$BIGNUM" --with-asm="$ASM" \
17+
--with-test-override-wide-multiply="$WIDEMUL" --with-asm="$ASM" \
1818
--enable-ecmult-static-precomputation="$STATICPRECOMPUTATION" --with-ecmult-gen-precision="$ECMULTGENPRECISION" \
1919
--enable-module-ecdh="$ECDH" --enable-module-recovery="$RECOVERY" \
2020
--enable-module-schnorrsig="$SCHNORRSIG" \

ci/linux-debian.Dockerfile

+2-2
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,6 @@ RUN apt-get update
88
RUN apt-get install --no-install-recommends --no-upgrade -y \
99
git ca-certificates \
1010
make automake libtool pkg-config dpkg-dev valgrind qemu-user \
11-
gcc clang libc6-dbg libgmp-dev \
12-
gcc-i686-linux-gnu libc6-dev-i386-cross libc6-dbg:i386 libgmp-dev:i386 \
11+
gcc clang libc6-dbg \
12+
gcc-i686-linux-gnu libc6-dev-i386-cross libc6-dbg:i386 \
1313
gcc-s390x-linux-gnu libc6-dev-s390x-cross libc6-dbg:s390x

configure.ac

-58
Original file line numberDiff line numberDiff line change
@@ -48,17 +48,12 @@ case $host_os in
4848
# in expected paths because they may conflict with system files. Ask
4949
# Homebrew where each one is located, then adjust paths accordingly.
5050
openssl_prefix=`$BREW --prefix openssl 2>/dev/null`
51-
gmp_prefix=`$BREW --prefix gmp 2>/dev/null`
5251
valgrind_prefix=`$BREW --prefix valgrind 2>/dev/null`
5352
if test x$openssl_prefix != x; then
5453
PKG_CONFIG_PATH="$openssl_prefix/lib/pkgconfig:$PKG_CONFIG_PATH"
5554
export PKG_CONFIG_PATH
5655
CRYPTO_CPPFLAGS="-I$openssl_prefix/include"
5756
fi
58-
if test x$gmp_prefix != x; then
59-
GMP_CPPFLAGS="-I$gmp_prefix/include"
60-
GMP_LIBS="-L$gmp_prefix/lib"
61-
fi
6257
if test x$valgrind_prefix != x; then
6358
VALGRIND_CPPFLAGS="-I$valgrind_prefix/include"
6459
fi
@@ -164,9 +159,6 @@ AC_ARG_ENABLE(external_default_callbacks,
164159
# Legal values are int64 (for [u]int64_t), int128 (for [unsigned] __int128), and auto (the default).
165160
AC_ARG_WITH([test-override-wide-multiply], [] ,[set_widemul=$withval], [set_widemul=auto])
166161

167-
AC_ARG_WITH([bignum], [AS_HELP_STRING([--with-bignum=gmp|no|auto],
168-
[bignum implementation to use [default=auto]])],[req_bignum=$withval], [req_bignum=auto])
169-
170162
AC_ARG_WITH([asm], [AS_HELP_STRING([--with-asm=x86_64|arm|no|auto],
171163
[assembly optimizations to use (experimental: arm) [default=auto]])],[req_asm=$withval], [req_asm=auto])
172164

@@ -245,32 +237,6 @@ else
245237
esac
246238
fi
247239

248-
if test x"$req_bignum" = x"auto"; then
249-
SECP_GMP_CHECK
250-
if test x"$has_gmp" = x"yes"; then
251-
set_bignum=gmp
252-
fi
253-
254-
if test x"$set_bignum" = x; then
255-
set_bignum=no
256-
fi
257-
else
258-
set_bignum=$req_bignum
259-
case $set_bignum in
260-
gmp)
261-
SECP_GMP_CHECK
262-
if test x"$has_gmp" != x"yes"; then
263-
AC_MSG_ERROR([gmp bignum explicitly requested but libgmp not available])
264-
fi
265-
;;
266-
no)
267-
;;
268-
*)
269-
AC_MSG_ERROR([invalid bignum implementation selection])
270-
;;
271-
esac
272-
fi
273-
274240
# Select assembly optimization
275241
use_external_asm=no
276242

@@ -308,24 +274,6 @@ auto)
308274
;;
309275
esac
310276

311-
# Select bignum implementation
312-
case $set_bignum in
313-
gmp)
314-
AC_DEFINE(HAVE_LIBGMP, 1, [Define this symbol if libgmp is installed])
315-
AC_DEFINE(USE_NUM_GMP, 1, [Define this symbol to use the gmp implementation for num])
316-
AC_DEFINE(USE_FIELD_INV_NUM, 1, [Define this symbol to use the num-based field inverse implementation])
317-
AC_DEFINE(USE_SCALAR_INV_NUM, 1, [Define this symbol to use the num-based scalar inverse implementation])
318-
;;
319-
no)
320-
AC_DEFINE(USE_NUM_NONE, 1, [Define this symbol to use no num implementation])
321-
AC_DEFINE(USE_FIELD_INV_BUILTIN, 1, [Define this symbol to use the native field inverse implementation])
322-
AC_DEFINE(USE_SCALAR_INV_BUILTIN, 1, [Define this symbol to use the native scalar inverse implementation])
323-
;;
324-
*)
325-
AC_MSG_ERROR([invalid bignum implementation])
326-
;;
327-
esac
328-
329277
# Set ecmult window size
330278
if test x"$req_ecmult_window" = x"auto"; then
331279
set_ecmult_window=15
@@ -390,11 +338,6 @@ else
390338
enable_openssl_tests=no
391339
fi
392340

393-
if test x"$set_bignum" = x"gmp"; then
394-
SECP_LIBS="$SECP_LIBS $GMP_LIBS"
395-
SECP_INCLUDES="$SECP_INCLUDES $GMP_CPPFLAGS"
396-
fi
397-
398341
if test x"$enable_valgrind" = x"yes"; then
399342
SECP_INCLUDES="$SECP_INCLUDES $VALGRIND_CPPFLAGS"
400343
fi
@@ -571,7 +514,6 @@ echo " module extrakeys = $enable_module_extrakeys"
571514
echo " module schnorrsig = $enable_module_schnorrsig"
572515
echo
573516
echo " asm = $set_asm"
574-
echo " bignum = $set_bignum"
575517
echo " ecmult window size = $set_ecmult_window"
576518
echo " ecmult gen prec. bits = $set_ecmult_gen_precision"
577519
# Hide test-only options unless they're used.

0 commit comments

Comments
 (0)