Repository: bitcoin-core/secp256k1
Pull request #944 (2021-05-13 19:37:32)

Various improvements related to CFLAGS
 - Fixes strange warnings in mingw builds (one item in #923).
 - Enables more and more fine-grained compiler warnings.
 - Fixes minor build issue on macOS  #896 on macOS.
 - Makes compiler warnings into errors on CI (`-Werror`, mentioned in https://github.com/bitcoin-core/secp256k1/issues/929#issuecomment-830861253).

real-or-random: CI seems to be confused here because this is the same source branch as on #938 . Trying to close and reopen the PR to fix it.
real-or-random: rebased
roconnor-blockstream: LGTM. I did test building in under Nixos, but that was the extent of my testing.
real-or-random: > Why a CI is not a "user" from the Automake's point of view? If so, why to introduce the `WERROR_CFLAGS` variable, while we have a dedicated `CFLAGS` one?

It may seem overly complicated but I think it's better for testing/CI because it does not interfere with autoconf's default for `CFLAGS` (which is `-g -O2` or just `-g` depending on the compiler). What we typically want to test in CI is building with unset `CFLAGS`. We could always set `-Werror -g -O2` instead of `-Werror` but that seems complicated too.

By the way, I think I overlooked this line: https://github.com/bitcoin-core/secp256k1/pull/944/files#diff-49473dca262eeab3b4a43002adb08b4db31020d190caaad1594b47f1d5daa810R397

Will address the other comments.

hebasto: > > Why a CI is not a "user" from the Automake's point of view? If so, why to introduce the `WERROR_CFLAGS` variable, while we have a dedicated `CFLAGS` one?
> 
> It may seem overly complicated but I think it's better for testing/CI because it does not interfere with autoconf's default for `CFLAGS` (which is `-g -O2` or just `-g` depending on the compiler). What we typically want to test in CI is building with unset `CFLAGS`. We could always set `-Werror -g -O2` instead of `-Werror` but that seems complicated too.

Ah, indeed. This repo does not have the `--enable-werror` configure option as Bitcoin Core has :)
real-or-random: Oh interesting, maybe I should just have looked at Core then... ;) 

For context:
https://github.com/bitcoin-core/secp256k1/pull/938#issuecomment-839632758

