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

Allow traits to declare unconstrained functions #5717

Closed
nventuro opened this issue Aug 13, 2024 · 0 comments · Fixed by #6057
Closed

Allow traits to declare unconstrained functions #5717

nventuro opened this issue Aug 13, 2024 · 0 comments · Fixed by #6057
Labels
enhancement New feature or request

Comments

@nventuro
Copy link
Contributor

Problem

Currently all trait functions must be constrained: adding the unconstrained keyword results in a parser error. It'd be good to also be able to make these functions unconstrained, as there are many scenarios in which we require this property from an interface.

An example is Aztec's note abstraction, which includes the ability to compute a note nullifier. These nullifiers include a secret that is injected by an oracle and then constrained by an external component. More concretely, we take a mutable reference to an array of validation requests that are constrained to later be fulfilled - this deferring exists because said constraining must be done by a different circuit.

However, we also need to have an unconstrained way of computing these notes, in which we essentially skip all of the validation. Due to these complicated interactions with the deferred validation requests, we've not been able to come up with a better abstraction than to simply have a second function that does not perform any constraining - the obvious problem being that we cannot mark this function as unconstrained, even though it is.

This may not be a huge deal today, but with #4442 there'll be an expectation of being unable to accidentally call unconstrained functions from constrained contexts, and lacking the ability to properly tag trait functions as such prevents us from leveraging this feature.

Happy Case

trait Foo {
  fn bar() -> Field;
  unconstrained fn bar_unconstrained() -> Field;
}

And then force implementations of the trait to make bar_unconstrained be indeed unconstrained.

Workaround

None

Workaround Description

No response

Additional Context

No response

Project Impact

None

Blocker Context

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@nventuro nventuro added the enhancement New feature or request label Aug 13, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Aug 13, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 17, 2024
# Description

## Problem

Fixes #5717

## Summary

In a previous PR we started allowing `unconstrained` and `comptime` in
trait impls. But two things remained:
1. Defining an `unconstrained` trait impl method would produce a
warning.
2. We should error if a trait method is unconstrained but the trait impl
method is not, or the other way around.

This PR will also include "unconstrained" in trait imp method stubs in
LSP.

## Additional Context

I initially also errored if there's a mismatch in `comptime`, but that's
probably not good as we have some implementation of `Append` be comptime
and some not. This made me wonder what's the purpose of `comptime` in
functions, if non-comptime functions can also be interpreted.

## 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: jfecher <[email protected]>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Sep 17, 2024
nventuro added a commit to AztecProtocol/aztec-packages that referenced this issue Sep 25, 2024
With noir-lang/noir#5717 closed, we can now
make `compute_nullifier_without_context` `unconstrained`, as it should
have been. This clears the multiple warnings caused by calling
`get_nsk_app` without an `unsafe` block.

I also moved the implementation of `TransparentNote` around, since we
were calling the now unconstrained version. This nicely would've
resulted in a warning about a missing `unsafe` block had I not changed
it 😁

---------

Co-authored-by: Tom French <[email protected]>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Sep 26, 2024
With noir-lang/noir#5717 closed, we can now
make `compute_nullifier_without_context` `unconstrained`, as it should
have been. This clears the multiple warnings caused by calling
`get_nsk_app` without an `unsafe` block.

I also moved the implementation of `TransparentNote` around, since we
were calling the now unconstrained version. This nicely would've
resulted in a warning about a missing `unsafe` block had I not changed
it 😁

---------

Co-authored-by: Tom French <[email protected]>
Rumata888 pushed a commit to AztecProtocol/aztec-packages that referenced this issue Sep 27, 2024
With noir-lang/noir#5717 closed, we can now
make `compute_nullifier_without_context` `unconstrained`, as it should
have been. This clears the multiple warnings caused by calling
`get_nsk_app` without an `unsafe` block.

I also moved the implementation of `TransparentNote` around, since we
were calling the now unconstrained version. This nicely would've
resulted in a warning about a missing `unsafe` block had I not changed
it 😁

---------

Co-authored-by: Tom French <[email protected]>
Rumata888 pushed a commit to AztecProtocol/aztec-packages that referenced this issue Sep 27, 2024
With noir-lang/noir#5717 closed, we can now
make `compute_nullifier_without_context` `unconstrained`, as it should
have been. This clears the multiple warnings caused by calling
`get_nsk_app` without an `unsafe` block.

I also moved the implementation of `TransparentNote` around, since we
were calling the now unconstrained version. This nicely would've
resulted in a warning about a missing `unsafe` block had I not changed
it 😁

---------

Co-authored-by: Tom French <[email protected]>
Rumata888 pushed a commit to AztecProtocol/aztec-packages that referenced this issue Sep 27, 2024
With noir-lang/noir#5717 closed, we can now
make `compute_nullifier_without_context` `unconstrained`, as it should
have been. This clears the multiple warnings caused by calling
`get_nsk_app` without an `unsafe` block.

I also moved the implementation of `TransparentNote` around, since we
were calling the now unconstrained version. This nicely would've
resulted in a warning about a missing `unsafe` block had I not changed
it 😁

---------

Co-authored-by: Tom French <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

1 participant