Skip to content

Commit

Permalink
[SECP256K1] Make scalar/field choice depend on C-detected __int128 av…
Browse files Browse the repository at this point in the history 
…ailability

Summary:
```
This PR does two things:

    It removes the ability to select the 5x52 field with a 8x32 scalar,
or the 10x26 field with a 4x64 scalar. It's both 128-bit wide versions,
or neither.
    The choice is made automatically by the C code, unless overridden by
a USE_FORCE_WIDEMUL_INT{64,128} define (which is available through
configure with a hidden option
--with-test-override-wide-multiplication={auto,int64,int128}).

This reduces the reliance on autoconf for this performance-critical
configuration option, and also reduces the number of different
combinations to test.

This removes one theoretically useful combination: if you had x86_64 asm
but no __int128 support in your compiler, it was possible to use the
64-bit field before but the 32-bit scalar. I think this doesn't matter
as all compilers/systems that support (our) x86_64 asm also support
__int128. Furthermore, #767 will break this.

As an unexpected side effect, this also means the gen_context static
precomputation tool will now use __int128 based implementations when
available (which required an addition to the 5x52 field; see first
commit).
```

Backport of secp2561k [[bitcoin-core/secp256k1#793 | PR793]].

Depends on D7610.

Test Plan:
  cmake -GNinja ..
  ninja check-secp256k1

  cmake -GNinja .. -DSECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY=int64
  ninja check-secp256k1

  cmake -GNinja .. -DSECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY=int128
  ninja check-secp256k1

  ../configure
  make -j4 check

  ../configure --with-test-override-wide-multiply=int64
  make -j4 check

  ../configure --with-test-override-wide-multiply=int128
  make -j4 check

Run the Travis build.
https://travis-ci.org/github/Fabcien/secp256k1/builds/731196575

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7629
  • Loading branch information
@sipa @Fabcien
sipa authored and Fabcien committed Sep 29, 2020
1 parent a894289 commit 736660e
Show file tree
Hide file tree
Showing 13 changed files with 83 additions and 195 deletions.
21 changes: 10 additions & 11 deletions .travis.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,8 @@ compiler:
- gcc
env:
global:
- FIELD=auto
- WIDEMUL=auto
- BIGNUM=gmp
- SCALAR=auto
- ENDOMORPHISM=no
- STATICPRECOMPUTATION=yes
- ECMULTGENPRECISION=auto
Expand All @@ -57,15 +56,15 @@ env:
- BENCH=yes
- ITERS=2
jobs:
- SCALAR=32bit RECOVERY=yes
- SCALAR=32bit FIELD=32bit ECDH=yes EXPERIMENTAL=yes MULTISET=yes
- SCALAR=64bit
- FIELD=64bit RECOVERY=yes
- FIELD=64bit ENDOMORPHISM=yes
- FIELD=64bit ENDOMORPHISM=yes ECDH=yes EXPERIMENTAL=yes MULTISET=yes
- FIELD=64bit ASM=x86_64
- FIELD=64bit ENDOMORPHISM=yes ASM=x86_64
- FIELD=32bit ENDOMORPHISM=yes
- WIDEMUL=int64 RECOVERY=yes
- WIDEMUL=int64 ECDH=yes EXPERIMENTAL=yes MULTISET=yes
- WIDEMUL=int64 ENDOMORPHISM=yes
- WIDEMUL=int128
- WIDEMUL=int128 RECOVERY=yes
- WIDEMUL=int128 ENDOMORPHISM=yes
- WIDEMUL=int128 ENDOMORPHISM=yes ECDH=yes EXPERIMENTAL=yes MULTISET=yes
- WIDEMUL=int128 ASM=x86_64
- WIDEMUL=int128 ENDOMORPHISM=yes ASM=x86_64
- BIGNUM=no
- BIGNUM=no ENDOMORPHISM=yes RECOVERY=yes EXPERIMENTAL=yes MULTISET=yes
- BIGNUM=no STATICPRECOMPUTATION=no
Expand Down
66 changes: 8 additions & 58 deletions CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,64 +134,13 @@ if(SECP256K1_USE_ASM)
endif()
endif()

# We make sure __int128 is defined
include(CheckTypeSize)
check_type_size(__int128 SIZEOF___INT128)
if(SIZEOF___INT128 EQUAL 16)
set(HAVE___INT128 1)
else()
# If we do not support __int128, we should be falling back
# on 32bits implementations for field and scalar.
endif()

# Select the finite field implementation to use.
# This can be autodetected or forced by setting USE_FIELD to 32bit or 64bit.
# See the truth table below:
# +----------------------------------------------------------------------------------------------------------------------+
# | USE_FIELD=64bit | USE_FIELD=32bit | HAVE___INT128 | USE_ASM_X86_64 | USE_FIELD_5X52 | USE_FIELD_10x26 | Config error |
# +----------------------------------------------------------------------------------------------------------------------+
# | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
# | 0 | 0 | 0 | 1 | 1 | 0 | 0 |
# | 0 | 0 | 1 | 0 | 1 | 0 | 0 |
# | 0 | 0 | 1 | 1 | 1 | 0 | 0 |
# | 0 | 1 | 0 | 0 | 0 | 1 | 0 |
# | 0 | 1 | 0 | 1 | 0 | 1 | 0 |
# | 0 | 1 | 1 | 0 | 0 | 1 | 0 |
# | 0 | 1 | 1 | 1 | 0 | 1 | 0 |
# | 1 | 0 | 0 | 0 | 0 | 0 | 1 |
# | 1 | 0 | 0 | 1 | 1 | 0 | 0 |
# | 1 | 0 | 1 | 0 | 1 | 0 | 0 |
# | 1 | 0 | 1 | 1 | 1 | 0 | 0 |
# +----------------------------------------------------------------------------------------------------------------------+
set(USE_FIELD "" CACHE STRING "Force the finite field implementation to use (can be 32bit or 64bit)")
if(USE_FIELD STREQUAL "64bit" AND NOT (HAVE___INT128 OR USE_ASM_X86_64))
message(SEND_ERROR "64 finite field requested but the compiler does not support __int128 or inline assembly")
elseif(NOT USE_FIELD STREQUAL "32bit" AND (HAVE___INT128 OR USE_ASM_X86_64))
set(USE_FIELD_5X52 1)
else()
set(USE_FIELD_10X26 1)
endif()

# Select the scalar implementation to use.
# This can be autodetected or forced by setting USE_SCALAR to 32bit or 64bit.
# See the truth table below:
# +--------------------------------------------------------------------------------------------------------+
# | USE_SCALAR=64bit | USE_SCALAR=32bit | HAVE___INT128 | USE_SCALAR_4X64 | USE_SCALAR_8X32 | Config error |
# +--------------------------------------------------------------------------------------------------------+
# | 0 | 0 | 0 | 0 | 1 | 0 |
# | 0 | 0 | 1 | 1 | 0 | 0 |
# | 0 | 1 | 0 | 0 | 1 | 0 |
# | 0 | 1 | 1 | 0 | 1 | 0 |
# | 1 | 0 | 0 | 0 | 0 | 1 |
# | 1 | 0 | 1 | 1 | 0 | 0 |
# +--------------------------------------------------------------------------------------------------------+
set(USE_SCALAR "" CACHE STRING "Force the scalar implementation to use (can be 32bit or 64bit)")
if(USE_SCALAR STREQUAL "64bit" AND NOT HAVE___INT128)
message(SEND_ERROR "64 scalar requested but the compiler does not support __int128")
elseif(NOT USE_SCALAR STREQUAL "32bit" AND HAVE___INT128)
set(USE_SCALAR_4X64 1)
else()
set(USE_SCALAR_8X32 1)
set(SECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY "" CACHE STRING "Test-only override of the (autodetected by the C code) \"widemul\" setting (can be int64 or int128)")
if(SECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY STREQUAL "int128")
message(STATUS "Force the use of the (unsigned) __int128 based wide multiplication implementation")
target_compile_definitions(secp256k1 PUBLIC USE_FORCE_WIDEMUL_INT128=1)
elseif(SECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY STREQUAL "int64")
message(STATUS "Force the use of the (u)int64_t based wide multiplication implementation")
target_compile_definitions(secp256k1 PUBLIC USE_FORCE_WIDEMUL_INT64=1)
endif()

option(SECP256K1_BUILD_TEST "Build secp256k1's unit tests" ON)
Expand Down Expand Up @@ -311,6 +260,7 @@ if(SECP256K1_ECMULT_STATIC_PRECOMPUTATION)
"-DSECP256K1_ECMULT_WINDOW_SIZE=${SECP256K1_ECMULT_WINDOW_SIZE}"
"-DSECP256K1_ECMULT_GEN_PRECISION=${SECP256K1_ECMULT_GEN_PRECISION}"
"-DSECP256K1_USE_ASM=OFF"
"-DSECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY=${SECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY}"
)
add_native_executable(gen_context src/gen_context.c)

Expand Down
5 changes: 0 additions & 5 deletions build-aux/m4/bitcoin_secp.m4
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
dnl libsecp25k1 helper checks
AC_DEFUN([SECP_INT128_CHECK],[
has_int128=$ac_cv_type___int128
])

dnl escape "$0x" below using the m4 quadrigaph @S|@, and escape it again with a \ for the shell.
AC_DEFUN([SECP_64BIT_ASM_CHECK],[
AC_MSG_CHECKING(for x86_64 assembly availability)
Expand Down
104 changes: 17 additions & 87 deletions configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,20 +151,18 @@ AC_ARG_ENABLE(external_default_callbacks,
[use_external_default_callbacks=$enableval],
[use_external_default_callbacks=no])

dnl Test-only override of the (autodetected by the C code) "widemul" setting.
dnl Legal values are int64 (for [u]int64_t), int128 (for [unsigned] __int128), and auto (the default).
AC_ARG_WITH([test-override-wide-multiply], [] ,[set_widemul=$withval], [set_widemul=auto])

AC_ARG_ENABLE(jni,
AS_HELP_STRING([--enable-jni],[enable libsecp256k1_jni [default=no]]),
[use_jni=$enableval],
[use_jni=no])

AC_ARG_WITH([field], [AS_HELP_STRING([--with-field=64bit|32bit|auto],
[finite field implementation to use [default=auto]])],[req_field=$withval], [req_field=auto])

AC_ARG_WITH([bignum], [AS_HELP_STRING([--with-bignum=gmp|no|auto],
[bignum implementation to use [default=auto]])],[req_bignum=$withval], [req_bignum=auto])

AC_ARG_WITH([scalar], [AS_HELP_STRING([--with-scalar=64bit|32bit|auto],
[scalar implementation to use [default=auto]])],[req_scalar=$withval], [req_scalar=auto])

AC_ARG_WITH([asm], [AS_HELP_STRING([--with-asm=x86_64|arm|no|auto],
[assembly optimizations to use (experimental: arm) [default=auto]])],[req_asm=$withval], [req_asm=auto])

Expand All @@ -185,8 +183,6 @@ AC_ARG_WITH([ecmult-gen-precision], [AS_HELP_STRING([--with-ecmult-gen-precision
)],
[req_ecmult_gen_precision=$withval], [req_ecmult_gen_precision=auto])

AC_CHECK_TYPES([__int128])

AC_ARG_WITH([valgrind], [AS_HELP_STRING([--with-valgrind=yes|no|auto],
[Build with extra checks for running inside Valgrind [default=auto]]
)],
Expand Down Expand Up @@ -294,63 +290,6 @@ else
esac
fi

if test x"$req_field" = x"auto"; then
if test x"set_asm" = x"x86_64"; then
set_field=64bit
fi
if test x"$set_field" = x; then
SECP_INT128_CHECK
if test x"$has_int128" = x"yes"; then
set_field=64bit
fi
fi
if test x"$set_field" = x; then
set_field=32bit
fi
else
set_field=$req_field
case $set_field in
64bit)
if test x"$set_asm" != x"x86_64"; then
SECP_INT128_CHECK
if test x"$has_int128" != x"yes"; then
AC_MSG_ERROR([64bit field explicitly requested but neither __int128 support or x86_64 assembly available])
fi
fi
;;
32bit)
;;
*)
AC_MSG_ERROR([invalid field implementation selection])
;;
esac
fi

if test x"$req_scalar" = x"auto"; then
SECP_INT128_CHECK
if test x"$has_int128" = x"yes"; then
set_scalar=64bit
fi
if test x"$set_scalar" = x; then
set_scalar=32bit
fi
else
set_scalar=$req_scalar
case $set_scalar in
64bit)
SECP_INT128_CHECK
if test x"$has_int128" != x"yes"; then
AC_MSG_ERROR([64bit scalar explicitly requested but __int128 support not available])
fi
;;
32bit)
;;
*)
AC_MSG_ERROR([invalid scalar implementation selected])
;;
esac
fi

