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 sha3 primitive #817

Merged
merged 3 commits into from Aug 29, 2023
Merged

feat: add sha3 primitive #817

merged 3 commits into from Aug 29, 2023

Conversation

NikitaMasych
Copy link
Contributor

@NikitaMasych NikitaMasych commented Aug 23, 2023

This PR is intended to bring SHA3 into the gnarks std.
Mainly, it implements sponge construction over Keccak f-[1600] permutation and adds the following schemes:

  • SHA3-256
  • SHA3-384
  • SHA3-512
  • Keccak-256
  • Keccak-512

Futhermore, it modifies existing keccakf implementation to accept and return [25]uints.U64 directly without convertion from/to [25]frontend.Variable.

There is also testing attached.

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2023

CLA assistant check
All committers have signed the CLA.

@ivokub
Copy link
Collaborator

ivokub commented Aug 28, 2023

Suggested edit:

diff --git a/std/hash/sha3/sha3_test.go b/std/hash/sha3/sha3_test.go
index a8061d11..03367465 100644
--- a/std/hash/sha3/sha3_test.go
+++ b/std/hash/sha3/sha3_test.go
@@ -1,6 +1,8 @@
 package sha3
 
 import (
+	"crypto/rand"
+	"fmt"
 	"hash"
 	"testing"
 
@@ -25,15 +27,19 @@ var testCases = map[string]testCase{
 	"Keccak-512": {NewLegacyKeccak512, sha3.NewLegacyKeccak512},
 }
 
-var currentHash func(api frontend.API) (zkhash.BinaryHasher, error)
-
 type sha3Circuit struct {
 	In       []uints.U8
 	Expected []uints.U8
+
+	hasher string
 }
 
 func (c *sha3Circuit) Define(api frontend.API) error {
-	h, err := currentHash(api)
+	newHasher, ok := testCases[c.hasher]
+	if !ok {
+		return fmt.Errorf("hash function unknown: %s", c.hasher)
+	}
+	h, err := newHasher.zk(api)
 	if err != nil {
 		return err
 	}
@@ -52,27 +58,33 @@ func (c *sha3Circuit) Define(api frontend.API) error {
 }
 
 func TestSHA3(t *testing.T) {
+	assert := test.NewAssert(t)
 	in := make([]byte, 310)
+	_, err := rand.Reader.Read(in)
+	assert.NoError(err)
 
-	for name, strategy := range testCases {
-		h := strategy.native()
-		h.Write(in)
-		expected := h.Sum(nil)
-
-		circuit := &sha3Circuit{
-			In:       make([]uints.U8, len(in)),
-			Expected: make([]uints.U8, len(expected)),
-		}
+	for name := range testCases {
+		assert.Run(func(assert *test.Assert) {
+			name := name
+			strategy := testCases[name]
+			h := strategy.native()
+			h.Write(in)
+			expected := h.Sum(nil)
 
-		witness := &sha3Circuit{
-			In:       uints.NewU8Array(in),
-			Expected: uints.NewU8Array(expected),
-		}
+			circuit := &sha3Circuit{
+				In:       make([]uints.U8, len(in)),
+				Expected: make([]uints.U8, len(expected)),
+				hasher:   name,
+			}
 
-		currentHash = strategy.zk
+			witness := &sha3Circuit{
+				In:       uints.NewU8Array(in),
+				Expected: uints.NewU8Array(expected),
+			}
 
-		if err := test.IsSolved(circuit, witness, ecc.BN254.ScalarField()); err != nil {
-			t.Fatalf("%s: %s", name, err)
-		}
+			if err := test.IsSolved(circuit, witness, ecc.BN254.ScalarField()); err != nil {
+				t.Fatalf("%s: %s", name, err)
+			}
+		}, name)
 	}
 }

Copy link
Collaborator

@ivokub ivokub 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 to me. Thanks for the contribution.
Not necessary for this PR, but maybe you have ideas how to implement std/hash.BinaryFixedLengthHasher interface?

I suggested some edits to remove the global variable in the test cases so that we can run the tests in parallel and then added different SHA instances as a subtest.

I also checked against the full solver and it works well. I didn't run against the full prover right now.

@NikitaMasych
Copy link
Contributor Author

NikitaMasych commented Aug 28, 2023

Applied your suggestion!

Straightforward approach for std/hash.BinaryFixedLengthHasher would be to trunc input array, and send it for padding, but for that it is necessary to convert argument length frontend.Variable to int; I tried to use api.Compiler().ConstantValue(length) for this purpose, but it doesn't seem to work (I am definitely missing something here). #809 confirms that it is not possible. For now I have no other ideas of resolution.

I am interested in possible applications of the hash.BinaryFixedLengthHasher, can you provide me some examples where having such a method might be useful?

@ivokub
Copy link
Collaborator

ivokub commented Aug 29, 2023

Thanks for the update. I'll wait for the CI and merge. Really appreciate your contribution.

I am interested in possible applications of the hash.BinaryFixedLengthHasher, can you provide me some examples where having such a method might be useful?

The sizes of the circuits are fixed, so we have to allocate sufficient number of variables to be able to fit maximal possible input size. This also means that for SHA3 (when implementing hash.BinaryHasher) we only allow inputs of fixed length if we want to prove the preimage-knowledge of the digest. However, it may happen that the preimage size is not known at compile-time or we may have different inputs with different input sizes. This happens for example in zkEVM where if we want to support precompiles and keccak opcode, we have to allow inputs of differents size.

@ivokub ivokub merged commit 2a6e749 into Consensys:master Aug 29, 2023
5 checks passed
@liyue201
Copy link
Contributor

Looks good to me. Thanks for the contribution. Not necessary for this PR, but maybe you have ideas how to implement std/hash.BinaryFixedLengthHasher interface?

I suggested some edits to remove the global variable in the test cases so that we can run the tests in parallel and then added different SHA instances as a subtest.

I also checked against the full solver and it works well. I didn't run against the full prover right now.

I have implemented this interface at #821, please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants