Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: check a note is read before nullifying it. #2076

Merged
merged 4 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/docs/dev_docs/contracts/state_variables.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ We can also remove a note from a set:

#include_code state_vars-SetRemove /yarn-project/noir-contracts/src/contracts/docs_example_contract/src/actions.nr rust

Note that the transaction won't fail if the note we are removing does not exist in the set. As a best practice, we should fetch the notes by calling [`get_notes`](#get_notes), which does a membership check under the hood to make sure the notes exist, and then feed the returned notes to the `remove` function.
Note that the proof will fail if the note we are removing does not exist. To avoid this, it's advisable to first retrieve the notes using [`get_notes`](#get_notes), which does a membership check under the hood to make sure the notes exist, and then we can safely provide these notes as input to the `remove` function.

### `.get_notes`

Expand Down
26 changes: 20 additions & 6 deletions yarn-project/noir-libs/noir-aztec/src/state_vars/set.nr
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
use dep::std::option::Option;
use crate::abi::PublicContextInputs;
use crate::constants_gen::{MAX_NOTES_PER_PAGE, MAX_READ_REQUESTS_PER_CALL};
use crate::context::{PrivateContext, PublicContext};
use crate::note::lifecycle::{create_note, create_note_hash_from_public, destroy_note};
use crate::note::{
lifecycle::{create_note, create_note_hash_from_public, destroy_note},
note_getter::{ensure_note_exists, ensure_note_hash_exists, get_notes, view_notes},
note_getter_options::NoteGetterOptions,
note_interface::NoteInterface,
note_viewer_options::NoteViewerOptions,
utils::compute_inner_note_hash,
utils::compute_note_hash_for_read_or_nullify,
};
use dep::std::option::Option;

struct Set<Note, N> {
private_context: Option<&mut PrivateContext>,
Expand Down Expand Up @@ -62,7 +62,12 @@ impl<Note, N> Set<Note, N> {
self.note_interface,
&mut note_with_header,
);
self.remove(note_with_header);
destroy_note(
self.private_context.unwrap(),
self.storage_slot,
note_with_header,
self.note_interface,
);
}

// TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386):
Expand All @@ -88,11 +93,20 @@ impl<Note, N> Set<Note, N> {
// this hack once public kernel injects nonces.
header.nonce = 1;
set_header(&mut note_with_header, header);

self.remove(note_with_header);
destroy_note(
self.private_context.unwrap(),
self.storage_slot,
note_with_header,
self.note_interface,
);
}

fn remove(self, note: Note) {
let note_hash = compute_note_hash_for_read_or_nullify(self.note_interface, note);
let read_requests = self.private_context.unwrap_unchecked().read_requests;
let has_been_read = read_requests.any(|r| r == note_hash);
assert(has_been_read, "Can only remove a note that has been read from the set.");

destroy_note(
self.private_context.unwrap(),
self.storage_slot,
Expand Down
95 changes: 82 additions & 13 deletions yarn-project/noir-libs/noir-aztec/src/types/vec.nr
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,87 @@ impl<T, MaxLen> BoundedVec<T, MaxLen> {
self.len -= 1;
elem
}

fn any<Env>(self, predicate: fn[Env](T) -> bool) -> bool {
LeilaWang marked this conversation as resolved.
Show resolved Hide resolved
let mut ret = false;
let mut exceeded_len = false;
for i in 0..MaxLen {
exceeded_len |= i == self.len;
if (!exceeded_len) {
ret |= predicate(self.storage[i]);
}
}
ret
}
}

#[test]
fn test_vec_push_pop() {
let mut vec: BoundedVec<Field, 3> = BoundedVec::new(0);
assert(vec.len == 0);
vec.push(2);
assert(vec.len == 1);
vec.push(4);
assert(vec.len == 2);
vec.push(6);
assert(vec.len == 3);
let x = vec.pop();
assert(x == 6);
assert(vec.len == 2);
assert(vec.get(0) == 2);
assert(vec.get(1) == 4);
}

#[test]
fn test_vec_push_array() {
let mut vec: BoundedVec<Field, 3> = BoundedVec::new(0);
vec.push_array([2, 4]);
assert(vec.len == 2);
assert(vec.get(0) == 2);
assert(vec.get(1) == 4);
}

#[test(should_fail)]
fn test_vec_get_out_of_bound() {
let mut vec: BoundedVec<Field, 2> = BoundedVec::new(0);
vec.push_array([2, 4]);
let _x = vec.get(2);
}

#[test(should_fail)]
fn test_vec_get_not_declared() {
let mut vec: BoundedVec<Field, 2> = BoundedVec::new(0);
vec.push_array([2]);
let _x = vec.get(1);
}

#[test(should_fail)]
fn test_vec_get_uninitialised() {
let mut vec: BoundedVec<Field, 2> = BoundedVec::new(0);
let _x = vec.get(0);
}

#[test(should_fail)]
fn test_vec_push_overflow() {
let mut vec: BoundedVec<Field, 1> = BoundedVec::new(0);
vec.push(1);
vec.push(2);
LeilaWang marked this conversation as resolved.
Show resolved Hide resolved
}

#[test]
fn test_vec_any() {
let mut vec: BoundedVec<Field, 3> = BoundedVec::new(0);
vec.push_array([2, 4, 6]);
assert(vec.any(|v| v == 2) == true);
assert(vec.any(|v| v == 4) == true);
assert(vec.any(|v| v == 6) == true);
assert(vec.any(|v| v == 3) == false);
LeilaWang marked this conversation as resolved.
Show resolved Hide resolved
}

// #[test]
// fn test_vec() {
// let vec: BoundedVec<Field, 2> = BoundedVec::new(0);
// assert(vec.len == 0);
// let vec1 = vec.push(1);
// assert(vec1.len == 1);
// let vec2 = vec1.push(1);
// assert(vec2.len == 2);
// let vec3 = vec2.push(1);
// assert(vec3.len == 3);
// let x = vec3.pop();
// assert(x == 1);
// }
#[test]
fn test_vec_any_not_default() {
let default_value = 1;
let mut vec: BoundedVec<Field, 3> = BoundedVec::new(default_value);
vec.push_array([2, 4]);
assert(vec.any(|v| v == default_value) == false);
}