if test x"$req_bignum" = x"auto"; then
SECP_GMP_CHECK
if test x"$has_gmp" = x"yes"; then
Expand Down Expand Up @@ -394,16 +333,18 @@ no)
;;
esac

# select field implementation
case $set_field in
64bit)
AC_DEFINE(USE_FIELD_5X52, 1, [Define this symbol to use the FIELD_5X52 implementation])
# select wide multiplication implementation
case $set_widemul in
int128)
AC_DEFINE(USE_FORCE_WIDEMUL_INT128, 1, [Define this symbol to force the use of the (unsigned) __int128 based wide multiplication implementation])
;;
int64)
AC_DEFINE(USE_FORCE_WIDEMUL_INT64, 1, [Define this symbol to force the use of the (u)int64_t based wide multiplication implementation])
;;
32bit)
AC_DEFINE(USE_FIELD_10X26, 1, [Define this symbol to use the FIELD_10X26 implementation])
auto)
;;
*)
AC_MSG_ERROR([invalid field implementation])
AC_MSG_ERROR([invalid wide multiplication implementation])
;;
esac

Expand All @@ -425,19 +366,6 @@ no)
;;
esac

#select scalar implementation
case $set_scalar in
64bit)
AC_DEFINE(USE_SCALAR_4X64, 1, [Define this symbol to use the 4x64 scalar implementation])
;;
32bit)
AC_DEFINE(USE_SCALAR_8X32, 1, [Define this symbol to use the 8x32 scalar implementation])
;;
*)
AC_MSG_ERROR([invalid scalar implementation])
;;
esac

