From dc5514144fb9d412aa3845432b053ee06a27da37 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Mon, 7 Aug 2023 02:09:58 +0200 Subject: [PATCH 1/2] tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize) `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 09ca4f32) for random non-square field elements and was later refactored into the non-zero helper in PR #25 (commit 6d6102fe). The copy-over to the exhaustive tests happened recently in PR #1118 (commit 0f864207). This case seems to be practically irrelevant and I'd argue for keeping things simple and removing it; 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. --- src/tests.c | 11 ++--------- src/tests_exhaustive.c | 11 ++--------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/tests.c b/src/tests.c index 5b7c914f8c..b4792ebe9d 100644 --- a/src/tests.c +++ b/src/tests.c @@ -2967,16 +2967,9 @@ static void random_fe(secp256k1_fe *x) { } static void random_fe_non_zero(secp256k1_fe *nz) { - int tries = 10; - while (--tries >= 0) { + do { random_fe(nz); - secp256k1_fe_normalize(nz); - if (!secp256k1_fe_is_zero(nz)) { - break; - } - } - /* Infinitesimal probability of spurious failure here */ - CHECK(tries >= 0); + } while (secp256k1_fe_is_zero(nz)); } static void random_fe_non_square(secp256k1_fe *ns) { diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c index 3af8ec1ee5..6cab1a6943 100644 --- a/src/tests_exhaustive.c +++ b/src/tests_exhaustive.c @@ -70,16 +70,9 @@ static void random_fe(secp256k1_fe *x) { } static void random_fe_non_zero(secp256k1_fe *nz) { - int tries = 10; - while (--tries >= 0) { + do { random_fe(nz); - secp256k1_fe_normalize(nz); - if (!secp256k1_fe_is_zero(nz)) { - break; - } - } - /* Infinitesimal probability of spurious failure here */ - CHECK(tries >= 0); + } while (secp256k1_fe_is_zero(nz)); } /** END stolen from tests.c */ From c45b7c4fbbf41b011f138c465a58322a36664fd3 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 17 Aug 2023 19:25:56 +0200 Subject: [PATCH 2/2] refactor: introduce testutil.h (deduplicate `random_fe_`, `ge_equals_` helpers) --- Makefile.am | 1 + src/tests.c | 43 +-------------------------------- src/tests_exhaustive.c | 45 +--------------------------------- src/testutil.h | 55 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 86 deletions(-) create mode 100644 src/testutil.h diff --git a/Makefile.am b/Makefile.am index 32bc729a41..9d0dc3dffc 100644 --- a/Makefile.am +++ b/Makefile.am @@ -46,6 +46,7 @@ noinst_HEADERS += src/precomputed_ecmult.h noinst_HEADERS += src/precomputed_ecmult_gen.h noinst_HEADERS += src/assumptions.h noinst_HEADERS += src/checkmem.h +noinst_HEADERS += src/testutil.h noinst_HEADERS += src/util.h noinst_HEADERS += src/int128.h noinst_HEADERS += src/int128_impl.h diff --git a/src/tests.c b/src/tests.c index b4792ebe9d..fe1f725308 100644 --- a/src/tests.c +++ b/src/tests.c @@ -23,6 +23,7 @@ #include "../include/secp256k1_preallocated.h" #include "testrand_impl.h" #include "checkmem.h" +#include "testutil.h" #include "util.h" #include "../contrib/lax_der_parsing.c" @@ -2956,22 +2957,6 @@ static void run_scalar_tests(void) { /***** FIELD TESTS *****/ -static void random_fe(secp256k1_fe *x) { - unsigned char bin[32]; - do { - secp256k1_testrand256(bin); - if (secp256k1_fe_set_b32_limit(x, bin)) { - return; - } - } while(1); -} - -static void random_fe_non_zero(secp256k1_fe *nz) { - do { - random_fe(nz); - } while (secp256k1_fe_is_zero(nz)); -} - static void random_fe_non_square(secp256k1_fe *ns) { secp256k1_fe r; random_fe_non_zero(ns); @@ -3691,15 +3676,6 @@ static void run_inverse_tests(void) /***** GROUP TESTS *****/ -static void ge_equals_ge(const secp256k1_ge *a, const secp256k1_ge *b) { - CHECK(a->infinity == b->infinity); - if (a->infinity) { - return; - } - CHECK(secp256k1_fe_equal(&a->x, &b->x)); - CHECK(secp256k1_fe_equal(&a->y, &b->y)); -} - /* This compares jacobian points including their Z, not just their geometric meaning. */ static int gej_xyz_equals_gej(const secp256k1_gej *a, const secp256k1_gej *b) { secp256k1_gej a2; @@ -3722,23 +3698,6 @@ static int gej_xyz_equals_gej(const secp256k1_gej *a, const secp256k1_gej *b) { return ret; } -static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) { - secp256k1_fe z2s; - secp256k1_fe u1, u2, s1, s2; - CHECK(a->infinity == b->infinity); - if (a->infinity) { - return; - } - /* Check a.x * b.z^2 == b.x && a.y * b.z^3 == b.y, to avoid inverses. */ - secp256k1_fe_sqr(&z2s, &b->z); - secp256k1_fe_mul(&u1, &a->x, &z2s); - u2 = b->x; - secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z); - s2 = b->y; - CHECK(secp256k1_fe_equal(&u1, &u2)); - CHECK(secp256k1_fe_equal(&s1, &s2)); -} - static void test_ge(void) { int i, i1; int runs = 6; diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c index 6cab1a6943..491248d612 100644 --- a/src/tests_exhaustive.c +++ b/src/tests_exhaustive.c @@ -28,54 +28,11 @@ #include "testrand_impl.h" #include "ecmult_compute_table_impl.h" #include "ecmult_gen_compute_table_impl.h" +#include "testutil.h" #include "util.h" static int count = 2; -/** stolen from tests.c */ -static void ge_equals_ge(const secp256k1_ge *a, const secp256k1_ge *b) { - CHECK(a->infinity == b->infinity); - if (a->infinity) { - return; - } - CHECK(secp256k1_fe_equal(&a->x, &b->x)); - CHECK(secp256k1_fe_equal(&a->y, &b->y)); -} - -static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) { - secp256k1_fe z2s; - secp256k1_fe u1, u2, s1, s2; - CHECK(a->infinity == b->infinity); - if (a->infinity) { - return; - } - /* Check a.x * b.z^2 == b.x && a.y * b.z^3 == b.y, to avoid inverses. */ - secp256k1_fe_sqr(&z2s, &b->z); - secp256k1_fe_mul(&u1, &a->x, &z2s); - u2 = b->x; - secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z); - s2 = b->y; - CHECK(secp256k1_fe_equal(&u1, &u2)); - CHECK(secp256k1_fe_equal(&s1, &s2)); -} - -static void random_fe(secp256k1_fe *x) { - unsigned char bin[32]; - do { - secp256k1_testrand256(bin); - if (secp256k1_fe_set_b32_limit(x, bin)) { - return; - } - } while(1); -} - -static void random_fe_non_zero(secp256k1_fe *nz) { - do { - random_fe(nz); - } while (secp256k1_fe_is_zero(nz)); -} -/** END stolen from tests.c */ - static uint32_t num_cores = 1; static uint32_t this_core = 0; diff --git a/src/testutil.h b/src/testutil.h new file mode 100644 index 0000000000..7333541dc5 --- /dev/null +++ b/src/testutil.h @@ -0,0 +1,55 @@ +/*********************************************************************** + * Distributed under the MIT software license, see the accompanying * + * file COPYING or https://www.opensource.org/licenses/mit-license.php.* + ***********************************************************************/ + +#ifndef SECP256K1_TESTUTIL_H +#define SECP256K1_TESTUTIL_H + +#include "field.h" +#include "testrand.h" +#include "util.h" + +static void random_fe(secp256k1_fe *x) { + unsigned char bin[32]; + do { + secp256k1_testrand256(bin); + if (secp256k1_fe_set_b32_limit(x, bin)) { + return; + } + } while(1); +} + +static void random_fe_non_zero(secp256k1_fe *nz) { + do { + random_fe(nz); + } while (secp256k1_fe_is_zero(nz)); +} + +static void ge_equals_ge(const secp256k1_ge *a, const secp256k1_ge *b) { + CHECK(a->infinity == b->infinity); + if (a->infinity) { + return; + } + CHECK(secp256k1_fe_equal(&a->x, &b->x)); + CHECK(secp256k1_fe_equal(&a->y, &b->y)); +} + +static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) { + secp256k1_fe z2s; + secp256k1_fe u1, u2, s1, s2; + CHECK(a->infinity == b->infinity); + if (a->infinity) { + return; + } + /* Check a.x * b.z^2 == b.x && a.y * b.z^3 == b.y, to avoid inverses. */ + secp256k1_fe_sqr(&z2s, &b->z); + secp256k1_fe_mul(&u1, &a->x, &z2s); + u2 = b->x; + secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z); + s2 = b->y; + CHECK(secp256k1_fe_equal(&u1, &u2)); + CHECK(secp256k1_fe_equal(&s1, &s2)); +} + +#endif /* SECP256K1_TESTUTIL_H */