Anyway, this thing here works and I don't want to touch it further unless necessary.
real-or-random: Addressed the comments. I also rebased on master (sorry, that wasn't even intentional...)

> The last two commits seems the one to me, maybe squash them?

Done.
hebasto: ACK a5c6ff2917edf97e888c441eb545ac70083b5376

Patch:
diff --git a/.cirrus.yml b/.cirrus.yml
index 6d63511..aca8e0b 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -1,21 +1,28 @@
 env:
-  WIDEMUL: auto
+  ### compiler options
+  HOST:
+  # Specific warnings can be disabled with -Wno-error=foo.
+  # -pedantic-errors is not equivalent to -Werror=pedantic and thus not implied by -Werror according to the GCC manual.
+  WERROR_CFLAGS: -Werror -pedantic-errors
+  MAKEFLAGS: -j2
+  BUILD: check
+  ### secp256k1 config
   STATICPRECOMPUTATION: yes
   ECMULTGENPRECISION: auto
   ASM: no
-  BUILD: check
+  WIDEMUL: auto
   WITH_VALGRIND: yes
   EXTRAFLAGS:
-  HOST:
+  ### secp256k1 modules
+  EXPERIMENTAL: no
   ECDH: no
   RECOVERY: no
   SCHNORRSIG: no
-  EXPERIMENTAL: no
-  CTIMETEST: yes
-  BENCH: yes
+  ### test options
   TEST_ITERS:
+  BENCH: yes
   BENCH_ITERS: 2
-  MAKEFLAGS: -j2
+  CTIMETEST: yes
 
 cat_logs_snippet: &CAT_LOGS
   always:
diff --git a/.gitignore b/.gitignore
index b62055a..79b740d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -23,6 +23,7 @@ aclocal.m4
 autom4te.cache/
 config.log
 config.status
+conftest*
 *.tar.gz
 *.la
 libtool
diff --git a/Makefile.am b/Makefile.am
index 23b2928..1e03560 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,5 +1,9 @@
 ACLOCAL_AMFLAGS = -I build-aux/m4
 
+# AM_CFLAGS will be automatically prepended to CFLAGS by Automake when compiling some foo
+# which does not have an explicit foo_CFLAGS variable set.
+AM_CFLAGS = $(SECP_CFLAGS)
+
 lib_LTLIBRARIES = libsecp256k1.la
 include_HEADERS = include/secp256k1.h
 include_HEADERS += include/secp256k1_preallocated.h
@@ -129,10 +133,10 @@ CPPFLAGS_FOR_BUILD +=-I$(top_srcdir) -I$(builddir)/src
 gen_context_OBJECTS = gen_context.o
 gen_context_BIN = gen_context$(BUILD_EXEEXT)
 gen_%.o: src/gen_%.c src/libsecp256k1-config.h
-	$(CC_FOR_BUILD) $(DEFS) $(CPPFLAGS_FOR_BUILD) $(CFLAGS_FOR_BUILD) -c $< -o $@
+	$(CC_FOR_BUILD) $(DEFS) $(CPPFLAGS_FOR_BUILD) $(SECP_CFLAGS_FOR_BUILD) $(CFLAGS_FOR_BUILD) -c $< -o $@
 
 $(gen_context_BIN): $(gen_context_OBJECTS)
-	$(CC_FOR_BUILD) $(CFLAGS_FOR_BUILD) $(LDFLAGS_FOR_BUILD) $^ -o $@
+	$(CC_FOR_BUILD) $(SECP_CFLAGS_FOR_BUILD) $(CFLAGS_FOR_BUILD) $(LDFLAGS_FOR_BUILD) $^ -o $@
 
 $(libsecp256k1_la_OBJECTS): src/ecmult_static_context.h
 $(tests_OBJECTS): src/ecmult_static_context.h
diff --git a/build-aux/m4/bitcoin_secp.m4 b/build-aux/m4/bitcoin_secp.m4
index e57888c..8245b2b 100644
--- a/build-aux/m4/bitcoin_secp.m4
+++ b/build-aux/m4/bitcoin_secp.m4
@@ -82,3 +82,19 @@ if test x"$has_valgrind" != x"yes"; then
   AC_CHECK_HEADER([valgrind/memcheck.h], [has_valgrind=yes; AC_DEFINE(HAVE_VALGRIND,1,[Define this symbol if valgrind is installed])])
 fi
 ])
+
+dnl SECP_TRY_APPEND_CFLAGS(flags, VAR)
+dnl Append flags to VAR if CC accepts them.
+AC_DEFUN([SECP_TRY_APPEND_CFLAGS], [
+  AC_MSG_CHECKING([if ${CC} supports $1])
+  SECP_TRY_APPEND_CFLAGS_saved_CFLAGS="$CFLAGS"
+  CFLAGS="$1 $CFLAGS"
+  AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])], [flag_works=yes], [flag_works=no])
+  AC_MSG_RESULT($flag_works)
+  CFLAGS="$SECP_TRY_APPEND_CFLAGS_saved_CFLAGS"
+  if test x"$flag_works" = x"yes"; then
+    $2="$$2 $1"
+  fi
+  unset flag_works
+  AC_SUBST($2)
+])
diff --git a/configure.ac b/configure.ac
index 1ed991a..7172a9f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -8,10 +8,6 @@ AH_TOP([#define LIBSECP256K1_CONFIG_H])
 AH_BOTTOM([#endif /*LIBSECP256K1_CONFIG_H*/])
 AM_INIT_AUTOMAKE([foreign subdir-objects])
 
-# Set -g if CFLAGS are not already set, which matches the default autoconf
-# behavior (see PROG_CC in the Autoconf manual) with the exception that we don't
-# set -O2 here because we set it in any case (see further down).
-: ${CFLAGS="-g"}
 LT_INIT
 
 # Make the compilation flags quiet unless V=1 is used.
@@ -70,35 +66,41 @@ case $host_os in
    ;;
 esac
 
-CFLAGS="-W $CFLAGS"
-
-warn_CFLAGS="-std=c89 -pedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-unused-function -Wno-long-long -Wno-overlength-strings"
-saved_CFLAGS="$CFLAGS"
-CFLAGS="$warn_CFLAGS $CFLAGS"
-AC_MSG_CHECKING([if ${CC} supports ${warn_CFLAGS}])
-AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
-    [ AC_MSG_RESULT([yes]) ],
-    [ AC_MSG_RESULT([no])
-      CFLAGS="$saved_CFLAGS"
-    ])
-
-saved_CFLAGS="$CFLAGS"
-CFLAGS="-Wconditional-uninitialized $CFLAGS"
-AC_MSG_CHECKING([if ${CC} supports -Wconditional-uninitialized])
-AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
-    [ AC_MSG_RESULT([yes]) ],
-    [ AC_MSG_RESULT([no])
-      CFLAGS="$saved_CFLAGS"
-    ])
-
-saved_CFLAGS="$CFLAGS"
-CFLAGS="-fvisibility=hidden $CFLAGS"
-AC_MSG_CHECKING([if ${CC} supports -fvisibility=hidden])
-AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
-    [ AC_MSG_RESULT([yes]) ],
-    [ AC_MSG_RESULT([no])
-      CFLAGS="$saved_CFLAGS"
-    ])
+# Try if some desirable compiler flags are supported and append them to SECP_CFLAGS.
+#
+# These are our own flags, so we append them to our own SECP_CFLAGS variable (instead of CFLAGS) as
+# recommended in the automake manual (Section "Flag Variables Ordering"). CFLAGS belongs to the user
+# and we are not supposed to touch it. In the Makefile, we will need to ensure that SECP_CFLAGS
+# is prepended to CFLAGS when invoking the compiler so that the user always has the last word (flag).
+#
+# Another advantage of not touching CFLAGS is that the contents of CFLAGS will be picked up by
+# libtool for compiling helper executables. For example, when compiling for Windows, libtool will
+# generate entire wrapper executables (instead of simple wrapper scripts as on Unix) to ensure
+# proper operation of uninstalled programs linked by libtool against the uninstalled shared library.
+# These executables are compiled from C source file for which our flags may not be appropriate,
+# e.g., -std=c89 flag has lead to undesirable warnings in the past.
+#
+# 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"
+])
+SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS)
 
 ###
 ### Define config arguments
@@ -213,10 +215,14 @@ AM_CONDITIONAL([VALGRIND_ENABLED],[test "$enable_valgrind" = "yes"])
 
 if test x"$enable_coverage" = x"yes"; then
     AC_DEFINE(COVERAGE, 1, [Define this symbol to compile out all VERIFY code])
-    CFLAGS="-O0 --coverage $CFLAGS"
+    SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
     LDFLAGS="--coverage $LDFLAGS"
 else
-    CFLAGS="-O2 $CFLAGS"
+    # Most likely the CFLAGS already contain -O2 because that is autoconf's default.
+    # We still add it here because passing it twice is not an issue, and handling
+    # this case would just add unnecessary complexity (see #896).
+    SECP_CFLAGS="-O2 $SECP_CFLAGS"
+    SECP_CFLAGS_FOR_BUILD="-O2 $SECP_CFLAGS_FOR_BUILD"
 fi
 
 if test x"$req_asm" = x"auto"; then
@@ -351,6 +357,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)
+SECP_CFLAGS="$SECP_CFLAGS $WERROR_CFLAGS"
+
 # Handle static precomputation (after everything which modifies CFLAGS and friends)
 if test x"$use_ecmult_static_precomputation" != x"no"; then
   if test x"$cross_compiling" = x"no"; then
@@ -360,8 +369,9 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
     fi
     # If we're not cross-compiling, simply use the same compiler for building the static precompation code.
     CC_FOR_BUILD="$CC"
-    CFLAGS_FOR_BUILD="$CFLAGS"
     CPPFLAGS_FOR_BUILD="$CPPFLAGS"
