Skip to content
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.

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

Upgrade CS #333

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Upgrade CS #333

wants to merge 6 commits into from

Conversation

Forpee
Copy link

@Forpee Forpee commented Nov 6, 2024

This PR aims to address #331

  • Ported bellpepper-core (& some parts of bellpepper) + Poseidon implementation from @argumentcomputer.

Remaining items:

There’s some unused code from the bellpepper and Poseidon port. Should we remove it now, or do you think it might be useful to keep for potential future needs?

commit cd8559dd3b08261121e4ac797bd364de317c9467
Author: Forpee <[email protected]>
Date:   Wed Nov 6 12:38:02 2024 +0200

    chore: restore cargo

commit f190e75d215b8a73d66c559d427b5e536ddb9c4e
Author: Forpee <[email protected]>
Date:   Wed Nov 6 12:32:32 2024 +0200

    chore: fix warnings, docs, clippy, fmt

commit b849a82e2d142c0fd1564721785295c67c4f2d34
Author: Forpee <[email protected]>
Date:   Wed Nov 6 11:56:28 2024 +0200

    chore: do some doc comments

commit 1fb584f14216f187a42b0eae9e5550042c08e31c
Author: Forpee <[email protected]>
Date:   Wed Nov 6 11:39:23 2024 +0200

    chore: clean up warnings

commit c68b709c53e812bac02bf09f43c08b0f8ce9f1bb
Author: Forpee <[email protected]>
Date:   Wed Nov 6 11:28:21 2024 +0200

    feat: add alloc_precommitted

commit 1642f15efb44c88a990f3e84e9c2a9d11b7bb735
Author: Forpee <[email protected]>
Date:   Wed Nov 6 11:18:36 2024 +0200

    feat: init precommitted var

commit 4aa27a7a32073bf6a3e87d5a9f9cafc2a305b853
Author: Forpee <[email protected]>
Date:   Wed Nov 6 10:53:59 2024 +0200

    chore: make poseidon folder

commit 110e782d888e56c9ea2fba75b807e201f97bc2c1
Author: Forpee <[email protected]>
Date:   Tue Nov 5 23:05:29 2024 +0200

    chore: rm doc tests

commit e9426b2d30078841c2b17492ce96e26e19d41d47
Author: Forpee <[email protected]>
Date:   Tue Nov 5 23:05:04 2024 +0200

    fix: poseidon in pp

commit aa12cbf04bdab6e5d18f45dceb0de6f33dd25dd5
Author: Forpee <[email protected]>
Date:   Tue Nov 5 22:04:57 2024 +0200

    feat: init new CS

commit 54cb3dfd0df2cc4bbde97b475daa4058680d14ff
Author: Forpee <[email protected]>
Date:   Tue Nov 5 20:38:43 2024 +0200

    feat: port poseidon sponge

commit e1368fdd74cd52cbcf65143e6e19d391a861ad66
Author: Forpee <[email protected]>
Date:   Tue Nov 5 20:34:12 2024 +0200

    feat: implement poseidon

commit 3d2e6fad71016afcec88f24c8219240e366f047e
Author: Forpee <[email protected]>
Date:   Tue Nov 5 18:41:20 2024 +0200

    feat: poseidon consts

commit f002464ab64933aa05667976ea9a2e3249ba552d
Author: Forpee <[email protected]>
Date:   Tue Nov 5 18:41:09 2024 +0200

    feat: init CS

commit 9ee87f60e2073ceb9031a93f1056ef10fb241ce2
Author: Forpee <[email protected]>
Date:   Tue Nov 5 14:40:46 2024 +0200

    fix: cargo
@Forpee
Copy link
Author

Forpee commented Nov 6, 2024

@microsoft-github-policy-service agree company="ICME"

@Forpee Forpee changed the title Init: Upgrade CS Upgrade CS Nov 6, 2024
@srinathsetty
Copy link
Collaborator

Hi @Forpee! thanks!

Yes, it is preferred to have a minimal implementation of the required methods. Please attribute the original source if they are taken from an existing library.

@Forpee Forpee marked this pull request as ready for review November 7, 2024 12:56
@srinathsetty
Copy link
Collaborator

For poseidon code, it seems like we are not using a bunch of code. For example, in src/lib.rs, replace pub mod provider with mod provider (this makes clippy complain about unused code). I get the following unused code. Can we please trim code to minimize unused code?

error: variants FullBuffer, IndexOutOfBounds, GpuError, and Other are never constructed
--> src/provider/poseidon/mod.rs:98:3
|
96 | pub enum PoseidonError {
| ------------- variants in this enum
97 | /// The allowed num...
98 | FullBuffer,
| ^^^^^^^^^^
99 | /// Attempt to reference an index element that is out of bounds
100 | IndexOutOfBounds,
| ^^^^^^^^^^^^^^^^
101 | /// GPU error
102 | GpuError(String),
| ^^^^^^^^
103 | /// Other error
104 | Other(String),
| ^^^^^
|
= note: PoseidonError has derived impls for the traits Clone and Debug, but these are intentionally ignored during dead code analysis
note: the lint level is defined here
--> src/lib.rs:4:3
|
4 | unused,
| ^^^^^^
= note: #[deny(dead_code)] implied by #[deny(unused)]

error: methods is_allocated and is_num are never used
--> src/provider/poseidon/circuit2.rs:31:16
|
29 | impl<Scalar: PrimeField> Elt {
| ------------------------------------ methods in this implementation
30 | /// Check if the Elt is allocated.
31 | pub const fn is_allocated(&self) -> bool {
| ^^^^^^^^^^^^
...
36 | pub const fn is_num(&self) -> bool {
| ^^^^^^

error: variants Correct and OptimizedDynamic are never constructed
--> src/provider/poseidon/poseidon_inner.rs:119:3
|
116 | pub enum HashMode {
| -------- variants in this enum
...
119 | Correct,
| ^^^^^^^
...
122 | OptimizedDynamic,
| ^^^^^^^^^^^^^^^^
|
= note: HashMode has derived impls for the traits Clone and Debug, but these are intentionally ignored during dead code analysis

error: associated items new_constant_length and with_length are never used
--> src/provider/poseidon/poseidon_inner.rs:150:10
|
130 | / impl<F, A> PoseidonConstants<F, A>
131 | | where
132 | | F: PrimeField,
133 | | A: Arity,
| |______________- associated items in this implementation
...
150 | pub fn new_constant_length(length: usize) -> Self {
| ^^^^^^^^^^^^^^^^^^^
...
157 | pub fn with_length(&self, length: usize) -> Self {
| ^^^^^^^^^^^

error: associated items new_with_preimage, set_preimage, reset, and input are never used
--> src/provider/poseidon/poseidon_inner.rs:298:10
|
268 | / impl<'a, F, A> Poseidon<'a, F, A>
269 | | where
270 | | F: PrimeField,
271 | | A: Arity,
| |______________- associated items in this implementation
...
298 | pub fn new_with_preimage(preimage: &[F], constants: &'a PoseidonConstan...
| ^^^^^^^^^^^^^^^^^
...
346 | pub fn set_preimage(&mut self, preimage: &[F]) {
| ^^^^^^^^^^^^
...
353 | pub fn reset(&mut self) {
| ^^^^^
...
368 | pub fn input(&mut self, element: F) -> Result<usize, PoseidonError> {
| ^^^^^

error: methods reset and is_squeeze are never used
--> src/provider/poseidon/sponge/api.rs:106:16
|
104 | impl SpongeOp {
| ------------- methods in this implementation
105 | /// Reset the SpongeOp
106 | pub const fn reset(&self) -> Self {
| ^^^^^
...
126 | pub const fn is_squeeze(&self) -> bool {
| ^^^^^^^^^^

error: variant Duplex is never constructed
--> src/provider/poseidon/sponge/vanilla.rs:38:3
|
34 | pub enum Mode {
| ---- variant in this enum
...
38 | Duplex,
| ^^^^^^
|
= note: Mode has a derived impl for the trait Clone, but this is intentionally ignored during dead code analysis

error: multiple associated items are never used
--> src/provider/poseidon/sponge/vanilla.rs:77:6
|
62 | pub trait SpongeTrait<'a, F: PrimeField, A: Arity>
| ----------- associated items in this trait
...
77 | fn simplex_constants(size: usize) -> PoseidonConstants<F, A> {
| ^^^^^^^^^^^^^^^^^
...
82 | fn duplex_constants() -> PoseidonConstants<F, A> {
| ^^^^^^^^^^^^^^^^
...
121 | fn make_elt(&self, val: F, acc: &mut Self::Acc) -> Self::Elt;
| ^^^^^^^^
...
147 | fn is_squeezing(&self) -> bool {
| ^^^^^^^^^^^^
...
192 | fn is_exhausted(&self) -> bool {
| ^^^^^^^^^^^^
...
333 | fn absorb_elements(
| ^^^^^^^^^^^^^^^
...
345 | fn squeeze_elements(&mut self, count: usize, acc: &mut Self::Acc) -> Vec<...
| ^^^^^^^^^^^^^^^^

/// entirely independent sub-circuits. Each can be synthesized in its own thread, then the
/// original `ConstraintSystem` can be extended with each, in the same order they would have
/// been synthesized sequentially.
fn extend(&mut self, _other: &Self) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This method seems unimplemented. Is it worth cutting out unimplemented methods to keep the codebase simple?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there are some places this actually gets used. Will look more.

/// # Returns
///
/// * `false` - By default, a `ConstraintSystem` is not a witness generator.
fn is_witness_generator(&self) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we use this method anywhere?

Copy link
Author

@Forpee Forpee Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, most notably in the Poseidon sponge circuit's permute_state method.

this line:
src/provider/poseidon/sponge/circuit.rs#L127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants