Removes _fe_equal_var, and unwanted _fe_normalize_weak calls (in tests)

[siv2r]

Fixes #946 and #1061

Changes:

• removes unwanted fe_normalize_weak calls to the second argument of fe_equal
• removes fe_equal_var

[real-or-random]

Concept ACK

I think then we should add (#ifdef VERIFY) magnitude checks to secp256k1_fe_equal_var and secp256k1_fe_equal.

If you're up to it, I think the other field function could need an audit for similar checks.

[siv2r]

In fe_sqrt:

secp256k1/src/field_impl.h

Lines 132 to 135 in a1102b1

	/* Check that a square root was actually calculated */
	secp256k1_fe_sqr(&t1, r);
	return secp256k1_fe_equal(&t1, a);

Here, the secp256k1_fe_sqr function ensures that t1->magnitude = 1 but there is no guarantee for the magnitude of a to be one since it is passed into the function. Hence, some fe_sqrt tests are failing after adding magnitude checks inside fe_equal.

We could do secp256k1_fe_normalize_weak(a) but this changes the value of a and we don't want that right? Alternatively, we could call fe_equal_var instead of fe_equal. Is it necessary for secp256k1_fe_sqrt to be of constant time?

[peterdettman]

@siv2r Regarding _fe_sqrt, there is an implicit magnitude restriction on a, since it is used as the input to _fe_mul and _fe_sqr at the top of the method. That magnitude restriction is <= 8. 8 is also a kind of global implicit limit for inputs where not otherwise stated (but we should definitely be working towards fully documenting and validating all such assumptions).

I think it might be better to just say that both inputs to the equals methods can be magnitude <= 8 too (indeed tied to the mul/sqr limit whatever it is or changes to). There's no real obstacle to the implementation(s) (just change the third argument to _fe_negate to 8) and no performance hit AFAICT.

[peterdettman]

Is it necessary for secp256k1_fe_sqrt to be of constant time?

It's only used in a single var-time caller, so technically it could be changed to sqrt_var, but preferably not.

[real-or-random]

Apparently secp256k1_fe_equal and secp256k1_fe_equal_var are identical up to the final call to normalizes_to_zero or normalizes_to_zero_var and so they both only require that a has magnitude 1. So we should just document that secp256k1_fe_equal only requires the magnitude of a to be 1.

Edited because I first thought secp256k1_fe_equal and secp256k1_fe_equal_var are exactly identical.

[siv2r]

It's only used in a single var-time caller

Oh, Okay. Then, creating a new function for secp256k1_fe_sqrt_var (while renaming the old one to _fe_sqrt_const) might be overkill.

we should just document that secp256k1_fe_equal only requires the magnitude of a to be 1.

Yes, I think this might be a better approach since it keeps the functionality of both fe_equal_var and fe_equal similar.

After this, I felt we needed to remove the _fe_normalize_weak calls before _fe_equal, but surprisingly _fe_equal is used rarely in the code. The only other place where it is used is:

secp256k1/src/modules/extrakeys/tests_impl.h

Lines 75 to 77 in a1102b1

	CHECK(secp256k1_fe_equal(&pk1.x, &pk2.x) == 1);
	secp256k1_fe_negate(&y, &pk2.y, 1);
	CHECK(secp256k1_fe_equal(&pk1.y, &y) == 1);

Even here, the magnitude of y is two, so modifying the _fe_sqrt function (which I suggested) is a bad idea.

[real-or-random]

secp256k1/src/modules/extrakeys/tests_impl.h

Lines 75 to 77 in a1102b1

	CHECK(secp256k1_fe_equal(&pk1.x, &pk2.x) == 1);
	secp256k1_fe_negate(&y, &pk2.y, 1);
	CHECK(secp256k1_fe_equal(&pk1.y, &y) == 1);

Hehe ok, and here it's basically an oversight. This is just test code, so fe_equal_var could be used instead. I mean, not that it matters.

[peterdettman]

fe_equal_var is really a bit dubious. It only has a fast path when the inputs are not equal, and the only callers are for public key parsing and (ECDSA) verification, where that would mean an error condition and is probably uncommon. If the fast path is not hit, it actually costs a couple of cycles more. (Not to mention that the performance difference is tiny relative to those operations anyway).

[peterdettman]

While I'm on the subject, the other callers to _fe_normalizes_to_zero_var expect a false result and so will almost always hit the fast path. Depending on the level of inlining, _fe_equal_var could be "spoiling" the branch prediction for that fast path check for the other callers.

[siv2r]

addressed #1062 (comment) and #1062 (review) (detailed explanation in 08186e8).

[siv2r]

I also benchmarked the tests.c file (using time ./tests) since a lot of secp256k1_fe_verify calls were added in this PR. These are the results (on 64-bit, i7-8750H) for three iterations:

branch	iter1	iter2	iter3
master	2min 26.21sec	2min 26.44sec	2min 26.68sec
this PR	2min 26.99sec	2min 26.91sec	2min 26.83sec

[real-or-random]

@peterdettman Good points. Do you think we should remove fe_equal_var entirely and maybe document in fe_equal that this is intentional?

[peterdettman]

@peterdettman Good points. Do you think we should remove fe_equal_var entirely and maybe document in fe_equal that this is intentional?

Yeah, pretty much. Until there's a performance-sensitive caller for whom a != b is the expected result, I don't see any value in _fe_equal_var. If such a caller does turn up, then there should be a fast path for inequality that doesn't even need the subtraction, so we'd rewrite it anyway.

[siv2r]

addressed #1061

[real-or-random]

@siv2r Do you plan to also remove the fe_equal_var in a further commit? Or should we do this in a separate PR?

(If you want to add it here, I think it's really ok to add it on top of the existing commits, no need to rewrite the existing commits).

[siv2r]

Okay, I will remove fe_equal_var in a new commit (this PR).

[siv2r]

• Removed fe_equal_var and documented the reason in fe_equal
• Replaced the fe_equal_var calls with fe_equal

[sipa]

I missed the "field: add magnitude check and _fe_verify check for internal field APIs" commit being added here before writing #1066 (which duplicates some of its efforts, but also goes a lot further in a number of ways). Feel free to keep it in (and I'll rebase), but if you think #1066 covers everything you're doing here, perhaps it's easier to leave that for that PR.

[siv2r]

No worries, I will rebase this PR.

[real-or-random]

No worries, I will rebase this PR.

Ok, merging #1066 took a while.... This PR here needs rebase now. :)

[siv2r]

Rebased.

#1066 took care of all the magnitude and fe_verify checks so, this PR currently does:

• removes unwanted fe_normalize_weak calls to the second argument of fe_equal
• removes fe_equal_var

[siv2r]

Not sure why normalize_var was used on bn. I assumed it intended to set bn's magnitude to 1 for the fe_equal call. So, I removed it. Please let me know if that's not the case, and I'll revert this.

All test cases passed with this change.

[real-or-random]

Nice. :)

#1066 took care of all the magnitude and fe_verify checks so, this PR currently does:

Can you update the PR title and the initial comment, so that they reflect the current status?

[real-or-random]

Needs rebase again, sorry, but should be trivial. :)

Thanks for updating the PR title. Can you also update the initial post/PR description (e.g., replace the contents with #1062 (comment) )? It's just cosmetic, but the text of will be added to the merge commit, so it makes sense to keep this updated.

[siv2r]

Rebased and updated the description. I forgot about the merge commit including the description too. Thanks!

[real-or-random]

utACK 54058d1

[jonasnick]

Until there's a performance-sensitive caller for whom a != b is the expected result, I don't see any value in _fe_equal_var.

I don't think we know what the expected result of the caller is. The difference seems to be negligible though.

ACK 54058d1