+    SECP_CFLAGS_FOR_BUILD="$SECP_CFLAGS"
+    CFLAGS_FOR_BUILD="$CFLAGS"
     LDFLAGS_FOR_BUILD="$LDFLAGS"
   else
     AX_PROG_CC_FOR_BUILD
@@ -371,22 +381,14 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
     cross_compiling=no
     SAVE_CC="$CC"
     CC="$CC_FOR_BUILD"
-    SAVE_CFLAGS="$CFLAGS"
-    CFLAGS="$CFLAGS_FOR_BUILD"
     SAVE_CPPFLAGS="$CPPFLAGS"
     CPPFLAGS="$CPPFLAGS_FOR_BUILD"
+    SAVE_CFLAGS="$CFLAGS"
+    CFLAGS="$CFLAGS_FOR_BUILD"
     SAVE_LDFLAGS="$LDFLAGS"
     LDFLAGS="$LDFLAGS_FOR_BUILD"
 
-    warn_CFLAGS_FOR_BUILD="-Wall -Wextra -Wno-unused-function"
-    saved_CFLAGS="$CFLAGS"
-    CFLAGS="$warn_CFLAGS_FOR_BUILD $CFLAGS"
-    AC_MSG_CHECKING([if native ${CC_FOR_BUILD} supports ${warn_CFLAGS_FOR_BUILD}])
-    AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
-        [ AC_MSG_RESULT([yes]) ],
-        [ AC_MSG_RESULT([no])
-          CFLAGS="$saved_CFLAGS"
-        ])
+    SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS_FOR_BUILD)
 
     AC_MSG_CHECKING([for working native compiler: ${CC_FOR_BUILD}])
     AC_RUN_IFELSE(
@@ -394,19 +396,17 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
       [working_native_cc=yes],
       [working_native_cc=no],[:])
 
-    CFLAGS_FOR_BUILD="$CFLAGS"
-
     # Restore the environment
     cross_compiling=$save_cross_compiling
     CC="$SAVE_CC"
-    CFLAGS="$SAVE_CFLAGS"
     CPPFLAGS="$SAVE_CPPFLAGS"
+    CFLAGS="$SAVE_CFLAGS"
     LDFLAGS="$SAVE_LDFLAGS"
 
     if test x"$working_native_cc" = x"no"; then
       AC_MSG_RESULT([no])
       set_precomp=no
-      m4_define([please_set_for_build], [Please set CC_FOR_BUILD, CFLAGS_FOR_BUILD, CPPFLAGS_FOR_BUILD, and/or LDFLAGS_FOR_BUILD.])
+      m4_define([please_set_for_build], [Please set CC_FOR_BUILD, CPPFLAGS_FOR_BUILD, CFLAGS_FOR_BUILD, and/or LDFLAGS_FOR_BUILD.])
       if test x"$use_ecmult_static_precomputation" = x"yes";  then
         AC_MSG_ERROR([native compiler ${CC_FOR_BUILD} does not produce working binaries. please_set_for_build])
       else
@@ -419,8 +419,9 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
   fi
 
   AC_SUBST(CC_FOR_BUILD)
-  AC_SUBST(CFLAGS_FOR_BUILD)
   AC_SUBST(CPPFLAGS_FOR_BUILD)
+  AC_SUBST(SECP_CFLAGS_FOR_BUILD)
+  AC_SUBST(CFLAGS_FOR_BUILD)
   AC_SUBST(LDFLAGS_FOR_BUILD)
 else
   set_precomp=no
@@ -490,6 +491,7 @@ AC_SUBST(SECP_INCLUDES)
 AC_SUBST(SECP_LIBS)
 AC_SUBST(SECP_TEST_LIBS)
 AC_SUBST(SECP_TEST_INCLUDES)
+AC_SUBST(SECP_CFLAGS)
 AM_CONDITIONAL([ENABLE_COVERAGE], [test x"$enable_coverage" = x"yes"])
 AM_CONDITIONAL([USE_TESTS], [test x"$use_tests" != x"no"])
 AM_CONDITIONAL([USE_EXHAUSTIVE_TESTS], [test x"$use_exhaustive_tests" != x"no"])
