Repository navigation
CI: QR-UOV KAT digests, implementation coverage guard, and a ref/opt/avx2 matrix - #1
Conversation
4134ad9 to
96a3962
Compare
2ed6ccc to
a349143
Compare
a4ad814 to
04686f2
Compare
|
The conflicts are from the QR-UOV base branch having been rebased since this PR was opened.
Could you please rebase these four commits onto the current feat/qruov-integration and force-push the PR branch? I’ll recheck the CI after that. |
04686f2 to
e5a1ba9
Compare
a349143 to
9a7bd56
Compare
|
Rebased onto the current |
Thanks for rebasing this onto the current head and preserving the open-quantum-safe#2595 scheduling changes. I’ll let the CI run and recheck the result before merging. |
|
Two notes on the checks, so nothing here looks stuck for the wrong reason. The four The QR-UOV ref/opt/avx2 matrix won't appear in PR checks at all. It lives in
I can dispatch |
Thanks, that clarifies the queued Zephyr jobs and the extended-test behavior. Yes, please dispatch extended.yml on your fork so we have a GitHub Actions run for the ref/opt/avx2 matrix before merging. |
|
Started: https://github.com/paschal533/liboqs/actions/runs/36589276155
So far |
|
All three QR-UOV jobs passed on GitHub Actions: https://github.com/paschal533/liboqs/actions/runs/36589276155
The run is on |
|
Correction to my last comment. The three QR-UOV jobs in run 36589276155 passed, but the run didn't, and I should have said so. Both Fixed on this branch in 82150a1. It adds
Same arrangement as before: How the suppressions are split. Line numbers are in the ref sources.
None of this contradicts the docs: This PR targets |
Thanks for the correction and for rerunning the full extended suite. |
e5a1ba9 to
35aefac
Compare
Every QR-UOV entry in tests/KATs/sig/kats.json carried only `single`, while all 77 pre-existing signature entries carry both `single` and `all`. tests/test_kat_all.py reads `kats[name]['all']` with no guard, so it failed 30 times with KeyError rather than reporting a data problem. The digests were computed with the method test_kat_all.py itself uses, a sha256 over `kat_sig <alg> --all` with CRLF normalised, cross-checked for one scheme against a direct reimplementation of that test. Each of the 30 outputs also reconstructs to the corresponding published Round 3 PQCsignKAT_32.rsp hash in qruov/round3@3d84d0f, so they are not merely self-consistent with this tree. They were computed on an avx2 build. `single` already agrees across ref, opt and avx2 for all 30, and `--all` differs only in running 100 vectors where `single` runs one, so the digests should not depend on the implementation; the matrix added alongside this checks that on ref and opt rather than assuming it. test_kats_json.py then makes a missing digest fail as what it is. It reads the json alone, so it needs no build and runs on every platform, and it names the algorithms at fault instead of surfacing as a KeyError inside an unrelated test.
src/sig/qruov/CMakeLists.txt tests OQS_ENABLE_SIG_qruov_<scheme>_opt in 30 places and .CMake/alg_support.cmake declares no such option, so the condition is false in every configuration. The 30 opt directories, and the shared qruov_qruov_opt, ship in the tarball and are compiled nowhere. Nothing notices: the build succeeds and the KATs pass on whichever implementation was selected, because all three produce identical vectors. The check compares the options a CMakeLists tests against the options alg_support.cmake declares. Across the whole tree that is 636 declared and 477 referenced, and today exactly 30 are referenced but undeclared, all QR-UOV opt, with no other family affected. It reads CMake files only, so it needs no configure and no build. src/oqsconfig.h.cmake is checked the same way and independently, since it carries a #cmakedefine for all 90 QR-UOV implementations while only 60 can ever be defined. Three generated surfaces disagree because the templates filter implementations differently, so a defect that is invisible in one is visible in another. Withholding an implementation is legitimate, so KNOWN_UNSELECTABLE records it with a reason, and the comparison is exact in both directions: an unselectable implementation that is not recorded fails, and a recorded one that has become selectable also fails. A one-way allowlist would go stale without anyone noticing, which is the same failure mode being fixed here. QR-UOV opt is recorded rather than declared because making it selectable produced reproducible SIGBUS crashes in 8 of the 30 variants on ARM64. The entry also notes that the wrapper selects with `#if _opt / #elif _avx2`, so whenever opt does become selectable that ordering has to change in the same commit or enabling opt will silently drop AVX2. Note that a fix for QR-UOV belongs in scripts/copy_from_upstream/.CMake/alg_support.cmake/add_enable_by_alg_conditional.fragment, which emits a per-implementation option only when the implementation declares a supported platform, and QR-UOV opt declares none. Editing the generated file would be reverted by the upstreamcheck job.
A matrix over the three implementations on x86-64 Linux, each building the 30 schemes and running the QR-UOV KATs. Each job checks which implementation the configure step actually selected before it builds anything. That is the point rather than a detail: all three implementations produce identical KAT vectors, so a job that silently fell back to ref would pass while testing nothing, and that is how opt came to ship for 30 parameter sets without being compiled anywhere. A job that cannot select its target fails and says what it got instead. ref turns the avx2 options off, avx2 requires all 30 to be on, and opt forces the cache variables directly, since no option declares them. The opt job is therefore a configuration override, not a source change, and it exists so the sources in the tarball are still built and KAT-checked on x86-64 while the ARM64 behaviour is investigated. It says nothing about ARM64. The two source-tree checks are also wired into the stylecheck job in basic.yml, which needs no build, so a missing digest or an unreachable implementation fails in about a second rather than after a full matrix.
Two blockers found in review, both of which defeated the stated purpose. The avx2 job did not test avx2. OQS_DIST_BUILD defaults ON, and the generated wrapper then keeps a run-time CPU check that falls back to ref when the host lacks AVX2, BMI2 or AES. The job asserted a compile-time define, so on such a runner it would have gone green while executing ref: exactly the failure it was written to prevent. All three jobs now set OQS_DIST_BUILD=OFF with an explicit OQS_OPT_TARGET, matching what nistkat-x64 already does, so the define implies the code path, and the assertion step refuses to continue if OQS_DIST_BUILD is set at all. The assertion step could pass vacuously. Masking grep's exit status swallowed a real error as well as "no matches", leaving an empty string, and comparing an empty string to a number inside an `if` reads as false rather than tripping set -e. A missing header therefore passed every guard, and the KAT step would then skip all 60 tests and exit 0. It now requires a non-empty header and tolerates only grep's "no match" status. The job moves from a new workflow called by pr.yml into extended.yml, next to nistkat-x64, the other job that runs test_kat_all. Three full builds plus 30 KAT sweeps on every PR ran against the project's stated direction of moving heavy testing out of the PR path. The two source-tree checks stay in basic.yml, which is the cheap half. The KAT step is now run in parallel with a 360 minute timeout, which is what the comparable job uses; 60 minutes serial was optimistic. The allowlist of 30 literal scheme names is gone. Whether an implementation can have an option is decided by the generator from docs/algorithms metadata, so that is where the set is now derived from, leaving only a reason per family and implementation. It no longer needs hand-editing when a parameter set is added or renamed. Comment stripping now removes bracket forms first. A declaration inside a bracket comment was read as live, which would have let the coverage check be silenced, and a hash inside a bracket argument truncated a line that was live, which would have hidden a real finding. Both directions now have regression tests, so the docstring's claim is enforced rather than asserted. kats.json is also checked for whole missing entries against the public header, using the same exclusions test_kat_all.py skips on. Iterating the file only ever validated what was in it, so deleting an algorithm outright passed every check and then returned as the KeyError this module exists to pre-empt. Both new modules are added to the pytest ignore lists that already name test_code_conventions.py, so the source-tree checks run once in stylecheck rather than in every build job.
The constant-time job in extended.yml fails on all 30 QR-UOV parameter sets, because nothing records the operations valgrind flags in the ref implementation. PR CI does not run extended.yml, so this first shows up in a manual or weekly extended run. passes/qruov holds the branches that only touch discarded data: rejection sampling of PRG output (fastop.h) and the retry on an inconsistent linear system (linsys.c:140). issues/qruov holds the rest: the LU pivot search (linsys.c:38), the secret-indexed gf_inv table lookup (gf.h:53), and the canonicalisation of s (qruov_common.c:22, 31, 33). The last is believed harmless, since it branches on s, which is published, but it reveals whether s was negated, so it stays in issues until that is confirmed.
82150a1 to
4fd2870
Compare
|
Rebased onto 35aefac. thank you! |
4b9556c
into
0rlych1kk4:feat/qruov-integration
…n-coverage CI: QR-UOV KAT digests, implementation coverage guard, and a ref/opt/avx2 matrix Signed-off-by: 0rlych1kk4 <108985066+0rlych1kk4@users.noreply.github.com>
The CI half of the split, as offered in #2582. Three commits: the 30 missing
allKAT digests, a guard against shipping an implementation nothing can enable, and a matrix that builds ref, opt and avx2 and checks which one it actually got.Based on
4134ad9. Nothing here changes QR-UOV sources or the importer.The 30 missing
alldigestsEvery QR-UOV entry in
tests/KATs/sig/kats.jsoncarried onlysingle, while all 77 pre-existing signature entries carry both.tests/test_kat_all.pyreadskats[name]['all']with no guard, so it failed 30 times withKeyErrorrather than reporting a data problem.The digests were computed with the method
test_kat_all.pyitself uses, a sha256 overkat_sig <alg> --allwith CRLF normalised, cross-checked for one scheme against a direct reimplementation of that test. Each of the 30 outputs also reconstructs to the corresponding published Round 3PQCsignKAT_32.rsphash inqruov/round3@3d84d0f, so they are not only self-consistent with this tree.Provenance worth stating plainly: they were computed on an avx2 build.
singlealready agrees across ref, opt and avx2 for all 30, and--alldiffers only in running 100 vectors wheresingleruns one, so the digests should not depend on the implementation. The matrix below checks that on ref and opt rather than leaving it assumed. If a digest turns out to be implementation-dependent, that job fails and we have learned something worth knowing.Since first posting this I re-derived all 30 digests on a ref build (gcc 16.1.0, no AVX2, 50 minutes) rather than leave the claim argued. All 30 reproduce exactly, the set matches
kats.jsonwith nothing skipped, and a separate reviewer independently reproduced 3 of them on their own ref build. The digests are therefore confirmed on two of the three implementations, on two operating systems; the matrix below covers opt.tests/test_kats_json.pythen makes a missing digest fail as what it is. It reads the json alone, needs no build, runs on every platform, and names the algorithms at fault.A guard for implementations that ship but cannot be enabled
src/sig/qruov/CMakeLists.txttestsOQS_ENABLE_SIG_qruov_<scheme>_optin 30 places and.CMake/alg_support.cmakedeclares no such option, so the condition is false in every configuration. The 30 opt directories, and the sharedqruov_qruov_opt, ship and are compiled nowhere.Nothing notices, and that is the part worth fixing generically. The build succeeds, and the KATs pass on whichever implementation was selected, because all three produce identical vectors. The result looks exactly like full coverage.
tests/test_cmake_option_coverage.pycompares the options aCMakeListstests against the optionsalg_support.cmakedeclares. Across the whole tree that is 636 declared and 477 referenced, and today exactly 30 are referenced but undeclared, all QR-UOV opt, with no other family affected.src/oqsconfig.h.cmakeis checked the same way and independently, since it carries a#cmakedefinefor all 90 QR-UOV implementations while only 60 can ever be defined.On the design, because your ARM64 finding changed it. I had offered a guard that simply fails when a shipped implementation cannot be enabled. Now that opt is deliberately withheld, that guard would have failed your build on the first run.
Rather than hand-list the 30 names, the check derives them from the same place the generator does.
add_enable_by_alg_conditional.fragmentemits a per-implementation option only for an implementation that is not the family default and that declares either a supported platform ormemory_optimized; QR-UOV's opt declaressupported-platforms: [], so no option can exist. Readingdocs/algorithms/*/*.ymlfor that condition yields exactly the QR-UOV opt implementations and nothing else tree-wide, so the test carries only a reason per family, not a list of schemes, and needs no edit when a parameter set is added or renamed. An implementation that cannot be enabled and has no recorded reason fails, and a reason recorded for something that has become selectable also fails.The recorded reason cites the ARM64 SIGBUS and notes that the wrapper selects with
#if _opt / #elif _avx2, so whenever opt does become selectable that ordering has to change in the same commit, or enabling opt will silently drop AVX2.If you would rather make opt selectable than record it, the fix belongs in
scripts/copy_from_upstream/.CMake/alg_support.cmake/add_enable_by_alg_conditional.fragment, which emits a per-implementation option only when the implementation declares a supported platform, and QR-UOV's opt declaressupported-platforms: []. Editing the generated.cmakewould be reverted by theupstreamcheckjob. Either way the guard is satisfied, which is the intent: it asks for a decision, not a particular one.The matrix
A
qruov-implementationsjob in.github/workflows/extended.yml, next tonistkat-x64, the other job that runstest_kat_all: ref, opt and avx2 on x86-64 Linux, each building the 30 schemes and running the QR-UOV KATs in parallel.Each job asserts which implementation the configure step actually selected before it builds. That is the point rather than a detail. All three produce identical KAT vectors, so a job that silently fell back to ref would pass while testing nothing, which is how opt came to ship for 30 parameter sets without being compiled anywhere.
Two corrections since the first version of this PR, both of which defeated that purpose. I had this reviewed before asking you to spend time on it, and it came back with two blockers worth repeating here because they are the same failure mode the PR is about.
The avx2 job did not test avx2.
OQS_DIST_BUILDdefaults ON, and the generated wrapper then keeps a run-time CPU check that falls back to ref when the host lacks AVX2, BMI2 or AES (sig_qruov_1q7L10aes.c:52-66). Asserting the compile-time define proved nothing about what ran, so on such a runner the job would have gone green while executing ref. All three jobs now setOQS_DIST_BUILD=OFFwith an explicitOQS_OPT_TARGET, matchingnistkat-x64, so the define does imply the code path; the assertion step refuses to continue ifOQS_DIST_BUILDis set at all. If a runner lacks the ISA the job now dies loudly instead of quietly grading ref.The assertion could also pass vacuously. Masking grep's exit status swallowed a real error as well as "no matches", leaving an empty string, and comparing an empty string to a number inside an
ifreads as false rather than trippingset -e. A missing header passed every guard, and the KAT step would then skip all 60 tests and exit 0. It now requires a non-empty header and tolerates only grep's "no match".It sits in
extended.ymlrather thanpr.ymlon the same reasoning as open-quantum-safe#2509: three full builds plus 30 KAT sweeps on every PR cuts against moving heavy testing off the PR path. The two source-tree checks stay inbasic.yml, which is the cheap half, and are added to the pytest ignore lists that already nametest_code_conventions.pyso they run once rather than in every build job.The opt job is a configuration override, not a source change. It exists so the sources in the tarball are still built and KAT-checked on x86-64 while the ARM64 behaviour is investigated, and it may be useful to that investigation: if opt passes here and crashes there, the fault is ARM-specific rather than in the opt sources generally. It says nothing about ARM64 itself.
One limit worth stating plainly: the three implementations share
qruov_qruov_common/, so agreement between them is evidence of reproducibility, not of correctness. A fault in the shared layer would be identical in all three and every job would stay green. What covers that is the comparison against the published Round 3 vectors described above, which is separate from this matrix.What I could not test
The workflow itself is still unrun on Linux. I am on Windows x86-64, where the avx2 options are gated to
Linux|Darwin, so I cannot exercise the Linux gating locally. What I did verify here: a clean ref build of all 30 schemes, 30 of 30 single-vector KATs passing on it, thatOQS_MINIMAL_BUILD="SIG_qruov"selects exactly the 30 schemes, and that the flag extraction rejects hostile scheme names (I fed it$(...), backticks,;and a-Dinjection attempt; the[A-Za-z0-9]+class drops all of them, so nothing outside that set reaches the shell). The guard and the digest checks have tests that confirm they fail in each direction they claim to, including a#[[ ]]bracket comment that previously read as live code.That gap is exactly why every job asserts its own selection rather than trusting my flags: if the flag logic is wrong on a Linux runner, the job fails with the counts instead of quietly testing ref three times. The first run on your branch is the real test, and I would rather it fail loudly than pass vacuously.
Unrelated and not addressed here:
tests/test_code_conventions.py::test_memory_functionscannot run on Windows, as it reads source files with the locale codec and hitsUnicodeDecodeError.Happy to move the matrix into
linux.ymlorplatforms.ymlinstead, drop the opt job, or turn the recorded reason into a hard failure once opt is selectable. Say which you prefer.