Skip to content

Commit 2b77240

Browse files
committed
Merge bitcoin-core/secp256k1#1172: benchmarks: fix bench_scalar_split
eb6beba scalar: restrict split_lambda args, improve doc and VERIFY_CHECKs (Jonas Nick) 7f49aa7 ci: add test job with -DVERIFY (Jonas Nick) 620ba3d benchmarks: fix bench_scalar_split (Jonas Nick) Pull request description: scalar_split_lambda requires that the input pointer is different to both output pointers. Without this fix, the internal benchmarks crash when compiled with -DVERIFY. This was introduced in commit bitcoin-core/secp256k1@362bb25 (which requires configuring with --enable-endomorphism to exhibit the crash). I tested that the new CI job would have caught this bug. ACKs for top commit: sipa: utACK eb6beba real-or-random: utACK eb6beba Tree-SHA512: c810545aefb01561ddb77b53618fa7acbb156ec13ab809c00523d4758492cafab1dfa01b6ebfb6195a3803bb49b16e63e8b0efcd1abb76ecefdb0476c3e483a3
2 parents 5fbff5d + eb6beba commit 2b77240

File tree

4 files changed

+14
-7
lines changed

4 files changed

+14
-7
lines changed

.cirrus.yml

+1
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ task:
8181
- env: {WIDEMUL: int128, ECDH: yes, SCHNORRSIG: yes}
8282
- env: {WIDEMUL: int128, ASM: x86_64}
8383
- env: { RECOVERY: yes, SCHNORRSIG: yes}
84+
- env: {CTIMETESTS: no, RECOVERY: yes, ECDH: yes, SCHNORRSIG: yes, CPPFLAGS: -DVERIFY}
8485
- env: {BUILD: distcheck, WITH_VALGRIND: no, CTIMETESTS: no, BENCH: no}
8586
- env: {CPPFLAGS: -DDETERMINISTIC}
8687
- env: {CFLAGS: -O0, CTIMETESTS: no}

src/bench_internal.c

+3-2
Original file line numberDiff line numberDiff line change
@@ -110,10 +110,11 @@ static void bench_scalar_mul(void* arg, int iters) {
110110
static void bench_scalar_split(void* arg, int iters) {
111111
int i, j = 0;
112112
bench_inv *data = (bench_inv*)arg;
113+
secp256k1_scalar tmp;
113114

114115
for (i = 0; i < iters; i++) {
115-
secp256k1_scalar_split_lambda(&data->scalar[0], &data->scalar[1], &data->scalar[0]);
116-
j += secp256k1_scalar_add(&data->scalar[0], &data->scalar[0], &data->scalar[1]);
116+
secp256k1_scalar_split_lambda(&tmp, &data->scalar[1], &data->scalar[0]);
117+
j += secp256k1_scalar_add(&data->scalar[0], &tmp, &data->scalar[1]);
117118
}
118119
CHECK(j <= iters);
119120
}

src/scalar.h

+4-3
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,10 @@ static int secp256k1_scalar_eq(const secp256k1_scalar *a, const secp256k1_scalar
8888

8989
/** Find r1 and r2 such that r1+r2*2^128 = k. */
9090
static void secp256k1_scalar_split_128(secp256k1_scalar *r1, secp256k1_scalar *r2, const secp256k1_scalar *k);
91-
/** Find r1 and r2 such that r1+r2*lambda = k,
92-
* where r1 and r2 or their negations are maximum 128 bits long (see secp256k1_ge_mul_lambda). */
93-
static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar *r2, const secp256k1_scalar *k);
91+
/** Find r1 and r2 such that r1+r2*lambda = k, where r1 and r2 or their
92+
* negations are maximum 128 bits long (see secp256k1_ge_mul_lambda). It is
93+
* required that r1, r2, and k all point to different objects. */
94+
static void secp256k1_scalar_split_lambda(secp256k1_scalar * SECP256K1_RESTRICT r1, secp256k1_scalar * SECP256K1_RESTRICT r2, const secp256k1_scalar * SECP256K1_RESTRICT k);
9495

9596
/** Multiply a and b (without taking the modulus!), divide by 2**shift, and round to the nearest integer. Shift must be at least 256. */
9697
static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r, const secp256k1_scalar *a, const secp256k1_scalar *b, unsigned int shift);

src/scalar_impl.h

+6-2
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,10 @@ static int secp256k1_scalar_set_b32_seckey(secp256k1_scalar *r, const unsigned c
5252
* nontrivial to get full test coverage for the exhaustive tests. We therefore
5353
* (arbitrarily) set r2 = k + 5 (mod n) and r1 = k - r2 * lambda (mod n).
5454
*/
55-
static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar *r2, const secp256k1_scalar *k) {
55+
static void secp256k1_scalar_split_lambda(secp256k1_scalar * SECP256K1_RESTRICT r1, secp256k1_scalar * SECP256K1_RESTRICT r2, const secp256k1_scalar * SECP256K1_RESTRICT k) {
56+
VERIFY_CHECK(r1 != k);
57+
VERIFY_CHECK(r2 != k);
58+
VERIFY_CHECK(r1 != r2);
5659
*r2 = (*k + 5) % EXHAUSTIVE_TEST_ORDER;
5760
*r1 = (*k + (EXHAUSTIVE_TEST_ORDER - *r2) * EXHAUSTIVE_TEST_LAMBDA) % EXHAUSTIVE_TEST_ORDER;
5861
}
@@ -119,7 +122,7 @@ static void secp256k1_scalar_split_lambda_verify(const secp256k1_scalar *r1, con
119122
*
120123
* See proof below.
121124
*/
122-
static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar *r2, const secp256k1_scalar *k) {
125+
static void secp256k1_scalar_split_lambda(secp256k1_scalar * SECP256K1_RESTRICT r1, secp256k1_scalar * SECP256K1_RESTRICT r2, const secp256k1_scalar * SECP256K1_RESTRICT k) {
123126
secp256k1_scalar c1, c2;
124127
static const secp256k1_scalar minus_b1 = SECP256K1_SCALAR_CONST(
125128
0x00000000UL, 0x00000000UL, 0x00000000UL, 0x00000000UL,
@@ -139,6 +142,7 @@ static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar
139142
);
140143
VERIFY_CHECK(r1 != k);
141144
VERIFY_CHECK(r2 != k);
145+
VERIFY_CHECK(r1 != r2);
142146
/* these _var calls are constant time since the shift amount is constant */
143147
secp256k1_scalar_mul_shift_var(&c1, k, &g1, 384);
144148
secp256k1_scalar_mul_shift_var(&c2, k, &g2, 384);

0 commit comments

Comments
 (0)