Skip to content
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 verbal specification and runtime-checking for fqmul() #289

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

hanno-becker
Copy link
Contributor

This is a preparatory PR for CBMC proofs. It reorders arguments to fqmul() so that the signed canonical twiddle is always in the second position, says as much in the textual description of fqmul(), and adds runtime assertions checking the input and output bounds during runtime (for debug builds).

@hanno-becker hanno-becker added the benchmark this PR should be benchmarked in CI label Oct 31, 2024
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - very good catch with the swapped arguments.

Some small nits.

mlkem/debug/debug.h Show resolved Hide resolved
mlkem/reduce.h Show resolved Hide resolved
We already have `mlkem_debug_check_bounds()` to check
bounds on all elements of an array.

In preparation for use in `fqmul()`, this commit adds
generic `mlkem_debug_assert()` and `ASSERT()` to checking
assertions, and a wrapper assertion `SCALAR_BOUND` to assert
absolute bounds on scalars.

Signed-off-by: Hanno Becker <[email protected]>
For bounds reasoning, we need to know that `fqmul()` is
always called with a signed canonical value in one of its
arguments.

To simplify the spec, this commit reorders arguments to `fqmul()`
so that the signed canonical twiddle is always the _second_ argument.

It checks this through a debug-only assertion, and also adds
a debug-assertion that the return value of `fqmul()` is always
of absolute value < MLKEM_Q.

Signed-off-by: Hanno Becker <[email protected]>
`mlkem_debug_check_bounds()` and its wrapper macros `BOUNDS()`
etc. are working with _exclusive_ bounds, but the documentation
for `mlkem_debug_check_bounds()` wrongly stated that its bounds
arguments would be inclusive.

This commit fixes the documentation of `mlkem_debug_check_bounds()`
to say that bounds are exclusive.

No change to the code is needed: We only work with the wrapper
macros, and those were already documented and used with exclusive
bounds.

Signed-off-by: Hanno Becker <[email protected]>
@hanno-becker hanno-becker merged commit 0936b16 into main Oct 31, 2024
27 checks passed
@hanno-becker hanno-becker deleted the fqmul_prepare branch October 31, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark this PR should be benchmarked in CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants