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

fix: 'gte'&'lte' need follow ASCII character set order when load leaves #3424

Closed
wants to merge 1 commit into from

Conversation

coldstar1993
Copy link

@coldstar1993 coldstar1993 commented Nov 25, 2023

As desc in: #3423

gte, lte are compared by characters (numbers) in the ASCII character set. So here is a potential bug that it would miss some Leaf when loadTree.
For example: When we load from levelDB a tree called NULLIFIER_TREE of height 16 (i.e, 2n ** BigInt(this.getDepth()) = 65536), the runtime code is as below:

this.db
        .createReadStream({
           gte: 'NULLIFIER_TREE:LEAF:0'
           lte: 'NULLIFIER_TREE:LEAF:65536'
         });

then, you could find that many leaves whose key starts with 'NULLIFIER_TREE:LEAF:7'/'NULLIFIER_TREE:LEAF:8'/'NULLIFIER_TREE:LEAF:9' will be missed.

Please provide a paragraph or two giving a summary of the change, including relevant motivation and context.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@Maddiaa0 Maddiaa0 requested a review from PhilWindle November 27, 2023 18:05
@PhilWindle
Copy link
Collaborator

@alexghr How does this compare with the solution you used in your PR?

@@ -242,7 +242,7 @@ export class StandardIndexedTree extends TreeBase implements IndexedTree {
this.db
.createReadStream({
gte: indexToKeyLeaf(this.getName(), startingIndex),
lte: indexToKeyLeaf(this.getName(), 2n ** BigInt(this.getDepth())),
lte: indexToKeyLeaf(this.getName(), BigInt((2n ** BigInt(this.getDepth())).toString().replace(/\d/g, '9'))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hye, I think there's a more efficient O(1) way of determining the number of digits (in base 10) 2^n has than converting bigints to strings.

We've also got serialisation primitives in foundation that convert bigints to buffers. In a separate PR we've encoded the leaf indices as big endian hex strings (which would sort naturally)

Copy link
Author

Choose a reason for hiding this comment

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

could u pls share me with your pr? thx

Copy link
Contributor

Choose a reason for hiding this comment

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

@Maddiaa0 Maddiaa0 closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants