WIP: New implementations and tests for Zerokit v0.1 #34

s1fr0 · 2022-08-22T17:59:32Z

Targets #31

richard-ramos

💯

Looking at the code, a random idea i had that wanted to ask you about is:
What do you think of extracting in a patch file or separate branch these changes?:

in poseidon_tree.rs:

//pub type PoseidonTree = FullMerkleTree<PoseidonHash>;
//pub type MerkleProof = FullMerkleProof<PoseidonHash>;

the functions defined in rln/src/utils.rs.

That way the source code would not contain commented code, and someone wanting to use either the FullMerkleTree, or the conversion utils could either cherry pick the commit from the branch, or apply the git patch (assuming this operation is documented somewhere)

s1fr0 · 2022-08-24T16:07:12Z

@richard-ramos seems a bit too much effort on the user side. I think we can reach a good compromise already using rust features and enable one Merkle tree implementation or the other by just checking which feature is active for the crate. What do you think?

richard-ramos · 2022-08-24T18:50:26Z

Sounds good! I did not know about rust features!

…orks arithmetic

s1fr0 · 2022-08-27T11:30:17Z

@richard-ramos I integrated poseidon hash implementation so that works directly with arkworks (and we avoid some heavy dependencies just for conversion to poseidon-rs field type) and added compilation time features to switch to FullMerkleTree implementation and to hide performance proof generation/verification logs when running in release mode.

Allows passing the wasm, zkey and verification key data as buffers, instead of using a path to a folder

oskarth

This is not a release PR, it is doing a lot of things at once. These should be separate PRs. The release PR should be minimal and only about adding release notes etc.

See e.g. how this is done in nwaku: https://github.com/status-im/nwaku/pull/1075/files

I'm also not convinced that we should decrease the capability this library has as part of the first release.

s1fr0 · 2022-09-13T22:05:27Z

Probably the title is unfortunate (and the description can be a bit more talkative), but the goal was to target the items of the milestone issue #31. Then we had some blockers I mentioned you with the merge, and to not stop completely development on zerokit (after the switch to the arkworks field arithmetic, it would have been impossible to continue working to a new branch based on master without solving lots of conflicts) we just continued working on this branch. However, besides the change in the field arithmetic, the main other aspect covered by this PR is minimizing usage of dependencies, which was added to the milestone issue description as well.

I agree that a release PR should be structured differently (but for this we might simply adjust description). If better, commits might be cherry-picked to different PRs instead.

I also agree that we should try to not decrease capacity of the library: I internally shared some possibilities on how we can possibly address this.

oskarth · 2022-09-14T03:48:40Z

multiplier/Cargo.toml

@@ -1,35 +0,0 @@
-[package]


This is a useful example for onboarding and understanding roughly how things fit together. I don't think it makes sense to remove this package.

We could move it to an example directory.

oskarth · 2022-09-14T03:49:18Z

poseidon-tornado/README.md

@@ -1,15 +0,0 @@
-# Poseidon-Tornado wrapper


We want this soon, but I agree it is in a so-so state and better to base based on RLN from scratch at this point.

Removed in 1d8a5f5

cc @staheri14 if you want to see what this code is doing

oskarth · 2022-09-14T03:52:15Z

rln/Cargo.toml

@@ -3,76 +3,39 @@ name = "rln"
 version = "0.1.0"
 edition = "2021"

-# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html


Do these stylistic/refactor change in a separate PR

oskarth · 2022-09-14T03:53:16Z

rln/src/circuit.rs

-use ark_circom::{read_zkey, CircomBuilder, CircomConfig, WitnessCalculator};
-use ark_ff::BigInteger256;
+use ark_bn254::{
+    Bn254, Fq as ArkFq, Fq2 as ArkFq2, Fr as ArkFr, G1Affine as ArkG1Affine,


This a change in functionality or so, belongs in a separate PR targeting an issue. Not in a release PR...

Addressed by #43

oskarth · 2022-09-14T03:53:49Z

rln/src/merkle_tree.rs

@@ -35,239 +32,6 @@ pub trait Hasher {
    fn hash(input: &[Self::Fr]) -> Self::Fr;
 }

-////////////////////////////////////////////////////////////


This shouldn't be removed, it is a regression

oskarth · 2022-09-14T03:54:27Z

rln/src/poseidon_hash.rs

+
+// The implementation is taken from https://github.com/arnaucube/poseidon-rs/blob/233027d6075a637c29ad84a8a44f5653b81f0410/src/lib.rs
+// and slightly adapted to work over arkworks field data type
+


This seems like another feature ??

Addressed by #44

oskarth · 2022-09-14T03:56:09Z

semaphore/Cargo.toml

@@ -1,49 +0,0 @@
-[package]


I think this is still useful, we aren't unlikely to want to use this in the feature. There are no problems to keep it in the code base as WIP, and when it gets prioritized to update it.

oskarth · 2022-09-14T03:57:01Z

Yeah I think better to close this PR and start from scratch with feature PRs and refactor PRs and then a minimal release note PR. This is just a monster right now with many separate intentions.

oskarth · 2022-09-14T04:20:18Z

rln/Cargo.toml

@@ -3,76 +3,39 @@ name = "rln"
 version = "0.1.0"


@richard-ramos @staheri14 of the changes in the rln crate, which of these are must haves from your POV? Both for normal integration (e.g. bugs found in nwaku and go-waku) as well as for the experimental wasm support?

Trying to figure out which things should be part of initial v0.1 release and which are OK to deal with later.

s1fr0 · 2022-09-16T23:59:01Z

Changes in this PR were/will be merged by smaller sub-PRs.

refactor(rln): removing unused crates/dependencies

a8ab705

cargo fmt

ea46351

s1fr0 linked an issue Aug 22, 2022 that may be closed by this pull request

Milestone: zerokit release v0.1 #31

Closed

10 tasks

s1fr0 added 4 commits August 23, 2022 18:42

refactor(rln): removed more dependencies; curve/fields as parameters

7a4f042

refactor(rln): use poseidon-rs hash instead of semaphore-rs poseidon

180b050

chore(rln): remove deps

e96620a

refactor(rln): use exclusively arkworks Fr

0165295

s1fr0 requested a review from richard-ramos August 24, 2022 08:42

s1fr0 mentioned this pull request Aug 24, 2022

RLN: Use arkworks field arithmetic only #35

Closed

1 task

richard-ramos approved these changes Aug 24, 2022

View reviewed changes

s1fr0 added 2 commits August 27, 2022 12:03

refactor(rln): integrate poseidon-rs implementation to work with arkw…

1563f80

…orks arithmetic

feat(rln): add features to select MT; remove prints if not in debug mode

8090199

fix(rln): collect test parameters in a vector

dc31272

s1fr0 added the track:zerokit Zerokit track (Applied ZK/Explorations) label Aug 29, 2022

richard-ramos and others added 7 commits August 31, 2022 11:11

feat(RLN): add new_with_params (#36)

7acbde2

Allows passing the wasm, zkey and verification key data as buffers, instead of using a path to a folder

chore(rln): simplify read wasm

5a98cd6

fix(rln): remove unused dependencies

88ba2d1

cargo fmt

370915e

fix(rln): update dependencies, fix commit

7839aed

refactor(rln): set default MT implementation

1948aca

chore(zerokit): remove test modules

08353f6

s1fr0 marked this pull request as ready for review September 11, 2022 22:15

s1fr0 requested a review from oskarth September 11, 2022 22:15

oskarth suggested changes Sep 13, 2022

View reviewed changes

oskarth reviewed Sep 14, 2022

View reviewed changes

s1fr0 changed the title ~~Zerokit release v0.1~~ WIP: Implementations and tests for Zerokit v0.1 Sep 15, 2022

s1fr0 changed the title ~~WIP: Implementations and tests for Zerokit v0.1~~ WIP: New implementations and tests for Zerokit v0.1 Sep 15, 2022

s1fr0 closed this Sep 16, 2022

s1fr0 deleted the release-v0.1 branch October 17, 2022 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: New implementations and tests for Zerokit v0.1 #34

WIP: New implementations and tests for Zerokit v0.1 #34

s1fr0 commented Aug 22, 2022 •

edited

Loading

richard-ramos left a comment

s1fr0 commented Aug 24, 2022

richard-ramos commented Aug 24, 2022

s1fr0 commented Aug 27, 2022 •

edited

Loading

oskarth left a comment

s1fr0 commented Sep 13, 2022 •

edited

Loading

oskarth Sep 14, 2022

oskarth Sep 14, 2022

oskarth Sep 14, 2022

oskarth Sep 14, 2022 •

edited

Loading

oskarth Sep 14, 2022

s1fr0 Sep 14, 2022

oskarth Sep 14, 2022

oskarth Sep 14, 2022

s1fr0 Sep 14, 2022

oskarth Sep 14, 2022

oskarth commented Sep 14, 2022

oskarth Sep 14, 2022

s1fr0 commented Sep 16, 2022


		// The implementation is taken from https://github.com/arnaucube/poseidon-rs/blob/233027d6075a637c29ad84a8a44f5653b81f0410/src/lib.rs
		// and slightly adapted to work over arkworks field data type

WIP: New implementations and tests for Zerokit v0.1 #34

WIP: New implementations and tests for Zerokit v0.1 #34

Conversation

s1fr0 commented Aug 22, 2022 • edited Loading

richard-ramos left a comment

Choose a reason for hiding this comment

s1fr0 commented Aug 24, 2022

richard-ramos commented Aug 24, 2022

s1fr0 commented Aug 27, 2022 • edited Loading

oskarth left a comment

Choose a reason for hiding this comment

s1fr0 commented Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarth Sep 14, 2022 • 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

oskarth commented Sep 14, 2022

Choose a reason for hiding this comment

s1fr0 commented Sep 16, 2022

s1fr0 commented Aug 22, 2022 •

edited

Loading

s1fr0 commented Aug 27, 2022 •

edited

Loading

s1fr0 commented Sep 13, 2022 •

edited

Loading

oskarth Sep 14, 2022 •

edited

Loading