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

Implemented updateMany method as required in issue #117 #314

Merged
merged 13 commits into from
Sep 9, 2024

Conversation

ChinoCribioli
Copy link
Contributor

@ChinoCribioli ChinoCribioli commented Aug 29, 2024

Description

This PR contains an implementation of a method that enables the user to update many leaves of the tree at once, with the restriction of those leaves being already in the tree. That is, the method doesn't insert new elements in the tree and throws an error when trying this.

Additionally, I included a test suite that checks the method behaves as expected and a short description of the method in its declaration.

Related Issue(s)

Closes #117

Other information

The proof of why the new method implementation doesn't improve worst-case time complexity compared with the naive approach is not trivial, and I didn't include it in the method description because it would take several lines. But I can write it down to include it in the LeanIMT paper if desired.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have run yarn style without getting any errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

packages/lean-imt/src/lean-imt.ts Outdated Show resolved Hide resolved
packages/lean-imt/src/lean-imt.ts Show resolved Hide resolved
@cedoor
Copy link
Member

cedoor commented Aug 30, 2024

@ChinoCribioli thanks for this PR 👍🏽

The proof of why the new method implementation doesn't improve worst-case time complexity compared with the naive approach is not trivial, and I didn't include it in the method description because it would take several lines. But I can write it down to include it in the LeanIMT paper if desired.

I think it makes sense to add this info to the function. No worries if it needs several lines.

@ChinoCribioli ChinoCribioli requested a review from cedoor August 31, 2024 22:36
Copy link
Member

@vplasencia vplasencia left a comment

Choose a reason for hiding this comment

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

Hey @ChinoCribioli great work! Thank you very much.

I just left some comments.

packages/lean-imt/src/lean-imt.ts Outdated Show resolved Hide resolved
packages/lean-imt/tests/lean-imt.test.ts Show resolved Hide resolved
packages/lean-imt/src/lean-imt.ts Show resolved Hide resolved
@@ -233,6 +233,67 @@ export default class LeanIMT<N = bigint> {
this._nodes[this.depth] = [node]
}

/**
* Updates m leaves all at once.
Copy link
Member

Choose a reason for hiding this comment

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

Re the time complexity of the algorithm.

n: Number of leaves of the tree.

I think that the worst case for updating multiple elements at once is to update all the leaves. Then the number of possible leaves to update is bounded by n.

Then the time complexity of the update function in a loop for the worst case would be O(n log n). Because you will need to go from a leaf to the root (log n operations) n times.

I think that the worst case for this updateMany function is O(n) because since you would be updating all the leaves, it's like rebuilding the tree. Rebuilding the tree is O(n) because the number of operations when building a tree from the leaves is n + ceiling(n/2) + ceiling(n/4) + ... + ceiling(n/(2^k)) that's <= 2*n + O(log n) and that's <= O(n) + O(log n) which is O(n).

Number of operations when building the tree (or updating all the elements in the tree at once):

$$ n + \lceil \frac{n}{2} \rceil + \lceil \frac{n}{4} \rceil + ... + \lceil \frac{n}{2^d} \rceil$$

That is the same as $$\sum_{k=0}^{d} \lceil \frac{n}{2^k} \rceil$$

$$\sum_{k=0}^{d} \lceil \frac{n}{2^k} \rceil \leq \sum_{k=0}^{d} (\frac{n}{2^k} + 1)$$

$$\leq \sum_{k=0}^{d} \frac{n}{2^k} + \sum_{k=0}^{d} 1$$

$$\leq 2n + O(\log n)$$

$$\leq O(n) + O(\log n)$$

$$\Rightarrow O(n)$$

$$\sum_{k=0}^{d} \frac{n}{2^k} = n \sum_{k=0}^{d} \frac{1}{2^k} \approx n * 2 \Rightarrow 2n$$

$$\sum_{k=0}^{d} 1 = d + 1 = O(\log n) + 1 \Rightarrow O(\log n)$$

For more reference on this, you can take a look at the InsertMany time complexity in the LeanIMT paper: https://github.com/vplasencia/leanimt-paper/blob/main/paper/leanimt-paper.pdf

Does it make sense to explain it like this?

If you use n instead of m, you will also get O(n). Does it make sense to modify your proof assuming that the worst case for m is n since we are calculating big O notation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you set m = n, the complexity as I stated it becomes O(n) since log(n)-log(m) = 0. I think it makes sense to add this case separately to the complexity statement, but maintaining the whole explanation with a smaller m.

That is, my proof is a generalization of the case m=n, so I don't see why we should take that out. Completely agree with mentioning that the algorithm is O(n), though.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the worst case the one when the nodes to update have as few ancestors as possible on all levels? In that case, time complexity would be quite similar to the one of the update function in a loop, i.e. O(n * log n).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I also meant n as the subset of nodes to be updated (and not all leaves). We are on the same page 👍🏽

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but as the number of leaves to update increases, the condition of "having few common ancestors" starts being stronger and stronger, to the point where common ancestors start appearing inevitably. This makes that we never reach n*log(n) operations.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @ChinoCribioli could you update the updateMany function documentation with something similar to what you have at the beginning and move the time complexity analysis to a new issue called something like UpdateMany time complexity and add a comment with your proof so that we can discuss it there. Then, if you want, you could add it to the LeanIMT paper. What do you think?

Copy link
Contributor Author

@ChinoCribioli ChinoCribioli Sep 2, 2024

Choose a reason for hiding this comment

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

@vplasencia Done. I changed the documentation to a more minimalist description and created this issue to discuss the deeper complexity analysis.

After the discussion is closed, you may reach out to me to add the whole analysis to the LeanIMT paper.

Copy link
Member

Choose a reason for hiding this comment

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

Great! Thank you very much! 🙏

Co-authored-by: Vivian Plasencia <v.pcalana@gmail.com>
Copy link
Member

@vplasencia vplasencia left a comment

Choose a reason for hiding this comment

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

Thank you very much for the great work @ChinoCribioli

@vplasencia vplasencia merged commit f910628 into privacy-scaling-explorations:main Sep 9, 2024
2 checks passed
Copy link

gitpoap-bot bot commented Sep 9, 2024

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2024 ZK-KIT Contributor:

GitPOAP: 2024 ZK-KIT Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

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.

Add a function to update many members at once to the LeanIMT implementation
3 participants