Conversation
Contributor
Author
|
Closing as this has been resolved on the Noir side. See noir-lang/noir#7289 for more details. |
This file contains hidden or 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
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.
Note: This is purely a debugging PR to display a Noir compilation bug, not a requested change (yet).
I was suspicious that some of the failures of the Noir sync were as a result of this pre-existing issue with labeling (un)constrained runtimes when using closures.
We merged a workaround for the sync noir-lang/noir#9376. However, it still was not fully clear as to why a panic was occurring. We were seeing a type mismatch panic in the Brillig VM. Even if we were accessing an array OOB, in Brillig every array access has preceding OOB checks. I felt there may be a mismatch in the runtime labeling as noir-lang/noir#9200 switched ACIR arrays to not have explicit OOB checks and that was the PR that started triggering the sync failures.
Testing against fix(ssa): Replace unreachable array access with a constrain in Brillig #9378 shows us that we in fact do have mislabeled runtimes. When based off of
nextwe started hitting the following e2e test failures:Details
Hitting an assertion failure in poseidon was especially unexpected. If we have an always failing array access we should not be suddenly hitting a constrain that replaces that access.
This PR simply changed all calls to
remove_constraintsto explicit calls to duplicated unconstrained functions. After doing this, there were no longer unexpected assertion failures and the e2e tests passed.