From bd10595a5275ac2c2da06bf4f839e4f86ec36c81 Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Fri, 10 May 2024 20:45:19 +0100 Subject: [PATCH] feat: enforce note hash read requests to read within own contract (#6310) Please read [contributing guidelines](CONTRIBUTING.md) and remove this line. --- .../aztec-nr/aztec/src/note/note_getter.nr | 6 +-- .../aztec-nr/aztec/src/note/utils.nr | 10 ++++ .../aztec/src/state_vars/private_set.nr | 4 +- .../src/note_hash_read_request_reset.nr | 5 +- .../types/src/abis/note_hash_leaf_preimage.nr | 6 +-- ...build_note_hash_read_request_hints.test.ts | 50 ++++++++----------- .../build_note_hash_read_request_hints.ts | 7 +-- .../src/client/private_execution.test.ts | 2 +- 8 files changed, 44 insertions(+), 46 deletions(-) diff --git a/noir-projects/aztec-nr/aztec/src/note/note_getter.nr b/noir-projects/aztec-nr/aztec/src/note/note_getter.nr index 24f1ba45dd9..6602653dd47 100644 --- a/noir-projects/aztec-nr/aztec/src/note/note_getter.nr +++ b/noir-projects/aztec-nr/aztec/src/note/note_getter.nr @@ -4,7 +4,7 @@ use crate::note::{ constants::{GET_NOTE_ORACLE_RETURN_LENGTH, MAX_NOTES_PER_PAGE, VIEW_NOTE_ORACLE_RETURN_LENGTH}, note_getter_options::{NoteGetterOptions, Select, Sort, SortOrder, Comparator, NoteStatus, PropertySelector}, note_interface::NoteInterface, note_viewer_options::NoteViewerOptions, - utils::compute_note_hash_for_consumption + utils::compute_note_hash_for_read_request }; use crate::oracle; @@ -92,7 +92,7 @@ pub fn get_note( check_note_header(*context, storage_slot, note); - let note_hash_for_read_request = compute_note_hash_for_consumption(note); + let note_hash_for_read_request = compute_note_hash_for_read_request(note); context.push_note_hash_read_request(note_hash_for_read_request); note @@ -130,7 +130,7 @@ pub fn _get_notes_constrain_get_notes_internal( } prev_fields = fields; - let note_hash_for_read_request = compute_note_hash_for_consumption(note); + let note_hash_for_read_request = compute_note_hash_for_read_request(note); // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1410): test to ensure // failure if malicious oracle injects 0 nonce here for a "pre-existing" note. context.push_note_hash_read_request(note_hash_for_read_request); diff --git a/noir-projects/aztec-nr/aztec/src/note/utils.nr b/noir-projects/aztec-nr/aztec/src/note/utils.nr index 444923c3fbb..59298bd3230 100644 --- a/noir-projects/aztec-nr/aztec/src/note/utils.nr +++ b/noir-projects/aztec-nr/aztec/src/note/utils.nr @@ -67,6 +67,16 @@ pub fn compute_note_hash_for_insertion(note: Note) -> Field where Note: compute_inner_note_hash(note) } +pub fn compute_note_hash_for_read_request(note: Note) -> Field where Note: NoteInterface { + let header = note.get_header(); + + if (header.nonce != 0) { + compute_unique_note_hash(note) + } else { + compute_inner_note_hash(note) + } +} + pub fn compute_note_hash_for_consumption(note: Note) -> Field where Note: NoteInterface { let header = note.get_header(); // There are 3 cases for reading a note intended for consumption: diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/private_set.nr b/noir-projects/aztec-nr/aztec/src/state_vars/private_set.nr index 433f6cd491a..716a6588355 100644 --- a/noir-projects/aztec-nr/aztec/src/state_vars/private_set.nr +++ b/noir-projects/aztec-nr/aztec/src/state_vars/private_set.nr @@ -4,7 +4,7 @@ use crate::note::{ constants::MAX_NOTES_PER_PAGE, lifecycle::{create_note, create_note_hash_from_public, destroy_note}, note_getter::{get_notes, view_notes}, note_getter_options::NoteGetterOptions, note_header::NoteHeader, note_interface::NoteInterface, note_viewer_options::NoteViewerOptions, - utils::compute_note_hash_for_consumption + utils::compute_note_hash_for_read_request }; use crate::state_vars::storage::Storage; @@ -58,7 +58,7 @@ impl PrivateSet { // docs:start:remove pub fn remove(self, note: Note) where Note: NoteInterface { let context = self.context.private.unwrap(); - let note_hash = compute_note_hash_for_consumption(note); + let note_hash = compute_note_hash_for_read_request(note); let has_been_read = context.note_hash_read_requests.any(|r: ReadRequest| r.value == note_hash); assert(has_been_read, "Can only remove a note that has been read from the set."); diff --git a/noir-projects/noir-protocol-circuits/crates/reset-kernel-lib/src/note_hash_read_request_reset.nr b/noir-projects/noir-protocol-circuits/crates/reset-kernel-lib/src/note_hash_read_request_reset.nr index a7f53d46d62..cb105f6543c 100644 --- a/noir-projects/noir-protocol-circuits/crates/reset-kernel-lib/src/note_hash_read_request_reset.nr +++ b/noir-projects/noir-protocol-circuits/crates/reset-kernel-lib/src/note_hash_read_request_reset.nr @@ -59,12 +59,11 @@ mod tests { global note_hashes = inner_note_hashes.map(|n| silo_note_hash(contract_address, n)); // Create 5 read requests. 0 and 3 are reading settled note hashes. 1, 2 and 4 are reading pending note hashes. - // TODO(#2847): Read request values for settled note hashes shouldn't have been siloed by apps. global read_requests = [ - ReadRequest { value: note_hashes[1], counter: 11 }.scope(contract_address), // settled + ReadRequest { value: inner_note_hashes[1], counter: 11 }.scope(contract_address), // settled ReadRequest { value: inner_note_hashes[3], counter: 13 }.scope(contract_address), // pending ReadRequest { value: inner_note_hashes[2], counter: 39 }.scope(contract_address), // pending - ReadRequest { value: note_hashes[0], counter: 46 }.scope(contract_address), // settled + ReadRequest { value: inner_note_hashes[0], counter: 46 }.scope(contract_address), // settled ReadRequest { value: inner_note_hashes[3], counter: 78 }.scope(contract_address), // pending ]; diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/abis/note_hash_leaf_preimage.nr b/noir-projects/noir-protocol-circuits/crates/types/src/abis/note_hash_leaf_preimage.nr index 031325e6430..9a0c9efb7ea 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/abis/note_hash_leaf_preimage.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/abis/note_hash_leaf_preimage.nr @@ -29,10 +29,8 @@ impl LeafPreimage for NoteHashLeafPreimage { impl Readable for NoteHashLeafPreimage { fn assert_match_read_request(self, read_request: ScopedReadRequest) { - // TODO(#2847): Read request value shouldn't have been siloed by apps. - // let siloed_value = silo_note_hash(read_request.contract_address, read_request.value); - // assert_eq(self.value, siloed_value, "Value of the note hash leaf does not match read request"); - assert_eq(self.value, read_request.value(), "Value of the note hash leaf does not match read request"); + let siloed_value = silo_note_hash(read_request.contract_address, read_request.value()); + assert_eq(self.value, siloed_value, "Value of the note hash leaf does not match read request"); } } diff --git a/yarn-project/circuits.js/src/hints/build_note_hash_read_request_hints.test.ts b/yarn-project/circuits.js/src/hints/build_note_hash_read_request_hints.test.ts index fa15e1b15ed..831c6d70a82 100644 --- a/yarn-project/circuits.js/src/hints/build_note_hash_read_request_hints.test.ts +++ b/yarn-project/circuits.js/src/hints/build_note_hash_read_request_hints.test.ts @@ -42,15 +42,9 @@ describe('buildNoteHashReadRequestHints', () => { const makeNoteHash = (value: number, counter = 1) => new NoteHash(new Fr(value), counter).scope(0, contractAddress); - const readPendingNoteHash = ({ - noteHashIndex, - readRequestIndex = numReadRequests, - hintIndex = numPendingReads, - }: { - noteHashIndex: number; - readRequestIndex?: number; - hintIndex?: number; - }) => { + const readPendingNoteHash = (noteHashIndex: number) => { + const readRequestIndex = numReadRequests; + const hintIndex = numPendingReads; noteHashReadRequests[readRequestIndex] = makeReadRequest(innerNoteHash(noteHashIndex)); expectedHints.readRequestStatuses[readRequestIndex] = ReadRequestStatus.pending(hintIndex); expectedHints.pendingReadHints[hintIndex] = new PendingReadHint(readRequestIndex, noteHashIndex); @@ -58,16 +52,12 @@ describe('buildNoteHashReadRequestHints', () => { numPendingReads++; }; - const readSettledNoteHash = ({ - readRequestIndex = numReadRequests, - hintIndex = numSettledReads, - }: { - readRequestIndex?: number; - hintIndex?: number; - } = {}) => { - const value = settledNoteHashes[hintIndex]; - noteHashLeafIndexMap.set(value.toBigInt(), settledLeafIndexes[hintIndex]); - noteHashReadRequests[readRequestIndex] = new ReadRequest(value, 1).scope(contractAddress); + const readSettledNoteHash = (noteHashIndex: number) => { + const readRequestIndex = numReadRequests; + const hintIndex = numSettledReads; + const value = settledNoteHashes[noteHashIndex]; + noteHashLeafIndexMap.set(value.toBigInt(), settledLeafIndexes[noteHashIndex]); + noteHashReadRequests[readRequestIndex] = makeReadRequest(settledNoteHashInnerValues[noteHashIndex]); expectedHints.readRequestStatuses[readRequestIndex] = ReadRequestStatus.settled(hintIndex); expectedHints.settledReadHints[hintIndex] = new SettledReadHint(readRequestIndex, {} as any, value); numReadRequests++; @@ -93,32 +83,32 @@ describe('buildNoteHashReadRequestHints', () => { }); it('builds hints for pending note hash read requests', async () => { - readPendingNoteHash({ noteHashIndex: 2 }); - readPendingNoteHash({ noteHashIndex: 1 }); + readPendingNoteHash(2); + readPendingNoteHash(1); const hints = await buildHints(); expect(hints).toEqual(expectedHints); }); it('builds hints for settled note hash read requests', async () => { - readSettledNoteHash(); - readSettledNoteHash(); + readSettledNoteHash(0); + readSettledNoteHash(1); const hints = await buildHints(); expect(hints).toEqual(expectedHints); }); it('builds hints for mixed pending and settled note hash read requests', async () => { - readPendingNoteHash({ noteHashIndex: 2 }); - readSettledNoteHash(); - readSettledNoteHash(); - readPendingNoteHash({ noteHashIndex: 1 }); - readPendingNoteHash({ noteHashIndex: 1 }); - readSettledNoteHash(); + readPendingNoteHash(2); + readSettledNoteHash(2); + readSettledNoteHash(0); + readPendingNoteHash(1); + readPendingNoteHash(1); + readSettledNoteHash(2); const hints = await buildHints(); expect(hints).toEqual(expectedHints); }); it('throws if cannot find a match in pending set and in the tree', async () => { - readPendingNoteHash({ noteHashIndex: 2 }); + readPendingNoteHash(2); // Tweak the value of the read request. noteHashReadRequests[0].readRequest.value = new Fr(123); await expect(() => buildHints()).rejects.toThrow('Read request is reading an unknown note hash.'); diff --git a/yarn-project/circuits.js/src/hints/build_note_hash_read_request_hints.ts b/yarn-project/circuits.js/src/hints/build_note_hash_read_request_hints.ts index 4fc62f76216..7fdfda24591 100644 --- a/yarn-project/circuits.js/src/hints/build_note_hash_read_request_hints.ts +++ b/yarn-project/circuits.js/src/hints/build_note_hash_read_request_hints.ts @@ -5,6 +5,7 @@ import { type MAX_NOTE_HASH_READ_REQUESTS_PER_TX, type NOTE_HASH_TREE_HEIGHT, } from '../constants.gen.js'; +import { siloNoteHash } from '../hash/index.js'; import { type MembershipWitness, NoteHashReadRequestHintsBuilder, @@ -51,13 +52,13 @@ export async function buildNoteHashReadRequestHints( if (pendingNoteHash !== undefined) { builder.addPendingReadRequest(i, pendingNoteHash.index); } else { - // TODO(#2847): Read request value for settled note hash shouldn't have been siloed by apps. - const leafIndex = noteHashLeafIndexMap.get(value.toBigInt()); + const siloedValue = siloNoteHash(readRequest.contractAddress, value); + const leafIndex = noteHashLeafIndexMap.get(siloedValue.toBigInt()); if (leafIndex === undefined) { throw new Error('Read request is reading an unknown note hash.'); } const membershipWitness = await oracle.getNoteHashMembershipWitness(leafIndex); - builder.addSettledReadRequest(i, membershipWitness, value); + builder.addSettledReadRequest(i, membershipWitness, siloedValue); } } return builder.toHints(); diff --git a/yarn-project/simulator/src/client/private_execution.test.ts b/yarn-project/simulator/src/client/private_execution.test.ts index cdcc07bd5a5..7c9666f8317 100644 --- a/yarn-project/simulator/src/client/private_execution.test.ts +++ b/yarn-project/simulator/src/client/private_execution.test.ts @@ -438,7 +438,7 @@ describe('Private Execution test suite', () => { const readRequests = getNonEmptyItems(result.callStackItem.publicInputs.noteHashReadRequests).map(r => r.value); expect(readRequests).toHaveLength(consumedNotes.length); - expect(readRequests).toEqual(expect.arrayContaining(consumedNotes.map(n => n.siloedNoteHash))); + expect(readRequests).toEqual(expect.arrayContaining(consumedNotes.map(n => n.uniqueNoteHash))); }); it('should be able to destroy_and_create with dummy notes', async () => {