Repository navigation
feat(circuit-verifier): mix circuit hash into the channel. - #648
Conversation
PR SummaryHigh Risk Overview The prover adds a host Multiverifier Reviewed by Cursor Bugbot for commit c62bd7d. 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. |
65ad54b to
8432a9b
Compare
7195e8e to
f22b909
Compare
8432a9b to
c8b446c
Compare
|
I think that it is a good practice. |
c8b446c to
84d68a8
Compare
f22b909 to
c2b6759
Compare
84d68a8 to
5f36cf9
Compare
yuvalsw
left a comment
There was a problem hiding this comment.
@yuvalsw reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on Gali-StarkWare and ilyalesokhin-starkware).
-- commits line 13 at r3:
what is out-of-circuit verifier?
"Regenerate the affected multiverifier and privacy proof fixtures" - redundant.
Add the host twin compute_circuit_hash_host and replace the
CircuitClaim::mix_into method with a free mix_public_inputs function shared by
the prover and out-of-circuit verifier.
Aren't these refactors that should be split into an earlier commit?
Code quote:
- 5f36cf9: feat(circuit-verifier): mix circuit hash into the channel.
Mix blake2s(config_words || preprocessed_root) into the channel before the
output values, on both the prover (mix_public_inputs) and the in-circuit
verifier (CircuitStatement::claims_to_mix), so the transcript commits to the
circuit's identity. Add the host twin compute_circuit_hash_host and replace the
CircuitClaim::mix_into method with a free mix_public_inputs function shared by
the prover and out-of-circuit verifier.
Regenerate the affected multiverifier and privacy proof fixtures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previously, yuvalsw wrote…
|
c2b6759 to
7300b3c
Compare
5f36cf9 to
06d9a70
Compare
06d9a70 to
c38aad3
Compare
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 7 files and all commit messages, and made 2 comments.
Reviewable status: 7 of 12 files reviewed, 4 unresolved discussions (waiting on ilyalesokhin-starkware and yuvalsw).
crates/circuit_multiverifier/src/backward_compatibility_test.rs line 29 at r5 (raw file):
/// An older Cairo verifier proof, produced at commit f5cd0f7a (that version's `proof_cairo.bin`). const BACKWARD_COMPATIBILITY_CAIRO_PROOF_PATH: &str = concat!( env!("CARGO_MANIFEST_DIR"),
Commit id?
Code quote:
/// The preprocessed root of the Cairo verifier circuit as it was at commit f5cd0f7a.
const BACKWARD_COMPATIBILITY_CAIRO_VERIFIER_PREPROCESSED_ROOT: [u32; 8] =
[695775592, 831947430, 3864682957, 1778749033, 1073148880, 3248306553, 1968525874, 3767461582];
/// The output values of the Cairo verifier circuit as it was at commit f5cd0f7a.
/// TODO(ilya): consider removing this and using the output values from the current Cairo verifier
/// proof when breaking backwards compatibility.
const BACKWARD_COMPATIBILITY_CAIRO_VERIFIER_OUTPUT_VALUES: [u32; 8] =
[3035180123, 3555538090, 587798257, 1881776298, 3385462846, 2102605012, 3369268656, 403460632];
/// An older Cairo verifier proof, produced at commit f5cd0f7a (that version's `proof_cairo.bin`).
const BACKWARD_COMPATIBILITY_CAIRO_PROOF_PATH: &str = concat!(
env!("CARGO_MANIFEST_DIR"),
"/../../test_data/circuit_multiverifier/backward_compatibility_cairo_proof.bin"
);crates/circuit_multiverifier/src/backward_compatibility_test.rs line 34 at r5 (raw file):
/// /// Checks that the multiverifier can verify, in the same circuit, both a Cairo verifier proof in /// slot 0 and a multiverifier proof in slot 1.
Why the doc change?
Code quote:
/// Multi-input regression test.
///
/// Checks that the multiverifier can verify, in the same circuit, both a Cairo verifier proof in
/// slot 0 and a multiverifier proof in slot 1.|
Previously, Gali-StarkWare wrote…
I fixed the proof to be the one from this commit, so I can't put it in the comment. |
|
Previously, Gali-StarkWare wrote…
fixed. |
ilyalesokhin-starkware
left a comment
There was a problem hiding this comment.
@ilyalesokhin-starkware made 1 comment and resolved 1 discussion.
Reviewable status: 7 of 12 files reviewed, 4 unresolved discussions (waiting on Gali-StarkWare and yuvalsw).
test_data/circuit_multiverifier/backward_compatibility_cairo_proof.bin at r4 (raw file):
Proof has been
c38aad3 to
2455154
Compare
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare partially reviewed 5 files and made 4 comments.
Reviewable status: 11 of 12 files reviewed, 8 unresolved discussions (waiting on ilyalesokhin-starkware and yuvalsw).
crates/circuit_prover/src/prover.rs line 149 at r5 (raw file):
// Preprocessed trace. commitment_scheme.commit_tree(preprocessed_tree, channel);
Suggestion:
// Preprocessed trace.
let preprocessed_root: HashValue<QM31> = preprocessed_tree.commitment.root().into();
commitment_scheme.commit_tree(preprocessed_tree, channel);crates/circuit_prover/src/prover.rs line 167 at r5 (raw file):
&preprocessed_root, ); mix_public_inputs(channel, &claim, &circuit_hash);
I think that's a bit weird, no?
Code quote:
let circuit_hash = compute_circuit_hash_host(
&component_log_sizes,
pcs_config.fri_config.log_blowup_factor,
&preprocessed_root,
);
mix_public_inputs(channel, &claim, &circuit_hash);crates/circuit_verifier/src/circuit_claim.rs line 31 at r5 (raw file):
claim: &CircuitClaim, circuit_hash: &Blake2sHash, ) {
why not just add a params to the mix_into func?
Suggestion:
/// Mixes the circuit's public inputs into the channel.
pub fn mix_into(
&self
channel: &mut impl Channel,
circuit_hash: &Blake2sHash,
) {crates/circuit_verifier/src/statement.rs line 110 at r5 (raw file):
let circuit_hash_words = self.circuit_hash.iter().copied().collect_vec(); let output_words = unpack_qm31s_to_u32_words(context, self.output_values.iter().copied()); vec![circuit_hash_words, output_words]
Keep the comments
Code quote:
let circuit_hash_words = self.circuit_hash.iter().copied().collect_vec();
let output_words = unpack_qm31s_to_u32_words(context, self.output_values.iter().copied());
vec![circuit_hash_words, output_words]|
Previously, Gali-StarkWare wrote…
What is weird about it? |
|
Previously, Gali-StarkWare wrote…
I think that making it a member function of the claim is weird. |
|
Previously, Gali-StarkWare wrote…
What information does it add? |
5caa814 to
883e83d
Compare
ab70f86 to
4e5cd7b
Compare
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 5 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on yuvalsw).
883e83d to
2350bb2
Compare
4e5cd7b to
13d60e4
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 13d60e4. Configure here.
13d60e4 to
7218985
Compare
Gali-StarkWare
left a comment
There was a problem hiding this comment.
@Gali-StarkWare reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on yuvalsw).
2350bb2 to
bad69a9
Compare
7218985 to
025b8fc
Compare
025b8fc to
e01e516
Compare
yuvalsw
left a comment
There was a problem hiding this comment.
@yuvalsw made 1 comment and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ilyalesokhin-starkware).
Previously, ilyalesokhin-starkware wrote…
I don't consider adding an unused function as a refactor.
discussed f2f. it's not a refactor. resolving.
Gali-StarkWare
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on ilyalesokhin-starkware).
e01e516 to
c1a77f0
Compare
Mix blake2s(config_words || preprocessed_root) into the channel before the output values. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c1a77f0 to
c62bd7d
Compare
OmriEshhar1
left a comment
There was a problem hiding this comment.
@OmriEshhar1 reviewed 14 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on yuvalsw).
OmriEshhar1
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on yuvalsw).
OmriEshhar1
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on yuvalsw).
yuvalsw
left a comment
There was a problem hiding this comment.
.
@yuvalsw made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ilyalesokhin-starkware).
yuvalsw
left a comment
There was a problem hiding this comment.
@yuvalsw reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ilyalesokhin-starkware).
yuvalsw
left a comment
There was a problem hiding this comment.
.
@yuvalsw made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ilyalesokhin-starkware).
yuvalsw
left a comment
There was a problem hiding this comment.
.
@yuvalsw made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ilyalesokhin-starkware).


Mix blake2s(config_words || preprocessed_root) into the channel before the
output values, on both the prover (mix_public_inputs) and the in-circuit
verifier (CircuitStatement::claims_to_mix), so the transcript commits to the
circuit's identity. Add the host twin compute_circuit_hash_host and replace the
CircuitClaim::mix_into method with a free mix_public_inputs function shared by
the prover and out-of-circuit verifier.
Regenerate the affected multiverifier and privacy proof fixtures.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com