-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 12 XMSS and 16 XMSSMT parameters. #1489
Add 12 XMSS and 16 XMSSMT parameters. #1489
Conversation
@dstebila I got CI timeout, what can I do to extend the test time longer? |
https://stackoverflow.com/questions/36173553/how-to-extend-timeout-for-tests-in-circleci#36209650 HOWEVER, may I question the practical value of code that apparently requires more than 10 minutes for a single signature operation? Or is the test too big? |
Hi @baentsch , I think the test is too big (if the test is all schemes in liboqs, XMSS test only 1 iteration per parameter), when it reaches to XMSS it's almost out of time. I assume CI is slower, if I multiply my local number by 100x, then it's about ~100 seconds. Still below the 600 seconds (10 minutes). Yes, I think the choice of parameters Sign/Verify (we don't test keygen here, because it's very very long) is reasonable. (again, 1 seconds for 28 Sign/Verify test in my local). Should I go ahead and modify the timeout? |
9969445
to
a2e6076
Compare
f58f079
to
f4f3799
Compare
That should only be the last resort. But it seems you found a way without doing so: Great! Or are the tests now doing less? |
@baentsch I realize that the You can see the idea in this section of the code: Lines 179 to 245 in d797221
Now I don't know why my test on |
No it's not off by one. But somehow I don't know why the hash is incorrect. |
The reason the output hash is incorrect because the comparison with In KAT file, when hash tree is at 2^40, it's overflow Next, Am I allowed to modify the reference code (as long as same KAT output) to fix the scan_build? @baentsch can you recommend what should I do to clear CI? |
I'd personally say "Yes, of course" -- if that corrects code otherwise containing flaws. Now, on second consideration the question is this: How did you import this reference code? If it's a fully automated process, please consider doing something similar to what we already do in |
Unfortunately, I import this code manually. The original reference does not:
And it was not updated for years, due to this I think automation from upstream is a bit overkill. I think my code is a little far ahead of reference implementation. I will look into the code to fix |
Good: That then IMO gives you a clear answer to
Yes -- by all means: Make this code base the new (better) code base of reference. |
Scan build is happy. This branch is ready to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to merge to "stateful-sigs". Thanks for the latest code enhancements, @ducnguyen-sb ! For a "main" branch merge, I'm personally missing quite a bit of documentation -- but that's arguably another separate task/PR.
* populate all 28 XMSS parameters * clean up * remove wanrings in scanbuild * change free to OQS_MEM_insecure_free * fix build warning * fix integer in i386 platforms * proper type for sigs_remain and sig_maximum * remove size_t in signature remain and total * make scan-build happy
* populate all 28 XMSS parameters * clean up * remove wanrings in scanbuild * change free to OQS_MEM_insecure_free * fix build warning * fix integer in i386 platforms * proper type for sigs_remain and sig_maximum * remove size_t in signature remain and total * make scan-build happy
* populate all 28 XMSS parameters * clean up * remove wanrings in scanbuild * change free to OQS_MEM_insecure_free * fix build warning * fix integer in i386 platforms * proper type for sigs_remain and sig_maximum * remove size_t in signature remain and total * make scan-build happy
* populate all 28 XMSS parameters * clean up * remove wanrings in scanbuild * change free to OQS_MEM_insecure_free * fix build warning * fix integer in i386 platforms * proper type for sigs_remain and sig_maximum * remove size_t in signature remain and total * make scan-build happy
* populate all 28 XMSS parameters * clean up * remove wanrings in scanbuild * change free to OQS_MEM_insecure_free * fix build warning * fix integer in i386 platforms * proper type for sigs_remain and sig_maximum * remove size_t in signature remain and total * make scan-build happy
* populate all 28 XMSS parameters * clean up * remove wanrings in scanbuild * change free to OQS_MEM_insecure_free * fix build warning * fix integer in i386 platforms * proper type for sigs_remain and sig_maximum * remove size_t in signature remain and total * make scan-build happy
commit 244288f Add XMSS parameter xmss_sha256_h10 (#1482) commit a7e26d9 Add 12 XMSS and 16 XMSSMT parameters. (#1489) commit 4694fc3 Add secret key object to XMSS (#1530) commit 99067be Add XMSS Serialize/Deserialize (#1542) commit 2dbfc40 Update XMSS secret key object APIs, sync with LMS (#1588) commit 47740ad Enforce idx from unsigned int to uint32_t. (#1611) commit 9610576 Fix windows-x86 and arm compiling error. (#1634) commit bb658b7 Address stateful-sigs comments in #1650 (#1656) commit 7db8ddf Update `sig_stfl.h` document for #1650 (#1655) commit c3e5750 Add Apache 2.0 and MIT License to XMSS (#1662) commit e1f02b2 Change XMSS License from `(Apache 2.0 AND MIT)` to `(Apache 2.0 OR MIT) AND CC0-1.0` (#1697) commit 17c12c3 Add return status for XMSS lock/unlock functions. (#1712) commit 1941636 Add return check for lock/unlock function (#1727) commit b45415c Use `abort()` instead of exit to get the trace log. (#1728) commit ba63672 Reduce number of `malloc/free` call in `XMSS/external` (#1724) Signed-off-by: Duc Tri Nguyen <dnguye69@gmu.edu>
fast
approach. This approach improved the performance ofsigning
at the cost of largersk
._fast
parameters is incompatible with the general version. At first, I couldn't believe it, I went back and generate original KATs file from original Github repo. And I confirm that the_fast
and general version, given the same message and same randomness, it will generate different signatures. Given the signature of_fast
, the general version cannot verify using_fast
parameter public key.Similar issue in xmss-reference: XMSS/xmss-reference#17
All the parameters added to XMSS. https://github.com/ducnguyen-sb/xmss-reference/blob/master/external/xmss_liboqs_params.md