Conversation
| @@ -1,25 +1,320 @@ | |||
| use aztec::{macros::notes::partial_note, oracle::random::random, prelude::AztecAddress}; | |||
| use dep::aztec::{ | |||
There was a problem hiding this comment.
This file is a copy-paste of uint_note.nr except using field instead of u128. Read that one instead.
| let encrypted_log = default_aes128::note::compute_log( | ||
| *context, | ||
| private_log_content, | ||
| storage_slot, | ||
| recipient, | ||
| sender, | ||
| ); | ||
| context.emit_private_log(encrypted_log); |
There was a problem hiding this comment.
is there a way of not-choosing to use default_aes128 encryption strategy here?
can the fn partial have as input an encryption method?
There was a problem hiding this comment.
As of today, encryption is hardcoded in PXE to be AES. We'll soon move it into aztec-nr, at which point it'd be possible to somehow configure which encryption algorithm the contracts wishes to use. But this is just a first step, so it'll take a while to get there. Keep in mind this is the very first attempt to do this this alternative way. We'd similarly want to allow for unconstrained encryption etc.
There was a problem hiding this comment.
this file imports encrypted_logs::log_assembly_strategies::default_aes128::note::encode_and_encrypt_note but could be using other encryption strategy, are both partial notes and full notes processed independently? (could they work with different setups?)
| NFT::at(context.this_address())._store_nft_set_partial_note(partial_note).enqueue(context); | ||
|
|
||
| partial_note | ||
| } |
There was a problem hiding this comment.
Fanstastic how concise ^ this has become.
benesjan
force-pushed
the
nv/redo-partial
branch
2 times, most recently
from
c6fc6cb to
106ce57
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
benesjan
force-pushed
the
nv/redo-partial
branch
3 times, most recently
from
2a9675f to
35e25d2
Compare
benesjan
force-pushed
the
nv/redo-partial
branch
from
b63c9a1 to
dfd887c
Compare
benesjan
force-pushed
the
nv/redo-partial
branch
from
dfd887c to
43a43b4
Compare
|
CI should pass once #12552 is merged. |
Back when PXE was a service processing all blocks, we'd remove nullified notes as we saw their nullifiers. This later got changed, and as of #10722 we remove all nullified notes whenever we 'sync' notes. However, note syncing is becoming less and less a part of PXE, and as of #12391 we even deliver notes _outside_ of the PXE-led note syncing process (whenever we complete partial note). This causes problems because we end up adding notes, failing to realize they've been nullified and then returning them via `get_notes` (which is what causes some tests in #12391 to fail). The next time a contract function is run we'll do note syncing again and they'll be then removed, but we did have a full fn call in which they were available. This PR makes it so we always check if newly-added notes have been nullified, and remove them if so. I also added some explanations re. why we're doing things this way, created some follow-up issues (mostly #12550 and #12553), and inlined `produceNoteDaos` to have the whole thing happen in a single place. I think it's now more readable but potentially slightly large - perhaps this will improve as we split `PxeOracleInterface` in multiple files or modules.
| let partial_note = NFTNote::partial( | ||
| to, | ||
| storage.private_nfts.at(to).storage_slot, | ||
| context, | ||
| to, | ||
| context.msg_sender(), | ||
| ); |
There was a problem hiding this comment.
(Not for this PR): It'd be cool if this could be created via the state variable. Like private_nfts.at(to).new_partial(...)
There was a problem hiding this comment.
Indeed! This is on the roadmap.
|
|
||
| // We prepare the private balance increase (the partial note). | ||
| let hiding_point_slot = _prepare_private_balance_increase(from, to, &mut context, storage); | ||
| let partial_note = _prepare_private_balance_increase(from, to, &mut context, storage); |
There was a problem hiding this comment.
It's technically a partial_note_commitment, given that the partial_note is actually a struct of information. (Sorry - I was referring to this PR for another reason, then saw this).
There was a problem hiding this comment.
Yeah this is ultimately an issue due to us not neatly following Rust's newtype idiom. PartialUintNote is a struct with a single value.
Debugging #12391 led me to discover that we cannot have concurrent simulations due to contracts now being allowed to read and write to PXE's stores at arbitrary moments. E.g. #12391 was failing CI due to multiple concurrent simulations deleting the same pending partial note from a capsule array. This PR disables that behavior by putting the problematic tasks in a serial queue. Multiple tests still call PXE expecting concurrency (typically via usage of `await Promise.all`), but I thought it made more sense to disable the behavior this way and issue a warning (to unblock #12391) and then worry about removing attempts to achieve concurrent behavior. I considered putting _all_ PXE functions in the serial queue, but refrained from doing so to avoid introducing a larger than strictly needed change. We may want to do this automatically via e.g. monkey-patching to avoid accidentally forgetting a case.
Fixes an issue in the cheatcode test that were introduced by #12391 when it updated the ordering of fields in the `UintNote` but not the test.
5 participants
Closes #9375.
This re-implements partial notes leveraging the work from #12122. We now emit a private log with all of the fields plus a 'public log completion tag', which is the tag of the public log that will contain the public fields. These two logs are merged when doing discovery, resulting in the note's packed representation.
All in all this means we need to store much less information in public storage (just one field), and we don't need to deal with publishing public logs with encrypted public content. It also removes a lot of PXE code that dealt with the destructuring of these logs, slicing off the public fields, etc. Once this is merged, moving decryption to NR should be trivial.
I iterated a bit on how the note structs are laid out and thing the current version is a reasonably good starting point. Eventually we'll autogenerate it via macros, but given we may want to introduce further changes I chose to not worry about that for now.
We're currently sort of hackily relying on the fact that note type ids are 7 bits long, and set the 8th bit to indicate the log is that of a partial note. Fixing this requires properly supporting events during log discovery (which would become message discovery), but that's out of scope for this PR.