From 59447da38d6fa394ab7b10f32aea55930da732be Mon Sep 17 00:00:00 2001 From: Peter Dettman Date: Sun, 22 Jun 2014 15:03:29 +0700 Subject: [PATCH 1/2] Test demonstrating discrepancy in sqr output --- src/field.h | 4 ++++ src/field_5x52_impl.h | 19 +++++++++++++++++++ src/tests.c | 24 ++++++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/src/field.h b/src/field.h index 067115d0a71ad..4e5488cf4c203 100644 --- a/src/field.h +++ b/src/field.h @@ -42,6 +42,10 @@ void static secp256k1_fe_start(void); /** Unload field element precomputation data. */ void static secp256k1_fe_stop(void); +#ifdef VERIFY +int static secp256k1_fe_verify(const secp256k1_fe_t * a); +#endif + /** Normalize a field element. */ void static secp256k1_fe_normalize(secp256k1_fe_t *r); diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index cc9c5fe1d97ba..d84819603882d 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -33,6 +33,25 @@ void static secp256k1_fe_inner_start(void) {} void static secp256k1_fe_inner_stop(void) {} +#ifdef VERIFY +int static secp256k1_fe_verify(const secp256k1_fe_t * a) { + const uint64_t *d = a->n; + int m = a->magnitude, r = 1; + r &= (d[0] <= 0xFFFFFFFFFFFFFULL * m); + r &= (d[1] <= 0xFFFFFFFFFFFFFULL * m); + r &= (d[2] <= 0xFFFFFFFFFFFFFULL * m); + r &= (d[3] <= 0xFFFFFFFFFFFFFULL * m); + r &= (d[4] <= 0x0FFFFFFFFFFFFULL * m); + if (a->normalized) { + r &= (m == 1); + if (r && (d[4] == 0x0FFFFFFFFFFFFULL) && ((d[3] & d[2] & d[1]) == 0xFFFFFFFFFFFFFULL)) { + r &= (d[0] < 0xFFFFEFFFFFC2FULL); + } + } + return r; +} +#endif + void static secp256k1_fe_normalize(secp256k1_fe_t *r) { uint64_t t0 = r->n[0], t1 = r->n[1], t2 = r->n[2], t3 = r->n[3], t4 = r->n[4]; diff --git a/src/tests.c b/src/tests.c index fca2bed0a4b07..90c1dc8255a97 100644 --- a/src/tests.c +++ b/src/tests.c @@ -312,6 +312,29 @@ void run_field_inv_all_var() { } } +void run_sqr() { + secp256k1_fe_t x, s; + +#if defined(USE_FIELD_5X52) + // Known issue with reduction part of sqr. For simplicity, we trigger the problem here + // with "negative" powers of 2, but the problem exists for large ranges of values. + { + secp256k1_fe_set_int(&x, 1); + secp256k1_fe_negate(&x, &x, 1); + + for (int i=1; i<=512; ++i) { + secp256k1_fe_mul_int(&x, 2); + secp256k1_fe_normalize(&x); + secp256k1_fe_sqr(&s, &x); + if (!secp256k1_fe_verify(&s)) { + printf("%4i: %016llx %016llx %016llx %016llx %016llx\n", + i, s.n[4], s.n[3], s.n[2], s.n[1], s.n[0]); + } + } + } +#endif +} + void test_sqrt(const secp256k1_fe_t *a, const secp256k1_fe_t *k) { secp256k1_fe_t r1, r2; int v = secp256k1_fe_sqrt(&r1, a); @@ -609,6 +632,7 @@ int main(int argc, char **argv) { run_field_inv_var(); run_field_inv_all(); run_field_inv_all_var(); + run_sqr(); run_sqrt(); // ecmult tests From 21f81a846957408d4a2fe30cff32370567887a93 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 15 Jul 2014 19:09:35 +0200 Subject: [PATCH 2/2] Correct secp256k1_fe_verify and use it everywhere --- src/field.h | 4 ---- src/field_5x52_impl.h | 54 +++++++++++++++++++++++++++++++------------ src/tests.c | 8 ------- 3 files changed, 39 insertions(+), 27 deletions(-) diff --git a/src/field.h b/src/field.h index 4e5488cf4c203..067115d0a71ad 100644 --- a/src/field.h +++ b/src/field.h @@ -42,10 +42,6 @@ void static secp256k1_fe_start(void); /** Unload field element precomputation data. */ void static secp256k1_fe_stop(void); -#ifdef VERIFY -int static secp256k1_fe_verify(const secp256k1_fe_t * a); -#endif - /** Normalize a field element. */ void static secp256k1_fe_normalize(secp256k1_fe_t *r); diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index d84819603882d..ccebdab078a0f 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -34,9 +34,9 @@ void static secp256k1_fe_inner_start(void) {} void static secp256k1_fe_inner_stop(void) {} #ifdef VERIFY -int static secp256k1_fe_verify(const secp256k1_fe_t * a) { +void static secp256k1_fe_verify(const secp256k1_fe_t *a) { const uint64_t *d = a->n; - int m = a->magnitude, r = 1; + int m = a->normalized ? 1 : 2 * a->magnitude, r = 1; r &= (d[0] <= 0xFFFFFFFFFFFFFULL * m); r &= (d[1] <= 0xFFFFFFFFFFFFFULL * m); r &= (d[2] <= 0xFFFFFFFFFFFFFULL * m); @@ -48,8 +48,10 @@ int static secp256k1_fe_verify(const secp256k1_fe_t * a) { r &= (d[0] < 0xFFFFEFFFFFC2FULL); } } - return r; + assert(r == 1); } +#else +void static secp256k1_fe_verify(const secp256k1_fe_t *a) {} #endif void static secp256k1_fe_normalize(secp256k1_fe_t *r) { @@ -91,6 +93,7 @@ void static secp256k1_fe_normalize(secp256k1_fe_t *r) { #ifdef VERIFY r->magnitude = 1; r->normalized = 1; + secp256k1_fe_verify(r); #endif } @@ -100,6 +103,7 @@ void static inline secp256k1_fe_set_int(secp256k1_fe_t *r, int a) { #ifdef VERIFY r->magnitude = 1; r->normalized = 1; + secp256k1_fe_verify(r); #endif } @@ -107,6 +111,7 @@ void static inline secp256k1_fe_set_int(secp256k1_fe_t *r, int a) { int static inline secp256k1_fe_is_zero(const secp256k1_fe_t *a) { #ifdef VERIFY assert(a->normalized); + secp256k1_fe_verify(a); #endif return (a->n[0] == 0 && a->n[1] == 0 && a->n[2] == 0 && a->n[3] == 0 && a->n[4] == 0); } @@ -114,6 +119,7 @@ int static inline secp256k1_fe_is_zero(const secp256k1_fe_t *a) { int static inline secp256k1_fe_is_odd(const secp256k1_fe_t *a) { #ifdef VERIFY assert(a->normalized); + secp256k1_fe_verify(a); #endif return a->n[0] & 1; } @@ -123,6 +129,8 @@ int static inline secp256k1_fe_equal(const secp256k1_fe_t *a, const secp256k1_fe #ifdef VERIFY assert(a->normalized); assert(b->normalized); + secp256k1_fe_verify(a); + secp256k1_fe_verify(b); #endif return (a->n[0] == b->n[0] && a->n[1] == b->n[1] && a->n[2] == b->n[2] && a->n[3] == b->n[3] && a->n[4] == b->n[4]); } @@ -139,6 +147,7 @@ void static secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { #ifdef VERIFY r->magnitude = 1; r->normalized = 1; + secp256k1_fe_verify(r); #endif } @@ -146,6 +155,7 @@ void static secp256k1_fe_set_b32(secp256k1_fe_t *r, const unsigned char *a) { void static secp256k1_fe_get_b32(unsigned char *r, const secp256k1_fe_t *a) { #ifdef VERIFY assert(a->normalized); + secp256k1_fe_verify(a); #endif for (int i=0; i<32; i++) { int c = 0; @@ -161,57 +171,71 @@ void static secp256k1_fe_get_b32(unsigned char *r, const secp256k1_fe_t *a) { void static inline secp256k1_fe_negate(secp256k1_fe_t *r, const secp256k1_fe_t *a, int m) { #ifdef VERIFY assert(a->magnitude <= m); - r->magnitude = m + 1; - r->normalized = 0; + secp256k1_fe_verify(a); #endif r->n[0] = 0xFFFFEFFFFFC2FULL * (m + 1) - a->n[0]; r->n[1] = 0xFFFFFFFFFFFFFULL * (m + 1) - a->n[1]; r->n[2] = 0xFFFFFFFFFFFFFULL * (m + 1) - a->n[2]; r->n[3] = 0xFFFFFFFFFFFFFULL * (m + 1) - a->n[3]; r->n[4] = 0x0FFFFFFFFFFFFULL * (m + 1) - a->n[4]; -} - -void static inline secp256k1_fe_mul_int(secp256k1_fe_t *r, int a) { #ifdef VERIFY - r->magnitude *= a; + r->magnitude = m + 1; r->normalized = 0; + secp256k1_fe_verify(r); #endif +} + +void static inline secp256k1_fe_mul_int(secp256k1_fe_t *r, int a) { r->n[0] *= a; r->n[1] *= a; r->n[2] *= a; r->n[3] *= a; r->n[4] *= a; -} - -void static inline secp256k1_fe_add(secp256k1_fe_t *r, const secp256k1_fe_t *a) { #ifdef VERIFY - r->magnitude += a->magnitude; + r->magnitude *= a; r->normalized = 0; + secp256k1_fe_verify(r); #endif +} + +void static inline secp256k1_fe_add(secp256k1_fe_t *r, const secp256k1_fe_t *a) { r->n[0] += a->n[0]; r->n[1] += a->n[1]; r->n[2] += a->n[2]; r->n[3] += a->n[3]; r->n[4] += a->n[4]; +#ifdef VERIFY + r->magnitude += a->magnitude; + r->normalized = 0; + secp256k1_fe_verify(r); + secp256k1_fe_verify(a); +#endif } void static secp256k1_fe_mul(secp256k1_fe_t *r, const secp256k1_fe_t *a, const secp256k1_fe_t *b) { #ifdef VERIFY assert(a->magnitude <= 8); assert(b->magnitude <= 8); + secp256k1_fe_verify(a); + secp256k1_fe_verify(b); +#endif + secp256k1_fe_mul_inner(a->n, b->n, r->n); +#ifdef VERIFY r->magnitude = 1; r->normalized = 0; + secp256k1_fe_verify(r); #endif - secp256k1_fe_mul_inner(a->n, b->n, r->n); } void static secp256k1_fe_sqr(secp256k1_fe_t *r, const secp256k1_fe_t *a) { #ifdef VERIFY assert(a->magnitude <= 8); +#endif + secp256k1_fe_sqr_inner(a->n, r->n); +#ifdef VERIFY r->magnitude = 1; r->normalized = 0; #endif - secp256k1_fe_sqr_inner(a->n, r->n); } #endif diff --git a/src/tests.c b/src/tests.c index 90c1dc8255a97..07aaa3c227af1 100644 --- a/src/tests.c +++ b/src/tests.c @@ -315,9 +315,6 @@ void run_field_inv_all_var() { void run_sqr() { secp256k1_fe_t x, s; -#if defined(USE_FIELD_5X52) - // Known issue with reduction part of sqr. For simplicity, we trigger the problem here - // with "negative" powers of 2, but the problem exists for large ranges of values. { secp256k1_fe_set_int(&x, 1); secp256k1_fe_negate(&x, &x, 1); @@ -326,13 +323,8 @@ void run_sqr() { secp256k1_fe_mul_int(&x, 2); secp256k1_fe_normalize(&x); secp256k1_fe_sqr(&s, &x); - if (!secp256k1_fe_verify(&s)) { - printf("%4i: %016llx %016llx %016llx %016llx %016llx\n", - i, s.n[4], s.n[3], s.n[2], s.n[1], s.n[0]); - } } } -#endif } void test_sqrt(const secp256k1_fe_t *a, const secp256k1_fe_t *k) {