Squashed 'src/secp256k1/' changes from 44c2452fd3..5398d0ae28

5398d0ae28 ElligatorSwift d29ce7c882 Add benchmark for key generation f4211ce366 Add x-only ecmult_const version for x=n/d 00ae789e89 doc: Describe Jacobi calculation in safegcd_implementation.md 90062550c2 Native jacobi symbol algorithm 63a3565e97 Merge bitcoin-core/secp256k1#1120: ecmult_gen: Skip RNG when creating blinding if no seed is available 55f8bc99dc ecmult_gen: Improve comments about projective blinding 7a86955800 ecmult_gen: Simplify code (no observable change) 4cc0b1b669 ecmult_gen: Skip RNG when creating blinding if no seed is available af65d30cc8 Merge bitcoin-core/secp256k1#1116: build: Fix #include "..." paths to get rid of further -I arguments 40a3473a9d build: Fix #include "..." paths to get rid of further -I arguments 43756da819 Merge bitcoin-core/secp256k1#1115: Fix sepc256k1 -> secp256k1 typo in group.h 069aba8125 Fix sepc256k1 -> secp256k1 typo in group.h accadc94df Merge bitcoin-core/secp256k1#1114: `_scratch_destroy`: move `VERIFY_CHECK` after invalid scrach space check cd47033335 Merge bitcoin-core/secp256k1#1084: ci: Add MSVC builds 1827c9bf2b scratch_destroy: move VERIFY_CHECK after invalid scrach space check 49e2acd927 configure: Improve rationale for WERROR_CFLAGS 8dc4b03341 ci: Add a C++ job that compiles the public headers without -fpermissive 51f296a46c ci: Run persistent wineserver to speed up wine 3fb3269c22 ci: Add 32-bit MinGW64 build 9efc2e5221 ci: Add MSVC builds 2be6ba0fed configure: Convince autotools to work with MSVC's archiver lib.exe bd81f4140a schnorrsig bench: Suppress a stupid warning in MSVC 09f3d71c51 configure: Add a few CFLAGS for MSVC 3b4f3d0d46 build: Reject C++ compilers in the preprocessor 1cc0941414 configure: Don't abort if the compiler does not define __STDC__ cca8cbbac8 configure: Output message when checking for valgrind 1a6be5745f bench: Make benchmarks compile on MSVC git-subtree-dir: src/secp256k1 git-subtree-split: 5398d0ae28f16ac4223e57791fd34e6d51e8be70
dhruv · Jul 20, 2022 · 21e2acb · 21e2acb
1 parent c41bfd1
commit 21e2acb
Show file tree

Hide file tree

Showing 36 changed files with 1,634 additions and 86 deletions.
diff --git a/.cirrus.yml b/.cirrus.yml
@@ -241,17 +241,57 @@ task:
   << : *CAT_LOGS
 
 task:
-  name: "x86_64 (mingw32-w64): Windows (Debian stable, Wine)"
   << : *LINUX_CONTAINER
   env:
-    WRAPPER_CMD: wine64-stable
-    SECP256K1_TEST_ITERS: 16
-    HOST: x86_64-w64-mingw32
+    WRAPPER_CMD: wine
+    WITH_VALGRIND: no
+    ECDH: yes
+    RECOVERY: yes
+    SCHNORRSIG: yes
+    CTIMETEST: no
+  matrix:
+    - name: "x86_64 (mingw32-w64): Windows (Debian stable, Wine)"
+      env:
+        HOST: x86_64-w64-mingw32
+    - name: "i686 (mingw32-w64): Windows (Debian stable, Wine)"
+      env:
+        HOST: i686-w64-mingw32
+  << : *MERGE_BASE
+  test_script:
+    - ./ci/cirrus.sh
+  << : *CAT_LOGS
+
+task:
+  << : *LINUX_CONTAINER
+  env:
+    WRAPPER_CMD: wine
+    WERROR_CFLAGS: -WX
     WITH_VALGRIND: no
     ECDH: yes
     RECOVERY: yes
+    EXPERIMENTAL: yes
     SCHNORRSIG: yes
     CTIMETEST: no
+    # Set non-essential options that affect the CLI messages here.
+    # (They depend on the user's taste, so we don't want to set them automatically in configure.ac.)
+    CFLAGS: -nologo -diagnostics:caret
+    LDFLAGS: -XCClinker -nologo -XCClinker -diagnostics:caret
+  # Use a MinGW-w64 host to tell ./configure we're building for Windows.
+  # This will detect some MinGW-w64 tools but then make will need only
+  # the MSVC tools CC, AR and NM as specified below.
+  matrix:
+    - name: "x86_64 (MSVC): Windows (Debian stable, Wine)"
+      env:
+        HOST: x86_64-w64-mingw32
+        CC: /opt/msvc/bin/x64/cl
+        AR: /opt/msvc/bin/x64/lib
+        NM: /opt/msvc/bin/x64/dumpbin -symbols -headers
+    - name: "i686 (MSVC): Windows (Debian stable, Wine)"
+      env:
+        HOST: i686-w64-mingw32
+        CC: /opt/msvc/bin/x86/cl
+        AR: /opt/msvc/bin/x86/lib
+        NM: /opt/msvc/bin/x86/dumpbin -symbols -headers
   << : *MERGE_BASE
   test_script:
     - ./ci/cirrus.sh
@@ -302,13 +342,12 @@ task:
   << : *CAT_LOGS
 
 task:
-  name: "C++ -fpermissive"
+  name: "C++ -fpermissive (entire project)"
   << : *LINUX_CONTAINER
   env:
-    # ./configure correctly errors out when given CC=g++.
-    # We hack around this by passing CC=g++ only to make.
-    CC: gcc
-    MAKEFLAGS: -j4 CC=g++ CFLAGS=-fpermissive\ -g
+    CC: g++
+    CFLAGS: -fpermissive -g
+    CPPFLAGS: -DSECP256K1_CPLUSPLUS_TEST_OVERRIDE
     WERROR_CFLAGS:
     ECDH: yes
     RECOVERY: yes
@@ -318,6 +357,14 @@ task:
     - ./ci/cirrus.sh
   << : *CAT_LOGS
 
