feat!: constrain note_getter filter (#6703)

The application of the filter was not constrained when reading notes,
this PR changes that. While filtering can be thought of as assisting PXE
during note retrieval to avoid unnecessary extra read requests, I think
most users will also assume that the function will also lay down
constrains.

The biggest motivation for this change is that this is how `select()`
work: instructions are sent to the PXE to only return certain notes, and
we then verify that the oracle behaved as expected. `filter()` is simply
an extension of this for conditions that cannot be expressed in a simple
select: it is extremely unexpected that filtering would not be
constrained when selects are.

Consider the following example:

```rust
// These guarantee that the retrieved notes have a value larger than 500
let opts = NoteGetterOptions::new().options.select(ValueNote::properties().value, 500, Comparator.GT);

// These make a cooperative PXE retrieve notes with a value between 500 and 700, both provide no guarantees whatsoever
let opts = NoteGetterOptions::with_filter(|opt_notes| {
  let mut filtered_notes = [0; opt_notes.len()];
  for i in 0..opt_notes.len() {
    let note_value = opt_notes[i].unwrap_or(0);
    if (note_value > 500) & (note_value < 700) {
      filtered_notes[i] = Option::some(note_value);
    }
  }

  filtered_notes
});
```

An example is `filter_notes_min_sum`, which keeps notes until as long as
the sum is below some minimum. Callers still have to process the sum
again on their end however and compare it to the minimum, since the
filter used to provide no guarantees. In some cases this was hidden
behind multiple layers of indirection, and as a result
`ValueNote::decrement_by_at_most` does not really uphold that the
balance is decremented by less than `max_balance` - a very subtle bug.

docs/docs/migration_notes.md

@@ -6,6 +6,12 @@ keywords: [sandbox, cli, aztec, notes, migration, updating, upgrading]
 
 Aztec is in full-speed development. Literally every version breaks compatibility with the previous ones. This page attempts to target errors and difficulties you might encounter when upgrading, and how to resolve them.
 
+## TBD
+
+### [Aztec.nr] Filtering is now constrained
+
+The `filter` argument of `NoteGetterOptions` (typically passed via the `with_filter()` function) is now applied in a constraining environment, meaning any assertions made during the filtering are guaranteed to hold. This mirrors the behavior of the `select()` function.
+
 ## 0.42.0
 
 ### [Aztec.nr] Emitting encrypted notes and logs
@@ -17,6 +23,7 @@ The `emit_encrypted_log` context function is now `encrypt_and_emit_log` or `encr
 + context.encrypt_and_emit_log(log1);
 + context.encrypt_and_emit_note(note1);
 ```
+
 Broadcasting a note will call `encrypt_and_emit_note` in the background. To broadcast a generic event, use `encrypt_and_emit_log`
 with the same encryption parameters as notes require. Currently, only fields and arrays of fields are supported as events.
 

docs/docs/reference/smart_contract_reference/storage/private_state.md

@@ -272,8 +272,6 @@ This setting enables us to skip the first `offset` notes. It's particularly usef
 
 Developers have the option to provide a custom filter. This allows specific logic to be applied to notes that meet the criteria outlined above. The filter takes the notes returned from the oracle and `filter_args` as its parameters.
 
-It's important to note that the process of applying the custom filter to get the final notes is not constrained. It's crucial to verify the returned notes even if certain assumptions are made in the custom filter.
-
 ### `filter_args: FILTER_ARGS`
 
 `filter_args` provides a means to furnish additional data or context to the custom filter.

noir-projects/aztec-nr/aztec/src/note/note_getter.nr

@@ -38,8 +38,8 @@ fn check_note_header<Note, N, M>(
 ) where Note: NoteInterface<N, M> {
     let header = note.get_header();
     let contract_address = context.this_address();
-    assert(header.contract_address.eq(contract_address));
-    assert(header.storage_slot == storage_slot);
+    assert(header.contract_address.eq(contract_address), "Mismatch note header contract address.");
+    assert(header.storage_slot == storage_slot, "Mismatch note header storage slot.");
 }
 
 fn check_note_fields<N>(serialized_note: [Field; N], selects: BoundedVec<Option<Select>, N>) {
@@ -118,10 +118,18 @@ fn constrain_get_notes_internal<Note, N, M, FILTER_ARGS>(
 ) -> [Option<Note>; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL] where Note: NoteInterface<N, M> {
     let mut returned_notes = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
 
+    // The filter is applied first to avoid pushing note read requests for notes we're not interested in. Note that
+    // while the filter function can technically mutate the contents of the notes (as opposed to simply removing some),
+    // the private kernel will later validate that these note actually exist, so transformations would cause for that
+    // check to fail.
+    let filter_fn = options.filter;
+    let filter_args = options.filter_args;
+    let filtered_notes = filter_fn(opt_notes, filter_args);
+
     let mut num_notes = 0;
     let mut prev_fields = [0; N];
-    for i in 0..opt_notes.len() {
-        let opt_note = opt_notes[i];
+    for i in 0..filtered_notes.len() {
+        let opt_note = filtered_notes[i];
         if opt_note.is_some() {
             let note = opt_note.unwrap_unchecked();
             let fields = note.serialize_content();
@@ -137,14 +145,18 @@ fn constrain_get_notes_internal<Note, N, M, FILTER_ARGS>(
             // failure if malicious oracle injects 0 nonce here for a "pre-existing" note.
             context.push_note_hash_read_request(note_hash_for_read_request);
 
-            // The below code is used to collapse a sparse array into one where the values are guaranteed to be at the front of the array
-            // We write at returned_notes[num_notes] because num_notes is only advanced when we have a value in opt_notes
+            // The below code is used to collapse a sparse array into one where the values are guaranteed to be at the 
+            // front of the array. This is highly useful because the caller knows that the returned array won't have
+            // more than option.limits notes, and can therefore loop over this limit value instead of the entire array,
+            // resulting in a smaller circuit and faster proving times.
+            // We write at returned_notes[num_notes] because num_notes is only advanced when we have a value in 
+            // filtered_notes.
             returned_notes[num_notes] = Option::some(note);
             num_notes += 1;
         };
     }
     if options.limit != 0 {
-        assert(num_notes <= options.limit, "Invalid number of return notes.");
+        assert(num_notes <= options.limit, "Got more notes than limit.");
     }
 
     assert(num_notes != 0, "Cannot return zero notes");
@@ -181,11 +193,14 @@ unconstrained fn get_notes_internal<Note, N, M, FILTER_ARGS>(
     storage_slot: Field,
     options: NoteGetterOptions<Note, N, M, FILTER_ARGS>
 ) -> [Option<Note>; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL] where Note: NoteInterface<N, M> {
+    // This function simply performs some transformations from NoteGetterOptions into the types required by the oracle.
+
     let (num_selects, select_by_indexes, select_by_offsets, select_by_lengths, select_values, select_comparators, sort_by_indexes, sort_by_offsets, sort_by_lengths, sort_order) = flatten_options(options.selects, options.sorts);
     let placeholder_opt_notes = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
     let placeholder_fields = [0; GET_NOTES_ORACLE_RETURN_LENGTH];
     let placeholder_note_length = [0; N];
-    let opt_notes = oracle::notes::get_notes(
+
+    oracle::notes::get_notes(
         storage_slot,
         num_selects,
         select_by_indexes,
@@ -203,11 +218,7 @@ unconstrained fn get_notes_internal<Note, N, M, FILTER_ARGS>(
         placeholder_opt_notes,
         placeholder_fields,
         placeholder_note_length
-    );
-
-    let filter = options.filter;
-    let filter_args = options.filter_args;
-    filter(opt_notes, filter_args)
+    )
 }
 
 unconstrained pub fn view_notes<Note, N, M>(

noir-projects/aztec-nr/aztec/src/note/note_getter/test.nr

@@ -23,33 +23,121 @@ fn build_valid_note(value: Field) -> MockNote {
 }
 
 #[test]
-    fn validates_note_and_returns_it() {
+fn processes_single_note() {
     let mut context = setup();
 
-    let value = 1337;
-    let test_note = build_valid_note(value);
+    let mut notes_to_constrain = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+    notes_to_constrain[0] = Option::some(build_valid_note(13));
+
+    let options = NoteGetterOptions::new();
+    let returned = constrain_get_notes_internal(&mut context, storage_slot, notes_to_constrain, options);
+
+    assert_eq(returned, notes_to_constrain);
+    assert_eq(context.note_hash_read_requests.len(), 1);
+}
+
+#[test]
+fn processes_many_notes() {
+    let mut context = setup();
+
+    let mut notes_to_constrain = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+    notes_to_constrain[0] = Option::some(build_valid_note(13));
+    notes_to_constrain[1] = Option::some(build_valid_note(19));
+
+    let options = NoteGetterOptions::new();
+    let returned = constrain_get_notes_internal(&mut context, storage_slot, notes_to_constrain, options);
+
+    assert_eq(returned, notes_to_constrain);
+    assert_eq(context.note_hash_read_requests.len(), 2);
+}
+
+#[test]
+fn collapses_notes_at_the_beginning_of_the_array() {
+    let mut context = setup();
 
     let mut opt_notes = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
-    opt_notes[0] = Option::some(test_note);
+    opt_notes[1] = Option::some(build_valid_note(0));
+    opt_notes[2] = Option::some(build_valid_note(1));
+    opt_notes[3] = Option::some(build_valid_note(2));
+    opt_notes[5] = Option::some(build_valid_note(3));
+    opt_notes[8] = Option::some(build_valid_note(4));
+    opt_notes[13] = Option::some(build_valid_note(5));
+    opt_notes[21] = Option::some(build_valid_note(6));
 
     let options = NoteGetterOptions::new();
     let returned = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
 
-    assert_eq(returned[0].unwrap().value, value);
+    let mut expected = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+    expected[0] = Option::some(build_valid_note(0));
+    expected[1] = Option::some(build_valid_note(1));
+    expected[2] = Option::some(build_valid_note(2));
+    expected[3] = Option::some(build_valid_note(3));
+    expected[4] = Option::some(build_valid_note(4));
+    expected[5] = Option::some(build_valid_note(5));
+    expected[6] = Option::some(build_valid_note(6));
+
+    assert_eq(returned, expected);
 }
 
-#[test(should_fail)]
-    fn cannot_return_zero_notes() {
+#[test(should_fail_with="Cannot return zero notes")]
+    fn rejects_zero_notes() {
+    let mut context = setup();
+
+    let opt_notes: [Option<MockNote>; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL] = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+
+    let options = NoteGetterOptions::new();
+    let _ = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
+}
+
+#[test(should_fail_with="Got more notes than limit.")]
+fn rejects_mote_notes_than_limit() {
     let mut context = setup();
 
     let mut opt_notes: [Option<MockNote>; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL] = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+    opt_notes[1] = Option::some(build_valid_note(0));
+    opt_notes[2] = Option::some(build_valid_note(1));
+    opt_notes[3] = Option::some(build_valid_note(2));
 
     let mut options = NoteGetterOptions::new();
+    options = options.set_limit(2);
     let _ = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
 }
 
-#[test(should_fail)]
-    fn mismatched_address() {
+#[test]
+fn applies_filter_before_constraining() {
+    let mut context = setup();
+
+    let mut notes_to_constrain = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+    let invalid_note = MockNote::new(13).build(); // This note does not have the correct address or storage slot
+    notes_to_constrain[0] = Option::some(invalid_note);
+    notes_to_constrain[1] = Option::some(build_valid_note(42));
+
+    let filter_fn = |opt_notes: [Option<MockNote>; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL], _| {
+        let mut selected = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+
+        for i in 0..opt_notes.len() {
+            // This will filter the notes so that only the valid one remains
+            if opt_notes[i].is_some() & (opt_notes[i].unwrap_unchecked().value == 42) {
+                selected[i] = opt_notes[i];
+            }
+        }
+        selected
+    };
+
+    let options = NoteGetterOptions::with_filter(filter_fn, ());
+    let returned = constrain_get_notes_internal(&mut context, storage_slot, notes_to_constrain, options);
+
+    // Only the note with value 42 should be returned, and moved to the beginning of the array. The other notes were not
+    // constrained, and hence validation did not fail.
+    let mut expected = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+    expected[0] = Option::some(build_valid_note(42));
+
+    assert_eq(returned, expected);
+    assert_eq(context.note_hash_read_requests.len(), 1);
+}
+
+#[test(should_fail_with="Mismatch note header contract address.")]
+fn rejects_mismatched_address() {
     let mut context = setup();
 
     let note = MockNote::new(1).storage_slot(storage_slot).build(); // We're not setting the right contract address
@@ -61,11 +149,11 @@ fn build_valid_note(value: Field) -> MockNote {
     let _ = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
 }
 
-#[test(should_fail)]
-    fn mismatched_storage_slot() {
+#[test(should_fail_with="Mismatch note header storage slot.")]
+fn rejects_mismatched_storage_slot() {
     let mut context = setup();
 
-    let note = MockNote::new(1).storage_slot(storage_slot + 1).build(); // We're not setting the right storage slot
+    let note = MockNote::new(1).contract_address(contract_address).build(); // We're not setting the right storage slot
 
     let mut opt_notes = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
     opt_notes[0] = Option::some(note);
@@ -74,8 +162,8 @@ fn build_valid_note(value: Field) -> MockNote {
     let _ = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
 }
 
-#[test(should_fail)]
-    fn mismatched_selector() {
+#[test(should_fail_with="Mismatch return note field.")]
+fn rejects_mismatched_selector() {
     let mut context = setup();
 
     let value = 10;
@@ -94,8 +182,8 @@ fn build_valid_note(value: Field) -> MockNote {
     let _ = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
 }
 
-#[test(should_fail)]
-    fn mismatched_desc_sort_order() {
+#[test(should_fail_with="Return notes not sorted in descending order.")]
+fn rejects_mismatched_desc_sort_order() {
     let mut context = setup();
 
     let mut opt_notes = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
@@ -111,8 +199,8 @@ fn build_valid_note(value: Field) -> MockNote {
     let _ = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
 }
 
-#[test(should_fail)]
-    fn mismatched_asc_sort_order() {
+#[test(should_fail_with="Return notes not sorted in ascending order.")]
+fn rejects_mismatched_asc_sort_order() {
     let mut context = setup();
 
     let mut opt_notes = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
@@ -127,24 +215,3 @@ fn build_valid_note(value: Field) -> MockNote {
         );
     let _ = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
 }
-
-#[test]
-    fn sparse_notes_array() {
-    let mut context = setup();
-
-    let mut opt_notes = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
-    opt_notes[1] = Option::some(build_valid_note(0));
-    opt_notes[2] = Option::some(build_valid_note(1));
-    opt_notes[3] = Option::some(build_valid_note(2));
-    opt_notes[5] = Option::some(build_valid_note(3));
-    opt_notes[8] = Option::some(build_valid_note(4));
-    opt_notes[13] = Option::some(build_valid_note(5));
-    opt_notes[21] = Option::some(build_valid_note(6));
-
-    let options = NoteGetterOptions::new();
-    let returned = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options);
-
-    for i in 0..7 {
-        assert(returned[i].unwrap().value == i as Field);
-    }
-}

noir-projects/aztec-nr/aztec/src/note/note_header.nr

@@ -1,5 +1,5 @@
 use dep::protocol_types::address::AztecAddress;
-use dep::protocol_types::traits::{Empty, Serialize};
+use dep::protocol_types::traits::{Empty, Eq, Serialize};
 
 struct NoteHeader {
     contract_address: AztecAddress,
@@ -16,6 +16,15 @@ impl Empty for NoteHeader {
     }
 }
 
+impl Eq for NoteHeader {
+    fn eq(self, other: Self) -> bool {
+        (self.contract_address == other.contract_address) & 
+        (self.nonce == other.nonce) & 
+        (self.storage_slot == other.storage_slot)& 
+        (self.is_transient == other.is_transient)
+    }
+}
+
 impl NoteHeader {
     pub fn new(contract_address: AztecAddress, nonce: Field, storage_slot: Field) -> Self {
         NoteHeader { contract_address, nonce, storage_slot, is_transient: false }

noir-projects/aztec-nr/aztec/src/test/mocks/mock_note.nr

@@ -1,6 +1,6 @@
 use crate::{context::PrivateContext, note::{note_header::NoteHeader, note_interface::NoteInterface}};
 
-use dep::protocol_types::{address::AztecAddress, grumpkin_point::GrumpkinPoint};
+use dep::protocol_types::{address::AztecAddress, grumpkin_point::GrumpkinPoint, traits::Eq};
 
 global MOCK_NOTE_LENGTH = 1;
 // MOCK_NOTE_LENGTH * 32 + 32(storage_slot as bytes) + 32(note_type_id as bytes)
@@ -76,6 +76,13 @@ impl NoteInterface<MOCK_NOTE_LENGTH, MOCK_NOTE_BYTES_LENGTH> for MockNote {
     }
 }
 
+impl Eq for MockNote {
+    fn eq(self, other: Self) -> bool {
+      (self.header == other.header) & 
+      (self.value == other.value)
+    }
+}
+
 struct MockNoteBuilder {
     value: Field,
     contract_address: Option<AztecAddress>,