Add num_instances to transcript #648

emmorais · 2024-11-27T19:24:35Z

Each opcode has its own transcript after forking. This PR includes num_instances for each opcode as the first element of each transcript (after forking). This way it is not possible to fold instances arbitrarily.

naure · 2024-11-28T12:21:06Z

ceno_zkvm/src/scheme/prover.rs

@@ -150,6 +150,7 @@ impl<E: ExtensionField, PCS: PolynomialCommitmentScheme<E>> ZKVMProver<E, PCS> {
                for lk_s in cs.lk_expressions_namespace_map.iter() {
                    tracing::debug!("opcode circuit {}: {}", circuit_name, lk_s);
                }
+                transcript.append_field_element(&E::BaseField::from(num_instances as u64));


Assuming it is line 131 above that does the skipping of empty circuits:

if witness.is_empty() { continue;

Then to say "there is an empty circuit" the num_instances (potentially 0) should be written before that if.

Thanks for the comment. Now this problem is addressed in the expected way, right after forking the transcripts and including zeros.

naure · 2024-11-28T12:25:03Z

ceno_zkvm/src/scheme/verifier.rs

@@ -148,6 +149,7 @@ impl<E: ExtensionField, PCS: PolynomialCommitmentScheme<E>> ZKVMVerifier<E, PCS>
                .circuit_vks
                .get(&name)
                .ok_or(ZKVMError::VKNotFound(name.clone()))?;
+            transcript.append_field_element(&E::BaseField::from(num_instances as u64));


In this loop there will be only non-empty circuits. To catch all circuits the self.vk should be involved.

The verifier now is basically doing the same as the prover, using the information included in the proofs, mimicking the same updates in the corresponding transcripts.

matthiasgoergens · 2024-12-04T11:24:51Z

ceno_zkvm/src/scheme/verifier.rs

@@ -117,7 +117,7 @@ impl<E: ExtensionField, PCS: PolynomialCommitmentScheme<E>> ZKVMVerifier<E, PCS>
            }
        }

