-
Notifications
You must be signed in to change notification settings - Fork 25
Remove unused spec for mld_montgomery_reduce, add bounds reasoning #668
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
Conversation
944c6be to
41f877b
Compare
41f877b to
3b686e6
Compare
mkannwischer
left a comment
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.
Thanks @hanno-becker - very welcome simplification. How about we eliminate the larger bound from the documentation altogether?
1ced8b1 to
ba6e679
Compare
0683b6e to
608ae8f
Compare
jammychiou1
left a comment
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.
Thank you @hanno-becker for your work! Very useful result for me to reference from the asm bound comments :D
Only two nits remaining now. Please see the comments.
d9a332b to
4b66648
Compare
Context: We previously had two specifications for mld_montgomery_reduce: - A 'weak' one where the output upper bound matches the input upper bound of mld_reduce32(). - A 'strong' one where the output interval is (-Q,Q), exclusively. Both contracts were combined into one in a somewhat ad-hoc way (the 'weak' version being encoded as pre- and post-conditions as usual, the 'strong' version being stated as an implication in the post-condition). The combined contract was used in the CBMC proofs of - mld_poly_pointwise_montgomery - mld_polyvecl_pointwise_acc_montgomery There are two more functions which rely on mld_montgomery_reduce, namely - mld_fqmul - mld_fqscale Observations: - The 'weak' contract of mld_montgomery_reduce is not needed: In the context of mld_poly_pointwise_montgomery and mld_polyvecl_pointwise_acc_montgomery, the strong version applies. - The 'strong' version also applies in the context of mld_fqmul and mld_fqscale. Accordingly, this commit removes the 'weak' version and re-formulates the 'strong' version in traditional pre/post condition form. The commit also removes the macro wrapper around the input bound, which I find difficult to read. It's easier to understand if the bound is explicit. The 'strong' version only applies to `mld_fqscale` because of the recent modification of the iNTT output bound from 3/4*Q to Q. Otherwise, an even stronger spec would have been needed. At present, one can even remove mld_fqscale and just call mld_fqmul directly, but this commit does not do this. The commit also takes the opportunity to add some pen-and-paper bounds reasoning for mld_montgomery_reduce. Fixes #602 Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
4b66648 to
5221bd1
Compare
jammychiou1
left a comment
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. Thank you for your hard work Hanno!
|
Thanks Hanno! |
Context:
We previously had two specifications for mld_montgomery_reduce:
Both contracts were combined into one in a somewhat ad-hoc way (the 'weak' version being encoded as pre- and post-conditions as usual, the 'strong' version being stated as an implication in the post-condition).
The combined contract was used in the CBMC proofs of
There are two more functions which rely on mld_montgomery_reduce, namely
Observations:
Accordingly, this commit removes the 'weak' version and re-formulates the 'strong' version in traditional pre/post condition form.
The commit also removes the macro wrapper around the input bound, which I find difficult to read. It's easier to understand if the bound is explicit.
The 'strong' version only applies to
mld_fqscalebecause of the recent modification of the iNTT output bound from 3/4*Q to Q. Otherwise, an even stronger spec would have been needed. At present, one can even remove mld_fqscale and just call mld_fqmul directly, but this commit does not do this.The commit also takes the opportunity to add some pen-and-paper bounds reasoning for mld_montgomery_reduce.
mld_fqmulandmld_fqscale#602