diff --git a/noir-projects/aztec-nr/aztec/src/messages/processing/log_retrieval_response.nr b/noir-projects/aztec-nr/aztec/src/messages/processing/log_retrieval_response.nr index fda2f8f9968..1ecf16379af 100644 --- a/noir-projects/aztec-nr/aztec/src/messages/processing/log_retrieval_response.nr +++ b/noir-projects/aztec-nr/aztec/src/messages/processing/log_retrieval_response.nr @@ -1,5 +1,8 @@ use crate::messages::logs::note::MAX_NOTE_PACKED_LEN; -use crate::protocol::{constants::{MAX_NOTE_HASHES_PER_TX, PRIVATE_LOG_CIPHERTEXT_LEN}, traits::Deserialize}; +use crate::protocol::{ + constants::{MAX_NOTE_HASHES_PER_TX, PRIVATE_LOG_CIPHERTEXT_LEN}, + traits::{Deserialize, Serialize}, +}; pub global MAX_PUBLIC_LOG_LEN_FOR_NOTE_COMPLETION: u32 = MAX_NOTE_PACKED_LEN; @@ -11,7 +14,7 @@ pub(crate) global MAX_LOG_CONTENT_LEN: u32 = std::cmp::max( /// A response to a `LogRetrievalRequest`, containing the payload of a log (i.e. the content minus the tag, called /// plaintext for public logs and ciphertext for private), plus contextual information about the transaction in which /// the log was emitted. This is the data required in order to discover notes that are being delivered in a log. -#[derive(Deserialize, Eq)] +#[derive(Deserialize, Eq, Serialize)] pub struct LogRetrievalResponse { pub log_payload: BoundedVec, pub tx_hash: Field, diff --git a/noir-projects/aztec-nr/aztec/src/messages/processing/mod.nr b/noir-projects/aztec-nr/aztec/src/messages/processing/mod.nr index 4b4b71564f4..fb2823eb600 100644 --- a/noir-projects/aztec-nr/aztec/src/messages/processing/mod.nr +++ b/noir-projects/aztec-nr/aztec/src/messages/processing/mod.nr @@ -95,6 +95,13 @@ pub unconstrained fn enqueue_note_for_validation( )) } +/// The note validation requests enqueued so far in the current context via [`enqueue_note_for_validation`], before +/// [`validate_and_store_enqueued_notes_and_events`] drains them. Exists so tests can inspect the pending queue; +/// production code should not read requests back once enqueued. +pub(crate) unconstrained fn get_enqueued_note_validation_requests() -> EphemeralArray { + EphemeralArray::at(NOTE_VALIDATION_REQUESTS_ARRAY_BASE_SLOT) +} + /// Enqueues an event for validation and storage by PXE. /// /// This is the primary way for custom message handlers (registered via diff --git a/noir-projects/aztec-nr/aztec/src/partial_notes/fsm.nr b/noir-projects/aztec-nr/aztec/src/partial_notes/fsm.nr index f926b1b25fc..fdb734c3a5c 100644 --- a/noir-projects/aztec-nr/aztec/src/partial_notes/fsm.nr +++ b/noir-projects/aztec-nr/aztec/src/partial_notes/fsm.nr @@ -66,9 +66,10 @@ use crate::facts::{ delete_fact_collection, Fact, FactCollection, get_fact_collections_by_type, OriginBlock, record_retractable_fact, RetractableFactOrigin, }; -use crate::logging::aztecnr_debug_log_format; +use crate::logging::{aztecnr_debug_log_format, aztecnr_warn_log_format}; use crate::messages::{ discovery::{ComputeNoteHash, ComputeNoteNullifier, nonce_discovery::attempt_note_nonce_discovery}, + logs::note::MAX_NOTE_PACKED_LEN, processing::{enqueue_note_for_validation, log_retrieval_response::{LogRetrievalResponse, MAX_LOG_CONTENT_LEN}}, }; use crate::protocol::{address::AztecAddress, hash::{poseidon2_hash, sha256_to_field}, traits::{Deserialize, Serialize}}; @@ -213,7 +214,17 @@ impl PartialNoteFsm { let completion_logs = maybe_completion_logs.unwrap(); let num_logs = completion_logs.len(); if num_logs != 0 { - assert(num_logs == 1, f"Expected at most 1 completion log per partial note, got {num_logs}"); + // The tag is not unique, so a pending partial note can resolve to more than one completion log (e.g. + // the same partial note completed twice). We complete with the first and ignore the rest rather than + // fail. + if num_logs > 1 { + let tag = self.read_pending_partial_note().note_completion_log_tag; + aztecnr_warn_log_format!( + "Found {0} completion logs for partial note with tag {1}, completing with the first", + )( + [num_logs as Field, tag], + ); + } self.complete_with_log(completion_logs.get(0), compute_note_hash, compute_note_nullifier); } } @@ -232,6 +243,10 @@ impl PartialNoteFsm { /// Combines the delivered private content with the log's public content, discovers the resulting note(s), enqueues /// them for validation, and transitions the FSM to `completed` state conditional to the completion log's block. /// + /// A completion log that cannot yield a note (an empty payload, content too large to pack into a note, or one + /// matching no real note) is skipped without failing and the FSM still advances, so one bad message cannot break + /// sync. + /// /// Because the completion fact is retractable, a reorg of the completion log's block rewinds the FSM back to /// `pending` state (which signals that completion log search should restart). unconstrained fn complete_with_log( @@ -242,53 +257,76 @@ impl PartialNoteFsm { ) { let pending = self.read_pending_partial_note(); - // The first field in the completion log payload is the storage slot, followed by the public note content. - let storage_slot = log.log_payload.get(0); - let public_note_content: BoundedVec = array::subbvec(log.log_payload, 1); - - // Public fields are placed at the end of the packed representation, so we combine the private and public - // packed content to get the complete packed note. - let complete_packed_note = array::append(pending.packed_private_note_content, public_note_content); + // The completion log payload is the storage slot followed by the public note content, so an empty payload + // (e.g. an attacker's tag-only log) has no storage slot and no content. + let payload_len = log.log_payload.len(); + let public_content_len = if payload_len == 0 { 0 } else { payload_len - 1 }; - let discovered_notes = attempt_note_nonce_discovery( - log.unique_note_hashes_in_tx, - log.first_nullifier_in_tx, - compute_note_hash, - compute_note_nullifier, - self.collection.contract_address, - pending.owner, - storage_slot, - pending.randomness, - pending.note_type_id, - complete_packed_note, - ); + let combined_len = pending.packed_private_note_content.len() + public_content_len; - // TODO(#11627): is there anything reasonable we can do if we get a log but it doesn't result in a note being - // found? - if discovered_notes.len() == 0 { - panic( - f"A partial note's completion log did not result in any notes being found - this should never happen", + if payload_len == 0 { + aztecnr_warn_log_format!( + "Partial note with tag {0} has an empty completion log, ignoring", + )( + [pending.note_completion_log_tag], ); - } - - aztecnr_debug_log_format!("Discovered {0} notes for partial note with tag {1}")([ - discovered_notes.len() as Field, - pending.note_completion_log_tag, - ]); - - discovered_notes.for_each(|discovered_note| { - enqueue_note_for_validation( + } else if combined_len > MAX_NOTE_PACKED_LEN { + // A valid note packs into at most MAX_NOTE_PACKED_LEN fields, so this content cannot form one and would + // overflow the append below. + aztecnr_warn_log_format!( + "Partial note with tag {0} has over-length content ({1} > {2} fields), ignoring", + )( + [pending.note_completion_log_tag, combined_len as Field, MAX_NOTE_PACKED_LEN as Field], + ); + } else { + let storage_slot = log.log_payload.get(0); + let public_note_content: BoundedVec = array::subbvec(log.log_payload, 1); + + // Public fields are placed at the end of the packed representation, so we combine the private and public + // packed content to get the complete packed note. + let complete_packed_note = array::append(pending.packed_private_note_content, public_note_content); + + let discovered_notes = attempt_note_nonce_discovery( + log.unique_note_hashes_in_tx, + log.first_nullifier_in_tx, + compute_note_hash, + compute_note_nullifier, self.collection.contract_address, pending.owner, storage_slot, pending.randomness, - discovered_note.note_nonce, + pending.note_type_id, complete_packed_note, - discovered_note.note_hash, - discovered_note.inner_nullifier, - log.tx_hash, ); - }); + + // Content delivered over an unconstrained channel need not correspond to a real note, so discovery can + // come up empty. + if discovered_notes.len() == 0 { + aztecnr_warn_log_format!( + "Partial note with tag {0} completed with no matching note discovered, ignoring", + )( + [pending.note_completion_log_tag], + ); + } else { + aztecnr_debug_log_format!("Discovered {0} notes for partial note with tag {1}")( + [discovered_notes.len() as Field, pending.note_completion_log_tag], + ); + } + + discovered_notes.for_each(|discovered_note| { + enqueue_note_for_validation( + self.collection.contract_address, + pending.owner, + storage_slot, + pending.randomness, + discovered_note.note_nonce, + complete_packed_note, + discovered_note.note_hash, + discovered_note.inner_nullifier, + log.tx_hash, + ); + }); + } self.mark_completed(OriginBlock { block_number: log.block_number, block_hash: log.block_hash }); } @@ -308,9 +346,13 @@ mod test { use crate::ephemeral::EphemeralArray; use crate::facts::OriginBlock; use crate::messages::logs::note::MAX_NOTE_PACKED_LEN; - use crate::messages::processing::log_retrieval_response::LogRetrievalResponse; + use crate::messages::logs::partial_note::MAX_PARTIAL_NOTE_PRIVATE_PACKED_LEN; + use crate::messages::processing::{ + get_enqueued_note_validation_requests, log_retrieval_response::LogRetrievalResponse, + }; use crate::partial_notes::DeliveredPendingPartialNote; use crate::protocol::address::AztecAddress; + use crate::protocol::hash::{compute_note_hash_nonce, compute_siloed_note_hash, compute_unique_note_hash}; use crate::test::helpers::test_environment::TestEnvironment; use super::PartialNoteFsm; @@ -431,6 +473,156 @@ mod test { }); } + #[test] + unconstrained fn complete_with_log_advances_a_pending_reception_whose_log_yields_no_notes() { + let (env, scope) = setup(); + env.private_context(|context| { + let address = context.this_address(); + PartialNoteFsm::init(address, scope, make_pending(scope), finalized_block()); + + // A malicious sender can deliver a partial note whose private content does not match the completion log it + // points at: the note hash computes but is absent from the log's transaction, so nonce discovery yields no + // note. + PartialNoteFsm::load_all(address, scope).get(0).complete_with_log( + completion_log_with_unrelated_note_hashes(), + |_, _, _, _, _, _| Option::some(DISCOVERED_NOTE_HASH), + dummy_compute_note_nullifier, + ); + + let all = PartialNoteFsm::load_all(address, scope); + assert_eq(all.len(), 1); + assert(all.get(0).is_completed()); + assert_eq(enqueued_note_count(), 0); + }); + } + + #[test] + unconstrained fn complete_with_log_advances_when_combined_content_exceeds_note_packed_len() { + let (env, scope) = setup(); + env.private_context(|context| { + let address = context.this_address(); + + // A poisoned delivery can carry a private half as long as MAX_PARTIAL_NOTE_PRIVATE_PACKED_LEN, which equals + // the packed-note capacity. Combined with any completion log's public content it exceeds that capacity, so + // completing must advance the reception rather than overflow the packed-note append and panic. + let pending = DeliveredPendingPartialNote { + owner: scope, + randomness: 0x11, + note_completion_log_tag: 0x22, + note_type_id: 0x33, + packed_private_note_content: BoundedVec::from_array([0; MAX_PARTIAL_NOTE_PRIVATE_PACKED_LEN]), + }; + PartialNoteFsm::init(address, scope, pending, finalized_block()); + + // A completion log carrying one public field (storage slot followed by one value). + let mut log = completion_log_with_unrelated_note_hashes(); + log.log_payload = BoundedVec::from_array([0, 0]); + + PartialNoteFsm::load_all(address, scope).get(0).complete_with_log( + log, + dummy_compute_note_hash, + dummy_compute_note_nullifier, + ); + + let all = PartialNoteFsm::load_all(address, scope); + assert_eq(all.len(), 1); + assert(all.get(0).is_completed()); + assert_eq(enqueued_note_count(), 0); + }); + } + + #[test] + unconstrained fn complete_with_log_advances_when_the_log_payload_is_empty() { + let (env, scope) = setup(); + env.private_context(|context| { + let address = context.this_address(); + PartialNoteFsm::init(address, scope, make_pending(scope), finalized_block()); + + // An attacker can emit a tag-only public log, which arrives with an empty payload (no storage slot). + // Completing must advance the reception rather than read the storage slot out of bounds and panic. + let mut log = completion_log_with_unrelated_note_hashes(); + log.log_payload = BoundedVec::new(); + + PartialNoteFsm::load_all(address, scope).get(0).complete_with_log( + log, + dummy_compute_note_hash, + dummy_compute_note_nullifier, + ); + + let all = PartialNoteFsm::load_all(address, scope); + assert_eq(all.len(), 1); + assert(all.get(0).is_completed()); + assert_eq(enqueued_note_count(), 0); + }); + } + + #[test] + unconstrained fn complete_with_log_discovers_a_note_whose_combined_content_fills_note_packed_len() { + let (env, scope) = setup(); + env.private_context(|context| { + let address = context.this_address(); + + // A private half one field short of the cap plus a single public field combines to exactly + // MAX_NOTE_PACKED_LEN, the largest a valid note can be. The guard rejects only content that *exceeds* the + // cap, so this boundary case must still go through discovery and not be dropped. + let mut packed_private_note_content: BoundedVec = + BoundedVec::new(); + for _ in 0..MAX_PARTIAL_NOTE_PRIVATE_PACKED_LEN - 1 { + packed_private_note_content.push(0); + } + let pending = DeliveredPendingPartialNote { + owner: scope, + randomness: 0x11, + note_completion_log_tag: 0x22, + note_type_id: 0x33, + packed_private_note_content, + }; + PartialNoteFsm::init(address, scope, pending, finalized_block()); + + // A matching completion log carrying one public field (storage slot followed by one value). + let mut log = matching_completion_log(address, COMPLETION_LOG_FIRST_NULLIFIER); + log.log_payload = BoundedVec::from_array([0, 0]); + + PartialNoteFsm::load_all(address, scope).get(0).complete_with_log( + log, + |_, _, _, _, _, _| Option::some(DISCOVERED_NOTE_HASH), + |_, _, _, _, _, _, _| Option::some(DISCOVERED_NOTE_NULLIFIER), + ); + + let all = PartialNoteFsm::load_all(address, scope); + assert_eq(all.len(), 1); + assert(all.get(0).is_completed()); + assert_eq(enqueued_note_count(), 1); + }); + } + + #[test] + unconstrained fn step_completes_once_when_multiple_completion_logs_are_found() { + let (env, scope) = setup(); + env.private_context(|context| { + let address = context.this_address(); + PartialNoteFsm::init(address, scope, make_pending(scope), finalized_block()); + + // A single pending partial note can resolve to more than one completion log (e.g. the same partial note + // completed twice). Both logs here would independently discover a note, but step must complete only once + // (enqueuing exactly one note) rather than panic, so this cannot break sync. + let completion_logs: EphemeralArray = EphemeralArray::empty(); + completion_logs.push(matching_completion_log(address, COMPLETION_LOG_FIRST_NULLIFIER)); + completion_logs.push(matching_completion_log(address, SECOND_COMPLETION_LOG_FIRST_NULLIFIER)); + + PartialNoteFsm::load_all(address, scope).get(0).step( + Option::some(completion_logs), + |_, _, _, _, _, _| Option::some(DISCOVERED_NOTE_HASH), + |_, _, _, _, _, _, _| Option::some(DISCOVERED_NOTE_NULLIFIER), + ); + + let all = PartialNoteFsm::load_all(address, scope); + assert_eq(all.len(), 1); + assert(all.get(0).is_completed()); + assert_eq(enqueued_note_count(), 1); + }); + } + #[test] unconstrained fn step_terminates_a_completed_reception_once_its_block_is_finalized() { let (env, scope) = setup(); @@ -474,6 +666,54 @@ mod test { }); } + global COMPLETION_LOG_FIRST_NULLIFIER: Field = 0x47; + global SECOND_COMPLETION_LOG_FIRST_NULLIFIER: Field = 0x48; + global DISCOVERED_NOTE_HASH: Field = 0x99; + global DISCOVERED_NOTE_NULLIFIER: Field = 0x55; + + /// The number of notes `complete_with_log` has enqueued for validation in the current context. + unconstrained fn enqueued_note_count() -> u32 { + get_enqueued_note_validation_requests().len() + } + + /// A completion log whose transaction contains the unique note hash that `DISCOVERED_NOTE_HASH` resolves to for the + /// given `first_nullifier_in_tx`, so nonce discovery finds a note in it. Paired with compute functions that return + /// `DISCOVERED_NOTE_HASH`. + unconstrained fn matching_completion_log( + contract_address: AztecAddress, + first_nullifier_in_tx: Field, + ) -> LogRetrievalResponse { + let note_nonce = compute_note_hash_nonce(first_nullifier_in_tx, 0); + let unique_note_hash = compute_unique_note_hash( + note_nonce, + compute_siloed_note_hash(contract_address, DISCOVERED_NOTE_HASH), + ); + LogRetrievalResponse { + log_payload: BoundedVec::from_array([0]), + tx_hash: 0, + unique_note_hashes_in_tx: BoundedVec::from_array([unique_note_hash]), + first_nullifier_in_tx, + block_number: 1, + block_timestamp: 0, + block_hash: 0, + } + } + + /// A well-formed completion log tied to a block TXE reports as finalized, carrying the note hashes of some + /// unrelated transaction. A note whose hash is not among them discovers to nothing, standing in for the real log a + /// poisoned partial note's tag resolves to. + unconstrained fn completion_log_with_unrelated_note_hashes() -> LogRetrievalResponse { + LogRetrievalResponse { + log_payload: BoundedVec::from_array([0]), + tx_hash: 0, + unique_note_hashes_in_tx: BoundedVec::from_array([0x1234]), + first_nullifier_in_tx: COMPLETION_LOG_FIRST_NULLIFIER, + block_number: 1, + block_timestamp: 0, + block_hash: 0, + } + } + unconstrained fn dummy_compute_note_hash( _packed_note: BoundedVec, _owner: AztecAddress, diff --git a/noir-projects/contract-snapshots/tests/snapshots/expand/token_contract/snapshots__expanded.snap b/noir-projects/contract-snapshots/tests/snapshots/expand/token_contract/snapshots__expanded.snap index 5e277ac07d6..c69025b6d66 100644 --- a/noir-projects/contract-snapshots/tests/snapshots/expand/token_contract/snapshots__expanded.snap +++ b/noir-projects/contract-snapshots/tests/snapshots/expand/token_contract/snapshots__expanded.snap @@ -4048,6 +4048,18 @@ mod test { utils::check_private_balance(env, token_contract_address, recipient, amount); } + /// A partial note can be completed more than once (`complete` only existence-checks its validity commitment, which is + /// never consumed), so both finalizes emit a completion log under the same tag. Discovery tolerates the two logs + /// rather than failing: it completes with the first (recovering that note) and ignores the rest. + #[test] + unconstrained fn discovery_tolerates_a_partial_note_completed_twice() { + let (env, token_contract_address, owner, recipient, _): (aztec::test::helpers::test_environment::TestEnvironment, aztec::protocol::address::AztecAddress, aztec::protocol::address::AztecAddress, aztec::protocol::address::AztecAddress, u128) = utils::setup_and_mint_to_public(false); + let partial_uint_note: PartialUintNote = env.call_private(owner, Token::at(token_contract_address).prepare_private_balance_increase(recipient)); + env.call_public(owner, Token::at(token_contract_address).finalize_transfer_to_private(1_u128, partial_uint_note)); + env.call_public(owner, Token::at(token_contract_address).finalize_transfer_to_private(1_u128, partial_uint_note)); + utils::check_private_balance(env, token_contract_address, recipient, 1_u128); + } + #[test(should_fail_with = "Invalid partial note or completer")] unconstrained fn transfer_to_private_transfer_not_prepared() { let (env, token_contract_address, owner, _, amount): (aztec::test::helpers::test_environment::TestEnvironment, aztec::protocol::address::AztecAddress, aztec::protocol::address::AztecAddress, aztec::protocol::address::AztecAddress, u128) = utils::setup_and_mint_to_public(false); diff --git a/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr b/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr index 386f0b68e78..e7b091d6404 100644 --- a/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr +++ b/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr @@ -82,6 +82,23 @@ unconstrained fn transfer_to_private_external_orchestration_multiple_notes() { utils::check_private_balance(env, token_contract_address, recipient, amount); } +/// A partial note can be completed more than once (`complete` only existence-checks its validity commitment, which is +/// never consumed), so both finalizes emit a completion log under the same tag. Discovery tolerates the two logs +/// rather than failing: it completes with the first (recovering that note) and ignores the rest. +#[test] +unconstrained fn discovery_tolerates_a_partial_note_completed_twice() { + let (env, token_contract_address, owner, recipient, _) = + utils::setup_and_mint_to_public(/* with_account_contracts */ false); + + let partial_uint_note = + env.call_private(owner, Token::at(token_contract_address).prepare_private_balance_increase(recipient)); + + env.call_public(owner, Token::at(token_contract_address).finalize_transfer_to_private(1, partial_uint_note)); + env.call_public(owner, Token::at(token_contract_address).finalize_transfer_to_private(1, partial_uint_note)); + + utils::check_private_balance(env, token_contract_address, recipient, 1); +} + #[test(should_fail_with = "Invalid partial note or completer")] unconstrained fn transfer_to_private_transfer_not_prepared() { // Setup without account contracts. We are not using authwits here, so dummy accounts are enough