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

Fix blst-verification regression test and update pinned commit #1622

Merged
merged 4 commits into from
Mar 23, 2022

Conversation

chameco
Copy link
Contributor

@chameco chameco commented Mar 18, 2022

See #1620 for context on this.

Note that this still uses git config to change SSH submodule clones into HTTPS. I'm not sure if there's a great way around this generally. It's often more convenient to use SSH for submodules while developing (e.g. if one wants to push commits from the submodule), while using SSH from Actions, especially inside Docker, will likely involve a lot of cumbersome key management.

I'm marking this as a draft for now because the new checked-out commit still isn't on main of blst-verification.

@chameco
Copy link
Contributor Author

chameco commented Mar 18, 2022

A complication: on the previously pinned commit, all of the BLST proofs still ran in a reasonable amount of time. Since then, the total runtime for the BLST proofs has significantly increased. Parallelizing only helps so much here; the blst-verification repo's CI breaks things up into a few parallel jobs, the longest of which takes more than 8 hours. While information about breakages or performance regressions in these proofs might be helpful, a multi-hour wait is too unwieldy for SAW's CI.

For now, I'm going to enable only the memory safety proofs and the validation of Cryptol specifications. This should hopefully bring the runtime down to a more reasonable duration. In the future, we should consider running the full proofs periodically in a way that doesn't block development (maybe on Fryingpan?) so that we are aware if things break.

Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Things LGTM (after getting the relevant commits pushed upstream, of course). Do open an issue about running the full BLST proofs in a nightly job of some kind, however, so that we don't forget.

@RyanGlScott RyanGlScott added the tooling: CI Issues involving CI/CD scripts or processes label Mar 23, 2022
@chameco chameco marked this pull request as ready for review March 23, 2022 15:52
@chameco chameco merged commit 5115afb into master Mar 23, 2022
@mergify mergify bot deleted the sb/update-blst branch March 23, 2022 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling: CI Issues involving CI/CD scripts or processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants