-
Notifications
You must be signed in to change notification settings - Fork 25
Strengthen preconditions on polyveck_add() and polyvecl_add() #724
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
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 @rod-chapman.
I think the pre-conditions can be simplified unless I am missing something.
2e3b6d1 to
833a812
Compare
|
New proof numbers for MLDSA-87 on my Mac, following removal of the redundant preconditions, plus proofs of the 2 calling functions: |
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.
Apologies for not noticing this earlier, but I belive the functional post-condition is no longer needed after #637.
That could further improve proof time.
Maybe the poly_add() contract can be adjusted accordingly (not sure about this part).
|
The post-condition and loop invariant of polyvecl_add() can be weakened as you suggest. The same for polyveck_add() cannot be done, since the stronger post-condition is required to prove crypto_sign_keypair_internal() I will commit and push now. |
I can't follow. In requires(forall(k0, 0, MLDSA_K,
array_bound(v->vec[k0].coeffs, 0, MLDSA_N, INT32_MIN, REDUCE32_DOMAIN_MAX)))So this should work. Now I notice however, that the post-condition of ensures(forall(q2, 0, MLDSA_L,
array_bound(u->vec[q2].coeffs, 0, MLDSA_N, INT32_MIN, REDUCE32_DOMAIN_MAX)))this should be ensures(forall(q2, 0, MLDSA_K,
array_bound(u->vec[q2].coeffs, 0, MLDSA_N, INT32_MIN, REDUCE32_DOMAIN_MAX)))With that fix it proves fine for me without the additional post-condition. |
|
Aha! Well spotted! I will fix as soon as I have a moment... |
1f95625 to
3606284
Compare
|
Corrected. Proof times for MLDSA87 on my Mac: |
|
CI looks good. |
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 @rod-chapman. LGTM. Please clean-up the commit history then this is good to merge.
3606284 to
d6609f4
Compare
Adds explicit bounds constraints on the inputs to these two functions. These significantly reduce the state space of the inputs, and therefore reduce the space that CBMC/Z3 has to search to find a proof. Calling functions (attempt_signature_generataion() and crypto_sign_keypair_internal() ) re-prove OK with these pre-conditions in place. Signed-off-by: Rod Chapman <rodchap@amazon.com>
d6609f4 to
31adeb1
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.
LGTM. Thanks @rod-chapman!
Adds explicit bounds constraints on the inputs to these two functions. These significantly reduce the state space of the inputs, and therefore reduce the space that CBMC/Z3 has to search to find a proof.
Calling functions (attempt_signature_generataion() and crypto_sign_keypair_internal() ) re-prove OK with
these pre-conditions in place.
Proof times on macOS (before this PR) from main, for each parameter set:
Proof times after this PR: