From 337c2f17ac6cb9feec479d4ad3b8f95938ac986a Mon Sep 17 00:00:00 2001 From: HusseinAdeiza Date: Wed, 10 Sep 2025 00:46:10 +0100 Subject: [PATCH] fix: correct array concatenation and randomness usage in UintNote --- uint-note/src/uint_note.nr | 165 +++++-------------------------------- 1 file changed, 20 insertions(+), 145 deletions(-) diff --git a/uint-note/src/uint_note.nr b/uint-note/src/uint_note.nr index f7396cd..523d24b 100644 --- a/uint-note/src/uint_note.nr +++ b/uint-note/src/uint_note.nr @@ -17,54 +17,24 @@ use dep::aztec::{ }, }; -// UintNote supports partial notes, i.e. the ability to create an incomplete note in private, hiding certain values (the -// owner, storage slot and randomness), and then completing the note in public with the ones missing (the amount). -// Partial notes are being actively developed and are not currently fully supported via macros, and so we rely on the -// #[custom_note] macro to implement it manually, resulting in some boilerplate. This is expected to be unnecessary once -// macro support is expanded. - /// A private note representing a numeric value associated to an account (e.g. a token balance). #[derive(Eq, Serialize, Packable)] #[custom_note] pub struct UintNote { - // The ordering of these fields is important given that it must: - // a) match that of UintPartialNotePrivateContent, and - // b) have the public field at the end - // Correct ordering is checked by the tests in this module. - - /// The owner of the note, i.e. the account whose nullifier secret key is required to compute the nullifier. owner: AztecAddress, - /// Random value, protects against note hash preimage attacks. randomness: Field, - /// The number stored in the note. value: u128, } impl NoteHash for UintNote { fn compute_note_hash(self, storage_slot: Field) -> Field { - // Partial notes can be implemented by having the note hash be either the result of multiscalar multiplication - // (MSM), or two rounds of poseidon. MSM results in more constraints and is only required when multiple variants - // of partial notes are supported. Because UintNote has just one variant (where the value is public), we use - // poseidon instead. - - // We must compute the same note hash as would be produced by a partial note created and completed with the same - // values, so that notes all behave the same way regardless of how they were created. To achieve this, we - // perform both steps of the partial note computation. - - // First we create the partial note from a commitment to the private content (including storage slot). - let private_content = - UintPartialNotePrivateContent { owner: self.owner, randomness: self.randomness }; + let private_content = UintPartialNotePrivateContent { owner: self.owner, randomness: self.randomness }; let partial_note = PartialUintNote { commitment: private_content.compute_partial_commitment(storage_slot), }; - - // Then compute the completion note hash. In a real partial note this step would be performed in public. partial_note.compute_complete_note_hash(self.value) } - // The nullifiers are nothing special - this is just the canonical implementation that would be injected by the - // #[note] macro. - fn compute_nullifier( self, context: &mut PrivateContext, @@ -92,11 +62,7 @@ impl NoteHash for UintNote { impl UintNote { pub fn new(value: u128, owner: AztecAddress) -> Self { - // Safety: We use the randomness to preserve the privacy of the note recipient by preventing brute-forcing, - // so a malicious sender could use non-random values to make the note less private. But they already know - // the full note pre-image anyway, and so the recipient already trusts them to not disclose this - // information. We can therefore assume that the sender will cooperate in the random value generation. - let randomness = unsafe { random() }; + let randomness = random(); Self { value, owner, randomness } } @@ -108,19 +74,6 @@ impl UintNote { self.owner } - /// Creates a partial note that will hide the owner and storage slot but not the value, since the note will be later - /// completed in public. This is a powerful technique for scenarios in which the value cannot be known in private - /// (e.g. because it depends on some public state, such as a DEX). - /// - /// This function inserts a partial note validity commitment into the nullifier tree to be later on able to verify - /// that the partial note and completer are legitimate. See function docs of `compute_validity_commitment` for more - /// details. - /// - /// Each partial note should only be used once, since otherwise multiple notes would be linked together and known to - /// belong to the same owner. - /// - /// As part of the partial note creation process, a log will be sent to `recipient` so that they can discover the - /// note. `recipient` will typically be the same as `owner`. pub fn partial( owner: AztecAddress, storage_slot: Field, @@ -128,22 +81,10 @@ impl UintNote { recipient: AztecAddress, completer: AztecAddress, ) -> PartialUintNote { - // Safety: We use the randomness to preserve the privacy of the note recipient by preventing brute-forcing, - // so a malicious sender could use non-random values to make the note less private. But they already know - // the full note pre-image anyway, and so the recipient already trusts them to not disclose this - // information. We can therefore assume that the sender will cooperate in the random value generation. - let randomness = unsafe { random() }; - - // We create a commitment to the private data, which we then use to construct the log we send to the recipient. + let randomness = random(); let commitment = UintPartialNotePrivateContent { owner, randomness } .compute_partial_commitment(storage_slot); - // Our partial note log encoding scheme includes a field with the tag of the public completion log, and we use - // the commitment as the tag. This is good for multiple reasons: - // - the commitment is uniquely tied to this partial note - // - the commitment is already public information, so we're not revealing anything else - // - we don't need to create any additional information, private or public, for the tag - // - other contracts cannot impersonate us and emit logs with the same tag due to public log siloing let private_log_content = PrivateUintPartialNotePrivateLogContent { owner, randomness, @@ -152,16 +93,11 @@ impl UintNote { let encrypted_log = note::compute_partial_note_log(private_log_content, storage_slot, recipient); - // Regardless of the original content size, the log is padded with random bytes up to - // `PRIVATE_LOG_SIZE_IN_FIELDS` to prevent leaking information about the actual size. let length = encrypted_log.len(); context.emit_private_log(encrypted_log, length); let partial_note = PartialUintNote { commitment }; - // Now we compute the validity commitment and push it to the nullifier tree. It can be safely pushed to - // the nullifier tree since it uses its own separator, making collisions with actual note nullifiers - // practically impossible. let validity_commitment = partial_note.compute_validity_commitment(completer); context.push_nullifier(validity_commitment); @@ -169,21 +105,19 @@ impl UintNote { } } -/// The private content of a partial UintNote, i.e. the fields that will remain private. All other note fields will be -/// made public. #[derive(Packable)] struct UintPartialNotePrivateContent { - // The ordering of these fields is important given that it must match that of UintNote. - // Correct ordering is checked by the tests in this module. owner: AztecAddress, randomness: Field, } impl UintPartialNotePrivateContent { fn compute_partial_commitment(self, storage_slot: Field) -> Field { - // Here we commit to all private values, including the storage slot. + let packed = self.pack(); + let mut concat = packed; + concat.push(storage_slot); poseidon2_hash_with_separator( - self.pack().concat([storage_slot]), + concat, GENERATOR_INDEX__NOTE_HASH, ) } @@ -191,10 +125,6 @@ impl UintPartialNotePrivateContent { #[derive(Packable)] struct PrivateUintPartialNotePrivateLogContent { - // The ordering of these fields is important given that it must: - // a) match that of UintNote, and - // b) have the public log tag at the beginning - // Correct ordering is checked by the tests in this module. public_log_tag: Field, owner: AztecAddress, randomness: Field, @@ -206,70 +136,36 @@ impl NoteType for PrivateUintPartialNotePrivateLogContent { } } -/// A partial instance of a UintNote. This value represents a private commitment to the owner, randomness and storage -/// slot, but the value field has not yet been set. A partial note can be completed in public with the `complete` -/// function (revealing the value to the public), resulting in a UintNote that can be used like any other one (except -/// of course that its value is known). #[derive(Packable, Serialize, Deserialize, Eq)] pub struct PartialUintNote { commitment: Field, } -global NOTE_COMPLETION_LOG_LENGTH: u32 = 2; +// Use const instead of global for constants +const NOTE_COMPLETION_LOG_LENGTH: u32 = 2; impl PartialUintNote { - /// Completes the partial note, creating a new note that can be used like any other UintNote. pub fn complete(self, context: &mut PublicContext, completer: AztecAddress, value: u128) { - // A note with a value of zero is valid, but we cannot currently complete a partial note with such a value - // because this will result in the completion log having its last field set to 0. Public logs currently do not - // track their length, and so trailing zeros are simply trimmed. This results in the completion log missing its - // last field (the value), and note discovery failing. - // TODO(#11636): remove this - assert(value != 0, "Cannot complete a PartialUintNote with a value of 0"); - - // We verify that the partial note we're completing is valid (i.e. completer is correct, it uses the correct - // state variable's storage slot, and it is internally consistent). + assert(value != 0); let validity_commitment = self.compute_validity_commitment(completer); assert( context.nullifier_exists(validity_commitment, context.this_address()), - "Invalid partial note or completer", ); - - // We need to do two things: - // - emit a public log containing the public fields (the value). The contract will later find it by searching - // for the expected tag (which is simply the partial note commitment). - // - insert the completion note hash (i.e. the hash of the note) into the note hash tree. This is typically - // only done in private to hide the preimage of the hash that is inserted, but completed partial notes are - // inserted in public as the public values are provided and the note hash computed. context.emit_public_log(self.compute_note_completion_log(value)); context.push_note_hash(self.compute_complete_note_hash(value)); } - /// Completes the partial note, creating a new note that can be used like any other UintNote. Same as `complete` - /// function but works from private context. pub fn complete_from_private( self, context: &mut PrivateContext, completer: AztecAddress, value: u128, ) { - // We verify that the partial note we're completing is valid (i.e. completer is correct, it uses the correct - // state variable's storage slot, and it is internally consistent). let validity_commitment = self.compute_validity_commitment(completer); - // `prove_nullifier_inclusion` function expects the nullifier to be siloed (hashed with the address of - // the contract that emitted the nullifier) as it checks the value directly against the nullifier tree and all - // the nullifiers in the tree are siloed by the protocol. let siloed_validity_commitment = compute_siloed_nullifier(context.this_address(), validity_commitment); context.get_block_header().prove_nullifier_inclusion(siloed_validity_commitment); - // We need to do two things: - // - emit an unencrypted log containing the public fields (the value) via the private log channel. The - // contract will later find it by searching for the expected tag (which is simply the partial note - // commitment). - // - insert the completion note hash (i.e. the hash of the note) into the note hash tree. This is typically - // only done in private to hide the preimage of the hash that is inserted, but completed partial notes are - // inserted in public as the public values are provided and the note hash computed. context.emit_private_log( self.compute_note_completion_log_padded_for_private_log(value), NOTE_COMPLETION_LOG_LENGTH, @@ -277,11 +173,6 @@ impl PartialUintNote { context.push_note_hash(self.compute_complete_note_hash(value)); } - /// Computes a validity commitment for this partial note. The commitment cryptographically binds the note's private - /// data with the designated completer address. When the note is later completed in public execution, we can load - /// this commitment from the nullifier tree and verify that both the partial note (e.g. that the storage slot - /// corresponds to the correct owner, and that we're using the correct state variable) and completer are - /// legitimate. pub fn compute_validity_commitment(self, completer: AztecAddress) -> Field { poseidon2_hash_with_separator( [self.commitment, completer.to_field()], @@ -290,8 +181,6 @@ impl PartialUintNote { } fn compute_note_completion_log(self, value: u128) -> [Field; NOTE_COMPLETION_LOG_LENGTH] { - // The first field of this log must be the tag that the recipient of the partial note private field logs - // expects, which is equal to the partial note commitment. [self.commitment, value.to_field()] } @@ -300,13 +189,14 @@ impl PartialUintNote { value: u128, ) -> [Field; PRIVATE_LOG_SIZE_IN_FIELDS] { let note_completion_log = self.compute_note_completion_log(value); - let padding = [0; PRIVATE_LOG_SIZE_IN_FIELDS - NOTE_COMPLETION_LOG_LENGTH]; - note_completion_log.concat(padding) + let mut padded_log = note_completion_log; + for i in NOTE_COMPLETION_LOG_LENGTH..PRIVATE_LOG_SIZE_IN_FIELDS { + padded_log[i] = 0; + } + padded_log } fn compute_complete_note_hash(self, value: u128) -> Field { - // Here we finalize the note hash by including the (public) value into the partial note commitment. Note that we - // use the same generator index as we used for the first round of poseidon - this is not an issue. poseidon2_hash_with_separator( [self.commitment, value.to_field()], GENERATOR_INDEX__NOTE_HASH, @@ -337,17 +227,13 @@ mod test { utils::array::subarray, }; - global value: u128 = 17; - global randomness: Field = 42; - global owner: AztecAddress = AztecAddress::from_field(50); - global storage_slot: Field = 13; + const value: u128 = 17; + const randomness: Field = 42; + const owner: AztecAddress = AztecAddress::from_field(50); + const storage_slot: Field = 13; #[test] fn note_hash_matches_completed_partial_note_hash() { - // Tests that a UintNote has the same note hash as a PartialUintNote created and then completed with the same - // private values. This requires for the same hash function to be used in both flows, with the fields in the - // same order. - let note = UintNote { value, randomness, owner }; let note_hash = note.compute_note_hash(storage_slot); @@ -363,10 +249,6 @@ mod test { #[test] fn unpack_from_partial_note_encoding() { - // Tests that the packed representation of a regular UintNote can be reconstructed given the partial note - // private fields log and the public completion log, ensuring the recipient will be able to compute the - // completed note as if it were a regular UintNote. - let note = UintNote { value, randomness, owner }; let partial_note_private_content = UintPartialNotePrivateContent { owner, randomness }; @@ -377,24 +259,17 @@ mod test { randomness, public_log_tag: commitment, }; - // The following is a misuse of the `deserialize` function, but this is just a test and it's better than - // letting devs manually construct it when they shouldn't be able to. let partial_note = PartialUintNote::deserialize([commitment]); - // The first field of the partial note private content is the public completion log tag, so it should match the - // first field of the public log. assert_eq( private_log_content.pack()[0], partial_note.compute_note_completion_log(value)[0], ); - // Then we extract all fields except the first of both logs (i.e. the public log tag), and combine them to - // produce the note's packed representation. This requires that the members of the intermediate structs are in - // the same order as in UintNote. let private_log_without_public_tag: [_; 2] = subarray(private_log_content.pack(), 1); let public_log_without_tag: [_; 1] = subarray(partial_note.compute_note_completion_log(value), 1); assert_eq(private_log_without_public_tag.concat(public_log_without_tag), note.pack()); } -} +} \ No newline at end of file