@@ -532,13 +534,15 @@ fi
 echo
 echo "  valgrind                = $enable_valgrind"
 echo "  CC                      = $CC"
-echo "  CFLAGS                  = $CFLAGS"
 echo "  CPPFLAGS                = $CPPFLAGS"
+echo "  SECP_CFLAGS             = $SECP_CFLAGS"
+echo "  CFLAGS                  = $CFLAGS"
 echo "  LDFLAGS                 = $LDFLAGS"
 echo
 if test x"$set_precomp" = x"yes"; then
 echo "  CC_FOR_BUILD            = $CC_FOR_BUILD"
-echo "  CFLAGS_FOR_BUILD        = $CFLAGS_FOR_BUILD"
 echo "  CPPFLAGS_FOR_BUILD      = $CPPFLAGS_FOR_BUILD"
+echo "  SECP_CFLAGS_FOR_BUILD   = $SECP_CFLAGS_FOR_BUILD"
+echo "  CFLAGS_FOR_BUILD        = $CFLAGS_FOR_BUILD"
 echo "  LDFLAGS_FOR_BUILD       = $LDFLAGS_FOR_BUILD"
 fi
diff --git a/contrib/lax_der_parsing.c b/contrib/lax_der_parsing.c
index bf56230..885a817 100644
--- a/contrib/lax_der_parsing.c
+++ b/contrib/lax_der_parsing.c
@@ -120,7 +120,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_
     /* Copy R value */
     if (rlen > 32) {
         overflow = 1;
-    } else if (rlen) {
+    } else {
         memcpy(tmpsig + 32 - rlen, input + rpos, rlen);
     }
 
@@ -132,7 +132,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_
     /* Copy S value */
     if (slen > 32) {
         overflow = 1;
-    } else if (slen) {
+    } else {
         memcpy(tmpsig + 64 - slen, input + spos, slen);
     }
 
diff --git a/contrib/lax_der_privatekey_parsing.c b/contrib/lax_der_privatekey_parsing.c
index a1b8200..372e84e 100644
--- a/contrib/lax_der_privatekey_parsing.c
+++ b/contrib/lax_der_privatekey_parsing.c
@@ -44,7 +44,7 @@ int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *out32, co
     if (end < privkey+2 || privkey[0] != 0x04 || privkey[1] > 0x20 || end < privkey+2+privkey[1]) {
         return 0;
     }
