From 7f701105c2ac44df9cafedc834d77d4eabd92710 Mon Sep 17 00:00:00 2001 From: Gregorio Juliana Date: Tue, 17 Dec 2024 17:03:23 +0100 Subject: [PATCH] feat: TXE detects duplicate nullifiers (#10764) This functionality was lost in a refactor. Added tests to verify --- .../contracts/counter_contract/src/main.nr | 8 +- .../easy_private_voting_contract/src/main.nr | 1 + .../src/test/first.nr | 111 ++++++++++++++++++ .../src/test/mod.nr | 2 + .../src/test/utils.nr | 21 ++++ yarn-project/txe/src/oracle/txe_oracle.ts | 54 ++++++++- 6 files changed, 187 insertions(+), 10 deletions(-) create mode 100644 noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/first.nr create mode 100644 noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/mod.nr create mode 100644 noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/utils.nr diff --git a/noir-projects/noir-contracts/contracts/counter_contract/src/main.nr b/noir-projects/noir-contracts/contracts/counter_contract/src/main.nr index 65110e8c61b..f15d51d2249 100644 --- a/noir-projects/noir-contracts/contracts/counter_contract/src/main.nr +++ b/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 = 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 = view_notes(owner_slot, options); assert(get_counter(owner) == 6); diff --git a/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/main.nr b/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/main.nr index eb1d7c4af50..c7f51654c76 100644 --- a/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/main.nr @@ -1,3 +1,4 @@ +mod test; use dep::aztec::macros::aztec; #[aztec] diff --git a/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/first.nr b/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/first.nr new file mode 100644 index 00000000000..fa1c63cb920 --- /dev/null +++ b/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()); +} diff --git a/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/mod.nr b/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/mod.nr new file mode 100644 index 00000000000..6fde1dc2625 --- /dev/null +++ b/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/mod.nr @@ -0,0 +1,2 @@ +mod first; +mod utils; diff --git a/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/utils.nr b/noir-projects/noir-contracts/contracts/easy_private_voting_contract/src/test/utils.nr new file mode 100644 index 00000000000..51082b188aa --- /dev/null +++ b/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) +} diff --git a/yarn-project/txe/src/oracle/txe_oracle.ts b/yarn-project/txe/src/oracle/txe_oracle.ts index ac5792a5b7c..ec4c7818525 100644 --- a/yarn-project/txe/src/oracle/txe_oracle.ts +++ b/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 = 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(notesToFilter, { + const notes = pickNotes([...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);