Make ec_ arithmetic more consistent and add documentation

[jonasnick]

As brought up by @afk11 on #secp256k1 there are a couple of undocumented and inconsistent surprises in the ec arithmetic functions (that is secp256k1_ec_pub/privkey_neg, _tweak_add and tweak_mul.

In particular:

1. Right now, ec_privkey_neg, ec_privkey_tweak_add and ec_privkey_tweak_mul do not fail if the seckey is invalid, but they may give unexpected results in that case (f.e. secp256k1_ec_privkey_negate and invalid privkeys #488). In order to avoid that, the user should call ec_seckey_verify before calling these functions, but that's not documented. Alternatively, these functions could call ec_seckey_verify themselves. In any case we should define what a valid seckey is (in the documentation of ec_seckey_verify for example) and refer to that in the argument documentation of other functions.

2. ec_privkey_tweak_add and ec_privkey_tweak_mul can fail (return 0). As a result the seckey argument will be zeroized. This is not documented. There's conflicting ideas whether this is a good idea:

   • "preserving any entropy in the bad input is the safer behaviour (e.g. the data in it might get fed elsewhere into some rng hash or something)" (@gmaxwell in Return 0 if invalid seckey is given to ec_privkey_negate #668).
   • vs.

     17:25 < andytoshi> leaving it alone sounds very dangerous, presumably the original key will "work" in subsequent algos
     17:25 < andytoshi> while a 0 key is pretty visible and will cause failures pretty-much no matter what you do with it
     

     However, right now we wouldn't detect the 0 key in ec_privkey_tweak_add and tweak_mul

3. privkey_tweak_add and privkey_tweak_mul fail if the resulting privkey is 0 but not if it overflows. That's not ideal because you won't be able to sign for it. Additionally, the documentation for privkey_tweak_add is wrong: returns: [...] 0 the resulting private key [...] would be invalid (only when the tweak is the complement of the corresponding private key) (same for pubkey_tweak_add). An overflowing private key is also invalid. (EDIT: actually the privkey can not overflow in these functions)

4. All this behavior should be tested

5. See naming: privkey vs seckey #670 for naming improvements (seckey vs. privkey)

[real-or-random]

What do people think about adding ec_seckey_tweak_add/mul and keeping ec_privkey_tweak_add/mul as a (maybe deprecated) aliases?

[elichai]

What do people think about adding ec_seckey_tweak_add/mul and keeping ec_privkey_tweak_add/mul as a (maybe deprecated) aliases?

I think that's a good idea. we should try to make the API breakage gradual.

[jonasnick]

What do people think about adding ec_seckey_tweak_add/mul and keeping ec_privkey_tweak_add/mul as a (maybe deprecated) aliases?

Sounds good to me. Should do that sooner rather than later because schnorrsig PR currently has secp256k1_xonly_privkey_tweak_add. I'll add that to my ec_arithmetic cleanup PR.

[real-or-random]

Ok. The only possible drawback is that we may want to rethink these functions anyway in the future (hash based? Add BIP32 module? Taproot module?). But I don't think that should stop us now from renaming.