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 7 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.
Seems useful. Nonetheless, the gadget already performs all necessary constraints to make sure that the inputs are indeed 8 bits at most. That means it is sound. But if you want to provide the user interface with a much more explicit type for this so they can see if it is ok at compile time, then I believe it is a great idea for user experience.
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.
This is important information! If we had a
UInt8
type, it would actually be preferable to constrain its range as it is introduced into provable code. This is how we usually do it, so that any user ofUInt8
in provable code can rely on the fact that this value can only be 8 bits long. (You might have seen the same done for booleans in snarky).So, could it be a possibility to add a flag to the existing gadgets that disables the 8 bit range check on inputs? @querolita
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.
Great idea @mitschabaude - makes sense.
Note typo:
draw a clan line
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.
Answering your question @mitschabaude, yes, it could be easily added so that you have access to it from SnarkyJS. From a cryptographic point of view, I still have the feeling that I do prefer to leave those constraints in the circuit. Meaning, even if the JS code won't allow you to introduce anything larger than UInt8, it still relies on trusting that the types are correct rather than a mathematical proof that it is indeed the case. But if this is something that SnarkyJS does often, I guess it is fine if you can disable the check in case the user introduces a UInt8 as input.
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.
@querolita I may have been not clear enough -- what I'm proposing is to add constraints which prove that inputs fit in 8 bits. It's just better to add those constraints when a value is introduced, and not inside a specific gadget.
Assume that we have multiple gadgets which operate on UInt8. If every gadget adds constraints to check that the Uint8 is only 8 bits long, then we will add those gates multiple times (if we use multiple of these gadgets).
If, instead, we add the constraints whenever a Uint8 is introduced (with
exists
), and not in each gadget, then we will have those constraints only once.In snarkyjs and in snarky, we already have a way to add constraints to the proof everytime a variable of a certain type is introduced with
exists
. In snarky, everytyp
can have acheck
method which contains those "type-defining" constraints.exists
callstyp.check
:https://github.com/o1-labs/snarky/blob/14f8e2ff981a9c9ea48c94b2cc1d8c161301537b/src/base/checked_runner.ml#L277
and types such as booleans include their defining constraints as part of
typ.check
:https://github.com/o1-labs/snarky/blob/14f8e2ff981a9c9ea48c94b2cc1d8c161301537b/src/base/utils.ml#L278-L281
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.
btw, I completely agree with you @querolita that we can never leave out constraints like this.
even if snarkyjs would throw an error on creating a
UInt8
which doesn't fit in 8 bits, that would not prevent adversarial provers from including such a value in their proof: they can simply create their proof using a modified version of snarkyjs which doesn't throw this error. a modification like this is trivial and doesn't change the constraints, so an adversarial proof like that would be valid against an on-chain verification key, which simply means smart contracts would have a security vulnerabilityThere 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.
Ooooooh!!! Okay I get you now! Nice very nice indeed! Actually we had discussions early in the days of the design of Keccak where we were analysing the possibility to add a "Byte" type, together with existing ones like Boolean or Field. But we thought it might not be super urgent at the Snarky level, but it indeed looks interesting at the SnarkyJS level.
I really like this idea now. And yes, I can create a flag to disable these checks inside the gadget, if they have already been constrained at UInt8 creation time. I will create a subsequent PR with this, together with other improvements that have been suggested along this discussion.
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.
I've spent some time researching and thinking about our approach to the API, and I feel a Hash interface could be a great fit for us.
I took a look into the functionality of various popular JS runtimes like Node.JS, Deno, and Bun, I noticed they offer builder patterns for creating hash functions. While their approach is great, considering the diverse configuration options they have to deal with, our needs aren't as complex (I think). So, perhaps we might not need to follow the builder pattern for our hash functions.
I also looked into how Noir structures its approach: https://noir-lang.org/standard_library/cryptographic_primitives/hashes.
They follow a
std:hash:[hash_name]
style, influenced by Rust, which I found quite appealing. It feels efficient and neat to call the function directly without any additional building process. My vote goes towards an interface-styled approach, happy to hear other opinions, though.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.
Agree with having the different hashes directly!
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.
+1
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.
Just checking... in the snark these hashes come with some circuit constants and it may be more efficient to include those in the circuit once at the start of the circuit. If the "direct hash" API isn't able to facilitate this, then the "builder" approach could have an advantage since in that approach you could add those constraints when building the hash configuration.
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.
@jspada, don't we have a way to include constants only once anyway? "Constant unification"? When you use a constant that already exists in the circuit, it will just be wired to the existing one?
If that's true and we can use it, it seems to me we don't need a special setup command to avoid adding the same constants multiple times. cc @mrmr1993?
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.
If we're considering to utilize the test vectors from organizations like
NIST
(to ensure the correctness of the implementation), then we should specify it.It will be good to also mention what will form the regression testing suite (for example, functional tests, specific edge cases, errors handling, backward compatibility, etc.).
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.
FYI, we have a "regression test suite" in use in snarkyjs CI, which this RFC refers to. it checks that constraints generated using a given list of contracts / provable methods don't change: https://github.com/o1-labs/snarkyjs/blob/main/src/examples/vk_regression.ts
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.
+1 for specific edge cases, errors handling
Since padding is important for security and there are at least two kinds of padding supported by our Keccak gadgets, from a security perspective testing different length messages (e.g. empty, zero, one less than padding lengths, one more, multiples of, etc) are great edge-cases to test imho!
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.
Thanks everyone! I slightly edited this section to reflect your ideas. I am a big fan of adding (maybe not all, but at least some) of the NIST test vectors - what are your thoughts on this? Is this already being done in the crypto side? (didn't explicitly see that)
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.