Skip to content

perf: Sequential reads in commit_columns_bit_reversed #560

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

Draft
nicole-graus wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

perf: Sequential reads in commit_columns_bit_reversed #560

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97b32c8
Replace scattered with sequential reads
nicole-graus Apr 24, 2026
a96b574
Merge branch 'main' into commit-columns-sequential-reads
nicole-graus Apr 24, 2026
5093451
Merge branch 'main' into commit-columns-sequential-reads
diegokingston Apr 24, 2026
4df366b
Merge branch 'main' into commit-columns-sequential-reads
nicole-graus Apr 27, 2026
f894d34
Merge branch 'main' into commit-columns-sequential-reads
diegokingston Apr 30, 2026
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
20 changes: 10 additions & 10 deletions crypto/stark/src/prover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,10 +389,11 @@ pub trait IsStarkProver<
/// Builds a Merkle tree commitment from column-major LDE evaluations with
/// bit-reverse permutation, without cloning the full evaluation matrix.
///
/// For each row index `i`, we hash `col_0[br(i)] || col_1[br(i)] || ...`
/// where `br(i)` is the bit-reversal of `i`. This produces the same Merkle
/// tree as the old clone + bit-reverse + columns2rows + batch_commit flow,
/// but avoids allocating the cloned and transposed matrices entirely.
/// Hashes `col_0[k] || col_1[k] || ...` for k = 0..num_rows (sequential column
/// reads, cache-friendly), then permutes the hash vector in bit-reversed order
/// so leaves[i] = hash(col_0[br(i)] || col_1[br(i)] || ...). Same Merkle tree
/// as reading at br(row_idx) inside the hashing loop, but the scattered column
/// access is replaced by a single small bit-reverse pass over 32-byte digests.
fn commit_columns_bit_reversed<E>(
columns: &[Vec<FieldElement<E>>],
) -> Option<(BatchedMerkleTree<E>, Commitment)>
Expand Down Expand Up @@ -420,21 +421,20 @@ pub trait IsStarkProver<
#[cfg(not(feature = "parallel"))]
let iter = 0..num_rows;

// One allocation per row (was one per field element): write all columns
// into a single buffer, then hash once.
let hashed_leaves: Vec<Commitment> = iter
.map(|row_idx| {
let br_idx = reverse_index(row_idx, num_rows as u64);
let mut hashed_leaves: Vec<Commitment> = iter
.map(|k| {
let total_bytes = num_cols * byte_len;
let mut buf = vec![0u8; total_bytes];
for col_idx in 0..num_cols {
columns[col_idx][br_idx]
columns[col_idx][k]
.write_bytes_be(&mut buf[col_idx * byte_len..(col_idx + 1) * byte_len]);
}
BatchedMerkleTreeBackend::<E>::hash_bytes(&buf)
})
.collect();

in_place_bit_reverse_permute(&mut hashed_leaves);
Copy link
Copy Markdown

@claude claude Bot Apr 24, 2026

Choose a reason for hiding this comment

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

Medium – No test validates the equivalence directly

The mathematical transformation is correct: sequential_hash[k] = hash(col_0[k] || ...), and after in_place_bit_reverse_permute, leaves[i] = hash(col_0[br(i)] || ...) — identical to the old scatter read. The prove-verify roundtrip tests cover this implicitly, but since this is a correctness-critical cryptographic primitive, a direct unit test comparing the Merkle root produced by both approaches (old scatter-read vs. new sequential + permute) would be valuable insurance against future regressions.

Low – in_place_bit_reverse_permute silently miscomputes for non-power-of-two lengths in release builds

The debug_assert! at line 414–417 only fires in debug mode. in_place_bit_reverse_permute itself has no such guard — it calls size.trailing_zeros() which is only log2(size) when size is a power of two. In a release build with a non-power-of-two num_rows, the permutation would silently produce a wrong (but non-panicking) digest ordering. This is a pre-existing issue (the old reverse_index call had the same gap), but it's now applied to the committed digest array. Consider promoting the assert or adding a release-mode guard in in_place_bit_reverse_permute.


let tree = BatchedMerkleTree::<E>::build_from_hashed_leaves(hashed_leaves)?;
let root = tree.root;
Some((tree, root))
Expand Down
Loading