Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: underconstrained bug #11174

Merged
merged 3 commits into from
Jan 13, 2025
Merged

fix: underconstrained bug #11174

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e35f952
underconstrained bug explanation
benesjan Jan 10, 2025
865e80c
Revert "underconstrained bug explanation"
benesjan Jan 10, 2025
52422a8
fix: underconstrained bug
benesjan Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 23 additions & 22 deletions noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::hash::accumulate_sha256;

// N = maximum leaves
// For now we only care about the root
pub struct VariableMerkleTree {
Expand Down Expand Up @@ -50,17 +51,15 @@ fn get_prev_power_2(value: u32) -> u32 {

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(prev_power_2 < value);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If value equals 0 or 1, the previous power of 2 is completely unconstrained and we could craft a response from get_next_power_exponent such that prev_power_2 is anything in [1, 2, 4, 8, 16, ...].

assert(value <= next_power_2);

prev_power_2
}

// This calculates the root of the minimal size merkle tree required
// to store num_non_empty_leaves
// Since we cannot isolate branches, it doesn't cost fewer gates than using
// MerkleTree on the full array of elements N, but is slightly cheaper on-chain
// and cleaner elsewhere.
// Calculates the root of the minimal size merkle tree required to store num_non_empty_leaves.
// Since we cannot isolate branches, it doesn't cost fewer gates than using MerkleTree on the full array of elements N,
// but is slightly cheaper on-chain and cleaner elsewhere.
impl VariableMerkleTree {
// Example - tx_0 with 3 msgs | tx_1 with 2 msgs creates:
//
Expand All @@ -72,40 +71,42 @@ impl VariableMerkleTree {
// | tx_0 | | tx_1 |
//
pub fn new_sha<let N: u32>(leaves: [Field; N], num_non_empty_leaves: u32) -> Self {
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 = num_non_empty_leaves == 0;
Copy link
Contributor Author

@benesjan benesjan Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the code changes we could do the following when hashing base layer.

For num_non_empty_leaves = 0, there is no damage because stop on the line above would be set to true.
For num_non_empty_leaves = 1, this could allow us to hash also the empty leaves getting a different root.


let num_nodes_layer_1 = if (num_non_empty_leaves == 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed prev_power_2 to num_nodes_layer_1 as I found it hard to reason about prev_power_2.

// For 0 leaves, there is no layer 1, no hashing happens and the root is set to 0
0
} else if (num_non_empty_leaves == 1) {
// For 1 leaf, 1 round of hashing happens and root = hash([leaf, 0])
1
} else {
// For more than 1 leaf, we dynamically compute num of nodes in layer 1 by finding the previous power of 2
get_prev_power_2(num_non_empty_leaves)
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ^ ifs are the fix --> now we don't enter get_prev_power_2 for num_non_empty_leaves < 2 and instead in those situations we explicitly set the values.


// We hash the base layer
let mut nodes = [0; N];
for i in 0..N / 2 {
// stop after non zero leaves
if i == prev_power_2 {
stop = true;
}
if (!stop) {
if (i < num_nodes_layer_1) {
nodes[i] = accumulate_sha256([leaves[2 * i], leaves[2 * i + 1]]);
}
}

// hash the other layers
stop = prev_power_2 == 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we can cause damage even for num_non_empty_leaves = 0, because we decide whether to hash just based on prev_power_2.

// We hash the other layers
let mut stop = num_non_empty_leaves < 3;

let mut next_layer_end = prev_power_2 / 2;
let mut next_layer_end = num_nodes_layer_1 / 2;
let mut next_layer_size = next_layer_end;
let mut root = nodes[0];
for i in 0..(N - 1 - N / 2) {
if !stop {
nodes[prev_power_2 + i] = accumulate_sha256([nodes[2 * i], nodes[2 * i + 1]]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For num_non_empty_leaves = 0 and 1 we could cause havoc here as prev_power_2 is directly used in index here and on line 108.

nodes[num_nodes_layer_1 + i] = accumulate_sha256([nodes[2 * i], nodes[2 * i + 1]]);
if i == next_layer_end {
// Reached next layer => move up one layer
next_layer_size = next_layer_size / 2;
next_layer_end += next_layer_size;
}
if (next_layer_size == 1) {
// Reached root
root = nodes[prev_power_2 + i];
root = nodes[num_nodes_layer_1 + i];
stop = true;
}
}
Expand Down
Loading