#set ecmult window size
if test x"$req_ecmult_window" = x"auto"; then
set_ecmult_window=15
Expand Down Expand Up @@ -621,10 +549,12 @@ echo " module schnorr = $enable_module_schnorr"
echo
echo " asm = $set_asm"
echo " bignum = $set_bignum"
echo " field = $set_field"
echo " scalar = $set_scalar"
echo " ecmult window size = $set_ecmult_window"
echo " ecmult gen prec. bits = $set_ecmult_gen_precision"
dnl Hide test-only options unless they're used.
if test x"$set_widemul" != xauto; then
echo " wide multiplication = $set_widemul"
fi
echo
echo " valgrind = $enable_valgrind"
echo " CC = $CC"
Expand Down
10 changes: 3 additions & 7 deletions src/basic-config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,20 @@
#undef USE_ENDOMORPHISM
#undef USE_EXTERNAL_ASM
#undef USE_EXTERNAL_DEFAULT_CALLBACKS
#undef USE_FIELD_10X26
#undef USE_FIELD_5X52
#undef USE_FIELD_INV_BUILTIN
#undef USE_FIELD_INV_NUM
#undef USE_NUM_GMP
#undef USE_NUM_NONE
#undef USE_SCALAR_4X64
#undef USE_SCALAR_8X32
#undef USE_SCALAR_INV_BUILTIN
#undef USE_SCALAR_INV_NUM
#undef USE_FORCE_WIDEMUL_INT64
#undef USE_FORCE_WIDEMUL_INT128
#undef ECMULT_WINDOW_SIZE
#undef HAVE___INT128 /* used in util.h */