+task:
+  name: "C++ (public headers)"
+  << : *LINUX_CONTAINER
+  test_script:
+    - g++ -Werror include/*.h
+    - clang -Werror -x c++-header include/*.h
+    - /opt/msvc/bin/x64/cl.exe -c -WX -TP include/*.h
+
 task:
   name: "sage prover"
   << : *LINUX_CONTAINER

diff --git a/Makefile.am b/Makefile.am
@@ -87,7 +87,7 @@ endif
 endif
 
 libsecp256k1_la_SOURCES = src/secp256k1.c
-libsecp256k1_la_CPPFLAGS = -I$(top_srcdir)/include -I$(top_srcdir)/src $(SECP_INCLUDES)
+libsecp256k1_la_CPPFLAGS = $(SECP_INCLUDES)
 libsecp256k1_la_LIBADD = $(SECP_LIBS) $(COMMON_LIB) $(PRECOMPUTED_LIB)
 libsecp256k1_la_LDFLAGS = -no-undefined -version-info $(LIB_VERSION_CURRENT):$(LIB_VERSION_REVISION):$(LIB_VERSION_AGE)
 
@@ -112,7 +112,7 @@ TESTS =
 if USE_TESTS
 noinst_PROGRAMS += tests
 tests_SOURCES = src/tests.c
-tests_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/include $(SECP_INCLUDES) $(SECP_TEST_INCLUDES)
+tests_CPPFLAGS = $(SECP_INCLUDES) $(SECP_TEST_INCLUDES)
 if VALGRIND_ENABLED
 tests_CPPFLAGS += -DVALGRIND
 noinst_PROGRAMS += valgrind_ctime_test
@@ -228,3 +228,7 @@ endif
 if ENABLE_MODULE_SCHNORRSIG
 include src/modules/schnorrsig/Makefile.am.include
 endif
+
+if ENABLE_MODULE_ELLSWIFT
+include src/modules/ellswift/Makefile.am.include
+endif
diff --git a/build-aux/m4/bitcoin_secp.m4 b/build-aux/m4/bitcoin_secp.m4
@@ -10,6 +10,7 @@ AC_MSG_RESULT([$has_64bit_asm])
 ])
 
 AC_DEFUN([SECP_VALGRIND_CHECK],[
+AC_MSG_CHECKING([for valgrind support])
 if test x"$has_valgrind" != x"yes"; then
   CPPFLAGS_TEMP="$CPPFLAGS"
   CPPFLAGS="$VALGRIND_CPPFLAGS $CPPFLAGS"
@@ -21,6 +22,7 @@ if test x"$has_valgrind" != x"yes"; then
     #endif
   ]])], [has_valgrind=yes; AC_DEFINE(HAVE_VALGRIND,1,[Define this symbol if valgrind is installed, and it supports the host platform])])
 fi
+AC_MSG_RESULT($has_valgrind)
 ])
 
 dnl SECP_TRY_APPEND_CFLAGS(flags, VAR)

diff --git a/ci/cirrus.sh b/ci/cirrus.sh
@@ -5,10 +5,20 @@ set -x
 
 export LC_ALL=C
 
+# Start persistent wineserver if necessary.
+# This speeds up jobs with many invocations of wine (e.g., ./configure with MSVC) tremendously.
+case "$WRAPPER_CMD" in
+    *wine*)
+        # This is apparently only reliable when we run a dummy command such as "hh.exe" afterwards.
+        wineserver -p && wine hh.exe
+        ;;
+esac
+
 env >> test_env.log
 
 $CC -v || true
 valgrind --version || true
+$WRAPPER_CMD --version || true
 
 ./autogen.sh
 
@@ -63,6 +73,9 @@ then
     make precomp
 fi
 
+# Shutdown wineserver again
+wineserver -k || true
+
 # Check that no repo files have been modified by the build.
 # (This fails for example if the precomp files need to be updated in the repo.)
 git diff --exit-code
diff --git a/ci/linux-debian.Dockerfile b/ci/linux-debian.Dockerfile
@@ -1,15 +1,14 @@
 FROM debian:stable
 
-RUN dpkg --add-architecture i386
-RUN dpkg --add-architecture s390x
-RUN dpkg --add-architecture armhf
-RUN dpkg --add-architecture arm64
-RUN dpkg --add-architecture ppc64el
-RUN apt-get update
+RUN dpkg --add-architecture i386 && \
+    dpkg --add-architecture s390x && \
+    dpkg --add-architecture armhf && \
+    dpkg --add-architecture arm64 && \
+    dpkg --add-architecture ppc64el
 
 # dkpg-dev: to make pkg-config work in cross-builds
 # llvm: for llvm-symbolizer, which is used by clang's UBSan for symbolized stack traces
-RUN apt-get install --no-install-recommends --no-upgrade -y \
+RUN apt-get update && apt-get install --no-install-recommends -y \
         git ca-certificates \
         make automake libtool pkg-config dpkg-dev valgrind qemu-user \
         gcc clang llvm libc6-dbg \
@@ -19,8 +18,20 @@ RUN apt-get install --no-install-recommends --no-upgrade -y \
         gcc-arm-linux-gnueabihf libc6-dev-armhf-cross libc6-dbg:armhf \
         gcc-aarch64-linux-gnu libc6-dev-arm64-cross libc6-dbg:arm64 \
         gcc-powerpc64le-linux-gnu libc6-dev-ppc64el-cross libc6-dbg:ppc64el \
-        wine gcc-mingw-w64-x86-64 \
+        gcc-mingw-w64-x86-64-win32 wine64 wine \
+        gcc-mingw-w64-i686-win32 wine32 \
         sagemath
 
-# Run a dummy command in wine to make it set up configuration
-RUN wine64-stable xcopy || true
+WORKDIR /root
+# The "wine" package provides a convience wrapper that we need
+RUN apt-get update && apt-get install --no-install-recommends -y \
+        git ca-certificates wine64 wine python3-simplejson python3-six msitools winbind procps && \
+    git clone https://github.com/mstorsjo/msvc-wine && \
+    mkdir /opt/msvc && \
+    python3 msvc-wine/vsdownload.py --accept-license --dest /opt/msvc Microsoft.VisualStudio.Workload.VCTools && \
+    msvc-wine/install.sh /opt/msvc
+
+# Initialize the wine environment. Wait until the wineserver process has
+# exited before closing the session, to avoid corrupting the wine prefix.
+RUN wine64 wineboot --init && \
+    while (ps -A | grep wineserver) > /dev/null; do sleep 1; done
diff --git a/configure.ac b/configure.ac
@@ -33,14 +33,18 @@ AM_INIT_AUTOMAKE([1.11.2 foreign subdir-objects])
 m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])])
 
 AC_PROG_CC
-if test x"$ac_cv_prog_cc_c89" = x"no"; then
-  AC_MSG_ERROR([c89 compiler support required])
-fi
 AM_PROG_AS
 AM_PROG_AR
 
+# Clear some cache variables as a workaround for a bug that appears due to a bad
+# interaction between AM_PROG_AR and LT_INIT when combining MSVC's archiver lib.exe.
+# https://debbugs.gnu.org/cgi/bugreport.cgi?bug=54421
+AS_UNSET(ac_cv_prog_AR)
+AS_UNSET(ac_cv_prog_ac_ct_AR)
 LT_INIT([win32-dll])
 
+PKG_PROG_PKG_CONFIG
+
 build_windows=no
 
 case $host_os in
@@ -87,23 +91,35 @@ esac
 #
 # TODO We should analogously not touch CPPFLAGS and LDFLAGS but currently there are no issues.
 AC_DEFUN([SECP_TRY_APPEND_DEFAULT_CFLAGS], [
-    # Try to append -Werror=unknown-warning-option to CFLAGS temporarily. Otherwise clang will
-    # not error out if it gets unknown warning flags and the checks here will always succeed
-    # no matter if clang knows the flag or not.
-    SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS="$CFLAGS"
-    SECP_TRY_APPEND_CFLAGS([-Werror=unknown-warning-option], CFLAGS)
-
-    SECP_TRY_APPEND_CFLAGS([-std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef], $1) # GCC >= 3.0, -Wlong-long is implied by -pedantic.
-    SECP_TRY_APPEND_CFLAGS([-Wno-overlength-strings], $1) # GCC >= 4.2, -Woverlength-strings is implied by -pedantic.
-    SECP_TRY_APPEND_CFLAGS([-Wall], $1) # GCC >= 2.95 and probably many other compilers
-    SECP_TRY_APPEND_CFLAGS([-Wno-unused-function], $1) # GCC >= 3.0, -Wunused-function is implied by -Wall.
-    SECP_TRY_APPEND_CFLAGS([-Wextra], $1) # GCC >= 3.4, this is the newer name of -W, which we don't use because older GCCs will warn about unused functions.
-    SECP_TRY_APPEND_CFLAGS([-Wcast-align], $1) # GCC >= 2.95
-    SECP_TRY_APPEND_CFLAGS([-Wcast-align=strict], $1) # GCC >= 8.0
-    SECP_TRY_APPEND_CFLAGS([-Wconditional-uninitialized], $1) # Clang >= 3.0 only
-    SECP_TRY_APPEND_CFLAGS([-fvisibility=hidden], $1) # GCC >= 4.0
-
-    CFLAGS="$SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS"
+    # GCC and compatible (incl. clang)
+    if test "x$GCC" = "xyes"; then
+      # Try to append -Werror=unknown-warning-option to CFLAGS temporarily. Otherwise clang will
+      # not error out if it gets unknown warning flags and the checks here will always succeed
+      # no matter if clang knows the flag or not.
+      SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS="$CFLAGS"
+      SECP_TRY_APPEND_CFLAGS([-Werror=unknown-warning-option], CFLAGS)
+
+      SECP_TRY_APPEND_CFLAGS([-std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef], $1) # GCC >= 3.0, -Wlong-long is implied by -pedantic.
+      SECP_TRY_APPEND_CFLAGS([-Wno-overlength-strings], $1) # GCC >= 4.2, -Woverlength-strings is implied by -pedantic.
+      SECP_TRY_APPEND_CFLAGS([-Wall], $1) # GCC >= 2.95 and probably many other compilers
+      SECP_TRY_APPEND_CFLAGS([-Wno-unused-function], $1) # GCC >= 3.0, -Wunused-function is implied by -Wall.
+      SECP_TRY_APPEND_CFLAGS([-Wextra], $1) # GCC >= 3.4, this is the newer name of -W, which we don't use because older GCCs will warn about unused functions.
+      SECP_TRY_APPEND_CFLAGS([-Wcast-align], $1) # GCC >= 2.95
+      SECP_TRY_APPEND_CFLAGS([-Wcast-align=strict], $1) # GCC >= 8.0
+      SECP_TRY_APPEND_CFLAGS([-Wconditional-uninitialized], $1) # Clang >= 3.0 only
+      SECP_TRY_APPEND_CFLAGS([-fvisibility=hidden], $1) # GCC >= 4.0
+
+      CFLAGS="$SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS"
+    fi
+
+    # MSVC
+    # Assume MSVC if we're building for Windows but not with GCC or compatible;
+    # libtool makes the same assumption internally.
+    # Note that "/opt" and "-opt" are equivalent for MSVC; we use "-opt" because "/opt" looks like a path.
+    if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then
+      SECP_TRY_APPEND_CFLAGS([-W2 -wd4146], $1) # Moderate warning level, disable warning C4146 "unary minus operator applied to unsigned type, result still unsigned"
+      SECP_TRY_APPEND_CFLAGS([-external:anglebrackets -external:W0], $1) # Suppress warnings from #include <...> files
+    fi
 ])
 SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS)
 
@@ -156,6 +172,11 @@ AC_ARG_ENABLE(module_schnorrsig,
     AS_HELP_STRING([--enable-module-schnorrsig],[enable schnorrsig module [default=no]]), [],
     [SECP_SET_DEFAULT([enable_module_schnorrsig], [no], [yes])])
 
+AC_ARG_ENABLE(module_ellswift,
+    AS_HELP_STRING([--enable-module-ellswift],[enable ElligatorSwift module (experimental)]),
+    [enable_module_ellswift=$enableval],
+    [enable_module_ellswift=no])
+
 AC_ARG_ENABLE(external_default_callbacks,
     AS_HELP_STRING([--enable-external-default-callbacks],[enable external default callback functions [default=no]]), [],
     [SECP_SET_DEFAULT([enable_external_default_callbacks], [no], [no])])
@@ -326,7 +347,9 @@ if test x"$enable_valgrind" = x"yes"; then
   SECP_INCLUDES="$SECP_INCLUDES $VALGRIND_CPPFLAGS"
 fi
 
-# Add -Werror and similar flags passed from the outside (for testing, e.g., in CI)
+# Add -Werror and similar flags passed from the outside (for testing, e.g., in CI).
+# We don't want to set the user variable CFLAGS in CI because this would disable
+# autoconf's logic for setting default CFLAGS, which we would like to test in CI.
 SECP_CFLAGS="$SECP_CFLAGS $WERROR_CFLAGS"
 
 ###
@@ -346,6 +369,10 @@ if test x"$enable_module_schnorrsig" = x"yes"; then
   enable_module_extrakeys=yes
 fi
 
+if test x"$enable_module_ellswift" = x"yes"; then
+  AC_DEFINE(ENABLE_MODULE_ELLSWIFT, 1, [Define this symbol to enable the ElligatorSwift module])
+fi
+
 # Test if extrakeys is set after the schnorrsig module to allow the schnorrsig
 # module to set enable_module_extrakeys=yes
 if test x"$enable_module_extrakeys" = x"yes"; then
@@ -391,6 +418,7 @@ AM_CONDITIONAL([ENABLE_MODULE_ECDH], [test x"$enable_module_ecdh" = x"yes"])
 AM_CONDITIONAL([ENABLE_MODULE_RECOVERY], [test x"$enable_module_recovery" = x"yes"])
 AM_CONDITIONAL([ENABLE_MODULE_EXTRAKEYS], [test x"$enable_module_extrakeys" = x"yes"])
 AM_CONDITIONAL([ENABLE_MODULE_SCHNORRSIG], [test x"$enable_module_schnorrsig" = x"yes"])
+AM_CONDITIONAL([ENABLE_MODULE_ELLSWIFT], [test x"$enable_module_ellswift" = x"yes"])
 AM_CONDITIONAL([USE_EXTERNAL_ASM], [test x"$enable_external_asm" = x"yes"])
 AM_CONDITIONAL([USE_ASM_ARM], [test x"$set_asm" = x"arm"])
 AM_CONDITIONAL([BUILD_WINDOWS], [test "$build_windows" = "yes"])
@@ -411,6 +439,7 @@ echo "  module ecdh             = $enable_module_ecdh"
 echo "  module recovery         = $enable_module_recovery"
 echo "  module extrakeys        = $enable_module_extrakeys"
 echo "  module schnorrsig       = $enable_module_schnorrsig"
+echo "  module ellswift         = $enable_module_ellswift"
 echo
 echo "  asm                     = $set_asm"
 echo "  ecmult window size      = $set_ecmult_window"

diff --git a/doc/safegcd_implementation.md b/doc/safegcd_implementation.md
@@ -1,7 +1,7 @@
 # The safegcd implementation in libsecp256k1 explained
 
-This document explains the modular inverse implementation in the `src/modinv*.h` files. It is based
-on the paper
+This document explains the modular inverse and Jacobi symbol implementations in the `src/modinv*.h` files.
+It is based on the paper
 ["Fast constant-time gcd computation and modular inversion"](https://gcd.cr.yp.to/papers.html#safegcd)
 by Daniel J. Bernstein and Bo-Yin Yang. The references below are for the Date: 2019.04.13 version.
 
@@ -769,3 +769,30 @@ def modinv_var(M, Mi, x):
         d, e = update_de(d, e, t, M, Mi)
     return normalize(f, d, Mi)
 ```
+
+## 8. From GCDs to Jacobi symbol
+
+We can also use a similar approach to calculate Jacobi symbol *(x | M)* by keeping track of an extra variable *j*, for which at every step *(x | M) = j (g | f)*. As we update *f* and *g*, we make corresponding updates to *j* using [properties of the Jacobi symbol](https://en.wikipedia.org/wiki/Jacobi_symbol#Properties). In particular, we update *j* whenever we divide *g* by *2* or swap *f* and *g*; these updates depend only on the values of *f* and *g* modulo *4* or *8*, and can thus be applied very quickly. Overall, this calculation is slightly simpler than the one for modular inverse because we no longer need to keep track of *d* and *e*.
+
+However, one difficulty of this approach is that the Jacobi symbol *(a | n)* is only defined for positive odd integers *n*, whereas in the original safegcd algorithm, *f, g* can take negative values. We resolve this by using the following modified steps:
+
+```python
+        # Before
+        if delta > 0 and g & 1:
+            delta, f, g = 1 - delta, g, (g - f) // 2
+
+        # After
+        if delta > 0 and g & 1:
+            delta, f, g = 1 - delta, g, (g + f) // 2
+```
+
+The algorithm is still correct, since the changed divstep, called a "posdivstep" (see section 8.4 and E.5 in the paper) preserves *gcd(f, g)*. However, there's no proof that the modified algorithm will converge. The justification for posdivsteps is completely empirical: in practice, it appears that the vast majority of inputs converge to *f=g=gcd(f<sub>0</sub>, g<sub>0<sub>)* in a number of steps proportional to their logarithm.
+
+Note that:
+- We require inputs to satisfy *gcd(x, M) = 1*.
+- We need to update the termination condition from *g=0* to *f=1*.
+- We deal with the case where *g=0* on input specially.
+
+We account for the possibility of nonconvergence by only performing a bounded number of posdivsteps, and then falling back to square-root based Jacobi calculation if a solution has not yet been found.
+
+The optimizations in sections 3-7 above are described in the context of the original divsteps, but in the C implementation we also adapt most of them (not including "avoiding modulus operations", since it's not necessary to track *d, e*, and "constant-time operation", since we never calculate Jacobi symbols for secret data) to the posdivsteps version.