Add num_instances to transcript by emmorais · Pull Request #648 · scroll-tech/ceno

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

                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

                .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

        }

-        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

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

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

+            .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

        // 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

            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

+            // 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

+            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.

hero78119 · 2025-04-09T10:25:39Z

Just a memo: #901 addressed this issue in more unified way

hero78119 · 2025-04-17T11:33:07Z

Done via #894

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

hero78119 mentioned this pull request Mar 27, 2025

improve PIOP implementation soundness via spongefish style alternative in plonky3 #884

Open

hero78119 mentioned this pull request Apr 9, 2025

control num_instances in one place to improve soundness check #901

Closed

hero78119 closed this Apr 17, 2025

Conversation

emmorais commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hero78119 commented Apr 9, 2025

Uh oh!

hero78119 commented Apr 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emmorais commented Nov 27, 2024 •

edited

Loading