refactor(Notes): Split get_note into get_note and get_note_unsafe

[nventuro]

The discussion in #3123 and conversations with @LHerskind made me realize that our two big state variables, Singleton and Set, behave very differently with reading notes despite having similar APIs.

Set.get_notes() simply reads a bunch of notes and returns them. The expectation is that these notes will be immediately consumed and nullified by calling remove(). Indeed, this is how things like BalanceSet work. Failing to call remove() introduces a security issue since nullifier non-inclusion will never be proved, and the burden of this call falls on the user of Set.

Singleton.get_note() reads a note, immediately nullifies it and creates a new one with the same value. This guarantees that the value read is not nullified and get_note() is therefore always safe to use. However, this is often suboptimal: if the value was mean to be replaced with a different one then the nullification and re-creation inside get_note() was redundant and unnecessary. (the kernels actually squash note creation followed by nullification, but we're still doing a lot of useless hashing)

I think this behavior mismatch on such basic types can be extremely confusing to new users and easily lead to costly mistakes. But there's also good reasons why things are the way they are, and both current implementations are good things that should exist. I propose we take the learnings from each of these, more explicitly lay out potential pit falls, and address naming inconsistencies.

---

The plan is to have for both state variables a get_note variant that does what it says and is always safe (but might be suboptimal), and a get_note_unsafe variant which, if you know what you're doing, can be used to produce better results while still remaining safe.

For Singleton:

• get_note would be the same as the current implementation. Example use case: some configuration value is read.
• get_note_unsafe would not destroy the current note nor create a new one. Example use case: some value is read and then modified (as was being discussed in refactor: singleton redesign #3753).

For Set:

• get_note would remove all read notes from the set and then reinsert them. Example use case: some configuration values are proven to exist (e.g. a set of permissions).
• get_note_unsafe would be the same as the current implementation. Example use case: a token balance in which read notes are consumed.

[nventuro]

@LHerskind suggested that we could also extend the interface in other ways, for example by adding pop_notes to Set, which would nullify all read notes. This would provide something like BalanceSet a way to achieve the desired behavior without having to resort to the unsafe variant. A valid use case for unsafe remains however when only some of the notes returned by the oracle are consumed and the rest are simply unused, in which case the non-nullification is not a problem.

[iAmMichaelConnor]

Permutations of scenarios are a mix and match of the following:

• 'Current-ness' of the note:
  • 1a. [Current]: You want to get a not-yet-nullified ("current") note.
  • 1b. [Old]: You want to get an already-nullified ("historical") note.
  • 1c. [Yolo get/don't care]: You want to get a note and don't care if it's already been nullified or not. (Weird use case?)
• 'Mutability' of the note:
  • 2a. [Mutable]: You want to get, nullify it, and replace the note with a different note preimage.
  • 2b. [Immutable]: You want to get, nullify it, and replace the note with the same note.
  • 2c. [Delete]: You want to get and nullify the note, but not replace the note. (I don't think this is possible with Singleton today, and there's a reinitialisation issue if you ever want to re-initialise this storage slot in future, if you delete a Singleton without replacing it. @lasse see footnote.).
  • 2d. [Don't nullify]: You want to get the note, but not nullify it.

Hmm... I'm not sure if that's actually a helpful list. But having thought more, here is a hopefully-comprehensive list of methods that I think we should support:

• Set: Get current & remove it. (get from db, emit read request, & nullify). read_and_delete_current_note

• Set: Get current. (get from db, emit read request, & nullify (same as previous - it can't be "current" if you don't nullify it)). read_current_note (perhaps this one is not clear, and so shouldn't be supported).

• Set: Get current as immutable. (get from db, read request, & nullify & insert a note of equal value). read_current_note_as_immutable

• Set: Get old. (get from db, read from historic tree or emit read request???, assert that it's been nullified by reading from a historical nullifier tree, and maybe create a tracker in the context to ensure it never gets accidentally re-nullified by the developer). read_old_note

• Set: Overwrite (get & replace in one go, without caring about the current value). (Get from db, read request, nullify, and insert the replacement note). overwrite_note

• Set: Get unsafe. (get from db, emit read request. it's up to the developer to make sure they deal with the note correctly later). read_note_unsafe

• Set: Get super unsafe. (get from db, no read request emitted. it's up to the developer to make sure they deal with the note correctly later. This is basically just injecting a preimage from the db into the circuit, with no checks that the note exists). inject_note_unsafe / view_note

• Singleton: Get current & remove it (a "delete", basically). (get from db, emit read request, & nullify). read_and_delete_current_note

• Singleton: Get current (get from db, emit read request, & nullify (same as previous - it can't be "current" if you don't nullify it)). read_current_note (perhaps this one is not clear, and so shouldn't be supported).

• Singleton: Get current, as mutable (with the definite intention of replacing it within this circuit) (get from db, emit read request, nullify, and track in the context to ensure the storage slot does get written-to later (or throw an error)). read_current_note_to_replace

• Singleton: Get current as immutable. (get from db, read request, & nullify & insert a note of equal value). read_current_note_as_immutable

• Singleton: Overwrite (get & replace in one go, without caring about the current value). Get from db, read request, nullify, and insert the replacement note). overwrite_note

• Singleton: Get unsafe. (get from db, emit read request. it's up to the developer to make sure they deal with the note correctly later). read_note_unsafe

• Singleton: Get super unsafe. (get from db. No read request emitted. This is basically just injecting a preimage from the db into the circuit, with no checks that the note exists). inject_note_unsafe / view_note

In all cases, we should track in the context whether a note has been nullified already within the same function, and throw an error if it gets re-nullified. Barretenberg would allow us to do this without adding constraints, through non-circuit types. Maybe noir might have features to enable this?

Footnote:
We should explore 2c more, because we might hit problems if we want to support escrowing NFTs privately. Suppose you own a shielded version of an nft at state variable token_owners[token].initialise(NFT_Note::new(my_address));, where token_owners: Map<AztecAddress, Singleton<NFT_Note>>. This emits an initialisation nullifier for token_owners[token].
You then want to unshield that nft, to use it in public-land. So you effectively delete the contents of the storage slot token_owners[token].delete(my_note);. Now Bob buys your NFT in public-land. Now Bob wants to shield that NFT. Uh oh, if he tries to re-initialise that same storage slot, he's going to emit a duplicate initialisation nullifier. I'll try to dig out my old EY notes on this initialisation problem. I think we found a solution.

[nventuro]

Superceded by #7834.