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

Batch Merkle Update #432

Conversation

@willmeister
Copy link
Collaborator

commented Sep 6, 2019

Adds MerkleTree.batchMerkleUpdate(...)

Also adds re-entrant locking in SMT.

Metadata

Fixes

Contributing Agreement

willmeister added 11 commits Sep 4, 2019
…ptimizedSparseMerkleTree

- Adds unit tests for getMerkleProof
- Fixes bug where proof siblings stored within verifyAndStore are stored with the wrong key (actual key instead of sibling key)
- Fixed issue in SMT.getLeaf(...)
- Added batchUpdate(...) to MerkleTree and SMT implementation
@willmeister

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 6, 2019

Note: Since the same branch from a previous PR was used, this should definitely be squashed before merge.

Copy link
Contributor

left a comment

This looks good! The update() function is indeed quite a bit nicer than it was previously thankfully. I left one comment where you can use the batching feature of LevelDB to maybe simplify the code/make it very slightly more performant.

Other than that one note it looks good & the tests seem reasonable!

await Promise.all([
...nodesToUpdate.map((n) => this.db.put(this.getNodeID(n), n.value)),
...idsToDelete.map((id) => this.db.del(id)),
])

This comment has been minimized.

Copy link
@karlfloersch

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 11, 2019

Author Collaborator

Yep, this totally makes sense 👍 . I made the change, and there appears to be some error in db.batch(...). Going to merge this as is and create a new ticket to fix it.

@@ -1,4 +1,5 @@
/* External Imports */
import * as domain from 'domain'

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 10, 2019

Contributor

Interesting how async-lock includes 'domain'

@willmeister willmeister merged commit fd9bf6e into plasma-group:master Sep 11, 2019
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@willmeister willmeister deleted the willmeister:feat/430/AutomaticallyVerifyEmptyLeaves branch Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.