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.

Sign up for GitHub

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

Jump to bottom

Feat: Builtin Keccak256 #144

Merged
merged 38 commits into from
Nov 9, 2023
Merged

Feat: Builtin Keccak256 #144

merged 38 commits into from
Nov 9, 2023

Conversation

jkktom
Copy link
Contributor

@jkktom jkktom commented Oct 26, 2023 •
edited
Loading

Ready for review : Implementing Keccak256 hashing

[x] Add padding to inputs
[x] Applied keccak CairoKeccak
[x] Verify test result from the web (same inputs, same output)
[x] LE, BE (Little Endian, Big Endian) way of Keccak implementation
[x] Tests for LE, BE inputs.

@jkktom jkktom marked this pull request as ready for review October 30, 2023 13:55
@jkktom
Copy link
Contributor Author

jkktom commented Oct 30, 2023

Dear @rodrigo-pino , This PR is ready for review.
Keccak256 works for suitable inputs with appropriate paddings applied. All tested with success.
All the test inputs in test codes verified from Web result : https://emn178.github.io/online-tools/keccak_256.html
Same inputs, Same results.

Still contains Println and Printf lines for verification and debug. This will be commented out after reveiw.
Thanks.

@jkktom jkktom self-assigned this Oct 30, 2023
@jkktom jkktom added the vm builtin Related with VM Builtins label Oct 30, 2023
@jkktom jkktom changed the title Builtin keccak Feat: Builtin keccak for issue #130 Oct 30, 2023
@jkktom jkktom changed the title Feat: Builtin keccak for issue #130 Feat: Builtin Keccak256 Oct 30, 2023
@jkktom
Copy link
Contributor Author

jkktom commented Oct 30, 2023

Resolves issue #130

@jmjac
Copy link
Contributor

jmjac commented Nov 1, 2023

Spend some time today trying to figure out how this should work and it seems the keccak builtin does the keccak-f1600 permutation to the 8 felt 200bit inputs and writes 8 felt 200bit outputs.

@jmjac jmjac requested a review from rodrigo-pino November 1, 2023 20:44
rodrigo-pino
rodrigo-pino reviewed Nov 6, 2023
View reviewed changes
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

@jkktom @jmjac good work! Just a couple of questions, and remember we need to add an integration test too. Just like we have one for the other builtins.

pkg/vm/builtins/keccak.go Show resolved Hide resolved
pkg/vm/builtins/keccak.go Outdated Show resolved Hide resolved
pkg/vm/builtins/keccak_math.go Outdated Show resolved Hide resolved
pkg/vm/builtins/keccak.go Outdated Show resolved Hide resolved
rodrigo-pino
rodrigo-pino reviewed Nov 8, 2023
View reviewed changes
pkg/vm/builtins/keccak.go Outdated Show resolved Hide resolved
pkg/vm/builtins/keccak_test.go Outdated Show resolved Hide resolved
rodrigo-pino
rodrigo-pino approved these changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

Amazing! Thanks @jkktom and @jmjac

@rodrigo-pino rodrigo-pino merged commit 1915b0f into main Nov 9, 2023
4 checks passed
@rodrigo-pino rodrigo-pino deleted the builtin-keccak branch November 9, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmjac jmjac jmjac left review comments

@rodrigo-pino rodrigo-pino rodrigo-pino approved these changes

Assignees

@jkktom jkktom

Labels
vm builtin Related with VM Builtins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jkktom @jmjac @rodrigo-pino