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

feat: add script to verify parameter checksums in parameters.json #1251

Merged
merged 2 commits into from
Aug 7, 2020

Conversation

vmx
Copy link
Contributor

@vmx vmx commented Aug 7, 2020

This commit adds a script to verify that a .params file (and the corresponding
.vk file) is part of the parameters.json and has the correct checksums.

I extracted this script from this PR: #1249

This commit adds a script to verify that a `.params` file (and the corresponding
`.vk file`) is part of the `parameters.json` and has the correct checksums.
@cryptonemo
Copy link
Collaborator

Will approve as soon as this is tested well.

Copy link
Collaborator

@cryptonemo cryptonemo left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor things.

#!/bin/sh

# This script verifies that a given `.params` file (and the corresponding
# `.vk` file) is part of `parameters.json` as has the correct digest.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor typo 'as has the' = 'and has the'

# utilities (`basename`, `head`, `grep`) as well as have `jq` and `b2sum`
# installed.
#
# The input a `parameter.json` file and a `.params' file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wording is weird ('The input a')


if [ "${#}" -ne 1 ]; then
echo "Verify that a given .params file (and the corresponding .vk file)"
echo "is part of parameters.json as has the correct digest."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

porcuquine
porcuquine previously approved these changes Aug 7, 2020
Copy link
Collaborator

@porcuquine porcuquine left a comment

Choose a reason for hiding this comment

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

Approved based on review of previous, though wording fixes per @cryptonemo would be worth making.

@cryptonemo
Copy link
Collaborator

@vmx This tested well!

@cryptonemo
Copy link
Collaborator

@vmx I updated this, and also included a small fix that I did manually while verifying. I'd like to merge after CI is complete.

@vmx
Copy link
Contributor Author

vmx commented Aug 7, 2020

@cryptonemo Thanks a lot for taking this on.

@cryptonemo cryptonemo self-requested a review August 7, 2020 18:28
@cryptonemo cryptonemo merged commit feb0862 into master Aug 7, 2020
@cryptonemo cryptonemo deleted the parameters-json-verification branch August 7, 2020 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants