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

No need to verifyAndStore(...) empty leaves before update(...) #431

Conversation

@willmeister
Copy link
Collaborator

commented Sep 5, 2019

Description

Makes it so that update(...) can be called without verifyAndStore(...) for empty leaf updates.

Metadata

Fixes

Blockers

  • #428 Since this has 428's changes merged in

Contributing Agreement

willmeister added 8 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)
Copy link
Contributor

left a comment

Cool! I had one small question/comment but I'm pretty convinced that this works. It's a little complex but some of that is just JS event loop & added complexity of BigNumbers. Anyway, it looks good!

One thing that I did notice which is very minor but the PR wouldn't let me comment on directly -- on line 23 where it says public static readonly emptyBuffer: Buffer = Buffer.alloc(32).fill('\x00') -- I believe that Buffer.alloc(32) does the .fill(...) operation for you. You won't get any non-zero elements. Instead I believe there's a Buffer.allocUnsafe(...) where you'd have to fill with zeros to prevent memory corruption

}

return this.treeMutex.runExclusive(async () => {
// Have to check to make sure nodesToUpdate didn't change while we didn't have the lock.

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 6, 2019

Contributor

I am probably missing some stuff but why not keep a lock the entire time that you're doing the update? Just to reduce a little complexity in this code. Is that possible?

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 6, 2019

Author Collaborator

There's another call that locks (verifyAndStore(...)), so this would deadlock if the lock is held. Other languages have the concept of re-entrant locks where if your thread is the one holding the lock, you can access anything protected by a lock, but unfortunately the mutex library doesn't do this, and my limited research shows that there's no easy swap-in replacement for it that does (since JS is single-threaded, and it may be hard to keep track of parallel contexts).

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 6, 2019

Author Collaborator

That said, I agree with you and am not happy with how ugly this looks.

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 6, 2019

Contributor

Ok that's fair. Then well it sounds like you did the due diligence here & so imo let's just roll with the ugly code & clean it up later as at least this technical debt is isolated.


it.only('updates: 1000, treeHeight: 160, keyRange: 50,000', async () => {
await runUpdateTest(160, 1000, 50_000)
}).timeout(9000)
})

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 6, 2019

Contributor

Nice these tests look way simpler with update!

@willmeister willmeister merged commit cbac215 into plasma-group:master Sep 6, 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 6, 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.