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

feat!: try out unconstrained wrapper #6618

Closed
wants to merge 1 commit into from
Closed

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented May 22, 2024

This is an exploration of @TomAFrench's suggested alternative approach to noir-lang/noir#4442, namely as mentioned in this comment.

It adds the following small library:

struct Unconstrained<T> {
    _value: T,
}

impl<T> Unconstrained<T> {
    fn new(value: T) -> Self {
        Self { _value: value }
    }

    fn make_constrained<Env>(self, constrainer: fn[Env](T) -> T) -> T {
        constrainer(self._value)
    }
}

With it, we can wrap oracle return values as Unconstrained<T>, and make the constraining step explicit. This nicely detects the bug we currently have in note_getter, where the filter function is called in an unconstrained environment. The new pattern forces us to move this inside the make_constrained closure. Even if it didn't force the move, it'd at least clearly delineate which exactly are the constraints being applied.

Unconstrained work

However, there's some issues. In some cases we perform some unconstrained computation, reducing circuit size. An example is get_note_internal, which reduces the array of Option<Note> to a single concrete Note which is later constrained.

A utility function lets us do this:

impl<T> Unconstrained<T> {
    unconstrained fn transform<Env, T2>(self, transformer: fn[Env](T) -> T2) -> Unconstrained<T2> {
        Unconstrained::new(transformer(self._value))
    }
}

fn get_note_internal(...) -> Note {
    oracle::notes::get_notes(...).transform(
        |opt_notes: [Option<Note>; 1]|
            opt_notes[0].unwrap()
    )
}

Unconstrained API

The above was slightly annoying, but not unreasonable. The real issue is that our API also includes unconstrained functions (e.g. view_notes), which are in turn used by state variables in unconstrained contexts. Doing any kind of computation in these environments would require that we annotate all return values with Unconstrained and use transform whenever we use a value.

Moreover, all of these transform calls are ultimately unnecessary. We absolutely do not care what happens inside unconstrained functions: we only care about crossing the unconstrained <> constrained boundary. But because we cannot know when an unconstrained value will be used, the only way to be sure a constrained caller will get a wrapped value is to make all unconstrained functions return wrapped values and transform them as they go all the call stack.

The only way I can see to solve this situation is with language support: we need something automatic that triggers when crossing domains. This can be an unsafe block, automatic wrapping in Unconstrained when calling from a constrained function or some third alternative, but we cannot detect these problematic callsites in advance given current language tools.

@nventuro nventuro added the S-do-not-merge Status: Do not merge this PR label May 22, 2024
@@ -234,7 +245,7 @@ unconstrained pub fn view_notes<Note, N, M>(
placeholder_opt_notes,
placeholder_fields,
placeholder_note_length
)
).make_constrained(|opt_notes| opt_notes) // <- we're not constraining anything, but we don't want to pollute the API
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only choices we have here (in an unconstrained public API function) are either:

  • to bubble up the Unconstrained value, polluting all callsites and forcing usage of transform (potentially unnecessarily if the value is ultimately not constrained)
  • to unwrap how I did here, losing knowledge of the fact that this value is unconstrained (which in this case is irrelevant because we never call this from constrained functions)

The issue is we currently cannot know if constrained functions will access this value at some point in their call stack, and can't decide which type to return.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not ideal, but what about an .unwrap() function just like in Rust? I think there's value in this approach, and this annoyance can be solved by a function that basically says: "gimme the result, I know what I am doing" as opposed to make_constrained which would be used when you actually do something

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would having an intrinsic within_unconstrained() -> bool available in the language (that gets resolved at compile time) help here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no, it wouldn't, since it's already inside an explicitely unconstrained fn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not ideal, but what about an .unwrap() function just like in Rust? I think there's value in this approach, and this annoyance can be solved by a function that basically says: "gimme the result, I know what I am doing" as opposed to make_constrained which would be used when you actually do something

I meant sth like this in my original comment. Yes it's not great, but maybe better than what we have now? (or maybe not worth it?)

The API taking constrainer will not work in general, I think: e.g., you could have an assert(a - b == c) which constrains 3 things at the same time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, if we combined the intrinsic with the unwrap approach? if you have that intrinsic, in unwrap you can assert that you are within unconstrained

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with unwrap() is that you'd need to re-wrap at the end: because you don't know who might call you, you are forced to wrap your return value in case the caller happens to be constrained. The issue is not transfrom vs unwrap + new, it's the need to a) add an intermediate processing step, and b) (unnecessarily) pollute the return values. If Noir were to support this, all of this code would be removed.

The API taking constrainer will not work in general, I think: e.g., you could have an assert(a - b == c) which constrains 3 things at the same time.

Generally you won't constrain two values from different unconstrained calls: they'll come together (e.g. tree node and sibling path). If you ever end up in this scenario however, you can always have an unconstrained fn that bundles your multiple values into e.g. a tuple so that you can then make_constrained. If you need access to constrained values, you just capture them in the closure.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what about mixing this approach (this is a prototype branch) with the wrapper?

pub fn decompose(x: Field) -> (Field, Field) {

The explicitely unconstrained hints could return the wrapped versions

Copy link
Contributor Author

@nventuro nventuro May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that'd compile? Because you'd have a return type (wrapped or no) that depends on who called you.

What I envision instead is that noir automatically wraps the return value when calling unconstrained from constrained - the same thing that I do here manually. The feature would be that I can't forget to do it.

@AztecProtocol AztecProtocol deleted a comment from AztecBot May 22, 2024
@nventuro
Copy link
Contributor Author

nventuro commented May 22, 2024

There is another learning from this proof of concept that I want to highlight. The original feature request was that we'd have an unsafe { } block in which constraining statements would be added. However, I now think that most of those would end up looking like this:

let foo = unsafe {
  let unsafe_foo = unsafe_fn();
  constrain_foo(foo)
}

because we'd want to be able to write unit tests for constrain_foo - that'd be made much more annoying if its contents were inlined into statements.

Therefore, perhaps it'd be better to go with something were e.g. Noir automatically wraps return values from unconstrained functions at constrained callsites in something like my Unconstrained<T> wrapper with its make_constrained method. I don't know much about the Noir internals, but I do know it detects calls from constrained into unconstrained since it needs to e.g. forbid mutable references from crossing that boundary.

Another point in favor of this approach is that we could begin using it today by manually doing the wrapping at unconstrained callsites: once language support is implemented all we'd need to do is remove the Unconstrained::new calls.

@Thunkar Thunkar mentioned this pull request May 29, 2024
4 tasks
@nventuro
Copy link
Contributor Author

Closing since we're going to follow a different approach.

@nventuro nventuro closed this Jun 27, 2024
@nventuro nventuro deleted the nv/unsafe-test branch June 27, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-do-not-merge Status: Do not merge this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants