Feat/structural witin add #740

10to4 · 2024-12-12T07:43:14Z

Work for #654

- Add check for structural witnesses

ceno_zkvm/src/circuit_builder.rs

See https://github.com/scroll-tech/ceno/security/dependabot/3 for why.

Just to avoid falling too far behind. There seem to be no issues with the update.

This PR try to turn `Transcript` into a trait so we can apply different implenents (For example, the `TranscriptSyncronized`) in the future. We try to split the trait into a basic `Transcript` trait and a extended `Forkable` (with the `fork` method being provided) one. The original `Transcript` struct has been renamed into `BasicTranscript`.

Fixes scroll-tech#691. - Refactors e2e into smaller steps. - Introduces a `run_partial` method which takes a `prefix` argument, indicating when to stop the run of the pipeline. According to this argument, different parts of the state are yielded back. This part isn't too pretty type-wise, but ultimately I think it's not dangerous and a reasonable compromise for the moment. Later edit: I've also included a benchmark for witness generation of the Fibonacci program, since it is a consumer of this refactor and helps to exemplify it.

Extracted from scroll-tech#667 to make that PR smaller and easier to review.

…croll-tech#721) This PR has fixed issue scroll-tech#715 by respect the assertion in `from_base` method of the Godilock extension field Notice there is two method `from_base` and `form_limbs` in the `ExtensionField` trait which is not well illustrated and maybe we should update them later. The two running mentioned in scroll-tech#715 have been success under this PR `cargo run --package ceno_zkvm --bin e2e -- --platform=sp1 ceno_zkvm/examples/fibonacci.elf` `cargo run --package ceno_zkvm --example riscv_opcodes -- --start 10 --end 11`

To catch some bugs, like the recent breakage. --------- Co-authored-by: noelwei <[email protected]>

Just use `cargo fmt` or `cargo fmt --all` for the same effect. Especially the `fmt` target was obsolote: why would you want to format specifically only `ceno_zkvm`?

To make the diff for scroll-tech#596 easier to read.

To help make scroll-tech#596 easier to read and reason about. Also introduce a few more conversion helpers.

…ch#723) Features `non_pow2_rayon_thread` and `riv64` are not tested: we can't even compile with them. Because `riv64` is gone, we can make `riv32` invisible: it's effects are now just the default.

Implementing a TODO from scroll-tech#596

…#737) Extracted from scroll-tech#736 for ease of review.

…ceno into feat/structural_witin_add

- cargo fmt check

- Modify create_structural_witin() & create_witin() to improve readability

hero78119

Thanks for the work! most looks neat
left some comment of missing functionality

ceno_zkvm/src/expression.rs

ceno_zkvm/src/tables/ram/ram_impl.rs

ceno_zkvm/src/scheme/verifier.rs

- Remove SetTableAddrType - Add more meta information in StructuralWitIn - Remove from_expr

@10to4

@10to4 Your #740 reminded me of some old improvements I had lying around. Especially the use of the standard library's `join`, instead of implementing the functionality manually.

hero78119

Just left some comments related to core design

ceno_zkvm/src/expression.rs

ceno_zkvm/src/scheme/prover.rs

ceno_zkvm/src/tables/ram/ram_impl.rs

matthiasgoergens · 2024-12-12T08:06:54Z

ceno_zkvm/src/expression.rs

