refactor(AztecMacro): Correctly determine the maximum serialized note length #4649
Comments
|
Note that we're making the array be as large as |
github-merge-queue bot
pushed a commit
that referenced
this issue
Feb 19, 2024
Closes #2918. This adds a new macro function that processes the unresolved trait impls and injects a new function before name resolution takes place. This lets us leverage the parser and write the function in Noir instead of having to deal with more complicated processed objects. In order to find all of the note types we look for impls of the `NoteInterface` trait. This is a bit more involved than it seems, since other crates may also have structs that impl this trait, and those will have already been processed. We rely on the fact that the contract crate is processed last, and search for external crate impls via the NodeInterner and internal ones in the unresolved functions. This method is robust as long as we do the contract crate after all of its imports, which holds because the imports are recursively collected from the root crate. I also added a small escape hatch mechanism by skipping any code generation if the function is already defined by the user. This could use some polishing since it is possible for the user to e.g. provide the function but get the arity or parameter types wrong, in which case they'd get a duplicate definition error. Diagnosis and error messages can be improved here (#4647), but I think a simple mechanism is sufficient for now. --- ### Minor issues - One of the function arguments is a fixed-size array, which should be as big as the largest note length. We are not yet pulling the note length (#4649), so I'm passing a hardcoded value for now. 12 Fields ought to be enough for anyone. - Doing this introduces an implicit dependency on `AztecAddress` on all contracts. This won't be an issue once #4496 is in, but I did have to manually add it to some of the account contracts for now. - I created a new macro function that is called on each crate after collection but before resolution. Due to some internal compilers shenanigans (mostly who holds mut references to what) I chose to specialize this function so that for now it only passes the collected unresolved functions, making it a bit niche for the current use case. @vezenovm and I are planning to generalize this down the road (#4653). - I'm now importing in the macro from some places that were not previously used, notably the HIR and Noir errors. I am not sure if we might want to pull those differently - I simply imported what I needed. - I also introduced some getters to provide mutable access to private fields of the HIR Context and CrateDefMap. This is because we need to modify the contract module in the def map by declaring the new function (which is how we get things like duplicate definition detection). We're already mutating the HIR Context in the macros, so also mutating its members doesn't feel like a stretch. - Finally, I don't know enough about how Noir errors work to know how to produce a useful `Location` value for the new function, if indeed that can be done. Providing an empty span seems to at least not cause weird errors on the LSP plugin, so that's how I left it for now.
Open
LHerskind
changed the title
AztecBot
pushed a commit
to AztecProtocol/aztec-nr
that referenced
this issue
Mar 19, 2024
Closes #2918. This adds a new macro function that processes the unresolved trait impls and injects a new function before name resolution takes place. This lets us leverage the parser and write the function in Noir instead of having to deal with more complicated processed objects. In order to find all of the note types we look for impls of the `NoteInterface` trait. This is a bit more involved than it seems, since other crates may also have structs that impl this trait, and those will have already been processed. We rely on the fact that the contract crate is processed last, and search for external crate impls via the NodeInterner and internal ones in the unresolved functions. This method is robust as long as we do the contract crate after all of its imports, which holds because the imports are recursively collected from the root crate. I also added a small escape hatch mechanism by skipping any code generation if the function is already defined by the user. This could use some polishing since it is possible for the user to e.g. provide the function but get the arity or parameter types wrong, in which case they'd get a duplicate definition error. Diagnosis and error messages can be improved here (AztecProtocol/aztec-packages#4647), but I think a simple mechanism is sufficient for now. --- ### Minor issues - One of the function arguments is a fixed-size array, which should be as big as the largest note length. We are not yet pulling the note length (AztecProtocol/aztec-packages#4649), so I'm passing a hardcoded value for now. 12 Fields ought to be enough for anyone. - Doing this introduces an implicit dependency on `AztecAddress` on all contracts. This won't be an issue once AztecProtocol/aztec-packages#4496 is in, but I did have to manually add it to some of the account contracts for now. - I created a new macro function that is called on each crate after collection but before resolution. Due to some internal compilers shenanigans (mostly who holds mut references to what) I chose to specialize this function so that for now it only passes the collected unresolved functions, making it a bit niche for the current use case. @vezenovm and I are planning to generalize this down the road (AztecProtocol/aztec-packages#4653). - I'm now importing in the macro from some places that were not previously used, notably the HIR and Noir errors. I am not sure if we might want to pull those differently - I simply imported what I needed. - I also introduced some getters to provide mutable access to private fields of the HIR Context and CrateDefMap. This is because we need to modify the contract module in the def map by declaring the new function (which is how we get things like duplicate definition detection). We're already mutating the HIR Context in the macros, so also mutating its members doesn't feel like a stretch. - Finally, I don't know enough about how Noir errors work to know how to produce a useful `Location` value for the new function, if indeed that can be done. Providing an empty span seems to at least not cause weird errors on the LSP plugin, so that's how I left it for now.
|
Feel free to close 5670 if this is a duplicate! |
|
It's not a duplicate - we could have a compiler instead of runtime error until we solve this. |
Thunkar
added a commit
that referenced
this issue
Apr 22, 2024
…NoteProcessor` warnings (#5838) Closes #5669, #5670, #4649 Correctly determines the signature of the autogenerated `compute_note_hash_and_nullifier` by looking up the serialized content size of notes used in the contract. Furthermore, that langth is compared against MAX_NOTE_FIELDS_LEN and a compile time error is emited if the user attempts to use a note that is bigger than currently supported. Finally changed the `NoteProcessor` warns to be errors (even thought this particular one shouldn't get that far anymore!)
AztecBot
added a commit
to noir-lang/noir
that referenced
this issue
Apr 22, 2024
…NoteProcessor` warnings (AztecProtocol/aztec-packages#5838) Closes AztecProtocol/aztec-packages#5669, AztecProtocol/aztec-packages#5670, AztecProtocol/aztec-packages#4649 Correctly determines the signature of the autogenerated `compute_note_hash_and_nullifier` by looking up the serialized content size of notes used in the contract. Furthermore, that langth is compared against MAX_NOTE_FIELDS_LEN and a compile time error is emited if the user attempts to use a note that is bigger than currently supported. Finally changed the `NoteProcessor` warns to be errors (even thought this particular one shouldn't get that far anymore!)
|
Closed in #5838 |
superstar0402
added a commit
to superstar0402/aztec-nr
that referenced
this issue
Aug 16, 2024
Closes #2918. This adds a new macro function that processes the unresolved trait impls and injects a new function before name resolution takes place. This lets us leverage the parser and write the function in Noir instead of having to deal with more complicated processed objects. In order to find all of the note types we look for impls of the `NoteInterface` trait. This is a bit more involved than it seems, since other crates may also have structs that impl this trait, and those will have already been processed. We rely on the fact that the contract crate is processed last, and search for external crate impls via the NodeInterner and internal ones in the unresolved functions. This method is robust as long as we do the contract crate after all of its imports, which holds because the imports are recursively collected from the root crate. I also added a small escape hatch mechanism by skipping any code generation if the function is already defined by the user. This could use some polishing since it is possible for the user to e.g. provide the function but get the arity or parameter types wrong, in which case they'd get a duplicate definition error. Diagnosis and error messages can be improved here (AztecProtocol/aztec-packages#4647), but I think a simple mechanism is sufficient for now. --- ### Minor issues - One of the function arguments is a fixed-size array, which should be as big as the largest note length. We are not yet pulling the note length (AztecProtocol/aztec-packages#4649), so I'm passing a hardcoded value for now. 12 Fields ought to be enough for anyone. - Doing this introduces an implicit dependency on `AztecAddress` on all contracts. This won't be an issue once AztecProtocol/aztec-packages#4496 is in, but I did have to manually add it to some of the account contracts for now. - I created a new macro function that is called on each crate after collection but before resolution. Due to some internal compilers shenanigans (mostly who holds mut references to what) I chose to specialize this function so that for now it only passes the collected unresolved functions, making it a bit niche for the current use case. @vezenovm and I are planning to generalize this down the road (AztecProtocol/aztec-packages#4653). - I'm now importing in the macro from some places that were not previously used, notably the HIR and Noir errors. I am not sure if we might want to pull those differently - I simply imported what I needed. - I also introduced some getters to provide mutable access to private fields of the HIR Context and CrateDefMap. This is because we need to modify the contract module in the def map by declaring the new function (which is how we get things like duplicate definition detection). We're already mutating the HIR Context in the macros, so also mutating its members doesn't feel like a stretch. - Finally, I don't know enough about how Noir errors work to know how to produce a useful `Location` value for the new function, if indeed that can be done. Providing an empty span seems to at least not cause weird errors on the LSP plugin, so that's how I left it for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
#4610 autogenerates the
compute_note_hash_and_nullifierfunction, but passes a hardcoded value for theseralized_notelength. This should instead be automatically determined as the largest length of all of the note types involved.There's a couple ways we could go about this. One option is to try to find the type over which
NoteInterfaceis generic (the note length), but my understanding is that this is done relatively late in the compilation pipeline. Another alternative is to look for the<NAME>_NOTE_LENfield, and use its value (assuming it exists!).The text was updated successfully, but these errors were encountered: