AztecProtocol · LeilaWang · Mar 6, 2026 · nventuro · Mar 6, 2026 · nventuro
@@ -2,7 +2,7 @@ use crate::protocol::{hash::poseidon2_hash_bytes, traits::{Deserialize, Empty, F
 
 #[derive(Eq, Serialize, Deserialize)]
 pub struct AuthorizationSelector {
-    // 1st 4-bytes (big-endian leftmost) of abi-encoding of an event.
+    // Low 32 bits of the poseidon2 hash of the function signature.
     inner: u32,
 }
 

@@ -29,6 +29,11 @@ impl FunctionCall {
         for i in 0..32 {
             bytes[i] = args_hash_bytes[i];
         }
+        // Q: The function selector is u32, so we should be able to just take the last 4 bytes.
+        // If it's used for hashing with sha256 (as suggested in DAppPayload.to_be_bytes()), it could save quite a bit
+        // of gates.
+        // But the code doesn't seem to be used anywhere. Hashing of the payload uses serialize() and poseidon instead.
+        // Do we want to keep this?
         let function_selector_bytes: [u8; 32] = self.function_selector.to_field().to_be_bytes();
         for i in 0..32 {
             bytes[i + 32] = function_selector_bytes[i];

@@ -1,2 +1,3 @@
+// TODO: Can be deleted. Not used anywhere.
 // protocl global vars are equal to the private ones so we just re-export them here under a different name
 pub use crate::protocol::abis::global_variables::GlobalVariables as PublicGlobalVariables;
@@ -40,7 +40,7 @@ use crate::protocol::{
         MAX_PRIVATE_CALL_STACK_LENGTH_PER_CALL, MAX_PRIVATE_LOGS_PER_CALL, MAX_TX_LIFETIME,
         NULL_MSG_SENDER_CONTRACT_ADDRESS, PRIVATE_LOG_SIZE_IN_FIELDS,
     },
-    hash::poseidon2_hash,
+    hash::compute_contract_class_log_hash,
     messaging::l2_to_l1_message::L2ToL1Message,
     side_effect::{Counted, scoped::Scoped},
     traits::{Empty, Hash, ToField},
@@ -575,6 +575,12 @@ impl PrivateContext {
     pub fn end_setup(&mut self) {
         // Incrementing the side effect counter when ending setup ensures non ambiguity for the counter where we change
         // phases.
+        // TODO: Can remove this manually incrementing the side effect counter.
+        // It's not necessary to increment the side effect counter here, because the side effect counter is always
+        // incremented by the next_counter function every time a side effect is emitted.
+        // This was added because the code below used to be:
+        // `self.min_revertible_side_effect_counter = self.side_effect_counter;`
+        // which made it necessary to increment the side effect counter manually.
         self.side_effect_counter += 1;
         // crate::protocol::logging::debug_log_format(
         //     "Ending setup at counter {0}",
@@ -949,10 +955,10 @@ impl PrivateContext {
         // Safety: The below length is constrained in the base rollup, which will make sure that all the fields beyond
         // length are zero. However, it won't be able to check that we didn't add extra padding (trailing zeroes) or
         // that we cut trailing zeroes from the end.
-        let length = unsafe { trimmed_array_length_hint(log_to_emit) };
+        let length = unsafe { trimmed_array_length_hint(log) };
         // We hash the entire padded log to ensure a user cannot pass a shorter length and so emit incorrect shorter
         // bytecode.
-        let log_hash = poseidon2_hash(log_to_emit);
+        let log_hash = compute_contract_class_log_hash(log_to_emit);
         // Safety: the below only exists to broadcast the raw log, so we can provide it to the base rollup later to be
         // constrained.
         unsafe {
@@ -1110,6 +1116,9 @@ impl PrivateContext {
         args_hash: Field,
         is_static_call: bool,
     ) -> ReturnsHash {
+        // Q: It can be misleading to make a static call when the caller specifically requests a non-static call
+        // (`is_static_call == false`). Although this is not allowed and will cause the kernel to fail.
+        // But rather than altering the user's intent, throw an error from the simulator might be better?
         let is_static_call = is_static_call | self.inputs.call_context.is_static_call;
         let start_side_effect_counter = self.side_effect_counter;
 
@@ -1143,14 +1152,11 @@ impl PrivateContext {
             },
         );
 
-        // TODO (fees) figure out why this crashes the prover and enable it we need this in order to pay fees inside
-        // child call contexts assert(
-        //     (item.public_inputs.min_revertible_side_effect_counter == 0 as u32)
-        //     | (item.public_inputs.min_revertible_side_effect_counter
-        //         > self.min_revertible_side_effect_counter)
-        // ); if item.public_inputs.min_revertible_side_effect_counter
-        //     > self.min_revertible_side_effect_counter { self.min_revertible_side_effect_counter =
-        // item.public_inputs.min_revertible_side_effect_counter; }
+        // The kernel circuits ensure that end_side_effect_counter is greater than start_side_effect_counter,
+        // and that all side effects emitted in the child call have counters within the range
+        // [start_side_effect_counter, end_side_effect_counter].
+        // Therefore, we only need to ensure that the next side effect from the current call starts after the
+        // end side effect from the child call.
         self.side_effect_counter = end_side_effect_counter + 1; // TODO: call `next_counter`
         // instead, for consistency
         ReturnsHash::new(returns_hash)

@@ -2,7 +2,7 @@ use crate::protocol::{hash::poseidon2_hash_bytes, traits::{Deserialize, Empty, F
 
 #[derive(Deserialize, Eq, Serialize)]
 pub struct EventSelector {
-    // 1st 4-bytes (big-endian leftmost) of abi-encoding of an event.
+    // Low 32 bits of the poseidon2 hash of the function signature.
     inner: u32,
 }
 

@@ -104,6 +104,8 @@ pub fn assert_nullifier_did_not_exist_by(block_header: BlockHeader, siloed_nulli
     );
 
     // 4) Prove that the low leaf is indeed smaller than the nullifier
+    // Q: Maybe use low_nullifier_leaf.nullifier.lt(siloed_nullifier) directly instead?
+    // We should probably remove full_field_less_than and full_field_greater_than.
     assert(
         full_field_less_than(low_nullifier_leaf.nullifier, siloed_nullifier),
         "Proving nullifier non-inclusion failed: low_nullifier.value < nullifier.value check failed",
@@ -112,6 +114,7 @@ pub fn assert_nullifier_did_not_exist_by(block_header: BlockHeader, siloed_nulli
     // 5) Prove that the low leaf is pointing "over" the nullifier, which means that the nullifier is not included in
     // the nullifier tree, since if it were it'd need to be between the low leaf and the next leaf. Note the special
     // case in which the low leaf is the largest of all entries, in which case there's no 'next' entry.
+    // Q: Same here, siloed_nullifier.lt(low_nullifier_leaf.next_nullifier) should be easier to read?
     assert(
         full_field_greater_than(low_nullifier_leaf.next_nullifier, siloed_nullifier)
             | (low_nullifier_leaf.next_index == 0),

@@ -84,5 +84,17 @@ where
         note_hash_for_nullification
     };
 
+    // Note: The `confirmed_note`'s metadata (including its phase stage) is determined at the time the note is
+    // fetched via `get_note`/`get_notes`. If the execution phase changes between fetching and destroying the note
+    // (e.g. via `end_setup()` or a child call that triggers it), the metadata becomes stale: a note that was
+    // `PENDING_SAME_PHASE` at fetch time is now effectively `PENDING_PREVIOUS_PHASE`, but the metadata still says
+    // `PENDING_SAME_PHASE`. This causes the wrong note hash to be used for nullification (unsiloed instead of
+    // unique), and the kernel will reject the transaction because it cannot match the nullifier to a pending note
+    // hash in the same phase.
+    //
+    // This only affects `#[allow_phase_change]` functions, since all other functions assert that the phase does not
+    // change during execution. If you are writing an `#[allow_phase_change]` function, ensure that all
+    // `get_note`/`get_notes` and corresponding `destroy_note` calls happen on the same side of the phase boundary.
+
     context.push_nullifier_for_note_hash(nullifier, note_hash)
 }
@@ -33,6 +33,8 @@ fn extract_property_value_from_selector<let N: u32>(packed_note: [Field; N], sel
     let mut acc: Field = 1;
     for i in 0..32 {
         if i < length {
+            // TODO: Any non-zero offset will cause the index to be out of bounds.
+            // Is it supposed to be `31 - offset - 1`?
             value_field += value[(31 + offset - i) as u32] as Field * acc;
             acc = acc * 256;
         }
@@ -41,6 +43,12 @@ fn extract_property_value_from_selector<let N: u32>(packed_note: [Field; N], sel
 }
 
 fn check_packed_note<let N: u32>(packed_note: [Field; N], selects: BoundedVec<Option<Select>, N>) {
+    // TODO: selects.len() as a loop bound requires compile-time known length, which is true for the existing use
+    // cases.
+    // But if a caller conditionally pushes selects based on a runtime variable, this will fail to compile.
+    // Consider using `for i in 0..N { if i < selects.len() ... }` instead. When .len() is compile-time known, the
+    // compiler optimizes both forms to the same gate count.
+    // Same for `check_notes_order`.
     for i in 0..selects.len() {
         let select = selects.get_unchecked(i).unwrap_unchecked();
         let value_field = extract_property_value_from_selector(packed_note, select.property_selector);

@@ -2,6 +2,8 @@ use crate::context::{PublicContext, UtilityContext};
 use crate::protocol::traits::Packable;
 use crate::state_vars::StateVariable;
 
+mod test;
+
 /// Mutable public values.
 ///
 /// This is one of the most basic public state variables. It is equivalent to a non-`immutable` non-`constant` Solidity

@@ -1,4 +1,5 @@
 use crate::{context::{PublicContext, UtilityContext}, state_vars::PublicMutable};
+use crate::state_vars::StateVariable;
 use crate::test::{helpers::test_environment::TestEnvironment, mocks::MockStruct};
 use std::mem::zeroed;
 

@@ -83,6 +83,7 @@ impl<Context> SingleUseClaim<Context> {
     /// [`SingleUseClaim::assert_claimed`] and [`SingleUseClaim::has_claimed`] to coherently write and read state.
     fn compute_nullifier(self, owner_nhk_app: Field) -> Field {
         // TODO(F-180): make sure we follow the nullifier convention
+        // Q: The separator should be DOM_SEP__NULLIFIER?
         poseidon2_hash_with_separator([owner_nhk_app, self.storage_slot], DOM_SEP__NOTE_HASH)
     }
 }

@@ -43,7 +43,7 @@ pub fn bytes_from_fields<let N: u32>(fields: BoundedVec<Field, N>) -> BoundedVec
     let mut bytes = BoundedVec::new();
 
     for i in 0..fields.len() {
-        let field = fields.get(i);
+        let field = fields.get_unchecked(i);
 
         // We expect that the field contains at most 31 bytes of information.
         field.assert_max_bit_size::<248>();

@@ -46,6 +46,8 @@ pub fn fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> BoundedVec<Fie
     let p = std::field::modulus_be_bytes();
 
     // Since input length is a multiple of 32, we can simply process all chunks fully
+    // TODO: Should be `for i in 0..N / 32 { if i < bytes.len() / 32 { ... } }` instead.
+    // This hasn't caused any problem because it's only used in an unconstrained fn at the moment.
     for i in 0..bytes.len() / 32 {
         let mut field = 0;
 

@@ -8,7 +8,7 @@ pub unconstrained fn get_random_bytes<let N: u32>() -> [u8; N] {
     for i in 0..N {
         if idx == 32 {
             randomness = random().to_be_bytes();
-            idx = 1; // Skip the first byte as it's always 0.
+            idx = 1; // Skip the first byte as it's biased (not uniformly distributed over 0-255).
         }
         bytes[i] = randomness[idx];
         idx += 1;

@@ -50,6 +50,8 @@ impl NoteHash for UintNote {
 
         // 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)
+
+        // Q: The partial note and the complete note probably shouldn't use the same domain separator?
     }
     // docs:end:compute_note_hash
 
@@ -225,6 +227,7 @@ impl PartialUintNote {
         // 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);
+        // Q: Why not use context.assert_nullifier_exists()?
         assert_nullifier_existed_by(
             context.get_anchor_block_header(),
             siloed_validity_commitment,

diff --git a/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr
@@ -464,6 +464,7 @@ pub contract Token {
         // Completer is the entity that can complete the partial note. In this case, it's the same as the minter
         // account.
         let minter_and_completer = self.msg_sender();
+        // TODO: This assertion can be removed. The same thing is checked in _finalize_mint_to_private.
         assert(self.storage.minters.at(minter_and_completer).read(), "caller is not minter");
 
         self.internal._finalize_mint_to_private(minter_and_completer, amount, partial_note);

@@ -3,7 +3,7 @@ use std::meta::derive;
 
 #[derive(Deserialize, Eq, Serialize)]
 pub struct FunctionSelector {
-    // 1st 4-bytes (big-endian leftmost) of abi-encoding of an event.
+    // Low 32 bits of the poseidon2 hash of the function signature.
     pub inner: u32,
 }
 

@@ -16,22 +16,11 @@ pub fn field_from_bytes<let N: u32>(bytes: [u8; N], big_endian: bool) -> Field {
 
 // Convert a 32 byte array to a field element by truncating the final byte
 pub fn field_from_bytes_32_trunc(bytes32: [u8; 32]) -> Field {
-    // Convert it to a field element
-    let mut v = 1;
-    let mut high = 0 as Field;
-    let mut low = 0 as Field;
-
-    for i in 0..15 {
-        // covers bytes 16..30 (31 is truncated and ignored)
-        low = low + (bytes32[15 + 15 - i] as Field) * v;
-        v = v * 256;
-        // covers bytes 0..14
-        high = high + (bytes32[14 - i] as Field) * v;
+    let mut result = 0 as Field;
+    for i in 0..31 {
+        result = result * 256 + (bytes32[i] as Field);
     }
-    // covers byte 15
-    low = low + (bytes32[15] as Field) * v;
-
-    low + high * v
+    result
 }
 
 // TODO to radix returns u8, so we cannot use bigger radixes. It'd be ideal to use a radix of the maximum range-constrained integer noir supports

diff --git a/yarn-project/stdlib/src/abi/selector.ts b/yarn-project/stdlib/src/abi/selector.ts
@@ -4,7 +4,7 @@ import { bufferToHex } from '@aztec/foundation/string';
 
 import { inspect } from 'util';
 
-/** A selector is the first 4 bytes of the hash of a signature. */
+/** A selector is the low 4 bytes of the poseidon2 hash of a signature. */
 export abstract class Selector {
   /** The size of the selector in bytes. */
   public static SIZE = 4;