Add plonky3 support #1158

Schaeff · 2024-03-13T14:24:47Z

plonky3 backend integration. It uses Poseidon with a random array of constants, which acts as a setup. I am not sure this is required, maybe using a fixed constant array is fine, in which case a lot of logic around setup and verification key disappear.

number/src/traits.rs

Required for #1158

…pport-plonky3

leonardoalt

checked all interfaces but not the plonky3 crate yet

pipeline/tests/pil.rs

leonardoalt · 2024-05-31T14:58:37Z

pipeline/src/test_util.rs

+        .with_prover_inputs(inputs)
+        .with_backend(powdr_backend::BackendType::Plonky3, None);
+
+    // Generate a proof with the setup and verification key generated on the fly


Suggested change

// Generate a proof with the setup and verification key generated on the fly

// Generate a proof with the verification key generated on the fly

I guess setup doesn't apply here?

I updated the description of the PR to explain why setup does apply here.

pipeline/src/test_util.rs

number/Cargo.toml

leonardoalt · 2024-05-31T14:59:53Z

pipeline/src/pipeline.rs

@@ -63,11 +62,11 @@ pub struct Artifacts<T: FieldElement> {
    /// An analyzed .pil file, with all dependencies imported, potentially from other files.
    analyzed_pil: Option<Analyzed<T>>,
    /// An optimized .pil file.
-    optimized_pil: Option<Rc<Analyzed<T>>>,
+    optimized_pil: Option<Arc<Analyzed<T>>>,


in general because of plonky3 trait bounds, but maybe the change was not required on this struct, will check.

Ok it is, because from Pipeline we create WitnessCallback, which is included in PowdrCircuit, which implements BaseAir, which requires Sync, which isn't implemented on Rc. So we need Arc everywhere.

leonardoalt · 2024-05-31T15:00:03Z

executor/src/witgen/mod.rs

@@ -49,15 +48,15 @@ impl<T, F> QueryCallback<T> for F where F: Fn(&str) -> Result<Option<T>, String>

 #[derive(Clone)]
 pub struct WitgenCallback<T> {
-    analyzed: Rc<Analyzed<T>>,
-    fixed_col_values: Rc<Vec<(String, Vec<T>)>>,
+    analyzed: Arc<Analyzed<T>>,


leonardoalt · 2024-05-31T15:00:44Z

backend/src/plonky3/mod.rs

+        pil: &'a Analyzed<T>,
+        fixed: &'a [(String, Vec<T>)],
+        _output_dir: Option<&'a Path>,
+        setup: Option<&mut dyn io::Read>,


what is the setup here?

leonardoalt

Nice!!

plonky3/Cargo.toml

leonardoalt · 2024-05-31T15:03:19Z

plonky3/src/prover/mod.rs

+
+use self::types::*;
+
+/// In the context of plonky3, "setup" generation is the process of instantiating Poseidon with randomly sampled constants


changed to static

leonardoalt · 2024-05-31T15:04:03Z

plonky3/src/prover/mod.rs

+    /// The constants which uniquely determine the prover, loosely interpreted as a setup
+    setup: Constants,
+    /// The constants which uniquely determine the prover, loosely interpreted as a verification key
+    vkey: Option<Constants>,


Is this the same as above? Shouldn't the vkey depend on the circuit as well?

good call, the verifier I'm using somehow depends on the air still, hmm

Ok I think it's fine as is, when you call verify the prover has access to the circuit

leonardoalt · 2024-05-31T15:06:06Z

plonky3/src/prover/mod.rs

+#[cfg(test)]
+mod tests {
+    use powdr_number::GoldilocksField;
+    use powdr_pipeline::Pipeline;


Not sure we should have Pipeline as a dependency here, even as a dev-dep. Maybe better to keep these tests together with the others?

Would this be fixed by moving plonky3 into backend?

leonardoalt · 2024-05-31T15:09:09Z

Maybe @georgwiese can take a look at the circuit builder too?

georgwiese

Really cool! I got a few questions & comments.

georgwiese · 2024-06-12T09:47:04Z

plonky3/src/prover/mod.rs

+}
+
+impl<'a, T: FieldElement> Plonky3Prover<'a, T> {
+    pub fn prove_ast(


What does ast stand for?

Good question 😆 I was sure I copied this from halo2. Changing to prove

georgwiese · 2024-06-12T10:56:22Z

plonky3/src/circuit_builder.rs

+pub fn cast_to_goldilocks<T: FieldElement>(v: T) -> Val {
+    Val::from_canonical_u64(v.to_integer().try_into_u64().unwrap())
+}


Can we write it in a way that it would fail for any non-Goldilocks field? Now it would fail for BN254 (because try_into_u64() fails), but I think it also fail for other fields that fit into u64 that we might have in the future.

georgwiese · 2024-06-12T10:58:04Z

plonky3/src/circuit_builder.rs

+    /// The analyzed PIL
+    analyzed: &'a Analyzed<T>,
+    /// The number of committed polynomials, computed from `analyzed` and cached
+    commitment_count: usize,


This field is redundant, it should equalanalyzed.commitment_count(), right?

Correct. I don't really like it but that method iterates through all definitions in the program and is needed to convert each reference. Seemed like a bit much for a number which never changes, but I didn't measure. I could alternatively pass it around in the functions so it's not in the state here.

Or even remove it completely, and eventually if it's an issue it should be cached from within Analyzed

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

plonky3/src/circuit_builder.rs

+                    PolynomialType::Committed => r.poly_id.id as usize,
+                    PolynomialType::Constant => self.commitment_count + r.poly_id.id as usize,


Using the Poly ID assumes that they are consecutive. I think that should be the case, but could you add an assertion?

I added a check that no poly_id can exceed the total count of polynomials of each kind. It does not prove that all declared polynomials are referenced somewhere, so it could technically be non-consecutive, but it makes sure there is no overlap in the plonky3 ids, which I think is what you want?

georgwiese · 2024-06-12T11:06:50Z

plonky3/src/circuit_builder.rs

+    fn to_plonky3_expr<AB: AirBuilder<F = Val>>(
+        &self,
+        e: &AlgebraicExpression<T>,
+        builder: &AB,


Maybe just receive the row pair / matrix?

Done, although it requires more type annotations

georgwiese · 2024-06-12T11:10:38Z

plonky3/src/circuit_builder.rs

+                    let left =
+                        self.to_plonky3_expr(identity.left.selector.as_ref().unwrap(), builder);
+
+                    builder.assert_zero(left);


Do you know why Plonky3 has a built-in is_first_row? I guess it saves a fixed column, because it is just the first Lagrange polynomial can be evaluated efficiently by the verifier?

We could detect when we're using a fixed column of the form [1] + [0]* and use the built-in. But if it only saves 1 commitment during setup + evaluation proof, it might be just a slight advantage and not worth the hassle now.

I remember Miden also has special treatment of the first row, so you could be right. I think this can be a separate issue

georgwiese · 2024-06-12T15:24:17Z

plonky3/src/circuit_builder.rs

+                    builder.assert_zero(left);
+                }
+                IdentityKind::Plookup => unimplemented!("Plonky3 does not support plookup"),
+                IdentityKind::Permutation => unimplemented!("Plonky does not support permutations"),


Suggested change

IdentityKind::Permutation => unimplemented!("Plonky does not support permutations"),

IdentityKind::Permutation => unimplemented!("Plonky3 does not support permutations"),

georgwiese · 2024-06-12T15:44:20Z

plonky3/src/circuit_builder.rs

+        self.publics
+            .iter()
+            .map(|(col_name, i)| cast_to_goldilocks(witness.get(col_name).unwrap()[*i]))
+            .collect()


Do you actually constrain them somewhere? If not, I think we should assert that the publics are empty for now.

Good point, I did not implement that logic in eval. Removing as it can be a separate PR.

georgwiese · 2024-06-12T15:57:40Z

plonky3/src/prover/params.rs

+//! The concrete parameters used in the prover
+//! Inspired from [this example](https://github.com/Plonky3/Plonky3/blob/6a1b0710fdf85136d0fdd645b92933615867740a/keccak-air/examples/prove_goldilocks_keccak.rs#L57)


OK, this looks like something a cryptographer should have a say on. I guess for now it doesn't really matter, except if it has performance implications?

georgwiese

How do fixed columns work? The comment says "Since plonky3 does not have fixed columns", but it needs to right? Otherwise, we're not proving the right statement?

I think we need the equivalent of this code somewhere to commit to fixed columns during setup.

georgwiese · 2024-06-13T14:38:26Z

plonky3/src/circuit_builder.rs

+        self.commitment_count + self.constant_count
+    }
+
+    fn preprocessed_trace(&self) -> Option<RowMajorMatrix<Val>> {


I don't know why this is actually in the BaseAir trait (doesn't seem to be called by Plonky3?), but from the name I think they mean this to return fixed columns only.

In SP1, they actually don't implement this function (they panic when it's called) and instead implement this trait, which has both generate_preprocessed_trace() and generate_trace(), which return fixed and committed columns.

Schaeff force-pushed the support-plonky3 branch from 55b0f06 to f8656e2 Compare March 13, 2024 14:29

leonardoalt reviewed Mar 13, 2024

View reviewed changes

number/src/traits.rs Outdated Show resolved Hide resolved

Schaeff force-pushed the support-plonky3 branch 2 times, most recently from c934ab6 to d7ac075 Compare April 4, 2024 15:56

upgrade rust

5d02fe5

Schaeff mentioned this pull request Apr 4, 2024

Upgrade rust to 1.77 #1253

Merged

Schaeff added 9 commits April 4, 2024 18:17

clippy and fmt

eb43320

add types and methods, next add tests

6ac529d

integrate with cli and tests, add failing test

fd8cb86

wip

c301dfe

support polynomial identities, add tests, failing when opening

8272861

add other plonky3 fields, make tests pass

3449539

keep only goldilocks, clean up

b34ecc0

fix

0e7b320

lock version

7909217

Schaeff force-pushed the support-plonky3 branch from d7ac075 to 7909217 Compare April 4, 2024 16:24

Schaeff changed the base branch from main to upgrade-to-rust-1-77 April 4, 2024 16:24

fix plonky3 impl

538ee58

github-merge-queue bot pushed a commit that referenced this pull request Apr 4, 2024

Upgrade rust to 1.77 (#1253)

ef2409b

Required for #1158

Base automatically changed from upgrade-to-rust-1-77 to main April 4, 2024 20:49

Schaeff added 7 commits April 19, 2024 18:15

create actual proofs, expose in backend

57b7a3a

merge main

f25e8cc

Merge branch 'main' of github.com:powdr-labs/powdr into support-plonky3

aef2c6e

fix end to end, clean

dc833f2

merge main

845c09f

clippy

b8bb421

remove plonky3 from simple sum test

d13b3a8

Schaeff force-pushed the support-plonky3 branch from 0f94e74 to d13b3a8 Compare May 27, 2024 15:49

Schaeff added 2 commits May 29, 2024 14:44

clean up, document

1ad8de2

Merge branch 'support-plonky3' of github.com:powdr-labs/powdr into su…

1ba7b80

…pport-plonky3

Schaeff added 3 commits May 29, 2024 15:06

remove leftover file, add aggr error

ece8695

self review

e864bab

Merge branch 'main' of github.com:powdr-labs/powdr into support-plonky3

e604f35

Schaeff marked this pull request as ready for review May 29, 2024 13:31

leonardoalt reviewed May 31, 2024

View reviewed changes

Schaeff added 2 commits June 5, 2024 15:40

review comments

4cce6a6

use static constant vector

81aeb55

Schaeff requested a review from leonardoalt June 6, 2024 12:22

georgwiese reviewed Jun 12, 2024

View reviewed changes

address review comments

376a690

georgwiese reviewed Jun 13, 2024

View reviewed changes

Schaeff added 2 commits June 14, 2024 16:23

removed cached values

470ff60

pass matrix instead of builder

7eb665c

	// Generate a proof with the setup and verification key generated on the fly
	// Generate a proof with the verification key generated on the fly


		use self::types::*;

		/// In the context of plonky3, "setup" generation is the process of instantiating Poseidon with randomly sampled constants

		PolynomialType::Committed => r.poly_id.id as usize,
		PolynomialType::Constant => self.commitment_count + r.poly_id.id as usize,

	IdentityKind::Permutation => unimplemented!("Plonky does not support permutations"),
	IdentityKind::Permutation => unimplemented!("Plonky3 does not support permutations"),

		//! The concrete parameters used in the prover
		//! Inspired from [this example](https://github.com/Plonky3/Plonky3/blob/6a1b0710fdf85136d0fdd645b92933615867740a/keccak-air/examples/prove_goldilocks_keccak.rs#L57)

Add plonky3 support #1158

Are you sure you want to change the base?

Add plonky3 support #1158

Conversation

Schaeff commented Mar 13, 2024 • edited

leonardoalt 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoalt 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

Choose a reason for hiding this comment

leonardoalt commented May 31, 2024

georgwiese 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

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

georgwiese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schaeff commented Mar 13, 2024 •

edited