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

Implements a fuzzer that generates the valid Merkel Patricia Proofs. #729

Merged
merged 20 commits into from
May 17, 2019

Conversation

pgev
Copy link
Contributor

@pgev pgev commented May 8, 2019

Resolves: #716
See also: #718, #719

Paruyr Gevorgyan added 7 commits May 8, 2019 09:39
Fixes failing test cases after linter error cleanup.
Replaces string usages by Buffer.
  - .gitignore ignores generated files from typescript files.
  - MerklePatriciaProof::verify and MerklePatriciaProof::verifyDebug
    functions uses toData instead of toBytes during comparsion agaist
    value.
  - Improved pattern validity to correctly handle ending branch node.
  - Fixed an issue with branch, extension and leaf nodes
    serialization.
  - .travis.yml runs fuzzy_proof_generator tests.
Copy link
Contributor

@benjaminbollen benjaminbollen left a comment

Choose a reason for hiding this comment

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

Putting a veto on this PR until further clarified

@@ -129,7 +129,7 @@ library MerklePatriciaProof {

if(currentNodeList.length == 17) {
if(pathPtr == path.length) {
if(keccak256(abi.encodePacked(RLP.toBytes(currentNodeList[16]))) == value) {
if(keccak256(abi.encodePacked(RLP.toData(currentNodeList[16]))) == value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this PR, focused on tests, should not contain changes to the implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Paruyr Gevorgyan added 7 commits May 13, 2019 14:27
In addition, contains the followings:

  - Store values in rlp encoding.
  - Introduces sanity tests for FuzzyProofGenerator::generate
    and FuzzyProofGenerator::generateByPattern functions.
  - Adds run.sh script to run FuzzyProofGenerator tests.
  - Updates .travis.yml to run FuzzyProofGenerator tests.
In addition, within './tools/fuzzy_proof_generator_tool/test/run.sh'
tests are disabled as currently failing.
@pgev pgev marked this pull request as ready for review May 13, 2019 12:15
@pgev pgev changed the title Implements draft version of fuzzy proof generator Implements a fuzzer that generates the valid Merkel Patricia Proofs. May 14, 2019
Copy link
Contributor

@benjaminbollen benjaminbollen left a comment

Choose a reason for hiding this comment

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

I haven't reviewed in full; but removing my veto on changes to the MPP library.

  1. good that we keep MPP in original form first; it might very well be valid change to align toBytes or toData, but let's do so after further analysis and deliberation
  2. good that path is now generated by the fuzzer

@schemar
Copy link
Contributor

schemar commented May 15, 2019

Reviewing 🦑

Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

As always, really good work @pgev, thank you! 🐙

It is quite a big PR and I hope I didn't miss anything. I took the time to do a proper review. A lot of my comments are "maybe"s and questions. Feel free to disregard those.

See all feedback in-line.

.gitignore Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
tools/fuzzy_proof_generator_tool/src/BranchNode.ts Outdated Show resolved Hide resolved
tools/fuzzy_proof_generator_tool/src/BranchNode.ts Outdated Show resolved Hide resolved
tools/fuzzy_proof_generator_tool/test/run.sh Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@schemar schemar removed their assignment May 16, 2019
  - Fixes a bug in FuzzyProofGenerator::processExtension() function.

  - Moves Util::encodeCompactExtensionNode() function to
    ExtensionNode::encodeCompact().
  - Moves Util::encodeCompactLeafNode() function to
    LeafNode::encodeCompact().
  - Moves Util::generateRandomKeccak256 function to
    FuzzyProofGenerator::generateRandomKeccak256().
  - Moves nibbles related functionality from Util.ts to NibblesUtil.ts.
  - Removes Util.ts.

  - Improves documentation for low-level bitwise operations
    within NibblesUtil.ts.

  - assert.throws() instances check against a specific error message.
  - Enhances generateByPattern test suites.

  - Fixes lint errors for tools/fuzzy_proof_generator ts files.
  - Checks "proof_generation" dir within `npm run lint`.
  - Disables eslint @typescript-eslint/camelcase" rule.
  - Adds .eslintignore file.

  - Updates dev dependencies within package.json.
Copy link
Contributor Author

@pgev pgev left a comment

Choose a reason for hiding this comment

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

Thanks for a great review Martin. I've updated code and responded to your comments.

tools/fuzzy_proof_generator_tool/src/BranchNode.ts Outdated Show resolved Hide resolved
tools/fuzzy_proof_generator_tool/src/BranchNode.ts Outdated Show resolved Hide resolved
tools/fuzzy_proof_generator_tool/src/Types.ts Show resolved Hide resolved
tools/fuzzy_proof_generator_tool/src/Util.ts Outdated Show resolved Hide resolved
tools/fuzzy_proof_generator_tool/test/run.sh Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
Paruyr Gevorgyan added 3 commits May 17, 2019 16:58
  - Renames NibblesUtil to Nibbles.
  - Fixes bugs and stronger requirement checks for
    FuzzyProofGenerator::generateByPattern().
  - Enhances code documentation and comments.
Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

Awesome 🦖

@schemar schemar merged commit 2637c92 into OpenST:develop May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write a fuzzer that generates the valid Merkel Patricia Proof
3 participants