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.

Sign up for GitHub

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

Jump to bottom

Initial Plonky2 Plugin #14

Open

bhgomes wants to merge 20 commits into main from feat/plonky2-plugin

Member

bhgomes commented Nov 3, 2022 •

edited

Loading

Builds a Plonky2 plugin that supports the poseidon gadget from openzl_crypto.

Before we can merge this PR, please make sure that all the following items have been checked off:

Linked to an issue with discussion and accepted design OR have an explanation in the PR that describes this work.
Added one line describing your change in CHANGELOG.md and added the appropriate L- label to the PR.
Re-reviewed Files changed in the GitHub PR explorer.
Checked that changes and commits conform to the standards outlined in CONTRIBUTING.md.

bhgomes added 4 commits

November 1, 2022 13:18


          chore: set up Plonky2 dependency

7ce3d34

Signed-off-by: Brandon H. Gomes <[email protected]>


          feat: add boolean and field variables

52e395f

Signed-off-by: Brandon H. Gomes <[email protected]>


          wip: move to plonky2 fork

b119db6

Signed-off-by: Brandon H. Gomes <[email protected]>


          Merge branch 'main' into feat/plonky2-plugin

b0df886

bhgomes self-assigned this

bhgomes and others added 13 commits

November 3, 2022 12:23


          wip: start ecdsa-wasm example

a17f960

Signed-off-by: Brandon H. Gomes <[email protected]>


          feat: add ECDSA WASM example

f717323

Signed-off-by: Brandon H. Gomes <[email protected]>


          chore: update dependencies

2bd4b08


          feat: finish allocation system and start proof system

8cc4f45

Signed-off-by: Brandon H. Gomes <[email protected]>


          feat: add ProofSystem impl for Plonky2

4a32873

Signed-off-by: Brandon H. Gomes <[email protected]>


          Merge branch 'main' into feat/plonky2-plugin

7fbefd7

Signed-off-by: Brandon H. Gomes <[email protected]>


          chore: add CHANGELOG

ce54cc6

Signed-off-by: Brandon H. Gomes <[email protected]>


          feat: add SBoxExponent marker trait

8f07581

Signed-off-by: Brandon H. Gomes <[email protected]>


          compiler changes

e2f76b4

Signed-off-by: Francisco Hernandez Iglesias <[email protected]>


          feat: move to new SBoxExponent API

79cfe47

Signed-off-by: Brandon H. Gomes <[email protected]>


          Merge branch 'main' into feat/plonky2-plugin

2d1c99c

Signed-off-by: Brandon H. Gomes <[email protected]>


          Merge branch 'main' into feat/plonky2-plugin

dc1cef3


          Merge branch 'main' into feat/plonky2-plugin

e290048

bhgomes added the L-added label

bhgomes added 2 commits

January 20, 2023 11:59


          fix: use correct poseidon impl

1fbb5c8

Signed-off-by: Brandon H. Gomes <[email protected]>


          chore: fix dependencies

16b729b

Signed-off-by: Brandon H. Gomes <[email protected]>

bhgomes marked this pull request as ready for review

January 20, 2023 18:44


          wip: remove stable from the CI since it won't work with plonky2 yet

69805e3

Signed-off-by: Brandon H. Gomes <[email protected]>

bhgomes requested review from SupremoUGH and GhostOfGauss

January 20, 2023 19:20

SupremoUGH requested changes

View reviewed changes

.github/workflows/ci.yml

    
            @@ -46,7 +46,7 @@ jobs:
          
                      os:

                        - ubuntu-latest

                      channel:

                        - stable

                        # TODO: add custom stable checks for those parts of the workspace that can run on it

Contributor

SupremoUGH Jan 23, 2023

Why these changes? Is there something wrong with the stable CI?

Member Author

bhgomes Jan 23, 2023

We can't use stable with plonky2 because it uses unstable features.

openzl-crypto/src/poseidon/mod.rs

Comment on lines +69 to +70

    
                  /// Converts a `u32` value to a field element.

                  fn from_u32(elem: u32) -> Self;

Contributor

SupremoUGH Jan 23, 2023

Why not an associated type NumberType and a function
fn from_number_type? This would cover both the arkworks and plonky2 plugin implementations of this trait without losing randomness from the arkworks side.

plugins/arkworks/src/poseidon/mod.rs

@@ @@ -297,6 +292,10 @@ where @@
                   }
               }
+              impl<const ARITY: usize> SBoxExponent for Spec<bn254::Fr, ARITY> {

Contributor

SupremoUGH Jan 23, 2023

Why only bn254 implementation?

plugins/plonky2/src/bool.rs

+                              .is_equal(self.target.target, rhs.target.target),
+                      )
+                  }
+              }

Contributor

SupremoUGH Jan 23, 2023

BitDecomposition impl?

plugins/plonky2/src/compiler.rs Show resolved Hide resolved

plugins/plonky2/src/poseidon/mod.rs

Comment on lines +174 to +185

+              impl<F, const D: usize, const ARITY: usize, COM> Constant<COM> for Spec<F, D, ARITY>
+              where
+                  F: RichField + Extendable<D>,
+              {
+                  type Type = Self;
+                  #[inline]
+                  fn new_constant(this: &Self::Type, compiler: &mut COM) -> Self {
+                      let _ = (this, compiler);
+                      Self(PhantomData)
+                  }
+              }

Contributor

SupremoUGH Jan 23, 2023

Why do we need this?

plugins/plonky2/src/poseidon/mod.rs

Comment on lines +330 to +346

+              impl<const D: usize, const ARITY: usize> poseidon::SBoxExponent
+                  for Spec<GoldilocksField, D, ARITY>
+              {
+                  const SBOX_EXPONENT: u64 = 7;
+              }
+              impl<const D: usize> poseidon::Constants for Spec<GoldilocksField, D, 7> {
+                  const WIDTH: usize = 8;
+                  const FULL_ROUNDS: usize = 8;
+                  const PARTIAL_ROUNDS: usize = 22;
+              }
+              impl<const D: usize> poseidon::Constants for Spec<GoldilocksField, D, 11> {
+                  const WIDTH: usize = 12;
+                  const FULL_ROUNDS: usize = 8;
+                  const PARTIAL_ROUNDS: usize = 22;
+              }

Contributor

SupremoUGH Jan 23, 2023

Where did you get these constants from? Maybe mention in the docs.

examples/ecdsa-wasm/src/lib.rs

Comment on lines +27 to +28

		let pw = PartialWitness::new();
		let mut builder = CircuitBuilder::<C::F, D>::new(config);

Contributor

SupremoUGH Jan 23, 2023

Ideally we'd interact with plonky2 through the plugin structs and not their native ones.

examples/ecdsa-wasm/src/lib.rs

+                  data.verify(proof).expect("Verification failed.");
+              }
+              /// Testing Module

Contributor

SupremoUGH Jan 23, 2023

Time for openzl-benchmark?

examples/ecdsa-wasm/src/lib.rs

+              /// Tests the proving and verification of the ECDSA signature circuit.
+              #[inline]
+              pub fn test_ecdsa_circuit<C, const D: usize>(config: CircuitConfig)

Contributor

SupremoUGH Jan 23, 2023

We don't use the eclair interface at all for this circuit. If the eclair creator doesn't use it in a simple signature scheme example, who will?

Member Author

bhgomes Jan 23, 2023

This stuff was copied directly from plonky2, I haven't converted it over yet.

Contributor

GhostOfGauss Jan 24, 2023

When we do change this let's allocate the pieces appropriately as public/private inputs, rather than all being constants as they are here.

GhostOfGauss reviewed

View reviewed changes

examples/ecdsa-wasm/src/lib.rs

+              /// Tests the proving and verification of the ECDSA signature circuit.
+              #[inline]
+              pub fn test_ecdsa_circuit<C, const D: usize>(config: CircuitConfig)

Contributor

GhostOfGauss Jan 24, 2023

When we do change this let's allocate the pieces appropriately as public/private inputs, rather than all being constants as they are here.

examples/ecdsa-wasm/src/lib.rs

+                      )
+                  }
+                  /// Runs the ECDSA benchmark for a narrow circuit configuration.

Contributor

GhostOfGauss Jan 24, 2023

Suggested change

      
                /// Runs the ECDSA benchmark for a narrow circuit configuration.
          
                /// Runs the ECDSA benchmark for a wide circuit configuration.

openzl-crypto/src/poseidon/mds.rs

    
            @@ -85,13 +85,13 @@ where
          
                  where

                      F: FieldGeneration,

                  {

                      let ys: Vec<F> = (t as u64..2 * t as u64).map(F::from_u64).collect();

                      let ys: Vec<F> = (t as u32..2 * t as u32).map(F::from_u32).collect(); // TODO: Change u64 to integer mod order(F)

Contributor

GhostOfGauss Jan 24, 2023

Was this change because of the smaller size of the Goldilocks field?

plugins/plonky2/Cargo.toml

+              num-bigint = { version = "0.4.3", default-features = false }
+              openzl-crypto = { path = "../../openzl-crypto", default-features = false, features = ["alloc"] }
+              openzl-util = { path = "../../openzl-util", default-features = false, features = ["alloc"] }
+              plonky2 = { git = "https://github.com/openzklib/plonky2", branch = "feat/circuit-data-serde", default-features = false }

Contributor

GhostOfGauss Jan 24, 2023

Why do we use this branch?

plugins/plonky2/src/compiler.rs

+                  }
+              }
+              impl<F, const D: usize> Default for Compiler<F, D>

Contributor

GhostOfGauss Jan 24, 2023

I think the problem is that CircuitBuilder doesn't impl Default even though you can construct it from a CircuitConfig which does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L-added