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

chore(rln): add ark-zkey support #242

Merged
merged 7 commits into from
May 6, 2024
Merged

chore(rln): add ark-zkey support #242

merged 7 commits into from
May 6, 2024

Conversation

seemenkina
Copy link
Collaborator

@seemenkina seemenkina commented May 1, 2024

For now makefile doesn't work correct because of using whole mopro project as submodule. The problem is that mopro uses a patch for wasmer and zerokit doesn't and as a result there is a conflict in dependencies.

In general benchmark and tests are working if it runs by cargo bench/tests.

So I opened an issue to publish ark-key as a library and will wait for the result. In the meantime, I will try to pull up only part of the sub-module as a kludge solution and do some other tasks

Part of #237

Ekaterina Broslavskaya added 2 commits May 1, 2024 19:13
@seemenkina seemenkina self-assigned this May 1, 2024
@seemenkina seemenkina marked this pull request as draft May 1, 2024 12:23
@seemenkina seemenkina changed the title Draft: chore(rln): add ark-zkey support chore(rln): add ark-zkey support May 1, 2024
Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

thanks for this PR! comments are inline

@rymnc
Copy link
Contributor

rymnc commented May 2, 2024

perhaps we need to shallow clone the repo in the benchmarks github job/?

@seemenkina
Copy link
Collaborator Author

perhaps we need to shallow clone the repo in the benchmarks github job/?

wouldn't that just make a dependency conflict? I mean between their patch wasmer and release wasmer?

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, comments inline :)

@rymnc
Copy link
Contributor

rymnc commented May 2, 2024

perhaps we need to shallow clone the repo in the benchmarks github job/?

wouldn't that just make a dependency conflict? I mean between their patch wasmer and release wasmer?

yup, but we should try to find a way for the benchmarks to run in CI before we can merge this :)

Copy link

github-actions bot commented May 2, 2024

Benchmark for 88670db

Click to view benchmark
Test Base PR %
FullMerkleTree::compute_root 0.0±0.00ns 0.0±0.00ns NaN%
FullMerkleTree::delete 893.9±11.79ns 890.3±29.69ns -0.40%
FullMerkleTree::get 3.4±0.14ns 3.4±0.04ns 0.00%
FullMerkleTree::override_range 2.2±0.03µs 2.2±0.04µs 0.00%
FullMerkleTree::set 891.4±14.89ns 883.0±12.37ns -0.94%
OptimalMerkleTree::compute_root 1050.2±13.47ns 1032.6±13.93ns -1.68%
OptimalMerkleTree::delete 1063.5±7.56ns 1025.3±13.06ns -3.59%
OptimalMerkleTree::get 23.3±0.20ns 23.1±0.31ns -0.86%
OptimalMerkleTree::override_range 5.2±0.07µs 5.2±0.17µs 0.00%
OptimalMerkleTree::set 1049.9±13.72ns 1025.0±12.12ns -2.37%

Copy link

github-actions bot commented May 2, 2024

Benchmark for 88670db

Click to view benchmark
Test Base PR %
Pmtree::compute_root 0.0±0.00ns 0.0±0.00ns NaN%
Pmtree::get 324.0±1.78ns 319.9±4.14ns -1.27%
Pmtree::override_range 230.0±3.32µs 236.7±5.88µs +2.91%
Pmtree::set 55.8±0.42µs 54.3±0.62µs -2.69%
Pmtree:delete 55.7±0.43µs 54.3±0.56µs -2.51%
zkey::upload_from_folder 3.3±0.01s N/A N/A

@seemenkina seemenkina marked this pull request as ready for review May 2, 2024 10:54
Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@seemenkina seemenkina merged commit 8581ac0 into master May 6, 2024
10 checks passed
@rymnc rymnc deleted the add-arkzkey-support branch May 6, 2024 11:10
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