Make ec_ arithmetic more consistent and add documentation #671

1. 
   jonasnick commented at 7:39 pm on October 9, 2019: contributor

   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. #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 #668).
      • vs.

        017:25 < andytoshi> leaving it alone sounds very dangerous, presumably the original key will "work" in subsequent algos
        117: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 #670 for naming improvements (seckey vs. privkey)

2. jonasnick cross-referenced this on Dec 17, 2019 from issue Make ec_ arithmetic more consistent and add documentation by jonasnick
3. 
   real-or-random commented at 11:39 am on December 18, 2019: contributor

   What do people think about adding ec_seckey_tweak_add/mul and keeping ec_privkey_tweak_add/mul as a (maybe deprecated) aliases?
4. 
   elichai commented at 12:27 pm on December 18, 2019: contributor

   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.

5. 
   jonasnick commented at 2:01 pm on December 19, 2019: contributor

   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.

6. 
   real-or-random commented at 2:34 pm on December 19, 2019: contributor

   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.
7. real-or-random cross-referenced this on Mar 31, 2020 from issue naming: privkey vs seckey by real-or-random
8. jonasnick added this to the milestone initial release on Apr 28, 2020
9. real-or-random closed this on Apr 30, 2020

   ---

Contributors
jonasnick real-or-random elichai

Milestone
stable release (1.0.0-rc.1)