-
Notifications
You must be signed in to change notification settings - Fork 36
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.
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
Enables optional verification of Keccak tables #657
base: sai/conditionally_verify_in_root_circuit
Are you sure you want to change the base?
Enables optional verification of Keccak tables #657
Conversation
…le_keccak_optional_proving
…le_keccak_optional_proving
34c9bad
to
ab2ec50
Compare
…le_keccak_optional_proving
…le_keccak_optional_proving
…le_keccak_optional_proving
…le_keccak_optional_proving
self.root.index_verifier_data[table], | ||
F::from_canonical_usize(*index_verifier_data), | ||
); | ||
let common_date = &table_circuits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let common_date = &table_circuits | |
let common_data = &table_circuits |
@@ -486,7 +486,11 @@ fn get_all_memory_address_and_values(memory_before: &MemoryState) -> Vec<(Memory | |||
res | |||
} | |||
|
|||
type TablesWithPVsAndFinalMem<F> = ([Vec<PolynomialValues<F>>; NUM_TABLES], PublicValues<F>); | |||
pub struct TablesWithPVsAndFinalMem<F: RichField> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: out of curiosity: why AndFinalMem
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed it. Thanks for the catch!
if KECCAK_TABLES_INDICES.contains(&i) && !use_keccak_tables { | ||
// Observe zero merkle caps when skipping Keccak tables. | ||
let zero_merkle_cap = cap.flatten().iter().map(|_| F::ZERO).collect::<Vec<F>>(); | ||
challenger.observe_elements(&zero_merkle_cap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can they not just be ignored? (as in only observe if it's not a keccak table)?
Also, has a similar change been applied on the verifier side (in get_challenges.rs
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is difficult to implement conditionally observe in root circuits, so I choose observe zero caps instead.
I was originally planning to implement it in the next PR (which requires changes on the Plonky2 side, and get_challenges.rs
is only used for testing). However, I agree with you that we should make this PR complete. I will implement it in this PR instead.
…le_keccak_optional_proving
Co-authored-by: Linda Guiga <[email protected]>
@LindaGuiga Thanks for reviewing! I have synced the zkVM verifier with the prover. In the next PR #690 , I will make the proofs optional in the proof table, but this requires changes on the Plonky2 side. This is also why I’m sending this PR for review before updating Plonky2. |
f61d7c0
to
503085b
Compare
.by_stark_size | ||
.keys() | ||
.min() | ||
.expect("No valid size in shrinking circuits"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it'd be best to return an error early than panicking in these calls
let use_keccak_tables = | ||
!state.traces.keccak_inputs.is_empty() || !state.traces.keccak_sponge_ops.is_empty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can't have one empty and the other non-empty, so no need for the second evaluation
let (keccak_proof, _) = prove_table!(keccak_stark, Table::Keccak); | ||
let (keccak_sponge_proof, _) = prove_table!(keccak_sponge_stark, Table::KeccakSponge); | ||
if !use_keccak_tables { | ||
// We need to connect the challenger state of Logic and CPU tables when the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (for consistency with actual ordering)
// We need to connect the challenger state of Logic and CPU tables when the | |
// We need to connect the challenger state of CPU and Logic tables when the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also would need to probably rework the way we mark empty tables once we include BP / Logic / MemAfter, as we may do multiple hops before finding a non-empty table, and I find the use of multiple use_foo
fields in AllProof
slightly ugly
Enables optional verification of Keccak tables in root circuits, saving unnecessary recursion time for empty Keccak tables. In my tests below, it reduces proving time by around 3 seconds when a segment contains no Keccak operations. #620
RUST_LOG=info /Users/sdeng/.cargo/bin/cargo test --color=always -r --lib fixed_recursive_verifier::tests::test_segment_proof_generation_without_keccak --no-fail-fast --manifest-path /Users/sdeng/Project/0xPolygonZero/zk_evm/evm_arithmetization/Cargo.toml -- --ignored