Skip to content

Expand test coverage: compile-fail invariants and nonce derivation correctness - #6

Merged
nadimkobeissi merged 6 commits into
symbolicsoft:mainfrom
danieldia-dev:main
May 24, 2026
Merged

Expand test coverage: compile-fail invariants and nonce derivation correctness#6
nadimkobeissi merged 6 commits into
symbolicsoft:mainfrom
danieldia-dev:main

Conversation

@danieldia-dev

@danieldia-dev danieldia-dev commented May 22, 2026

Copy link
Copy Markdown
Contributor

This PR makes two small independent improvements:

  1. Three trybuild compile-fail tests are added to lock in type-system invariants that were previously untested: Context is not Clone, ExportOnly cannot be used with seal_base, and PQ KEMs cannot be used with seal_auth. A regression in any of these previously would have silently widened the API.

  2. A nonce_derivation_xors_seq_into_base_nonce test is added to context.rs that directly verifies the XOR formula from RFC 9180 §5.2 (i.e. nonce = base_nonce XOR I2OSP(seq, Nn) where I2OSP is "Integer to Octet String Primitive") across four specific sequence number values: 0, 1, 256, and 0x0102_0304_0506_0708. This tests the correctness of the individual nonce derivation computation, as opposed to running a large number of operations and checking for uniqueness.

All compile-fail tests pass under cargo test --features pq --test compile_fail.

The README testing section is updated to document the new cargo test --features pq --test compile_fail command and to mention the compile-fail invariant tests and nonce derivation unit test in the coverage summary.

Note that the value 0x0102_0304_0506_0708 was chosen as a test vector because every byte is distinct and non-zero, which means that a bug in byte indexing or byte ordering (e.g. writing a byte to the wrong position or reversing the endianness) would produce a visibly wrong result rather than accidentally passing.

@nadimkobeissi nadimkobeissi self-assigned this May 24, 2026
@nadimkobeissi nadimkobeissi added the enhancement New feature or request label May 24, 2026
@nadimkobeissi

Copy link
Copy Markdown
Member

@danieldia-dev Thanks for this great PR. Some fixes are required before merging:

  • compute_nonce_for_test may be unnecessary: the new tests module is a child of mod context and already has visibility into compute_nonce, so it could be called directly. Please double check this.

  • Regarding the addition to README.md, cargo test --features pq compile_fail uses compile_fail as a name-filter rather than a binary selector; cargo test --features pq --test compile_fail more precisely targets the test binary declared in Cargo.toml. Please switch it to the latter.

  • Please revert the capitalization of X-wing in src/kem/pq.rs: the rest of the file uses X-Wing for the brand and x-wing for the crate, but the PR changes the comment to X-wing which matches neither convention: the original x-wing was actually correct since the sentence is about the crate's rand_core dependency.

  • trybuild stderr is sensitive to compiler version: the .stderr fixtures match exactly against rustc output, so toolchain bumps can break these tests. Worth pinning the CI toolchain (the crate already declares rust-version = "1.95") and documenting TRYBUILD=overwrite cargo test --features pq --test compile_fail as the regen procedure.

  • rand_core = "0.9" dev-dep looks redundant: it's already declared as a regular dependency, so it should already be reachable from integration tests. Please remove the redundancy.

  • Unused bindings in compile-fail tests: let (enc, ct) = ... in export_only_no_seal.rs and pq_no_auth.rs should be let (_enc, _ct) = ... so that if a future change ever makes the call typecheck, the test fails on the intended trait-bound assertion rather than on a stray unused-variable warning.

@danieldia-dev

Copy link
Copy Markdown
Contributor Author

Understood, I will implement the above fixes, as required. Thanks for the review!

@nadimkobeissi

Copy link
Copy Markdown
Member

@danieldia-dev The README's TRYBUILD=overwrite cargo test --features pq --test compile_fail line is wrong as the documented test command, since TRYBUILD=overwrite rewrites the .stderr fixtures with whatever the compiler emits and therefore makes the test always pass unconditionally, defeating the regression-check purpose of the compile-fail suite. The fix is to split it into two clearly-labeled entries: the bare form as the normal invocation, and the TRYBUILD=overwrite form documented separately as the regen-only procedure for intentional fixture updates.

@nadimkobeissi
nadimkobeissi merged commit 3456706 into symbolicsoft:main May 24, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

2 participants