Squashed 'src/secp256k1/' changes from 06bff6dec8..f473c959f0

f473c959f0 Merge bitcoin-core/secp256k1#1543: cmake: Do not modify build types when integrating by downstream project d403eea484 Merge bitcoin-core/secp256k1#1546: cmake: Rename `SECP256K1_LATE_CFLAGS` and switch to Bitcoin Core's approach d7ae25ce6f Merge bitcoin-core/secp256k1#1550: fix: typos in secp256k1.c 0e2fadb20c fix: typos in secp256k1.c 69b2192ad4 Merge bitcoin-core/secp256k1#1545: cmake: Do not set `CTEST_TEST_TARGET_ALIAS` 5dd637f3cf Merge bitcoin-core/secp256k1#1548: README: mention ellswift module 7454a53736 README: mention ellswift module 4706be2cd0 cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach c2764dbb99 cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS` f87a3589f4 cmake: Do not set `CTEST_TEST_TARGET_ALIAS` 158f9e5eae cmake: Do not modify build types when integrating by downstream project 35c0fdc86b Merge bitcoin-core/secp256k1#1529: cmake: Fix cache issue when integrating by downstream project 4392f0f717 Merge bitcoin-core/secp256k1#1533: tests: refactor: tidy up util functions (bitcoin#1491) bedffd53d8 Merge bitcoin-core/secp256k1#1488: ci: Add native macOS arm64 job 4b8d5eeacf Merge bitcoin-core/secp256k1#1532: cmake: Disable eager MSan in ctime_tests f55703ba49 autotools: Delete unneeded compiler test 396e885886 autotools: Align MSan checking code with CMake's implementation abde59f52d cmake: Report more compiler details in summary 7abf979a43 cmake: Disable `ctime_tests` if build with `-fsanitize=memory` 1791f6fce4 Merge bitcoin-core/secp256k1#1517: autotools: Disable eager MSan in ctime_tests e73f6f8fd9 tests: refactor: drop `secp256k1_` prefix from testrand.h functions 0ee7453a99 tests: refactor: add `testutil_` prefix to testutil.h functions 0c6bc76dcd tests: refactor: move `random_` helpers from tests.c to testutil.h 0fef8479be tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude` 59db007f0f tests: refactor: rename `random_group_element_...` -> `random_ge_...` ebfb82ee2f ci: Add job with -fsanitize-memory-param-retval e1bef0961c configure: Move "experimental" warning to bottom 55e5d975db autotools: Disable eager MSan in ctime_tests ec4c002faa cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation cae9a7ad14 cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable 218f0cc93b ci: Add native macOS arm64 job git-subtree-dir: src/secp256k1 git-subtree-split: f473c959f08edcb73669142f872d2189950bc54a
fanquake · Jun 25, 2024 · b941c15 · b941c15
1 parent ca3d945
commit b941c15
Show file tree

Hide file tree

Showing 19 changed files with 580 additions and 458 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -485,18 +485,24 @@ jobs:
       matrix:
         configuration:
           - env_vars:
+              CTIMETESTS: 'yes'
               CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -g'
           - env_vars:
               ECMULTGENKB: 2
               ECMULTWINDOW: 2
+              CTIMETESTS: 'yes'
               CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -g -O3'
+          - env_vars:
+              # -fsanitize-memory-param-retval is clang's default, but our build system disables it
+              # when ctime_tests when enabled.
+              CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -fsanitize-memory-param-retval -g'
+              CTIMETESTS: 'no'
 
     env:
       ECDH: 'yes'
       RECOVERY: 'yes'
       SCHNORRSIG: 'yes'
       ELLSWIFT: 'yes'
-      CTIMETESTS: 'yes'
       CC: 'clang'
       SECP256K1_TEST_ITERS: 32
       ASM: 'no'
@@ -585,10 +591,10 @@ jobs:
         run: env
         if: ${{ always() }}
 
-  macos-native:
-    name: "x86_64: macOS Monterey"
+  x86_64-macos-native:
+    name: "x86_64: macOS Monterey, Valgrind"
     # See: https://github.com/actions/runner-images#available-images.
