Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make fe_cmov take max of magnitudes #1317

Merged
merged 1 commit into from
May 19, 2023
Merged

Make fe_cmov take max of magnitudes #1317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/field.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,9 @@ static void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_f
*
* On input, both r and a must be valid field elements. Flag must be 0 or 1.
* Performs {r = flag ? a : r}.
* On output, r's magnitude and normalized will equal a's in case of flag=1, unchanged otherwise.
*
* On output, r's magnitude will be the maximum of both input magnitudes.
* It will be normalized if and only if both inputs were normalized.
*/
static void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag);

Expand Down
6 changes: 2 additions & 4 deletions src/field_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,8 @@ SECP256K1_INLINE static void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
secp256k1_fe_verify(a);
secp256k1_fe_verify(r);
secp256k1_fe_impl_cmov(r, a, flag);
if (flag) {
r->magnitude = a->magnitude;
r->normalized = a->normalized;
}
if (a->magnitude > r->magnitude) r->magnitude = a->magnitude;
if (!a->normalized) r->normalized = 0;
secp256k1_fe_verify(r);
}

Expand Down
26 changes: 14 additions & 12 deletions src/tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3101,10 +3101,6 @@ static void run_field_be32_overflow(void) {
/* Returns true if two field elements have the same representation. */
static int fe_identical(const secp256k1_fe *a, const secp256k1_fe *b) {
int ret = 1;
#ifdef VERIFY
ret &= (a->magnitude == b->magnitude);
ret &= (a->normalized == b->normalized);
#endif
/* Compare the struct member that holds the limbs. */
ret &= (secp256k1_memcmp_var(a->n, b->n, sizeof(a->n)) == 0);
return ret;
Expand Down Expand Up @@ -3192,16 +3188,22 @@ static void run_field_misc(void) {
q = x;
secp256k1_fe_cmov(&x, &z, 0);
#ifdef VERIFY
CHECK(x.normalized && x.magnitude == 1);
CHECK(!x.normalized);
CHECK((x.magnitude == q.magnitude) || (x.magnitude == z.magnitude));
CHECK((x.magnitude >= q.magnitude) && (x.magnitude >= z.magnitude));
#endif
x = q;
secp256k1_fe_cmov(&x, &x, 1);
Copy link
Contributor

@stratospher stratospher May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

31b4bbe: do we need this line? don't think it does anything. (fe_cmov part)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the point: the test verifies that it does nothing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, i see.

CHECK(!fe_identical(&x, &z));
CHECK(fe_identical(&x, &q));
secp256k1_fe_cmov(&q, &z, 1);
#ifdef VERIFY
CHECK(!q.normalized && q.magnitude == z.magnitude);
CHECK(!q.normalized);
CHECK((q.magnitude == x.magnitude) || (q.magnitude == z.magnitude));
CHECK((q.magnitude >= x.magnitude) && (q.magnitude >= z.magnitude));
#endif
CHECK(fe_identical(&q, &z));
q = z;
secp256k1_fe_normalize_var(&x);
secp256k1_fe_normalize_var(&z);
CHECK(!secp256k1_fe_equal_var(&x, &z));
Expand All @@ -3215,7 +3217,7 @@ static void run_field_misc(void) {
secp256k1_fe_normalize_var(&q);
secp256k1_fe_cmov(&q, &z, (j&1));
#ifdef VERIFY
CHECK((q.normalized != (j&1)) && q.magnitude == ((j&1) ? z.magnitude : 1));
CHECK(!q.normalized && q.magnitude == z.magnitude);
#endif
}
secp256k1_fe_normalize_var(&z);
Expand Down Expand Up @@ -7558,23 +7560,23 @@ static void fe_cmov_test(void) {
secp256k1_fe a = zero;

secp256k1_fe_cmov(&r, &a, 0);
CHECK(secp256k1_memcmp_var(&r, &max, sizeof(r)) == 0);
CHECK(fe_identical(&r, &max));

r = zero; a = max;
secp256k1_fe_cmov(&r, &a, 1);
CHECK(secp256k1_memcmp_var(&r, &max, sizeof(r)) == 0);
CHECK(fe_identical(&r, &max));

a = zero;
secp256k1_fe_cmov(&r, &a, 1);
CHECK(secp256k1_memcmp_var(&r, &zero, sizeof(r)) == 0);
CHECK(fe_identical(&r, &zero));

a = one;
secp256k1_fe_cmov(&r, &a, 1);
CHECK(secp256k1_memcmp_var(&r, &one, sizeof(r)) == 0);
CHECK(fe_identical(&r, &one));

r = one; a = zero;
secp256k1_fe_cmov(&r, &a, 0);
CHECK(secp256k1_memcmp_var(&r, &one, sizeof(r)) == 0);
CHECK(fe_identical(&r, &one));
}

static void fe_storage_cmov_test(void) {
Expand Down