From 82427a63f3096c098d9c4db1e887dd2b6a60818f Mon Sep 17 00:00:00 2001 From: benesjan Date: Fri, 10 Jan 2025 18:01:39 +0000 Subject: [PATCH 1/4] refactor: VariableMerkleTree readability improvements --- .../src/merkle_tree/variable_merkle_tree.nr | 72 +++++++++++++------ 1 file changed, 51 insertions(+), 21 deletions(-) diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr index 701646552f2..6f42bf1c611 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr @@ -6,14 +6,54 @@ pub struct VariableMerkleTree { pub root: Field, } -unconstrained fn get_height(input: u32, start: u8) -> u8 { - let mut height = 0; +/// Recursively calculates the smallest power exponent `n` where `2^n` is greater than or equal +/// to the input value, starting from a given exponent. +/// +/// # Arguments +/// +/// * `input` - Target value to find the next power of 2 that exceeds or equals it +/// * `start` - Initial exponent to start checking from +/// +/// # Returns +/// +/// The smallest exponent `n` where `2^n >= input` +unconstrained fn get_next_power_exponent(input: u32, start: u8) -> u8 { + let mut next_power_exponent = 0; if input <= 2 << start { - height = start; + next_power_exponent = start; } else { - height = get_height(input, start + 1); + next_power_exponent = get_next_power_exponent(input, start + 1); } - height + next_power_exponent +} + +/// Calculates the previous power of 2. +/// +/// The previous power of 2 is the largest power of 2 that is smaller than `value`. +/// For example: +/// - For 7, previous power of 2 is 4 (2^2) +/// - For 10, previous power of 2 is 8 (2^3) +/// - For 16, previous power of 2 is 8 (2^3) +/// +/// # Arguments +/// * value - Integer value for which we need to find the previous power of 2 +/// +/// # Returns +/// The previous power of 2 +fn get_prev_power_2(value: u32) -> u32 { + let next_power_exponent = unsafe { + //@safety This is a hint that we'll use to compute the next and previous powers of two, which we check to be + // larger and smaller than respectively. The get_next_power_exponent function happens to return exactly what + // we need. + get_next_power_exponent(value, 0) + }; + + let next_power_2 = 2 << next_power_exponent; + let prev_power_2 = next_power_2 / 2; + assert((value == 0) | (value == 1) | (value > prev_power_2)); + assert(value <= next_power_2); + + prev_power_2 } // This calculates the root of the minimal size merkle tree required @@ -32,23 +72,12 @@ impl VariableMerkleTree { // | tx_0 | | tx_1 | // pub fn new_sha(leaves: [Field; N], num_non_empty_leaves: u32) -> Self { - // Find size of tree required - let height = unsafe { get_height(num_non_empty_leaves, 0) }; - let next_power_2 = 2 << height; - let prev_power_2 = next_power_2 / 2; - assert( - (num_non_empty_leaves == 0) - | (num_non_empty_leaves == 1) - | (num_non_empty_leaves > prev_power_2), - ); - assert(num_non_empty_leaves <= next_power_2); + let prev_power_2 = get_prev_power_2(num_non_empty_leaves); + // hash base layer // If we have no num_non_empty_leaves, we return 0 - let mut stop = if num_non_empty_leaves == 0 { - true - } else { - false - }; + let mut stop = num_non_empty_leaves == 0; + let mut nodes = [0; N]; for i in 0..N / 2 { // stop after non zero leaves @@ -61,7 +90,8 @@ impl VariableMerkleTree { } // hash the other layers - stop = if prev_power_2 == 1 { true } else { false }; + stop = prev_power_2 == 1; + let mut next_layer_end = prev_power_2 / 2; let mut next_layer_size = next_layer_end; let mut root = nodes[0]; From 91a5ae980b47c44712cabb7c76c75a01ebbde453 Mon Sep 17 00:00:00 2001 From: benesjan Date: Fri, 10 Jan 2025 18:02:43 +0000 Subject: [PATCH 2/4] fmt --- .../crates/types/src/merkle_tree/variable_merkle_tree.nr | 1 + 1 file changed, 1 insertion(+) diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr index 6f42bf1c611..22f13811692 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr @@ -220,3 +220,4 @@ pub mod tests { assert_eq(tree.root, full_tree.get_root()); } } + From 0d66dd7576b4575dd5d90c02bcbf1ba504ec3cd0 Mon Sep 17 00:00:00 2001 From: benesjan Date: Fri, 10 Jan 2025 18:06:58 +0000 Subject: [PATCH 3/4] underconstrained bug explanation --- .../crates/types/src/merkle_tree/variable_merkle_tree.nr | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr index 22f13811692..52d3287e37e 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr @@ -50,6 +50,8 @@ fn get_prev_power_2(value: u32) -> u32 { let next_power_2 = 2 << next_power_exponent; let prev_power_2 = next_power_2 / 2; + // If value equals 0 or 1, the previous power of 2 is completely unconstrained and we could craft response + // of `get_next_power_exponent` such that prev_power_2 is anything in [1, 2, 4, 8, 16, ...]. assert((value == 0) | (value == 1) | (value > prev_power_2)); assert(value <= next_power_2); @@ -78,6 +80,10 @@ impl VariableMerkleTree { // If we have no num_non_empty_leaves, we return 0 let mut stop = num_non_empty_leaves == 0; + // What damage could we cause in the base layer due to the underconstrained bug? + // For num_non_empty_leaves = 0, there is no damage because stop is set to true. + // For num_non_empty_leaves = 1, this could allow us to hash also the empty leaves. This seems fine as this + // is only an optimization. Right? or do we somewhere rely on the other leaves actually being empty? let mut nodes = [0; N]; for i in 0..N / 2 { // stop after non zero leaves @@ -91,12 +97,15 @@ impl VariableMerkleTree { // hash the other layers stop = prev_power_2 == 1; + // Here we can cause damage even for num_non_empty_leaves = 0, because it's not part of the stop condition let mut next_layer_end = prev_power_2 / 2; let mut next_layer_size = next_layer_end; let mut root = nodes[0]; for i in 0..(N - 1 - N / 2) { if !stop { + // For `num_non_empty_leaves` = 0 and 1 we can essentially cause havoc here as prev_power_2 is + // directly used in index here and on line 117 nodes[prev_power_2 + i] = accumulate_sha256([nodes[2 * i], nodes[2 * i + 1]]); if i == next_layer_end { // Reached next layer => move up one layer From 3bcccd6afbdc692514db36fe6851beb7162130bd Mon Sep 17 00:00:00 2001 From: benesjan Date: Fri, 10 Jan 2025 18:15:28 +0000 Subject: [PATCH 4/4] Revert "underconstrained bug explanation" This reverts commit a8ef6fac947952641a7c2975fa79943ae1ffc269. --- .../crates/types/src/merkle_tree/variable_merkle_tree.nr | 9 --------- 1 file changed, 9 deletions(-) diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr index 52d3287e37e..22f13811692 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr @@ -50,8 +50,6 @@ fn get_prev_power_2(value: u32) -> u32 { let next_power_2 = 2 << next_power_exponent; let prev_power_2 = next_power_2 / 2; - // If value equals 0 or 1, the previous power of 2 is completely unconstrained and we could craft response - // of `get_next_power_exponent` such that prev_power_2 is anything in [1, 2, 4, 8, 16, ...]. assert((value == 0) | (value == 1) | (value > prev_power_2)); assert(value <= next_power_2); @@ -80,10 +78,6 @@ impl VariableMerkleTree { // If we have no num_non_empty_leaves, we return 0 let mut stop = num_non_empty_leaves == 0; - // What damage could we cause in the base layer due to the underconstrained bug? - // For num_non_empty_leaves = 0, there is no damage because stop is set to true. - // For num_non_empty_leaves = 1, this could allow us to hash also the empty leaves. This seems fine as this - // is only an optimization. Right? or do we somewhere rely on the other leaves actually being empty? let mut nodes = [0; N]; for i in 0..N / 2 { // stop after non zero leaves @@ -97,15 +91,12 @@ impl VariableMerkleTree { // hash the other layers stop = prev_power_2 == 1; - // Here we can cause damage even for num_non_empty_leaves = 0, because it's not part of the stop condition let mut next_layer_end = prev_power_2 / 2; let mut next_layer_size = next_layer_end; let mut root = nodes[0]; for i in 0..(N - 1 - N / 2) { if !stop { - // For `num_non_empty_leaves` = 0 and 1 we can essentially cause havoc here as prev_power_2 is - // directly used in index here and on line 117 nodes[prev_power_2 + i] = accumulate_sha256([nodes[2 * i], nodes[2 * i + 1]]); if i == next_layer_end { // Reached next layer => move up one layer