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

Use int64 in memoryMerkleTree #321

Merged
merged 1 commit into from Jan 27, 2017
Merged

Use int64 in memoryMerkleTree #321

merged 1 commit into from Jan 27, 2017

Conversation

gdbelvin
Copy link
Contributor

@gdbelvin gdbelvin commented Jan 25, 2017

This is a PR in preparation for building a LogVerifier.
We need a common set of tree functions to use in the merkle package

@Martin2112
Copy link
Contributor

Can you investigate the travis failure first.

@gdbelvin
Copy link
Contributor Author

The Travis failure needs to be fixed by a more comprehensive refactor of the NewLogStorage API.
Happy to chat about #324

@Martin2112
Copy link
Contributor

Fixing the tests should be the priority to avoid introducing additional issues.

@gdbelvin
Copy link
Contributor Author

Fixing in #327

@gdbelvin
Copy link
Contributor Author

The tests are passing now. Is this PR ready to go?

@Martin2112
Copy link
Contributor

Martin2112 commented Jan 27, 2017 via email

Copy link
Contributor

@Martin2112 Martin2112 left a comment

Choose a reason for hiding this comment

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

This looks OK. However in case you're planning to test at scale I should probably point out that this is a complete tree and there won't be enough RAM available to construct a tree large enough to require int64.

@gdbelvin gdbelvin merged commit 4d3935d into google:master Jan 27, 2017
@gdbelvin
Copy link
Contributor Author

I'm adding a proper LogVerifier next.
Once we have a log verifier we can replace instances of the MemoryMerkleTree -> compare inclusion proofs with a proper InclusionProof check.

@gdbelvin gdbelvin deleted the merkle branch July 4, 2017 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants