RFR: 8295011: EC point multiplication improvement for secp256r1 [v2]

[Xue-Lei Andrew Fan]

On Fri, 28 Oct 2022 09:53:56 GMT, Daniel Jeliński <djelinski at openjdk.org> wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   add missed update to use ECPoint directly
>
> src/jdk.crypto.ec/share/classes/sun/security/ec/ECOperations.java line 209:
> 
>> 207:     }
>> 208: 
>> 209:     public MutablePoint multiply(ECPoint ecPoint, byte[] s) {
> 
> Unused

Ooops, I missed a few update in the commit.  This is used to remove unnecessary conversion from ECPoint to AffinePoint in the caller side.

> src/jdk.crypto.ec/share/classes/sun/security/ec/ECOperations.java line 414:
> 
>> 412: 
>> 413:     static final class Secp256R1Ops extends ECOperations {
>> 414:         private static final ECOperations instance = new Secp256R1Ops();
> 
> I'd make it an instance field in ECOperations and drop this class

Hm, it looks like a better choice.

> src/jdk.crypto.ec/share/classes/sun/security/ec/ECOperations.java line 437:
> 
>> 435:         }
>> 436: 
>> 437:         static PointMultiplier of(ECOperations ecOps, ECPoint ecPoint) {
> 
> Unused

Hm, the code that use this method will be added.

> src/jdk.crypto.ec/share/classes/sun/security/ec/ECOperations.java line 538:
> 
>> 536:         }
>> 537: 
>> 538:         final class Secp256R1 implements PointMultiplier {
> 
> Given that this class can only multiply the generator of SecP256R1, maybe rename it to something like Secp256R1GeneratorMultiplier?

Good point.

> src/jdk.crypto.ec/share/classes/sun/security/ec/ECOperations.java line 547:
> 
>> 545: 
>> 546:             @Override
>> 547:             public ProjectivePoint.Mutable pointMultiply(byte[] s) {
> 
> This method (or class) should really have a comment describing what it does

Yes.  I added comment in the interface method.

> Did you try using AffinePoints instead of ProjectivePoints?

Yes.  See [JDK-8295763](https://bugs.openjdk.org/browse/JDK-8295763).  Unfortunately, the mixed point addition formulas used in JDK implementation does not support additional of neutral point. It may be doable again if the formulas get changed in the future.

> Also, unless you switch to affine points, just use m.fixed().

Yes, the change to AffinePoints is mainly for checking in the following block.  BTW, the class loading is slow, hard-coded tables should be used in a coming update very soon.

> src/jdk.crypto.ec/share/classes/sun/security/ec/ECOperations.java line 634:
> 
>> 632: 
>> 633:                 // Check that the tables are correctly generated.
>> 634:                 for (int d = 0; d < 4; d++) {
> 
> Since this part of code is for runtime verification only, maybe guard it with something like:
> `if (ECOperations.class.desiredAssertionStatus())`
> this way it will only execute if assertions are enabled

Good point.

-------------

PR: https://git.openjdk.org/jdk/pull/10893