Skip to content

Commit

Permalink
feat: Constrain protocol VK hashing (#9304)
Browse files Browse the repository at this point in the history
We were not constraining that the vk.hash matched vk.key in protocol
vks. This PR addresses that and adds some functions for making fake keys
for tests (since previously we only did fake vk hashes)
  • Loading branch information
sirasistant authored Oct 21, 2024
1 parent 6ce20e9 commit 3d17e13
Show file tree
Hide file tree
Showing 22 changed files with 157 additions and 82 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ impl Verifiable for RootParityInput {

impl RootParityInput {
fn validate_in_vk_tree(self) {
self.verification_key.check_hash();
assert_check_membership(
self.verification_key.hash,
BASE_PARITY_INDEX as Field,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ impl RootParityInputs {
assert(
self.children[i].vk_path == self.children[0].vk_path, "Inconsistent vk paths across base parity circuits"
);
//TODO: Do we need to validate this following hash
//assert(hash(self.children[i].verification_key) == self.children[i].verification_key.hash);
self.children[i].verify();
self.children[i].validate_in_vk_tree();
}
Expand All @@ -59,6 +57,8 @@ mod tests {
use dep::types::constants::BASE_PARITY_INDEX;

use super::NUM_BASE_PARITY_PER_ROOT_PARITY;
use types::hash::verification_key_hash;
use types::tests::fixtures::vk_tree::generate_fake_honk_vk_for_index;

fn test_setup() -> [RootParityInput; NUM_BASE_PARITY_PER_ROOT_PARITY] {
// 31 byte test SHA roots
Expand All @@ -71,12 +71,10 @@ mod tests {

let vk_tree = fixtures::vk_tree::get_vk_merkle_tree();

let vk_hash = vk_tree.leaves[BASE_PARITY_INDEX];
let vk_path = vk_tree.get_sibling_path(BASE_PARITY_INDEX);
let vk_tree_root = vk_tree.get_root();

let mut vk1 = VerificationKey::empty();
vk1.hash = vk_hash;
let vk1 = generate_fake_honk_vk_for_index(BASE_PARITY_INDEX);

let children = [
RootParityInput {
Expand Down Expand Up @@ -123,7 +121,8 @@ mod tests {
#[test(should_fail_with = "Inconsistent vk hashes across base parity circuits")]
fn test_asserts_incorrect_vk_hash_1() {
let mut vk2 = VerificationKey::empty();
vk2.hash = 0x53042d820859d80c474d4694e03778f8dc0ac88fc1c3a97b4369c1096e904a;
vk2.key[0] = 0x53042d820859d80c474d4694e03778f8dc0ac88fc1c3a97b4369c1096e904a;
vk2.hash = verification_key_hash(vk2.key);
let mut children = test_setup();
children[1].verification_key = vk2;
let root_parity_inputs = RootParityInputs { children };
Expand All @@ -139,7 +138,8 @@ mod tests {
#[test(should_fail_with = "Inconsistent vk hashes across base parity circuits")]
fn test_asserts_incorrect_vk_hash_2() {
let mut vk2 = VerificationKey::empty();
vk2.hash = 0x53042d820859d80c474d4694e03778f8dc0ac88fc1c3a97b4369c1096e904a;
vk2.key[0] = 0x53042d820859d80c474d4694e03778f8dc0ac88fc1c3a97b4369c1096e904a;
vk2.hash = verification_key_hash(vk2.key);
let mut children = test_setup();
children[2].verification_key = vk2;
let root_parity_inputs = RootParityInputs { children };
Expand All @@ -155,7 +155,8 @@ mod tests {
#[test(should_fail_with = "Inconsistent vk hashes across base parity circuits")]
fn test_asserts_incorrect_vk_hash_3() {
let mut vk2 = VerificationKey::empty();
vk2.hash = 0x53042d820859d80c474d4694e03778f8dc0ac88fc1c3a97b4369c1096e904a;
vk2.key[0] = 0x53042d820859d80c474d4694e03778f8dc0ac88fc1c3a97b4369c1096e904a;
vk2.hash = verification_key_hash(vk2.key);
let mut children = test_setup();
children[3].verification_key = vk2;
let root_parity_inputs = RootParityInputs { children };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ impl Verifiable for RootRollupParityInput {

impl RootRollupParityInput {
fn validate_in_vk_tree(self) {
self.verification_key.check_hash();
assert_check_membership(
self.verification_key.hash,
ROOT_PARITY_INDEX as Field,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ impl Empty for PreviousRollupBlockData {

impl PreviousRollupBlockData {
fn validate_in_vk_tree<let N: u32>(self, allowed_indices: [u32; N]) {
self.vk.check_hash();

let leaf_index = self.vk_witness.leaf_index as u32;
let index_hint = unsafe {
find_index_hint(allowed_indices, |index: u32| index == leaf_index)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ impl Empty for PreviousRollupData {

impl PreviousRollupData {
fn validate_in_vk_tree<let N: u32>(self, allowed_indices: [u32; N]) {
self.vk.check_hash();

let leaf_index = self.vk_witness.leaf_index as u32;
let index_hint = unsafe {
find_index_hint(allowed_indices, |index: u32| index == leaf_index)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ mod tests {
use crate::{tests::block_merge_rollup_inputs::default_block_merge_rollup_inputs};
use dep::types::{hash::accumulate_sha256};
use dep::types::constants::{BLOCK_ROOT_ROLLUP_INDEX, BLOCK_MERGE_ROLLUP_INDEX, ROOT_PARITY_INDEX};
use types::merkle_tree::merkle_tree::MerkleTree;
use dep::types::tests::fixtures;

#[test(should_fail_with = "input blocks have different chain id")]
fn constants_different_chain_id_fails() {
Expand Down Expand Up @@ -145,8 +147,10 @@ mod tests {
#[test]
fn valid_previous_circuit_block_root() {
let mut inputs = default_block_merge_rollup_inputs();
let vk_tree = dep::types::tests::fixtures::vk_tree::get_vk_merkle_tree();
inputs.previous_rollup_data[0].vk.hash = vk_tree.leaves[BLOCK_ROOT_ROLLUP_INDEX];
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};
inputs.previous_rollup_data[0].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(BLOCK_ROOT_ROLLUP_INDEX);
inputs.previous_rollup_data[0].vk_witness.leaf_index = BLOCK_ROOT_ROLLUP_INDEX as Field;
inputs.previous_rollup_data[0].vk_witness.sibling_path = vk_tree.get_sibling_path(BLOCK_ROOT_ROLLUP_INDEX);
let _outputs = inputs.block_merge_rollup_circuit();
Expand All @@ -155,8 +159,10 @@ mod tests {
#[test]
fn valid_previous_circuit_block_merge() {
let mut inputs = default_block_merge_rollup_inputs();
let vk_tree = dep::types::tests::fixtures::vk_tree::get_vk_merkle_tree();
inputs.previous_rollup_data[0].vk.hash = vk_tree.leaves[BLOCK_MERGE_ROLLUP_INDEX];
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};
inputs.previous_rollup_data[0].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(BLOCK_MERGE_ROLLUP_INDEX);
inputs.previous_rollup_data[0].vk_witness.leaf_index = BLOCK_MERGE_ROLLUP_INDEX as Field;
inputs.previous_rollup_data[0].vk_witness.sibling_path = vk_tree.get_sibling_path(BLOCK_MERGE_ROLLUP_INDEX);
let _outputs = inputs.block_merge_rollup_circuit();
Expand All @@ -165,8 +171,10 @@ mod tests {
#[test(should_fail_with = "Invalid vk index")]
fn invalid_previous_circuit() {
let mut inputs = default_block_merge_rollup_inputs();
let vk_tree = dep::types::tests::fixtures::vk_tree::get_vk_merkle_tree();
inputs.previous_rollup_data[0].vk.hash = vk_tree.leaves[ROOT_PARITY_INDEX];
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};
inputs.previous_rollup_data[0].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(ROOT_PARITY_INDEX);
inputs.previous_rollup_data[0].vk_witness.leaf_index = ROOT_PARITY_INDEX as Field;
inputs.previous_rollup_data[0].vk_witness.sibling_path = vk_tree.get_sibling_path(ROOT_PARITY_INDEX);
let _outputs = inputs.block_merge_rollup_circuit();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ mod tests {
use crate::{tests::merge_rollup_inputs::default_merge_rollup_inputs};
use dep::types::hash::accumulate_sha256;
use dep::types::constants::{MERGE_ROLLUP_INDEX, BASE_ROLLUP_INDEX, ROOT_PARITY_INDEX};
use types::merkle_tree::merkle_tree::MerkleTree;
use types::tests::fixtures;

#[test(should_fail_with = "The rollup should be filled greedily from L to R, but received a L base and R merge")]
fn different_rollup_type_fails() {
Expand Down Expand Up @@ -178,9 +180,11 @@ mod tests {
fn valid_previous_circuit_base() {
let mut inputs = default_merge_rollup_inputs();

let vk_tree = dep::types::tests::fixtures::vk_tree::get_vk_merkle_tree();
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};

inputs.previous_rollup_data[0].vk.hash = vk_tree.leaves[BASE_ROLLUP_INDEX];
inputs.previous_rollup_data[0].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(BASE_ROLLUP_INDEX);
inputs.previous_rollup_data[0].vk_witness.leaf_index = BASE_ROLLUP_INDEX as Field;
inputs.previous_rollup_data[0].vk_witness.sibling_path = vk_tree.get_sibling_path(BASE_ROLLUP_INDEX);

Expand All @@ -191,9 +195,11 @@ mod tests {
fn valid_previous_circuit_merge() {
let mut inputs = default_merge_rollup_inputs();

let vk_tree = dep::types::tests::fixtures::vk_tree::get_vk_merkle_tree();
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};

inputs.previous_rollup_data[0].vk.hash = vk_tree.leaves[MERGE_ROLLUP_INDEX];
inputs.previous_rollup_data[0].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(MERGE_ROLLUP_INDEX);
inputs.previous_rollup_data[0].vk_witness.leaf_index = MERGE_ROLLUP_INDEX as Field;
inputs.previous_rollup_data[0].vk_witness.sibling_path = vk_tree.get_sibling_path(MERGE_ROLLUP_INDEX);

Expand All @@ -203,8 +209,10 @@ mod tests {
#[test(should_fail_with = "Invalid vk index")]
fn invalid_previous_circuit() {
let mut inputs = default_merge_rollup_inputs();
let vk_tree = dep::types::tests::fixtures::vk_tree::get_vk_merkle_tree();
inputs.previous_rollup_data[0].vk.hash = vk_tree.leaves[ROOT_PARITY_INDEX];
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};
inputs.previous_rollup_data[0].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(ROOT_PARITY_INDEX);
inputs.previous_rollup_data[0].vk_witness.leaf_index = ROOT_PARITY_INDEX as Field;
inputs.previous_rollup_data[0].vk_witness.sibling_path = vk_tree.get_sibling_path(ROOT_PARITY_INDEX);
let _outputs = inputs.merge_rollup_circuit();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
use dep::types::constants::ROOT_PARITY_INDEX;
use dep::types::tests::fixtures;
use dep::parity_lib::{root::root_rollup_parity_input::RootRollupParityInput};
use types::merkle_tree::merkle_tree::MerkleTree;

pub fn default_root_rollup_parity_input() -> RootRollupParityInput {
let mut input = RootRollupParityInput::empty();

let vk_index = ROOT_PARITY_INDEX;
let vk_tree = fixtures::vk_tree::get_vk_merkle_tree();
let vk_hash = vk_tree.leaves[vk_index];
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};
let vk_path = vk_tree.get_sibling_path(vk_index);
let vk_tree_root = vk_tree.get_root();

input.verification_key.hash = vk_hash;
input.verification_key = fixtures::vk_tree::generate_fake_honk_vk_for_index(vk_index);
input.vk_path = vk_path;
input.public_inputs.vk_tree_root = vk_tree_root;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,23 @@ use dep::types::constants::BLOCK_ROOT_ROLLUP_INDEX;
use dep::types::tests::fixtures;
use dep::types::abis::append_only_tree_snapshot::AppendOnlyTreeSnapshot;
use dep::types::merkle_tree::MembershipWitness;
use types::merkle_tree::merkle_tree::MerkleTree;

pub fn default_previous_rollup_block_data() -> [PreviousRollupBlockData; 2] {
let mut previous_rollup_data = [PreviousRollupBlockData::empty(); 2];

let vk_index = BLOCK_ROOT_ROLLUP_INDEX;
let vk_tree = fixtures::vk_tree::get_vk_merkle_tree();
let vk_hash = vk_tree.leaves[vk_index];
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};
let vk_path = vk_tree.get_sibling_path(vk_index);
let vk_tree_root = vk_tree.get_root();

previous_rollup_data[0].block_root_or_block_merge_public_inputs.vk_tree_root = vk_tree_root;
previous_rollup_data[1].block_root_or_block_merge_public_inputs.vk_tree_root = vk_tree_root;

previous_rollup_data[0].vk.hash = vk_hash;
previous_rollup_data[1].vk.hash = vk_hash;
previous_rollup_data[0].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(vk_index);
previous_rollup_data[1].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(vk_index);

previous_rollup_data[0].vk_witness = MembershipWitness {
leaf_index: vk_index as Field,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,23 @@ use dep::types::constants::BASE_ROLLUP_INDEX;
use dep::types::tests::fixtures;
use dep::types::abis::append_only_tree_snapshot::AppendOnlyTreeSnapshot;
use dep::types::merkle_tree::MembershipWitness;
use types::merkle_tree::merkle_tree::MerkleTree;

pub fn default_previous_rollup_data() -> [PreviousRollupData; 2] {
let mut previous_rollup_data = [PreviousRollupData::empty(); 2];

let vk_index = BASE_ROLLUP_INDEX;
let vk_tree = fixtures::vk_tree::get_vk_merkle_tree();
let vk_hash = vk_tree.leaves[vk_index];
let vk_tree: MerkleTree<fixtures::vk_tree::VK_TREE_WIDTH> = comptime {
fixtures::vk_tree::get_vk_merkle_tree()
};
let vk_path = vk_tree.get_sibling_path(vk_index);
let vk_tree_root = vk_tree.get_root();

previous_rollup_data[0].base_or_merge_rollup_public_inputs.constants.vk_tree_root = vk_tree_root;
previous_rollup_data[1].base_or_merge_rollup_public_inputs.constants.vk_tree_root = vk_tree_root;

previous_rollup_data[0].vk.hash = vk_hash;
previous_rollup_data[1].vk.hash = vk_hash;
previous_rollup_data[0].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(vk_index);
previous_rollup_data[1].vk = fixtures::vk_tree::generate_fake_honk_vk_for_index(vk_index);

previous_rollup_data[0].vk_witness = MembershipWitness {
leaf_index: vk_index as Field,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ impl Verifiable for KernelData {

impl KernelData {
fn validate_in_vk_tree<let N: u32>(self, allowed_indices: [u32; N]) {
self.vk.check_hash();

let index_hint = unsafe {
find_index_hint(allowed_indices, |index: u32| index == self.vk_index)
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ pub struct PrivateKernelData {

impl PrivateKernelData {
fn validate_in_vk_tree<let N: u32>(self, allowed_indices: [u32; N]) {
self.vk.check_hash();

let index_in_allowed_list = if self.vk_index >= PRIVATE_KERNEL_RESET_INDEX {
// Kernel circuits only need to include PRIVATE_KERNEL_RESET_INDEX in the list to allow all private kernel reset variants.
PRIVATE_KERNEL_RESET_INDEX
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ impl Verifiable for PublicKernelData {

impl PublicKernelData {
fn validate_in_vk_tree<let N: u32>(self, allowed_indices: [u32; N]) {
self.vk.check_hash();

let index_hint = unsafe {
find_index_hint(allowed_indices, |index: u32| index == self.vk_index)
};
Expand Down
38 changes: 19 additions & 19 deletions noir-projects/noir-protocol-circuits/crates/types/src/constants.nr
Original file line number Diff line number Diff line change
Expand Up @@ -98,25 +98,25 @@ global MAX_PUBLIC_DATA_HINTS: u32 = 128;
global NUMBER_OF_L1_L2_MESSAGES_PER_ROLLUP: u32 = 16;

// VK TREE CONSTANTS
global EMPTY_NESTED_INDEX: u32 = 0;
global PRIVATE_KERNEL_EMPTY_INDEX: u32 = 1;
global PRIVATE_KERNEL_INIT_INDEX: u32 = 2;
global PRIVATE_KERNEL_INNER_INDEX: u32 = 3;
global PRIVATE_KERNEL_TAIL_INDEX: u32 = 4;
global PRIVATE_KERNEL_TAIL_TO_PUBLIC_INDEX: u32 = 5;
global PUBLIC_KERNEL_MERGE_INDEX: u32 = 6;
global PUBLIC_KERNEL_TAIL_INDEX: u32 = 7;
global PUBLIC_KERNEL_INNER_INDEX: u32 = 8; // TODO(#7124): To be deprecated.
global BASE_PARITY_INDEX: u32 = 10;
global ROOT_PARITY_INDEX: u32 = 11;
global BASE_ROLLUP_INDEX: u32 = 12;
global MERGE_ROLLUP_INDEX: u32 = 13;
global BLOCK_ROOT_ROLLUP_INDEX: u32 = 14;
global BLOCK_MERGE_ROLLUP_INDEX: u32 = 15;
global ROOT_ROLLUP_INDEX: u32 = 16;
global BLOCK_ROOT_ROLLUP_EMPTY_INDEX: u32 = 17;
global TUBE_INDEX: u32 = 18;
global PRIVATE_KERNEL_RESET_INDEX: u32 = 20;
comptime global EMPTY_NESTED_INDEX: u32 = 0;
comptime global PRIVATE_KERNEL_EMPTY_INDEX: u32 = 1;
comptime global PRIVATE_KERNEL_INIT_INDEX: u32 = 2;
comptime global PRIVATE_KERNEL_INNER_INDEX: u32 = 3;
comptime global PRIVATE_KERNEL_TAIL_INDEX: u32 = 4;
comptime global PRIVATE_KERNEL_TAIL_TO_PUBLIC_INDEX: u32 = 5;
comptime global PUBLIC_KERNEL_MERGE_INDEX: u32 = 6;
comptime global PUBLIC_KERNEL_TAIL_INDEX: u32 = 7;
comptime global PUBLIC_KERNEL_INNER_INDEX: u32 = 8; // TODO(#7124): To be deprecated.
comptime global BASE_PARITY_INDEX: u32 = 10;
comptime global ROOT_PARITY_INDEX: u32 = 11;
comptime global BASE_ROLLUP_INDEX: u32 = 12;
comptime global MERGE_ROLLUP_INDEX: u32 = 13;
comptime global BLOCK_ROOT_ROLLUP_INDEX: u32 = 14;
comptime global BLOCK_MERGE_ROLLUP_INDEX: u32 = 15;
comptime global ROOT_ROLLUP_INDEX: u32 = 16;
comptime global BLOCK_ROOT_ROLLUP_EMPTY_INDEX: u32 = 17;
comptime global TUBE_INDEX: u32 = 18;
comptime global PRIVATE_KERNEL_RESET_INDEX: u32 = 20;
// Important: Do not define indexes after the PRIVATE_KERNEL_RESET_INDEX. They are allocated for the variants of private kernel reset.

// MISC CONSTANTS
Expand Down
4 changes: 4 additions & 0 deletions noir-projects/noir-protocol-circuits/crates/types/src/hash.nr
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,10 @@ pub fn compute_tx_note_logs_hash(logs: [LogHash; MAX_NOTE_ENCRYPTED_LOGS_PER_TX]
hash
}

pub fn verification_key_hash<let N: u32>(key: [Field; N]) -> Field {
crate::hash::poseidon2_hash(key)
}

#[inline_always]
pub fn pedersen_hash<let N: u32>(inputs: [Field; N], hash_index: u32) -> Field {
std::hash::pedersen_hash_with_separator(inputs, hash_index)
Expand Down
6 changes: 3 additions & 3 deletions noir-projects/noir-protocol-circuits/crates/types/src/lib.nr
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ mod scalar;
mod contrakt;
mod transaction;
mod abis;
mod constants;
pub mod constants;
mod contract_class_id;
mod merkle_tree;
mod contract_instance;

mod messaging;
mod hash;
mod traits;
pub mod hash;
pub mod traits;
mod type_serialization;

mod content_commitment;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ mod append_only_tree;
mod indexed_tree;
mod leaf_preimage;
mod membership;
mod merkle_tree;
pub mod merkle_tree;
mod root;
mod variable_merkle_tree;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ pub struct VerificationKey<let N: u32> {
hash: Field,
}

impl<let N: u32> VerificationKey<N> {
pub fn check_hash(self) {
assert_eq(self.hash, crate::hash::verification_key_hash(self.key), "Invalid VK hash");
}
}

impl<let N: u32> Serialize<N + 1> for VerificationKey<N> {
fn serialize(self) -> [Field; N + 1] {
let mut fields = [0; N + 1];
Expand Down
Loading

0 comments on commit 3d17e13

Please sign in to comment.