Skip to content

Commit ee7aaf2

Browse files
real-or-randomreal-or-random
committed
Merge bitcoin-core/secp256k1#1395: tests: simplify random_fe_non_zero (remove loop limit and unneeded normalize)
c45b7c4 refactor: introduce testutil.h (deduplicate `random_fe_`, `ge_equals_` helpers) (Sebastian Falbesoner) dc55141 tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize) (Sebastian Falbesoner) Pull request description: `random_fe_non_zero` contains a loop iteration limit that ensures that we abort if `random_fe` ever yielded zero more than ten times in a row. This construct was first introduced in PR #19 (commit 09ca4f3) for random non-square field elements and was later refactored into the non-zero helper in PR #25 (commit 6d6102f). The copy-over to the exhaustive tests happened recently in PR #1118 (commit 0f86420). This case seems to be practically irrelevant and I'd argue for keeping things simple and removing it (which was already suggested in bitcoin-core/secp256k1#1118 (comment)); if there's really a worry that the test's random generator is heavily biased towards certain values or value ranges then there should consequently be checks at other places too (e.g. directly in `random_fe` for 256-bit values that repeatedly overflow, i.e. >= p). Also, the _fe_normalize call is not needed and can be removed, as the result of `random_fe` is already normalized. ACKs for top commit: real-or-random: utACK c45b7c4 siv2r: ACK `c45b7c4` (reviewed the changes and tests for both the commits passed locally). Tree-SHA512: 4ffa66dd0b8392d7d0083a71e7b0682ad18f9261fd4ce8548c3059b497d3462db97e16114fded9787661ca447a877a27f5b996bd7d47e6f91c4454079d28a8ac
2 parents ba9cb6f + c45b7c4 commit ee7aaf2

File tree

4 files changed

+58
-100
lines changed

4 files changed

+58
-100
lines changed

‎Makefile.am

+1
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ noinst_HEADERS += src/precomputed_ecmult.h
4646
noinst_HEADERS += src/precomputed_ecmult_gen.h
4747
noinst_HEADERS += src/assumptions.h
4848
noinst_HEADERS += src/checkmem.h
49+
noinst_HEADERS += src/testutil.h
4950
noinst_HEADERS += src/util.h
5051
noinst_HEADERS += src/int128.h
5152
noinst_HEADERS += src/int128_impl.h

‎src/tests.c

+1-49
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "../include/secp256k1_preallocated.h"
2424
#include "testrand_impl.h"
2525
#include "checkmem.h"
26+
#include "testutil.h"
2627
#include "util.h"
2728

2829
#include "../contrib/lax_der_parsing.c"
@@ -2921,29 +2922,6 @@ static void run_scalar_tests(void) {
29212922

29222923
/***** FIELD TESTS *****/
29232924

2924-
static void random_fe(secp256k1_fe *x) {
2925-
unsigned char bin[32];
2926-
do {
2927-
secp256k1_testrand256(bin);
2928-
if (secp256k1_fe_set_b32_limit(x, bin)) {
2929-
return;
2930-
}
2931-
} while(1);
2932-
}
2933-
2934-
static void random_fe_non_zero(secp256k1_fe *nz) {
2935-
int tries = 10;
2936-
while (--tries >= 0) {
2937-
random_fe(nz);
2938-
secp256k1_fe_normalize(nz);
2939-
if (!secp256k1_fe_is_zero(nz)) {
2940-
break;
2941-
}
2942-
}
2943-
/* Infinitesimal probability of spurious failure here */
2944-
CHECK(tries >= 0);
2945-
}
2946-
29472925
static void random_fe_non_square(secp256k1_fe *ns) {
29482926
secp256k1_fe r;
29492927
random_fe_non_zero(ns);
@@ -3663,15 +3641,6 @@ static void run_inverse_tests(void)
36633641

36643642
/***** GROUP TESTS *****/
36653643

3666-
static void ge_equals_ge(const secp256k1_ge *a, const secp256k1_ge *b) {
3667-
CHECK(a->infinity == b->infinity);
3668-
if (a->infinity) {
3669-
return;
3670-
}
3671-
CHECK(secp256k1_fe_equal(&a->x, &b->x));
3672-
CHECK(secp256k1_fe_equal(&a->y, &b->y));
3673-
}
3674-
36753644
/* This compares jacobian points including their Z, not just their geometric meaning. */
36763645
static int gej_xyz_equals_gej(const secp256k1_gej *a, const secp256k1_gej *b) {
36773646
secp256k1_gej a2;
@@ -3694,23 +3663,6 @@ static int gej_xyz_equals_gej(const secp256k1_gej *a, const secp256k1_gej *b) {
36943663
return ret;
36953664
}
36963665

3697-
static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) {
3698-
secp256k1_fe z2s;
3699-
secp256k1_fe u1, u2, s1, s2;
3700-
CHECK(a->infinity == b->infinity);
3701-
if (a->infinity) {
3702-
return;
3703-
}
3704-
/* Check a.x * b.z^2 == b.x && a.y * b.z^3 == b.y, to avoid inverses. */
3705-
secp256k1_fe_sqr(&z2s, &b->z);
3706-
secp256k1_fe_mul(&u1, &a->x, &z2s);
3707-
u2 = b->x;
3708-
secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z);
3709-
s2 = b->y;
3710-
CHECK(secp256k1_fe_equal(&u1, &u2));
3711-
CHECK(secp256k1_fe_equal(&s1, &s2));
3712-
}
3713-
37143666
static void test_ge(void) {
37153667
int i, i1;
37163668
int runs = 6;

‎src/tests_exhaustive.c

+1-51
Original file line numberDiff line numberDiff line change
@@ -28,61 +28,11 @@
2828
#include "testrand_impl.h"
2929
#include "ecmult_compute_table_impl.h"
3030
#include "ecmult_gen_compute_table_impl.h"
31+
#include "testutil.h"
3132
#include "util.h"
3233

3334
static int count = 2;
3435

35-
/** stolen from tests.c */
36-
static void ge_equals_ge(const secp256k1_ge *a, const secp256k1_ge *b) {
37-
CHECK(a->infinity == b->infinity);
38-
if (a->infinity) {
39-
return;
40-
}
41-
CHECK(secp256k1_fe_equal(&a->x, &b->x));
42-
CHECK(secp256k1_fe_equal(&a->y, &b->y));
43-
}
44-
45-
static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) {
46-
secp256k1_fe z2s;
47-
secp256k1_fe u1, u2, s1, s2;
48-
CHECK(a->infinity == b->infinity);
49-
if (a->infinity) {
50-
return;
51-
}
52-
/* Check a.x * b.z^2 == b.x && a.y * b.z^3 == b.y, to avoid inverses. */
53-
secp256k1_fe_sqr(&z2s, &b->z);
54-
secp256k1_fe_mul(&u1, &a->x, &z2s);
55-
u2 = b->x;
56-
secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z);
57-
s2 = b->y;
58-
CHECK(secp256k1_fe_equal(&u1, &u2));
59-
CHECK(secp256k1_fe_equal(&s1, &s2));
60-
}
61-
62-
static void random_fe(secp256k1_fe *x) {
63-
unsigned char bin[32];
64-
do {
65-
secp256k1_testrand256(bin);
66-
if (secp256k1_fe_set_b32_limit(x, bin)) {
67-
return;
68-
}
69-
} while(1);
70-
}
71-
72-
static void random_fe_non_zero(secp256k1_fe *nz) {
73-
int tries = 10;
74-
while (--tries >= 0) {
75-
random_fe(nz);
76-
secp256k1_fe_normalize(nz);
77-
if (!secp256k1_fe_is_zero(nz)) {
78-
break;
79-
}
80-
}
81-
/* Infinitesimal probability of spurious failure here */
82-
CHECK(tries >= 0);
83-
}
84-
/** END stolen from tests.c */
85-
8636
static uint32_t num_cores = 1;
8737
static uint32_t this_core = 0;
8838

‎src/testutil.h

+55
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/***********************************************************************
2+
* Distributed under the MIT software license, see the accompanying *
3+
* file COPYING or https://www.opensource.org/licenses/mit-license.php.*
4+
***********************************************************************/
5+
6+
#ifndef SECP256K1_TESTUTIL_H
7+
#define SECP256K1_TESTUTIL_H
8+
9+
#include "field.h"
10+
#include "testrand.h"
11+
#include "util.h"
12+
13+
static void random_fe(secp256k1_fe *x) {
14+
unsigned char bin[32];
15+
do {
16+
secp256k1_testrand256(bin);
17+
if (secp256k1_fe_set_b32_limit(x, bin)) {
18+
return;
19+
}
20+
} while(1);
21+
}
22+
23+
static void random_fe_non_zero(secp256k1_fe *nz) {
24+
do {
25+
random_fe(nz);
26+
} while (secp256k1_fe_is_zero(nz));
27+
}
28+
29+
static void ge_equals_ge(const secp256k1_ge *a, const secp256k1_ge *b) {
30+
CHECK(a->infinity == b->infinity);
31+
if (a->infinity) {
32+
return;
33+
}
34+
CHECK(secp256k1_fe_equal(&a->x, &b->x));
35+
CHECK(secp256k1_fe_equal(&a->y, &b->y));
36+
}
37+
38+
static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) {
39+
secp256k1_fe z2s;
40+
secp256k1_fe u1, u2, s1, s2;
41+
CHECK(a->infinity == b->infinity);
42+
if (a->infinity) {
43+
return;
44+
}
45+
/* Check a.x * b.z^2 == b.x && a.y * b.z^3 == b.y, to avoid inverses. */
46+
secp256k1_fe_sqr(&z2s, &b->z);
47+
secp256k1_fe_mul(&u1, &a->x, &z2s);
48+
u2 = b->x;
49+
secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z);
50+
s2 = b->y;
51+
CHECK(secp256k1_fe_equal(&u1, &u2));
52+
CHECK(secp256k1_fe_equal(&s1, &s2));
53+
}
54+
55+
#endif /* SECP256K1_TESTUTIL_H */

0 commit comments

Comments
 (0)