Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
db30738 to
a0eb6da
Compare
Stavbe
left a comment
There was a problem hiding this comment.
Required changes for the integration:
-
Update the builtin names in
BuiltinSegmentsto match their names inair-infra. -
Add
#[derive(Default)]toPublicData,MemorySmallValue,SegmentRange,PublicSegmentRanges, andPublicMemoryfor use byCairoClaimGenerator::default. -
Add a
RANGE_CHECK96_MEMORY_CELLSconstant for cleaner and more unified code in the codegen. -
Remove
BuiltinsClaimGenerator,OpcodesClaimGenerator,BlakeContextClaimGenerator,PedersenContextClaimGenerator,RangeChecksClaimGenerator, andPoseidonContextClaimGenerator, and update theirwrite_tracefunctions to construct the existing claim.
@Stavbe made 1 comment.
Reviewable status: 0 of 20 files reviewed, all discussions resolved.
a0eb6da to
5e7218e
Compare
Stavbe
left a comment
There was a problem hiding this comment.
- Clone the memory in the BlakeRound::ClaimGenerator::new ()
@Stavbe made 1 comment.
Reviewable status: 0 of 22 files reviewed, all discussions resolved.
anatgstarkware
left a comment
There was a problem hiding this comment.
It looks great!
@anatgstarkware reviewed 16 files and made 3 comments.
Reviewable status: 16 of 22 files reviewed, 2 unresolved discussions (waiting on @Stavbe).
stwo_cairo_prover/crates/prover/src/witness/cairo.rs line 200 at r2 (raw file):
}; cairo_claim_generator.public_data = public_data;
I think that if you call fill_cairo_claim_generator here, you can pass the memory, and you don't need to clone it
Suggestion:
let mut cairo_claim_generator = CairoClaimGenerator {
public_data;
..
}
cairo_claim_generator.fill_components(&all_components, )stwo_cairo_prover/crates/prover/src/witness/opcodes.rs line 17 at r2 (raw file):
use crate::witness::utils::TreeBuilder; pub fn get_opcodes(casm_states_by_opcodes: &CasmStatesByOpcode) -> Vec<&'static str> {
Suggestion:
casm_states_by_opcode5e7218e to
d415be9
Compare
Stavbe
left a comment
There was a problem hiding this comment.
@Stavbe made 3 comments.
Reviewable status: 8 of 25 files reviewed, 3 unresolved discussions (waiting on @anatgstarkware).
stwo_cairo_prover/crates/prover/src/witness/cairo.rs line 200 at r2 (raw file):
Previously, anatgstarkware (anatg) wrote…
I think that if you call
fill_cairo_claim_generatorhere, you can pass the memory, and you don't need to clone it
Done. Note that I needed the loop on the memory after the fill_components, that's why I took it down.
Moreover, the problem with cloning the memory was inside the fill_components function (because Blake Round consumed it), so it doesn't affect that (regardless, I moved the clone inside the Blake Round for a cleaner auto-gen, as we discussed).
stwo_cairo_prover/crates/prover/src/witness/opcodes.rs line 17 at r2 (raw file):
use crate::witness::utils::TreeBuilder; pub fn get_opcodes(casm_states_by_opcodes: &CasmStatesByOpcode) -> Vec<&'static str> {
Done.
stwo_cairo_prover/crates/common/src/builtins.rs line 1 at r3 (raw file):
pub const ADD_MOD_MEMORY_CELLS: usize = 7;
I renamed this file to builtins_constant
anatgstarkware
left a comment
There was a problem hiding this comment.
@anatgstarkware reviewed 12 files and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: 20 of 25 files reviewed, 4 unresolved discussions (waiting on @Stavbe).
stwo_cairo_prover/crates/common/src/builtins.rs line 1 at r3 (raw file):
Previously, Stavbe wrote…
I renamed this file to builtins_constant
why? this is a very big pr, please don't add unrelated changes
stwo_cairo_prover/crates/prover/src/witness/cairo.rs line 200 at r2 (raw file):
Previously, Stavbe wrote…
Done. Note that I needed the loop on the memory after the fill_components, that's why I took it down.
Moreover, the problem with cloning the memory was inside the fill_components function (because Blake Round consumed it), so it doesn't affect that (regardless, I moved the clone inside the Blake Round for a cleaner auto-gen, as we discussed).
I don't understand why we can't fill the memory tables before fill_components?
stwo_cairo_prover/crates/prover/Cargo.toml line 38 at r3 (raw file):
tracing.workspace = true dashmap.workspace = true indexmap = "2.2.6"
please add to the workspace toml
stwo_cairo_prover/crates/prover/src/witness/cairo_claim_generator.rs line 95 at r3 (raw file):
self.blake_compress_opcode = components.contains(&"blake_compress_opcode").then(|| { blake_compress_opcode::ClaimGenerator::new( casm_states_by_opcode.blake_compress_opcode.clone(),
My mistake, sorry, we shouldn't pass CasmStatesByOpcode by reference just to clone it here
Code quote:
.clone()d415be9 to
4380f02
Compare
Stavbe
left a comment
There was a problem hiding this comment.
@Stavbe made 2 comments and resolved 1 discussion.
Reviewable status: 10 of 26 files reviewed, 3 unresolved discussions (waiting on @anatgstarkware).
stwo_cairo_prover/crates/prover/Cargo.toml line 38 at r3 (raw file):
Previously, anatgstarkware (anatg) wrote…
please add to the workspace toml
Done.
stwo_cairo_prover/crates/prover/src/witness/cairo_claim_generator.rs line 95 at r3 (raw file):
Previously, anatgstarkware (anatg) wrote…
My mistake, sorry, we shouldn't pass
CasmStatesByOpcodeby reference just to clone it here
Done.
anatgstarkware
left a comment
There was a problem hiding this comment.
please rebase
@anatgstarkware partially reviewed 13 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: 23 of 26 files reviewed, all discussions resolved (waiting on @Stavbe).
|
I think you can revert the changes to this file |
4380f02 to
e2f2f88
Compare
Stavbe
left a comment
There was a problem hiding this comment.
Done
@Stavbe made 2 comments.
Reviewable status: 9 of 26 files reviewed, 1 unresolved discussion (waiting on @anatgstarkware and @Stavbe).
stwo_cairo_prover/crates/cairo-air/src/air.rs line 234 at r4 (raw file):
Previously, anatgstarkware (anatg) wrote…
I think you can revert the changes to this file
I thought about that as well, but apparently the ..Default() call initializes the struct first and then overrides the specified fields — so it’s still needed. I could have explicitly set None for all the fields, but I preferred this way.
anatgstarkware
left a comment
There was a problem hiding this comment.
@anatgstarkware reviewed 14 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: 23 of 26 files reviewed, 1 unresolved discussion (waiting on @Stavbe).
stwo_cairo_prover/crates/prover/src/witness/components/poseidon.rs line 56 at r5 (raw file):
.expect("Should have poseidon round keys trace generator at this point"); let mut range_check_252_width_27_trace_generator = range_check_252_width_27_trace_generator .expect("Should have range check 252 width 27 trace generator at this point");
Why is this different than the rest? Why not just unwrap?
Code quote:
let poseidon_aggregator_trace_generator = poseidon_aggregator_trace_generator
.expect("Should have poseidon aggregator trace generator at this point");
let mut poseidon_3_partial_rounds_chain_trace_generator =
poseidon_3_partial_rounds_chain_trace_generator
.expect("Should have poseidon 3 partial rounds chain trace generator at this point");
let mut poseidon_full_round_chain_trace_generator = poseidon_full_round_chain_trace_generator
.expect("Should have poseidon full round chain trace generator at this point");
let mut cube_252_trace_generator =
cube_252_trace_generator.expect("Should have cube 252 trace generator at this point");
let poseidon_round_keys_trace_generator = poseidon_round_keys_trace_generator
.expect("Should have poseidon round keys trace generator at this point");
let mut range_check_252_width_27_trace_generator = range_check_252_width_27_trace_generator
.expect("Should have range check 252 width 27 trace generator at this point");
anatgstarkware
left a comment
There was a problem hiding this comment.
@anatgstarkware reviewed 3 files and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Stavbe).
anatgstarkware
left a comment
There was a problem hiding this comment.
@anatgstarkware resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Stavbe).

This change is