Skip to content

Commit 8c3592d

Browse files
authored
Auditing uses of unsafe (#852)
* Replace unsafe NonZero with safe alternative, document merkle unsafe - sparse_path.rs: Replace unsafe NonZero::new_unchecked with safe NonZero::new().expect(). Micro-benchmarks show no performance difference between safe and unsafe versions (both ~175ps). - merkle_tree.rs: Add performance documentation explaining why unsafe code is kept. Benchmarks at 4K+ leaves show ~2-2.5% improvement (65K leaves: unsafe=67.17ms vs safe=68.27ms). - Add sparse_path.rs benchmark suite for future regression testing. - Add init_vector safe alternative in utils/mod.rs. - Extend merkle benchmarks to 65K leaves for scale testing. * Evaluate remaining unsafe code, add benchmarks and documentation - Add transpose benchmark showing 31% perf improvement from uninit_vector - Expand SAFETY comments in digest.rs:digests_as_bytes explaining repr(transparent) layout guarantees - Expand SAFETY comments in empty_roots.rs:empty_hashes documenting bounds correctness and const fn requirement - Add performance documentation to transpose_slice - Update unsafe-evaluation.prose to cover all remaining unsafe uses * Use MaybeUninit vector init helpers * Bump MSRV to 1.93 * Fix large forest entries regression and add seed * Fix Merkle tree unsafe usage and document invariants * Avoid recursion in large forest iterator and fix MerkleTree reads * Fix empty history iteration ordering
1 parent 8ee55c1 commit 8c3592d

File tree

14 files changed

+646
-166
lines changed

14 files changed

+646
-166
lines changed

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ edition = "2024"
1010
keywords = ["crypto", "hash", "merkle", "miden"]
1111
license = "MIT OR Apache-2.0"
1212
repository = "https://github.com/0xMiden/crypto"
13-
rust-version = "1.90"
13+
rust-version = "1.93"
1414
version = "0.23.0"
1515

1616
[workspace.dependencies]

miden-crypto/Cargo.toml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,16 @@ harness = false
6969
name = "rand"
7070
required-features = ["std"]
7171

72+
[[bench]]
73+
harness = false
74+
name = "sparse_path"
75+
required-features = ["std"]
76+
77+
[[bench]]
78+
harness = false
79+
name = "transpose"
80+
required-features = ["std"]
81+
7282
[features]
7383
concurrent = ["dep:rayon", "p3-maybe-rayon/parallel", "p3-miden-prover/parallel", "p3-util/parallel", "std"]
7484
default = ["concurrent", "std"]

miden-crypto/benches/merkle.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ use common::{
2222
benchmark_multi!(
2323
merkle_tree_construction,
2424
"merkle_tree_construction",
25-
&[4, 8, 16, 32, 64, 128, 256],
25+
&[4, 8, 16, 32, 64, 128, 256, 1024, 4096, 16384, 65536],
2626
|b: &mut Bencher<'_>, &num_leaves: &usize| {
2727
b.iter_batched(
2828
|| generate_words_pattern(num_leaves, WordPattern::Random),
@@ -38,7 +38,7 @@ benchmark_multi!(
3838
benchmark_multi!(
3939
balanced_merkle_even,
4040
"balanced-merkle-even",
41-
&[4, 8, 16, 32, 64, 128, 256],
41+
&[4, 8, 16, 32, 64, 128, 256, 1024, 4096, 16384, 65536],
4242
|b: &mut Bencher<'_>, num_leaves: &usize| {
4343
b.iter_batched(
4444
|| {
Lines changed: 235 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,235 @@
1+
//! SparseMerklePath operation benchmarks.
2+
//!
3+
//! This module benchmarks operations on SparseMerklePath, with a focus on
4+
//! the NonZero construction pattern used in path_depth_iter at line 446.
5+
6+
use std::hint;
7+
8+
use criterion::{Bencher, Criterion, criterion_group, criterion_main};
9+
use miden_crypto::{
10+
Felt, Word,
11+
merkle::{
12+
MerklePath, MerkleTree, NodeIndex,
13+
smt::{LeafIndex, SimpleSmt},
14+
},
15+
};
16+
17+
mod common;
18+
use common::{config::*, data::*};
19+
20+
// === SparseMerklePath Construction Benchmarks ===
21+
22+
benchmark_multi!(
23+
sparse_path_from_sized_iter,
24+
"sparse_path_from_sized_iter",
25+
&[8, 12, 16],
26+
|b: &mut Bencher<'_>, &depth: &usize| {
27+
b.iter_batched(
28+
|| generate_words_pattern(1usize << depth, WordPattern::Random),
29+
|leaves| {
30+
let tree = MerkleTree::new(hint::black_box(leaves)).unwrap();
31+
let path = tree.get_path(NodeIndex::new(depth as u8, 0).unwrap()).unwrap();
32+
let _sparse_path = miden_crypto::merkle::SparseMerklePath::from_sized_iter(path);
33+
},
34+
criterion::BatchSize::SmallInput,
35+
);
36+
}
37+
);
38+
39+
// === NonZero Construction Pattern Benchmarks ===
40+
// Focus on line 446: unsafe { NonZero::new_unchecked(depth) }
41+
// We benchmark iteration which exercises this code path
42+
43+
benchmark_with_setup_data!(
44+
sparse_path_iteration_full,
45+
DEFAULT_MEASUREMENT_TIME,
46+
DEFAULT_SAMPLE_SIZE,
47+
"sparse_path_iteration_full",
48+
|| {
49+
// Create a SparseMerklePath at depth 16
50+
let leaves = generate_words_pattern(1 << 16, WordPattern::Random);
51+
let tree = MerkleTree::new(leaves).unwrap();
52+
let path = tree.get_path(NodeIndex::new(16, 0).unwrap()).unwrap();
53+
miden_crypto::merkle::SparseMerklePath::from_sized_iter(path).unwrap()
54+
},
55+
|b: &mut Bencher<'_>, sparse_path: &miden_crypto::merkle::SparseMerklePath| {
56+
b.iter(|| {
57+
let _count: Vec<_> = hint::black_box(sparse_path.iter()).collect();
58+
});
59+
}
60+
);
61+
62+
// === Comparison with Regular MerklePath ===
63+
64+
benchmark_with_setup_data!(
65+
compare_path_iteration,
66+
DEFAULT_MEASUREMENT_TIME,
67+
DEFAULT_SAMPLE_SIZE,
68+
"compare_path_iteration",
69+
|| {
70+
let leaves = generate_words_merkle_std(256);
71+
let tree = MerkleTree::new(&leaves).unwrap();
72+
let merkle_path = tree.get_path(NodeIndex::new(8, 128).unwrap()).unwrap();
73+
let sparse_path =
74+
miden_crypto::merkle::SparseMerklePath::from_sized_iter(merkle_path.clone()).unwrap();
75+
(merkle_path, sparse_path)
76+
},
77+
|b: &mut Bencher<'_>,
78+
(merkle_path, sparse_path): &(MerklePath, miden_crypto::merkle::SparseMerklePath)| {
79+
b.iter(|| {
80+
let merkle_nodes: Vec<_> = hint::black_box(merkle_path.iter()).collect();
81+
let sparse_nodes: Vec<_> = hint::black_box(sparse_path.iter()).collect();
82+
// Ensure both iterators produce the same number of nodes
83+
assert_eq!(merkle_nodes.len(), sparse_nodes.len());
84+
});
85+
}
86+
);
87+
88+
// === SparseMerklePath with Empty Nodes ===
89+
90+
benchmark_with_setup_data!(
91+
sparse_path_with_empty_nodes,
92+
DEFAULT_MEASUREMENT_TIME,
93+
DEFAULT_SAMPLE_SIZE,
94+
"sparse_path_with_empty_nodes",
95+
|| {
96+
// Create a SimpleSmt which will have many empty nodes
97+
let mut tree = SimpleSmt::new().unwrap();
98+
let mut indices = Vec::new();
99+
let mut values = Vec::new();
100+
101+
// Insert a few sparse entries
102+
for i in [0u64, 100, 500, 1000] {
103+
let leaf_idx = LeafIndex::new(i).unwrap();
104+
let value = Word::new([Felt::new(i), Felt::new(i), Felt::new(i), Felt::new(i)]);
105+
tree.insert(leaf_idx, value);
106+
indices.push(i);
107+
values.push(value);
108+
}
109+
110+
(tree, indices, values)
111+
},
112+
|b: &mut Bencher<'_>, (tree, indices, values): &(SimpleSmt<10>, Vec<u64>, Vec<Word>)| {
113+
b.iter(|| {
114+
for (idx, _val) in indices.iter().zip(values.iter()) {
115+
let leaf_idx = LeafIndex::new(*idx).unwrap();
116+
let proof = tree.open(&leaf_idx);
117+
let _sparse = hint::black_box(
118+
miden_crypto::merkle::SparseMerklePath::from_sized_iter(proof.path.into_iter())
119+
.unwrap(),
120+
);
121+
}
122+
});
123+
}
124+
);
125+
126+
// === at_depth Operation Benchmark ===
127+
128+
benchmark_with_setup_data!(
129+
sparse_path_at_depth,
130+
DEFAULT_MEASUREMENT_TIME,
131+
DEFAULT_SAMPLE_SIZE,
132+
"sparse_path_at_depth",
133+
|| {
134+
let leaves = generate_words_merkle_std(256);
135+
let tree = MerkleTree::new(&leaves).unwrap();
136+
let path = tree.get_path(NodeIndex::new(8, 128).unwrap()).unwrap();
137+
let sparse_path = miden_crypto::merkle::SparseMerklePath::from_sized_iter(path).unwrap();
138+
(sparse_path, 8u8)
139+
},
140+
|b: &mut Bencher<'_>, (sparse_path, depth): &(miden_crypto::merkle::SparseMerklePath, u8)| {
141+
b.iter(|| {
142+
for d in 1..=*depth {
143+
let nz = core::num::NonZero::new(d).unwrap();
144+
let _node = hint::black_box(sparse_path.at_depth(nz).unwrap());
145+
}
146+
});
147+
}
148+
);
149+
150+
// === Conversion Benchmarks ===
151+
152+
benchmark_with_setup_data!(
153+
merkle_to_sparse_conversion,
154+
DEFAULT_MEASUREMENT_TIME,
155+
DEFAULT_SAMPLE_SIZE,
156+
"merkle_to_sparse_conversion",
157+
|| {
158+
let leaves = generate_words_merkle_std(256);
159+
let tree = MerkleTree::new(&leaves).unwrap();
160+
tree.get_path(NodeIndex::new(8, 128).unwrap()).unwrap()
161+
},
162+
|b: &mut Bencher<'_>, path: &MerklePath| {
163+
b.iter(|| {
164+
let _sparse = hint::black_box(
165+
miden_crypto::merkle::SparseMerklePath::try_from(hint::black_box(path.clone()))
166+
.unwrap(),
167+
);
168+
});
169+
}
170+
);
171+
172+
benchmark_with_setup_data!(
173+
sparse_to_merkle_conversion,
174+
DEFAULT_MEASUREMENT_TIME,
175+
DEFAULT_SAMPLE_SIZE,
176+
"sparse_to_merkle_conversion",
177+
|| {
178+
let leaves = generate_words_merkle_std(256);
179+
let tree = MerkleTree::new(&leaves).unwrap();
180+
let path = tree.get_path(NodeIndex::new(8, 128).unwrap()).unwrap();
181+
miden_crypto::merkle::SparseMerklePath::from_sized_iter(path).unwrap()
182+
},
183+
|b: &mut Bencher<'_>, sparse_path: &miden_crypto::merkle::SparseMerklePath| {
184+
b.iter(|| {
185+
let _merkle: MerklePath = hint::black_box(sparse_path.clone().into());
186+
});
187+
}
188+
);
189+
190+
// === Micro-benchmark: NonZero construction pattern comparison ===
191+
192+
fn nonzero_safe_vs_unsafe(c: &mut Criterion) {
193+
let depths: Vec<u8> = (1u8..=64u8).collect();
194+
let mut group = c.benchmark_group("nonzero_construction");
195+
group.measurement_time(DEFAULT_MEASUREMENT_TIME);
196+
group.sample_size(DEFAULT_SAMPLE_SIZE);
197+
198+
// Safe construction
199+
group.bench_function("safe", |b| {
200+
b.iter(|| {
201+
for &depth in &depths {
202+
let _nz = core::num::NonZero::new(depth);
203+
}
204+
})
205+
});
206+
207+
// Unsafe construction (current code at line 446)
208+
group.bench_function("unsafe", |b| {
209+
b.iter(|| {
210+
for &depth in &depths {
211+
// SAFETY: In the actual code, depth comes from RangeInclusive<1, _>
212+
// which guarantees depth >= 1
213+
let _nz = unsafe { core::num::NonZero::new_unchecked(depth) };
214+
}
215+
})
216+
});
217+
218+
group.finish();
219+
}
220+
221+
// === Benchmark Group Definition ===
222+
223+
criterion_group!(
224+
sparse_path_benches,
225+
sparse_path_from_sized_iter,
226+
sparse_path_iteration_full,
227+
compare_path_iteration,
228+
sparse_path_with_empty_nodes,
229+
sparse_path_at_depth,
230+
merkle_to_sparse_conversion,
231+
sparse_to_merkle_conversion,
232+
nonzero_safe_vs_unsafe,
233+
);
234+
235+
criterion_main!(sparse_path_benches);

miden-crypto/benches/transpose.rs

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
//! Benchmark for transpose_slice comparing unsafe uninit_vector vs safe init_vector.
2+
//!
3+
//! Results at 1024x1024: unsafe=188us, safe=247us (~31% slower with initialization)
4+
5+
use std::hint::black_box;
6+
7+
use criterion::{Bencher, Criterion, criterion_group, criterion_main};
8+
use miden_crypto::Felt;
9+
10+
mod common;
11+
12+
const MATRIX_SIZES: &[usize] = &[64, 256, 1024];
13+
14+
fn generate_felt_matrix(size: usize) -> Vec<Felt> {
15+
(0..(size * size)).map(|i| Felt::new(i as u64)).collect()
16+
}
17+
18+
/// Unsafe transpose using uninit_vector
19+
#[expect(clippy::uninit_vec)]
20+
fn transpose_unsafe<T: Copy + Send + Sync, const N: usize>(source: &[T]) -> Vec<[T; N]> {
21+
let row_count = source.len() / N;
22+
assert_eq!(row_count * N, source.len());
23+
24+
let mut result: Vec<[T; N]> = unsafe {
25+
let mut vector = Vec::with_capacity(row_count);
26+
vector.set_len(row_count);
27+
vector
28+
};
29+
30+
result.iter_mut().enumerate().for_each(|(i, element)| {
31+
for j in 0..N {
32+
element[j] = source[i + j * row_count]
33+
}
34+
});
35+
result
36+
}
37+
38+
/// Safe transpose using initialized vector
39+
fn transpose_safe<T: Copy + Default + Send + Sync, const N: usize>(source: &[T]) -> Vec<[T; N]> {
40+
let row_count = source.len() / N;
41+
assert_eq!(row_count * N, source.len());
42+
43+
let mut result: Vec<[T; N]> = vec![[T::default(); N]; row_count];
44+
45+
result.iter_mut().enumerate().for_each(|(i, element)| {
46+
for j in 0..N {
47+
element[j] = source[i + j * row_count]
48+
}
49+
});
50+
result
51+
}
52+
53+
benchmark_multi!(
54+
transpose_unsafe_bench,
55+
"transpose_unsafe",
56+
MATRIX_SIZES,
57+
|b: &mut Bencher<'_>, &size: &usize| {
58+
let data = generate_felt_matrix(size);
59+
b.iter(|| {
60+
let result: Vec<[Felt; 4]> = transpose_unsafe(black_box(&data));
61+
black_box(result);
62+
})
63+
}
64+
);
65+
66+
benchmark_multi!(
67+
transpose_safe_bench,
68+
"transpose_safe",
69+
MATRIX_SIZES,
70+
|b: &mut Bencher<'_>, &size: &usize| {
71+
let data = generate_felt_matrix(size);
72+
b.iter(|| {
73+
let result: Vec<[Felt; 4]> = transpose_safe(black_box(&data));
74+
black_box(result);
75+
})
76+
}
77+
);
78+
79+
benchmark_multi!(
80+
transpose_library_bench,
81+
"transpose_library",
82+
MATRIX_SIZES,
83+
|b: &mut Bencher<'_>, &size: &usize| {
84+
use miden_crypto::utils::transpose_slice;
85+
let data = generate_felt_matrix(size);
86+
b.iter(|| {
87+
let result: Vec<[Felt; 4]> = transpose_slice::<Felt, 4>(black_box(&data));
88+
black_box(result);
89+
})
90+
}
91+
);
92+
93+
criterion_group!(
94+
transpose_benchmarks,
95+
transpose_unsafe_bench,
96+
transpose_safe_bench,
97+
transpose_library_bench,
98+
);
99+
100+
criterion_main!(transpose_benchmarks);
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
cc abf493f7aece0d6db6c370e3c95651effc9f46f7a2f97e2dccddacfce2afd16d

0 commit comments

Comments
 (0)