fix: use new implementation of sha256 for array_dynamic_blackbox_input test #4360
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vezenovm
reviewed
Feb 13, 2024
There was a problem hiding this comment.
I noticed that when making main unconstrained while still using #[foreign(sha256)] I also got a root that matches the result here. So the mismatch looks to come from passing a dynamic array to the old std::hash::sha256 in a constrained environment. So #4351 has some deeper bug. I'm not sure whether this test is worth merging as when running this program I never got into the DynamicArray case during flatten.
I'm currently looking into whether this is truly due to the dynamic array being read not being what we expect or something else with the sha blackbox func.
EDIT: I have found the actual bug with flattening a dynamic array and will post a PR
This was referenced Feb 13, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 14, 2024
# Description ## Problem\* Resolves #4356 Supercedes #4360 ## Summary\* An ACIR dynamic array is a pointer to flat memory. We have been treating this flat memory as a list of fields, however, this breaks if we do in fact need accurate numeric type information such as when working black box function inputs. For example for hash inputs we set up the byte array based upon the bit size. This needs to be the correct bit size or else we will get a lot of extra garbage when calling `fetch_nearest_bytes` on a FieldElement. This PR attaches a list of `Vec<NumericType>` to the `AcirDynamicArray` structure. This gives us the expected output result for `sha` then. We probably could restrict the `AcirDynamicArray` to be created only through a constructor where we check that the `value_types` match the supplied len in size. I left it for a follow-up as this is a quick fix but I can do it as part of this PR. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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: TomAFrench <tom@tomfren.ch>
|
Closing in favor of #4364 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Problem*
Resolves #4356
Summary*
Use the new method for sha256, which give a different result than the previous sha256.
This is really strange, there is probably something more going on. I give more information in the issue.
Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.