Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SnarkyJS SHA/Keccak #9
base: main
Are you sure you want to change the base?
SnarkyJS SHA/Keccak #9
Changes from all commits
284d681
f946534
dbcb7be
7a95911
14665bf
3955e5c
949fdeb
1d038d9
287a513
a30e4c7
3595a78
5e07de6
9d78394
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can you update this with the most object too?
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.
What exactly do you mean by "the most object"?
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.
Can we remove this section if there will be separate RFC?
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.
Yes! It will be removed with the other PR, which is stacked on top of this PR #14
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.
Test plan and functional requirements
andSHA3/Keccak
sections are of the same level. Later one probably should be a sub-section (###).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.
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.
Good question - @querolita to what extend has the original crypto implementation be tested? Do you have any recommendations to what we should test, if at all?
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.
The tests performed to the implementation were:
I would create similar tests for SnarkyJS, but probably more high level since you don't have access to low level details such as constraint system creation (that I am aware of). In particular, I would make sure that inputs which are not "correct formatted hex values" (meaning an odd number of hex digits) is not passed to the function. And of course making sure that all combinations actually produce the expected value (pairs of output lengths and padding types), for some random values. I wouldn't go much deeper than that regarding the snarkyjs level alone.
In that sense, we are not testing against collision, just comparing against official outputs of the hash function on those given inputs, nor performance testing (all we know is 1 block of encryption, which is enough for 1080bits of input data, fits in 15k kimchi rows, thus 4 full blocks can be hashed without chunking), we haven't looked for keccak's potential vulnerabilities, and we have not gathered usability info from users other than ourselves.
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.
@shimkiv does this sounds good to you? Is there anything else you would change here?
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.
It will be good to add some implementation example links here.
Can we please be more specific here? What kind of set? Is it standardised set? If not then how we determine the set to be good enough?
Open question
Can we please include links here?
It will be good if we can be more explicit here and at least highlight the major cases we're aware of and that will be included into the testing plan.
If we agree on this then it would be good if we can add listed items into the "out of the scope" testing section with some rationale.