Avoid double computation of note commitment

[suyash67]

When destroying a note, along with the nullifier of that note, we also output the note commitment. For this, we compute the note commitment from the note itself. Ideally, we must reuse the note commitment (that must already be computed before nullifying a note).

aztec-packages/yarn-project/noir-libs/noir-aztec/src/note/lifecycle.nr

Line 77 in ec5241c

nullified_commitment = compute_inner_note_hash(note_interface, note);

[iAmMichaelConnor]

Perhaps the note could contain a cached _inner_note_hash data member, which gets set whenever compute_inner_note_hash is first called?
This approach has the downside that it would worsen the learning curve for developers who which to write their own notes (because they would then have to understand why they need to include this field, and how to set this field). Maybe it's a bad suggestion, because it kind-of breaks the interfaces that we have? If Noir had 'inheritance', then users could inherit a base note class which could implement these things. I'm not sure what the 'Rust' equivalent of a inheritance is.

[iAmMichaelConnor]

Ideally, we must reuse the note commitment (that must already be computed before nullifying a note)

I think it might actually be essential, from a security perspective, because we can't trust the compute_inner_note_hash function to always return the same note value (because its implementation is user-defined)

[iAmMichaelConnor]

Tagging @LeilaWang for thoughts too

[suyash67]

Discussed this issue with @iAmMichaelConnor @dbanks12 and @jeanmon. This does not look like a security issue with the protocol. The function compute_inner_note_hash in the note interface is user-defined and a malicious developer can make this function return two different note hashes for a same note while:

1. Creating the note hash when creating the note
2. Computing the note hash of a note while destroying a note
   This is dependent only on the implementation of compute_inner_note_hash method and it is the developers responsibility to make sure it is a deterministic function.

We still might want to cache the inner_note_hash as a part of the note interface but that would require changing the current note interface (at the cost of confusing developers).

[suyash67]

We can extend this optimisation to other parts of the code as well. For example, even when siloing a note, we re-compute the inner note hash.

aztec-packages/yarn-project/noir-libs/noir-aztec/src/note/utils.nr

Lines 21 to 31 in bae313d

	fn compute_siloed_note_hash<Note, N>(
	note_interface: NoteInterface<Note, N>,
	note_with_header: Note,
	) -> Field {
	let get_header = note_interface.get_header;
	let header = get_header(note_with_header);
	let inner_note_hash = compute_inner_note_hash(note_interface, note_with_header);
	compute_siloed_hash(header.contract_address, inner_note_hash)
	}

aztec-packages/yarn-project/noir-libs/noir-aztec/src/note/utils.nr

Lines 33 to 43 in bae313d

	fn compute_unique_siloed_note_hash<Note, N>(
	note_interface: NoteInterface<Note, N>,
	note_with_header: Note,
	) -> Field {
	let get_header = note_interface.get_header;
	let header = get_header(note_with_header);
	let siloed_note_hash = compute_siloed_note_hash(note_interface, note_with_header);
	compute_unique_hash(header.nonce, siloed_note_hash)
	}

This can only be optimised if we introduce a new member inner_hash (as Mike suggested) in the Note struct. From my understanding, this will be tricky since it'd touch a lot of aztec-noir code and must be done only after initial public release.

[iAmMichaelConnor]

That's a nice idea, @suyash67 .
Returning to this issue after some time, perhaps the note's header should contain the "inner hash"/"siloed hash"/"unique hash" data members, so that the user-defined note isn't cluttered with low-level concepts