Make ec_ arithmetic more consistent and add documentation

[jonasnick]

Fixes #671. Supersedes #668.

This PR unifies handling of invalid secret keys by introducing a new function scalar_set_b32_secret which returns false if the b32 overflows or is 0. By using this in privkey_{negate, tweak_add, tweak_mul} these function will now return 0 if the secret key is invalid which matches the behavior of ecdsa_sign and pubkey_create.

Instead of deciding whether to zeroize the secret key on failure, I only added documentation for now that the value is undefined on failure.

[real-or-random]

Nice! Looks good except nits.

[jonasnick]

@real-or-random I added a commit that rephrases the docs. Let me know if you think that's better.

(Also, I replaced secp256k1_rand256_test with secp256k1_rand256 because the former is apparently not random, may produce invalid seckeys and travis randomly hit that in one of the test configurations).

[jonasnick]

Removed the (attempted) constant time AND in set_b32_seckey as per @sipa's suggestion

[real-or-random]

Is there a reason to believe that this will break downstream code (because it changes the API)?

[dr-orlovsky]

while the changes are negligible, if one is aimed to write a deterministic code (like in the case of rust wrapper), it may be good to cover this case as well.

It is possible to add to the docs that in case tweak<Z_p, the function will return 0?
Also, do you see any value of differentiating between tweak<Z_p and point at infinity cases? Right now both will return 0, but from the end-user perspective the first case requires change of the tweaking factor, while the second may be solved just by using a different public key...

[real-or-random]

while the changes are negligible, if one is aimed to write a deterministic code (like in the case of rust wrapper), it may be good to cover this case as well.

Believe me, you don't care about a failure probability of 2^-128. It's astronomically small. It's much smaller than the probability of a hardware error (e.g., RAM returns wrong value). You can simply abort in this case.

[apoelstra]

Can you add a check with ctmp and ctmp2 swapped? Or maybe there is little value in this since both of these are invalid.

[jonasnick]

Are you suggesting to test this function with an overflowing tweak? That should happen a couple of lines below. I pushed a commit to make the comments more clear.

[apoelstra]

ack except nits

[gmaxwell]

@jonasnick the _test rng's are biased intentionally to hit corner cases and it worked perfectly there if it detected that your scalar generation was generating an invalid scalar. Maybe you want random_scalar_order_test or random_scalar_order instead? It's better to make the tests not fail even with rare values because they can also be moved over to smaller groups where the failures would be common.

[jonasnick]

@gmaxwell Thanks, random_scalar_order is almost what I was looking for and agree that it's better than rand256. I added a function similar to random_scalar_order but returning a byte array.

[real-or-random]

Can you squash this?

[jonasnick]

Rebasing required a bit of thought due to #710. Please have a look.

[jonasnick]

I improved ecdsa_sign's uses of secp256k1_scalar_set_b32_seckey a little bit.

[real-or-random]

Sorry for the many comments now, I probably could have pointed out some of those things earlier.

Since there are invalid tweaks, do we need a tweak_verify function?

[real-or-random]

related to line 617: Is it really the tweak that must be in range or the resulting secret key? I think both?

[jonasnick]

I don't understand what you're suggesting. Yes, the tweak must be in range. The resulting seckey will be in range if the tweak is not the negation of the seckey.

[real-or-random]

Neat! This really improves the logic in the signing function.

Now actually with the duplicate code, we should do this in the recover module as well... Not sure in which order to perform the changes. :/ Do you have a suggestion?

[jonasnick]

Afaics it's just wasted work if we fix the duplicated recovery code. Better to make recovery use the common code in a separate PR (right after this one).

[real-or-random]

ACK except nits

[jonasnick]

@apoelstra would you mind reACKing?

[real-or-random]

ACK 7e3952a I read the diff carefully and tested the changes

[apoelstra]

ACK 7e3952a