-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
Resolves: OpenST#716 See also: OpenST#718, OpenST#719
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.
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
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.
There was a problem hiding this 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.
- good that we keep MPP in original form first; it might very well be valid change to align
toBytes
ortoData
, but let's do so after further analysis and deliberation - good that path is now generated by the fuzzer
Reviewing 🦑 |
There was a problem hiding this 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.
tools/fuzzy_proof_generator_tool/test/Util/nibblesToBuffer.test.ts
Outdated
Show resolved
Hide resolved
- 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.
There was a problem hiding this 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/test/FuzzyProofGenerator/assertPatternValidity.test.ts
Show resolved
Hide resolved
tools/fuzzy_proof_generator_tool/test/FuzzyProofGenerator/generateByPattern.js
Show resolved
Hide resolved
- Renames NibblesUtil to Nibbles. - Fixes bugs and stronger requirement checks for FuzzyProofGenerator::generateByPattern(). - Enhances code documentation and comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🦖
Resolves: #716
See also: #718, #719