-    if (privkey[1]) memcpy(out32 + 32 - privkey[1], privkey + 2, privkey[1]);
+    memcpy(out32 + 32 - privkey[1], privkey + 2, privkey[1]);
     if (!secp256k1_ec_seckey_verify(ctx, out32)) {
         memset(out32, 0, 32);
         return 0;
diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h
index c32141e..156a33d 100644
--- a/src/ecdsa_impl.h
+++ b/src/ecdsa_impl.h
@@ -140,7 +140,7 @@ static int secp256k1_der_parse_integer(secp256k1_scalar *r, const unsigned char
         overflow = 1;
     }
     if (!overflow) {
-        if (rlen) memcpy(ra + 32 - rlen, *sig, rlen);
+        memcpy(ra + 32 - rlen, *sig, rlen);
         secp256k1_scalar_set_b32(r, ra, &overflow);
     }
     if (overflow) {


Test patch:
diff --git a/src/tests.c b/src/tests.c
index a7cc240..6ceaba5 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -2508,70 +2508,6 @@ void run_field_misc(void) {
     }
 }
 
-void test_fe_mul(const secp256k1_fe* a, const secp256k1_fe* b, int use_sqr)
-{
-    secp256k1_fe c, an, bn;
-    /* Variables in BE 32-byte format. */
-    unsigned char a32[32], b32[32], c32[32];
-    /* Variables in LE 16x uint16_t format. */
-    uint16_t a16[16], b16[16], c16[16];
-    /* Field modulus in LE 16x uint16_t format. */
-    static const uint16_t m16[16] = {
-        0xfc2f, 0xffff, 0xfffe, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff,
-        0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff,
-    };
-    uint16_t t16[32];
-    int i;
-
-    /* Compute C = A * B in fe format. */
-    c = *a;
-    if (use_sqr) {
-        secp256k1_fe_sqr(&c, &c);
-    } else {
-        secp256k1_fe_mul(&c, &c, b);
-    }
-
-    /* Convert A, B, C into LE 16x uint16_t format. */
-    an = *a;
-    bn = *b;
-    secp256k1_fe_normalize_var(&c);
-    secp256k1_fe_normalize_var(&an);
-    secp256k1_fe_normalize_var(&bn);
-    secp256k1_fe_get_b32(a32, &an);
-    secp256k1_fe_get_b32(b32, &bn);
-    secp256k1_fe_get_b32(c32, &c);
-    for (i = 0; i < 16; ++i) {
-        a16[i] = a32[31 - 2*i] + ((uint16_t)a32[30 - 2*i] << 8);
-        b16[i] = b32[31 - 2*i] + ((uint16_t)b32[30 - 2*i] << 8);
-        c16[i] = c32[31 - 2*i] + ((uint16_t)c32[30 - 2*i] << 8);
-    }
-    /* Compute T = A * B in LE 16x uint16_t format. */
-    mulmod256(t16, a16, b16, m16);
-    /* Compare */
-    CHECK(secp256k1_memcmp_var(t16, c16, 32) == 0);
-}
-
-void run_fe_mul(void) {
-    int i;
-    for (i = 0; i < 100 * count; ++i) {
-        secp256k1_fe a, b, c, d;
-        random_fe(&a);
-        random_field_element_magnitude(&a);
-        random_fe(&b);
-        random_field_element_magnitude(&b);
-        random_fe_test(&c);
-        random_field_element_magnitude(&c);
-        random_fe_test(&d);
-        random_field_element_magnitude(&d);
-        test_fe_mul(&a, &a, 1);
-        test_fe_mul(&c, &c, 1);
-        test_fe_mul(&a, &b, 0);
-        test_fe_mul(&a, &c, 0);
-        test_fe_mul(&c, &b, 0);
-        test_fe_mul(&c, &d, 0);
-    }
-}
-
 void run_sqr(void) {
     secp256k1_fe x, s;
 
@@ -2659,7 +2595,7 @@ void test_inverse_scalar(secp256k1_scalar* out, const secp256k1_scalar* x, int v
 {
     secp256k1_scalar l, r, t;
 
-    (var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse)(&l, x);  /* l = 1/x */
+    (var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse_var)(&l, x);  /* l = 1/x */
     if (out) *out = l;
     if (secp256k1_scalar_is_zero(x)) {
         CHECK(secp256k1_scalar_is_zero(&l));
@@ -2669,9 +2605,9 @@ void test_inverse_scalar(secp256k1_scalar* out, const secp256k1_scalar* x, int v
     CHECK(secp256k1_scalar_is_one(&t));                                          /* x*(1/x) == 1 */
     secp256k1_scalar_add(&r, x, &scalar_minus_one);                              /* r = x-1 */
     if (secp256k1_scalar_is_zero(&r)) return;
-    (var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse)(&r, &r); /* r = 1/(x-1) */
+    (var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse_var)(&r, &r); /* r = 1/(x-1) */
     secp256k1_scalar_add(&l, &scalar_minus_one, &l);                             /* l = 1/x-1 */
-    (var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse)(&l, &l); /* l = 1/(1/x-1) */
+    (var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse_var)(&l, &l); /* l = 1/(1/x-1) */
     secp256k1_scalar_add(&l, &l, &secp256k1_scalar_one);                         /* l = 1/(1/x-1)+1 */
     secp256k1_scalar_add(&l, &r, &l);                                            /* l = 1/(1/x-1)+1 + 1/(x-1) */
     CHECK(secp256k1_scalar_is_zero(&l));                                         /* l == 0 */
@@ -6576,7 +6512,6 @@ int main(int argc, char **argv) {
     /* field tests */
     run_field_misc();
     run_field_convert();
-    run_fe_mul();
     run_sqr();
     run_sqrt();