Merge bitcoin-core/secp256k1#1209: build: Add SECP256K1_API_VAR to fix importing variables from DLLs

e4330341bd ci: Shutdown wineserver whenever CI script exits (Tim Ruffing)
9a5a611a21 build: Suppress stupid MSVC linker warning (Tim Ruffing)
739c53b19a examples: Extend sig examples by call that uses static context (Tim Ruffing)
914276e4d2 build: Add SECP256K1_API_VAR to fix importing variables from DLLs (Tim Ruffing)

Pull request description:

  ... and more Windows fixes, please see the individual commits.

  The fixed issues were discovered in https://github.com/bitcoin-core/secp256k1/pull/1198.

ACKs for top commit:
  sipa:
    utACK e4330341bd
  hebasto:
    ACK e4330341bd, tested on Windows using [CMake](https://github.com/bitcoin-core/secp256k1/pull/1113) (which means that the 3rd commit is reviewed only, but not tested). FWIW, `LNK4217` warnings have been indeed observed.

Tree-SHA512: ce7845b106190cdc517988c30aaf2cc9f1d6da22904dfc5cb6bf4ee05f063929dc8b3038479e703b6cebac79d1c21d0c84560344d2478cb1c1740087383f40e3

.cirrus.yml

@@ -258,7 +258,7 @@ task:
     # 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
+    LDFLAGS: -Xlinker -Xlinker -Xlinker -nologo
   matrix:
     - name: "x86_64 (MSVC): Windows (Debian stable, Wine)"
     - name: "x86_64 (MSVC): Windows (Debian stable, Wine, int128_struct)"

ci/cirrus.sh

@@ -34,6 +34,8 @@ print_environment
 # This speeds up jobs with many invocations of wine (e.g., ./configure with MSVC) tremendously.
 case "$WRAPPER_CMD" in
     *wine*)
+        # Make sure to shutdown wineserver whenever we exit.
+        trap "wineserver -k || true" EXIT INT HUP
         # This is apparently only reliable when we run a dummy command such as "hh.exe" afterwards.
         wineserver -p && wine hh.exe
         ;;
@@ -111,9 +113,6 @@ 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

configure.ac

@@ -115,6 +115,12 @@ AC_DEFUN([SECP_TRY_APPEND_DEFAULT_CFLAGS], [
     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
+      # We pass -ignore:4217 to the MSVC linker to suppress warning 4217 when
+      # importing variables from a statically linked secp256k1.
+      # (See the libtool manual, section "Windows DLLs" for background.)
+      # Unfortunately, libtool tries to be too clever and strips "-Xlinker arg"
+      # into "arg", so this will be " -Xlinker -ignore:4217" after stripping.
+      LDFLAGS="-Xlinker -Xlinker -Xlinker -ignore:4217 $LDFLAGS" 
     fi
 ])
 SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS)

examples/ecdsa.c

@@ -34,7 +34,7 @@ int main(void) {
     unsigned char compressed_pubkey[33];
     unsigned char serialized_signature[64];
     size_t len;
-    int is_signature_valid;
+    int is_signature_valid, is_signature_valid2;
     int return_val;
     secp256k1_pubkey pubkey;
     secp256k1_ecdsa_signature sig;
@@ -116,10 +116,18 @@ int main(void) {
     printf("Signature: ");
     print_hex(serialized_signature, sizeof(serialized_signature));
 
-
     /* This will clear everything from the context and free the memory */
     secp256k1_context_destroy(ctx);
 
+    /* Bonus example: if all we need is signature verification (and no key
+       generation or signing), we don't need to use a context created via
+       secp256k1_context_create(). We can simply use the static (i.e., global)
+       context secp256k1_context_static. See its description in
+       include/secp256k1.h for details. */
+    is_signature_valid2 = secp256k1_ecdsa_verify(secp256k1_context_static,
+                                                 &sig, msg_hash, &pubkey);
+    assert(is_signature_valid2 == is_signature_valid);
+
     /* It's best practice to try to clear secrets from memory after using them.
      * This is done because some bugs can allow an attacker to leak memory, for
      * example through "out of bounds" array access (see Heartbleed), Or the OS

examples/schnorr.c

@@ -26,7 +26,7 @@ int main(void) {
     unsigned char auxiliary_rand[32];
     unsigned char serialized_pubkey[32];
     unsigned char signature[64];
-    int is_signature_valid;
+    int is_signature_valid, is_signature_valid2;
     int return_val;
     secp256k1_xonly_pubkey pubkey;
     secp256k1_keypair keypair;
@@ -135,6 +135,15 @@ int main(void) {
     /* This will clear everything from the context and free the memory */
     secp256k1_context_destroy(ctx);
 
+    /* Bonus example: if all we need is signature verification (and no key
+       generation or signing), we don't need to use a context created via
+       secp256k1_context_create(). We can simply use the static (i.e., global)
+       context secp256k1_context_static. See its description in
+       include/secp256k1.h for details. */
+    is_signature_valid2 = secp256k1_schnorrsig_verify(secp256k1_context_static,
+                                                      signature, msg_hash, 32, &pubkey);
+    assert(is_signature_valid2 == is_signature_valid);
+
     /* It's best practice to try to clear secrets from memory after using them.
      * This is done because some bugs can allow an attacker to leak memory, for
      * example through "out of bounds" array access (see Heartbleed), Or the OS

include/secp256k1.h

@@ -145,21 +145,28 @@ typedef int (*secp256k1_nonce_function)(
 # define SECP256K1_NO_BUILD
 #endif
 
-/** At secp256k1 build-time DLL_EXPORT is defined when building objects destined
+/* Symbol visibility. See libtool manual, section "Windows DLLs". */
- *  for a shared library, but not for those intended for static libraries.
+#if defined(_WIN32) && !defined(__GNUC__)
- */
+# ifdef SECP256K1_BUILD
-
+#  ifdef DLL_EXPORT
-#ifndef SECP256K1_API
+#   define SECP256K1_API            __declspec (dllexport)
-# if defined(_WIN32)
+#   define SECP256K1_API_VAR extern __declspec (dllexport)
-#  if defined(SECP256K1_BUILD) && defined(DLL_EXPORT)
-#   define SECP256K1_API __declspec(dllexport)
-#  else
-#   define SECP256K1_API
 #  endif
-# elif defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
+# elif defined _MSC_VER
-#  define SECP256K1_API __attribute__ ((visibility ("default")))
+#  define SECP256K1_API
+#  define SECP256K1_API_VAR  extern __declspec (dllimport)
+# elif defined DLL_EXPORT
+#  define SECP256K1_API             __declspec (dllimport)
+#  define SECP256K1_API_VAR  extern __declspec (dllimport)
+# endif
+#endif
+#ifndef SECP256K1_API
+# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
+#  define SECP256K1_API             __attribute__ ((visibility ("default")))
+#  define SECP256K1_API_VAR  extern __attribute__ ((visibility ("default")))
 # else
 #  define SECP256K1_API
+#  define SECP256K1_API_VAR  extern
 # endif
 #endif
 
@@ -231,10 +238,10 @@ typedef int (*secp256k1_nonce_function)(
  *
  *  It is highly recommended to call secp256k1_selftest before using this context.
  */
-SECP256K1_API extern const secp256k1_context *secp256k1_context_static;
+SECP256K1_API_VAR const secp256k1_context *secp256k1_context_static;
 
 /** Deprecated alias for secp256k1_context_static. */
-SECP256K1_API extern const secp256k1_context *secp256k1_context_no_precomp
+SECP256K1_API_VAR const secp256k1_context *secp256k1_context_no_precomp
 SECP256K1_DEPRECATED("Use secp256k1_context_static instead");
 
 /** Perform basic self tests (to be used in conjunction with secp256k1_context_static)
@@ -631,10 +638,10 @@ SECP256K1_API int secp256k1_ecdsa_signature_normalize(
  * If a data pointer is passed, it is assumed to be a pointer to 32 bytes of
  * extra entropy.
  */
-SECP256K1_API extern const secp256k1_nonce_function secp256k1_nonce_function_rfc6979;
+SECP256K1_API_VAR const secp256k1_nonce_function secp256k1_nonce_function_rfc6979;
 
 /** A default safe nonce generation function (currently equal to secp256k1_nonce_function_rfc6979). */
-SECP256K1_API extern const secp256k1_nonce_function secp256k1_nonce_function_default;
+SECP256K1_API_VAR const secp256k1_nonce_function secp256k1_nonce_function_default;
 
 /** Create an ECDSA signature.
  *

include/secp256k1_ecdh.h

@@ -27,11 +27,11 @@ typedef int (*secp256k1_ecdh_hash_function)(
 
 /** An implementation of SHA256 hash function that applies to compressed public key.
  * Populates the output parameter with 32 bytes. */
-SECP256K1_API extern const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_sha256;
+SECP256K1_API_VAR const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_sha256;
 
 /** A default ECDH hash function (currently equal to secp256k1_ecdh_hash_function_sha256).
  * Populates the output parameter with 32 bytes. */
-SECP256K1_API extern const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_default;
+SECP256K1_API_VAR const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_default;
 
 /** Compute an EC Diffie-Hellman secret in constant time
  *

include/secp256k1_schnorrsig.h

@@ -61,7 +61,7 @@ typedef int (*secp256k1_nonce_function_hardened)(
  *  Therefore, to create BIP-340 compliant signatures, algo must be set to
  *  "BIP0340/nonce" and algolen to 13.
  */
-SECP256K1_API extern const secp256k1_nonce_function_hardened secp256k1_nonce_function_bip340;
+SECP256K1_API_VAR const secp256k1_nonce_function_hardened secp256k1_nonce_function_bip340;
 
 /** Data structure that contains additional arguments for schnorrsig_sign_custom.
  *