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

Clean up uses of coerceSize #101

Closed
mccleeary-galois opened this issue Jul 30, 2024 · 6 comments · Fixed by #107
Closed

Clean up uses of coerceSize #101

mccleeary-galois opened this issue Jul 30, 2024 · 6 comments · Fixed by #107
Assignees
Labels
good first issue Good for newcomers improvement Addresses fixes or changes to existing specs

Comments

@mccleeary-galois
Copy link
Contributor

mccleeary-galois commented Jul 30, 2024

Now that GaloisInc/cryptol#1713 has been merged, some use cases of coerceSize should be able to be removed from cryptol-specs, particularly in ML-KEM, e.g.

new_v = coerceSize (lower # upper)

@mccleeary-galois mccleeary-galois changed the title Clean up uses of coerce-size Clean up uses of coerceSize Jul 30, 2024
@rod-chapman
Copy link
Contributor

If we make this change now, are we happy that cryptol-specs will depend on a nightly build of Cryptol, rather than a formally tagged release? Put another way - should we create a release 3.2 of Cryptol before updating this repo?

@mccleeary-galois
Copy link
Contributor Author

cryptol-specs current target is nightly at the moment,

image: ghcr.io/galoisinc/cryptol:nightly

However, the CI overhaul in the roadmap plans to test cryptol-specs against nightly and a predetermined set of releases of cryptol.

I am okay with holding off on this ticket until the new CI is in place (we can start cutting tickets over here for that as well).

@rod-chapman
Copy link
Contributor

OK... sounds good

@marsella
Copy link
Contributor

marsella commented Aug 5, 2024

Oops, I missed the discussion on this issue about waiting to fix. I have a branch that adds all the fixes, I'll just leave it as a draft PR until we're ready to stabilize CI and the version of cryptol with this fix in it. See #107.

@mccleeary-galois
Copy link
Contributor Author

This will still be useful for testing nightly in the meantime. Thanks Marcella!

@mccleeary-galois
Copy link
Contributor Author

mccleeary-galois commented Aug 12, 2024

Discussed this internally and we came to the conclusion of releasing a new Cryptol version and having cryptol-specs CI target nightly and the most recently released version of Cryptol as a starting point.

@mccleeary-galois mccleeary-galois added enhancement New feature or request good first issue Good for newcomers labels Aug 29, 2024
@marsella marsella added improvement Addresses fixes or changes to existing specs and removed enhancement New feature or request labels Aug 29, 2024
marsella added a commit that referenced this issue Aug 29, 2024
- I misread `max 1 n - 1` when making the previous change. Removes the
  `max`s that are obsoleted by the numeric constraint guards I added.
- Adds docstrings for properties in `ntt` that I used to debug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers improvement Addresses fixes or changes to existing specs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants