feat: TXE detects duplicate nullifiers (#10764)

This functionality was lost in a refactor. Added tests to verify

noir-projects/noir-contracts/contracts/counter_contract/src/main.nr

@@ -224,13 +224,13 @@ contract Counter {
         assert(notes.len() == 4);
         assert(get_counter(owner) == 7);
 
-        // Checking from the note cache, note that we MUST advance the block to get a correct and updated value.
+        // Checking from the note cache
         Counter::at(contract_address).decrement(owner, sender).call(&mut env.private());
         let notes: BoundedVec<ValueNote, MAX_NOTES_PER_PAGE> = view_notes(owner_slot, options);
-        assert(get_counter(owner) == 11);
-        assert(notes.len() == 5);
+        assert(get_counter(owner) == 6);
+        assert(notes.len() == 4);
 
-        // We advance the block here to have the nullification of the prior note be applied. Should we try nullifiying notes in our DB on notifyNullifiedNote ?
+        // Checking that the note was discovered from private logs
         env.advance_block_by(1);
         let notes: BoundedVec<ValueNote, MAX_NOTES_PER_PAGE> = view_notes(owner_slot, options);
         assert(get_counter(owner) == 6);

noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/main.nr

@@ -1,3 +1,4 @@
+mod test;
 use dep::aztec::macros::aztec;
 
 #[aztec]

noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/first.nr

@@ -0,0 +1,111 @@
+use crate::test::utils;
+use dep::aztec::oracle::{execution::get_block_number, storage::storage_read};
+use dep::aztec::protocol_types::storage::map::derive_storage_slot_in_map;
+
+use crate::EasyPrivateVoting;
+
+#[test]
+unconstrained fn test_initializer() {
+    let (_, voting_contract_address, admin) = utils::setup();
+
+    let block_number = get_block_number();
+    let admin_slot = EasyPrivateVoting::storage_layout().admin.slot;
+    let admin_storage_value = storage_read(voting_contract_address, admin_slot, block_number);
+    assert(admin_storage_value == admin, "Vote ended should be false");
+}
+
+#[test]
+unconstrained fn test_check_vote_status() {
+    let (_, voting_contract_address, _) = utils::setup();
+
+    let vote_ended_expected: bool = false;
+
+    let block_number = get_block_number();
+    let status_slot = EasyPrivateVoting::storage_layout().vote_ended.slot;
+    let vote_ended_read: bool = storage_read(voting_contract_address, status_slot, block_number);
+    assert(vote_ended_expected == vote_ended_read, "Vote ended should be false");
+}
+
+#[test]
+unconstrained fn test_end_vote() {
+    let (env, voting_contract_address, admin) = utils::setup();
+
+    env.impersonate(admin);
+    EasyPrivateVoting::at(voting_contract_address).end_vote().call(&mut env.public());
+
+    let vote_ended_expected = true;
+
+    let block_number = get_block_number();
+    let status_slot = EasyPrivateVoting::storage_layout().vote_ended.slot;
+    let vote_ended_read: bool = storage_read(voting_contract_address, status_slot, block_number);
+    assert(vote_ended_expected == vote_ended_read, "Vote ended should be true");
+}
+
+#[test(should_fail)]
+unconstrained fn test_fail_end_vote_by_non_admin() {
+    let (env, voting_contract_address, _) = utils::setup();
+    let alice = env.create_account();
+
+    env.impersonate(alice);
+    EasyPrivateVoting::at(voting_contract_address).end_vote().call(&mut env.public());
+}
+
+#[test]
+unconstrained fn test_cast_vote() {
+    let (env, voting_contract_address, _) = utils::setup();
+    let alice = env.create_account();
+    env.impersonate(alice);
+
+    let candidate = 1;
+    env.advance_block_by(6);
+    EasyPrivateVoting::at(voting_contract_address).cast_vote(candidate).call(&mut env.private());
+
+    // Read vote count from storage
+    let block_number = get_block_number();
+    let tally_slot = EasyPrivateVoting::storage_layout().tally.slot;
+    let candidate_tally_slot = derive_storage_slot_in_map(tally_slot, candidate);
+    let vote_count: u32 = storage_read(voting_contract_address, candidate_tally_slot, block_number);
+
+    assert(vote_count == 1, "vote tally should be incremented");
+}
+
+#[test]
+unconstrained fn test_cast_vote_with_separate_accounts() {
+    let (env, voting_contract_address, _) = utils::setup();
+    let alice = env.create_account();
+    let bob = env.create_account();
+
+    let candidate = 101;
+
+    env.impersonate(alice);
+    env.advance_block_by(1);
+    EasyPrivateVoting::at(voting_contract_address).cast_vote(candidate).call(&mut env.private());
+
+    env.impersonate(bob);
+    env.advance_block_by(1);
+    EasyPrivateVoting::at(voting_contract_address).cast_vote(candidate).call(&mut env.private());
+
+    // Read vote count from storage
+    let block_number = get_block_number();
+    let tally_slot = EasyPrivateVoting::storage_layout().tally.slot;
+    let candidate_tally_slot = derive_storage_slot_in_map(tally_slot, candidate);
+    let vote_count: u32 = storage_read(voting_contract_address, candidate_tally_slot, block_number);
+
+    assert(vote_count == 2, "vote tally should be 2");
+}
+
+#[test(should_fail)]
+unconstrained fn test_fail_vote_twice() {
+    let (env, voting_contract_address, _) = utils::setup();
+    let alice = env.create_account();
+
+    let candidate = 101;
+
+    env.impersonate(alice);
+    env.advance_block_by(1);
+    EasyPrivateVoting::at(voting_contract_address).cast_vote(candidate).call(&mut env.private());
+
+    // Vote again as alice
+    env.advance_block_by(1);
+    EasyPrivateVoting::at(voting_contract_address).cast_vote(candidate).call(&mut env.private());
+}

noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/mod.nr

@@ -0,0 +1,2 @@
+mod first;
+mod utils;

noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/utils.nr

@@ -0,0 +1,21 @@
+use dep::aztec::{
+    note::{note_getter::{MAX_NOTES_PER_PAGE, view_notes}, note_viewer_options::NoteViewerOptions},
+    prelude::AztecAddress,
+    protocol_types::storage::map::derive_storage_slot_in_map,
+    test::helpers::test_environment::TestEnvironment,
+};
+
+use crate::EasyPrivateVoting;
+
+pub fn setup() -> (&mut TestEnvironment, AztecAddress, AztecAddress) {
+    let mut env = TestEnvironment::new();
+
+    let admin = env.create_account();
+
+    let initializer_call_interface = EasyPrivateVoting::interface().constructor(admin);
+    let voting_contract = env.deploy_self("EasyPrivateVoting").with_public_void_initializer(
+        initializer_call_interface,
+    );
+    // std::println(voting_contract);
+    (&mut env, voting_contract.to_address(), admin)
+}

yarn-project/txe/src/oracle/txe_oracle.ts

@@ -108,6 +108,7 @@ export class TXE implements TypedOracle {
 
   private uniqueNoteHashesFromPublic: Fr[] = [];
   private siloedNullifiersFromPublic: Fr[] = [];
+  private siloedNullifiersFromPrivate: Set<string> = new Set();
   private privateLogs: PrivateLog[] = [];
   private publicLogs: UnencryptedL2Log[] = [];
 
@@ -259,6 +260,22 @@ export class TXE implements TypedOracle {
     );
   }
 
+  async addNullifiersFromPrivate(contractAddress: AztecAddress, nullifiers: Fr[]) {
+    const siloedNullifiers = nullifiers.map(nullifier => siloNullifier(contractAddress, nullifier));
+    const db = await this.trees.getLatest();
+    const nullifierIndexesInTree = await db.findLeafIndices(
+      MerkleTreeId.NULLIFIER_TREE,
+      siloedNullifiers.map(n => n.toBuffer()),
+    );
+    const notInTree = nullifierIndexesInTree.every(index => index === undefined);
+    const notInCache = siloedNullifiers.every(n => !this.siloedNullifiersFromPrivate.has(n.toString()));
+    if (notInTree && notInCache) {
+      siloedNullifiers.forEach(n => this.siloedNullifiersFromPrivate.add(n.toString()));
+    } else {
+      throw new Error(`Rejecting tx for emitting duplicate nullifiers`);
+    }
+  }
+
   async addSiloedNullifiersFromPublic(siloedNullifiers: Fr[]) {
     this.siloedNullifiersFromPublic.push(...siloedNullifiers);
 
@@ -483,14 +500,16 @@ export class TXE implements TypedOracle {
     sortOrder: number[],
     limit: number,
     offset: number,
-    _status: NoteStatus,
+    status: NoteStatus,
   ) {
-    const syncedNotes = await this.simulatorOracle.getNotes(this.contractAddress, storageSlot, _status);
+    // Nullified pending notes are already removed from the list.
     const pendingNotes = this.noteCache.getNotes(this.contractAddress, storageSlot);
 
-    const notesToFilter = [...pendingNotes, ...syncedNotes];
+    const pendingNullifiers = this.noteCache.getNullifiers(this.contractAddress);
+    const dbNotes = await this.simulatorOracle.getNotes(this.contractAddress, storageSlot, status);
+    const dbNotesFiltered = dbNotes.filter(n => !pendingNullifiers.has((n.siloedNullifier as Fr).value));
 
-    const notes = pickNotes<NoteData>(notesToFilter, {
+    const notes = pickNotes<NoteData>([...dbNotesFiltered, ...pendingNotes], {
       selects: selectByIndexes.slice(0, numSelects).map((index, i) => ({
         selector: { index, offset: selectByOffsets[i], length: selectByLengths[i] },
         value: selectValues[i],
@@ -530,7 +549,8 @@ export class TXE implements TypedOracle {
     return Promise.resolve();
   }
 
-  notifyNullifiedNote(innerNullifier: Fr, noteHash: Fr, counter: number) {
+  async notifyNullifiedNote(innerNullifier: Fr, noteHash: Fr, counter: number) {
+    await this.addNullifiersFromPrivate(this.contractAddress, [innerNullifier]);
     this.noteCache.nullifyNote(this.contractAddress, innerNullifier, noteHash);
     this.sideEffectCounter = counter + 1;
     return Promise.resolve();
@@ -617,7 +637,10 @@ export class TXE implements TypedOracle {
         ),
       ...this.uniqueNoteHashesFromPublic,
     ];
-    txEffect.nullifiers = [new Fr(blockNumber + 6969), ...this.noteCache.getAllNullifiers()];
+    txEffect.nullifiers = [
+      new Fr(blockNumber + 6969),
+      ...Array.from(this.siloedNullifiersFromPrivate).map(n => Fr.fromString(n)),
+    ];
 
     // Using block number itself, (without adding 6969) gets killed at 1 as it says the slot is already used,
     // it seems like we commit a 1 there to the trees before ? To see what I mean, uncomment these lines below
@@ -636,6 +659,7 @@ export class TXE implements TypedOracle {
 
     this.privateLogs = [];
     this.publicLogs = [];
+    this.siloedNullifiersFromPrivate = new Set();
     this.uniqueNoteHashesFromPublic = [];
     this.siloedNullifiersFromPublic = [];
     this.noteCache = new ExecutionNoteCache(new Fr(1));
@@ -710,6 +734,24 @@ export class TXE implements TypedOracle {
       publicInputs.privateLogs.filter(privateLog => !privateLog.isEmpty()).map(privateLog => privateLog.log),
     );
 
+    const executionNullifiers = publicInputs.nullifiers
+      .filter(nullifier => !nullifier.isEmpty())
+      .map(nullifier => nullifier.value);
+    // We inject nullifiers into siloedNullifiersFromPrivate from notifyNullifiedNote,
+    // so top level calls to destroyNote work as expected. As such, we are certain
+    // that we would insert duplicates if we just took the nullifiers from the public inputs and
+    // blindly inserted them into siloedNullifiersFromPrivate. To avoid this, we extract the first
+    // (and only the first!) duplicated nullifier from the public inputs, so we can just push
+    // the ones that were not created by deleting a note
+    const firstDuplicateIndexes = executionNullifiers
+      .map((nullifier, index) => {
+        const siloedNullifier = siloNullifier(targetContractAddress, nullifier);
+        return this.siloedNullifiersFromPrivate.has(siloedNullifier.toString()) ? index : -1;
+      })
+      .filter(index => index !== -1);
+    const nonNoteNullifiers = executionNullifiers.filter((_, index) => !firstDuplicateIndexes.includes(index));
+    await this.addNullifiersFromPrivate(targetContractAddress, nonNoteNullifiers);
+
     this.setContractAddress(currentContractAddress);
     this.setMsgSender(currentMessageSender);
     this.setFunctionSelector(currentFunctionSelector);