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: Aztec prelude #4496

Closed
wants to merge 48 commits into from
Closed

feat: Aztec prelude #4496

wants to merge 48 commits into from

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Feb 7, 2024

Summary

Resolves #2837

Adds an aztec prelude similar to the stdlib prelude in Noir or Rust. Now when contracts have an aztec dependency developers do not need to directly import the types specified in the prelude, they are just automatically imported by any contract using aztec-nr. A developer can still import the types if they like but I think it is nice that a developer no longer needs to import types such as AztecAddress, state vars, etc., and can instead just start working with the aztec dep directly.

Changes

The MacroProcessor in Noir operates over an untyped AST before name resolution or def collection, and a typed AST that is processed after name resolution and before type checking.

Attempting to make changes to the untyped AST before collection of definitions means we only have the AST of the root file in our crate and are missing the definitions in lower modules of the crate. This PR lets us add definitions to the definition collector for a crate as part of Noir compilation before name resolution.

This PR focuses only on adding an aztec prelude and is only adding to the collected imports. However, this process can be expanded to support injecting other objects into the definition collector. I wanted to start with a simple example and an aztec prelude seemed like a good starting point.

EDIT: I decided to add a prelude.nr into aztec-nr so it is not hidden from those working on aztec-nr. The macro now simply checks whether we have a certain dependency and returns an extra preludes we want to inject using Noir's already preexisting process for adding preludes. Due to this I had to keep the old strategy for imported use dep::aztec; into every contract for the aztec macros. This could perhaps be improved to use a different name so that we can just use prelude.nr exclusively, but I felt this would be better left for a separate PR. cc'ing @Maddiaa0

Another goal of this PR is to also provide a simple example of how to re-use Noir's parser to create the AST types we would like to insert instead of having a bunch of helpers such as call(..) like we currently do in aztec_macros/lib.rs. Hopefully switching to this strategy will also speed up macro development. @nventuro is building on this with his work to remove compute_note_hash_and_nullifier.

For now I just removed the imports from the noir-contracts but this could be extended to anywhere we see dep::aztec.

Additional Context

It would be nice to automatically include the aztec dependency for Aztec contracts, however, this would touch the noirc driver rather than simply the noirc frontend and would require a different process for enabling additional std dependencies.

@nventuro nventuro mentioned this pull request Feb 8, 2024
@AztecBot
Copy link
Collaborator

AztecBot commented Feb 8, 2024

Benchmark results

Metrics with a significant change:

  • batch_insert_into_append_only_tree_16_depth_ms (8): 16.9 (+34%)
  • batch_insert_into_append_only_tree_16_depth_hash_ms (8): 0.717 (+35%)
Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit 8619c084 and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric 8 txs 32 txs 64 txs
l1_rollup_calldata_size_in_bytes 5,700 18,884 36,452
l1_rollup_calldata_gas 66,132 238,976 469,676
l1_rollup_execution_gas 194,080 500,282 908,934
l2_block_processing_time_in_ms 1,198 (-3%) 4,520 (-2%) 9,015 (-1%)
note_successful_decrypting_time_in_ms 202 (-1%) 529 (-6%) 1,023 (-2%)
note_trial_decrypting_time_in_ms 9.91 (-34%) 59.2 (-37%) 61.7 (-5%)
l2_block_building_time_in_ms 16,142 (-1%) 64,122 128,546
l2_block_rollup_simulation_time_in_ms 12,245 (-1%) 48,725 97,683
l2_block_public_tx_process_time_in_ms 3,866 (-1%) 15,321 30,689 (+1%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric 5 blocks 10 blocks
node_history_sync_time_in_ms 14,136 (-5%) 27,643
note_history_successful_decrypting_time_in_ms 1,241 (-1%) 2,395 (-1%)
note_history_trial_decrypting_time_in_ms 95.4 (-2%) 135 (-13%)
node_database_size_in_bytes 18,796,624 35,475,536
pxe_database_size_in_bytes 29,923 59,478

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit circuit_simulation_time_in_ms circuit_input_size_in_bytes circuit_output_size_in_bytes
private-kernel-init 249 44,736 28,001
private-kernel-ordering 178 (-1%) 52,625 14,627
base-rollup 1,297 177,932 933
root-rollup 71.3 4,192 825
private-kernel-inner 316 73,715 28,001
public-kernel-app-logic 193 (+1%) 32,254 25,379
merge-rollup 5.78 (-1%) 2,712 933

Tree insertion stats

The duration to insert a fixed batch of leaves into each tree type.

Metric 1 leaves 2 leaves 8 leaves 16 leaves 32 leaves 64 leaves 128 leaves 512 leaves 1024 leaves 2048 leaves 4096 leaves
batch_insert_into_append_only_tree_16_depth_ms 9.88 (-1%) 10.9 (-1%) ⚠️ 16.9 (+34%) 16.1 (-4%) 22.0 (-13%) 35.5 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_count 16.9 17.5 23.0 31.6 47.0 79.0 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_ms 0.573 0.603 ⚠️ 0.717 (+35%) 0.497 (-3%) 0.461 (-12%) 0.442 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_32_depth_ms N/A N/A N/A N/A N/A 45.9 72.2 (-2%) 230 (-3%) 442 (-1%) 867 (-1%) 1,723 (-1%)
batch_insert_into_append_only_tree_32_depth_hash_count N/A N/A N/A N/A N/A 96.0 159 543 1,055 2,079 4,127
batch_insert_into_append_only_tree_32_depth_hash_ms N/A N/A N/A N/A N/A 0.470 0.445 (-2%) 0.420 (-2%) 0.414 (-1%) 0.412 (-1%) 0.412
batch_insert_into_indexed_tree_20_depth_ms N/A N/A N/A N/A N/A 54.0 (-1%) 108 (-1%) 337 (-3%) 656 (-1%) 1,318 2,624
batch_insert_into_indexed_tree_20_depth_hash_count N/A N/A N/A N/A N/A 104 207 691 1,363 2,707 5,395
batch_insert_into_indexed_tree_20_depth_hash_ms N/A N/A N/A N/A N/A 0.479 0.485 (-1%) 0.455 (-2%) 0.452 (-1%) 0.456 0.454
batch_insert_into_indexed_tree_40_depth_ms N/A N/A N/A N/A 61.6 N/A N/A N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_count N/A N/A N/A N/A 109 N/A N/A N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_ms N/A N/A N/A N/A 0.538 N/A N/A N/A N/A N/A N/A

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric 0 deployed contracts
tx_size_in_bytes 19,179

Transaction processing duration by data writes.

Metric 0 new note hashes 1 new note hashes
tx_pxe_processing_time_ms 2,601 (-1%) 1,364
Metric 0 public data writes 1 public data writes
tx_sequencer_processing_time_ms 0.0344 (+2%) 471

@vezenovm vezenovm marked this pull request as ready for review February 8, 2024 17:17
noir/aztec_macros/src/lib.rs Outdated Show resolved Hide resolved
noir/aztec_macros/src/lib.rs Outdated Show resolved Hide resolved
noir/compiler/noirc_frontend/src/hir/def_map/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

Over all an extremely nice example of this! If we have some stuff colliding with function names, i think it may be worth not including them in the prelude to prevent user frustration.

noir/aztec_macros/src/lib.rs Outdated Show resolved Hide resolved
yarn-project/aztec-nr/value-note/src/utils.nr Outdated Show resolved Hide resolved
Copy link
Contributor

@rahul-kothari rahul-kothari left a comment

Choose a reason for hiding this comment

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

This is great work!
Would it be at all possible to include everything with a aztec::? e.g. aztec::get_portal_address()? This way we do not pollute keywords that devs might not know about?

curious to hear @iAmMichaelConnor opinion as well

Copy link
Contributor

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@benesjan benesjan enabled auto-merge (squash) February 29, 2024 12:14
@AztecBot
Copy link
Collaborator

AztecBot commented Feb 29, 2024

Docs Preview

Hey there! 👋 You can check your preview at https://65e5e9bc5761583e3cf60081--aztec-docs-dev.netlify.app

@benesjan benesjan disabled auto-merge February 29, 2024 12:31
@benesjan benesjan requested a review from TomAFrench February 29, 2024 12:31
@TomAFrench
Copy link
Member

I'm concerned that this adds a little too much "magic" to how we use these types as well as adding a lot of potential for conflicts in future. If every macro processor can add their own preludes then it's almost guaranteed that they're going to clash, especially with types as general as Map.

In general, I'm not a fan of importing a large number of types/functions from a library just because we're depending on it. Instead this should be opt in, if we look at preludes from Rust libraries for instance we'd need an use aztec::prelude::* import to make use of these types.

It's important to note that while they're core types to the aztec contract writing experience, from Noir's perspective aztec-nr is fundamentally just another library.

@TomAFrench
Copy link
Member

I would prefer that for now we just keep the prelude file which would simplify users performing the imports themselves (as they don't need to remember exactly where in aztec-nr their import is defined).

Alternatively, in contracts decorated with #[aztec] we could inject an import of the prelude (although this may be confusing on why the prelude exists in the contract but not outside of it.

@benesjan
Copy link
Contributor

benesjan commented Mar 1, 2024

If every macro processor can add their own preludes then it's almost guaranteed that they're going to clash, especially with types as general as Map.

@TomAFrench I don't know what is the best solution implementation-wise here but not including the PR and putting the burden of importing these basic types on aztec contract devs doesn't sound good to me. But if you feel like at this point this would cause more issues then benefits I am ok with not merging it now. But I think once things are more stable we should have something like this (at least for types like AztecAddress).

if we look at preludes from Rust libraries for instance we'd need an use aztec::prelude::* import to make use of these types.

@TomAFrench In Solidity you can just use types like address directly and I feel like solidity devs are the core audience here. Looking at a contract as a non-rust dev and seeing that I have to import some prelude thing I would have no idea what's going on. But if you feel that some of the types there are too generic I would not mind removing them.

It's important to note that while they're core types to the aztec contract writing experience, from Noir's perspective aztec-nr is fundamentally just another library.

@TomAFrench Why does this matter when the aztec macros are only used for aztec contracts?

@TomAFrench
Copy link
Member

In Solidity you can just use types like address directly and I feel like solidity devs are the core audience here. Looking at a contract as a non-rust dev and seeing that I have to import some prelude thing I would have no idea what's going on. But if you feel that some of the types there are too generic I would not mind removing them.

I don't think that Solidity is the right comparison here as the language and the smart contract development framework are one and the same - there's no non smart contract solidity development. A better comparison in my mind would be Arbitrum Stylus or Solana as they're both using a general purpose langauge with a smart contract framework built on top, in both of these cases it's necessary to import from a library to get smart contract types, etc.

Why does this matter when the aztec macros are only used for aztec contracts?

Because the ultimate aim is to have the macro logic expressed in Noir source code such that the aztec macros can be included in the aztec-nr crate. At this point then any crate can implement similar logic to aztec-nr so if aztec-nr can arbitrarily add imports then so can every other crate you depend on.

@vezenovm
Copy link
Contributor Author

vezenovm commented Mar 1, 2024

especially with types as general as Map.

We can remove these general names if we would like or just use an alias such as AztecMap.

In general, I'm not a fan of importing a large number of types/functions from a library just because we're depending on it. Instead this should be opt in, if we look at preludes from Rust libraries for instance we'd need an use aztec::prelude::* import to make use of these types.

It's important to note that while they're core types to the aztec contract writing experience, from Noir's perspective aztec-nr is fundamentally just another library.

I agree! However, these statements can be applied to the entirety of aztec_macros not just the prelude itself. The macros package already injects imports and code based upon those imports because we are dependent on aztec-nr. This is what the initial macro #[aztec(private)] does. The prelude isn't adding new functionality in this sense, but rather extending what already exists.

I'm ok with making this opt-in but its feels like it would actually add confusion for users with some of the new macros the aztec team is working to add. For example here #4610 the NoteInterface trait and note::utils::compute_note_hash_and_nullifier essentially became required names. I find it more confusing that someone would have to import a function that is not written explicitly anywhere in the contract.

in both of these cases it's necessary to import from a library to get smart contract types, etc.

We can improve this UX though. We should maintain the boundary of library and language, but we don't yet have full macros on the Noir side that can enable this.

Because the ultimate aim is to have the macro logic expressed in Noir source code such that the aztec macros can be included in the aztec-nr crate. At this point then any crate can implement similar logic to aztec-nr so if aztec-nr can arbitrarily add imports then so can every other crate you depend on.

Yes absolutely. But we are not at this point yet, and we do not have any timeline for it right now. And with new macros potentially using reserved names, aztec-nr should have a prelude as mentioned above. This feels like a more general question of whether we should commit fully to Noir macros and pause development on the aztec_macros package.

@vezenovm
Copy link
Contributor Author

vezenovm commented Mar 1, 2024

@TomAFrench Most of these comments feel like reasons as to why we should move to get rid of aztec_macros itself, but not whether a prelude makes sense for aztec-nr.

We should set aside a discussion next week on the future of aztec_macros as well as get some context on future macros the aztec team plans to add. But none of this feels blocking for this PR.

* master: (39 commits)
  fix: Remove the `VerificationKey` from `ProverInstance` (#4908)
  chore(avm-transpiler): minor rust fixes (#4889)
  add some migration notes (#4913)
  fix: depreciated ci image (#4911)
  feat: Public initializer check (#4894)
  feat: Use yarns topological build to get rid of explicit sequential steps, and let it solve. (#4868)
  git subrepo push --branch=master barretenberg
  fix: Temporarily skip failing deployment test
  feat: Check initializer by default in private functions (#4832)
  feat: Allow nullifier proofs in public (#4892)
  refactor: Move remaining data out of Honk UltraComposer (#4848)
  chore: Do not download foundry during L1 contracts fast bootstrap (#4865)
  chore: Add custom inspect for base types (#4890)
  refactor: rename read request to note hash read request (#4888)
  feat(avm-simulator): implement NOTEHASHEXISTS (#4882)
  Fix noir mirror path.
  feat: public refunds via FPC (#4750)
  chore: purge SafeU120 (#4819)
  refactor: replacing use of `L2Tx` with `TxEffect` (#4876)
  fix: Fetch Headers and Bodies separately #4167 (#4632)
  ...
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this pull request 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.
@vezenovm
Copy link
Contributor Author

This was succeeded by #4929 so i am closing.

@vezenovm vezenovm closed this Mar 26, 2024
superstar0402 added a commit to superstar0402/aztec-nr that referenced this pull request 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.
@ludamad ludamad deleted the mv/aztec-prelude branch August 22, 2024 15:19
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 this pull request may close these issues.

feat(AztecNr): Hidden prelude
9 participants