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

Migrate aztec-nr state variables to use context traits #5886

Closed
Tracked by #2783
nventuro opened this issue Apr 19, 2024 · 0 comments · Fixed by #6442
Closed
Tracked by #2783

Migrate aztec-nr state variables to use context traits #5886

nventuro opened this issue Apr 19, 2024 · 0 comments · Fixed by #6442
Assignees

Comments

@nventuro
Copy link
Contributor

nventuro commented Apr 19, 2024

Bad Aztec.nr Developer Experience: a Tale of Two Contexts

State variables currently hold a Context object which is passed in the new function, e.g.:

impl<T> PublicImmutable<T> {
    pub fn new(
        context: Context,
        storage_slot: Field
    ) -> Self {

new is in turn called for each state variable by the init function, which the macros autogenerate:

impl Storage {
    fn init(context: Context) -> Self {
        Storage {
            leader: PublicMutable::new(context, 1),
            legendary_card: PrivateMutable::new(context, 3),
            profiles: Map::new(context, 4),

This context object is created on each function as either a PublicContext or a PrivateContext depending on the function type, and then wrapped in the Context type, also by the macros:

let mut context = PrivateContext::new(inputs, args_hasher.hash());
let mut storage = Storage::init(Context::private(&mut context));

When a function invokes a state variable method, the object already holds the context, and can only inspect its contents during function execution (e.g. self.context.private.unwrap()), at runtime. This means that if a state variable function expected to be called in private is instead called in public, the error will go undetected all the way to public execution, until the unwrap() call panics.

This results in terrible developer experience, which is made worse by the slowness of not just the test run but also the entire compile -> transpile -> create typings -> rebuild tests chain. It also means all methods end up with _in_private or _in_public_suffixes.

A Better Way

We can take advantage of how monomorphization works in Noir, and the fact that all contract functions are compiled as standalone circuits. For any given contract function, we'll be working in either a private or public context, and we'll create a state variable object on which no methods that require the other type will be called (i.e. we won't call public context methods on a state variable created via the init -> new chain in a private contract function). Armed with this, we can make calling functions from the wrong domain a compilation error.

Instead of having all state variables hold a concrete Context object, we can make them be generic over a C type, which in practice will be either a private or a public context.

struct PublicImmutable<T, C> {
    context: C,
    storage_slot: Field,
}

impl<T, C> PublicImmutable<T, C> {
    pub fn new(
        context: C,
        storage_slot: Field
    ) -> Self {

This change gets propagated to the init call, which simply means we no longer wrap in Context in the autogenerated function prelude:

let mut context = PrivateContext::new(inputs, args_hasher.hash());  
- let mut storage = Storage::init(Context::private(&mut context));
+ let mut storage = Storage::init(&mut context);

Finally, state variable methods no longer .unwrap() (because there's no wrapper) and simply call functions on the context directly. To do this, they require that the context is of the correct type by requesting that it implements a trait:

- pub fn get_current_value_in_public(self) -> T {
-    let block_number = self.context.public.unwrap().block_number() as u32;
+ pub fn get_current_value_in_public(self) -> T where C: PublicContextInterface {
+    let block_number = self.context.block_number() as u32;

With this, if storage.myVariable.get_current_value_in_public is called in a private contract function, compilation will fail because storage.myVariable will have been specialized for PrivateContext, as that type is the one that was passed to new, and PrivateContext does not implement the PublicContextInterface trait.

✨ happy developer ✨

Note that this means that the Storage struct itself must also be made generic over a C type which is passed to all state variables.

Implementing this change requires minimal changes to the macros, and the only user code change is the new C type in the Storage struct (uneless there's also manual interactions with contexts, in which we may need to remove the .private.unwrap() chains). State variables of course would need updating and adding of where clauses, but this should be fairly simple to do. Finally, we'd need to create the PublicContextInterface and PrivateContextInterface traits, which are not currently a thing (they're currently just structs with an impl block).

@nventuro nventuro self-assigned this May 13, 2024
spalladino pushed a commit that referenced this issue May 21, 2024
Closes #5886.

This removes the `Context` struct and makes all state variables generic
over a new `Context` type parameter, with each state variable providing
implementations for `PrivateContext`, `PublicContext` or `()` (used to
marked `unconstarined` - more on this later).

The end result is that we get compile-time errors when calling functions
that are unavailable in the current context, reduced wrapping and
unwrapping, and no obscure `explicit trap hit in brilling
'self._is_some'` runtime errors, such as in
#3123.

## How

The implementation is prety much as described in #5886, except instead
of using traits we specialize the type directly for the contexts we
know.

```rust
struct MyStateVar<Context> { context: Context }

impl MyStateVar<&mut PrivateContext> { fn private_read() { } }
```

This works because there's only a couple of them, and users are not
expected to extend them.

The macros were altered so that instead of wrapping the context object
in `Context` and then passing that, we simply pass the raw object to the
`Storage::init` function. This means that `Storage` itself is now also
generic, resulting in some unfortunate new boilerplate in the struct
declaration.

All instances of `self.context.private.unwrap()` and friends were
removed: each function is now available under the corresponding `impl`
block that is specialized for the corresponding context. We could even
rename some since `read_public` and `read_private` is no longer
required: both impls can have `read` functions since they are
effectively methods for different types, so the shared name is a
non-issue.

## Oddities

This change revelead a number of small bugs in the codebase, in the form
of uncallable functions. These were undetected since no test called
them. I'll do a pass over the entire PR and leave comments where
relevant.

## Top-level unconstrained

This PR continues the formalization of what I've been calling 'top-level
unconstrained' (i.e. an unconstrained contract function) as a
fundamental Aztec.nr concept and third execution environment, alongside
private and public execution. So far we've been accessing oracles in
these unconstrained functions without much care (sometimes for
perfomance reasons - see
#5911), but the new
stricter compile-time checks force us to be a bit more careful.

In my mind, we are arriving at the following model:
- public execution: done by the sequencer, can be simulated locally with
old data, not unlike the evm
- private execution: able to produce valid private kernel proofs with
side effects collected in the context
- top-level unconstrained execution: local computation using both
private and old public data, with certain restrictions from private exec
lifted (e.g. unbounded loops), unable to produce any kind of proofs or
reason about state changes. only useful for computing values doing
arbitrary computation over both private and public state, with zero
validation and guarantees of correctness

Private execution requires a context object a it needs to collect side
effects, but public notably does not - it simply calls oracles and gets
them to do things. In this sense, the `PublicContext` type is acting as
a marker of the current execution environment in order to prevent
developers from accidentally doing things that are invalid in public,
which would otherwise result in either transpilation error or inability
to create public kernel proofs.

This means that we may want a third `UnconstrainedContext` to act as a
similar marker for this third type (where we can e.g. call `view_notes`,
read old public state, etc.). It currently doesn't exist: we simply have
`Context::none()`, and it is defined as the absense of one of the other
contexts. Because of this, I chose to temporarily use the unit type
(`()`) to mark this environment.

Note that in some cases the different execution environments share code
paths: `view_notes` is simply `get_notes` without any constraints, and
public storage reads are performed by calling the same oracles in both
public and unconstrained. I imagine small differences will arise in the
future, specially as work on the AVM continues.

---------

Co-authored-by: esau <[email protected]>
Co-authored-by: thunkar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant