Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add new BrilligFunctionUnsatisfiedConstrain resolution error variant #5403

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

anaPerezGhiglia
Copy link
Contributor

Description

When an assertion fails on Brillig mode, the debugger fails to indicate the user where the program failed

image

while when running on ACIR mode it provides enough information 👇
image

Problem

Resolves

Summary

Context

assert constraints are translated as "jump to trap" if the condition is not met when running in Brillig mode

Changes

  • Add a new variant OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain to represent failed constraints on Brillig
  • Return this new error when brillig code fails due to a Trap failure
  • Return the ownership of the solver to brillig context when the debugger fails due to this new resolution error so that the the last opcode is known for showing the error
  • Changed the variant checking on ACVM.map_brillig_error from OpcodeResolutionError::BrilligFunctionFailed to OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain since assertion errors will now fail with the latter
  • Add new branch on related match expressions to treat the new variant the same way as BrilligFunctionFailed for all other cases
image

Observations

  • should we return the solver ownership to the brillig context in all Err scenarios? At first sight it seems that this would be the right thing to do, but I decided to handle only this known scenario in case I'm missing something.

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 and/or cargo fmt on default settings.

…ariant (#6)

* Convert BrilligSolver Trap failure into `BrilligFunctionUnsatisfiedConstrain` error

* extract extract_failure_payload function to improve readability

* Changed the variant checking on ACVM.map_brillig_error from OpcodeResolutionError::BrilligFunctionFailed to OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain

* return ownership of the solver to brillig context when the debugger fails due to `BrilligFunctionUnsatisfiedConstrain`
Copy link
Contributor

github-actions bot commented Jul 3, 2024

Thank you for your contribution to the Noir language.

Please do not force push to this branch after the Noir team have started review of this PR. Doing so will only delay us merging your PR as we will need to start the review process from scratch.

Thanks for your understanding.

@TomAFrench TomAFrench self-requested a review July 3, 2024 17:20
@jfecher jfecher requested a review from sirasistant July 3, 2024 19:41
@Savio-Sou Savio-Sou added enhancement New feature or request debugger labels Jul 3, 2024
TomAFrench added a commit that referenced this pull request Jul 4, 2024
Co-authored by: Ana Perez Ghiglia <aghiglia@manas.tech>
TomAFrench added a commit that referenced this pull request Jul 4, 2024
Co-authored-by: Ana Perez Ghiglia <aghiglia@manas.tech>
@TomAFrench
Copy link
Member

TomAFrench commented Jul 4, 2024

I would say that replacing the brillig solver in all cases is probably the way to go in this scenario. I think it was probably left off due to an expectation that execution has finished so we can discard the VM.

github-merge-queue bot pushed a commit that referenced this pull request Jul 8, 2024
# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

This PR pulls out a modified version of @anaPerezGhiglia's refactoring
to how we extract error payloads from the brillig VM's memory from #5403
to separate it from the creation of the new error variant.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] 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: Ana Perez Ghiglia <aghiglia@manas.tech>
@TomAFrench
Copy link
Member

I expect that this doesn't require the new error variant now, correct?

@anaPerezGhiglia
Copy link
Contributor Author

@TomAFrench I guess the need of the new variant is debatable. I left the new variant since with it

  • we have a better explanation for the user about what happened. With the new variant the executor shows "Cannot satisfy constraint", while without it shows "Failed to solve brillig function"
  • it feels better to check against a specific constrain error in map_brillig_error function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debugger enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants