chore: `folding-schemes` docs + lint #188

Autoparallel · 2024-12-20T14:43:03Z

In this PR

Here's what I'm doing:

Docs

Working to adding more documentation adhering to the crate level attributes:

#![warn(missing_docs, clippy::missing_docs_in_private_items)]

Personally, I enjoy having these lint warnings appear as they keep me honest in keeping well documented code. I do have to say, Sonobe is pretty well documented already! But this has helped me find some spots to add to, and docs_in_private_items is really nice for other developers.

Lint

I usually have my clippy set to pedantic so I get a bunch of... pedantic lint warnings! These have helped me catch things like redundant clones, methods that don't need to return Result, and also catch spots where we cast into usize from types like u64.

`TODO`s

I've added various TODOs around the modules I've touched with my name on them. I've just noticed a few things that could be addressed (e.g., some possible panics that could be avoided and usize casting I already mentioned). Also just a few spots that could be cleaned up or have some other oddities.

Notes

I'm going to leave some notes here that may be good to turn into issues if we come to agreement on them.

`CommitmentScheme` trait

Basically I think that there should likely be two implementations for CommitmentScheme<G>,
one that is hiding, and one that is not (as opposed to the const generic H in the trait itself define two versions). We could have inheritance like so HidingCommitmentScheme: CommitmentScheme.
Then, if we impl, for example, CommitmentScheme<G> for KZG<'a, E, H>, then we can impl HidingCommitmentScheme for KZG<'a, E, true>. The implementation of HidingCommitmentScheme
would be super straight forward as it would just add the blinding factor to the output of the "super" CommitmentScheme commit and prove methods. Then those
methods on CommitmentScheme do not have to take in blind: Option<E::ScalarField> or the dyn Rng.

This would mean no need for user to handle Option and no potential erroneous inputs (i.e., H == true but also blinding.is_none() == true). This would just provide comptime assurance of the API.

Prelude

I think the folding-schemes crate could use a prelude module that is not only used throughout the crate itself, but would be convenient for those who bring this crate in externally. There's a lot of traits to track otherwise. The one issue here would be that some trait names interfere with struct names (e.g., Decider).

This PR is still a WIP. I will continue to add to it and flush out the folding-schemes crate prior to marking ready to review.

Autoparallel · 2024-12-20T16:23:10Z

Leaving off for here now.

Autoparallel · 2024-12-21T14:22:17Z

Pinging here real quick.

Note commits 4779599 and 56f3063 I make some internal API changes that remove a good amount of cloning. I suspect that this could be taken further, but it might be smarter to cherry pick these into a different branch and focus this PR on docs.

What do maintainers think?

arnaucube

Thanks for this PR, it really improves the code quality and it's docs! Also the removal of several clones is a nice improvement.

Left some comments, some of them are more questions to understand better the possibilities.

arnaucube · 2024-12-21T14:52:04Z

folding-schemes/src/commitment/ipa.rs

 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct IPA<C: CurveGroup, const H: bool = false> {
+    /// The inner [`CurveGroup`] type


I'm not sure on how much needed these comments are. Like, what is describing is something that is clear at simple sight. If clippy is complaining about them maybe clippy is set to hardcore?
Or maybe it is just a matter of following the official rust style, since at the end is not so bad (eg. does not complicate the readability) and in this way the code is more aligned with the official rust style? and so that in this way despite cases like this one where the comment does not bring much new info to the code, this enforces that in other cases where it's not so clear to have a comment we would be enforced to put the comment and thus improving readability for future readers of that more complex part.

arnaucube · 2024-12-21T14:56:56Z

folding-schemes/src/arith/r1cs/mod.rs

+///
+/// Returns a tuple (w, x) containing:
+/// * w: The witness assignment vector
+/// * x: The public input vector (excluding the constant 1)
 pub fn extract_w_x<F: PrimeField>(cs: &ConstraintSystem<F>) -> (Vec<F>, Vec<F>) {


Q1)
Is there some standard way for the new method comments?
in some cases it lists the 'returned parameters', eg:

/// # Returns /// /// Returns a tuple (w, x) containing: /// * w: The witness assignment vector /// * x: The public input vector (excluding the constant 1) pub fn extract_w_x<F: PrimeField>(cs: &ConstraintSystem<F>) -> (Vec<F>, Vec<F>) {

but in others only mentions the 'returned Error cases', without listing the 'returned parameter' (R1CS<F> in the following case), eg:

/// # Errors /// /// Returns an error if: /// * The constraint system matrices haven't been generated yet /// * The conversion between matrix formats fails pub fn extract_r1cs<F: PrimeField>(cs: &ConstraintSystem<F>) -> Result<R1CS<F>, Error> {

And some others (eg. comitment/mod.rs methods) include also the Arguments section.

Q2) Related to #188 (comment) , what I'm not sure is about 'maintainibility' of the style of the comments, like, when somebody will open a new PR adding a feature, how do we keep coherence of the new code style with the previous existing code, without a human not missing any spot in the PR review. Like, if there is any automated way (eg. clippy (?)) that can ensure that the new code follows the same structure for the comments.

arnaucube · 2024-12-21T15:02:35Z

folding-schemes/src/arith/r1cs/mod.rs

+    /// Returns the number of witness variables
    #[inline]
-    pub fn num_witnesses(&self) -> usize {
+    pub const fn num_witnesses(&self) -> usize {


Q1) One question from my Rust style ignorance: how of a 'must' are those comments? looks like in these cases the name of the method is already self-descriptive and the comment does not bring any new information, right?

I insist that this question comes from my Rust-style ignorance, if it's a must to have all those comments I don't have any objection, but want to understand if it is really a 'must'.

Q2) Also, I wonder, in case we incorporate all those comments, how do we enforce it for future PRs, without depending on a human not missing any new function without its respective comment. --> would this be just a matter of increasing Clippy's severity?

arnaucube · 2024-12-21T15:15:58Z

folding-schemes/src/commitment/ipa.rs

-/// i. <s, b> computation is done in log time following a modification of the equation 3 in section
-/// 3.2 from the paper.
-/// ii. s computation is done in 2^{k+1}-2 instead of k*2^k.
+//! IPA implements the modified Inner Product Argument described in


Updating the /// from the top of the modules into //! is a good point, cool!

arnaucube · 2024-12-21T15:16:46Z

folding-schemes/src/commitment/ipa.rs

-        let r: Vec<C::ScalarField>;
-        if H {
+        let (l, r) = if H {
            let rng = rng.ok_or(Error::MissingRandomness)?;
-            l = std::iter::repeat_with(|| C::ScalarField::rand(rng))
-                .take(k)
-                .collect();
-            r = std::iter::repeat_with(|| C::ScalarField::rand(rng))
-                .take(k)
-                .collect();
+            (
+                std::iter::repeat_with(|| C::ScalarField::rand(rng))
+                    .take(k)
+                    .collect(),
+                std::iter::repeat_with(|| C::ScalarField::rand(rng))
+                    .take(k)
+                    .collect(),
+            )
        } else {
-            l = vec![];
-            r = vec![];
-        }
+            (vec![], vec![])
+        };


oh right! much better this way ^^

arnaucube · 2024-12-22T07:35:05Z

folding-schemes/src/folding/mod.rs

+//! - [Nova: Recursive Zero-Knowledge Arguments from Folding Schemes](https://eprint.iacr.org/2021/370)
+//! - [HyperNova: Recursive Arguments for Customizable Constraint Systems](https://eprint.iacr.org/2023/573)
+//! - [CycleFold: Folding-scheme-based Recursive Arguments over Different Curves](https://eprint.iacr.org/2023/1192)
+


