diff --git a/CODEOWNERS b/CODEOWNERS index 1c1ed2ad249f..4e9fcc3fb111 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -34,7 +34,7 @@ /l1-contracts/src @LHerskind @Maddiaa0 @just-mitch # Notify the circuit team of changes to the protocol circuits -/noir-projects/noir-protocol-circuits @LeilaWang +/noir-projects/noir-protocol-circuits @LeilaWang @iAmMichaelConnor # Notify devrel of changes to docs examples /docs/examples @AztecProtocol/devrel diff --git a/cspell.json b/cspell.json index 0bdfba1d6e33..37f20889cd09 100644 --- a/cspell.json +++ b/cspell.json @@ -157,6 +157,7 @@ "ierc", "IGSE", "incentivized", + "incrementation", "indexeddb", "interruptible", "IPFS", diff --git a/l1-contracts/src/core/libraries/rollup/ProposeLib.sol b/l1-contracts/src/core/libraries/rollup/ProposeLib.sol index dc1ae7c8d40e..db6ad15a4d40 100644 --- a/l1-contracts/src/core/libraries/rollup/ProposeLib.sol +++ b/l1-contracts/src/core/libraries/rollup/ProposeLib.sol @@ -217,7 +217,7 @@ library ProposeLib { header: v.header, digest: v.payloadDigest, manaBaseFee: FeeLib.summedBaseFee(components), - blobsHashesCommitment: v.blobsHashesCommitment, + blobsHashesCommitment: v.blobsHashesCommitment, // Q: why do we store this ugly (difficult to derive within a snark) representation of the blobs within the header, instead of the blobCommitmentsHash (derived below)? Is it because we want something that doesn't contain accumulated data from earlier checkpoints in the epoch? flags: CheckpointHeaderValidationFlags({ignoreDA: false}) }) ); diff --git a/noir-projects/aztec-nr/aztec/src/utils/array/subbvec.nr b/noir-projects/aztec-nr/aztec/src/utils/array/subbvec.nr index 0ce808bd9b98..bf79fe706022 100644 --- a/noir-projects/aztec-nr/aztec/src/utils/array/subbvec.nr +++ b/noir-projects/aztec-nr/aztec/src/utils/array/subbvec.nr @@ -77,7 +77,7 @@ mod test { unconstrained fn subbvec_dst_len_causes_enlarge() { let bvec = BoundedVec::<_, 10>::from_array([1, 2, 3, 4, 5]); - // subbvec does not supprt capacity increases + // subbvec does not support capacity increases let _: BoundedVec<_, 11> = subbvec(bvec, 0); } diff --git a/noir-projects/aztec-nr/aztec/src/utils/mod.nr b/noir-projects/aztec-nr/aztec/src/utils/mod.nr index 333051b3ad95..7bc53db16148 100644 --- a/noir-projects/aztec-nr/aztec/src/utils/mod.nr +++ b/noir-projects/aztec-nr/aztec/src/utils/mod.nr @@ -3,6 +3,5 @@ pub mod comparison; pub mod conversion; pub mod point; pub mod random; -pub mod to_bytes; pub mod with_hash; pub mod remove_constraints; diff --git a/noir-projects/aztec-nr/aztec/src/utils/point.nr b/noir-projects/aztec-nr/aztec/src/utils/point.nr index 87b37232471d..dc1a5409d839 100644 --- a/noir-projects/aztec-nr/aztec/src/utils/point.nr +++ b/noir-projects/aztec-nr/aztec/src/utils/point.nr @@ -5,34 +5,12 @@ use protocol_types::{point::Point, utils::field::sqrt}; global BN254_FR_MODULUS_DIV_2: Field = 10944121435919637611123202872628637544274182200208017171849102093287904247808; -/// Converts a point to a byte array. -/// -/// We don't serialize the point at infinity flag because this function is used in situations where we do not want -/// to waste the extra byte (encrypted log). -pub fn point_to_bytes(p: Point) -> [u8; 32] { - // Note that there is 1 more free bit in the 32 bytes (254 bits currently occupied by the x coordinate, 1 bit for - // the "sign") so it's possible to use that last bit as an "is_infinite" flag if desired in the future. - assert(!p.is_infinite, "Cannot serialize point at infinity as bytes."); - - let mut result: [u8; 32] = p.x.to_be_bytes(); - - if get_sign_of_point(p) { - // y is <= (modulus - 1) / 2 so we set the sign bit to 1 - // Here we leverage that field fits into 254 bits (log2(Fr.MODULUS) < 254) and given that we serialize Fr to 32 - // bytes and we use big-endian the 2 most significant bits are never populated. Hence we can use one of - // the bits as a sign bit. - result[0] += 128; - } - - result -} - /** * Returns: true if p.y <= MOD_DIV_2, else false. */ pub fn get_sign_of_point(p: Point) -> bool { // We store only a "sign" of the y coordinate because the rest can be derived from the x coordinate. To get - // the sign we check if the y coordinate is less or equal than the curve's order minus 1 divided by 2. + // the sign we check if the y coordinate is less or equal than the field's modulus minus 1 divided by 2. // Ideally we'd do `y <= MOD_DIV_2`, but there's no `lte` function, so instead we do `!(y > MOD_DIV_2)`, which is // equivalent, and then rewrite that as `!(MOD_DIV_2 < y)`, since we also have no `gt` function. !BN254_FR_MODULUS_DIV_2.lt(p.y) @@ -70,45 +48,12 @@ pub fn point_from_x_coord_and_sign(x: Field, sign: bool) -> Option { } mod test { - use crate::utils::point::{point_from_x_coord, point_from_x_coord_and_sign, point_to_bytes}; + use crate::utils::point::{ + BN254_FR_MODULUS_DIV_2, get_sign_of_point, point_from_x_coord, point_from_x_coord_and_sign, + }; use dep::protocol_types::point::Point; use dep::protocol_types::utils::field::pow; - #[test] - unconstrained fn test_point_to_bytes_positive_sign() { - let p = Point { - x: 0x1af41f5de96446dc3776a1eb2d98bb956b7acd9979a67854bec6fa7c2973bd73, - y: 0x07fc22c7f2c7057571f137fe46ea9c95114282bc95d37d71ec4bfb88de457d4a, - is_infinite: false, - }; - - let compressed_point = point_to_bytes(p); - - let expected_compressed_point_positive_sign = [ - 154, 244, 31, 93, 233, 100, 70, 220, 55, 118, 161, 235, 45, 152, 187, 149, 107, 122, - 205, 153, 121, 166, 120, 84, 190, 198, 250, 124, 41, 115, 189, 115, - ]; - assert_eq(expected_compressed_point_positive_sign, compressed_point); - } - - #[test] - unconstrained fn test_point_to_bytes_negative_sign() { - let p = Point { - x: 0x247371652e55dd74c9af8dbe9fb44931ba29a9229994384bd7077796c14ee2b5, - y: 0x26441aec112e1ae4cee374f42556932001507ad46e255ffb27369c7e3766e5c0, - is_infinite: false, - }; - - let compressed_point = point_to_bytes(p); - - let expected_compressed_point_negative_sign = [ - 36, 115, 113, 101, 46, 85, 221, 116, 201, 175, 141, 190, 159, 180, 73, 49, 186, 41, 169, - 34, 153, 148, 56, 75, 215, 7, 119, 150, 193, 78, 226, 181, - ]; - - assert_eq(expected_compressed_point_negative_sign, compressed_point); - } - #[test] unconstrained fn test_point_from_x_coord_and_sign() { // Test positive y coordinate @@ -150,4 +95,70 @@ mod test { assert(maybe_point.is_none()); } + #[test] + unconstrained fn test_both_roots_satisfy_curve() { + // Derive a point from x = 8 (known to be valid from test_point_from_x_coord_valid) + let x: Field = 8; + let point = point_from_x_coord(x).unwrap(); + + // Check y satisfies curve equation + assert_eq(point.y * point.y, x * x * x - 17); + + // Check -y also satisfies curve equation + let neg_y = 0 - point.y; + assert_eq(neg_y * neg_y, x * x * x - 17); + + // Verify they are different (unless y = 0) + assert(point.y != neg_y); + } + + #[test] + unconstrained fn test_point_from_x_coord_and_sign_invalid() { + // x = 3 has no valid point on the curve (from test_point_from_x_coord_invalid) + let x = Field::from(3); + let result_positive = point_from_x_coord_and_sign(x, true); + let result_negative = point_from_x_coord_and_sign(x, false); + + assert(result_positive.is_none()); + assert(result_negative.is_none()); + } + + #[test] + unconstrained fn test_get_sign_of_point() { + // Derive a point from x = 8, then test both possible y values + let point = point_from_x_coord(8).unwrap(); + let neg_point = Point { x: point.x, y: 0 - point.y, is_infinite: false }; + + // One should be "positive" (y <= MOD_DIV_2) and one "negative" + let sign1 = get_sign_of_point(point); + let sign2 = get_sign_of_point(neg_point); + assert(sign1 != sign2); + + // y = 0 should return true (0 <= MOD_DIV_2) + let zero_y_point = Point { x: 0, y: 0, is_infinite: false }; + assert(get_sign_of_point(zero_y_point) == true); + + // y = MOD_DIV_2 should return true (exactly at boundary) + let boundary_point = Point { x: 0, y: BN254_FR_MODULUS_DIV_2, is_infinite: false }; + assert(get_sign_of_point(boundary_point) == true); + + // y = MOD_DIV_2 + 1 should return false (just over boundary) + let over_boundary_point = Point { x: 0, y: BN254_FR_MODULUS_DIV_2 + 1, is_infinite: false }; + assert(get_sign_of_point(over_boundary_point) == false); + } + + #[test] + unconstrained fn test_point_from_x_coord_zero() { + // x = 0: y^2 = 0^3 - 17 = -17, which is not a quadratic residue in BN254 scalar field + let result = point_from_x_coord(0); + assert(result.is_none()); + } + + #[test] + unconstrained fn test_bn254_fr_modulus_div_2() { + // Verify that BN254_FR_MODULUS_DIV_2 == (p - 1) / 2 + // This means: 2 * BN254_FR_MODULUS_DIV_2 + 1 == p == 0 (in the field) + assert_eq(2 * BN254_FR_MODULUS_DIV_2 + 1, 0); + } + } diff --git a/noir-projects/aztec-nr/aztec/src/utils/random.nr b/noir-projects/aztec-nr/aztec/src/utils/random.nr index 6cb65f361058..62bcd7b8955e 100644 --- a/noir-projects/aztec-nr/aztec/src/utils/random.nr +++ b/noir-projects/aztec-nr/aztec/src/utils/random.nr @@ -8,10 +8,47 @@ pub unconstrained fn get_random_bytes() -> [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 has biased entropy. } bytes[i] = randomness[idx]; idx += 1; } bytes } + +mod test { + use super::get_random_bytes; + + #[test] + unconstrained fn get_random_bytes_zero() { + let bytes: [u8; 0] = get_random_bytes(); + assert_eq(bytes.len(), 0); + } + + #[test] + unconstrained fn get_random_bytes_one() { + let bytes: [u8; 1] = get_random_bytes(); + assert_eq(bytes.len(), 1); + } + + #[test] + unconstrained fn get_random_bytes_31() { + // 31 bytes fits in a single field (using bytes 1-31) + let bytes: [u8; 31] = get_random_bytes(); + assert_eq(bytes.len(), 31); + } + + #[test] + unconstrained fn get_random_bytes_32() { + // 32 bytes requires two field elements (31 from first, 1 from second) + let bytes: [u8; 32] = get_random_bytes(); + assert_eq(bytes.len(), 32); + } + + #[test] + unconstrained fn get_random_bytes_large() { + // Test a larger size that spans multiple field elements + let bytes: [u8; 100] = get_random_bytes(); + assert_eq(bytes.len(), 100); + } +} diff --git a/noir-projects/aztec-nr/aztec/src/utils/to_bytes.nr b/noir-projects/aztec-nr/aztec/src/utils/to_bytes.nr deleted file mode 100644 index 8b439c8d3884..000000000000 --- a/noir-projects/aztec-nr/aztec/src/utils/to_bytes.nr +++ /dev/null @@ -1,25 +0,0 @@ -pub fn arr_to_be_bytes_arr(fields: [Field; L]) -> [u8; L * 32] { - let mut bytes = [0 as u8; L * 32]; - for i in 0..L { - // Note that bytes.append() results in bound error - let to_add: [u8; 32] = fields[i].to_be_bytes(); - for j in 0..32 { - bytes[i * 32 + j] = to_add[j]; - } - } - bytes -} - -// each character of a string is converted into a byte -// then an ACVM field via the oracle => we recreate here -pub fn str_to_be_bytes_arr(string: str) -> [u8; L * 32] { - let chars_bytes: [u8; L] = string.as_bytes(); - let mut bytes = [0 as u8; L * 32]; - for i in 0..L { - let to_add: [u8; 32] = (chars_bytes[i] as Field).to_be_bytes(); - for j in 0..32 { - bytes[i * 32 + j] = to_add[j]; - } - } - bytes -} diff --git a/noir-projects/noir-contracts/contracts/protocol/contract_instance_registry/src/main.nr b/noir-projects/noir-contracts/contracts/protocol/contract_instance_registry/src/main.nr index 269e8cf723f6..46bc1812104f 100644 --- a/noir-projects/noir-contracts/contracts/protocol/contract_instance_registry/src/main.nr +++ b/noir-projects/noir-contracts/contracts/protocol/contract_instance_registry/src/main.nr @@ -117,6 +117,7 @@ pub contract ContractInstanceRegistry { let partial_address = PartialAddress::compute(contract_class_id, salt, initialization_hash, deployer); + // TODO: assert that the public keys are on the curve (or are a valid representation of the point at infinity). Check that the `add` fn of the stdlib performs such checks, otherwise add those checks. let address = AztecAddress::compute(public_keys, partial_address); // Emit the address as a nullifier: @@ -163,6 +164,9 @@ pub contract ContractInstanceRegistry { "New contract class is not registered", ); + // TODO: assert that the new_contract_class_id.to_field() is not zero, + // because it could trick the avm/kernel into thinking an update hasn't happened before! + let scheduled_value_update = self .storage .updated_class_ids diff --git a/noir-projects/noir-protocol-circuits/ABOUT.md b/noir-projects/noir-protocol-circuits/ABOUT.md new file mode 100644 index 000000000000..c44bf77c403f --- /dev/null +++ b/noir-projects/noir-protocol-circuits/ABOUT.md @@ -0,0 +1,501 @@ +# Protocol Circuits + +- App circuits +- private-kernel + - init + - inner + - reset + - private only: + - tail + - hiding + - public fns: + - tail-to-public + - hiding-to-public +- rollup + - tx + - private only: + - tx-base-private + - public fns: + - chonk-verifier-public + - avm + - tx-base-public + - block + - tx-merge + - block-root + - First block: + - parity + - base + - root + - block-root-first + - checkpoint + - block-merge + - checkpoint-root + - epoch + - checkpoint-merge + - root + +## Diagram + +I recommend looking at this diagram first, to understand the topology of how the circuits knit together. Keep it open as you read the circuits. + +https://drive.google.com/drive/folders/1odV663TQs1DULL1-CIX7SNEH5iEKPa9g?usp=sharing + +> It uses draw.io because it's open source and easier to share. +> That link is for public, view-only access. Ping Mike if you want to make edits and we'll figure out how to get you a link with edit rights. + +## General circuit pattern + +A circuit-specific `validator` validates the consistency of all private inputs to the circuit without mutating any of the input data. + +A circuit-specific public inputs `composer` then generates the outputs (public inputs) of the circuit from the private inputs. + +Sometimes, the `composer` will perform validation (which should really be the job of the `validator`), simply because it was too cumbersome to build the circuits the "proper" way. In such cases, there should be a comment highlighting and justifying that deviation from the norm. + +Generation of the outputs is sometimes done through an unconstrained function for efficiency reasons. The composer then efficiently constrains the correctness of those generated outputs relative to the circuit's private inputs. Auditors should pay extra attention to ensure such unconstrained outputs are being constrained. + +## Unravelling a circuit's private inputs and public inputs + +Many of the input/output structs of a circuit are huge, spanning many many files. If your human brain wants to read them more easily, there's a couple of janky scripts at `noir-protocol-circuits/scripts/unravel_struct.js` and `noir-protocol-circuits/scripts/unravel_favourite_structs.js`. + +## Client-side proving + +## Sequencer-side proving + +### Tx-level circuits + +#### `chonk-verifier-public` + +**Valid previous circuits:** `hiding-kernel-to-public` +**Valid next circuits:** `tx-base-public` + +- Verifies the `hiding-kernel-to-public` proof. +- Validates that the vk used for verification exists at `HIDING_KERNEL_TO_PUBLIC_VK_INDEX` in the vk tree. +- Propagates the public inputs of the `hiding-kernel-to-public` proof. + + +#### `tx-base-private` + +**Valid previous circuits:** `hiding-kernel-to-rollup` +**Valid next circuits:** +- `tx-merge` +- `block-root` <-- BUG? +- `block-root-single-tx` (if the only tx in the block) or +- `block-root-first-single-tx` (if it's the only tx in the block and it's the first block of a checkpoint). + +If a tx makes no public function calls, then the tx in the mempool will contain: +- A `hiding-kernel-to-rollup` proof and public inputs. +- `contract_class_log_fields` - if applicable, the underlying fields of a new contract class that are being published by this tx. Only a hash of the contract class log fields is exposed as a public input of the proof, to reduce client-side constraints. + +Some of those public inputs of the tx make claims about the state of the blockchain, such as historic tree roots, or the non-existence of nullifiers. This `tx-base-private` circuit therefore also takes-in arguments about the _current_ state of the chain; as at the moment just before this tx is to be processed (i.e. the state as at the end of the previous tx). This circuit ensures the tx's claims are consistent with the current state of the chain. + +Validator: +- Verifies the `hiding-kernel-to-rollup` proof. + - Validates that the vk used for verification exists at `HIDING_KERNEL_TO_ROLLUP_VK_INDEX` in the vk tree. +- Validates the public inputs of the `hiding-kernel-to-rollup` proof against arguments relating to the current state of the chain: + - **Performs a membership check to ensure that the claimed anchor block header used during the tx's execution exists as a leaf of the latest archive tree.** + - Asserts equality (between the tx and current chain) of the chain_id, version, vk_tree_root, protocol_contracts_hash. + - Asserts that the tx's chosen gas prices are sufficiently high, relative to the block's minimum requirements. + - Asserts that the tx doesn't exceed the L2 gas limit. + - Asserts that the tx's `include_by_timestamp` hasn't already passed, relative to the block's timestamp. +- Hashes the `contract_class_log_fields` and compares them against the tx's claimed contract class log hash. + +Composer: +- **Computes siloed L2-to-L1 message hashes** (siloed with the contract address of the function that emitted the message). +- **Computes the tx fee**, given the tx's gas settings and the gas used. +- **Decrements the tx fee from the fee_payer's FeeJuice balance.** + - This is a rare example of a protocol circuit directly mutating the state of a smart contract. It's ugly, but the FeeJuice contract is considered a "protocol contract". + - A "public data write" for the decremented FeeJuice balance is computed. + - Validation that the claimed public data tree leaf _actually represents_ the FeeJuice balance of the fee_payer is deferred until a later Validation step in this circuit (see below). +- **Computes the tx_hash**: a hash of the `PrivateToRollupKernelCircuitPublicInputs`; the public inputs of the tx. +- Computes the array lengths of the tx effects: + - Asserts that the array of private log arrays is left-packed with any nonempty private logs. + - Asserts that the array of contract class log arrays (which at the time of writing is an array of length 1) is left-packed with any nonempty contract class logs. + - Validates the array lengths of other tx effects. +- **Builds the end tree snapshots, by inserting the tx effects into the trees:** + - Note Tree: + - Computes a subtree from the tx's new note_hashes. + - Inserts that subtree into the next available position in the append-only note hash tree. + - Nullifiers: + - Computes a subtree from the tx's new nullifiers. (Note: the nullifier tree is an indexed merkle tree, where new leaves are inserted from left to right, so it does support batch insertion). + - Checks for non-existence of the new nullifiers in the tree. + - Inserts the subtree into the next available position in the nullifier tree. + - Public Data Tree: + - Checks that the claimed leaf of the fee_payer's FeeJuice balance actually exists in the public data tree. + - Updates the relevant public data tree leaf with the fee_payer's just-decremented FeeJuice balance. + +Validator (continued): +- Validates the claimed public data tree leaf of the fee_payer _actually_ represents the fee_payer's FeeJuice balance. +- Validates that the starting FeeJuice balance is >= the earlier-calculated tx fee. + +Composer `.finish()`: +- **Appends the tx effects to the next available position of a blob.** + - Tx effects meaning: (tx_hash, revert_code, tx_fee, note_hashes, nullifiers, l2_to_l1_msgs, public_data_writes, private_logs, public_logs, contract_class_logs) + - The way it "appends" is by absorbing all of those fields into a gigantic poseidon2 sponge ("SpongeBlob"), which will only get squeezed within the checkpoint root circuit. + - There is a large, dedicated hackmd describing the blobs subprotocol here: https://hackmd.io/pC2DcVNQSpGZMnPUvfv85g +- **Computes the out_hash for this tx**: a subtree root of a sha256 greedy merkle tree, whose unbalanced leaves are the L2-to-L1 messages emitted by this tx. As we recursively prove the binary tree of rollup proofs, we'll progressively build a larger L2-to-L1 messages subtree, whose root will be called the `out_hash`, and which will be stored on L1. +- Propagates public inputs which will be read by the next circuit. + +#### `tx-base-public` + +**Valid previous circuits:** both `avm` and `chonk-verifier-public` proofs are input. +**Valid next circuits:** +- `tx-merge` (if there are multiple ) +- `block-root` <-- BUG? +- `block-root-single-tx` (if the only tx in the block) or +- `block-root-first-single-tx` (if it's the only tx in the block and it's the first block of a checkpoint). + +If a tx makes public function calls, then the tx in the mempool will contain: +- A `hiding-kernel-to-public` proof and public inputs. +- `contract_class_log_fields` - if applicable, the underlying fields of a new contract class that are being published by this tx. Only a hash of the contract class log fields is exposed as a public input of the proof, to reduce client-side constraints. + +Before reaching this circuit, the tx's enqueued public function calls are all processed by a single `avm` circuit instance, and -- in parallel -- the `hiding-kernel-to-public` proof is verified within a `chonk-verifier-public` circuit and its public inputs are propagated. + +Whereas for private-function-only txs much of the tree-insertion and fee-payment logic happens in the `tx-base-private` circuit, for a tx with public functions the AVM circuit handles most of that logic for us. + +> Note: if you're comparing this circuit's bullet points against those of the `tx-base-private`, many of the checks and computations that exist in the latter are likely being done by the `avm` when there are public functions to be processed. It's always worth questioning seemingly missing checks, though! + +Composer: +- **Computes siloed L2-to-L1 message hashes** (siloed with the contract address of the function that emitted the message). +- If any of the public functions reverted within the `avm`, then the tx is considered to have reverted, and no revertible side-effects will be broadcast nor inserted into trees. But, the protocol allows _non-revertible_ side-effects to still be broadcast / inserted. + - **Conditionally discards the revertible private logs** + - **Conditionally discards the revertible contract class logs.** + - (The former two items come directly from the hiding proof, and are not passed into the avm, so the avm isn't able to discard them). +- Hashes the `contract_class_log_fields` and compares them against the tx's claimed contract class log hash. + - Inconsistent location versus `tx-base-private`, because we needed to conditionally discard revertible contract class logs first. +- **Computes the tx_hash**: a hash of the `PrivateToRollupKernelCircuitPublicInputs`; the public inputs of the tx. +- Computes the array lengths of the tx effects: + - Asserts that the array of private log arrays is left-packed with any nonempty private logs. + - Asserts that the array of contract class log arrays (which at the time of writing is an array of length 1) is left-packed with any nonempty contract class logs. + +Validator: +- Verifies the `chonk-verifier-public` proof. + - Validates that the vk used for verification exists at `PUBLIC_CHONK_VERIFIER_VK_INDEX` in the vk tree. +- Verifies the `avm` proof. + - Validates that the vk used for verification exists at `AVM_VK_INDEX` in the vk tree. +- Validates the public inputs of the `chonk-verifier-public` proof against arguments relating to the current state of the chain (some of which are copied-over from the avm's public inputs): + - **Performs a membership check to ensure that the claimed anchor block header used during the tx's execution exists as a leaf of the latest archive tree.** + - Asserts equality (between the tx and current chain) of the chain_id, version, vk_tree_root, protocol_contracts_hash. + - Asserts that the tx's chosen gas prices are sufficiently high, relative to the block's minimum requirements. + - Asserts that the tx doesn't exceed the L2 gas limit. + - Asserts that the tx's `include_by_timestamp` hasn't already passed, relative to the block's timestamp. + +Composer `.finish()`: +- **Appends the tx effects to the next available position of a blob.** + - Tx effects meaning: (tx_hash, revert_code, tx_fee, note_hashes, nullifiers, l2_to_l1_msgs, public_data_writes, private_logs, public_logs, contract_class_logs) + - The way it "appends" is by absorbing all of those fields into a gigantic poseidon2 sponge ("SpongeBlob"), which will only get squeezed within the checkpoint root circuit. + - There is a large, dedicated hackmd describing the blobs subprotocol here: https://hackmd.io/pC2DcVNQSpGZMnPUvfv85g +- **Computes the out_hash for this tx**: a subtree root of a sha256 greedy merkle tree, whose unbalanced leaves are the L2-to-L1 messages emitted by this tx. As we recursively prove the binary tree of rollup proofs, we'll progressively build a larger L2-to-L1 messages subtree, whose root will be called the `out_hash`, and which will be stored on L1. +- Propagates public inputs which will be read by the next circuit. + +### Block-level circuits + +#### `tx-merge` + + +#### `block-root-first` (empty or single or regular) / `block-root` (single or regular) + +> Note: there is no `block-root-empty-tx`; only a `block-root-first-empty-tx` + +**Valid previous circuits:** +- First block in the checkpoint: + - `parity-root` and one of: + - For `block-root-first`: + - There are valid pairs of left and right vks. Not all pairs are valid. + - ``` + global ALLOWED_PREVIOUS_VK_INDICES: [u32; 3] = [ + TX_MERGE_ROLLUP_VK_INDEX, + PRIVATE_TX_BASE_ROLLUP_VK_INDEX, + PUBLIC_TX_BASE_ROLLUP_VK_INDEX, + ]; + ``` + - For `block-root-first-single-tx`: + - ``` + global ALLOWED_PREVIOUS_VK_INDICES: [u32; 2] = [ + PRIVATE_TX_BASE_ROLLUP_VK_INDEX, + PUBLIC_TX_BASE_ROLLUP_VK_INDEX + ]; + ``` + - For `block-root-first-empty-tx`: + - None. Empty block. +- Subsequent blocks in the checkpoint: + - For `block-root`: + - There are valid pairs of left and right vks. Not all pairs are valid. + - ``` + global ALLOWED_PREVIOUS_VK_INDICES: [u32; 3] = [ + TX_MERGE_ROLLUP_VK_INDEX, + PRIVATE_TX_BASE_ROLLUP_VK_INDEX, + PUBLIC_TX_BASE_ROLLUP_VK_INDEX, + ]; + ``` + - For `block-root-single-tx`: + - ``` + global ALLOWED_PREVIOUS_VK_INDICES: [u32; 2] = [ + PRIVATE_TX_BASE_ROLLUP_VK_INDEX, + PUBLIC_TX_BASE_ROLLUP_VK_INDEX + ]; + ``` + +**Valid next circuits:** +- First block in the checkpoint: + - For `block-root-first`: + - `block-merge` + - `checkpoint-root` + - For `block-root-first-single-tx`: + - `checkpoint-root` + - For `block-root-first-empty-tx`: + - `checkpoint-root` +- Subsequent blocks in the checkpoint: + - For `block-root`: + - `block-merge` + - `checkpoint-root` + - For `block-root-single-tx`: + - `checkpoint-root` + +##### Empty. (`block-root-first-empty-tx` only) + +For when there are no txs in the block, but the block proposer still wants block rewards: +Propagates default empty values: +- Empty spongeblob. +- num_txs = 0 +- out_hash = 0 +- accumulated fees & mana = 0 +Propagates unchanged state (except for the L1->L2 msgs which will be appended-to (see below)) + +##### First (`block-root-first-empty-tx`, `block-root-first-single-tx`, `block-root-first`). + +Validator: +- Validate the parity root proof and vk. + +Composer: +- **Inserts the "poseidon2 version" of the l1_to_l2_msg subtree** into the next available leaf of the L1-to-L2-msgs tree. + +##### `block-root`, `block-root-first`, `block-root-single-tx`, `block-root-first-single-tx` + +Validator: +- Verifies the previous proof(s) (see valid previous circuits above). + - For the "single" variants, that means verifying a single tx-base proof. + - For the non-single variants, that means verifying two proofs: some combination of tx-merge proofs and/or tx-base proofs. + - Validates that the vk(s) used for verification exist(s) in the circuit's hard-coded list of valid vk indices, and within the vk tree. + +##### `block-root`, `block-root-first` + +Validator: +- **Validates the "consecutiveness" of the two input proofs**. + - That the left and right subtrees follow the "greedy tree" rules. + - That constants used in the left and right proofs' public inputs are equal. + - That the end states of the left subtree are equal to the start states of the right subtree. + +Composer: +- **Merges the left and right input subtrees**: + - Sum the num txs. + - sha256-hash the left and right out_hashes. + - Recall: `out_hash` is the root of a subtree containing L2->L1 messages. + - Sum the left and right accumulated fees. + - Sum the left and right accumulated mana used. + +##### All + +Composer: +- **Absorbs Block End Data of one block into the SpongeBlob**; + - There is a large, dedicated hackmd describing the blobs subprotocol here: https://hackmd.io/pC2DcVNQSpGZMnPUvfv85g +- Squeezes a copy of this SpongeBlob, and inserts that squeezed sponge into the block's BlockHeader + - as a record of the emitted data of that block. +- Propagates the unsqueezed SpongeBlob + - (to the Block Merge Rollup circuit), so that the next block can continue to absorb data into it. +- Ensures the block number of the block being built matches the next block number (according to the archive tree's next available leaf index). +- **Creates the block header**. +- **Inserts the block header's leaf** (block header hash) into the next available leaf index of the Archive Tree. + +#### `parity-base` + +**Valid previous circuits:** None. + +**Valid next circuits:** `parity-root` + + +#### `parity-root` + +**Valid previous circuits:** `parity-base` + +**Valid next circuits:** +- `block-root-first` +- `block-root-first-single-tx` +- `block-root-first-empty-tx` <-- double-check that this is the case, Mike + + +### Checkpoint-level circuits + +#### `block-merge` + +**Valid previous circuits:** +- `block-root-first` +- `block-root` +- `block-merge` + +**Valid next circuits:** +- `block-merge` +- `checkpoint-root` + +#### `checkpoint-root` + +**Valid previous circuits:** +- ``` + global ALLOWED_PREVIOUS_VK_INDICES: [u32; 6] = [ + BLOCK_ROOT_FIRST_ROLLUP_VK_INDEX, + BLOCK_ROOT_SINGLE_TX_FIRST_ROLLUP_VK_INDEX, + BLOCK_ROOT_EMPTY_TX_FIRST_ROLLUP_VK_INDEX, + BLOCK_ROOT_ROLLUP_VK_INDEX, + BLOCK_ROOT_SINGLE_TX_ROLLUP_VK_INDEX, + BLOCK_MERGE_ROLLUP_VK_INDEX, + ]; + ``` + - Notice: + - Ordinarily, a Checkpoint will contain multiple blocks, in which case the input proof will be from a `block-merge` circuit. + - If the checkpoint only contains a single block, then by definition that block is the "first" block of the checkpoint. Valid circuits are then: + - `BLOCK_ROOT_FIRST_ROLLUP_VK_INDEX`, + - `BLOCK_ROOT_SINGLE_TX_FIRST_ROLLUP_VK_INDEX`, + - `BLOCK_ROOT_EMPTY_TX_FIRST_ROLLUP_VK_INDEX`, + - depending on the contents of that block. + +**Valid next circuits:** +- `checkpoint-merge` +- `root` + +Validator: + +- Verifies the previous proof(s) (see valid previous circuits above). + - For the "single" variants, that means verifying a single tx-base proof. + - For the non-single variants, that means verifying two proofs: some combination of block-merge proofs and/or block-root proofs. + - Validates that the vk(s) used for verification exist(s) in the circuit's hard-coded list of valid vk indices, and within the vk tree. +- **Validates the "consecutiveness" of the two input proofs**. + - That the left and right subtrees follow the "greedy tree" rules. + - That constants used in the left and right proofs' public inputs are equal. + - That the end states of the left subtree are equal to the start states of the right subtree. + - That the left timestamp is <= the right timestamp. + - Note: blocks in the same checkpoint can have equal timestamps. + - That the right.in_hash == 0. +- Verify that the left start states were the default "empty" values: + - Empty start SpongeBlob + - Start state as per the previous block header (i.e. the last block header of the previous checkpoint). + - Timestamp of the first block of this checkpoint is > the timestamp of the last block of the previous checkpoint. + - The in_hash (copied as part of the first block) cannot be empty. + - Validate that the claimed previous_block_header is indeed the last nonempty leaf of the claimed previous archive tree. + - Note: the correctness of the claimed previous archive tree will be checked with this epoch's proof is submitted to L1. + +Composer: + +- **Merges the left and right input subtrees**: + - Accumulate the `block_headers_hash`. + - Set the accumulated in_hash to simply be the left in_hash. + - All L1->L2 messages are copied-over as part of the first block in a checkpoint (see the `block-root-first` circuit variants above) (hence coming from the left-most block-root circuit of the checkpoint). + - Other blocks in the checkpoint have a `0` in_hash. + - sha256-hash the left and right out_hashes. + - Recall: `out_hash` is the root of a subtree containing L2->L1 messages. + - Sum the left and right accumulated fees. + - Sum the left and right accumulated mana used. +- Blob stuff + - There is a large, dedicated hackmd describing the blobs subprotocol here: https://hackmd.io/pC2DcVNQSpGZMnPUvfv85g + - TODO: incorporate this hackmd into a protocol spec. + - Computes the Checkpoint End marker. + - Absorbs the Checkpoint End marker into the SpongeBlob. + - Ensures the number of absorbed fields is `<=` the max amount of blob data for a checkpoint (6 * 4096 fields). + - Squeezes the sponge (which represents all blobs of this checkpoint), finally. + - Takes as a private input hint the entire stream of 6 * 4096 fields of the checkpoint (`blob_fields`) + - Ensures these fields are all zero after the computed length. + - It poseidon2-hashes these fields to make sure the result matches the sponge we just squeezed. I.e. it makes sure this stream of `blobs_fields` matches the data that the earlier circuits "saw". + - For each of those 6 $blob_i$'s of 4096 fields: + - Evaluates the interpolated polynomial $p_i(X)$ at $z$ to yield the evaluation $y_i$. + - Adds the next summand in the incremental computations of the batched kzg commitment $C$, and the batched evaluation $y$. + - Adds the next contribution to the incremental computations of the batch challenges $z$ and $\gamma$. + - Adds the next contribution to the incremental computation of the $\texttt{blobCommitmentsHash}_M$ that was stored on L1 when checkpoint $M$ was proposed. + - We do this to prove that the $C_i$'s "seen" by the circuits match the $C_i$'s that were submitted to L1 as part of checkpoint proposals. + - Propagates the accumulated blob data to the next circuit. (See the hackmd). +- Computes a Checkpoint Header. + + +### Epoch-level circuits + +#### `checkpoint-merge` + +**Valid previous circuits**: +``` +global ALLOWED_VK_INDICES: [u32; 3] = [ + CHECKPOINT_ROOT_ROLLUP_VK_INDEX, + CHECKPOINT_ROOT_SINGLE_BLOCK_ROLLUP_VK_INDEX, + CHECKPOINT_MERGE_ROLLUP_VK_INDEX, +]; +``` + +**Valid next circuits:** +- `checkpoint-merge` +- `root` + + +(Validator): + +- Verifies the previous proofs (see valid previous circuits above). + - Validates that the vks used for verification exist in the circuit's hard-coded list of valid vk indices, and within the vk tree. +- **Validates the "consecutiveness" of the two input proofs**. + - That the left and right subtrees follow the "greedy tree" rules. + - That constants used in the left and right proofs' public inputs are equal. + - That the end states of the left subtree are equal to the start states of the right subtree. + - That both subtrees claim the same final blob challenges z & gamma. + +(Composer): +- **Merges the left and right input subtrees**: + - Sum the number of checkpoints of the two input subtrees of checkpoints. + - Assert that the sum hasn't exceeded the max number of checkpoints in an epoch. + - Merge the left and right arrays of `checkpoint_header_hashes`. + - The final array will be propagated to L1 for some further checks. + - Merge the left and right arrays of `fees`. + +#### `root` + +#### `checkpoint-merge` + +**Valid previous circuits**: +``` +global ALLOWED_LEFT_ROLLUP_VK_INDICES: [u32; 3] = [ + CHECKPOINT_ROOT_ROLLUP_VK_INDEX, + CHECKPOINT_ROOT_SINGLE_BLOCK_ROLLUP_VK_INDEX, + CHECKPOINT_MERGE_ROLLUP_VK_INDEX, +]; + +global ALLOWED_RIGHT_ROLLUP_VK_INDICES: [u32; 4] = [ + CHECKPOINT_ROOT_ROLLUP_VK_INDEX, + CHECKPOINT_ROOT_SINGLE_BLOCK_ROLLUP_VK_INDEX, + CHECKPOINT_MERGE_ROLLUP_VK_INDEX, + // Padding checkpoint rollup can only be the right child of the root rollup. + CHECKPOINT_PADDING_ROLLUP_VK_INDEX, +]; +``` + +**Valid next circuits:** + +Next stop: L1. The `EpochProofLib.sol` will verify this `root` proof. + +(Validator): + +- Verifies the previous proofs (see valid previous circuits above). + - Validates that the vks used for verification exist in the circuit's hard-coded list of valid vk indices, and within the vk tree. + - If the left subtree represents a single checkpoint, the right proof is a special "padding proof" (a dummy proof, of sorts). + - The circuit checks the conditions that permit a padding proof to be used. +- **Validates the "consecutiveness" of the two input proofs**. + - That the left and right subtrees follow the "greedy tree" rules. + - That constants used in the left and right proofs' public inputs are equal. + - That the end states of the left subtree are equal to the start states of the right subtree. + - That both subtrees claim the same final blob challenges z & gamma. + +(Composer): +- **Merges the left and right input subtrees**: + - Sum the number of checkpoints of the two input subtrees of checkpoints. + - Assert that the sum hasn't exceeded the max number of checkpoints in an epoch. + - Merge the left and right arrays of `checkpoint_header_hashes`. + - The final array will be propagated to L1 for some further checks. + - Merge the left and right arrays of `fees`. +- Asserts that the starting blob_accumulator of the epoch was empty. +- Validates the final_blob_batching_challenges, comparing the claimed challenges against the accumulated computations of those challenges. + - See the blobs hackmd for more detail on what that means. +- Compresses the final batched blob commitment. + + diff --git a/noir-projects/noir-protocol-circuits/OTHER_HASHES_DOMAIN_SEPARATION_REPORT.md b/noir-projects/noir-protocol-circuits/OTHER_HASHES_DOMAIN_SEPARATION_REPORT.md new file mode 100644 index 000000000000..12127f76caa4 --- /dev/null +++ b/noir-projects/noir-protocol-circuits/OTHER_HASHES_DOMAIN_SEPARATION_REPORT.md @@ -0,0 +1,260 @@ +# Non-Poseidon2 Hashing and Domain Separation Report + +This document provides a comprehensive list of all non-Poseidon2 hash usages in `noir-protocol-circuits`, including SHA256, Pedersen, and other hash functions. + +## Key Findings + +- **SHA256 has NO domain separation** - All 8 production uses of SHA256 lack domain separators +- **Pedersen wrapper supports separation** - But the only actual usage (test code) doesn't use it +- **No Keccak or Blake usage** - These hash functions are not used in this crate +- **Potential collision risks** - Multiple 2-field SHA256 hashes could collide + +## Hash Functions Found + +| Hash Function | Production Uses | Test Uses | Domain Separation | +|---------------|-----------------|-----------|-------------------| +| SHA256 (`sha256_to_field`, `accumulate_sha256`) | 8 | Many | **NONE** | +| Pedersen (`pedersen_hash`) | 0 | 1 | Wrapper supports it | +| Keccak | 0 | 0 | N/A | +| Blake | 0 | 0 | N/A | + +--- + +## SHA256 Hashes + +> **WARNING:** None of the SHA256 hashes use domain separation. This means hashes with the same byte-length input could potentially collide across different contexts. + +### Core Functions + +#### 1. `sha256_to_field` +- **File**: `crates/types/src/hash.nr:30-34` +- **Purpose**: Base function that computes SHA256 and truncates to a field element +- **Domain Separation**: **NONE** +```noir +pub fn sha256_to_field(bytes_to_hash: [u8; N]) -> Field { + let sha256_hashed = sha256::digest(bytes_to_hash); + let hash_in_a_field = field_from_bytes_32_trunc(sha256_hashed); + hash_in_a_field +} +``` + +#### 2. `accumulate_sha256` +- **File**: `crates/types/src/hash.nr:214-220` +- **Preimage**: 64 bytes (two 32-byte field encodings) +- **Purpose**: Hashes two fields together for Merkle trees and accumulation +- **Domain Separation**: **NONE** +```noir +pub fn accumulate_sha256(v0: Field, v1: Field) -> Field { + let v0_as_bytes: [u8; 32] = v0.to_be_bytes(); + let v1_as_bytes: [u8; 32] = v1.to_be_bytes(); + let hash_input_flattened = v0_as_bytes.concat(v1_as_bytes); + sha256_to_field(hash_input_flattened) +} +``` + +--- + +### Production Uses of SHA256 + +#### 3. `compute_l2_to_l1_hash` +- **File**: `crates/types/src/hash.nr:164-191` +- **Preimage**: 148 bytes (contract_address, rollup_version_id, recipient, chain_id, content) +- **Purpose**: Computing L2 to L1 message hashes for cross-chain communication +- **Domain Separation**: **NONE** (but has unique 148-byte structure) +```noir +pub fn compute_l2_to_l1_hash( + contract_address: AztecAddress, + recipient: EthAddress, + content: Field, + rollup_version_id: Field, + chain_id: Field, +) -> Field { + // ... builds 148-byte array ... + sha256_to_field(bytes) +} +``` + +#### 4. `CheckpointHeader::hash` +- **File**: `crates/types/src/abis/checkpoint_header.nr:97-100` +- **Preimage**: `CHECKPOINT_HEADER_SIZE_IN_BYTES` bytes (316 bytes) +- **Purpose**: Hashing checkpoint headers for L1 verification +- **Domain Separation**: **NONE** (but has unique 316-byte structure) +```noir +impl Hash for CheckpointHeader { + fn hash(self) -> Field { + sha256_to_field(self.to_be_bytes()) + } +} +``` + +#### 5. `BlobAccumulator::init` (blob_commitments_hash) +- **File**: `crates/blob/src/abis/blob_accumulator.nr:81` +- **Preimage**: Compressed blob commitment bytes (96 bytes) +- **Purpose**: Initial blob commitment hash for batched KZG proofs +- **Domain Separation**: **NONE** +```noir +blob_commitments_hash_acc: sha256_to_field(first_output.c_i.compressed), +``` + +#### 6. `BlobAccumulator::accumulate` (blob_commitments_hash) +- **File**: `crates/blob/src/abis/blob_accumulator.nr:129-132` +- **Preimage**: Previous hash (32 bytes) + new commitment (96 bytes) = 128 bytes +- **Purpose**: Accumulating blob commitment hashes +- **Domain Separation**: **NONE** +```noir +blob_commitments_hash_acc: sha256_to_field(self + .blob_commitments_hash_acc + .to_be_bytes::<32>() + .concat(other.c_i.compressed)), +``` + +#### 7. `accumulate_out_hash` (Tx Merge) +- **File**: `crates/rollup-lib/src/tx_merge/utils/merge_tx_rollups.nr:4-11` +- **Preimage**: 64 bytes (two 32-byte hashes) +- **Purpose**: Merging transaction output hashes during rollup +- **Domain Separation**: **NONE** +```noir +pub fn accumulate_out_hash(left_out_hash: Field, right_out_hash: Field) -> Field { + if left_out_hash == 0 { + right_out_hash + } else if right_out_hash == 0 { + left_out_hash + } else { + accumulate_sha256(left_out_hash, right_out_hash) + } +} +``` + +#### 8. `MerkleTree::new_sha` +- **File**: `crates/types/src/merkle_tree/merkle_tree.nr:21-24` +- **Preimage**: 64 bytes per hash (two 32-byte siblings) +- **Purpose**: Building SHA256-based Merkle trees +- **Domain Separation**: **NONE** +```noir +pub fn new_sha(leaves: [Field; N]) -> Self { + let nodes = compute_merkle_tree_nodes(leaves, accumulate_sha256); + MerkleTree { leaves, nodes } +} +``` + +#### 9. `UnbalancedMerkleTree::new_sha` +- **File**: `crates/types/src/merkle_tree/unbalanced_merkle_tree.nr:12-22` +- **Preimage**: 64 bytes per hash (two 32-byte siblings) +- **Purpose**: Building unbalanced SHA256-based Merkle trees +- **Domain Separation**: **NONE** +```noir +pub fn new_sha( + leaves: [Field; N], + num_non_empty_leaves: u32, +) -> Self { + let root = compute_unbalanced_merkle_root::( + leaves, + num_non_empty_leaves, + accumulate_sha256, + ); + UnbalancedMerkleTree { root } +} +``` + +--- + +## Pedersen Hashes + +### Core Function + +#### 1. `pedersen_hash` (Wrapper) +- **File**: `crates/types/src/hash.nr:224-226` +- **Domain Separation**: **SUPPORTS** via `hash_index` parameter +- **Note**: This is just a wrapper around `std::hash::pedersen_hash_with_separator` +```noir +pub fn pedersen_hash(inputs: [Field; N], hash_index: u32) -> Field { + std::hash::pedersen_hash_with_separator(inputs, hash_index) +} +``` + +### Usages + +#### 2. `TestLeafPreimage::as_leaf` (Test Only) +- **File**: `crates/types/src/tests/types/test_leaf_preimage.nr:20-22` +- **Preimage**: `[self.value]` (1 field) +- **Purpose**: Test leaf preimage hashing +- **Domain Separation**: **NONE** (uses `std::hash::pedersen_hash` directly, not the wrapper) +```noir +fn as_leaf(self) -> Field { + pedersen_hash([self.value]) +} +``` + +--- + +## Summary + +| Category | Count | +|----------|-------| +| SHA256 hashes (production) | 8 | +| SHA256 hashes WITH domain separator | **0** | +| Pedersen hashes (production) | 0 | +| Pedersen hashes (test) | 1 | + +--- + +## Security Considerations + +### Critical: No Domain Separation in SHA256 + +All SHA256 usages lack domain separation. While some have unique input sizes that provide implicit separation, this is not a robust security practice. + +#### Potential Collision Risks by Input Size: + +| Input Size | Usages | Risk Level | +|------------|--------|------------| +| 64 bytes | `accumulate_sha256`, `accumulate_out_hash`, Merkle trees | **HIGH** - Multiple uses with same size | +| 96 bytes | `BlobAccumulator::init` | Low - Unique structure | +| 128 bytes | `BlobAccumulator::accumulate` | Low - Unique structure | +| 148 bytes | `compute_l2_to_l1_hash` | Low - Unique structure | +| 316 bytes | `CheckpointHeader::hash` | Low - Unique structure | + +### High-Risk: 64-byte SHA256 Hashes + +The following all hash 64 bytes (two fields) without any domain separation: + +1. **`accumulate_sha256`** - Generic 2-field hash +2. **`accumulate_out_hash`** - Tx output hash accumulation +3. **`MerkleTree::new_sha`** - SHA Merkle tree nodes +4. **`UnbalancedMerkleTree::new_sha`** - Unbalanced SHA Merkle tree nodes + +**Scenario**: If the same two field values are used in different contexts (e.g., as Merkle siblings vs. as output hashes), they would produce identical hashes. + +### Why This May Be Acceptable + +1. **Context isolation**: These hashes are used in separate, non-overlapping contexts +2. **Value constraints**: The fields being hashed have different semantic meanings and value ranges +3. **L1 compatibility**: SHA256 is used specifically for Ethereum/L1 compatibility where domain separation may not be standard + +### Recommendations + +1. **Add domain separators to SHA256**: Prepend a domain byte to differentiate contexts: + ```noir + // Example: Add domain byte for Merkle hashing + fn merkle_sha256(left: Field, right: Field) -> Field { + let mut bytes = [0u8; 65]; + bytes[0] = 0x01; // Domain separator for Merkle + // ... copy left and right ... + sha256_to_field(bytes) + } + ``` + +2. **Document intentional omissions**: If domain separation is intentionally omitted (e.g., for L1 compatibility), document this clearly. + +3. **Review cross-context usage**: Audit all places where 64-byte SHA256 hashes are used to ensure no cross-context collisions are possible. + +--- + +## Comparison with Poseidon2 + +| Aspect | Poseidon2 | SHA256 | +|--------|-----------|--------| +| Domain separation function | `poseidon2_hash_with_separator` | **None** | +| % with domain separation | ~50% | **0%** | +| Separator placement | Start of preimage | N/A | +| Primary use case | In-circuit hashing | L1/cross-chain compatibility | diff --git a/noir-projects/noir-protocol-circuits/POSEIDON2_DOMAIN_SEPARATION_REPORT.md b/noir-projects/noir-protocol-circuits/POSEIDON2_DOMAIN_SEPARATION_REPORT.md new file mode 100644 index 000000000000..7c23b995dfb4 --- /dev/null +++ b/noir-projects/noir-protocol-circuits/POSEIDON2_DOMAIN_SEPARATION_REPORT.md @@ -0,0 +1,667 @@ +# Poseidon2 Hashing and Domain Separation Report + +This document provides a comprehensive list of all Poseidon2 hash usages in `noir-protocol-circuits`, including domain separators. + +## Key Findings + +- **~50% of hashes lack domain separation** - 19 uses of `poseidon2_hash` (incl. variants) vs 17 uses of `poseidon2_hash_with_separator` +- **Domain separator is always at the START** - No instances of separator at the end +- **16 unique domain separators are used** - Out of 37 defined constants + +## Domain Separator Placement + +**The domain separator is placed at the START of the preimage.** + +From `crates/types/src/hash.nr:230-236`: +```noir +pub fn poseidon2_hash_with_separator(inputs: [Field; N], separator: T) -> Field +where + T: ToField, +{ + let inputs_with_separator = [separator.to_field()].concat(inputs); + poseidon2_hash(inputs_with_separator) +} +``` + +--- + +## Domain Separator Constants + +Defined in `crates/types/src/constants.nr:631-718`. Tests for domain separator consistency are in `crates/types/src/domain_separators.nr`. + +### Note Hashes + +| Value | Constant Name | +|-------|---------------| +| 1 | `DOM_SEP__NOTE_HASH` | +| 2 | `DOM_SEP__NOTE_HASH_NONCE` | +| 3 | `DOM_SEP__UNIQUE_NOTE_HASH` | +| 4 | `DOM_SEP__SILOED_NOTE_HASH` | + +### Nullifiers + +| Value | Constant Name | +|-------|---------------| +| 5 | `DOM_SEP__MESSAGE_NULLIFIER` | +| 6 | `DOM_SEP__INITIALIZATION_NULLIFIER` | +| 7 | `DOM_SEP__OUTER_NULLIFIER` | +| 53 | `DOM_SEP__NOTE_NULLIFIER` | + +### Public Storage + +| Value | Constant Name | +|-------|---------------| +| 23 | `DOM_SEP__PUBLIC_LEAF_INDEX` | + +### Contract Address + +| Value | Constant Name | +|-------|---------------| +| 11 | `DOM_SEP__FUNCTION_LEAF` | +| 13 | `DOM_SEP__CONSTRUCTOR` | +| 15 | `DOM_SEP__CONTRACT_ADDRESS_V1` | +| 16 | `DOM_SEP__CONTRACT_CLASS_ID` | +| 27 | `DOM_SEP__PARTIAL_ADDRESS` | +| 60 | `DOM_SEP__PUBLIC_BYTECODE` | + +### Keys + +| Value | Constant Name | +|-------|---------------| +| 48 | `DOM_SEP__NSK_M` | +| 49 | `DOM_SEP__IVSK_M` | +| 50 | `DOM_SEP__OVSK_M` | +| 51 | `DOM_SEP__TSK_M` | +| 52 | `DOM_SEP__PUBLIC_KEYS_HASH` | + +### Transactions and Blocks + +| Value | Constant Name | +|-------|---------------| +| 28 | `DOM_SEP__BLOCK_HASH` | +| 33 | `DOM_SEP__TX_REQUEST` | +| 56 | `DOM_SEP__PUBLIC_TX_HASH` | +| 57 | `DOM_SEP__PRIVATE_TX_HASH` | + +### Protocol + +| Value | Constant Name | +|-------|---------------| +| 43 | `DOM_SEP__PUBLIC_CALLDATA` | +| 44 | `DOM_SEP__FUNCTION_ARGS` | +| 59 | `DOM_SEP__EVENT_COMMITMENT` | +| 61 | `DOM_SEP__PROTOCOL_CONTRACTS` | + +### Authwit (may be used by protocol contracts) + +| Value | Constant Name | +|-------|---------------| +| 45 | `DOM_SEP__AUTHWIT_INNER` | +| 46 | `DOM_SEP__AUTHWIT_OUTER` | +| 47 | `DOM_SEP__AUTHWIT_NULLIFIER` | + +### Encryption and Notes (may be used by protocol contracts) + +| Value | Constant Name | +|-------|---------------| +| 54 | `DOM_SEP__SYMMETRIC_KEY` (u8) | +| 55 | `DOM_SEP__SYMMETRIC_KEY_2` (u8) | +| 58 | `DOM_SEP__PARTIAL_NOTE_VALIDITY_COMMITMENT` | + +### Other (may move to aztec-nr) + +| Value | Constant Name | +|-------|---------------| +| 20 | `DOM_SEP__SECRET_HASH` | +| 32 | `DOM_SEP__TX_NULLIFIER` | +| 34 | `DOM_SEP__SIGNATURE_PAYLOAD` | + +--- + +## Hashes WITH Domain Separation + +### 1. `compute_note_hash_nonce` +- **File**: `crates/types/src/hash.nr:52-58` +- **Domain Separator**: `DOM_SEP__NOTE_HASH_NONCE` (2) +- **Preimage**: `[first_nullifier_in_tx, note_index_in_tx]` +- **Purpose**: Computing unique nonce for note hashes +```noir +pub fn compute_note_hash_nonce(first_nullifier_in_tx: Field, note_index_in_tx: u32) -> Field { + poseidon2_hash_with_separator( + [first_nullifier_in_tx, note_index_in_tx as Field], + DOM_SEP__NOTE_HASH_NONCE, + ) +} +``` + +### 2. `compute_unique_note_hash` +- **File**: `crates/types/src/hash.nr:61-63` +- **Domain Separator**: `DOM_SEP__UNIQUE_NOTE_HASH` (3) +- **Preimage**: `[note_nonce, siloed_note_hash]` +- **Purpose**: Computing unique note hashes from siloed hashes +```noir +pub fn compute_unique_note_hash(note_nonce: Field, siloed_note_hash: Field) -> Field { + let inputs = [note_nonce, siloed_note_hash]; + poseidon2_hash_with_separator(inputs, DOM_SEP__UNIQUE_NOTE_HASH) +} +``` + +### 3. `compute_siloed_note_hash` +- **File**: `crates/types/src/hash.nr:75-79` +- **Domain Separator**: `DOM_SEP__SILOED_NOTE_HASH` (4) +- **Preimage**: `[app.to_field(), note_hash]` +- **Purpose**: Siloing note hashes to specific contract addresses +```noir +pub fn compute_siloed_note_hash(app: AztecAddress, note_hash: Field) -> Field { + poseidon2_hash_with_separator( + [app.to_field(), note_hash], + DOM_SEP__SILOED_NOTE_HASH, + ) +} +``` + +### 4. `compute_siloed_nullifier` +- **File**: `crates/types/src/hash.nr:102-107` +- **Domain Separator**: `DOM_SEP__OUTER_NULLIFIER` (7) +- **Preimage**: `[app.to_field(), nullifier]` +- **Purpose**: Siloing nullifiers to specific contract addresses +```noir +pub fn compute_siloed_nullifier(contract_address: AztecAddress, nullifier: Field) -> Field { + poseidon2_hash_with_separator( + [contract_address.to_field(), nullifier], + DOM_SEP__OUTER_NULLIFIER, + ) +} +``` + +### 5. `compute_app_secret_key` +- **File**: `crates/types/src/hash.nr:146-155` +- **Domain Separator**: Dynamic `app_secret_generator` (passed as argument) +- **Preimage**: `[master_secret_key.hi, master_secret_key.lo, app_address.to_field()]` +- **Purpose**: Computing app-specific secret keys +```noir +pub fn compute_app_secret_key( + master_secret_key: EmbeddedCurveScalar, + app_address: AztecAddress, + app_secret_generator: Field, +) -> Field { + poseidon2_hash_with_separator( + [master_secret_key.hi, master_secret_key.lo, app_address.to_field()], + app_secret_generator, + ) +} +``` + +### 6. `compute_public_data_leaf_slot` +- **File**: `crates/types/src/data/hash.nr:3-7` +- **Domain Separator**: `DOM_SEP__PUBLIC_LEAF_INDEX` (23) +- **Preimage**: `[contract_address.to_field(), storage_slot]` +- **Purpose**: Computing public data leaf slots +```noir +pub fn compute_public_data_leaf_slot(contract_address: AztecAddress, storage_slot: Field) -> Field { + crate::hash::poseidon2_hash_with_separator( + [contract_address.to_field(), storage_slot], + DOM_SEP__PUBLIC_LEAF_INDEX, + ) +} +``` + +### 7. `ContractClassFunctionLeafPreimage::hash` +- **File**: `crates/types/src/abis/contract_class_function_leaf_preimage.nr:11-17` +- **Domain Separator**: `DOM_SEP__FUNCTION_LEAF` (11) +- **Preimage**: `[self.selector.to_field(), self.vk_hash]` +- **Purpose**: Hashing contract class function leaf preimages +```noir +impl Hash for ContractClassFunctionLeafPreimage { + fn hash(self) -> Field { + poseidon2_hash_with_separator( + [self.selector.to_field(), self.vk_hash], + DOM_SEP__FUNCTION_LEAF, + ) + } +} +``` + +### 8. `ContractClassId::compute` +- **File**: `crates/types/src/contract_class_id.nr:29-38` +- **Domain Separator**: `DOM_SEP__CONTRACT_CLASS_ID` (16) +- **Preimage**: `[artifact_hash, private_functions_root, public_bytecode_commitment]` +- **Purpose**: Computing contract class IDs +```noir +pub fn compute( + artifact_hash: Field, + private_functions_root: Field, + public_bytecode_commitment: Field, +) -> Self { + let hash = crate::hash::poseidon2_hash_with_separator( + [artifact_hash, private_functions_root, public_bytecode_commitment], + DOM_SEP__CONTRACT_CLASS_ID, + ); + ContractClassId::from_field(hash) +} +``` + +### 9. `AztecAddress::compute` +- **File**: `crates/types/src/address/aztec_address.nr:86-92` +- **Domain Separator**: `DOM_SEP__CONTRACT_ADDRESS_V1` (15) +- **Preimage**: `[public_keys_hash.to_field(), partial_address.to_field()]` +- **Purpose**: Computing contract addresses from partial addresses +```noir +pub fn compute(public_keys: PublicKeys, partial_address: PartialAddress) -> AztecAddress { + let public_keys_hash = public_keys.hash(); + let pre_address = poseidon2_hash_with_separator( + [public_keys_hash.to_field(), partial_address.to_field()], + DOM_SEP__CONTRACT_ADDRESS_V1, + ); + // ... derive address point +} +``` + +### 10. `PartialAddress::compute_from_salted_initialization_hash` +- **File**: `crates/types/src/address/partial_address.nr:45-52` +- **Domain Separator**: `DOM_SEP__PARTIAL_ADDRESS` (27) +- **Preimage**: `[contract_class_id.to_field(), salted_initialization_hash.to_field()]` +- **Purpose**: Computing partial addresses +```noir +pub fn compute_from_salted_initialization_hash( + contract_class_id: ContractClassId, + salted_initialization_hash: SaltedInitializationHash, +) -> Self { + PartialAddress::from_field(poseidon2_hash_with_separator( + [contract_class_id.to_field(), salted_initialization_hash.to_field()], + DOM_SEP__PARTIAL_ADDRESS, + )) +} +``` + +### 11. `SaltedInitializationHash::compute` +- **File**: `crates/types/src/address/salted_initialization_hash.nr:23-27` +- **Domain Separator**: `DOM_SEP__PARTIAL_ADDRESS` (27) +- **Preimage**: `[salt, initialization_hash, deployer.to_field()]` +- **Purpose**: Computing salted initialization hashes +```noir +pub fn compute(salt: Field, initialization_hash: Field, deployer: AztecAddress) -> Self { + SaltedInitializationHash::from_field(poseidon2_hash_with_separator( + [salt, initialization_hash, deployer.to_field()], + DOM_SEP__PARTIAL_ADDRESS, + )) +} +``` + +### 12. `BlockHeader::hash` +- **File**: `crates/types/src/abis/block_header.nr:44-47` +- **Domain Separator**: `DOM_SEP__BLOCK_HASH` (28) +- **Preimage**: `self.serialize()` (full block header serialization) +- **Purpose**: Hashing block headers +```noir +impl Hash for BlockHeader { + fn hash(self) -> Field { + poseidon2_hash_with_separator(self.serialize(), DOM_SEP__BLOCK_HASH) + } +} +``` + +### 13. `TxRequest::hash` +- **File**: `crates/types/src/abis/transaction/tx_request.nr:31-34` +- **Domain Separator**: `DOM_SEP__TX_REQUEST` (33) +- **Preimage**: `self.serialize()` (full tx request serialization) +- **Purpose**: Hashing transaction requests +```noir +impl Hash for TxRequest { + fn hash(self) -> Field { + poseidon2_hash_with_separator(self.serialize(), DOM_SEP__TX_REQUEST) + } +} +``` + +### 14. `PublicKeys::hash` +- **File**: `crates/types/src/public_keys.nr:103-108` +- **Domain Separator**: `DOM_SEP__PUBLIC_KEYS_HASH` (52) +- **Preimage**: `self.serialize()` (full public keys serialization) +- **Purpose**: Hashing public keys +```noir +impl PublicKeys { + pub fn hash(self) -> PublicKeysHash { + PublicKeysHash::from_field(poseidon2_hash_with_separator( + self.serialize(), + DOM_SEP__PUBLIC_KEYS_HASH as Field, + )) + } +} +``` + +### 15. `PrivateToPublicKernelCircuitPublicInputs::hash` +- **File**: `crates/types/src/abis/kernel_circuit_public_inputs/private_to_public_kernel_circuit_public_inputs.nr:38-41` +- **Domain Separator**: `DOM_SEP__PUBLIC_TX_HASH` (56) +- **Preimage**: `self.serialize()` (full circuit public inputs serialization) +- **Purpose**: Computing public transaction hashes +```noir +impl Hash for PrivateToPublicKernelCircuitPublicInputs { + fn hash(self) -> Field { + poseidon2_hash_with_separator(self.serialize(), DOM_SEP__PUBLIC_TX_HASH) + } +} +``` + +### 16. `PrivateToRollupKernelCircuitPublicInputs::hash` +- **File**: `crates/types/src/abis/kernel_circuit_public_inputs/private_to_rollup_kernel_circuit_public_inputs.nr:34-37` +- **Domain Separator**: `DOM_SEP__PRIVATE_TX_HASH` (57) +- **Preimage**: `self.serialize()` (full circuit public inputs serialization) +- **Purpose**: Computing private transaction hashes +```noir +impl Hash for PrivateToRollupKernelCircuitPublicInputs { + fn hash(self) -> Field { + poseidon2_hash_with_separator(self.serialize(), DOM_SEP__PRIVATE_TX_HASH) + } +} +``` + +### 17. `ProtocolContracts::hash` +- **File**: `crates/types/src/abis/protocol_contracts.nr:30-35` +- **Domain Separator**: `DOM_SEP__PROTOCOL_CONTRACTS` (61) +- **Preimage**: `self.derived_addresses.map(|address| address.to_field())` +- **Purpose**: Hashing protocol contracts +```noir +pub fn hash(self) -> Field { + poseidon2_hash_with_separator( + self.derived_addresses.map(|address| address.to_field()), + DOM_SEP__PROTOCOL_CONTRACTS, + ) +} +``` + +--- + +## Hashes WITHOUT Domain Separation + +> **Note:** These hashes use `poseidon2_hash()` directly with no domain separator, meaning preimage collision is possible between hashes with the same number of elements. + +### 2-Element Preimages + +#### 1. `merkle_hash` +- **File**: `crates/types/src/hash.nr:157-159` +- **Preimage**: `[left, right]` +- **Purpose**: Merkle tree hashing (combining left and right children) +```noir +pub fn merkle_hash(left: Field, right: Field) -> Field { + poseidon2_hash([left, right]) +} +``` + +#### 2. `compute_siloed_private_log_field` +- **File**: `crates/types/src/hash.nr:127-129` +- **Preimage**: `[contract_address.to_field(), field]` +- **Purpose**: Siloing private log fields +```noir +pub fn compute_siloed_private_log_field(contract_address: AztecAddress, field: Field) -> Field { + poseidon2_hash([contract_address.to_field(), field]) +} +``` + +#### 3. `derive_storage_slot_in_map` +- **File**: `crates/types/src/storage/map.nr:3-8` +- **Preimage**: `[storage_slot, key.to_field()]` +- **Purpose**: Deriving storage slots for map entries +```noir +pub fn derive_storage_slot_in_map(storage_slot: Field, key: K) -> Field +where + K: ToField, +{ + poseidon2_hash([storage_slot, key.to_field()]) +} +``` + +#### 4. `accumulate_block_headers_hash` +- **File**: `crates/rollup-lib/src/block_merge/utils/merge_block_rollups.nr:4-6` +- **Preimage**: `[left_hash, right_hash]` +- **Purpose**: Accumulating block headers during merge +```noir +pub fn accumulate_block_headers_hash(left_hash: Field, right_hash: Field) -> Field { + poseidon2_hash([left_hash, right_hash]) +} +``` + +#### 5. `BlobAccumulator::accumulate` (z_acc) +- **File**: `crates/blob/src/abis/blob_accumulator.nr:133` +- **Preimage**: `[self.z_acc, other.z_i]` +- **Purpose**: Accumulating z challenges for blob batching +```noir +z_acc: poseidon2_hash([self.z_acc, other.z_i]), +``` + +#### 6. `BlobAccumulator::accumulate` (gamma_acc) +- **File**: `crates/blob/src/abis/blob_accumulator.nr:136` +- **Preimage**: `[self.gamma_acc, hashed_y_i]` +- **Purpose**: Accumulating gamma for blob batching +```noir +gamma_acc: poseidon2_hash([self.gamma_acc, hashed_y_i]), +``` + +#### 7. `validate_final_blob_batching_challenges` +- **File**: `crates/blob/src/utils/validate_final_blob_batching_challenges.nr:14` +- **Preimage**: `[accumulator.gamma_acc, accumulator.z_acc]` +- **Purpose**: Computing final gamma challenge for blob batching validation +```noir +let gamma = poseidon2_hash([accumulator.gamma_acc, accumulator.z_acc]); +``` + +### 3-Element Preimages + +#### 8. `Point::hash` +- **File**: `crates/types/src/point.nr:16-19` +- **Preimage**: `self.serialize()` = `[x, y, is_infinite]` +- **Purpose**: Hashing elliptic curve points +```noir +impl Hash for Point { + fn hash(self) -> Field { + poseidon2_hash(self.serialize()) + } +} +``` + +#### 9. `NullifierLeafPreimage::hash` +- **File**: `crates/types/src/abis/nullifier_leaf_preimage.nr:21-28` +- **Preimage**: `self.serialize()` = `[nullifier, next_nullifier, next_index]` +- **Purpose**: Hashing nullifier leaf preimages for indexed merkle tree +```noir +impl Hash for NullifierLeafPreimage { + fn hash(self) -> Field { + if self.is_empty() { + 0 + } else { + crate::hash::poseidon2_hash(self.serialize()) + } + } +} +``` + +#### 10. `compute_blob_challenge` / `evaluate_blob` +- **File**: `crates/blob/src/blob_batching.nr:35-42`, `crates/blob/src/blob.nr:322` +- **Preimage**: `[blob_fields_hash, kzg_commitment[0], kzg_commitment[1]]` +- **Purpose**: Computing challenge z for blob evaluation +```noir +let challenge = poseidon2_hash([blob_fields_hash, compressed_fields[0], compressed_fields[1]]); +``` + +#### 11. `BlobAccumulator::init` / `accumulate` (hashed y) +- **File**: `crates/blob/src/abis/blob_accumulator.nr:79, 119` +- **Preimage**: `y_i.get_limbs().map(|l| l as Field)` (3 limbs of BLS12_381_Fr) +- **Purpose**: Hashing blob evaluation result for gamma accumulation +```noir +let hashed_y_0 = poseidon2_hash(first_output.y_i.get_limbs().map(|l| l as Field)); +``` + +### 4-Element Preimages + +#### 12. `PublicDataTreeLeafPreimage::hash` +- **File**: `crates/types/src/data/public_data_tree_leaf_preimage.nr:21-33` +- **Preimage**: `[slot, value, next_index, next_slot]` +- **Purpose**: Hashing public data tree leaf preimages +```noir +impl Hash for PublicDataTreeLeafPreimage { + fn hash(self) -> Field { + if self.is_empty() { + 0 + } else { + crate::hash::poseidon2_hash([ + self.slot, + self.value, + (self.next_index as Field), + self.next_slot, + ]) + } + } +} +``` + +### Variable-Length Preimages + +#### 13. `DelayedPublicMutableValues::hash` +- **File**: `crates/types/src/delayed_public_mutable/delayed_public_mutable_values.nr:131-137` +- **Preimage**: `self.pack()` (packed representation of delayed mutable values) +- **Purpose**: Hashing delayed public mutable values +```noir +impl Hash for DelayedPublicMutableValues +where + T: Packable, +{ + fn hash(self) -> Field { + poseidon2_hash(self.pack()) + } +} +``` + +#### 14. `validate_with_hash_hints` (inline hash) +- **File**: `crates/types/src/delayed_public_mutable/with_hash.nr:33` +- **Preimage**: `with_hash_value_hint.pack()` +- **Purpose**: Validating hashed hints for delayed public mutable values +```noir +let hashed_value = poseidon2_hash(with_hash_value_hint.pack()); +``` + +#### 15. `compute_contract_class_log_hash` +- **File**: `crates/types/src/hash.nr:142-144` +- **Preimage**: `log` (array of CONTRACT_CLASS_LOG_SIZE_IN_FIELDS) +- **Purpose**: Hashing contract class logs +```noir +pub fn compute_contract_class_log_hash(log: [Field; CONTRACT_CLASS_LOG_SIZE_IN_FIELDS]) -> Field { + poseidon2_hash(log) +} +``` + +#### 16. `FunctionSelector::from_signature` +- **File**: `crates/types/src/abis/function_selector.nr:33-38` +- **Preimage**: Signature string bytes converted to fields (via `poseidon2_hash_bytes`) +- **Purpose**: Computing function selectors from signature strings +```noir +pub fn from_signature(signature: str) -> Self { + let bytes = signature.as_bytes(); + let hash = crate::hash::poseidon2_hash_bytes(bytes); + FunctionSelector::from_field(hash) +} +``` +- **Note**: Uses `poseidon2_hash_bytes` which packs bytes into 31-byte field chunks, then hashes + +### Test-Only Hashes + +#### 17. Test functions in `blob.nr` +- **File**: `crates/blob/src/blob.nr:362, 389` +- **Preimage**: `blob_fields` (array of blob field elements) +- **Purpose**: Computing blob fields hash in tests +```noir +let blob_fields_hash = poseidon2_hash(blob_fields); +``` + +#### 18. Test in `rollup_fixture_builder.nr` +- **File**: `crates/rollup-lib/src/tests/rollup_fixture_builder.nr:395` +- **Preimage**: `[end_block_accumulator.gamma_acc, z]` +- **Purpose**: Test fixture for gamma computation +```noir +let gamma = unsafe { __from_field(poseidon2_hash([end_block_accumulator.gamma_acc, z])) }; +``` + +#### 19. Test in `blob_tests.nr` +- **File**: `crates/rollup-lib/src/checkpoint_root/tests/blob_tests.nr:109` +- **Preimage**: `end_blob_accumulator.y_acc.get_limbs().map(|l| l as Field)` +- **Purpose**: Hashing y_acc for test verification +```noir +poseidon2_hash(end_blob_accumulator.y_acc.get_limbs().map(|l| l as Field)); +``` + +--- + +## Summary + +| Category | Count | +|----------|-------| +| Hashes WITH domain separator | 17 | +| Hashes WITHOUT domain separator | 19 | +| **Total unique domain separators used** | **16** | + +### Domain Separators Actually Used: + +| Value | Constant | Usage Count | +|-------|----------|-------------| +| 2 | `DOM_SEP__NOTE_HASH_NONCE` | 1 | +| 3 | `DOM_SEP__UNIQUE_NOTE_HASH` | 1 | +| 4 | `DOM_SEP__SILOED_NOTE_HASH` | 1 | +| 7 | `DOM_SEP__OUTER_NULLIFIER` | 1 | +| 11 | `DOM_SEP__FUNCTION_LEAF` | 1 | +| 15 | `DOM_SEP__CONTRACT_ADDRESS_V1` | 1 | +| 16 | `DOM_SEP__CONTRACT_CLASS_ID` | 1 | +| 23 | `DOM_SEP__PUBLIC_LEAF_INDEX` | 1 | +| 27 | `DOM_SEP__PARTIAL_ADDRESS` | 2 | +| 28 | `DOM_SEP__BLOCK_HASH` | 1 | +| 33 | `DOM_SEP__TX_REQUEST` | 1 | +| 52 | `DOM_SEP__PUBLIC_KEYS_HASH` | 1 | +| 56 | `DOM_SEP__PUBLIC_TX_HASH` | 1 | +| 57 | `DOM_SEP__PRIVATE_TX_HASH` | 1 | +| 61 | `DOM_SEP__PROTOCOL_CONTRACTS` | 1 | +| (dynamic) | `app_secret_generator` | 1 | + +### Unused Domain Separators (defined but not used in noir-protocol-circuits): + +Values 1, 5, 6, 13, 20, 32, 34, 43, 44, 45, 46, 47, 48, 49, 50, 51, 53, 54, 55, 58, 59, 60 + +Note: Many previously-defined domain separators (8, 9, 10, 12, 14, 17, 18, 19, 21, 22, 24, 25, 26, 29, 30, 31, 41) were removed as unused. The remaining unused separators above are used in other parts of the Aztec codebase (e.g., `aztec-nr`, `yarn-project`). + +--- + +## Security Considerations + +### Potential Collision Risks + +Hashes without domain separation could potentially collide if their preimages have the same structure. Notable cases: + +1. **2-element hashes without separation:** + - `merkle_hash([left, right])` + - `derive_storage_slot_in_map([storage_slot, key])` + - `compute_siloed_private_log_field([contract_address, field])` + - `accumulate_block_headers_hash([left_hash, right_hash])` + - Blob accumulator: `poseidon2_hash([self.z_acc, other.z_i])` + - Blob accumulator: `poseidon2_hash([self.gamma_acc, hashed_y_i])` + +2. **3-element hashes without separation:** + - `Point::hash([x, y, is_infinite])` + - `NullifierLeafPreimage::hash([nullifier, next_nullifier, next_index])` + - Blob challenge: `poseidon2_hash([blob_fields_hash, commitment[0], commitment[1]])` + +3. **4-element hashes without separation:** + - `PublicDataTreeLeafPreimage::hash([slot, value, next_index, next_slot])` + +4. **Variable-length hashes without separation:** + - `FunctionSelector::from_signature` (via `poseidon2_hash_bytes`) + - `compute_contract_class_log_hash` + - `DelayedPublicMutableValues::hash` + +### Why This May Be Acceptable + +- **Different contexts**: Many of these hashes operate in isolated contexts (e.g., blob operations vs merkle trees) +- **Structural differences**: The semantic meaning of fields differs even if count matches +- **Value ranges**: Some fields have constrained value ranges that prevent overlap + +### Recommendations + +1. Consider adding domain separation to tree leaf hashes (`NullifierLeafPreimage`, `PublicDataTreeLeafPreimage`) if they could share preimage space with other 3-4 element hashes +2. Document why certain hashes intentionally omit domain separation +3. Ensure blob-related hashes cannot collide with protocol-level hashes diff --git a/noir-projects/noir-protocol-circuits/audit.md b/noir-projects/noir-protocol-circuits/audit.md new file mode 100644 index 000000000000..3a4f0989e5d2 --- /dev/null +++ b/noir-projects/noir-protocol-circuits/audit.md @@ -0,0 +1,57 @@ + +## External Dependencies + +### Protocol Circuits - External Dependencies + +The following external dependencies are used across the protocol circuits: + +| Dependency | Version | Repository | +|------------|---------|------------| +| sha256 | v0.3.0 | https://github.com/noir-lang/sha256 | +| poseidon | v0.1.1 | https://github.com/noir-lang/poseidon | +| bignum | v0.8.2 | https://github.com/noir-lang/noir-bignum | +| bigcurve | v0.12.0 | https://github.com/noir-lang/noir_bigcurve | + +#### Internal Dependencies (within noir-protocol-circuits) + +- `types` - Core type definitions (depends on sha256, poseidon) +- `blob` - Blob handling (depends on bignum, bigcurve, types) +- `parity_lib` - Parity tree library (depends on types) +- `rollup_lib` - Rollup library (depends on bignum, bigcurve, types, parity_lib, blob) +- `private_kernel_lib` - Private kernel library (depends on types) + +### aztec-nr Dependencies + +#### External Dependencies + +| Dependency | Version | Repository | +|------------|---------|------------| +| sha256 | v0.3.0 | https://github.com/noir-lang/sha256 | +| poseidon | v0.1.1 | https://github.com/noir-lang/poseidon | + +#### Cross-Project Dependencies + +- `protocol_types` - References `noir-protocol-circuits/crates/types` + +#### Internal Dependencies (within aztec-nr) + +- `aztec` - Core Aztec library (depends on protocol_types, sha256, poseidon) +- `address_note` - Address note type (depends on aztec) +- `balance_set` - Balance set utilities (depends on aztec, uint_note) +- `compressed_string` - Compressed string utilities (depends on aztec) +- `field_note` - Field note type (depends on aztec) +- `uint_note` - Uint note type (depends on aztec) + +## Primitives that might need attention from the crypto team + +noir-protocol-circuits/crates/types/src/hash/poseidon2_chunks.nr +noir-protocol-circuits/crates/types/src/poseidon2.nr + + +# Random thoughts as I go. + +See arrays.md for a report on the state of arrays.nr. I didn't have time to ask Claude to fix it and add tests. + +Should we strive to use the `for_each_i` helper functions everywhere, to avoid accidental bugs repeating the "dynamic for loop" pattern everywhere? + +I need to finish drawing a big diagram of this whole thing. Probably only ready some time mid January. diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/abis/blob_accumulator.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/abis/blob_accumulator.nr index 94f98f4989d3..2de66b48fc74 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/abis/blob_accumulator.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/abis/blob_accumulator.nr @@ -74,6 +74,7 @@ impl BlobAccumulator { * For all blobs i > 0 accumulated, see the below documentation for accumulate(). * */ + // AUDIT_COMMENT: we might be able to optimise this `init` branch away pub fn init(first_output: BlobAccumulationInputs, final_gamma: BLS12_381_Fr) -> Self { // TODO(#13608): use a BLS12 based hash? Is using BN based safe - since the output is smaller is there a skew? let hashed_y_0 = poseidon2_hash(first_output.y_i.get_limbs().map(|l| l as Field)); @@ -94,7 +95,7 @@ impl BlobAccumulator { * NB: blob_commitments_hash is written as v below * * Each accumulation: - * - v_acc := sha256(v_acc, C_i) + * - v_acc := sha256(v_acc, C_i) // TODO: this name doesn't seem to appear anymore. Rename in this comment. * - z_acc := poseidon2(z_acc, z_i) * - y_acc := y_acc + (gamma^i * y_i) * - c_acc := c_acc + (gamma^i * c_i) diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/abis/final_blob_batching_challenges.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/abis/final_blob_batching_challenges.nr index 5c1cbe817512..d449bc874e39 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/abis/final_blob_batching_challenges.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/abis/final_blob_batching_challenges.nr @@ -9,7 +9,7 @@ use types::{ * Final values z and gamma are injected into each block root circuit. We ensure they are correct by: * - Checking equality in each block merge circuit and propagating up * - Checking final z_acc == z in root circuit -* - Checking final gamma_acc == gamma in root circuit +* - Checking final gamma_acc == gamma in root circuit <-- TODO: this doesn't reflect what happens. gamma = h(gamma_acc, z_acc). Explore further. * * - z = H(...H(H(z_0, z_1) z_2)..z_n) * - where z_i = H(H(fields of blob_i), C_i), diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/blob_batching.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/blob_batching.nr index a5cbf8a39f7c..fdcb25201b70 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/blob_batching.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/blob_batching.nr @@ -4,7 +4,7 @@ use crate::{ FinalBlobBatchingChallenges, }, blob::barycentric_evaluate_blob_at_z, - utils::{compress_to_blob_commitment, validate_canonical_point_at_infinity}, + utils::{compress_to_blob_commitment, validate_canonical_representation_if_infinity}, }; use bigcurve::BigCurve; use bignum::BLS12_381_Fr; @@ -42,7 +42,7 @@ fn compute_blob_challenge( challenge } -/// Evaluates each blob required for an L1 block: +/// Evaluates each blob required for an L1 checkpoint: /// - Compresses each of the blob's injected commitments. /// - Evaluates each blob individually to find its challenge `z_i` and evaluation `y_i`. /// - Updates the batched blob accumulator. @@ -54,13 +54,16 @@ pub fn evaluate_blobs_and_batch( final_blob_challenges: FinalBlobBatchingChallenges, start_accumulator: BlobAccumulator, ) -> BlobAccumulator { - // The rationale behind this is that the start accumulator is hinted, and in merge we just assert that left.end.eq(right.start) + // The rationale behind this line is that the start accumulator is hinted, and in merge we just assert that left.end.eq(right.start) // And on root we check that left.start.eq(empty()). // However, currently the equals method of big curve points doesn't check for equality of the limbs on infinity points. // This means that infinity points can be swapped with non canonical versions without the eq realizing it. // I don't think there is a way to take advantage of that, since we should never have infinity points in c_acc, // but as good measure we'll validate that it's canonical here. - validate_canonical_point_at_infinity(start_accumulator.c_acc); + // Also, the Root Rollup circuit technically validates that the start accumulator of the checkpoint is Empty, + // and the Empty impl uses the canonical representation of infinity. + // Q: Is it possible for a subsequent checkpoint to have a nonempty start_accumulator.c_acc? I think not. + validate_canonical_representation_if_infinity(start_accumulator.c_acc); let mut end_accumulator = start_accumulator; for i in 0..NumBlobs { @@ -68,7 +71,8 @@ pub fn evaluate_blobs_and_batch( let commitment_point = kzg_commitments_points[i]; // We need to validate that the point is on the curve and canonical (0,0 if infinity), since it's hinted in this verification scheme commitment_point.validate_on_curve(); - validate_canonical_point_at_infinity(commitment_point); + // Strangely, `validate_on_curve` doesn't validate the point at infinity, so we check that here: + validate_canonical_representation_if_infinity(commitment_point); let c_i = compress_to_blob_commitment(commitment_point); // Note that with multiple blobs per block, each blob uses the same blob_fields_hash in z_i. @@ -94,6 +98,7 @@ pub fn evaluate_blobs_and_batch( // items in ALL blobs, not just blob `i`. let should_accumulate = i * FIELDS_PER_BLOB < num_fields; if should_accumulate { + // AUDIT_COMMENT: As an optimisation, we could pull the `i=0` case outside the `for` loop, so that we don't run the costly `init` code unnecessarily five more times. if (i == 0) & start_accumulator.is_empty() { // Call `init` if it starts from an empty accumulator. This must always occur at blob i = 0 in the first // checkpoint of an epoch (and nowhere else). diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/utils/mod.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/utils/mod.nr index 005300d6548a..920250edbcda 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/utils/mod.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/utils/mod.nr @@ -4,4 +4,4 @@ mod validate_point; pub use compress_to_blob_commitment::compress_to_blob_commitment; pub use validate_final_blob_batching_challenges::validate_final_blob_batching_challenges; -pub use validate_point::validate_canonical_point_at_infinity; +pub use validate_point::validate_canonical_representation_if_infinity; diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/utils/validate_final_blob_batching_challenges.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/utils/validate_final_blob_batching_challenges.nr index a36ba06761a8..d5c79e4bfac1 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/utils/validate_final_blob_batching_challenges.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/utils/validate_final_blob_batching_challenges.nr @@ -9,8 +9,10 @@ pub fn validate_final_blob_batching_challenges( accumulator: BlobAccumulator, challenges: FinalBlobBatchingChallenges, ) { + // AUDIT_COMMENT: I'm not yet convinced that the value of `z` that's been propagated through the checkpoint circuits is being asserted to equal accumulator.z_acc. Need to spend longer reading this. assert_eq(accumulator.z_acc, challenges.z, "Final blob challenge z mismatch."); let gamma = poseidon2_hash([accumulator.gamma_acc, accumulator.z_acc]); + // AUDIT_COMMENT: come back to this to check whether this `to_field` conversion is lossy assert_eq(gamma, bignum::to_field(challenges.gamma), "Final blob challenge gamma mismatch."); } diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/utils/validate_point.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/utils/validate_point.nr index ed70c98615ec..bf184909bb10 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/utils/validate_point.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/utils/validate_point.nr @@ -3,7 +3,7 @@ use bigcurve::BigCurve; use bignum::{BigNum, fields::bls12_381Fq::BLS12_381_Fq}; /// Validates that if the point is at infinity, it has the canonical x and y coordinates. -pub fn validate_canonical_point_at_infinity(point: BLSPoint) { +pub fn validate_canonical_representation_if_infinity(point: BLSPoint) { if point.is_infinity() { let canonical_point_at_infinity = BLSPoint::point_at_infinity(); assert_eq(point.x, canonical_point_at_infinity.x, "Non canonical x coordinate at infinity"); @@ -14,7 +14,7 @@ pub fn validate_canonical_point_at_infinity(point: BLSPoint) { #[test] fn should_accept_canonical_point_at_infinity() { let point = BLSPoint { x: BLS12_381_Fq::zero(), y: BLS12_381_Fq::zero(), is_infinity: true }; - validate_canonical_point_at_infinity(point); + validate_canonical_representation_if_infinity(point); } #[test(should_fail_with = "Non canonical x coordinate at infinity")] @@ -22,7 +22,7 @@ fn should_fail_if_non_canonical_x_coordinate_point_at_infinity() { let x = BLS12_381_Fq::one(); let y = BLS12_381_Fq::zero(); let point = BLSPoint { x, y, is_infinity: true }; - validate_canonical_point_at_infinity(point); + validate_canonical_representation_if_infinity(point); } #[test(should_fail_with = "Non canonical y coordinate at infinity")] @@ -30,5 +30,5 @@ fn should_fail_if_non_canonical_y_coordinate_point_at_infinity() { let x = BLS12_381_Fq::zero(); let y = BLS12_381_Fq::one(); let point = BLSPoint { x, y, is_infinity: true }; - validate_canonical_point_at_infinity(point); + validate_canonical_representation_if_infinity(point); } diff --git a/noir-projects/noir-protocol-circuits/crates/private-kernel-init/src/main.nr b/noir-projects/noir-protocol-circuits/crates/private-kernel-init/src/main.nr index c0adcaf9b73b..4d8dd30cdefd 100644 --- a/noir-projects/noir-protocol-circuits/crates/private-kernel-init/src/main.nr +++ b/noir-projects/noir-protocol-circuits/crates/private-kernel-init/src/main.nr @@ -6,7 +6,9 @@ use types::abis::protocol_contracts::ProtocolContracts; use types::abis::transaction::tx_request::TxRequest; /** - * @param first_nullifier_hint: 0 if there is no nullifier yet; otherwise it's a claim on whatever has been pushed as the 0th nullifier already by some private call. + * @param first_nullifier_hint: 0 if there is no nullifier yet; otherwise it's a claim on whatever + * has been pushed as the 0th nullifier already by some private call. (Note: the 0th nullifier is + * used to compute note nonces, which are injected into notes to prevent Faerie-Gold attacks). */ fn main( tx_request: TxRequest, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/block_merge_rollup_private_inputs.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/block_merge_rollup_private_inputs.nr index 5b682dfb5db0..6adcd71e7a22 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/block_merge_rollup_private_inputs.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/block_merge_rollup_private_inputs.nr @@ -21,6 +21,7 @@ global ALLOWED_LEFT_ROLLUP_VK_INDICES: [u32; 6] = ALLOWED_RIGHT_ROLLUP_VK_INDICE // The first block roots may only appear as the left child of the block merge rollup. // However, this doesn't prevent having 2 first block roots in the same checkpoint. We rely on the checks in // `validate_consecutive_block_rollups` and `checkpoint_root_inputs_validator` to enforce this. + // Q: should we add more explicit checks of all the valid _pairs_? It makes me nervous to rely on the greedy tree checks. BLOCK_ROOT_FIRST_ROLLUP_VK_INDEX, BLOCK_ROOT_SINGLE_TX_FIRST_ROLLUP_VK_INDEX, BLOCK_ROOT_EMPTY_TX_FIRST_ROLLUP_VK_INDEX, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/utils/merge_block_rollups.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/utils/merge_block_rollups.nr index 764ed7601f65..8c79151e4759 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/utils/merge_block_rollups.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/utils/merge_block_rollups.nr @@ -2,6 +2,7 @@ use crate::{abis::BlockRollupPublicInputs, tx_merge::accumulate_out_hash}; use types::hash::poseidon2_hash; pub fn accumulate_block_headers_hash(left_hash: Field, right_hash: Field) -> Field { + // TODO: this needs a domain separator, to be safe. poseidon2_hash([left_hash, right_hash]) } @@ -9,6 +10,7 @@ pub fn merge_block_rollups( left: BlockRollupPublicInputs, right: BlockRollupPublicInputs, ) -> BlockRollupPublicInputs { + // Q: what is this block_headers_hash and where does it get used? let block_headers_hash = accumulate_block_headers_hash(left.block_headers_hash, right.block_headers_hash); diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/utils/validate_consecutive_block_rollups.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/utils/validate_consecutive_block_rollups.nr index 6409f4be5bdf..fc4d369998a1 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/utils/validate_consecutive_block_rollups.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_merge/utils/validate_consecutive_block_rollups.nr @@ -35,12 +35,17 @@ fn assert_prev_block_rollups_follow_on_from_each_other( "Mismatched sponge blobs: expected right.start_sponge_blob to match left.end_sponge_blob", ); + // Notice: consecutive blocks within a checkpoint can have the same timestamp. + // I.e. the `<=` is _intentional_. assert( left.end_timestamp <= right.start_timestamp, "Rollup block timestamps do not follow on from each other", ); - // Non-empty `in_hash` originates from the first block root and must propagate through all merge steps via the left - // rollup only. Which means the right rollup must not carry or propagate the `in_hash`. + // TODO: Consider extracting into its own function: + // A non-empty `in_hash` originates from the first block root only, and is propagated through all merge + // steps via the left rollup only (see merge_block_rollups.nr). + // We prevent the right rollup from propagating a nonzero `in_hash`, so it's impossible for any + // block but the first to propagate a nonzero `in_hash` up to the checkpoint root. assert_eq(right.in_hash, 0, "Right rollup must not carry in_hash"); } diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/block_root_empty_tx_first_rollup_private_inputs.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/block_root_empty_tx_first_rollup_private_inputs.nr index 2a8d9efdd201..961492ab5c69 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/block_root_empty_tx_first_rollup_private_inputs.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/block_root_empty_tx_first_rollup_private_inputs.nr @@ -37,6 +37,7 @@ impl BlockRootEmptyTxFirstRollupPrivateInputs { validate_parity_root(self.parity_root, self.constants.vk_tree_root); // Since it's the first block in a checkpoint, the start sponge blob is empty. + // Refactor: Consider just assigning this within `new_from_no_rollups`, much like we set `num_txs = 0`, etc. let start_sponge_blob = SpongeBlob::init(); BlockRollupPublicInputsComposer::new_from_no_rollups( diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/components/block_rollup_public_inputs_composer.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/components/block_rollup_public_inputs_composer.nr index c7fe5769688f..62c3496528af 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/components/block_rollup_public_inputs_composer.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/components/block_rollup_public_inputs_composer.nr @@ -51,7 +51,7 @@ impl BlockRollupPublicInputsComposer { end_tree_snapshots: previous_state.partial, start_sponge_blob, end_sponge_blob: start_sponge_blob, - timestamp, + timestamp, // Will be constrained to be > the previous. // The followings are 0 since there are no tx effects in this block. out_hash: 0, accumulated_fees: 0, @@ -88,7 +88,7 @@ impl BlockRollupPublicInputsComposer { out_hash: rollup.out_hash, accumulated_fees: rollup.accumulated_fees, accumulated_mana_used: rollup.accumulated_mana_used, - num_txs: rollup.num_txs, + num_txs: rollup.num_txs, // Q: should we assert that this is `> 0`? // The followings are updated by calling `update_l1_to_l2_tree_snapshots` explicitly. previous_l1_to_l2: rollup.constants.l1_to_l2_tree_snapshot, in_hash: 0, @@ -178,8 +178,10 @@ impl BlockRollupPublicInputsComposer { // Block number equals the index at which the new block header hash is inserted into the archive tree. let block_number = self.previous_archive.next_available_leaf_index as u32; + // TODO: this feels like "validation" that should therefore be done by the "Validator", rather than this "Composer". + // TODO: consider doing this as part of the "empty" variant's validation. It doesn't seem to belong here? Or do all variants need this check? // Note: For non-empty blocks, a check is performed in `block_root_rollup_inputs_validator.nr` to ensure that - // the leaf index matches the block number (u32) in the tx constants, which implies that the leaf index does not + // the leaf index (field) matches the block number (u32) in the tx constants, which implies that the leaf index does not // exceed u32. However, for empty blocks, without the check below, the block number would be truncated to a // smaller value (dropping the high bits) if the leaf index exceeded u32. This would allow the block to follow // a block with a much smaller block number, while `last_archive` would still contain the large index. @@ -188,6 +190,10 @@ impl BlockRollupPublicInputsComposer { // Note: An empty block with a mismatched `last_archive` leaf index could be rolled up if it is in the first // checkpoint of an epoch, since only the `last_archive.root` is output from the root rollup and checked on L1. self.previous_archive.next_available_leaf_index.assert_max_bit_size::<32>(); + std::static_assert( + ARCHIVE_HEIGHT <= 32, + "Need up update the max bit size on the line above", + ); // TODO: do we need these safety nets for all tree heights? let global_variables = GlobalVariables { chain_id: self.constants.chain_id, @@ -202,7 +208,14 @@ impl BlockRollupPublicInputsComposer { // Absorb data for this block into the end sponge blob. let mut block_end_sponge_blob = self.end_sponge_blob; + // `in_hash` is not 0 if and only if it's the first block in the checkpoint. + // Note: the `in_hash` of the first block in the checkpoint is asserted to be nonzero + // by the Checkpoint Root circuit. + // Note: the `in_hash` of subsequent blocks in the checkpoint is asserted to be 0 + // within `validate_consecutive_block_rollups.nr`, by asserting that the `in_hash` of all + // "right" rollups is `0`. + // Q: what if the L1 subtree being copied to L2 is empty? What is the value in this case? let is_first_block_in_checkpoint = self.in_hash != 0; block_end_sponge_blob.absorb_block_end_data( global_variables, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/components/block_root_rollup_inputs_validator.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/components/block_root_rollup_inputs_validator.nr index c59ccd17fe2f..8deb8a488a39 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/components/block_root_rollup_inputs_validator.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/block_root/components/block_root_rollup_inputs_validator.nr @@ -39,6 +39,7 @@ pub fn validate_previous_rollups { merged_rollup: BlockRollupPublicInputs, + // The below are all hints: start_blob_accumulator: BlobAccumulator, final_blob_challenges: FinalBlobBatchingChallenges, blobs_fields: [Field; FIELDS_PER_BLOB * NumBlobs], @@ -30,6 +31,7 @@ pub struct CheckpointRollupPublicInputsComposer { impl CheckpointRollupPublicInputsComposer { pub fn new( previous_rollups: [BlockRollupPublicInputs; NumPreviousRollups], + // The below args are all hints: start_blob_accumulator: BlobAccumulator, final_blob_challenges: FinalBlobBatchingChallenges, blobs_fields: [Field; NumBlobs * FIELDS_PER_BLOB], @@ -62,9 +64,15 @@ impl CheckpointRollupPublicInputsComposer { prover_id: merged_rollup.constants.prover_id, }; + // Note: it might seem strange that we only write to the 0th index of an array of length + // `AZTEC_MAX_EPOCH_DURATION`. Why not output a single `Field`? It's because we want the + // layout of the public inputs of this Checkpoint Root circuit to match that of the Checkpoint + // Merge circuit. In the Checkpoint Merge circuit, we use more elements of this array: we + // progressively merge the arrays from left and right subtrees. let mut checkpoint_header_hashes = [0; AZTEC_MAX_EPOCH_DURATION]; checkpoint_header_hashes[0] = self.create_checkpoint_header().hash(); + // Note: as per the above note. let mut fees = [FeeRecipient::empty(); AZTEC_MAX_EPOCH_DURATION]; fees[0] = FeeRecipient { recipient: merged_rollup.constants.coinbase, @@ -114,12 +122,26 @@ impl CheckpointRollupPublicInputsComposer { end_sponge_blob.absorb_checkpoint_end_marker(); let num_blob_fields = end_sponge_blob.num_absorbed_fields; - // Check that the first `num_blob_fields` of the given fields do match what's been absorbed into the sponge. - let mut expected_end_sponge = SpongeBlob::init(); - expected_end_sponge.absorb(self.blobs_fields, num_blob_fields); + + let mut expected_end_sponge_blob = SpongeBlob::init(); + + // Check that the first `num_blob_fields` of the hinted `self.blobs_fields` do match what's + // been absorbed into the sponge. + // Elaboration: + // We have an `end_sponge_blob`, which is the result of incrementally absorbing data to the sponge + // over many circuits (Tx Base circuits, Block Root circuits, and this Checkpoint Root circuit). + // We need to give this Checkpoint Root circuit access to _all_ of that underlying data, flattened, + // because later in this circuit we'll be interpolating that data into polynomials (one polynomial + // per blob that we've used-up). + // The flattened, underlying data is hinted to this circuit as `self.blobs_fields`. + // To ensure `self.blobs_fields` actually matches the data that all of the previous circuits saw, + // we repeat the entire sponge computation -- the one that was previously split across many + // circuits -- entirely within this circuit in the line below. + // We hope that `expected_end_sponge_blob == end_sponge_blob`. + expected_end_sponge_blob.absorb(self.blobs_fields, num_blob_fields); assert_eq( end_sponge_blob, - expected_end_sponge, + expected_end_sponge_blob, "Provided blob fields do not match the fields that were absorbed", ); // Check that all fields after `num_blob_fields` are zero. diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/checkpoint_root/components/checkpoint_root_inputs_validator.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/checkpoint_root/components/checkpoint_root_inputs_validator.nr index ea9d075019c0..9b76e834e9d9 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/checkpoint_root/components/checkpoint_root_inputs_validator.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/checkpoint_root/components/checkpoint_root_inputs_validator.nr @@ -42,7 +42,7 @@ impl CheckpointRootInputsVal self.validate_start_states(); - self.validate_end_states(); + // self.validate_end_states(); } fn validate_previous_rollup_proofs_and_vks(self) { @@ -72,12 +72,17 @@ impl CheckpointRootInputsVal "The start blob sponge was not correctly initialized", ); + // Note: we validate the correctness of the previous block header at the end of this function. + assert_eq( first_rollup.start_state, self.previous_block_header.state, "The start state of the checkpoint does not match the state in the previous block header", ); + // Notice: unlike blocks within the same checkpoint (which can share the same timestamp), + // the blocks of _different_ checkpoints must have strictly increasing timestamps. + // I.e. the `>` here is _intentional_. assert( first_rollup.start_timestamp > self.previous_block_header.global_variables.timestamp, "The start timestamp must be after the previous block's timestamp", @@ -87,6 +92,7 @@ impl CheckpointRootInputsVal // In `validate_consecutive_block_rollups`, it ensures that the hash only propagates through all merge steps // exclusively via the left rollup. The value in the left rollup must not be 0 when it reaches the checkpoint // root (this point). + // Q: what if the L1 subtree being copied to L2 is empty? What is the value in this case? assert( first_rollup.in_hash != 0, "in_hash must be set via the first block root in the checkpoint", @@ -94,7 +100,7 @@ impl CheckpointRootInputsVal // `previous_block_header` is provided as a private input and is used to validate the values above. // Here we make sure it's indeed the header of the previous block by checking it against the previous archive. - // The previous archive will be validated on L1. + // The correctness of the claimed previous archive will be validated when this epoch's proof is submitted to L1. self.validate_previous_block_header_in_archive(); } @@ -114,20 +120,21 @@ impl CheckpointRootInputsVal "Hash of the previous block header is not the last leaf in the archive tree", ); } - - fn validate_end_states(self) { - let end_sponge_blob = - self.previous_rollups[NumPreviousRollups - 1].public_inputs.end_sponge_blob; - - // Check that the number of absorbed blob fields is not larger than the maximum number of fields allowed. - // Note: It must not equal the max allowed number because an additional checkpoint end marker will be absorbed - // in `checkpoint_rollup_public_inputs_composer`. - assert( - end_sponge_blob.num_absorbed_fields < FIELDS_PER_BLOB * BLOBS_PER_CHECKPOINT, - "Attempted to overfill blobs", - ); - - // The `end_sponge_blob` is validated against the injected blob fields in - // `checkpoint_rollup_public_inputs_composer.nr`, after the checkpoint end marker is absorbed. - } + // AUDIT_COMMENT: Consider removing this function, and instead asserting `<=` _after_ inserting the blob end marker in sponge_blob.nr. + // It removes a function (yay!) and means if someone ever increases the size of a blob end marker, we'll + // be able to prevent accidental overflow beyond the size of the blob. (Currently, they might forget + // to update this function, if they ever increase the size of a blob end marker). + // fn validate_end_states(self) { + // let end_sponge_blob = + // self.previous_rollups[NumPreviousRollups - 1].public_inputs.end_sponge_blob; + // // Check that the number of absorbed blob fields is not larger than the maximum number of fields allowed. + // // Note: It must not equal the max allowed number because an additional checkpoint end marker will be absorbed + // // in `checkpoint_rollup_public_inputs_composer`. + // assert( + // end_sponge_blob.num_absorbed_fields < FIELDS_PER_BLOB * BLOBS_PER_CHECKPOINT, + // "Attempted to overfill blobs", + // ); + // // The `end_sponge_blob` is validated against the injected blob fields in + // // `checkpoint_rollup_public_inputs_composer.nr`, after the checkpoint end marker is absorbed. + // } } diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/root/root_rollup_private_inputs.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/root/root_rollup_private_inputs.nr index 76b0ed8b26cb..700ee874ae62 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/root/root_rollup_private_inputs.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/root/root_rollup_private_inputs.nr @@ -12,6 +12,12 @@ use types::{ traits::Empty, }; +// Q: Maybe we should also be distinguishing between "allowed left" and "allowed right" in the other circuits? +// I see we make the distinction in the block root, but not the checkpoint root. See comments in those circuits +// which suggests we should go a step further and establish valid pairs of left & right circuits (because not all combinations of pairs are valid). +// Similar comment for this circuit: not all Left & Right pairs are valid. +// - E.g. Right = CHECKPOINT_MERGE_ROLLUP_VK_INDEX; Left = CHECKPOINT_ROOT_SINGLE_BLOCK_ROLLUP_VK_INDEX is invalid. +// Sure, the greedy tree logic might protect against this already, but a 2nd line of defense sounds prudent in case there's a bug elsewhere. global ALLOWED_LEFT_ROLLUP_VK_INDICES: [u32; 3] = [ CHECKPOINT_ROOT_ROLLUP_VK_INDEX, CHECKPOINT_ROOT_SINGLE_BLOCK_ROLLUP_VK_INDEX, @@ -33,6 +39,10 @@ pub struct RootRollupPrivateInputs { impl RootRollupPrivateInputs { /// VkIndex: ROOT_ROLLUP_VK_INDEX pub fn execute(self) -> RootRollupPublicInputs { + // Q: why do some circuits perform checks through a `...Validator` struct and a `...Composer` struct, + // but the Root-Rollup and Checkpoint-Merge rollup circuits (for example) do not? + // Tbf, I kind of like the readability of this fn. + // Verify the previous rollup proofs if !dep::std::runtime::is_unconstrained() { self.previous_rollups[0].verify_proof_in_root(); @@ -75,8 +85,10 @@ impl RootRollupPrivateInputs { // // Below we check: // 1. The first accumulator of the entire epoch (left.start_blob_accumulator) is empty. + // Notice: the "empty" representation of C_acc is rightly the canonical representation of + // the point at infinity with is_infinity=true. merged.start_blob_accumulator.assert_empty("Epoch did not start with empty blob state."); - // 2. The `final_blob_challenges` matches the final state of the accumulator. + // 2. The claimed `final_blob_challenges` match the final state of the accumulator. validate_final_blob_batching_challenges( merged.end_blob_accumulator, merged.final_blob_challenges, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tail_validator.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tail_validator.nr index f4ff1e5228c3..6601a046fe62 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tail_validator.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tail_validator.nr @@ -18,13 +18,16 @@ use dep::types::{ utils::arrays::array_padded_with, }; -/// Validate the tx constants against the block constants. +// AUDIT_COMMENT: With this fn being used for both the tx-base-private and tx-base-public codepaths, it's difficult to audit that +// all items are being constrained, and keeping track of where data came from is pretty difficult. +/// Validate some of the tx constants that were propagated from private-land (from the private_tail's public inputs) +/// against the corresponding items of this circuit's block constants. /// - For private-only txs, the block constants are provided via private inputs. -/// - For public-inclusive txs, some values are copied over from the AVM proof's public inputs. This function indirectly -/// checks that the values in `private_tail` match those from the AVM. +/// - For public-inclusive txs, some values are copied over from the AVM proof's public inputs. This function +/// checks that the relevant values in `private_tail` match those from the AVM. pub fn validate_tx_constant_data( tx_constants: TxConstantData, - block_constants: BlockConstantData, + block_constants: BlockConstantData, // where applicable, these constants have been copied-over from the avm's public inputs. anchor_block_archive_sibling_path: [Field; ARCHIVE_HEIGHT], ) { // Values from `tx_constants` to be checked in this function: @@ -35,11 +38,13 @@ pub fn validate_tx_constant_data( let tx_vk_tree_root = tx_constants.vk_tree_root; let tx_protocol_contracts_hash = tx_constants.protocol_contracts_hash; - // Values from `block_constants` to be checked against those from `tx_constants`: + // Values from `block_constants` to be checked against those from `tx_constants`. + // (When executing this fn as part of the public tx base, we comment when an item has been copied-over + // from the avm's public inputs). let block_last_archive_root = block_constants.last_archive.root; - let block_chain_id = block_constants.global_variables.chain_id; - let block_version = block_constants.global_variables.version; - let block_gas_fees = block_constants.global_variables.gas_fees; + let block_chain_id = block_constants.global_variables.chain_id; // from avm + let block_version = block_constants.global_variables.version; // from avm + let block_gas_fees = block_constants.global_variables.gas_fees; // from avm let block_vk_tree_root = block_constants.vk_tree_root; let block_protocol_contracts_hash = block_constants.protocol_contracts_hash; @@ -75,6 +80,8 @@ pub fn validate_tx_constant_data( "Mismatched vk_tree_root between kernel and rollup", ); + // This assertion is not strictly necessary for public tx base, since the value in `block_constants` is copied over + // from `tx_constants`. However, using this single function for both private and public tx bases is cleaner. assert_eq( tx_protocol_contracts_hash, block_protocol_contracts_hash, @@ -82,7 +89,8 @@ pub fn validate_tx_constant_data( ); // Ensure that the `max_fees_per_gas` specified by the tx is greater than or equal to the `gas_fees` for the block. - // Note that for private only txs, the gas against the limit is checked on tail, since it doesn't have public execution. + // Note that for private-only txs, the gas against the limit is checked in the private-kernel-tail, + // since it doesn't have public execution. assert( tx_gas_settings.max_fees_per_gas.fee_per_da_gas >= block_gas_fees.fee_per_da_gas, "da gas is higher than the maximum specified by the tx", diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tx_effect_builder.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tx_effect_builder.nr index 576cbde0335e..8275938c4767 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tx_effect_builder.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tx_effect_builder.nr @@ -24,7 +24,9 @@ pub fn build_tx_effect( ) -> (TxEffect, TxEffectArrayLengths) { let accumulated_data = private_to_rollup.end; - // Silo L2 to L1 messages. + // Compute siloed L2-to-L1 message hashes (siloed with the contract address of the function that + // emitted the message). We do this sequencer-side, because the hashes are computed with sha256 + // for EVM compatibility. let l2_to_l1_msgs = accumulated_data.l2_to_l1_msgs.map(|message| silo_l2_to_l1_message( message, private_to_rollup.constants.tx_context.version, @@ -48,7 +50,7 @@ pub fn build_tx_effect( [PublicDataWrite::empty(); MAX_TOTAL_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX]; public_data_writes[0] = fee_payer_balance_write; - // Construct contract class logs from the hashes from kernel circuits and the log fields given via private inputs. + // Construct ContractClassLogs from the hashes from kernel circuits and the log fields given via private inputs. let contract_class_logs = accumulated_data.contract_class_logs_hashes.mapi(|i, log_hash| { ContractClassLog { log: Log::new(contract_class_log_fields[i], log_hash.inner.length), @@ -83,7 +85,7 @@ pub fn build_tx_effect( note_hashes: array_length(accumulated_data.note_hashes), nullifiers: array_length(accumulated_data.nullifiers), l2_to_l1_msgs: array_length(l2_to_l1_msgs), - public_data_writes: 1, + public_data_writes: 1, // No public functions were called. This is the fee_payer's FeeJuice balance decrementation. private_logs: private_logs_array_length, contract_class_logs: contract_class_logs_array_length, }; diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_base_inputs_validator.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_base_inputs_validator.nr index b90d61b6eff8..7a29effe3951 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_base_inputs_validator.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_base_inputs_validator.nr @@ -84,7 +84,7 @@ impl PublicTxBaseInputsValidator { private_tail_validator::validate_tx_constant_data( private_tail.constants, - self.constants, + self.constants, // where applicable, these constants have been copied-over from the avm's public inputs. self.anchor_block_archive_sibling_path, ); diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_effect_builder.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_effect_builder.nr index f4f1f995ac72..fbeb139385bd 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_effect_builder.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_effect_builder.nr @@ -20,12 +20,29 @@ pub fn build_tx_effect( let revert_code = if reverted { 1 } else { 0 }; let global_variables = avm.global_variables; - // Silo L2 to L1 messages. + // Compute siloed L2-to-L1 message hashes (siloed with the contract address of the function that + // emitted the message). We do this sequencer-side, because the hashes are computed with sha256. let siloed_l2_to_l1_msgs = avm.accumulated_data.l2_to_l1_msgs.map(|message| { silo_l2_to_l1_message(message, global_variables.version, global_variables.chain_id) }); - // Construct private logs. + // If any of the public functions reverted within the `avm`, then the tx is considered to have reverted, + // and none of the tx's _revertible_ side-effects (be they from private or public functions) will be broadcast + // nor inserted into trees. But, the protocol will still broadcast/insert the _non-revertible_ side-effects + // of the tx. + // + // The non-revertible side-effects exist as a consequence of fee abstraction: the `fee_payer` (whomever + // has "put themselves on the hook" to pay fees on the user's behalf) must have a guarantee that + // certain notes/nullifiers/public-state-updates/logs _will_ be included. These "non-revertible + // side-effects" will typically be the side-effects which represent payment of a fee (through some abstract + // fee payment mechanism) to the `fee_payer`. + // + // Note: the other revertible side-effects (notes, nullifiers, public data updates, l2-to-l1 msgs) + // are conditionally discarded by the avm circuit. Private logs and contract class logs are + // different, in that they're only emitted by private functions and so the avm doesn't care about them + // or "see" them. + + // Conditionally discard the revertible private logs. let private_logs = if reverted { private_to_public.non_revertible_accumulated_data.private_logs } else { @@ -35,7 +52,7 @@ pub fn build_tx_effect( ) }; - // Construct contract class logs. + // Conditionally discard the revertible contract class logs. let contract_class_log_hashes = if reverted { private_to_public.non_revertible_accumulated_data.contract_class_logs_hashes } else { @@ -46,11 +63,17 @@ pub fn build_tx_effect( }; // Validate the given log fields against the log hashes from kernel circuits. + // Notice: this validation check is being done here (instead of by this circuit's 'validator', + // and inconsistently vs the tx-private-base circuit), because in the case of an avm revert, + // we only wish to reconcile against the contract class log hashes of _non-revertible_ contract + // class logs -- and the conditional discarding of the revertible logs has only just happened + // (because we try to avoid mutation within a circuit's 'validator'). private_tail_validator::validate_contract_class_logs( contract_class_log_hashes, contract_class_log_fields, ); - // Assign the log fields with the correct length and contract address. + + // Construct ContractClassLogs from the hashes from kernel circuits and the log fields given via private inputs. let contract_class_logs = contract_class_log_hashes.mapi(|i, log_hash| { ContractClassLog { log: Log::new(contract_class_log_fields[i], log_hash.inner.length), @@ -78,6 +101,9 @@ pub fn build_tx_effect( let contract_class_logs_array_length = array_length_until(contract_class_log_hashes, |log| log.inner.length == 0); + // AUDIT_COMMENT: unlike the private_tx_effect_builder.nr, the note_hashes, nullifiers, l2_to_l2_msgs array lengths + // are not being validated here. Does the AVM perform this check for us? Or are we validating the + // lengths elsewhere? let tx_effect_array_lengths = TxEffectArrayLengths { note_hashes: avm.accumulated_data_array_lengths.note_hashes, nullifiers: avm.accumulated_data_array_lengths.nullifiers, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/tree_snapshot_builder.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/tree_snapshot_builder.nr index 5ce50f75be32..4c69679dd4bd 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/tree_snapshot_builder.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/tree_snapshot_builder.nr @@ -30,6 +30,7 @@ pub struct TreeSnapshotDiffHints { pub fee_payer_balance_membership_witness: MembershipWitness, } +// TODO: Consider renaming to: build_end_tree_snapshots pub fn build_tree_snapshots( start_tree_snapshots: PartialStateReference, note_hashes: [Field; MAX_NOTE_HASHES_PER_TX], diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/tx_base_public_inputs_composer.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/tx_base_public_inputs_composer.nr index 1eca58afb9cd..c4c6abd9d15f 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/tx_base_public_inputs_composer.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/tx_base_public_inputs_composer.nr @@ -79,15 +79,20 @@ impl TxBasePublicInputsComposer { ) -> Self { let private_to_public = public_chonk_verifier.private_tail; - // Note: Some values below are copied from `avm` rather than `private_to_public` and this must remain unchanged. - // The constants are checked in `private_tail_validator` to ensure they match the values in `private_to_public`. + // Note: Some values below are copied from the avm's public inputs rather than the private tail's public inputs, + // and this MUST remain unchanged. + // Then in `private_tail_validator.nr`, these avm constants are checked to ensure they match those of the private tail's + // public inputs (`private_to_public`). This ensures that the values match between the avm and the private tail. + // If the values in constants were instead copied over from private tail's public inputs, then the validations in + // `private_tail_validator.nr` become pointless. let constants = BlockConstantData { last_archive, l1_to_l2_tree_snapshot: avm.start_tree_snapshots.l1_to_l2_message_tree, global_variables: avm.global_variables, vk_tree_root: private_to_public.constants.vk_tree_root, protocol_contracts_hash: private_to_public.constants.protocol_contracts_hash, - // We take the prover id of the public chonk verifier, but we validate that it matches the prover id of the avm in validate_public_chonk_verifier_against_avm + // We take the prover id of the public chonk verifier, but we validate that it matches the prover id of the avm in validate_public_chonk_verifier_against_avm. That seems inconsistent and confusing. + // why isn't this copied-over from the avm? Seems inconsistent. Where is the avm's protocol_contracts_hash checked? Or does the avm not make use of any protocol contract addresses? prover_id: public_chonk_verifier.prover_id, }; diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/public_tx_base_rollup_private_inputs.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/public_tx_base_rollup_private_inputs.nr index a704bc4348bf..733b0c169769 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/public_tx_base_rollup_private_inputs.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/public_tx_base_rollup_private_inputs.nr @@ -34,8 +34,10 @@ pub struct PublicTxBaseRollupPrivateInputs { pub(crate) anchor_block_archive_sibling_path: [Field; ARCHIVE_HEIGHT], // Fields of the contract class logs. - // These fields are validated against the hashes committed to and propagated from the private tail kernel. The - // fields themselves are added to the blobs, instead of the hashes. + // These fields are validated against the contract class log hashes that are committed to and propagated + // from the private tail kernel. The fields themselves are added to the blobs, instead of the hashes. + // Note: if the avm reverted, only the _non-revertible_ contract class logs will be fed into this circuit, and + // only the _non-revertible_ contract class log hashes will be reconciled. pub(crate) contract_class_log_fields: [[Field; CONTRACT_CLASS_LOG_SIZE_IN_FIELDS]; MAX_CONTRACT_CLASS_LOGS_PER_TX], } @@ -52,7 +54,7 @@ impl PublicTxBaseRollupPrivateInputs { PublicTxBaseInputsValidator::new( self.public_chonk_verifier_proof_data, self.avm_proof_data, - composer.constants, + composer.constants, // where applicable, these constants have been copied-over from the avm's public inputs. self.anchor_block_archive_sibling_path, ) .validate(); diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_merge/utils/merge_tx_rollups.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_merge/utils/merge_tx_rollups.nr index 6be62756f751..c32884145702 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_merge/utils/merge_tx_rollups.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_merge/utils/merge_tx_rollups.nr @@ -1,31 +1,43 @@ use crate::abis::TxRollupPublicInputs; use dep::types::hash::accumulate_sha256; +// The `out_hash` is the root of a (potentially wonky, greedy) subtree of L2->L1 messages that the +// various functions of txs of this block have emitted. pub fn accumulate_out_hash(left_out_hash: Field, right_out_hash: Field) -> Field { + // An out_hash will only be 0 if no L2->L1 msgs were emitted by the functions of the txs of the + // left/right subtree. if left_out_hash == 0 { right_out_hash } else if right_out_hash == 0 { left_out_hash } else { + // TODO: should probably have a domain separator, to be safe + // We use sha256 for EVM compatibility: some time in the future, an L1 contract will consume + // a message from this `out_hash` subtree (the subtree of L2->L1 messages) by doing a merkle + // membership check in the Outbox.sol contract: sha256 is cheaper than poseidon2 on L1. accumulate_sha256(left_out_hash, right_out_hash) } } +// Accumulates items that need to be accumulated to the next rollup circuit. +// Constructs the public inputs of this circuit, taking care to ascribe left and right +// subtree data to the correct start and end positions. pub fn merge_tx_rollups( left: TxRollupPublicInputs, right: TxRollupPublicInputs, ) -> TxRollupPublicInputs { - let num_txs = left.num_txs + right.num_txs; + let num_txs = left.num_txs + right.num_txs; // Q: should we assert that this is >= 2? let out_hash = accumulate_out_hash(left.out_hash, right.out_hash); + // TODO: check that we cap these fees and mana, to prevent overflow. let accumulated_fees = left.accumulated_fees + right.accumulated_fees; let accumulated_mana_used = left.accumulated_mana_used + right.accumulated_mana_used; TxRollupPublicInputs { num_txs, - constants: left.constants, + constants: left.constants, // Checked to equal the right constants by this circuit's Validator. start_tree_snapshots: left.start_tree_snapshots, end_tree_snapshots: right.end_tree_snapshots, start_sponge_blob: left.start_sponge_blob, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_merge/utils/validate_consecutive_tx_rollups.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_merge/utils/validate_consecutive_tx_rollups.nr index 1bd0d9cc43ca..35d15b2a2153 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_merge/utils/validate_consecutive_tx_rollups.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_merge/utils/validate_consecutive_tx_rollups.nr @@ -13,7 +13,25 @@ pub fn validate_consecutive_tx_rollups(left: TxRollupPublicInputs, right: TxRoll /// When called from the `tx_merge` or `block_root`, it represents the total number of `tx_base` rollups. /// When called from the `block_merge` or `checkpoint_root`, it represents the total number of `block_root` rollups. /// When called from the `checkpoint_merge` or `root`, it represents the total number of `checkpoint_root` rollups. +/// +/// Valid configurations: +/// +/// 2 3 4 5 6 7 +/// . . . . . . +/// / \ / \ / \ / \ / \ / \ +/// . . . . . . . . . . . . +/// / \ / \ / \ / \ / \ / \ / \ / \ +/// . . . . . . . . . . . . . . . . +/// / \ / \ / \ / \ / \ / \ / \ +/// . . . . . . . . . . . . . . +/// +/// etc... +/// +/// See also: https://hackmd.io/CKdVk7zZS2qWd0vBN7e1SA +/// pub fn assert_rollups_filled_greedily(num_left_rollups: u16, num_right_rollups: u16) { + // Q: should we assert that `num_right_rollups != 0`, to be sure? + // The left rollup must be a balanced tree (i.e., `num_left_rollups` is a power of 2). assert( is_power_of_2(num_left_rollups), diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/abis/block_header.nr b/noir-projects/noir-protocol-circuits/crates/types/src/abis/block_header.nr index 07f5695c49c4..a118bbcf78a3 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/abis/block_header.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/abis/block_header.nr @@ -13,7 +13,7 @@ use std::meta::derive; #[derive(Deserialize, Eq, Serialize)] pub struct BlockHeader { pub last_archive: AppendOnlyTreeSnapshot, - pub state: StateReference, + pub state: StateReference, // AUDIT_COMMENT: Use TreeSnapshots instead. // The hash of the sponge blob for this block, which commits to the tx effects added in this block. // Note: it may also include tx effects from previous blocks within the same checkpoint. diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/sponge_blob.nr b/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/sponge_blob.nr index 815cde30d68b..11005068a4a6 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/sponge_blob.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/sponge_blob.nr @@ -52,6 +52,43 @@ impl SpongeBlob { Self { sponge: Poseidon2Sponge::new(iv), num_absorbed_fields: 0 } } + /// |----------|--------------------------------|-------------------------------------| + /// | Field 0 | TX_START_PREFIX | 64 bits | + /// | | num_note_hashes | 16 bits | + /// | | num_nullifiers | 16 bits | + /// | | num_l1_to_l2_msgs | 16 bits | + /// | | num_public_data_writes | 16 bits | + /// | | num_private_logs | 16 bits | + /// | | private_logs_length | 16 bits | + /// | | public_logs_length | 32 bits | + /// | | contract_class_log_length | 16 bits | + /// | | revert_code | 8 bits | + /// | | num_blob_fields | 32 bits | + /// |----------|--------------------------------|-------------------------------------| + /// | Field 1 | tx_effect.tx_hash | A field | + /// |----------|--------------------------------|-------------------------------------| + /// | Field 2 | tx_effect.transaction_fee | A field | + /// |----------|--------------------------------|-------------------------------------| + /// | | tx_effect.note_hashes | `num_note_hashes` fields | + /// |----------|--------------------------------|-------------------------------------| + /// | | tx_effect.nullifiers | `num_nullifiers` fields | + /// |----------|--------------------------------|-------------------------------------| + /// | | tx_effect.l2_to_l1_msgs | `num_l1_to_l2_msgs` fields | + /// |----------|--------------------------------|-------------------------------------| + /// | | tx_effect.public_data_writes | `num_public_data_writes * 2` fields | + /// |----------|--------------------------------|-------------------------------------| + /// | | (tx_effect.private_logs): | (private_logs_length) | + /// | | Where, for each log: | | + /// | | log.length | A field | + /// | | log.fields | `log.length` fields | + /// |----------|--------------------------------|-------------------------------------| + /// | | tx_effect.public_logs.payload | `public_logs.length` fields | + /// |----------|--------------------------------|-------------------------------------| + /// | | tx_effect.contract_class_logs | (contract_class_log_length) | + /// | | Where, for each log: | | + /// | | log.contract_address | A field | + /// | | log.fields | `log.length` fields | + /// |----------|--------------------------------|-------------------------------------| pub fn absorb_tx_blob_data( &mut self, tx_effect: TxEffect, @@ -61,6 +98,28 @@ impl SpongeBlob { self.absorb(tx_blob_data, num_blob_fields); } + /// |----------|---------------------------------------------|---------| + /// | Field 0 | BLOCK_END_PREFIX | 72 bits | + /// | | timestamp | 64 bits | + /// | | block_number | 32 bits | + /// | | num_txs | 16 bits | + /// |----------|---------------------------------------------|---------| + /// | Field 1 | l1_to_l2_message_next_available_leaf_index | 36 bits | + /// | | note_hash_next_available_leaf_index | 42 bits | + /// | | nullifier_next_available_leaf_index | 42 bits | + /// | | public_data_next_available_leaf_index | 40 bits | + /// | | total_mana_used | 48 bits | + /// |----------|---------------------------------------------|---------| + /// | Field 2 | last_archive.root | A field | + /// |----------|---------------------------------------------|---------| + /// | Field 3 | note_hash_tree.root | A field | + /// |----------|---------------------------------------------|---------| + /// | Field 4 | nullifier_tree.root | A field | + /// |----------|---------------------------------------------|---------| + /// | Field 5 | public_data_tree.root | A field | + /// |----------|---------------------------------------------|---------| + /// | Field 6 | l1_to_l2_message_tree.root | A field | + /// |----------|---------------------------------------------|---------| pub fn absorb_block_end_data( &mut self, global_variables: GlobalVariables, @@ -89,9 +148,20 @@ impl SpongeBlob { self.absorb(blob_data, num_blob_data_to_absorb); } + /// |----------|---------------------------------------------|---------| + /// | Field 0 | CHECKPOINT_END_PREFIX | 112 bits| + /// | | num_blob_fields | 32 bits | + /// |----------|---------------------------------------------|---------| pub fn absorb_checkpoint_end_marker(&mut self) { - self.num_absorbed_fields += 1; let end_marker = create_checkpoint_end_marker(self.num_absorbed_fields); + // If you ever increase the size of an `end_marker` beyond 1 Field, you need to adjust this + // incrementation accordingly: + self.num_absorbed_fields += 1; + // Check that the number of absorbed blob fields is not larger than the maximum number of fields allowed. + assert( + self.num_absorbed_fields <= FIELDS_PER_BLOB * BLOBS_PER_CHECKPOINT, + "Attempted to overfill blobs", + ); self.sponge.absorb(end_marker); } diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/tx_blob_data.nr b/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/tx_blob_data.nr index d340c036d74a..585f5e01aeab 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/tx_blob_data.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/tx_blob_data.nr @@ -4,10 +4,12 @@ use crate::{ MAX_CONTRACT_CLASS_LOGS_PER_TX, MAX_L2_TO_L1_MSGS_PER_TX, MAX_NOTE_HASHES_PER_TX, MAX_NULLIFIERS_PER_TX, MAX_PRIVATE_LOGS_PER_TX, MAX_TOTAL_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, PRIVATE_LOG_SIZE_IN_FIELDS, TX_START_PREFIX, + TX_START_PREFIX_BIT_SIZE, }, traits::ToField, }; use super::tx_effect::TxEffect; +use std::static_assert; pub global MAX_TX_BLOB_DATA_SIZE_IN_FIELDS: u32 = 1 // tx start marker + 1 // tx hash @@ -64,6 +66,22 @@ pub fn create_tx_start_marker( contract_class_log_length: u32, // Assumed to be a u16, checked on tail to be less than CONTRACT_CLASS_LOG_SIZE_IN_FIELDS revert_code: u8, ) -> Field { + static_assert( + NUM_BLOB_FIELDS_BIT_SIZE + + REVERT_CODE_BIT_SIZE + + CONTRACT_CLASS_LOG_LENGTH_BIT_SIZE + + PUBLIC_LOGS_LENGTH_BIT_SIZE + + PRIVATE_LOGS_LENGTH_BIT_SIZE + + NUM_PRIVATE_LOG_BIT_SIZE + + NUM_PUBLIC_DATA_WRITE_BIT_SIZE + + NUM_L2_TO_L1_MSG_BIT_SIZE + + NUM_NULLIFIER_BIT_SIZE + + NUM_NOTE_HASH_BIT_SIZE + + TX_START_PREFIX_BIT_SIZE + <= 253, + "These bit-sizes don't fit within a field anymore. Have you changed a constant?", + ); + let num_blob_fields_field = num_blob_fields as Field; num_blob_fields_field.assert_max_bit_size::(); let mut marker = num_blob_fields_field; @@ -86,6 +104,9 @@ pub fn create_tx_start_marker( marker += public_logs_length_field * shift; shift *= (1 << PUBLIC_LOGS_LENGTH_BIT_SIZE as u64) as Field; + // This is the length of all private logs (concatenated) that will be inserted into the blob. + // Each private_log conveys its own `length` field; all such `length` fields are included in this + // private_logs_length value. let private_logs_length_field = private_logs_length as Field; private_logs_length_field.assert_max_bit_size::(); marker += private_logs_length_field * shift; diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr index 3630c923edcb..75ab23598daa 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr @@ -161,7 +161,7 @@ pub global INITIAL_CHECKPOINT_NUMBER: Field = 1; pub global INITIAL_L2_BLOCK_NUM: Field = 1; pub global FIELDS_PER_BLOB: u32 = 4096; pub global BLOBS_PER_CHECKPOINT: u32 = 6; -pub global AZTEC_MAX_EPOCH_DURATION: u32 = 48; +pub global AZTEC_MAX_EPOCH_DURATION: u32 = 48; // Is this the max number of checkpoints per epoch? If so, we should rename it. pub global MAX_INCLUDE_BY_TIMESTAMP_DURATION: u64 = 86400; // 1 day // The genesis values are taken from world_state.test.cpp > WorldStateTest.GetInitialTreeInfoForAllTrees pub global GENESIS_BLOCK_HEADER_HASH: Field = @@ -1136,9 +1136,11 @@ pub global AVM_SSTORE_DYN_DA_GAS: u32 = 2 * DA_BYTES_PER_FIELD * DA_GAS_PER_BYTE // BLOB PREFIXES // Used when decoding blobs of tightly packed effects // TODO: Maybe hash instead of directly encoding to field for NUMS -pub global TX_START_PREFIX: Field = 0x74785f7374617274; // = "tx_start".to_field() in nr -pub global BLOCK_END_PREFIX: Field = 0x626c6f636b5f656e64; // = "block_end".to_field() in nr -pub global CHECKPOINT_END_PREFIX: Field = 0x636865636b706f696e745f656e64; // = "checkpoint_end".to_field() in nr +// TODO: we need noir tests that demonstrate that these derivations are correct. +pub global TX_START_PREFIX: Field = 0x74785f7374617274; // = "tx_start".to_field() in nr (8 bytes) +pub global TX_START_PREFIX_BIT_SIZE: u32 = 64; +pub global BLOCK_END_PREFIX: Field = 0x626c6f636b5f656e64; // = "block_end".to_field() in nr (9 bytes) +pub global CHECKPOINT_END_PREFIX: Field = 0x636865636b706f696e745f656e64; // = "checkpoint_end".to_field() in nr (14 bytes) // Constants related to proof type of a recursive proof verification. // Keep following constants in sync with the enum acir_format::PROOF_TYPE in recursion_constraint.hpp diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/hash.nr b/noir-projects/noir-protocol-circuits/crates/types/src/hash.nr index 26df569e0db9..d5d5e2c4ed75 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/hash.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/hash.nr @@ -27,6 +27,8 @@ pub use poseidon2_chunks::poseidon2_absorb_in_chunks_existing_sponge; use poseidon2_chunks::poseidon2_absorb_in_chunks; use std::embedded_curve_ops::EmbeddedCurveScalar; +// TODO: refactor these into their own files: sha256, poseidon2, some protocol-specific hash computations, some merkle computations. + pub fn sha256_to_field(bytes_to_hash: [u8; N]) -> Field { let sha256_hashed = sha256::digest(bytes_to_hash); let hash_in_a_field = field_from_bytes_32_trunc(sha256_hashed); @@ -208,6 +210,7 @@ pub fn silo_l2_to_l1_message( } } +// TODO: consider a variant that enables domain separation with a u32 (we seem to have standardised u32s for domain separators) /// Computes sha256 hash of 2 input fields. /// /// @returns A truncated field (i.e., the first byte is always 0). @@ -220,6 +223,7 @@ pub fn accumulate_sha256(v0: Field, v1: Field) -> Field { sha256_to_field(hash_input_flattened) } +// TODO: remove this. The protocol doesn't need it. #[inline_always] pub fn pedersen_hash(inputs: [Field; N], hash_index: u32) -> Field { std::hash::pedersen_hash_with_separator(inputs, hash_index) diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/indexed_tree.nr b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/indexed_tree.nr index e4eeb35a2565..48da5cbbb884 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/indexed_tree.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/indexed_tree.nr @@ -29,6 +29,7 @@ where Value: IndexedTreeLeafValue, Leaf: IndexedTreeLeafPreimage, { + // AUDIT_COMMENT: change to static_assert? // Sanity check: ensure this function is called with correct generics. assert_eq(SubtreeWidth, 1 << SubtreeHeight, "subtree items must be 2^subtree height"); diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/merkle_tree.nr b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/merkle_tree.nr index bc6239f95bae..b2d9feca48b9 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/merkle_tree.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/merkle_tree.nr @@ -1,4 +1,4 @@ -use crate::{hash::{accumulate_sha256, merkle_hash}, traits::Empty}; +use crate::{hash::{accumulate_sha256, merkle_hash}, traits::Empty, utils::math::is_power_of_2_u32}; #[derive(Eq)] pub struct MerkleTree { @@ -28,7 +28,7 @@ impl MerkleTree { } pub fn get_sibling_path(self, leaf_index: u32) -> [Field; K] { - assert_eq(2.pow_32(K as Field), N as Field, "Invalid path length"); + std::static_assert(2.pow_32(K as Field) == N as Field, "Invalid path length"); let mut path = [0; K]; let mut current_index = leaf_index; @@ -63,11 +63,14 @@ pub fn sibling_index(index: u32) -> u32 { } } -// Note: `N` must be a power of 2. fn compute_merkle_tree_nodes( leaves: [Field; N], hasher: fn(Field, Field) -> Field, ) -> [Field; N - 1] { + // Note: `N` must be a power of 2. + std::static_assert(is_power_of_2_u32(N), "N must be a power of 2"); + std::static_assert(N != 1, "2 must divide N"); + let mut nodes = [0; N - 1]; let total_nodes = N - 1; diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/tests/types/test_leaf_preimage.nr b/noir-projects/noir-protocol-circuits/crates/types/src/tests/types/test_leaf_preimage.nr index e3c08976c678..51c1f2828ab8 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/tests/types/test_leaf_preimage.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/tests/types/test_leaf_preimage.nr @@ -18,6 +18,7 @@ impl LeafPreimage for TestLeafPreimage { } fn as_leaf(self) -> Field { + // TODO: change this to _not_ use pedersen. Then we can remove pedersen entirely from the noir-protocol-circuits. pedersen_hash([self.value]) } } diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.md b/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.md index eb9ff0133e1c..9779682a61e3 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.md +++ b/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.md @@ -85,3 +85,397 @@ then RHS asserted to be 0 out hint: [ a, b, d, f, p, p, p| 0] +--- + +# arrays.nr Audit Report by Claude + +**File**: `noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr` + +--- + +## Overview + +This file provides array utilities for the Aztec protocol circuits, including: +- Basic array operations (`subarray`, `array_merge`, `array_length`) +- Index finding functions (`find_index_hint`, `find_index_hint_in_reverse`) +- Validation utilities (`check_permutation`, `array_padded_with`, `array_length_until`) +- The `ClaimedLengthArray` wrapper struct for kernel circuit use + +The file also re-exports from submodules: +- `assert_trailing_zeros` from `assert_trailing_zeros.nr` +- `find_first_index`, `find_last_index` from `find_index.nr` +- `get_sorted_tuples`, `SortedTuple` from `get_sorted_tuples.nr` + +--- + +## Issues Found + +### 1. Bad/Unclear Comments + +#### 1.1 Inverted assertion message in `array_length_until` (lines 170-173) + +**Location**: `arrays.nr:170-173` + +```noir +assert( + stop == false, + "matching element found after already encountering a non-matching element", +); +``` + +**Problem**: The error message is backwards. When this assertion fails: +- `stop` is `true` → a matching element was previously encountered +- We're in the `else` branch → current element is non-matching + +**Correct message**: `"non-matching element found after already encountering a matching element"` + +**Severity**: Medium - causes confusion during debugging + +--- + +#### 1.2 Confusing assertion message in `assert_dense_trimmed` (line 241) + +**Location**: `arrays.nr:241` + +```noir +assert(!self.array[i].is_empty(), "LHS of input array is not dense") +``` + +**Problem**: The message phrasing is awkward. If the assertion fails, the element IS empty, so the LHS is NOT dense. The message is technically correct but reads strangely as an error. + +**Suggested fix**: `"Expected LHS of array to be dense, but found empty element at index"` + +**Severity**: Low - grammatically correct but confusing + +--- + +#### 1.3 Cryptic comment about constructor (line 232) + +**Location**: `arrays.nr:232` + +```noir +// No constructor. Append to an empty one. +``` + +**Problem**: This comment is cryptic. It's not clear what "append to an empty one" means. + +**Suggested fix**: +```noir +// Create instances using `ClaimedLengthArray::empty()` then call `push()`, +// or use `ClaimedLengthArray::from_bounded_vec()`. +``` + +**Severity**: Low + +--- + +#### 1.4 Inconsistent example in `for_each_i` comment (lines 284-285) + +**Location**: `arrays.nr:284-285` + +```noir +// E.g. +// dest.for_each_i(|source_item, i| { assert_eq(dest.array[i], source_item); }) +``` + +**Problem**: +- Uses `dest` as the receiver but `source_item` as parameter name +- The lambda receives items from `self.array`, not from a "source" +- Accessing `dest.array[i]` inside a method called on `dest` is redundant + +**Suggested fix**: +```noir +// E.g. comparing two arrays: +// self.for_each_i(|item, i| { assert_eq(other.array[i], item); }) +``` + +**Severity**: Low + +--- + +#### 1.5 Vague TODO about compiler bug (line 301) + +**Location**: `arrays.nr:301` + +```noir +// TODO: compiler bug. No idea why this is needed, if we have #[derive(Eq)] above the struct definition. +``` + +**Problem**: +- The struct does NOT have `#[derive(Eq)]` - only `#[derive(Deserialize, Serialize)]` +- The comment claims there's a compiler bug but provides no issue reference +- This may be stale or the author forgot to add the derive + +**Action needed**: Either: +1. Add `#[derive(Eq)]` to see if it works now and remove manual impl +2. File a Noir issue and reference it here +3. Investigate and document why the derive doesn't work + +**Severity**: Low - but indicates technical debt + +--- + +#### 1.6 Unexplained acronym "RHS" (lines 102-103) + +**Location**: `arrays.nr:102-103` + +```noir +// Returns an array length defined by fully trimming _all_ "empty" items +// from the RHS. +``` + +**Problem**: "RHS" (Right Hand Side) may not be obvious to all readers. + +**Suggested fix**: `"...from the end of the array."` + +**Severity**: Very low + +--- + +#### 1.7 Misleading test comment (line 413) + +**Location**: `arrays.nr:413` + +```noir +assert_eq(array_padded_with(array, 5, 44), true); // Index out of bounds. +``` + +**Problem**: The comment says "Index out of bounds" but: +- The index is NOT out of bounds in the traditional sense +- Passing `from_index=5` for a 5-element array means the loop never checks anything +- The function correctly returns `true` because no elements fail the check + +**Suggested fix**: `// from_index past array end, so nothing is checked` + +**Severity**: Very low + +--- + +### 2. Missing Documentation + +#### 2.1 `check_permutation` has no documentation (line 180) + +**Location**: `arrays.nr:180-196` + +```noir +pub fn check_permutation( + original_array: [T; N], + permuted_array: [T; N], + original_indexes: [u32; N], +) +``` + +**Problem**: Public function with no doc comment. Should explain: +- What the function verifies +- The meaning of `original_indexes` (maps permuted index → original index) +- The assertion behavior + +**Suggested documentation**: +```noir +/// Verifies that `permuted_array` is a valid permutation of `original_array`. +/// +/// # Arguments +/// * `original_array` - The source array +/// * `permuted_array` - The permuted result to verify +/// * `original_indexes` - For each index `i` in `permuted_array`, `original_indexes[i]` +/// gives the index in `original_array` where that element came from +/// +/// # Panics +/// * "Invalid index" - if `permuted_array[i] != original_array[original_indexes[i]]` +/// * "Duplicated index" - if any index in `original_indexes` is used more than once +``` + +--- + +#### 2.2 `assert_dense_trimmed` has no documentation (line 236) + +**Location**: `arrays.nr:236-248` + +**Problem**: Important validation method with no doc comment. + +**Suggested documentation**: +```noir +/// Validates that the array is properly structured: +/// - All elements at indices `0..self.length` are non-empty (dense) +/// - All elements at indices `self.length..N` are empty (trimmed) +/// +/// This is used to validate that the claimed length matches the actual array contents. +``` + +--- + +#### 2.3 `get_sorted_tuples` has no documentation (get_sorted_tuples.nr:15) + +**Location**: `get_sorted_tuples.nr:15-27` + +**Problem**: No doc comment explaining the function or the `ordering` parameter. + +**Suggested documentation**: +```noir +/// Sorts an array while preserving original indices. +/// +/// # Arguments +/// * `array` - The array to sort +/// * `ordering` - Comparison function. Returns `true` if first argument should come before second. +/// For ascending order: `|a, b| a < b` +/// For descending order: `|a, b| a > b` +/// +/// # Returns +/// Array of `SortedTuple` where each tuple contains the element and its original index. +``` + +--- + +#### 2.4 `SortedTuple` struct has no documentation (get_sorted_tuples.nr:1) + +**Location**: `get_sorted_tuples.nr:1-4` + +**Suggested documentation**: +```noir +/// A tuple pairing an element with its original index before sorting. +/// Used by `get_sorted_tuples` to track element origins after sorting. +pub struct SortedTuple { + /// The element value + pub elem: T, + /// The index this element had in the original unsorted array + pub original_index: u32, +} +``` + +--- + +### 3. Potential Bugs + +#### 3.1 Error message inversion in `array_length_until` + +**Location**: `arrays.nr:170-173` + +As described in section 1.1, the error message states the opposite of what's happening. This is a UX/debugging bug that would cause confusion when the assertion fails. + +**Impact**: Medium - incorrect error messages waste developer time + +--- + +### 4. Missing Tests + +#### 4.1 Functions in `arrays.nr` with no tests + +| Function | Lines | Notes | +|----------|-------|-------| +| `subarray` | 19-28 | No tests | +| `array_merge` | 120-138 | No tests | +| `array_merge_helper` | 140-159 | Unconstrained helper, could test indirectly | +| `trimmed_array_length_hint` | 104-116 | No tests | + +#### 4.2 `ClaimedLengthArray` methods with no tests + +| Method | Line | Notes | +|--------|------|-------| +| `assert_dense_trimmed` | 236 | Critical validation, needs tests | +| `assert_empty` | 250 | Needs tests | +| `assert_length_within_bounds` | 257 | Needs tests | +| `push` | 261 | Needs tests including overflow case | +| `pop` | 269 | Needs tests including underflow case | +| `for_each` | 279 | Needs tests | +| `for_each_i` | 286 | Needs tests | +| `from_bounded_vec` | 296 | Needs tests | +| `empty` (Empty impl) | 315 | Needs tests | +| `eq` (Eq impl) | 306 | Needs tests | + +#### 4.3 Submodule test gaps + +**`assert_trailing_zeros.nr`**: +- No tests at all +- Should test: valid trailing zeros, non-zero after breakpoint, edge cases (empty, all zeros, no zeros) + +**`get_sorted_tuples.nr`**: +- Only one test (ascending u64) +- Missing tests for: + - Descending order + - Empty arrays (if N=0 is valid) + - Single element arrays + - Arrays with duplicate values + - Stability (same values maintain relative order?) + - Different types + +**`find_index.nr`**: +- Has good test coverage ✓ + +--- + +## Code Quality Observations + +### Positive + +1. Good use of `unconstrained` for hint functions that are verified separately +2. Safety comments explain why `unsafe` blocks are sound +3. `ClaimedLengthArray` is well-designed for the kernel circuit use case +4. `for_i_in_0_` usage avoids runtime-dependent loop bounds + +### Areas for Improvement + +1. **Deprecation annotation**: `array_length` is marked as "Deprecated" in doc comment but lacks `#[deprecated]` attribute +2. **Consistency**: Some functions have doc comments (`///`), others have regular comments (`//`) +3. **Section organization**: The "FREE ARRAY FUNCTIONS (to deprecate...)" section header suggests technical debt + +--- + +## Recommended Actions + +### High Priority +1. Fix inverted error message in `array_length_until` (line 172) +2. Add tests for `ClaimedLengthArray` methods +3. Add tests for `assert_trailing_zeros` + +### Medium Priority +4. Add documentation for `check_permutation` +5. Add documentation for `assert_dense_trimmed` +6. Add tests for `subarray`, `array_merge`, `trimmed_array_length_hint` +7. Clarify the "No constructor" comment + +### Low Priority +8. Investigate and resolve the `#[derive(Eq)]` TODO +9. Expand `get_sorted_tuples` test coverage +10. Fix misleading test comment about "Index out of bounds" +11. Consider adding `#[deprecated]` attribute to `array_length` + +--- + +## File Structure + +``` +arrays.nr +├── Module declarations & re-exports (lines 1-11) +├── ARRAY section (lines 13-63) +│ ├── subarray +│ ├── find_index_hint +│ └── find_index_hint_in_reverse +├── FREE ARRAY FUNCTIONS section (lines 65-211) +│ ├── array_length (deprecated) +│ ├── trimmed_array_length_hint +│ ├── array_merge + helper +│ ├── array_length_until +│ ├── check_permutation +│ └── array_padded_with +├── ARRAY WRAPPERS section (lines 213-318) +│ ├── ClaimedLengthArray struct +│ ├── ClaimedLengthArray impl (methods) +│ ├── Eq impl +│ └── Empty impl +└── Tests (lines 320-415) + ├── array_length tests + ├── array_length_until tests + ├── find_index_hint tests + ├── check_permutation tests + └── array_padded_with tests +``` + +--- + +## Submodule Summary + +| File | Functions | Test Coverage | +|------|-----------|---------------| +| `assert_trailing_zeros.nr` | `assert_trailing_zeros` | None | +| `find_index.nr` | `find_first_index`, `find_last_index` | Good | +| `get_sorted_tuples.nr` | `get_sorted_tuples`, `SortedTuple` | Minimal | diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr b/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr index 6e52875f120e..2b90f94245a6 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr @@ -117,6 +117,7 @@ where /// This function assumes that `array1` and `array2` contain no more than N non-empty elements between them, /// if this is not the case then elements from the end of `array2` will be dropped. +/// Consider renaming to array_merge_unsafe, because it doesn't check the sizes of the arrays. pub fn array_merge(array1: [T; N], array2: [T; N]) -> [T; N] where T: Empty, @@ -214,49 +215,6 @@ where // ARRAY WRAPPERS //********************************************************************************** -/* - * - * - * - * |-----------------------------------------|------------------------------| - * | LHS | RHS | - * |-----------------------------------------|------------------------------| - * ClaimedLengthArray | Interspersed 0s possible. | Unvalidated. | - * | Possibly not fully trimmed. | Nonempty elements possible. | - * |-----------------------------------------|------------------------------| - * EmptyRHSArray | Interspersed 0s possible. | All 0s (validated). | - * | Possibly not fully trimmed. | | - * |-----------------------------------------|------------------------------| - * TrimmedArray | Interspersed 0s possible. | All 0s (validated) | - * | Last lhs element validated as nonempty. | | - * | (I.e. fully trimmed) | | - * |-----------------------------------------|------------------------------| - * DenseTrimmedArray | Dense (validated). | All 0s (validated) | - * |-----------------------------------------|------------------------------| - * - * - * | What guarantees do we have? | - * |--------|--------|--------------------------------| - * | Dense? | RHS | Length vs Fully Trimmed Length | - * |--------|--------|--------------------------------| - * ClaimedLengthArray | ? | ? | ? | - * | | | | - * |--------|--------|--------------------------------| - * EmptyRHSArray | ? | All 0s | Length >= Fully Trimmed Length | - * | | | | - * |--------|--------|--------------------------------| - * TrimmedArray | ? | All 0s | Length == Fully Trimmed Length | - * | | | | - * | | | | - * |--------|--------|--------------------------------| - * DenseTrimmedArray | Yes | All 0s | Length == Fully Trimmed Length | - * |--------|--------|--------------------------------| - * - * - * An ClaimedLengthArray is distinct from a regular array [T; N], because it carries a length. - * - */ - /// ClaimedLengthArray - An array interpreted by Kernel circuits. /// Its `length` is merely a claim that must eventually be validated. /// Validation must include: diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/utils/field.nr b/noir-projects/noir-protocol-circuits/crates/types/src/utils/field.nr index 66213063b7ae..85cfdee58f38 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/utils/field.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/utils/field.nr @@ -1,3 +1,5 @@ +// TODO: consider a dedicated sqrt.nr file, since a lot of this file relates to sqrt. + global KNOWN_NON_RESIDUE: Field = 5; // This is a non-residue in Noir's native Field. pub fn field_from_bytes(bytes: [u8; N], big_endian: bool) -> Field { @@ -204,7 +206,7 @@ unconstrained fn bytes_field_test() { 151, 202, 67, 55, 77, 233, 80, 187, 224, 167, 158, ]; let field2 = field_from_bytes_32_trunc(inputs2); - let return_bytes2: [u8; 31] = field.to_be_bytes(); + let return_bytes2: [u8; 31] = field2.to_be_bytes(); assert_eq(return_bytes2, return_bytes); assert_eq(field2, field); @@ -239,3 +241,153 @@ unconstrained fn sqrt_invalid_test() { let result = sqrt(x); assert(result.is_none()); } + +#[test] +unconstrained fn sqrt_zero_test() { + let result = sqrt(0); + assert(result.is_some()); + assert_eq(result.unwrap(), 0); +} + +#[test] +unconstrained fn sqrt_one_test() { + let result = sqrt(1); + assert(result.is_some()); + assert_eq(result.unwrap() * result.unwrap(), 1); +} + +#[test] +unconstrained fn field_from_bytes_empty_test() { + let empty: [u8; 0] = []; + let result = field_from_bytes(empty, true); + assert_eq(result, 0); + + let result_le = field_from_bytes(empty, false); + assert_eq(result_le, 0); +} + +#[test] +unconstrained fn field_from_bytes_little_endian_test() { + // Test little-endian conversion: [0x01, 0x02] should be 0x0201 = 513 + let bytes = [0x01, 0x02]; + let result_le = field_from_bytes(bytes, false); + assert_eq(result_le, 0x0201); + + // Compare with big-endian: [0x01, 0x02] should be 0x0102 = 258 + let result_be = field_from_bytes(bytes, true); + assert_eq(result_be, 0x0102); +} + +#[test] +unconstrained fn pow_test() { + assert_eq(pow(2, 0), 1); + assert_eq(pow(2, 1), 2); + assert_eq(pow(2, 10), 1024); + assert_eq(pow(3, 5), 243); + assert_eq(pow(0, 5), 0); + assert_eq(pow(1, 100), 1); +} + +#[test] +unconstrained fn min_test() { + assert_eq(min(5, 10), 5); + assert_eq(min(10, 5), 5); + assert_eq(min(7, 7), 7); + assert_eq(min(0, 1), 0); +} + +#[test] +unconstrained fn full_field_comparison_test() { + assert(full_field_less_than(5, 10)); + assert(!full_field_less_than(10, 5)); + assert(!full_field_less_than(5, 5)); + + assert(full_field_greater_than(10, 5)); + assert(!full_field_greater_than(5, 10)); + assert(!full_field_greater_than(5, 5)); +} + +#[test] +unconstrained fn sqrt_has_two_roots_test() { + // Every square has two roots: r and -r (i.e., p - r) + // sqrt(16) can return 4 or -4 + let x = 16; + let result = sqrt(x).unwrap(); + assert(result * result == x); + // The other root is -result + let other_root = 0 - result; + assert(other_root * other_root == x); + // Verify they are different (unless x = 0) + assert(result != other_root); + + // Same for 9: roots are 3 and -3 + let y = 9; + let result_y = sqrt(y).unwrap(); + assert(result_y * result_y == y); + let other_root_y = 0 - result_y; + assert(other_root_y * other_root_y == y); + assert(result_y != other_root_y); +} + +#[test] +unconstrained fn sqrt_negative_one_test() { + // -1 (i.e., p-1) is not a quadratic residue in BN254's scalar field + // since r = 3 (mod 4), meaning -1 has no square root + let x = 0 - 1; + let result = sqrt(x); + assert(result.is_none()); +} + +#[test] +unconstrained fn sqrt_negative_numbers_test() { + // Test a few negative numbers (which wrap around in the field) + // -4 should not have a sqrt (since -1 doesn't have one and -4 = -1 * 4) + let result = sqrt(0 - 4); + assert(result.is_none()); + + // -9 should not have a sqrt + let result2 = sqrt(0 - 9); + assert(result2.is_none()); +} + +#[test] +unconstrained fn validate_sqrt_hint_valid_test() { + // 4 is a valid sqrt of 16 + validate_sqrt_hint(16, 4); + // -4 is also a valid sqrt of 16 + validate_sqrt_hint(16, 0 - 4); + // 0 is a valid sqrt of 0 + validate_sqrt_hint(0, 0); + // 1 is a valid sqrt of 1 + validate_sqrt_hint(1, 1); + // -1 is also a valid sqrt of 1 + validate_sqrt_hint(1, 0 - 1); +} + +#[test(should_fail_with = "is not the sqrt of x")] +unconstrained fn validate_sqrt_hint_invalid_test() { + // 5 is not a valid sqrt of 16 + validate_sqrt_hint(16, 5); +} + +#[test] +unconstrained fn validate_not_sqrt_hint_valid_test() { + // 5 (KNOWN_NON_RESIDUE) is not a square, so we can prove it + // We need sqrt(5 * 5) = sqrt(25) = 5 + let x = KNOWN_NON_RESIDUE; + let hint = tonelli_shanks_sqrt(x * KNOWN_NON_RESIDUE); + validate_not_sqrt_hint(x, hint); +} + +#[test(should_fail_with = "0 has a square root")] +unconstrained fn validate_not_sqrt_hint_zero_test() { + // 0 has a square root, so we cannot claim it is not square + validate_not_sqrt_hint(0, 0); +} + +#[test(should_fail_with = "does not demonstrate that")] +unconstrained fn validate_not_sqrt_hint_wrong_hint_test() { + // Provide a wrong hint for a non-square + let x = KNOWN_NON_RESIDUE; + validate_not_sqrt_hint(x, 123); +} diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/utils/math.nr b/noir-projects/noir-protocol-circuits/crates/types/src/utils/math.nr index 101839c754a4..a4bc391d608f 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/utils/math.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/utils/math.nr @@ -7,6 +7,19 @@ pub fn is_power_of_2(n: u16) -> bool { } } +// TODO: make the above generic for all uint types. +// (I couldn't figure out how to get a generic representation of `1`. It got ugly and worrisome +// using traits std::ops::{Sub, BitAdd}, Eq, and std::mem::zeroed()). +pub fn is_power_of_2_u32(n: u32) -> bool { + if n == 0 { + false + } else { + // See https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2 + n & (n - 1) == 0 + } +} + +// TODO: this test should go as high as the max number of leaves there could possibly be. #[test] fn test_is_power_of_2() { assert(is_power_of_2(0) == false); @@ -19,3 +32,16 @@ fn test_is_power_of_2() { assert(is_power_of_2(7) == false); assert(is_power_of_2(8) == true); } + +#[test] +fn test_is_power_of_2_u32() { + assert(is_power_of_2_u32(0) == false); + assert(is_power_of_2_u32(1) == true); + assert(is_power_of_2_u32(2) == true); + assert(is_power_of_2_u32(3) == false); + assert(is_power_of_2_u32(4) == true); + assert(is_power_of_2_u32(5) == false); + assert(is_power_of_2_u32(6) == false); + assert(is_power_of_2_u32(7) == false); + assert(is_power_of_2_u32(8) == true); +} diff --git a/noir-projects/noir-protocol-circuits/scripts/unravel_favourite_structs.sh b/noir-projects/noir-protocol-circuits/scripts/unravel_favourite_structs.sh index 79c0f2375330..47409bbb48b6 100755 --- a/noir-projects/noir-protocol-circuits/scripts/unravel_favourite_structs.sh +++ b/noir-projects/noir-protocol-circuits/scripts/unravel_favourite_structs.sh @@ -27,6 +27,9 @@ node scripts/unravel_struct.js target/private_kernel_reset_4_4_4_4_4_4_4_4_4.jso append_line_break "PRIVATE KERNEL TAIL" node scripts/unravel_struct.js target/private_kernel_tail.json --all >> "$FILE" +append_line_break "PRIVATE KERNEL TAIL TO PUBLIC" +node scripts/unravel_struct.js target/private_kernel_tail_to_public.json --all >> "$FILE" + append_line_break "PRIVATE TX BASE" node scripts/unravel_struct.js target/rollup_tx_base_private.json PrivateTxBaseRollupPrivateInputs >> "$FILE" diff --git a/yarn-project/stdlib/src/tx/tx.ts b/yarn-project/stdlib/src/tx/tx.ts index 63294ff18a18..285b83d402db 100644 --- a/yarn-project/stdlib/src/tx/tx.ts +++ b/yarn-project/stdlib/src/tx/tx.ts @@ -30,7 +30,11 @@ export class Tx extends Gossipable { private calldataMap: Map | undefined; constructor( - /** Identifier of the tx */ + /** + * Identifier of the tx. + * It's a hash of the public inputs of the tx's proof. + * This claimed hash is reconciled against the tx's public inputs (`this.data`) in data_validator.ts. + */ public readonly txHash: TxHash, /** * Output of the private kernel circuit for this tx. @@ -43,11 +47,14 @@ export class Tx extends Gossipable { /** * Contract class log fields emitted from the tx. * Their order should match the order of the log hashes returned from `this.data.getNonEmptyContractClassLogsHashes`. - * It's checked in data_validator.ts + * This claimed data is reconciled against a hash of this data (that is contained within + * the tx's public inputs (`this.data`)), in data_validator.ts. */ public readonly contractClassLogFields: ContractClassLogFields[], /** * An array of calldata for the enqueued public function calls and the teardown function call. + * This claimed data is reconciled against hashes of this data (that are contained within + * the tx's public inputs (`this.data`)), in data_validator.ts. */ public readonly publicFunctionCalldata: HashedValues[], ) {