[SECP256K1] Add macOS to the CI

Summary: ``` This adds macOS to the travis so we can test all the same configurations under macOS, both with Apple's clang and proper gcc on macOS. It also runs the ctime test, and valgrind on the tests just like on linux. The current travis script is pretty messy because we added more and more configuration over time each one required more complex ifs making it harder to read, and because of the somewhat complex logic it failed[1] on macOS(having an old bash version) so I decided to just throw it into a standalone sh script instead, that way it can be formatted nicely and with -e it will fail on every error without needing to put && everywhere. Some changed in the script: Replaced the usage of libtool with the locally generated libtool (#723 (comment)) Added --error-exitcode=42 to the ctime tests because they currently silently fail on -O0 (https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/681571334#L454) and disabled the ctime tests on -O0. Moved the valgrind tests to the matrix so that they'll run on both gcc and clang and on macOS. (also, now that #710 is merged we always pass -DVALGRIND when the valgrind headers exist but I left the explicit CFLAGS in those tests anyway, there's no harm in explicitly doing that) Removed the use of EXTRAFLAGS for setting CFLAGS, it's enough to just set CFLAGS directly and it can cause troubles in sh (the whole EXTRAFLAGS="--disable-openssl-tests CPPFLAGS=-DVALGRIND") We have to explicitly set the gcc version on macOS+gcc because macOS ship with a fake gcc which is basically just an alias to their clang compiler, and installing proper gcc from brew adds a gcc-* binary and doesn't replace the gcc binary, so we have to explicitly set CC=gcc-9 under that scenario, so I also explicitly install gcc@9 so it shouldn't break when macOS gets gcc-10. Bumped ubuntu to bionic because of #748 (comment) (the end of End of Standard Support is in a year anyway) it's in a separate commit so that if anyone have concerns I'll just drop that commit. https://travis-ci.org/github/elichai/secp256k1/jobs/681663742#L336 ``` Backport of secp256k1 [[bitcoin-core/secp256k1#750 | PR750]]. This backport adds a few changes to the original PR: - This PR contains a bug fix on the valgrind return value (was always 0 ) for the constant time check test. This makes the schnorr signature to trigger a false positive, and this diff includes a fix extracted from PR558. - This PR works around a bug in the brew plugin for Travis that causes an update if a package is installed via the built-in addon. However forcing no update will install a cmake version which is too old, so this workaround has not been ported. - The java tests fails with autotools on OSX due to an RPATH issue with the libraries. Since there is no trivial fix the test is skipped for now. Test Plan: Run the Travis build. https://travis-ci.org/github/Fabcien/secp256k1/builds/731184003 Note: this has been tested by part since the whole tests take > 6h Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7610
Bitcoin-ABC · Sep 29, 2020 · 7d90f72 · 7d90f72
1 parent 73aaab9
commit 7d90f72
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 38 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -1,6 +1,11 @@
 language: c
-os: linux
-dist: xenial
+os:
+  - linux
+  - osx
+
+dist: bionic
+# Valgrind currently supports upto macOS 10.13, the latest xcode of that version is 10.1
+osx_image: xcode10.1
 addons:
   apt:
     packages:
@@ -11,8 +16,16 @@ addons:
       - libtool-bin
       - ninja-build
       - valgrind
+  homebrew:
+    packages:
+      - cmake
+      - gcc@9
+      - gmp
+      - ninja
+      - valgrind
+    update: true
 install:
-  - ./travis/install_cmake.sh
+  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ./travis/install_cmake.sh; fi
 cache:
   directories:
     - /opt/cmake
@@ -42,7 +55,7 @@ env:
     - MULTISET=no
     - CTIMETEST=yes
     - BENCH=yes
-    - SECP256K1_BENCH_ITERS=2
+    - ITERS=2
   jobs:
     - SCALAR=32bit    RECOVERY=yes
     - SCALAR=32bit    FIELD=32bit       ECDH=yes  EXPERIMENTAL=yes  MULTISET=yes
@@ -58,45 +71,49 @@ env:
     - BIGNUM=no       STATICPRECOMPUTATION=no
     - AUTOTOOLS_TARGET=distcheck       CMAKE_TARGET=install    CTIMETEST= BENCH=
     - AUTOTOOLS_EXTRA_FLAGS=CPPFLAGS=-DDETERMINISTIC CMAKE_EXTRA_FLAGS=-DCMAKE_C_FLAGS=-DDETERMINISTIC
-    - AUTOTOOLS_EXTRA_FLAGS=CFLAGS=-O0               CMAKE_EXTRA_FLAGS=-DCMAKE_BUILD_TYPE=Debug
+    - AUTOTOOLS_EXTRA_FLAGS=CFLAGS=-O0               CMAKE_EXTRA_FLAGS=-DCMAKE_BUILD_TYPE=Debug CTIMETEST=
     - AUTOTOOLS_TARGET=check-java      CMAKE_TARGET=check-secp256k1-java JNI=yes ECDH=yes EXPERIMENTAL=yes CTIMETEST= BENCH=
     - ECMULTGENPRECISION=2
     - ECMULTGENPRECISION=8
+    - VALGRIND=yes
+      BIGNUM=no ENDOMORPHISM=yes ASM=x86_64 EXPERIMENTAL=yes ECDH=yes RECOVERY=yes OPENSSL_TESTS=no MULTISET=yes
+      AUTOTOOLS_EXTRA_FLAGS=CPPFLAGS=-DVALGRIND AUTOTOOLS_TARGET=
+      CMAKE_EXTRA_FLAGS=-DCMAKE_C_FLAGS=-DVALGRIND CMAKE_TARGET="secp256k1-tests secp256k1-exhaustive_tests"
+    # The same as above but without endomorphism.
+    - VALGRIND=yes
+      BIGNUM=no                  ASM=x86_64 EXPERIMENTAL=yes ECDH=yes RECOVERY=yes OPENSSL_TESTS=no MULTISET=yes
+      AUTOTOOLS_EXTRA_FLAGS=CPPFLAGS=-DVALGRIND  AUTOTOOLS_TARGET=
+      CMAKE_EXTRA_FLAGS=-DCMAKE_C_FLAGS=-DVALGRIND CMAKE_TARGET="secp256k1-tests secp256k1-exhaustive_tests"
     - SCHNORR=no
 jobs:
   fast_finish: true
   include:
     - compiler: clang
+      os: linux
       env: HOST=i686-linux-gnu ENDOMORPHISM=yes OPENSSL_TESTS=no
     - compiler: clang
+      os: linux
       env: HOST=i686-linux-gnu BIGNUM=no OPENSSL_TESTS=no
     - compiler: gcc
+      os: linux
       env: HOST=i686-linux-gnu ENDOMORPHISM=yes BIGNUM=no OPENSSL_TESTS=no
     - compiler: gcc
+      os: linux
       env: HOST=i686-linux-gnu OPENSSL_TESTS=no
-    - compiler: gcc
-      env:
-        - VALGRIND=yes
-        - BIGNUM=no  ENDOMORPHISM=yes  ASM=x86_64  EXPERIMENTAL=yes  ECDH=yes  RECOVERY=yes  OPENSSL_TESTS=no  MULTISET=yes
-        - AUTOTOOLS_EXTRA_FLAGS=CPPFLAGS=-DVALGRIND  AUTOTOOLS_TARGET=
-        - CMAKE_EXTRA_FLAGS=-DCMAKE_C_FLAGS=-DVALGRIND  CMAKE_TARGET="secp256k1-tests secp256k1-exhaustive_tests"
-    - compiler: gcc
-      env: # The same as above but without endomorphism.
-        - VALGRIND=yes
-        - BIGNUM=no  ENDOMORPHISM=no  ASM=x86_64  EXPERIMENTAL=yes  ECDH=yes  RECOVERY=yes  OPENSSL_TESTS=no  MULTISET=yes
-        - AUTOTOOLS_EXTRA_FLAGS=CPPFLAGS=-DVALGRIND  AUTOTOOLS_TARGET=
-        - CMAKE_EXTRA_FLAGS=-DCMAKE_C_FLAGS=-DVALGRIND  CMAKE_TARGET="secp256k1-tests secp256k1-exhaustive_tests"
 
+before_script:
+  # This limits the iterations in the benchmarks below to ITER iterations.
+  - export SECP256K1_BENCH_ITERS="$ITERS"
+
+# travis auto terminates jobs that go for 10 minutes without printing to stdout,
+# but travis_wait doesn't work well with forking programs like valgrind
+# (https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received https://github.com/bitcoin-core/secp256k1/pull/750#issuecomment-623476860)
 script:
+  - function keep_alive() { while true; do echo -en "\a"; sleep 60; done }
+  - keep_alive &
   - ./travis/build_autotools.sh
   - ./travis/build_cmake.sh
-  - # travis_wait extends the 10 minutes without output allowed (https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received)
-  - # the `--error-exitcode` is required to make the test fail if valgrind found errors, otherwise it'll return 0 (http://valgrind.org/docs/manual/manual-core.html)
-  - if [ -n "$VALGRIND" ]; then
-    travis_wait 30 valgrind --error-exitcode=42 ./buildautotools/tests 16 &&
-    travis_wait 30 valgrind --error-exitcode=42 ./buildautotools/exhaustive_tests;
-    fi
-  - if [ -n "$VALGRIND" ]; then
-    travis_wait 30 valgrind --error-exitcode=42 ./buildcmake/secp256k1-tests 16 &&
-    travis_wait 30 valgrind --error-exitcode=42 ./buildcmake/secp256k1-exhaustive_tests;
-    fi
+  - kill %keep_alive
+
+after_script:
+  - valgrind --version
diff --git a/src/modules/schnorr/schnorr_impl.h b/src/modules/schnorr/schnorr_impl.h
@@ -152,6 +152,11 @@ static int secp256k1_schnorr_sig_sign(
     secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &Rj, &k);
     secp256k1_ge_set_gej(&R, &Rj);
 
+    /*
+     * We declassify R to allow using it as a branch point.
+     * This is fine because R is not a secret.
+     */
+    secp256k1_declassify(ctx, &R, sizeof(R));
     /** Negate the nonce if R.y is not a quadratic residue. */
     secp256k1_scalar_cond_negate(&k, !secp256k1_fe_is_quad_var(&R.y));
 

diff --git a/travis/build_autotools.sh b/travis/build_autotools.sh
@@ -4,6 +4,16 @@ export LC_ALL=C
 
 set -ex
 
+# FIXME The java tests will fail on macOS with autotools due to the
+# libsepc256k1_jni library referencing the libsecp256k1 library with an absolute
+# path. This needs to be rewritten with install_name_tool to use relative paths
+# via the @variables supported by the macOS loader.
+if [ "$TRAVIS_OS_NAME" = "osx" ] && [ "$JNI" = "yes" ]
+then
+  echo "Skipping the java tests built with autotools on OSX"
+  exit 0
+fi
+
 if [ -n "$HOST" ]; then
   USE_HOST="--host=$HOST"
 fi
@@ -12,6 +22,13 @@ if [ "x$HOST" = "xi686-linux-gnu" ]; then
   CC="$CC -m32"
 fi
 
+if [ "$TRAVIS_OS_NAME" = "osx" ] && [ "$TRAVIS_COMPILER" = "gcc" ]
+then
+  CC="gcc-9"
+fi
+
+$CC --version
+
 ./autogen.sh
 
 mkdir buildautotools
@@ -45,28 +62,38 @@ trap 'print_logs' ERR
 
 make -j2 $AUTOTOOLS_TARGET
 
+if [ -n "$VALGRIND" ]; then
+  # the `--error-exitcode` is required to make the test fail if valgrind found
+  # errors, otherwise it'll return 0
+  # (http://valgrind.org/docs/manual/manual-core.html)
+  valgrind --error-exitcode=42 ./tests 16
+  valgrind --error-exitcode=42 ./exhaustive_tests
+fi
+
 if [ -n "$BENCH" ]; then
   if [ -n "$VALGRIND" ]; then
-    EXEC='libtool --mode=execute valgrind --error-exitcode=42';
+    # Using the local `libtool` because on macOS the system's libtool has
+    # nothing to do with GNU libtool
+    EXEC='./libtool --mode=execute valgrind --error-exitcode=42';
   else
     EXEC= ;
   fi
-  $EXEC ./bench_ecmult &>> bench.log
-  $EXEC ./bench_internal &>> bench.log
-  $EXEC ./bench_sign &>> bench.log
-  $EXEC ./bench_verify &>> bench.log
+  $EXEC ./bench_ecmult >> bench.log 2>&1
+  $EXEC ./bench_internal >> bench.log 2>&1
+  $EXEC ./bench_sign >> bench.log 2>&1
+  $EXEC ./bench_verify >> bench.log 2>&1
   if [ "$RECOVERY" == "yes" ]; then
-    $EXEC ./bench_recover &>> bench.log
+    $EXEC ./bench_recover >> bench.log 2>&1
   fi
   if [ "$ECDH" == "yes" ]; then
-    $EXEC ./bench_ecdh &>> bench.log
+    $EXEC ./bench_ecdh >> bench.log 2>&1
   fi
   if [ "$MULTISET" == "yes" ]; then
-    $EXEC ./bench_multiset &>> bench.log
+    $EXEC ./bench_multiset >> bench.log 2>&1
   fi
 fi
 if [ -n "$CTIMETEST" ]; then
-  libtool --mode=execute valgrind  ./valgrind_ctime_test &> valgrind_ctime_test.log
+  ./libtool --mode=execute valgrind --error-exitcode=42 ./valgrind_ctime_test > valgrind_ctime_test.log 2>&1
 fi
 
 popd
diff --git a/travis/build_cmake.sh b/travis/build_cmake.sh
@@ -7,6 +7,10 @@ set -ex
 if [ "x$HOST" = "xi686-linux-gnu" ]; then
   CMAKE_EXTRA_FLAGS="$CMAKE_EXTRA_FLAGS -DCMAKE_C_FLAGS=-m32"
 fi
+if [ "$TRAVIS_OS_NAME" = "osx" ] && [ "$TRAVIS_COMPILER" = "gcc" ]
+then
+  CMAKE_EXTRA_FLAGS="$CMAKE_EXTRA_FLAGS -DCMAKE_C_COMPILER=gcc-9"
+fi
 
 # "auto" is not a valid value for SECP256K1_ECMULT_GEN_PRECISION with cmake.
 # In this case we use the default value instead by not setting the cache
@@ -18,8 +22,13 @@ fi
 mkdir -p buildcmake/install
 pushd buildcmake
 
-# Use the cmake version installed via the install_cmake.sh script.
-CMAKE_COMMAND=/opt/cmake/bin/cmake
+# Use the cmake version installed via the install_cmake.sh script on linux
+if [ "$TRAVIS_OS_NAME" = "linux" ]
+then
+  CMAKE_COMMAND=/opt/cmake/bin/cmake
+else
+  CMAKE_COMMAND=cmake
+fi
 ${CMAKE_COMMAND} --version
 
 ${CMAKE_COMMAND} -GNinja .. \
@@ -41,4 +50,12 @@ ${CMAKE_COMMAND} -GNinja .. \
 
 ninja $CMAKE_TARGET
 
+if [ -n "$VALGRIND" ]; then
+  # the `--error-exitcode` is required to make the test fail if valgrind found
+  # errors, otherwise it'll return 0
+  # (http://valgrind.org/docs/manual/manual-core.html)
+  valgrind --error-exitcode=42 ./secp256k1-tests 16
+  valgrind --error-exitcode=42 ./secp256k1-exhaustive_tests
+fi
+
 popd