On Fri, 28 Oct 2022 09:53:56 GMT, Daniel Jeliński <djelin...@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
