group: ge(j) should have as invariant that the curve equation holds #1376

1. 
   real-or-random commented at 11:12 pm on July 11, 2023: contributor

   I was surprised to see that this may be violated in secp256k1_eckey_pubkey_parse: https://github.com/bitcoin-core/secp256k1/blob/cc557575522c4cf11e5bcde1fea9637339cea21f/src/eckey_impl.h#L26-L31

   I claim

   • ge(j) objects should always represent valid points on the curve.
   • This invariant should be checked in VERIFY mode, in secp256k1_ge(j)_verify or at least in secp256k1_ge_set_xy
   • There should be a separate function secp256k1_ge_try_set_xy which checks if (x,y) is on the curve, and only if yes, returns 1 and outputs a ge. That function can be used to implement secp256k1_eckey_pubkey_parse.
   • secp256k1_ge_is_valid_var should be removed (or repurposed to secp256k1_ge_verify_on_curve_var without return value, as mentioned above).
2. real-or-random added the label assurance on Jul 11, 2023
3. real-or-random added the label refactor/smell on Jul 11, 2023
4. 
   sipa commented at 1:22 am on July 12, 2023: contributor

   secp256k1_ge(j) are also used to represent points on effective affine isomorphic curves. Does that mean that in VERIFY mode we need to store the isomorphism factor inside ge(j).
5. 
   real-or-random commented at 12:12 pm on July 12, 2023: contributor

   secp256k1_ge(j) are also used to represent points on effective affine isomorphic curves.

   Oh right, that’s a valid point. [1]

   Does that mean that in VERIFY mode we need to store the isomorphism factor inside ge(j).

   Perhaps it makes sense to have a separate type for effective affine computations. This will also make the code more self-documenting and thus enhance readability.

   [1] Absolutely no pun intended.

6. 
   apoelstra commented at 12:35 pm on July 12, 2023: contributor

   When doing the exhaustive tests we also use off-curve points (though we stick to a single group, so we use a fixed isomoprhism class). So we would need to be able to configure or disable this check.

   I was going to say that there’s also some really awful abuse of ge_storage in ecmult_impl.h, which I introduced in #557, but sadly(?) all that got deleted in #956 when we deleted ecmult_context.

   Perhaps it makes sense to have a separate type for effective affine computations. This will also make the code more self-documenting and thus enhance readability.

   I think this is a good idea, independently of this.

Contributors
real-or-random sipa apoelstra

Labels
assurance refactor/smell