-    runs-on: macos-12 # Use M1 once available https://github.com/github/roadmap/issues/528
+    runs-on: macos-12
 
     env:
       CC: 'clang'
@@ -644,6 +650,62 @@ jobs:
         run: env
         if: ${{ always() }}
 
+  arm64-macos-native:
+    name: "ARM64: macOS Sonoma"
+    # See: https://github.com/actions/runner-images#available-images.
+    runs-on: macos-14
+
+    env:
+      CC: 'clang'
+      HOMEBREW_NO_AUTO_UPDATE: 1
+      HOMEBREW_NO_INSTALL_CLEANUP: 1
+      WITH_VALGRIND: 'no'
+      CTIMETESTS: 'no'
+
+    strategy:
+      fail-fast: false
+      matrix:
+        env_vars:
+          - { WIDEMUL: 'int64',  RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes' }
+          - { WIDEMUL: 'int128_struct', ECMULTGENPRECISION: 2, ECMULTWINDOW: 4 }
+          - { WIDEMUL: 'int128',                  ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes' }
+          - { WIDEMUL: 'int128', RECOVERY: 'yes' }
+          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes' }
+          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', CC: 'gcc' }
+          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', CPPFLAGS: '-DVERIFY' }
+          - BUILD: 'distcheck'
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Install Homebrew packages
+        run: |
+          brew install automake libtool gcc
+          ln -s $(brew --prefix gcc)/bin/gcc-?? /usr/local/bin/gcc
+
+      - name: CI script
+        env: ${{ matrix.env_vars }}
+        run: ./ci/ci.sh
+
+      - run: cat tests.log || true
+        if: ${{ always() }}
+      - run: cat noverify_tests.log || true
+        if: ${{ always() }}
+      - run: cat exhaustive_tests.log || true
+        if: ${{ always() }}
+      - run: cat ctime_tests.log || true
+        if: ${{ always() }}
+      - run: cat bench.log || true
+        if: ${{ always() }}
+      - run: cat config.log || true
+        if: ${{ always() }}
+      - run: cat test_env.log || true
+        if: ${{ always() }}
+      - name: CI env
+        run: env
+        if: ${{ always() }}
+
   win64-native:
     name: ${{ matrix.configuration.job_name }}
     # See: https://github.com/actions/runner-images#available-images.

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -18,15 +18,14 @@ project(libsecp256k1
 )
 
 if(CMAKE_VERSION VERSION_LESS 3.21)
-  get_directory_property(parent_directory PARENT_DIRECTORY)
-  if(parent_directory)
-    set(PROJECT_IS_TOP_LEVEL OFF CACHE INTERNAL "Emulates CMake 3.21+ behavior.")
-    set(${PROJECT_NAME}_IS_TOP_LEVEL OFF CACHE INTERNAL "Emulates CMake 3.21+ behavior.")
+  # Emulates CMake 3.21+ behavior.
+  if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
+    set(PROJECT_IS_TOP_LEVEL ON)
+    set(${PROJECT_NAME}_IS_TOP_LEVEL ON)
   else()
-    set(PROJECT_IS_TOP_LEVEL ON CACHE INTERNAL "Emulates CMake 3.21+ behavior.")
-    set(${PROJECT_NAME}_IS_TOP_LEVEL ON CACHE INTERNAL "Emulates CMake 3.21+ behavior.")
+    set(PROJECT_IS_TOP_LEVEL OFF)
+    set(${PROJECT_NAME}_IS_TOP_LEVEL OFF)
   endif()
-  unset(parent_directory)
 endif()
 
 # The library version is based on libtool versioning of the ABI. The set of
@@ -214,23 +213,25 @@ mark_as_advanced(
   CMAKE_SHARED_LINKER_FLAGS_COVERAGE
 )
 
-get_property(is_multi_config GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
-set(default_build_type "RelWithDebInfo")
-if(is_multi_config)
-  set(CMAKE_CONFIGURATION_TYPES "${default_build_type}" "Release" "Debug" "MinSizeRel" "Coverage" CACHE STRING
-    "Supported configuration types."
-    FORCE
-  )
-else()
-  set_property(CACHE CMAKE_BUILD_TYPE PROPERTY
-    STRINGS "${default_build_type}" "Release" "Debug" "MinSizeRel" "Coverage"
-  )
-  if(NOT CMAKE_BUILD_TYPE)
-    message(STATUS "Setting build type to \"${default_build_type}\" as none was specified")
-    set(CMAKE_BUILD_TYPE "${default_build_type}" CACHE STRING
-      "Choose the type of build."
+if(PROJECT_IS_TOP_LEVEL)
+  get_property(is_multi_config GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
+  set(default_build_type "RelWithDebInfo")
+  if(is_multi_config)
+    set(CMAKE_CONFIGURATION_TYPES "${default_build_type}" "Release" "Debug" "MinSizeRel" "Coverage" CACHE STRING
+      "Supported configuration types."
       FORCE
     )
+  else()
+    set_property(CACHE CMAKE_BUILD_TYPE PROPERTY
+      STRINGS "${default_build_type}" "Release" "Debug" "MinSizeRel" "Coverage"
+    )
+    if(NOT CMAKE_BUILD_TYPE)
+      message(STATUS "Setting build type to \"${default_build_type}\" as none was specified")
+      set(CMAKE_BUILD_TYPE "${default_build_type}" CACHE STRING
+        "Choose the type of build."
+        FORCE
+      )
+    endif()
   endif()
 endif()
 
@@ -263,25 +264,34 @@ endif()
 
 set(CMAKE_C_VISIBILITY_PRESET hidden)
 
-# Ask CTest to create a "check" target (e.g., make check) as alias for the "test" target.
-# CTEST_TEST_TARGET_ALIAS is not documented but supposed to be user-facing.
-# See: https://gitlab.kitware.com/cmake/cmake/-/commit/816c9d1aa1f2b42d40c81a991b68c96eb12b6d2
-set(CTEST_TEST_TARGET_ALIAS check)
+set(print_msan_notice)
+if(SECP256K1_BUILD_CTIME_TESTS)
+  include(CheckMemorySanitizer)
+  check_memory_sanitizer(msan_enabled)
+  if(msan_enabled)
+    try_append_c_flags(-fno-sanitize-memory-param-retval)
+    set(print_msan_notice YES)
+  endif()
+  unset(msan_enabled)
+endif()
+
 include(CTest)
 # We do not use CTest's BUILD_TESTING because a single toggle for all tests is too coarse for our needs.
 mark_as_advanced(BUILD_TESTING)
 if(SECP256K1_BUILD_BENCHMARK OR SECP256K1_BUILD_TESTS OR SECP256K1_BUILD_EXHAUSTIVE_TESTS OR SECP256K1_BUILD_CTIME_TESTS OR SECP256K1_BUILD_EXAMPLES)
   enable_testing()
 endif()
 
-set(SECP256K1_LATE_CFLAGS "" CACHE STRING "Compiler flags that are added to the command line after all other flags added by the build system.")
-include(AllTargetsCompileOptions)
+set(SECP256K1_APPEND_CFLAGS "" CACHE STRING "Compiler flags that are appended to the command line after all other flags added by the build system. This variable is intended for debugging and special builds.")
+if(SECP256K1_APPEND_CFLAGS)
+  # Appending to this low-level rule variable is the only way to
+  # guarantee that the flags appear at the end of the command line.
+  string(APPEND CMAKE_C_COMPILE_OBJECT " ${SECP256K1_APPEND_CFLAGS}")
+endif()
 
 add_subdirectory(src)
-all_targets_compile_options(src "${SECP256K1_LATE_CFLAGS}")
 if(SECP256K1_BUILD_EXAMPLES)
   add_subdirectory(examples)
-  all_targets_compile_options(examples "${SECP256K1_LATE_CFLAGS}")
 endif()
 
 message("\n")
@@ -332,7 +342,7 @@ message("Valgrind .............................. ${SECP256K1_VALGRIND}")
 get_directory_property(definitions COMPILE_DEFINITIONS)
 string(REPLACE ";" " " definitions "${definitions}")
 message("Preprocessor defined macros ........... ${definitions}")
-message("C compiler ............................ ${CMAKE_C_COMPILER}")
+message("C compiler ............................ ${CMAKE_C_COMPILER_ID} ${CMAKE_C_COMPILER_VERSION}, ${CMAKE_C_COMPILER}")
 message("CFLAGS ................................ ${CMAKE_C_FLAGS}")
 get_directory_property(compile_options COMPILE_OPTIONS)
 string(REPLACE ";" " " compile_options "${compile_options}")
@@ -355,10 +365,17 @@ else()
   message(" - LDFLAGS for executables ............ ${CMAKE_EXE_LINKER_FLAGS_DEBUG}")
   message(" - LDFLAGS for shared libraries ....... ${CMAKE_SHARED_LINKER_FLAGS_DEBUG}")
 endif()
-if(SECP256K1_LATE_CFLAGS)
-  message("SECP256K1_LATE_CFLAGS ................. ${SECP256K1_LATE_CFLAGS}")
+if(SECP256K1_APPEND_CFLAGS)
+  message("SECP256K1_APPEND_CFLAGS ............... ${SECP256K1_APPEND_CFLAGS}")
+endif()
+message("")
+if(print_msan_notice)
+  message(
+    "Note:\n"
+    "  MemorySanitizer detected, tried to add -fno-sanitize-memory-param-retval to compile options\n"
+    "  to avoid false positives in ctime_tests. Pass -DSECP256K1_BUILD_CTIME_TESTS=OFF to avoid this.\n"
+  )
 endif()
-message("\n")
 if(SECP256K1_EXPERIMENTAL)
   message(
     "  ******\n"

diff --git a/README.md b/README.md
@@ -20,6 +20,7 @@ Features:
 * Optional module for public key recovery.
 * Optional module for ECDH key exchange.
 * Optional module for Schnorr signatures according to [BIP-340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki).
+* Optional module for ElligatorSwift key exchange according to [BIP-324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki).
 
 Implementation details
 ----------------------

diff --git a/build-aux/m4/bitcoin_secp.m4 b/build-aux/m4/bitcoin_secp.m4
@@ -45,6 +45,22 @@ fi
 AC_MSG_RESULT($has_valgrind)
 ])
 
+AC_DEFUN([SECP_MSAN_CHECK], [
+AC_MSG_CHECKING(whether MemorySanitizer is enabled)
+AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
+  #if defined(__has_feature)
+  #  if __has_feature(memory_sanitizer)
+       /* MemorySanitizer is enabled. */
+  #  elif
+  #    error "MemorySanitizer is disabled."
+  #  endif
+  #else
+  #  error "__has_feature is not defined."
+  #endif
+  ]])], [msan_enabled=yes], [msan_enabled=no])
+AC_MSG_RESULT([$msan_enabled])
+])
+
 dnl SECP_TRY_APPEND_CFLAGS(flags, VAR)
 dnl Append flags to VAR if CC accepts them.
 AC_DEFUN([SECP_TRY_APPEND_CFLAGS], [

diff --git a/cmake/AllTargetsCompileOptions.cmake b/cmake/AllTargetsCompileOptions.cmake
diff --git a/cmake/CheckMemorySanitizer.cmake b/cmake/CheckMemorySanitizer.cmake
@@ -0,0 +1,18 @@
+include_guard(GLOBAL)
+include(CheckCSourceCompiles)
+
+function(check_memory_sanitizer output)
+  set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
+  check_c_source_compiles("
+    #if defined(__has_feature)
+    #  if __has_feature(memory_sanitizer)
+         /* MemorySanitizer is enabled. */
+    #  elif
+    #    error \"MemorySanitizer is disabled.\"
+    #  endif
+    #else
+    #  error \"__has_feature is not defined.\"
+    #endif
+  " HAVE_MSAN)
+  set(${output} ${HAVE_MSAN} PARENT_SCOPE)
+endfunction()
diff --git a/configure.ac b/configure.ac
@@ -247,6 +247,20 @@ if test x"$enable_ctime_tests" = x"auto"; then
     enable_ctime_tests=$enable_valgrind
 fi
 
+print_msan_notice=no
+if test x"$enable_ctime_tests" = x"yes"; then
+  SECP_MSAN_CHECK
+  # MSan on Clang >=16 reports unitialized memory in function parameters and return values, even if
+  # the uninitalized variable is never actually "used". This is called "eager" checking, and it's
+  # sounds like good idea for normal use of MSan. However, it yields many false positives in the
+  # ctime_tests because many return values depend on secret (i.e., "uninitialized") values, and
+  # we're only interested in detecting branches (which count as "uses") on secret data.
+  if test x"$msan_enabled" = x"yes"; then
+    SECP_TRY_APPEND_CFLAGS([-fno-sanitize-memory-param-retval], SECP_CFLAGS)
+    print_msan_notice=yes
+  fi
+fi
+
 if test x"$enable_coverage" = x"yes"; then
     SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
     SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
@@ -426,12 +440,7 @@ fi
 ### Check for --enable-experimental if necessary
 ###
 
-if test x"$enable_experimental" = x"yes"; then
-  AC_MSG_NOTICE([******])
-  AC_MSG_NOTICE([WARNING: experimental build])
-  AC_MSG_NOTICE([Experimental features do not have stable APIs or properties, and may not be safe for production use.])
-  AC_MSG_NOTICE([******])
-else
+if test x"$enable_experimental" = x"no"; then
   if test x"$set_asm" = x"arm32"; then
     AC_MSG_ERROR([ARM32 assembly is experimental. Use --enable-experimental to allow.])
   fi
@@ -492,3 +501,17 @@ echo "  CPPFLAGS                = $CPPFLAGS"
 echo "  SECP_CFLAGS             = $SECP_CFLAGS"
 echo "  CFLAGS                  = $CFLAGS"
 echo "  LDFLAGS                 = $LDFLAGS"
+
+if test x"$print_msan_notice" = x"yes"; then
+  echo
+  echo "Note:"
+  echo "  MemorySanitizer detected, tried to add -fno-sanitize-memory-param-retval to SECP_CFLAGS"
+  echo "  to avoid false positives in ctime_tests. Pass --disable-ctime-tests to avoid this."
+fi
+
+if test x"$enable_experimental" = x"yes"; then
+  echo
+  echo "WARNING: Experimental build"
+  echo "  Experimental features do not have stable APIs or properties, and may not be safe for"
+  echo "  production use."
+fi
diff --git a/src/modules/ecdh/tests_impl.h b/src/modules/ecdh/tests_impl.h
@@ -56,7 +56,7 @@ static void test_ecdh_generator_basepoint(void) {
         size_t point_ser_len = sizeof(point_ser);
         secp256k1_scalar s;
 
-        random_scalar_order(&s);
+        testutil_random_scalar_order(&s);
         secp256k1_scalar_get_b32(s_b32, &s);
 
         CHECK(secp256k1_ec_pubkey_create(CTX, &point[0], s_one) == 1);
@@ -95,7 +95,7 @@ static void test_bad_scalar(void) {
     secp256k1_pubkey point;
 
     /* Create random point */
-    random_scalar_order(&rand);
+    testutil_random_scalar_order(&rand);
     secp256k1_scalar_get_b32(s_rand, &rand);
     CHECK(secp256k1_ec_pubkey_create(CTX, &point, s_rand) == 1);
 
@@ -127,7 +127,7 @@ static void test_result_basepoint(void) {
     CHECK(secp256k1_ecdh(CTX, out_base, &point, s_one, NULL, NULL) == 1);
 
     for (i = 0; i < 2 * COUNT; i++) {
-        random_scalar_order(&rand);
+        testutil_random_scalar_order(&rand);
         secp256k1_scalar_get_b32(s, &rand);
         secp256k1_scalar_inverse(&rand, &rand);
         secp256k1_scalar_get_b32(s_inv, &rand);