Re-work eddsa in the stdlib for hasher trait when turbo fish is implemented #4514
Labels
Comments
guipublic
changed the title
|
It's worth mentioning with that function signature we'd have no way of specifying what the hasher should be. Since we don't have the turbofish operator yet. Edit: This was mentioned in the PR as well #4440 |
This was referenced May 16, 2024
github-merge-queue bot
pushed a commit
that referenced
this issue
May 21, 2024
#5041) # Description ## Problem\* Resolves <!-- Link to GitHub Issue --> Not sure if there is another issue for this I will have to look again, but I found this bug when trying to implement #4514 ## Summary\* When type checking a `MethodCall` from the HIR now we also add a mutable reference to the object type when we have a `HirMethodReference::TraitMethodId`. We were previously only doing this for a `HirMethodReference::FuncId`. For example that meant this program would fail with the following errors laid out in the comments: ```rust fn hash_simple_array<H>(input: [Field; 2]) -> Field where H: Hasher + Default { // Check that we can a trait method to instead a trait implementation let mut hasher: H = H::default(); // Regression that the object is converted to a mutable reference type `&mut _`. // Otherwise will see `Expected type &mut _, found type H`. // Then we need to make sure to also auto dereference later in the type checking process // when searching for a matching impl or else we will get `No matching impl found for `&mut H: Hasher` hasher.write(input[0]); hasher.write(input[1]); hasher.finish() } ``` I also had to add auto dereferencing when verifying the trait constraints later inside of `type_check_func`. So first we add a mutable reference to the method call's object type to avoid a mismatched type error, and then we later dereference it to find the appropriate trait implementation. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Jake Fecher <jake@aztecprotocol.com> Co-authored-by: Jake Fecher <jfecher11@gmail.com> Co-authored-by: Tom French <tom@tomfren.ch>
github-merge-queue bot
pushed a commit
that referenced
this issue
May 21, 2024
# Description ## Problem\* Resolves <!-- Link to GitHub Issue --> Working towards #4514 and fully resolving #4710 ## Summary\* We need to accurately monomorphize functions with turbofish operators where no function parameters or the return type using the specified generic. Without this change the following would pass: ``` fn main(x: Field, y: pub Field) { let mut hasher = PoseidonHasher::default(); hasher.write(x); hasher.write(y); let poseidon_expected_hash = hasher.finish(); assert(hash_simple_array::<PoseidonHasher>([x, y]) == poseidon_expected_hash); let mut hasher = Poseidon2Hasher::default(); hasher.write(x); hasher.write(y); let poseidon2_expected_hash = hasher.finish(); assert(hash_simple_array::<Poseidon2Hasher>([x, y]) == poseidon_expected_hash); assert(hash_simple_array::<Poseidon2Hasher>([x, y]) != poseidon2_expected_hash); } fn hash_simple_array<H>(input: [Field; 2]) -> Field where H: Hasher + Default { let mut hasher: H = H::default(); hasher.write(input[0]); hasher.write(input[1]); hasher.finish() } ``` Essentially any future invocations of `hash_simple_array` would use the `PoseidonHasher`. We now correctly monomorphize the functions to use the correct type. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Jake Fecher <jake@aztecprotocol.com> Co-authored-by: Jake Fecher <jfecher11@gmail.com> Co-authored-by: Tom French <tom@tomfren.ch>
github-merge-queue bot
pushed a commit
that referenced
this issue
May 21, 2024
# Description ## Problem\* Resolves #4514 ## Summary\* This updates `eddsa_verify_with_hasher` to have the following signature now that we support the turbofish operator: ```rust pub fn eddsa_verify_with_hasher<H>( pub_key_x: Field, pub_key_y: Field, signature_s: Field, signature_r8_x: Field, signature_r8_y: Field, message: Field ) -> bool where H: Hasher + Default { [function body ...] } ``` This re-work was only possible with further bug fixes from #5049 and #5041. The original turbofish PR can be found here (#3542). ## Additional Context This PR can most likely be merged into its parent #5049 but I just made this separate PR to separate the bug fix and the stdlib library breaking change. ## Documentation\* Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Jake Fecher <jake@aztecprotocol.com> Co-authored-by: Jake Fecher <jfecher11@gmail.com> Co-authored-by: Tom French <tom@tomfren.ch>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
eddsa_verify_with_hasher (from PR #4440) is taking a hasher as parameter. However the hasher can have some pre-seeded state which will muck with the verification algorithm.
Happy Case
We should rather instantiate a new hasher inside the function:
and then we can rename it to:
eddsa_verify<H>Project Impact
None
Impact Context
No response
Workaround
None
Workaround Description
No response
Additional Context
No response
Would you like to submit a PR for this Issue?
None
Support Needs
No response
The text was updated successfully, but these errors were encountered: