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

Make more modules panic free #713

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Make more modules panic free #713

wants to merge 16 commits into from

Conversation

karthikbhargavan
Copy link
Contributor

@karthikbhargavan karthikbhargavan commented Dec 15, 2024

This PR adds runtime safety proofs for portable/sampling.rs and for polynomial.rs.
Since polynomial now has new preconditions, the verification of NTT and Matrix needs to be updated next.
It also bumps up the Z3 rlimits for IND-CPA to restore verification for that module.

@mamonet mamonet requested a review from a team as a code owner December 20, 2024 06:57
@mamonet
Copy link
Member

mamonet commented Dec 20, 2024

I uploaded panic-free polynomial module (some functions have proofs)

@franziskuskiefer
Copy link
Member

This should be against main now.

@karthikbhargavan karthikbhargavan changed the base branch from dev to main December 20, 2024 07:45
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Generally appears to be ok. But needs more context.
Please add some description on what this is doing.

@karthikbhargavan
Copy link
Contributor Author

Yes, I made this into a PR to get CI working on it, but it will go through one more round of work on Monday before I ask for a "real" review.

@franziskuskiefer franziskuskiefer marked this pull request as draft December 23, 2024 08:33
@franziskuskiefer
Copy link
Member

Yes, I made this into a PR to get CI working on it, but it will go through one more round of work on Monday before I ask for a "real" review.

I made it into a draft for you

@franziskuskiefer franziskuskiefer linked an issue Jan 15, 2025 that may be closed by this pull request
@mamonet mamonet marked this pull request as ready for review January 17, 2025 13:43
@mamonet
Copy link
Member

mamonet commented Jan 17, 2025

The changes are ready for review, and the branch passes verification process.

panic freedom improvements
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

lgtm with the two nits addressed/commented on.

I checked the C extraction, that's the new functions that were added in polynomial. The eurydice version is newer now, but the code doesn't change with that. So no new extraction needed.

These changes are unfortunate because of the functional style of the code and this causes a bunch of memcpys in the C code. But that's an improvement for another PR.

#[hax_lib::fstar::before(r#"[@@ "opaque_to_smt"]"#)]
#[hax_lib::requires(fstar!(r#"add_vector_pre $lhs $rhs"#))]
#[hax_lib::ensures(|result| fstar!(r#"add_vector_post $result $lhs $rhs"#))]
fn add_vector<Vector: Operations>(lhs: Vector, rhs: &Vector) -> Vector {
Copy link
Member

Choose a reason for hiding this comment

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

Having to pull out these again is pretty ugly. Please add some comments to both of these functions with some text why it's necessary for the proof right now. And ideally we have an issue on hax for tracking this.

@@ -286,7 +286,7 @@ pub(crate) fn ntt_at_layer_7<Vector: Operations>(re: &mut PolynomialRingElement<
}

#[inline(always)]
#[hax_lib::fstar::verification_status(panic_free)]
#[hax_lib::fstar::verification_status(lax)]
Copy link
Member

Choose a reason for hiding this comment

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

Do we have issus to get back to panic freedom?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admitted functions in MLKEM
3 participants