Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115 #236

Closed
wants to merge 24 commits into from

Conversation

jonasnick
Copy link
Contributor

[bitcoin-core/secp256k1#1056]: Save negations in var-time group addition
[bitcoin-core/secp256k1#1104]: Fix the false positive of SECP_64BIT_ASM_CHECK
[bitcoin-core/secp256k1#1105]: Don'''t export symbols in static libraries
[bitcoin-core/secp256k1#1084]: ci: Add MSVC builds
[bitcoin-core/secp256k1#1114]: _scratch_destroy: move VERIFY_CHECK after invalid scrach space check
[bitcoin-core/secp256k1#1115]: Fix sepc256k1 -> secp256k1 typo in group.h

This PR can be recreated with ./contrib/sync-upstream.sh range 43756da819a235d813e7ecd53eae6df073b53247.

real-or-random and others added 23 commits March 16, 2022 16:36
This removes a check for $ac_cv_prog_cc_c89 which is set by AC_PROG_CC
if defined(__STDC__) in the preprocessor. (Standard compliant compilers
are supposed to define __STDC__ to 1 but the value is actually not
checked here.)

Unfortunately, MSVC doesn't define it, so configure fails for MSVC.

This check is not very useful in practice. Over 30 years after C89 has
been released, there are no C compilers out there that are not
sufficiently compliant with C89 for the project. The only practically
relevant case was that the check rejected C++ compilers. A different
method to reject C++ compilers will be introduced in a later commit.
- Updated _gej_add_var, _gej_add_ge_var, _gej_add_zinv_var
- 2 fewer _fe_negate in each method
- Updated operation counts and standardize layout
- Added internal benchmark for _gej_add_zinv_var
- Update sage files (fixed by Tim Ruffing)
…ddition

2f984ff Save negations in var-time group addition (Peter Dettman)

Pull request description:

  - Updated _gej_add_var, _gej_add_ge_var, _gej_add_zinv_var
  - 2 fewer _fe_negate in each method
  - Updated operation counts and standardize layout
  - Added internal benchmark for _gej_add_zinv_var

  benchmark_internal shows about 2% speedup in each method as a result (64bit).

ACKs for top commit:
  real-or-random:
    ACK 2f984ff
  jonasnick:
    ACK 2f984ff

Tree-SHA512: 01366fa23c83a8dd37c9a0a24e0acc53ce38a201607fe4da6672ea5618d82c62d1299f0e0aa50317883821539af739ea52b6561faff230c148e6fdc5bc5af30b
…BIT_ASM_CHECK`

7efc983 Fix the false positive of `SECP_64BIT_ASM_CHECK` (Sprite)

Pull request description:

  I'm trying to compile this project for RISC-V architecture, and I encountered errors:
  ```
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r15' in 'asm'
     28 | __asm__ __volatile__(
        | ^
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r14' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r13' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r12' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r11' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r10' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r9' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r8' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%rdx' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%rcx' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: unknown register name '%rax' in 'asm'
  src/field_5x52_asm_impl.h:28:1: error: output number 0 not directly addressable
  src/field_5x52_asm_impl.h: In function 'secp256k1_fe_sqr':
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r15' in 'asm'
    298 | __asm__ __volatile__(
        | ^
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r14' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r13' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r12' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r11' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r10' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r9' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r8' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%rdx' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%rcx' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%rbx' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: unknown register name '%rax' in 'asm'
  src/field_5x52_asm_impl.h:298:1: error: output number 0 not directly addressable
  ```

  After further investigation I found that for RISC-V, macro `USE_ASM_X86_64` was defined unexpectedly, and `checking for x86_64 assembly availability... yes` appeared in the compilation log file, which means `SECP_64BIT_ASM_CHECK` was not working as expected.

  For unknown reasons, `AC_COMPILE_IFELSE` does not check if `__asm__` can be compiled, and an example can verify this point:
  ```m4
  AC_DEFUN([SECP_64BIT_ASM_CHECK],[
  AC_MSG_CHECKING(for x86_64 assembly availability)
  AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
    #include <stdint.h>]],[[
    __asm__ __volatile__("this is obviously wrong");
    ]])],[has_64bit_asm=yes],[has_64bit_asm=no])
  AC_MSG_RESULT([$has_64bit_asm])
  ])
  ```

  It always gives results: `checking for x86_64 assembly availability... yes`

  After testing, replacing `AC_COMPILE_IFELSE` with `AC_LINK_IFELSE` can correctly check if `__asm__` can be compiled and make the project able to compile for RISC-V.

ACKs for top commit:
  real-or-random:
    ACK 7efc983

Tree-SHA512: 7318dd42004b2930cfcd6541c5a9ce0aa186e2179a668b76089a908bea8d9f70fcfdb264512f971e395a3ce9dc7f9ca24c8f3d46175cad2972a2d713f518ed85
libtool takes care of building both object versions, we just need to pick the
right one to export symbols.
…raries

6f6cab9 abi: Don't export symbols in static Windows libraries (Cory Fields)

Pull request description:

  For context, Bitcoin Core has recently merged [libbitcoin-kernel](bitcoin/bitcoin#24322), a small library that intends to eventually minimally encompass Core's validation engine. This kernel lib includes a static libsecp256k1. Without this change, because libsecp256k1.a ends up with exported symbols, we end up with libsecp256k1 symbols exported by our libbitcoin-kernel library (which causes unrelated problems not worth getting into here).

  libtool takes care of building both object versions, and it automatically builds objects for shared libs with -DDLL_EXPORT. We just need to opt-in to its functionality.

  I can't imagine this having any negative impact on any current statically-linking applications, if anything they'll just be a tiny bit smaller because they can now strip unused symbols.

ACKs for top commit:
  real-or-random:
    utACK 6f6cab9
  theuni:
    > Not sure what other changes made compilation on CI fail but Concept ACK [6f6cab9](bitcoin-core/secp256k1@6f6cab9). This should be entirely harmless.
  sipa:
    utACK 6f6cab9
  laanwj:
    utACK 6f6cab9

Tree-SHA512: 39f240046639738f7a8c01068e728b2f9ceac2754cc4b0a5fa46c28f6f57a8c4124653b56dfbf5c13106b07c11ac599cc41b508e16862d539ce1af6c3365a205
This adds MSVC builds built on Linux using wine. This requires some
settings of tools and flags because the autotools support for MSVC is
naturally somewhat limited.

The advantage of this approach is that it is compatible with our
existing CI scripts, so there's no need to write a Windows CI script
(in PowerShell or similar). If we want to test building and running on
Windows native (e.g., as supported by Cirrus CI) we could still do this
in the future.

Another advantage of this approach is that contributors can simply use
the docker image if they need a MSVC installation in a non-Windows
environment.

This commit also improves the Dockerfile by grouping RUN commands
according to Docker docs:
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run
This commit also raises the TEST_ITERS for wine tasks to the default.
The overhead of wine is negligible, so we can certainly afford the same
number of iterations as for native Linux tests.
49e2acd configure: Improve rationale for WERROR_CFLAGS (Tim Ruffing)
8dc4b03 ci: Add a C++ job that compiles the public headers without -fpermissive (Tim Ruffing)
51f296a ci: Run persistent wineserver to speed up wine (Tim Ruffing)
3fb3269 ci: Add 32-bit MinGW64 build (Tim Ruffing)
9efc2e5 ci: Add MSVC builds (Tim Ruffing)
2be6ba0 configure: Convince autotools to work with MSVC's archiver lib.exe (Tim Ruffing)
bd81f41 schnorrsig bench: Suppress a stupid warning in MSVC (Tim Ruffing)
09f3d71 configure: Add a few CFLAGS for MSVC (Tim Ruffing)
3b4f3d0 build: Reject C++ compilers in the preprocessor (Tim Ruffing)
1cc0941 configure: Don't abort if the compiler does not define __STDC__ (Tim Ruffing)
cca8cbb configure: Output message when checking for valgrind (Tim Ruffing)
1a6be57 bench: Make benchmarks compile on MSVC (Tim Ruffing)

Pull request description:

ACKs for top commit:
  jonasnick:
    ACK 49e2acd

Tree-SHA512: 986c498fb218231fff3519167d34a92e11dea6a4383788a9723be105c20578cd483c6b06ba5686c6669e3a02cfeebc29b8e5f1428552ebf4ec67fa7a86957548
…HECK` after invalid scrach space check

1827c9b scratch_destroy: move VERIFY_CHECK after invalid scrach space check (siv2r)

Pull request description:

ACKs for top commit:
  sipa:
    utACK 1827c9b
  jonasnick:
    ACK 1827c9b

Tree-SHA512: 5c4f97ca16f46380b30d1d077a54e25eab21efb10e0ce3ca27e7f73a2318d130f0f0773e26b2fdfc8f9d98c1334880f9d80a51b0941be3ba0af2b656b7c0ea7e
… group.h

069aba8 Fix sepc256k1 -> secp256k1 typo in group.h (henopied)

Pull request description:

ACKs for top commit:
  real-or-random:
    ACK 069aba8

Tree-SHA512: 0fcb7d042f201737870da99f5425c8449e9ec3f5f8e9bbe5eb719e46cdf230db057509fb9102d4ce50a94d616015233c29249665c754e726899174fea3ea9f40
.cirrus.yml Outdated
Comment on lines 260 to 263
ECDH: yes
RECOVERY: yes
EXPERIMENTAL: yes
SCHNORRSIG: yes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably enable all our modules here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants