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

Implement MerkleTree.getMerkleProof(...) #428

Conversation

@willmeister
Copy link
Collaborator

commented Sep 4, 2019

Description

  • Adds getMerkleProof(...) on MerkleTree interface and implements it for OptimizedSparseMerkleTree
  • 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)

Metadata

Fixes

Contributing Agreement

willmeister added 2 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

Great stuff! 😃 Only thing I noticed that currently, the leafValue passed for getMerkleProof is never actually used other than in the returned object itself, so it seems unnecessary. One option would be to authenticate that it is indeed the value at the requested leafKey, alternatively just removing the argument and using getLeaf internally is an option. What do you think?

willmeister added 2 commits Sep 5, 2019
@willmeister

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Great stuff! 😃 Only thing I noticed that currently, the leafValue passed for getMerkleProof is never actually used other than in the returned object itself, so it seems unnecessary. One option would be to authenticate that it is indeed the value at the requested leafKey, alternatively just removing the argument and using getLeaf internally is an option. What do you think?

That's a good call. I added a check because that's something we should definitely check. Also changed Optimized SMT implementation to be non-optimized to make things easier.

Copy link
Contributor

left a comment

This looks good to me! Only thing that I think is really worth changing is using Buffer.alloc instead of new Buffer so that we don't get Node: Deprecated call new Buffer output logs when we run tests... Plus it shouldn't change the code at all otherwise.

The other thing would be using buffer for keys, but that's not worth the time and effort at this point--later

packages/core/src/app/utils/merkle-tree.ts Outdated Show resolved Hide resolved
@@ -161,48 +126,28 @@ export class OptimizedSparseMerkleTree implements SparseMerkleTree {
})
}

public async update(key: BigNumber, value: Buffer): Promise<boolean> {
public async update(leafKey: BigNumber, leafValue: Buffer): Promise<boolean> {

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 5, 2019

Contributor

I already mentioned this in our call but of course long term we'll probably want 1) to use native bigint in the backend of our BigNumber library / replace with bitint directly / replace with buffer directly.

That said, none of these are worth the effort pre-devocon.


assert(proof.rootHash.equals(await tree.getRootHash()))
})
})

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 5, 2019

Contributor

Went through the tests and they look good! I did write a little benchmark test in addition here:

    it.only('can make a whole lot of updates at random places in the tree', async () => {
      const tree: SparseMerkleTree = new SparseMerkleTreeImpl(
        db,
        undefined,
        24,
        hashFunction
      )

      const startTime = +new Date()
      for (let i = 0; i < 100; i++) {
        const key = new BigNumber(Math.floor(Math.random()*50000))
        await tree.update(key, Buffer.from('yo what is gucci'))
      }
      const finishTime = +new Date()
      const durationInMiliseconds = finishTime - startTime
      console.log('Duration:', durationInMiliseconds)
    }).timeout(9000)

Could consider adding that at the bottom but commented out, or just not at all because this benchmark is truly a hasty and ugly construction lol :P

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 5, 2019

Author Collaborator

I added them and have them uncommented. They go very quickly, so I'm not too worried about having them in.

@karlfloersch

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Oh another note that I totally forgot -- you could consider moving the merkle tree into the block-production directory / rename the block-production directory to merklization or something. Just because we've got other merklization code in the block-production section

willmeister added 3 commits Sep 5, 2019
@willmeister

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Oh another note that I totally forgot -- you could consider moving the merkle tree into the block-production directory / rename the block-production directory to merklization or something. Just because we've got other merklization code in the block-production section

Sounds good. Moved it to block-production 👍

@willmeister willmeister merged commit 51c2e8f 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/411/AddGetMerkleProofToMerkleTree 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
3 participants
You can’t perform that action at this time.