feat!: emit nullifiers in set.get_notes#4940
Conversation
Working on #4940 I found that this contract uses `PrivateSet` (aka `Set`), when `PrivateImmutable` would actually be a much better fit. The escrow was reading set notes without nullifying them because it doesn't ever remove notes from the set (and hence all notes that were ever created are always valid). This can be very confusing for a beginner, and was a bad example to showcase. I also removed the docs decorators since we don't ever reference them.
nventuro
changed the title
nventuro
force-pushed
the
nv/aztecnr-get_note
branch
from
77b0146 to
e7e74a4
Compare
|
Currently blocked by noir-lang/noir#4497. |
nventuro
force-pushed
the
nv/aztecnr-get_note
branch
from
46ae7a8 to
5008c26
Compare
|
Blocked by noir-lang/noir#5008. |
|
I thought noir-lang/noir#5008 was a blocker, but it was just a crash instead of the compiler reporting that we're doing something invalid: we can't have mutable references go from constrained into unconstrained. This is a big problem because our state variables hold The way I addressed that here in 4b723d2 was by simply calling the oracle myself instead of using the aztec-nr code, but this is of course not a viable long-term solution. The contract where I had to do this is not a good example of real usage as it mostly serves as an example for how to construct a note inclusion proof. and does not necesarily represent how a real application might use this API. Regardless, it does slow a flaw in the design. A possible path forward might be #5886, which would let us have impls of the state variables that hold |
Docs PreviewHey there! 👋 You can check your preview at https://6650cdba34c06844c2779d03--aztec-docs-dev.netlify.app |
LHerskind
left a comment
LHerskind
left a comment
There was a problem hiding this comment.
It looks good to me, unclear why it is failing ci though 😬
| ); | ||
| } | ||
|
|
||
| // DEPRECATED |
There was a problem hiding this comment.
Should we not get rid of this one if deprecated, why keep it? 👀 better fit for separate issue, just saw it here.
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
4 participants
Closes #4346.
This changes
PrivateSet(previously calledSet) so that it emits nullifiers inget_notes. This is to ensure the notes are active: the old API made it possible to 'get' notes that had been nulllified, which is highly non-obvious to someone who is not an expert at Aztec.Almost all of the callsites emitted nullifiers immediately after using the notes, so these were relatively easy changes. They seemed to rely on a call to
removefailing unless some note had been actually retrieved, so I added someis_someassertions. We may want to expand the note getter API to also provide a minimum number of notes that must be returned: I created #4988 for this. Some callsites also made me think that we want to rename this topop_notes(#4989).Some of the weird callsites are the inclusion proof tests/examples, which don't really need
get_notessince they are already proving inclusion themselves - I adapted them to useview_notesinstead. This meant however that we can't rely on the options (e.g. selects) being enforced, so I had to manually add assertions - this was also not obvious, and the type of issue that noir-lang/noir#4442 would help prevent. My overall impression is that the current API is not good enough for the historical inclusion proof use case: we may want a version ofget_notesthat performs content assertions but does not check existance, or an altogether different inclusion proof API in which we e.g. say 'prove that a note that passes these selects, filters, etc. exists'.The final strange callsite is the escrow contract, which uses the set like a
Singleton(nowPrivateMutable). I addressed this in #4942.Finally,
PrivateSetno longer allows querying for nullified notes (because it attempts to nullify them). We're not using this anywhere, but it also hints at the API not being stellar for the historical inclusion proof usecase.I also removed some doc delimiters that we're not currently using.