On Tue, 15 Apr 2025 18:21:00 GMT, Ferenc Rakoczi <d...@openjdk.org> wrote:
>>> @ferakocz
>>>
>>> Hi Ferenc,
>>>
>>> Sorry, but I still had a few comments to add to the KyberNTTMult routine to
>>> clarify exactly how the load, compute and store operations relate to the
>>> original Java source. That's the only remaining code that I felt needed
>>> further clarification for maintainers. So, after you work through them I
>>> can approve the PR.
>>
>> No problem , it was easy to make the changes. Thanks again!
>
>> @ferakocz I reran test
>> jtreg:test/jdk/sun/security/provider/acvp/Launcher.java and hit a Java
>> assertion:
>>
>> ```
>> >> ML-KEM-512 encapsulation
>> 1 STDERR:
>> java.lang.AssertionError
>> at
>> java.base/com.sun.crypto.provider.ML_KEM.twelve2Sixteen(ML_KEM.java:1371)
>> at java.base/com.sun.crypto.provider.ML_KEM.decodePoly(ML_KEM.java:1408)
>> at
>> java.base/com.sun.crypto.provider.ML_KEM.decodeVector(ML_KEM.java:1337)
>> at java.base/com.sun.crypto.provider.ML_KEM.kPkeEncrypt(ML_KEM.java:712)
>> at java.base/com.sun.crypto.provider.ML_KEM.encapsulate(ML_KEM.java:555)
>> at
>> java.base/com.sun.crypto.provider.ML_KEM_Impls$K.implEncapsulate(ML_KEM_Impls.java:134)
>> at
>> java.base/sun.security.provider.NamedKEM$KeyConsumerImpl.engineEncapsulate(NamedKEM.java:124)
>> at java.base/javax.crypto.KEM$Encapsulator.encapsulate(KEM.java:265)
>> at java.base/javax.crypto.KEM$Encapsulator.encapsulate(KEM.java:225)
>> at ML_KEM_Test.encapDecapTest(ML_KEM_Test.java:98)
>> at ML_KEM_Test.run(ML_KEM_Test.java:41)
>> at Launcher.run(Launcher.java:160)
>> at Launcher.main(Launcher.java:122)
>> at
>> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
>> at java.base/java.lang.reflect.Method.invoke(Method.java:565)
>> at
>> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:335)
>> at java.base/java.lang.Thread.run(Thread.java:1447)
>>
>> JavaTest Message: Test threw exception: java.lang.AssertionError
>> JavaTest Message: shutting down test
>> ```
>>
>> The offending code is this:
>>
>> ```
>> private void twelve2Sixteen(byte[] condensed, int index,
>> short[] parsed, int parsedLength) {
>> int i = parsedLength / 64;
>> int remainder = parsedLength - i * 64;
>> if (remainder != 0) {
>> i++;
>> }
>> assert (((remainder != 0) && (remainder != 48)) || <== assert here
>> index + i * 96 > condensed.length);
>> ```
>>
>> I believe the logic is reversed here i.e. it should be:
>>
>> ```
>> assert ((remainder == 0) || (remainder == 48)) &&
>> index + i * 96 <= condensed.length);
>> ```
>>
>> Does that sound right?
>
> Aarrrrgh, yes. I forgot to negate that condition when I went from throwing an
> exception to assert, and I also thought, incorrectly, that -ea would enable
> my assertions when I tested :-( .
> Thanks a lot for catching it!
> @ferakocz I reran the test and the perf test and all appears to be good. Nice
> work!
@adinn Thanks, Andrew, for all the help, it really looks nicer than it looked
before your review! Would you /sponsor the integration?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/23663#issuecomment-2809306397
