feat: Solve Keccak opcodes #247
Conversation
| generic_sha3::<Keccak256>(initial_witness, gadget_call) | ||
| } | ||
|
|
||
| fn generic_sha3<D: sha3::Digest>( |
There was a problem hiding this comment.
This is the same Digest trait as used by generic_hash_256 so we don't need a duplicate implementation ( you may need to adjust versions of dependencies)
There was a problem hiding this comment.
It is not the same because the two crates do not use the same version.
There was a problem hiding this comment.
Indeed, you'll need to adjust the dependency version for the blake2 crate to match.
| let status = solve_black_box_function(initial_witness, bb_func); | ||
| if matches!(status, Err(OpcodeResolutionError::OpcodeNotSolvable(_))) { | ||
| self.solve_black_box_function_call(initial_witness, bb_func) | ||
| } else { | ||
| status | ||
| } |
There was a problem hiding this comment.
This change is unrelated to supporting keccak opcodes and has significant knock on effects for all backends so shouldn't be included in this PR.
This change is breaking.
There was a problem hiding this comment.
I don't think it is a breaking change, what is broken?
There was a problem hiding this comment.
It's changing the contract between backends and ACVM.
Previously ACVM provided reference methods for executing black box functions to the backends however backends were in charge of solving the black box function and returning the result. After this change this is still the case for all other opcodes but we'll never send keccak256 in particular to the backend.
This split between keccak256 and all the other functions doesn't make sense and we shouldn't split responsibility for black box function execution between the backend and the ACVM. Until we have a rust implementation of all black box functions then we should allow the backend to handle this.
There was a problem hiding this comment.
So in fact this is not breaking anything.
Solving Keccak in ACVM does make sense, as well as all other 'blackbox'. I will move the solving into ACVM for all but pedersen, which will still be handled by the backend until we have a rust implementation.
There was a problem hiding this comment.
This change would mean that a lot of code in aztec_backend would become dead code without us actually realizing it. The backend has a match on all black box functions but those code paths would never be executed.
This could lead to confusing bugs between Noir and the backend and it seems hard to communicate to backend implementors.
I'm torn on where this should happen because I don't want aztec_backend to implement keccak itself 😬 I'd probably be fine making a separate PR for this change, just so we can mark it breaking and have a clear CHANGELOG entry. @TomAFrench thoughts?
There was a problem hiding this comment.
Of course we would remove the dead code as long as it is integrated in ACVM, so I don't see this as an issue.
There was a problem hiding this comment.
Of course we would remove the dead code as long as it is integrated in ACVM, so I don't see this as an issue.
This assumes that we are in control of all the backends, but we need to develop as though there are other backends that need to integrate ACVM and they will be confused without accurate documentation & changelog entries.
There was a problem hiding this comment.
We could land something like #257 before this and then it wouldn't be an issue.
|
I'm not a fan of PRs that don't say what they do in the title, since it makes it harder to find where changes happened and why and it also invites feature creap into PRs. Lets just add Keccak into this PR exactly like how it is done for other functions, then any other changes can be done in a separate PR and discussed. |
Related issue(s)
Related to (noir-lang/noir#1189)
Description
Summary of changes
Solve Keccak opcodes
Checklist
cargo fmtwith default settings.