#define USE_NUM_NONE 1
#define USE_FIELD_INV_BUILTIN 1
#define USE_SCALAR_INV_BUILTIN 1
#define USE_FIELD_10X26 1
#define USE_SCALAR_8X32 1
#define USE_WIDEMUL_64 1
#define ECMULT_WINDOW_SIZE 15

#endif /* USE_BASIC_CONFIG */
Expand Down
12 changes: 6 additions & 6 deletions src/field.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,16 @@
#include "libsecp256k1-config.h"
#endif

#if defined(USE_FIELD_10X26)
#include "field_10x26.h"
#elif defined(USE_FIELD_5X52)
#include "util.h"

#if defined(SECP256K1_WIDEMUL_INT128)
#include "field_5x52.h"
#elif defined(SECP256K1_WIDEMUL_INT64)
#include "field_10x26.h"
#else
#error "Please select field implementation"
#error "Please select wide multiplication implementation"
#endif

#include "util.h"

/** Normalize a field element. This brings the field element to a canonical representation, reduces
* its magnitude to 1, and reduces it modulo field size `p`.
*/
Expand Down
6 changes: 6 additions & 0 deletions src/field_5x52.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,10 @@ typedef struct {
(d6) | (((uint64_t)(d7)) << 32) \
}}

#define SECP256K1_FE_STORAGE_CONST_GET(d) \
(uint32_t)(d.n[3] >> 32), (uint32_t)d.n[3], \
(uint32_t)(d.n[2] >> 32), (uint32_t)d.n[2], \
(uint32_t)(d.n[1] >> 32), (uint32_t)d.n[1], \
(uint32_t)(d.n[0] >> 32), (uint32_t)d.n[0]

#endif /* SECP256K1_FIELD_REPR_H */
8 changes: 4 additions & 4 deletions src/field_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
#include "util.h"
#include "num.h"

#if defined(USE_FIELD_10X26)
#include "field_10x26_impl.h"
#elif defined(USE_FIELD_5X52)
#if defined(SECP256K1_WIDEMUL_INT128)
#include "field_5x52_impl.h"
#elif defined(SECP256K1_WIDEMUL_INT64)
#include "field_10x26_impl.h"
#else
#error "Please select field implementation"
#error "Please select wide multiplication implementation"
#endif

SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) {
Expand Down
7 changes: 4 additions & 3 deletions src/scalar.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,20 @@
#define SECP256K1_SCALAR_H

#include "num.h"
#include "util.h"

#if defined HAVE_CONFIG_H
#include "libsecp256k1-config.h"
#endif

#if defined(EXHAUSTIVE_TEST_ORDER)
#include "scalar_low.h"
#elif defined(USE_SCALAR_4X64)
#elif defined(SECP256K1_WIDEMUL_INT128)
#include "scalar_4x64.h"
#elif defined(USE_SCALAR_8X32)
#elif defined(SECP256K1_WIDEMUL_INT64)
#include "scalar_8x32.h"
#else
#error "Please select scalar implementation"
#error "Please select wide multiplication implementation"
#endif

/** Clear a scalar to prevent the leak of sensitive data. */
Expand Down
Loading

0 comments on commit 736660e

Please sign in to comment.