Repository navigation
feat(circuit-verifier): add compute_circuit_hash - #647
Conversation
PR SummaryMedium Risk Overview
A unit test compares the in-circuit hash to an independent host Reviewed by Cursor Bugbot for commit 2350bb2. Bugbot is set up for automated code reviews on this repo. Configure here. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
f22b909 to
c2b6759
Compare
c2b6759 to
7300b3c
Compare
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on ilyalesokhin-starkware).
crates/circuit_verifier/src/circuit_hash.rs line 140 at r1 (raw file):
assert_eq!(in_circuit, host, "in-circuit builder must match the host twin"); } }
Move to test file
Code quote:
#[cfg(test)]
mod tests {
use blake2::{Blake2s256, Digest};
use circuits::blake::HashValue;
use circuits::context::TraceContext;
use circuits::ivalue::IValue;
use circuits::ops::Guess;
use circuits_stark_verifier::order_hash_map::OrderedHashMap;
use stwo::core::fields::qm31::QM31;
use super::*;
fn sample_component_log_sizes() -> OrderedHashMap<&'static str, u32> {
COMPONENT_NAMES.iter().enumerate().map(|(i, name)| (*name, (i % 20) as u32)).collect()
}
/// Independent reference for [`compute_circuit_hash`] using the standard `blake2` crate.
fn expected_circuit_hash(
component_log_sizes: &OrderedHashMap<&'static str, u32>,
log_blowup_factor: u32,
preprocessed_root: [u32; BLAKE2S_DIGEST_N_WORDS],
) -> [u32; BLAKE2S_DIGEST_N_WORDS] {
let mut config_bytes = vec![log_blowup_factor as u8];
config_bytes.extend(
COMPONENT_NAMES.iter().map(|name| *component_log_sizes.get(*name).unwrap() as u8),
);
let config_words =
config_bytes.chunks_exact(4).map(|b| u32::from_le_bytes(b.try_into().unwrap()));
let mut hasher = Blake2s256::new();
for word in config_words.chain(preprocessed_root) {
hasher.update(word.to_le_bytes());
}
let digest: [u8; 4 * BLAKE2S_DIGEST_N_WORDS] = hasher.finalize().into();
std::array::from_fn(|i| u32::from_le_bytes(digest[i * 4..i * 4 + 4].try_into().unwrap()))
}
/// Computes the circuit hash in a fresh context and reads back the eight concrete output words.
fn circuit_hash_words(
component_log_sizes: &OrderedHashMap<&'static str, u32>,
log_blowup_factor: u32,
preprocessed_root: [u32; BLAKE2S_DIGEST_N_WORDS],
) -> [u32; BLAKE2S_DIGEST_N_WORDS] {
let mut context = TraceContext::default();
let root = HashValue::<QM31>::from(preprocessed_root).guess(&mut context);
let hash =
compute_circuit_hash(&mut context, component_log_sizes, log_blowup_factor, &root);
std::array::from_fn(|i| context.get(*hash[i].get()).unpack_u32())
}
#[test]
fn compute_circuit_hash_matches_expected() {
let sizes = sample_component_log_sizes();
let root = std::array::from_fn(|i| i as u32);
let expected = expected_circuit_hash(&sizes, 3, root);
let in_circuit = circuit_hash_words(&sizes, 3, root);
let host_hash = compute_circuit_hash_host(&sizes, 3, &HashValue::<QM31>::from(root));
let host: [u32; BLAKE2S_DIGEST_N_WORDS] = std::array::from_fn(|i| {
u32::from_le_bytes(host_hash.0[i * 4..i * 4 + 4].try_into().unwrap())
});
// The host twin must match an independent reference implementation, and the in-circuit
// builder must match the host twin (the invariant the Fiat-Shamir transcript relies on).
assert_eq!(host, expected, "host twin must match the independent reference");
assert_eq!(in_circuit, host, "in-circuit builder must match the host twin");
}
}crates/circuit_verifier/src/circuit_hash.rs line 16 at r2 (raw file):
// `config_bytes` is packed 4 per u32 with no padding, so its length must be a multiple of 4. const _: () = assert!(CONFIG_N_BYTES.is_multiple_of(4), "CONFIG_N_BYTES must pack into whole u32 words");
What do we do if we'd need another component? Or one less?
Code quote:
// `config_bytes` is packed 4 per u32 with no padding, so its length must be a multiple of 4.
const _: () =
assert!(CONFIG_N_BYTES.is_multiple_of(4), "CONFIG_N_BYTES must pack into whole u32 words");crates/circuit_verifier/src/circuit_hash.rs line 20 at r2 (raw file):
/// Packs `log_blowup_factor` (byte 0) then each component's preprocessed log size (in /// [`COMPONENT_NAMES`] order), one byte each, into little-endian u32 words. fn config_words(
Should we also put min lifting lof size?
crates/circuit_verifier/src/circuit_hash.rs line 32 at r2 (raw file):
let size = *component_log_sizes.get(COMPONENT_NAMES[i - 1]).unwrap(); u8::try_from(size).unwrap_or_else(|_| panic!("log_size {size} does not fit in a byte")) });
Why not this way?
Suggestion:
let config_bytes: [u8; CONFIG_N_BYTES] = std::array::from_fn(|i| {
if i == 0 {
u8::try_from(log_blowup_factor).unwrap_or_else(|_| panic!("log_blowup_factor does not fit in a byte"))
}
let size = *component_log_sizes.get(COMPONENT_NAMES[i - 1]).unwrap();
u8::try_from(size).unwrap_or_else(|_| panic!("log_size {size} does not fit in a byte"))
});crates/circuit_verifier/src/circuit_hash.rs line 48 at r2 (raw file):
.into_iter() .map(|word| U32Wrapper::new_unsafe(context.constant(QM31::pack_u32(word)))) .collect();
Why not using pack_u32s, so the constants created are of the form [word0, word1, word2, word3] instead of [word0, 0, 0, 0] and n_bytes = config_words.len() + 4*BLAKE2S_DIGEST_N_WORDS?
Code quote:
let config_words: Vec<U32Wrapper<Var>> = config_words(component_log_sizes, log_blowup_factor)
.into_iter()
.map(|word| U32Wrapper::new_unsafe(context.constant(QM31::pack_u32(word))))
.collect();crates/circuit_verifier/src/circuit_hash.rs line 70 at r2 (raw file):
} hasher.finalize() }
Why is it here? Should be in prover?
Code quote:
/// Host (out-of-circuit) twin of [`compute_circuit_hash`]. Used by the prover to mix the circuit
/// hash into the channel.
pub fn compute_circuit_hash_host(
component_log_sizes: &OrderedHashMap<&'static str, u32>,
log_blowup_factor: u32,
preprocessed_root: &HashValue<QM31>,
) -> Blake2sHash {
let config_words = config_words(component_log_sizes, log_blowup_factor);
let root_words = preprocessed_root.iter().map(|word| word.get().unpack_u32());
let mut hasher = Blake2sHasher::new();
for word in config_words.into_iter().chain(root_words) {
hasher.update(&word.to_le_bytes());
}
hasher.finalize()
}crates/circuit_verifier/src/circuit_hash.rs line 138 at r2 (raw file):
// builder must match the host twin (the invariant the Fiat-Shamir transcript relies on). assert_eq!(host, expected, "host twin must match the independent reference"); assert_eq!(in_circuit, host, "in-circuit builder must match the host twin");
Doesn't it make more sense to compare in_circuit to expected and then host to expected? Maybe in 2 different tests?
Code quote:
// The host twin must match an independent reference implementation, and the in-circuit
// builder must match the host twin (the invariant the Fiat-Shamir transcript relies on).
assert_eq!(host, expected, "host twin must match the independent reference");
assert_eq!(in_circuit, host, "in-circuit builder must match the host twin");|
Previously, Gali-StarkWare wrote…
No, the evaluation size is determined by max(component_log_sizes) + log_blowup_factor |
|
Previously, Gali-StarkWare wrote…
we will have the change the way the the circuit_hash is defined. |
|
Previously, Gali-StarkWare wrote…
The Rust Verifier also needs it. |
|
Previously, Gali-StarkWare wrote…
"Doesn't it make more sense to compare in_circuit to expected and then host to expected" "Maybe in 2 different tests?" |
|
Previously, Gali-StarkWare wrote…
pack_u32 splits each u32 into 2 u16s: This is the input format for blake2s_u32s. |
7300b3c to
209de23
Compare
|
Previously, Gali-StarkWare wrote…
What is the advantage? |
|
Previously, Gali-StarkWare wrote…
Done. |
ilyalesokhin-starkware
left a comment
There was a problem hiding this comment.
@ilyalesokhin-starkware resolved 1 discussion.
Reviewable status: 3 of 5 files reviewed, 6 unresolved discussions (waiting on Gali-StarkWare).
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 2 files and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ilyalesokhin-starkware).
crates/circuit_verifier/src/circuit_hash.rs line 32 at r2 (raw file):
Previously, ilyalesokhin-starkware wrote…
What is the advantage?
What is the advantage this way? I think it looks cleaner. The way it is right now looks like it's being used somewhere else in the func, and it's strange that the bytes are created differently
crates/circuit_verifier/src/circuit_hash.rs line 70 at r2 (raw file):
Previously, ilyalesokhin-starkware wrote…
The Rust Verifier also needs it.
What rust verifier?
|
Previously, Gali-StarkWare wrote…
|
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ilyalesokhin-starkware).
crates/circuit_verifier/src/circuit_hash.rs line 70 at r2 (raw file):
Previously, ilyalesokhin-starkware wrote…
Which is in the prover's crate :)
|
Previously, Gali-StarkWare wrote…
It makes the lambda simpler, and the lambda is complicated as is. |
ilyalesokhin-starkware
left a comment
There was a problem hiding this comment.
@ilyalesokhin-starkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Gali-StarkWare).
crates/circuit_verifier/src/circuit_hash.rs line 70 at r2 (raw file):
Previously, Gali-StarkWare wrote…
Which is in the prover's crate :)
Quite surprising but ok.
209de23 to
5caa814
Compare
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 3 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on az-starkware and ilyalesokhin-starkware).
crates/circuit_verifier/src/circuit_hash.rs line 70 at r2 (raw file):
Previously, ilyalesokhin-starkware wrote…
Quite surprising but ok.
You didn't need it there?
crates/circuit_verifier/src/circuit_hash.rs line 23 at r4 (raw file):
/// Packs `log_blowup_factor` (byte 0) then each component's preprocessed log size (in /// [`COMPONENT_NAMES`] order), one byte each, into little-endian u32 words. Shared by the /// in-circuit builder here and the host twin in the prover crate so both hash the same preimage.
I don't think we need this extra sentence, the prover can use a lot of the verifier's code
Suggestion:
/// Packs `log_blowup_factor` (byte 0) then each component's preprocessed log size (in
/// [`COMPONENT_NAMES`] order), one byte each, into little-endian u32 words.
az-starkware
left a comment
There was a problem hiding this comment.
@az-starkware reviewed all commit messages and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ilyalesokhin-starkware).
crates/circuit_verifier/src/circuit_hash.rs line 37 at r4 (raw file):
u8::try_from(size).unwrap_or_else(|_| panic!("log_size {size} does not fit in a byte")) }); std::array::from_fn(|i| u32::from_le_bytes(config_bytes[i * 4..i * 4 + 4].try_into().unwrap()))
Use le_u32s_from_bytes
az-starkware
left a comment
There was a problem hiding this comment.
@az-starkware reviewed 5 files and made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ilyalesokhin-starkware).
crates/circuit_verifier/src/circuit_hash_test.rs line 25 at r4 (raw file):
.extend(COMPONENT_NAMES.iter().map(|name| *component_log_sizes.get(*name).unwrap() as u8)); let config_words = config_bytes.chunks_exact(4).map(|b| u32::from_le_bytes(b.try_into().unwrap()));
This is mostly a copy of config_words() from above. Let's use that function instead.
If you want to test config_words() you can add a regression test (though I think it can also be left untested).
Code quote:
let mut config_bytes = vec![log_blowup_factor as u8];
config_bytes
.extend(COMPONENT_NAMES.iter().map(|name| *component_log_sizes.get(*name).unwrap() as u8));
let config_words =
config_bytes.chunks_exact(4).map(|b| u32::from_le_bytes(b.try_into().unwrap()));5caa814 to
91d8a77
Compare
ilyalesokhin-starkware
left a comment
There was a problem hiding this comment.
@ilyalesokhin-starkware made 4 comments and resolved 1 discussion.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on az-starkware and Gali-StarkWare).
crates/circuit_verifier/src/circuit_hash.rs line 70 at r2 (raw file):
Previously, Gali-StarkWare wrote…
You didn't need it there?
I didn't need it in this PR.
I've added it here earlier as I thought it was part of this file.
crates/circuit_verifier/src/circuit_hash.rs line 23 at r4 (raw file):
Previously, Gali-StarkWare wrote…
I don't think we need this extra sentence, the prover can use a lot of the verifier's code
done
crates/circuit_verifier/src/circuit_hash.rs line 37 at r4 (raw file):
Previously, az-starkware wrote…
Use
le_u32s_from_bytes
done
crates/circuit_verifier/src/circuit_hash_test.rs line 25 at r4 (raw file):
Previously, az-starkware wrote…
This is mostly a copy of
config_words()from above. Let's use that function instead.
If you want to testconfig_words()you can add a regression test (though I think it can also be left untested).
Done.
az-starkware
left a comment
There was a problem hiding this comment.
@az-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ilyalesokhin-starkware).
91d8a77 to
883e83d
Compare
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ilyalesokhin-starkware).
crates/circuit_verifier/src/circuit_hash_test.rs line 23 at r5 (raw file):
let mut hasher = Blake2s256::new(); for word in config_words(component_log_sizes, log_blowup_factor).into_iter().chain(preprocessed_root)
Why did you change the order?
Code quote:
config_words(component_log_sizes, log_blowup_factor).into_iter().chain(preprocessed_root)
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 1 file.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ilyalesokhin-starkware).
|
Previously, Gali-StarkWare wrote…
I guess you are referring to the order of the arguments to config_words. changed it to be consistent with the actual hashing order. |
Add a `circuit_hash` module with `compute_circuit_hash`, which builds in-circuit the blake2s hash of the packed circuit config (`log_blowup_factor` + per-component preprocessed log sizes) concatenated with the preprocessed root — the value that uniquely identifies a circuit. This mirrors the multiverifier's inline circuit-hash computation and lives in `circuit_verifier` so it can be shared by the circuit statement and the multiverifier. A unit test cross-checks the in-circuit result against an independent host `blake2s` reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
883e83d to
2350bb2
Compare
ilyalesokhin-starkware
left a comment
There was a problem hiding this comment.
@ilyalesokhin-starkware reviewed 5 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Gali-StarkWare).
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ilyalesokhin-starkware).

Add a
circuit_hashmodule withcompute_circuit_hash, which buildsin-circuit the blake2s hash of the packed circuit config
(
log_blowup_factor+ per-component preprocessed log sizes) concatenatedwith the preprocessed root — the value that uniquely identifies a circuit.
This mirrors the multiverifier's inline circuit-hash computation and lives
in
circuit_verifierso it can be shared by the circuit statement and themultiverifier. A unit test cross-checks the in-circuit result against an
independent host
blake2sreference.Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com