-        for (name, (_, proof)) in vm_proof.opcode_proofs.iter() {
+        for (name, (i, proof)) in vm_proof.opcode_proofs.iter() {


Why the change here? You don't use the variable i, do you?

matthiasgoergens · 2024-12-04T11:26:51Z

tracing.folded

You probably want to put the name of this file into the .gitignore?

matthiasgoergens · 2024-12-04T11:30:06Z

ceno_zkvm/src/scheme/verifier.rs

+            .vk
+            .circuit_vks
+            .iter() // Sorted by key.
+            .zip_eq(transcripts.iter_mut().enumerate())


It's probably less confusing to do the enumerate once on the outside of the zip:

diff --git a/ceno_zkvm/src/scheme/verifier.rs b/ceno_zkvm/src/scheme/verifier.rs index 52c11a2c..d6b3e41a 100644 --- a/ceno_zkvm/src/scheme/verifier.rs +++ b/ceno_zkvm/src/scheme/verifier.rs @@ -4,7 +4,7 @@ use ark_std::iterable::Iterable; use ceno_emul::WORD_SIZE; use ff_ext::ExtensionField; -use itertools::{Itertools, interleave, izip}; +use itertools::{Itertools, enumerate, interleave, izip}; use mpcs::PolynomialCommitmentScheme; use multilinear_extensions::{ mle::{IntoMLE, MultilinearExtension}, @@ -142,12 +142,8 @@ impl<E: ExtensionField, PCS: PolynomialCommitmentScheme<E>> ZKVMVerifier<E, PCS> // For each opcode, include the num_instances // into its corresponding fork of the transcript. - for ((name, _), (i, transcript)) in self - .vk - .circuit_vks - .iter() // Sorted by key. - .zip_eq(transcripts.iter_mut().enumerate()) - { + // Note: circuits are sorted by key. + for (i, ((name, _), transcript)) in enumerate(izip!(self.vk.circuit_vks, transcripts)) { // get num_instances from opcode proof let opcode_result = vm_proof.opcode_proofs.get(name); let num_instances = opcode_result.map(|(_, p)| p.num_instances).unwrap_or(0);

Enumeration is not needed just for updating the transcripts, so I removed it.

Just a remark: without "iter_mut()" the compiler asks to clone the transcripts, which is not what we want, right? So maybe I am missing something from your suggestion here. Any way, since the enumeration was removed, this discussion may be useless now. But I appreciate your comment, thank you.

matthiasgoergens · 2024-12-04T11:31:20Z

ceno_zkvm/src/scheme/prover.rs

@@ -92,6 +92,7 @@ impl<E: ExtensionField, PCS: PolynomialCommitmentScheme<E>> ZKVMProver<E, PCS> {
        // commit to opcode circuits first and then commit to table circuits, sorted by name
        for (circuit_name, witness) in witnesses.into_iter_sorted() {
            let num_instances = witness.num_instances();
+            //transcript.append_field_element(&E::BaseField::from(num_instances as u64));


Please remove this from the final PR.

…scroll-tech/ceno into add_num_instances_to_transcript

naure · 2024-12-06T12:38:45Z

ceno_zkvm/src/scheme/prover.rs

            // TODO: add an enum for circuit type either in constraint_system or vk
            let cs = pk.get_cs();
            let is_opcode_circuit = cs.lk_table_expressions.is_empty()
                && cs.r_table_expressions.is_empty()
                && cs.w_table_expressions.is_empty();
+            if is_opcode_circuit {


Some of the table circuits may also be skipped. Is there a blocker that requires this if?

Although it is a little less obvious how the num_instances of tables is communicated. Look at the verifier variable expected_rounds. Maybe we can do the table case somehow in another PR.

naure · 2024-12-06T12:55:23Z

ceno_zkvm/src/scheme/verifier.rs

+            // get num_instances from opcode proof
+            let opcode_result = vm_proof.opcode_proofs.get(name);
+            let num_instances = opcode_result.map(|(_, p)| p.num_instances).unwrap_or(0);
+            if opcode_result.is_some() {


This is supposed to be the same as if is_opcode_circuit in the prover side, but I’m not sure that’s the case. Looking at vm_proof.opcode_proofs.insert which ultimately becomes opcode_result.is_some(), not the same as is_opcode_circuit?

naure · 2024-12-06T13:10:11Z

ceno_zkvm/src/scheme/verifier.rs

+            let opcode_result = vm_proof.opcode_proofs.get(name);
+            let num_instances = opcode_result.map(|(_, p)| p.num_instances).unwrap_or(0);
+            if opcode_result.is_some() {
+                transcript.append_field_element(&E::BaseField::from(num_instances as u64));


It would protect against more potential attacks to do this before the alpha, beta read_challenge above. It’s before the transcript fork, so just a sequence of append to the root transcript.

Add num_instances to transcript

6982631

emmorais changed the title ~~Add num_instances to transcript~~ [WIP] Add num_instances to transcript Nov 27, 2024

emmorais requested a review from naure November 27, 2024 19:36

naure reviewed Nov 28, 2024

View reviewed changes

naure linked an issue Nov 28, 2024 that may be closed by this pull request

Remove free choice of circuits #542

Open

emmorais added 2 commits December 3, 2024 16:43

Add num_instances as first element after fork, including zeros

6fe4dea

Update comments

6e003dc

emmorais self-assigned this Dec 4, 2024

matthiasgoergens reviewed Dec 4, 2024

View reviewed changes

emmorais and others added 5 commits December 4, 2024 09:00

Minor adjustments

2eb7281

Remove enumeration from loop

b16e13f

Delete tracing.folded

6352f30

Merge branch 'master' into add_num_instances_to_transcript

8595ffa

Merge branch 'add_num_instances_to_transcript' of https://github.com/…

0162f86

…scroll-tech/ceno into add_num_instances_to_transcript

emmorais marked this pull request as ready for review December 4, 2024 12:55

emmorais changed the title ~~[WIP] Add num_instances to transcript~~ Add num_instances to transcript Dec 4, 2024

emmorais and others added 3 commits December 4, 2024 09:58

Fmt

54564d6

Clippy

373bc65

Merge branch 'master' into add_num_instances_to_transcript

c809571

naure reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add num_instances to transcript #648

Add num_instances to transcript #648

emmorais commented Nov 27, 2024 •

edited

Loading

naure Nov 28, 2024

emmorais Dec 4, 2024

naure Nov 28, 2024

emmorais Dec 4, 2024

matthiasgoergens Dec 4, 2024

emmorais Dec 4, 2024

matthiasgoergens Dec 4, 2024

emmorais Dec 4, 2024

matthiasgoergens Dec 4, 2024

emmorais Dec 4, 2024

matthiasgoergens Dec 4, 2024

emmorais Dec 4, 2024

naure Dec 6, 2024

naure Dec 6, 2024

naure Dec 6, 2024

naure Dec 6, 2024

Add num_instances to transcript #648

Are you sure you want to change the base?

Add num_instances to transcript #648

Conversation

emmorais commented Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmorais commented Nov 27, 2024 •

edited

Loading