@@ -25,6 +25,9 @@ use crate::{
 pub enum Expression<E: ExtensionField> {
    /// WitIn(Id)
    WitIn(WitnessId),
+    /// StructuralWitIn(WitnessId)
+    /// same as witin, but it's structual witness and be evaluated by verifier succinctly.


Sorry, I have a bit of a hard time understanding what you are trying to say.

Could you please fix the grammar? That might already help.

matthiasgoergens · 2024-12-12T08:08:03Z

ceno_zkvm/src/expression.rs

@@ -71,6 +75,7 @@ impl<E: ExtensionField> Expression<E> {
        &self,
        fixed_in: &impl Fn(&Fixed) -> T,
        wit_in: &impl Fn(WitnessId) -> T, // witin id
+        structural_wit_in: &impl Fn(WitnessId) -> T,


The arguments to this function are getting quite involved, I wonder whether we should make a struct to hold all the arguments.

So far, it doesn't seem to need other arguments yet.

matthiasgoergens · 2024-12-17T09:35:04Z

ceno_zkvm/src/witness.rs

@@ -59,7 +59,11 @@ impl<T: Sized + Sync + Clone + Send + Copy> RowMajorMatrix<T> {
    }

    pub fn num_instances(&self) -> usize {
-        self.values.len() / self.num_col - self.num_padding_rows
+        let mut num = 0;


No need for mutation:

diff --git a/ceno_zkvm/src/witness.rs b/ceno_zkvm/src/witness.rs index 88bfdcf5..47d4f2ee 100644 --- a/ceno_zkvm/src/witness.rs +++ b/ceno_zkvm/src/witness.rs @@ -59,11 +59,11 @@ impl<T: Sized + Sync + Clone + Send + Copy> RowMajorMatrix<T> { } pub fn num_instances(&self) -> usize { - let mut num = 0; - if self.num_col != 0 { - num = self.values.len() / self.num_col - self.num_padding_rows + if self.num_col == 0 { + 0 + } else { + self.values.len() / self.num_col - self.num_padding_rows } - num } pub fn num_padding_instances(&self) -> usize {

matthiasgoergens · 2024-12-17T09:36:17Z

Please resolve the merge conflicts.

Feat/structural witin add

Merge master

- StructuralWitIn: Addr -> u32 - Combine witness RowMajorMatrix and structural witness RowMajorMatrix

Modify annotations for StructuralWitIn

ceno_zkvm/src/tables/ram/ram_impl.rs

ceno_zkvm/src/scheme/prover.rs

ceno_zkvm/src/scheme/verifier.rs

bugfix & minor

minor

hero78119

overall LGTM! 👍 thanks for the effort!

hero78119 · 2024-12-26T01:30:23Z

ceno_zkvm/src/scheme/verifier.rs

-            .all_equal();
+        // in table proof, we always skip same point sumcheck for now
+        // as tower sumcheck batch product argument/logup in same length
+        let is_skip_same_point_sumcheck = true;


related to respective "prover.rs", an advice to make thing consistent:

diff --git a/ceno_zkvm/src/scheme/prover.rs b/ceno_zkvm/src/scheme/prover.rs index daaa1099..1a690fa0 100644 --- a/ceno_zkvm/src/scheme/prover.rs +++ b/ceno_zkvm/src/scheme/prover.rs @@ -965,12 +965,9 @@ impl<E: ExtensionField, PCS: PolynomialCommitmentScheme<E>> ZKVMProver<E, PCS> { ); exit_span!(tower_span); - // same point sumcheck is optional when all witin + fixed are in same num_vars - let is_skip_same_point_sumcheck = witnesses - .iter() - .chain(fixed.iter()) - .map(|v| v.num_vars()) - .all_equal(); + // in table proof, we always skip same point sumcheck for now + // as tower sumcheck batch product argument/logup in same length + let is_skip_same_point_sumcheck = true; let (input_open_point, same_r_sumcheck_proofs, rw_in_evals, lk_in_evals) = if is_skip_same_point_sumcheck {

It is required that all the lengths are equal, right?

If yes, an assertion would be nice (restore the formula and add assert!(is_skip_same_point_sumcheck)).

hero78119 · 2024-12-26T02:19:28Z

Hey @naure would kindly request you to have a quick review on this design, see whether any frontend/backend information leaked in between

Inspired by reviewing scroll-tech#740

…witin_add

matthiasgoergens · 2024-12-26T05:33:23Z

ceno_zkvm/src/circuit_builder.rs

@@ -75,12 +67,13 @@ pub struct DynamicAddr {

 #[derive(Clone, Debug)]
 pub struct SetTableSpec {
-    pub addr_type: SetTableAddrType,
-    pub len: usize,
+    pub len: Option<usize>,


How does Some(0) differ from None?

naure · 2025-01-02T15:26:40Z

Hey @naure would kindly request you to have a quick review on this design, see whether any frontend/backend information leaked in between

Yes! I did review this and the follow-up #797. See the discussion there which mostly apply to both witness and fixed.

hero78119 and others added 3 commits December 6, 2024 15:26

init structural witin

87899de

skip structural witin commitment & PCS(scroll-tech#654)

2679755

skip structural witin commitment & PCS(scroll-tech#654):

2c21443

- Add check for structural witnesses

10to4 requested a review from hero78119 December 12, 2024 07:43

matthiasgoergens mentioned this pull request Dec 12, 2024

Clean up Circuit Builder #741

Merged

matthiasgoergens reviewed Dec 12, 2024

View reviewed changes

ceno_zkvm/src/circuit_builder.rs Show resolved Hide resolved

10to4 and others added 17 commits December 12, 2024 16:13

Merge branch 'master' into feat/structural_witin_add

9c9f4a5

Upgrade pprof (scroll-tech#706)

e9d24ab

See https://github.com/scroll-tech/ceno/security/dependabot/3 for why.

Update toolchain (scroll-tech#707)

357595b

Just to avoid falling too far behind. There seem to be no issues with the update.

Simplify SLTI tests (scroll-tech#710)

be59026

Extracted from scroll-tech#667 to make that PR smaller and easier to review.

Enable debug assertions in integration tests (scroll-tech#714)

2a35302

To catch some bugs, like the recent breakage. --------- Co-authored-by: noelwei <[email protected]>

Remove redundant format Make targets (scroll-tech#724)

a45eced

Just use `cargo fmt` or `cargo fmt --all` for the same effect. Especially the `fmt` target was obsolote: why would you want to format specifically only `ceno_zkvm`?

Rename mod divu to mod div (scroll-tech#733)

cf15c14

To make the diff for scroll-tech#596 easier to read.

Introduce Value::as_i32 (scroll-tech#732)

e28e557

To help make scroll-tech#596 easier to read and reason about. Also introduce a few more conversion helpers.

Remove broken features non_pow2_rayon_thread and riv64 (scroll-te…

f3566bc

…ch#723) Features `non_pow2_rayon_thread` and `riv64` are not tested: we can't even compile with them. Because `riv64` is gone, we can make `riv32` invisible: it's effects are now just the default.

Rename AssertLTConfig to AssertLtConfig (scroll-tech#731)

5077eb1

Implementing a TODO from scroll-tech#596

Remove unused and untested u128/i128 implementations (scroll-tech…

c07fa00

…#737) Extracted from scroll-tech#736 for ease of review.

Merge branch 'feat/structural_witin_add' of https://github.com/10to4/…

1812068

…ceno into feat/structural_witin_add

skip structural witin commitment & PCS(scroll-tech#654):

55e3b6b

- cargo fmt check

skip structural witin commitment & PCS(scroll-tech#654):

65869f1

- Modify create_structural_witin() & create_witin() to improve readability

hero78119 reviewed Dec 13, 2024

View reviewed changes

skip structural witin commitment & PCS(scroll-tech#654):

92687a4

- Remove SetTableAddrType - Add more meta information in StructuralWitIn - Remove from_expr

hero78119 reviewed Dec 17, 2024

View reviewed changes

ceno_zkvm/src/expression.rs Outdated Show resolved Hide resolved

ceno_zkvm/src/scheme/prover.rs Outdated Show resolved Hide resolved

ceno_zkvm/src/tables/ram/ram_impl.rs Outdated Show resolved Hide resolved

matthiasgoergens reviewed Dec 17, 2024

View reviewed changes

hero78119 and others added 6 commits December 17, 2024 22:49

cleanup multi-factor backend concept from ram circuit frontend

1f06873

Merge pull request #1 from hero78119/feat/structural_witin_add

601cc48

Feat/structural witin add

skip structural witin commitment & PCS(scroll-tech#654):

77b1887

Merge master

skip structural witin commitment & PCS(scroll-tech#654):

103d82c

- StructuralWitIn: Addr -> u32 - Combine witness RowMajorMatrix and structural witness RowMajorMatrix

Merge branch 'master' into feat/structural_witin_add

c040d45

skip structural witin commitment & PCS(scroll-tech#654):

c05d3e2

Modify annotations for StructuralWitIn

hero78119 reviewed Dec 24, 2024

View reviewed changes

ceno_zkvm/src/tables/ram/ram_impl.rs Outdated Show resolved Hide resolved

ceno_zkvm/src/tables/ram/ram_impl.rs Outdated Show resolved Hide resolved

ceno_zkvm/src/scheme/prover.rs Outdated Show resolved Hide resolved

ceno_zkvm/src/scheme/verifier.rs Outdated Show resolved Hide resolved

10to4 added 3 commits December 24, 2024 13:56

Merge branch 'master' into feat/structural_witin_add

669ae05

skip structural witin commitment & PCS(scroll-tech#654):

e8eed3c

bugfix & minor

skip structural witin commitment & PCS(scroll-tech#654):

acf0d9f

minor

hero78119 approved these changes Dec 26, 2024

View reviewed changes

hero78119 requested a review from naure December 26, 2024 02:19

hero78119 mentioned this pull request Dec 26, 2024

skip PCS of range table #789

Open

Less cloning

88cd547

Inspired by reviewing scroll-tech#740

matthiasgoergens mentioned this pull request Dec 26, 2024

refactor: less cloning #790

Closed

matthiasgoergens added 2 commits December 26, 2024 12:58

Merge branch 'matthias/less-cloning-2' into matthias/feat/structural_…

689691e

…witin_add

Remove unnecessary wrapping in Expression

c60475b

matthiasgoergens enabled auto-merge December 26, 2024 05:19

matthiasgoergens approved these changes Dec 26, 2024

View reviewed changes

matthiasgoergens disabled auto-merge December 26, 2024 05:21

matthiasgoergens force-pushed the feat/structural_witin_add branch from dbdddeb to c60475b Compare December 26, 2024 05:30

Unwrap instead of unreachable

0601b4b

matthiasgoergens enabled auto-merge December 26, 2024 05:32

matthiasgoergens reviewed Dec 26, 2024

View reviewed changes

Doc comment

606e1bf

matthiasgoergens added this pull request to the merge queue Dec 26, 2024

Merged via the queue into scroll-tech:master with commit f1de609 Dec 26, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/structural witin add #740

Feat/structural witin add #740

10to4 commented Dec 12, 2024

hero78119 left a comment

hero78119 left a comment

matthiasgoergens Dec 12, 2024

10to4 Dec 23, 2024

matthiasgoergens Dec 12, 2024

10to4 Dec 23, 2024

matthiasgoergens Dec 17, 2024

10to4 Dec 23, 2024

matthiasgoergens commented Dec 17, 2024

hero78119 left a comment

hero78119 Dec 26, 2024

naure Jan 2, 2025

hero78119 commented Dec 26, 2024

matthiasgoergens Dec 26, 2024

naure commented Jan 2, 2025

Feat/structural witin add #740

Feat/structural witin add #740

Conversation

10to4 commented Dec 12, 2024

hero78119 left a comment

Choose a reason for hiding this comment

hero78119 left a 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

matthiasgoergens commented Dec 17, 2024

hero78119 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hero78119 commented Dec 26, 2024

Choose a reason for hiding this comment

naure commented Jan 2, 2025