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

Remove height 32 from RLN #239

Merged
merged 5 commits into from
Apr 29, 2024
Merged

Remove height 32 from RLN #239

merged 5 commits into from
Apr 29, 2024

Conversation

tyshko-rostyslav
Copy link
Contributor

@tyshko-rostyslav tyshko-rostyslav commented Apr 22, 2024

Remove tree_height 32 resources and associated circuit loading within

Copy link

Benchmark for a70d395

Click to view benchmark
Test Base PR %
Pmtree::compute_root 0.0±0.00ns 0.0±0.00ns NaN%
Pmtree::get 317.5±3.32ns 317.8±4.80ns +0.09%
Pmtree::override_range 239.1±5.63µs 234.5±5.78µs -1.92%
Pmtree::set 55.8±0.91µs 55.8±0.21µs 0.00%
Pmtree:delete 55.7±0.39µs 55.9±0.68µs +0.36%

Copy link

Benchmark for a70d395

Click to view benchmark
Test Base PR %
FullMerkleTree::compute_root 0.0±0.00ns 0.0±0.00ns NaN%
FullMerkleTree::delete 888.0±12.90ns 892.5±49.56ns +0.51%
FullMerkleTree::get 3.4±0.04ns 3.4±0.03ns 0.00%
FullMerkleTree::override_range 2.2±0.03µs 2.2±0.03µs 0.00%
FullMerkleTree::set 884.2±6.75ns 882.8±12.63ns -0.16%
OptimalMerkleTree::compute_root 1036.9±12.68ns 1034.9±8.80ns -0.19%
OptimalMerkleTree::delete 1027.1±8.61ns 1028.6±12.92ns +0.15%
OptimalMerkleTree::get 23.5±0.24ns 23.5±0.32ns 0.00%
OptimalMerkleTree::override_range 5.2±0.14µs 5.2±0.06µs 0.00%
OptimalMerkleTree::set 1027.4±7.45ns 1034.1±60.70ns +0.65%

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! Please look at the changes requested :)

@@ -33,8 +33,8 @@ const WASM_FILENAME: &str = "rln.wasm";
// Note that the circuit and keys in TEST_RESOURCES_FOLDER are compiled for Merkle trees of height 20 & 32
// Changing these parameters to other values than these defaults will cause zkSNARK proof verification to fail
pub const TEST_PARAMETERS_INDEX: usize = 0;
pub const TEST_TREE_HEIGHT: usize = [20, 32][TEST_PARAMETERS_INDEX];
pub const TEST_RESOURCES_FOLDER: &str = ["tree_height_20", "tree_height_32"][TEST_PARAMETERS_INDEX];
pub const TEST_TREE_HEIGHT: usize = [20][TEST_PARAMETERS_INDEX];
Copy link
Contributor

Choose a reason for hiding this comment

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

you should also remove the tree_height_32 resources from the resources sub-directory.
please fix the comments which reference the tree height 32 too.
we can remove the TEST_TREE_HEIGHT, TEST_PARAMETERS_INDEX constants since they can have only one value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed resources.
Removed TEST_PARAMETERS_INDEX , but left TEST_TREE_HEIGHT, because it is used all over the place, and removing it would make it less descriptive.

@rymnc
Copy link
Contributor

rymnc commented Apr 22, 2024

Please also add some description to your PR, and link back to the issue in which this was assigned to you

@tyshko-rostyslav
Copy link
Contributor Author

Part of #237

Copy link

Benchmark for f03469a

Click to view benchmark
Test Base PR %
Pmtree::compute_root 0.0±0.00ns 0.0±0.00ns NaN%
Pmtree::get 319.2±4.13ns 327.5±3.35ns +2.60%
Pmtree::override_range 237.3±3.89µs 240.3±5.32µs +1.26%
Pmtree::set 55.7±0.45µs 56.6±1.25µs +1.62%
Pmtree:delete 55.7±0.54µs 56.2±1.12µs +0.90%

@tyshko-rostyslav
Copy link
Contributor Author

Please also add some description to your PR, and link back to the issue in which this was assigned to you

Done

Copy link

Benchmark for f03469a

Click to view benchmark
Test Base PR %
FullMerkleTree::compute_root 0.0±0.00ns 0.0±0.00ns NaN%
FullMerkleTree::delete 888.8±7.71ns 903.5±16.19ns +1.65%
FullMerkleTree::get 3.4±0.06ns 3.5±0.28ns +2.94%
FullMerkleTree::override_range 2.2±0.03µs 2.2±0.03µs 0.00%
FullMerkleTree::set 885.3±6.94ns 900.0±25.89ns +1.66%
OptimalMerkleTree::compute_root 1036.8±14.00ns 1038.3±16.45ns +0.14%
OptimalMerkleTree::delete 1040.0±43.06ns 1046.9±86.38ns +0.66%
OptimalMerkleTree::get 23.5±1.36ns 23.3±0.15ns -0.85%
OptimalMerkleTree::override_range 5.2±0.06µs 5.3±0.22µs +1.92%
OptimalMerkleTree::set 1050.6±17.07ns 1029.6±15.80ns -2.00%

@rymnc rymnc merged commit a372053 into master Apr 29, 2024
10 checks passed
@rymnc rymnc deleted the rm-height-32 branch April 29, 2024 12:56
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.

None yet

2 participants