also ProtoGalaxy ^^
[ProtoGalaxy: Efficient ProtoStar-style folding of multiple instances](https://eprint.iacr.org/2023/1106)

arnaucube · 2024-12-22T07:36:55Z

folding-schemes/src/folding/mod.rs

+        let poseidon_config = poseidon_canonical_config::<Fr>();
+
+        let f_circuit = FC::new(())?;
+
+        let prep_param = NovaPreprocessorParam::new(poseidon_config.clone(), f_circuit);
+        test_serialize_ivc_opt::<G1, G2, FC, N>("nova", &prep_param)?;
+        test_serialize_ivc_opt::<G1, G2, FC, HN>("hypernova", &prep_param)?;
+
        let prep_param = (poseidon_config, f_circuit);
-        test_serialize_ivc_opt::<G1, G2, FC, P>("protogalaxy".to_string(), prep_param)?;
+        test_serialize_ivc_opt::<G1, G2, FC, P>("protogalaxy", &prep_param)?;


Oh nice, this reorg makes it more clear!

arnaucube · 2024-12-22T07:38:51Z

folding-schemes/src/lib.rs

+//! * [Nova: Recursive Zero-Knowledge Arguments from Folding Schemes](https://eprint.iacr.org/2021/370)
+//! * [HyperNova: Recursive Arguments for Customizable Constraint Systems](https://eprint.iacr.org/2023/573)
+//! * [CycleFold: Folding-scheme-based Recursive Arguments over Different Curves](https://eprint.iacr.org/2023/1192)
+


also ProtoGalaxy ^^
[ProtoGalaxy: Efficient ProtoStar-style folding of multiple instances](https://eprint.iacr.org/2023/1106)

arnaucube · 2024-12-22T07:41:14Z

folding-schemes/src/lib.rs

@@ -1,6 +1,51 @@
+//! Sonobe is a library implementing various folding schemes for recursive SNARKs (Succinct Non-interactive ARguments of Knowledge)
+//! and IVC (Incremental Verifiable Computation). It provides a modular, extensible framework for working with different folding
+//! schemes including Nova, HyperNova, and other variants.


Probably worth mentioning ProtoGalaxy here.
The main 3 folding schemes we have now are Nova,HyperNova,ProtoGalaxy, the 3 of them with CycleFold, and additionally there are a couple of variations of Nova which are not finished yet but probably no need to mention them already here, which are Mova and Ova (variations in the sense that they are Nova with a modification in how to deal with the E term of the RelaxedR1CS).

arnaucube · 2024-12-22T07:54:43Z

folding-schemes/src/commitment/pedersen.rs

@@ -77,6 +92,7 @@ impl<C: CurveGroup, const H: bool> CommitmentScheme<C, H> for Pedersen<C, H> {
        Ok(params.h.mul(r) + C::msm_unchecked(&params.generators[..v.len()], v))
    }

+    // TODO (autoparallel): I'm guessing `_rng` is marked with prefix `_` because it goes unused always and it is planned for the future? Otherwise we should remove that prefix.


In this case (Pedersen), it is not used, but in an other implementation of the CommitmentScheme trait (ie. IPA) it is used.

Autoparallel · 2024-12-22T14:24:41Z

@arnaucube thank you for the feedback! Let me address some of the comments about docs more generally prior to addressing the review comments directly.

Overview

In the lib.rs we have set some crate-level attributes:

#![allow(non_snake_case)]
#![allow(non_upper_case_globals)]
#![allow(non_camel_case_types)]
#![warn(missing_docs, clippy::missing_docs_in_private_items)] // new from me

With clippy running in CI and failing with warnings (or, we can set up cargo-deny, but I digress), if docs are missing from public items, we will trigger a warning via:

#![warn(missing_docs)]

and the second #![warn(clippy::missing_docs_in_private_items)] is a more aggressive lint on my part that throws warning when private items miss docs. The latter is really only useful for developers of the crate itself, right? The former is useful for anyone using the crate.

Rust Style

The general Rust style so that docs.rs is consistent usually relies on what missing_docs throws. These docs are the same as what you see in hover-over in an IDE. Anything marked pub, including fields of a struct or variants of a pub enum then require docs. Of course, you don't have to do this, but I do think it adds a nice touch. You are right though that sometimes these docs are then a bit unnecessary, but (and this is my personal preference -- you're entitled to yours!) I'd rather have some unnecessary docs but a fully documented crate than one that lacks docs.

Furthermore, cargo docs do have some implicit formatting structure guidance. These include:

#![warn(clippy::missing_errors_doc)]
#![warn(clippy::missing_panic_doc)]
#![warn(clippy::missing_safety_doc)]

Respectively, these trigger if you don't include why a function that returns Result may error, why a function that can panic would panic, and why a use of unsafe is valid.

You can go as to others such as as:

#![warn(rustdoc::missing_doc_code_examples)]

to trigger when a public component doesn't also provide an example of usage.

Final Comments

My suggestion is to have a lint workflow (this repo already does) and to set warning attributes for what is deemed acceptable. I do think the two I added are nice and I am happy to finish this crate with it completely. Moving forward, it would then force contributors to write docs for what they add or change. The extra cargo/rustdoc style lints are maybe too pedantic especially to drop all at once, but are also worth consideration in my mind.

If at the end of the day we prefer not to lint these at all, I still use them when I'm writing the docs to tell me where I need them :)

As a side note, in my rust analyzer, I have set clippy to be pedantic. I think this is overkill for a crate itself, but it does point towards some things. It may be interesting for others to try this setting to see what kind of stuff gets caught in their crates!

Autoparallel added 4 commits December 20, 2024 06:08

lint: add doc attribute

f2c1bba

docs: crate docs

5bead4e

docs: arith module

7d76948

docs: commitment module

ca885c0

Autoparallel force-pushed the chore/docs-lint branch from f579363 to ca885c0 Compare December 20, 2024 16:22

Autoparallel changed the title ~~chore: docs + lint~~ chore: folding-schemes docs + lint Dec 20, 2024

Autoparallel added 5 commits December 21, 2024 06:47

docs: cleanup in commitment::kzg

6f9500a

docs: folding module

e2ba5f5

docs: folding::circuits

f368325

lint: pass pp_hash by reference

4779599

lint: remove more cloning using refs

56f3063

WIP: removing clones

35358a7

arnaucube reviewed Dec 22, 2024

View reviewed changes

WIP: removing clones

444eabf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: `folding-schemes` docs + lint #188

chore: `folding-schemes` docs + lint #188

Autoparallel commented Dec 20, 2024 •

edited

Loading

Autoparallel commented Dec 20, 2024

Autoparallel commented Dec 21, 2024

arnaucube left a comment

arnaucube Dec 21, 2024 •

edited

Loading

arnaucube Dec 21, 2024 •

edited

Loading

arnaucube Dec 21, 2024

arnaucube Dec 21, 2024

arnaucube Dec 21, 2024

arnaucube Dec 22, 2024

arnaucube Dec 22, 2024

arnaucube Dec 22, 2024

arnaucube Dec 22, 2024

arnaucube Dec 22, 2024

Autoparallel commented Dec 22, 2024

chore: folding-schemes docs + lint #188

Are you sure you want to change the base?

chore: folding-schemes docs + lint #188

Conversation

Autoparallel commented Dec 20, 2024 • edited Loading

In this PR

Docs

Lint

TODOs

Notes

CommitmentScheme trait

Prelude

Autoparallel commented Dec 20, 2024

Autoparallel commented Dec 21, 2024

arnaucube left a comment

Choose a reason for hiding this comment

arnaucube Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

arnaucube Dec 21, 2024 • edited Loading

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

Autoparallel commented Dec 22, 2024

Overview

Rust Style

Final Comments

chore: `folding-schemes` docs + lint #188

chore: `folding-schemes` docs + lint #188

Autoparallel commented Dec 20, 2024 •

edited

Loading

`TODO`s

`CommitmentScheme` trait

arnaucube Dec 21, 2024 •

edited

Loading

arnaucube Dec 21, 2024 •

edited

Loading