-
Notifications
You must be signed in to change notification settings - Fork 20.7k
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
crypto/secp256k1: update libsecp256k1 #31242
Conversation
yep broken rn, fixing |
It would be nice to sanity check this on all platforms |
crypto/secp256k1/secp256.go
Outdated
#define USE_SCALAR_INV_BUILTIN | ||
#define ECMULTWINDOW 15 | ||
#define ECMULTGENKB 22 | ||
#define WIDEMUL auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do anything. We are defining C preprocessor macros here. I checked, and libsecp256k1 does not use this macro. It uses macros like this:
#if defined(EXHAUSTIVE_TEST_ORDER)
#include "scalar_low_impl.h"
#elif defined(SECP256K1_WIDEMUL_INT128)
#include "scalar_4x64_impl.h"
#elif defined(SECP256K1_WIDEMUL_INT64)
#include "scalar_8x32_impl.h"
#else
#error "Please select wide multiplication implementation"
#endif
So we need to define either SECP256K1_WIDEMUL_INT128
or SECP256K1_WIDEMUL_INT64
depending on int128
availability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mistook autoconf feature names for #define
s here. For the ECMULTWINDOW
setting, you need to define ECMULT_WINDOW_SIZE
. The default value is 15
, so you can just leave this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I just checked and libsecp256k1 has compile-time autodetection for int128: https://github.com/bitcoin-core/secp256k1/blob/1b1fc09341c956e8918adba1eeaa43b47d73ea84/src/util.h#L303-L332
So we don't need to define anything here and it will choose the defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we need to define it ourselves? Seems like they did so in util.h
#if defined(USE_FORCE_WIDEMUL_INT128_STRUCT)
/* If USE_FORCE_WIDEMUL_INT128_STRUCT is set, use int128_struct. */
# define SECP256K1_WIDEMUL_INT128 1
# define SECP256K1_INT128_STRUCT 1
#elif defined(USE_FORCE_WIDEMUL_INT128)
/* If USE_FORCE_WIDEMUL_INT128 is set, use int128. */
# define SECP256K1_WIDEMUL_INT128 1
# define SECP256K1_INT128_NATIVE 1
#elif defined(USE_FORCE_WIDEMUL_INT64)
/* If USE_FORCE_WIDEMUL_INT64 is set, use int64. */
# define SECP256K1_WIDEMUL_INT64 1
#elif defined(UINT128_MAX) || defined(__SIZEOF_INT128__)
/* If a native 128-bit integer type exists, use int128. */
# define SECP256K1_WIDEMUL_INT128 1
# define SECP256K1_INT128_NATIVE 1
#elif defined(_MSC_VER) && (defined(_M_X64) || defined(_M_ARM64))
/* On 64-bit MSVC targets (x86_64 and arm64), use int128_struct
* (which has special logic to implement using intrinsics on those systems). */
# define SECP256K1_WIDEMUL_INT128 1
# define SECP256K1_INT128_STRUCT 1
#elif SIZE_MAX > 0xffffffff
/* Systems with 64-bit pointers (and thus registers) very likely benefit from
* using 64-bit based arithmetic (even if we need to fall back to 32x32->64 based
* multiplication logic). */
# define SECP256K1_WIDEMUL_INT128 1
# define SECP256K1_INT128_STRUCT 1
#else
/* Lastly, fall back to int64 based arithmetic. */
# define SECP256K1_WIDEMUL_INT64 1
#endif
Updates the libsecp256k1 dependency to commit: c0d9480fbbf8eccbd4be23ed27f6f2af6f3b211e PR: ``` BenchmarkSign-24 57756 21214 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 37156 33044 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 36889 32935 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 41163 29207 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 318624 4062 ns/op 304 B/op 6 allocs/op ``` Master: ``` BenchmarkSign-24 34509 35330 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 25418 47725 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 25735 47591 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 29108 41097 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 294747 4143 ns/op 304 B/op 6 allocs/op ``` Performance seems to be improved significantly: ``` Sign-24 34.86µ ± 3% 21.66µ ± 2% -37.86% (p=0.000 n=10) Recover-24 46.14µ ± 3% 33.24µ ± 2% -27.95% (p=0.000 n=10) ```
Updates the libsecp256k1 dependency to commit: c0d9480fbbf8eccbd4be23ed27f6f2af6f3b211e PR: ``` BenchmarkSign-24 57756 21214 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 37156 33044 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 36889 32935 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 41163 29207 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 318624 4062 ns/op 304 B/op 6 allocs/op ``` Master: ``` BenchmarkSign-24 34509 35330 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 25418 47725 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 25735 47591 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 29108 41097 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 294747 4143 ns/op 304 B/op 6 allocs/op ``` Performance seems to be improved significantly: ``` Sign-24 34.86µ ± 3% 21.66µ ± 2% -37.86% (p=0.000 n=10) Recover-24 46.14µ ± 3% 33.24µ ± 2% -27.95% (p=0.000 n=10) ```
Updates the libsecp256k1 dependency to commit: c0d9480fbbf8eccbd4be23ed27f6f2af6f3b211e PR: ``` BenchmarkSign-24 57756 21214 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 37156 33044 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 36889 32935 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 41163 29207 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 318624 4062 ns/op 304 B/op 6 allocs/op ``` Master: ``` BenchmarkSign-24 34509 35330 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 25418 47725 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 25735 47591 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 29108 41097 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 294747 4143 ns/op 304 B/op 6 allocs/op ``` Performance seems to be improved significantly: ``` Sign-24 34.86µ ± 3% 21.66µ ± 2% -37.86% (p=0.000 n=10) Recover-24 46.14µ ± 3% 33.24µ ± 2% -27.95% (p=0.000 n=10) ```
Updates the libsecp256k1 dependency to commit: c0d9480fbbf8eccbd4be23ed27f6f2af6f3b211e PR: ``` BenchmarkSign-24 57756 21214 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 37156 33044 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 36889 32935 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 41163 29207 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 318624 4062 ns/op 304 B/op 6 allocs/op ``` Master: ``` BenchmarkSign-24 34509 35330 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 25418 47725 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 25735 47591 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 29108 41097 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 294747 4143 ns/op 304 B/op 6 allocs/op ``` Performance seems to be improved significantly: ``` Sign-24 34.86µ ± 3% 21.66µ ± 2% -37.86% (p=0.000 n=10) Recover-24 46.14µ ± 3% 33.24µ ± 2% -27.95% (p=0.000 n=10) ```
Updates the libsecp256k1 dependency to commit: c0d9480fbbf8eccbd4be23ed27f6f2af6f3b211e PR: ``` BenchmarkSign-24 57756 21214 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 37156 33044 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 36889 32935 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 41163 29207 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 318624 4062 ns/op 304 B/op 6 allocs/op ``` Master: ``` BenchmarkSign-24 34509 35330 ns/op 164 B/op 3 allocs/op BenchmarkRecover-24 25418 47725 ns/op 80 B/op 1 allocs/op BenchmarkEcrecoverSignature-24 25735 47591 ns/op 80 B/op 1 allocs/op BenchmarkVerifySignature-24 29108 41097 ns/op 0 B/op 0 allocs/op BenchmarkDecompressPubkey-24 294747 4143 ns/op 304 B/op 6 allocs/op ``` Performance seems to be improved significantly: ``` Sign-24 34.86µ ± 3% 21.66µ ± 2% -37.86% (p=0.000 n=10) Recover-24 46.14µ ± 3% 33.24µ ± 2% -27.95% (p=0.000 n=10) ```
Updates the libsecp256k1 dependency to commit: c0d9480fbbf8eccbd4be23ed27f6f2af6f3b211e
PR:
Master:
Performance seems to be improved quite significantly: