Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat!: Add variable length keccak opcode #314

Merged
merged 10 commits into from
May 26, 2023
Merged

Conversation

kevaundray
Copy link
Contributor

@kevaundray kevaundray commented May 24, 2023

Related issue(s)

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves #313

Description

Summary of changes

(Describe the changes in this PR. Point out breaking changes if any.)

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

@kevaundray kevaundray changed the title feat!: Add variable length keccak feat!: Add variable length keccak opcode May 24, 2023
@kevaundray kevaundray marked this pull request as ready for review May 25, 2023 16:52
@TomAFrench
Copy link
Member

Do we need to have two variants here? What are the benefits of this compared to just having the variable version and passing in msg.len() for the fixed size keccak?

@kevaundray
Copy link
Contributor Author

Do we need to have two variants here? What are the benefits of this compared to just having the variable version and passing in msg.len() for the fixed size keccak?

They have different implementations on the backend, so if we put the msg.len() parameter on Keccak, then backends will need to implement KeccakVar which is harder to implement than Keccak.

Theres also the fact that regular Keccak is more performant when var_message_size is a constant, I don't know how much more performant it is though

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Ok cool, this looks sensible if we're going down the two opcodes route.

Shall we add a todo to add variable variants of the other hash functions?

@kevaundray
Copy link
Contributor Author

Ok cool, this looks sensible if we're going down the two opcodes route.

Shall we add a todo to add variable variants of the other hash functions?

Yep sure, made #320

@kevaundray kevaundray added this pull request to the merge queue May 26, 2023
Merged via the queue into master with commit 7bfd169 May 26, 2023
@github-actions github-actions bot mentioned this pull request May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Keccak Opcode which allows the message size to vary at prover time
3 participants