Merkle proof library with tests and docs #260
Conversation
|
@maraoz Bump! |
|
Sorry, we're overwhelmed with the amount of PRs! Will try to review next week :) |
| for (uint256 i = 32; i <= _proof.length; i += 32) { | ||
| assembly { | ||
| // Load the current element of the proof | ||
| proofElement := mload(add(_proof, i)) |
There was a problem hiding this comment.
WDYT about adding a check that proof.length is actually multiple of 4, so we don't risk loading into memory unexpected data in this line? I'm not quite sure about what parts of memory would be readable this way, though most likely it's the rest of the params, so this shouldn't be an issue; but it's a cheap check and it doesn't hurt to be extra careful.
There was a problem hiding this comment.
I agree that a check for proof.length just to be careful makes sense. Though, I think the check should be for if proof.length is a multiple of 32 since proof is supposed to be an array of bytes32 hashes?
There was a problem hiding this comment.
Absolutely, my bad! The check is for 32 as you say.
|
Added a check that the Merkle proof length is actually a multiple of 32 |
| if (_proof.length % 32 != 0) return false; | ||
|
|
||
| bytes32 proofElement; | ||
| bytes32 computedHash = _leaf; |
There was a problem hiding this comment.
Comparing this implementation to https://github.com/raiden-network/raiden/blob/master/raiden/smart_contracts/NettingChannelLibrary.sol#L268, it seems the only difference is whether to hash the leaf element before entering the loop.
I guess the difference is on how you build the tree: is it the tree who hashes every element initially to form the leaves, or should the tree's user do it (as is the case here)?
There was a problem hiding this comment.
I think it makes sense for _leaf to be the hash of the leaf element instead of the actual leaf element since it avoids an additional keccak256 operation on-chain - the more logic that can be pushed off-chain the better imo. Then, the comments/documentation can make it clearer that _leaf should be the hash of the leaf element and not the actual leaf element.
test/MerkleProof.js
Outdated
|
|
||
| describe("verifyProof", function() { | ||
| it("should return true for a valid Merkle proof", async function() { | ||
| const elements = ["a", "b", "c", "d"].map(el => sha3(el)); |
There was a problem hiding this comment.
See the comment above. Shouldn't the MerkleTree class be responsible for initially hashing the elements?
There was a problem hiding this comment.
Good point, I think that makes sense from a usability standpoint. A user can generate a merkle tree off-chain by passing in an array of elements and then when the user wants to submit a proof the user uses the same merkle tree to generate a proof and submits it along with the hash of the element to be proved
|
Great contribution @yondonfu! |
Merkle proof library with tests and docs
Addresses #254
merkleTree.jscontains a helper Merkle tree class that is used to generate Merkle trees, roots and proofs in the JS tests. When generating the Merkle tree, leaves are sorted and pre-images are also pair-wise sorted before being concatenated/hashed to create the parent of a pair of pre-images.verifyProofinMerkleProof.solassumes that the Merkle root provided is from a Merkle tree with sorted leaves and pair-wise sorted pre-images.https://github.com/ameensol/merkle-tree-solidity/blob/master/src/MerkleProof.sol also contains a way to verify Merkle proofs that maintains the order of the leaves rather than sorting them, but verifying the proof in Solidity would also require the index of the leaf in the tree as a parameter. This initial implementation does not include this variation of verifying a Merkle proof, but depending on the needs of the community, it can be added as well.