feat: merkle tree verifier implementation to support all numbers of leaves #253

rach-id · 2023-11-01T17:21:34Z

Overview

Closes #249

The implementation is taken from: https://github.com/celestiaorg/celestia-core/blob/0498541b8db00c7fefa918d906877ef2ee0a3710/crypto/merkle/proof.go#L166-L197

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

Summary by CodeRabbit

The existing bullet-point list is still valid based on the provided information. No changes are required.

…eaves

rach-id · 2023-11-02T10:16:38Z

@coderabbitai review

coderabbitai · 2023-11-02T10:16:45Z

Walkthrough

The changes primarily revolve around the enhancement of the BinaryMerkleTree library and its associated test suite. New utility functions have been introduced in Utils.sol and redundant ones removed from NamespaceMerkleTree.sol. The computeRootHash function in BinaryMerkleTree.sol has been improved to handle edge cases and a new slice function has been added. The test suite has been expanded with new tests for verifying leaf nodes and invalid proofs.

Changes

File Path	Summary
`src/lib/tree/Utils.sol`	Introduced two new utility functions: `_bitsLen(uint256)` and `_getSplitPoint(uint256)`.
`src/lib/tree/.../NamespaceMerkleTree.sol`	Removed two private functions `_bitsLen` and `_getSplitPoint`.
`src/lib/tree/binary/BinaryMerkleTree.sol`	Modified `computeRootHash` function to handle edge cases and added a new `slice` function.
`src/lib/tree/binary/test/BinaryMerkleTree.t.sol`	Added new test functions for verifying leaf nodes and invalid proofs.

🐇🍂
As the leaves fall, so do the bugs in our code,
With each commit, we lighten the load.
Autumn winds whisper, "Well done, my friend",
For every start, there must be an end.
So here's to the changes, big and small,
In the grand scheme, they matter all.
Now let's hop to it, there's more to amend,
In the world of code, the fun never ends! 🍁🎉

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai help to get help.
@coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

coderabbitai

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 9dade12 and a413aeb.

Files selected for processing (5)

src/lib/tree/Utils.sol (1 hunks)
src/lib/tree/binary/BinaryMerkleTree.sol (1 hunks)
src/lib/tree/binary/test/BinaryMerkleTree.t.sol (2 hunks)
src/lib/tree/namespace/NamespaceMerkleTree.sol (1 hunks)
wrappers/Blobstream.sol/wrapper.go (1 hunks)

Files not summarized due to errors (1)

wrappers/Blobstream.sol/wrapper.go: Error: Message exceeds token limit

Files not reviewed due to errors (1)

wrappers/Blobstream.sol/wrapper.go (Error: diff too large)

Files skipped from review due to trivial changes (3)

src/lib/tree/Utils.sol
src/lib/tree/binary/test/BinaryMerkleTree.t.sol
src/lib/tree/namespace/NamespaceMerkleTree.sol

Additional comments: 2

src/lib/tree/binary/BinaryMerkleTree.sol (2)

42-47: The verify function has been updated to handle cases where the number of leaves is 0 or 1. It checks the length of sideNodes to ensure it is correct. The function also computes the root hash by calling the computeRootHash function.

58-78: The computeRootHash function has been updated to handle cases where the number of leaves is 0 or 1. It checks the length of sideNodes to ensure it is correct. The function also recursively computes the root hash by splitting the tree into left and right subtrees.

src/lib/tree/binary/BinaryMerkleTree.sol

…ix-merkle-implementatio

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between a413aeb and 01e669f.

Files selected for processing (1)

src/lib/tree/binary/BinaryMerkleTree.sol (1 hunks)

Additional comments: 3

src/lib/tree/binary/BinaryMerkleTree.sol (3)

42-47: The verify function has been updated to use the new computeRootHash function. Ensure that the new function behaves as expected and doesn't introduce any regressions.

53-78: The computeRootHash function has been updated to handle cases with 0 or 1 leaves and to recursively compute the root hash by splitting the tree into left and right subtrees. This is a significant change and should be thoroughly tested.

80-93: The slice function has been added to create a slice of bytes32 from a given range of elements in an array. This function should be tested to ensure it behaves as expected.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 01e669f and b880871.

Files selected for processing (1)

src/lib/tree/binary/BinaryMerkleTree.sol (1 hunks)

Additional comments: 3

src/lib/tree/binary/BinaryMerkleTree.sol (3)

42-48: The verify function has been updated to use the new computeRootHash function. Ensure that the function is being called correctly and that the returned value is being used as expected.

53-78: The computeRootHash function has been updated to handle cases with 0 or 1 leaves and to recursively compute the root hash by splitting the tree into left and right subtrees. This is a significant change and should be thoroughly tested.

80-96: The new slice function creates a slice of bytes32 from a given range of elements in an array. This function should be tested to ensure it behaves as expected, especially in edge cases (e.g., when _begin is equal to _end, when _begin is greater than _end, when _begin or _end are out of bounds, etc.).

evan-forbes

left one comment for adding tests for false positives, other than that this implementation makes sense to me

evan-forbes · 2023-11-02T13:15:34Z

src/lib/tree/binary/test/BinaryMerkleTree.t.sol

+        BinaryMerkleProof memory proof = BinaryMerkleProof(sideNodes, key, numLeaves);
+        bytes memory data = hex"03";
+        bool isValid = BinaryMerkleTree.verify(root, proof, data);
+        assertTrue(isValid);


can we also include a test where we expect the verification to fail?

It was a good idea to add invalid proofs 👍 Found a bug :D

actually not a bug, but an edge case:

Assume you have a merkle tree of 5 leaves. When creating the proof, if you set the number of leaves = [5, 8], the proof going to pass even if {6, 7, 8} are invalid values because with those numbers of leaves, the verifier will behave the same.

Verified with the implementation of tendermint and it also has the same behaviour.

Could you expand on this? What does number of leaves = [5, 8] mean? That's a range, not a number.

My bad, i meant any number from that range will result in a valid proof. The verifier won't complain about it.

So if you have a tree of 5 leaves, and in the proof you set the total number to 7, the proof is still considered valid

…ix-merkle-implementatio

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between b880871 and a8855d0.

Files selected for processing (3)

src/lib/tree/binary/BinaryMerkleTree.sol (1 hunks)
src/lib/tree/binary/test/BinaryMerkleTree.t.sol (2 hunks)
wrappers/Blobstream.sol/wrapper.go (1 hunks)

Files not summarized due to errors (1)

wrappers/Blobstream.sol/wrapper.go: Error: Message exceeds token limit

Files not reviewed due to errors (1)

wrappers/Blobstream.sol/wrapper.go (Error: diff too large)

Files skipped from review due to trivial changes (1)

src/lib/tree/binary/test/BinaryMerkleTree.t.sol

Additional comments: 3

src/lib/tree/binary/BinaryMerkleTree.sol (3)

42-48: The verify function has been updated to handle the case when the number of leaves is 0 or 1. It checks for the correct number of inner hashes and returns the root hash accordingly. This is a good update as it makes the function more robust and able to handle edge cases.

53-78: The computeRootHash function has been updated to handle the case when the number of leaves is 0 or 1. It now checks for the correct number of inner hashes and returns the root hash accordingly. This is a good update as it makes the function more robust and able to handle edge cases.

80-99: The slice function has been added to create a slice of bytes32 from a given range of elements in an array. This function is useful for creating subarrays from a larger array, which can be useful in many different contexts. However, it's important to ensure that the _begin and _end parameters are validated to prevent out-of-bounds errors.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between a8855d0 and de1f723.

Files selected for processing (1)

src/lib/tree/binary/test/BinaryMerkleTree.t.sol (2 hunks)

Files skipped from review due to trivial changes (1)

src/lib/tree/binary/test/BinaryMerkleTree.t.sol

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between de1f723 and dc33cea.

Files selected for processing (1)

wrappers/Blobstream.sol/wrapper.go (1 hunks)

Files not summarized due to errors (1)

wrappers/Blobstream.sol/wrapper.go: Error: Message exceeds token limit

Files not reviewed due to errors (1)

wrappers/Blobstream.sol/wrapper.go (Error: diff too large)

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between dc33cea and 2e6acb3.

Files selected for processing (1)

wrappers/Blobstream.sol/wrapper.go (1 hunks)

Files not summarized due to errors (1)

wrappers/Blobstream.sol/wrapper.go: Error: Message exceeds token limit

Files not reviewed due to errors (1)

wrappers/Blobstream.sol/wrapper.go (Error: diff too large)

evan-forbes

lgtm. given the importance of this lib I think it would be worth setting up a fuzzer.

rach-id · 2023-11-06T15:35:19Z

lgtm. given the importance of this lib I think it would be worth setting up a fuzzer.

#256

…ix-merkle-implementatio

# Conflicts: # wrappers/Blobstream.sol/wrapper.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 2e6acb3 and 85bc59d.

Files selected for processing (1)

src/lib/tree/binary/test/BinaryMerkleTree.t.sol (4 hunks)

Additional comments: 12

src/lib/tree/binary/test/BinaryMerkleTree.t.sol (12)

2-8: The new import import "forge-std/Vm.sol"; is added. Ensure that this library is available in the project dependencies.

107-120: The new test function testVerifyLeafTwoOfEight is added. It seems to be testing the verification of the second leaf in a Merkle tree with eight leaves.

122-135: The new test function testVerifyLeafThreeOfEight is added. It seems to be testing the verification of the third leaf in a Merkle tree with eight leaves.

173-186: The new test function testVerifyProofOfFiveLeaves is added. It seems to be testing the verification of a proof with five leaves.

188-202: The new test function testVerifyInvalidProofRoot is added. It seems to be testing the verification of a proof with an invalid root.

204-218: The new test function testVerifyInvalidProofKey is added. It seems to be testing the verification of a proof with an invalid key.

220-234: The new test function testVerifyInvalidProofNumberOfLeaves is added. It seems to be testing the verification of a proof with an invalid number of leaves.

236-250: The new test function testVerifyInvalidProofSideNodes is added. It seems to be testing the verification of a proof with invalid side nodes.

252-266: The new test function testVerifyInvalidProofData is added. It seems to be testing the verification of a proof with invalid data.

268-279: The new test function testValidSlice is added. It seems to be testing the slicing of an array with valid indices.

281-290: The new test function testInvalidSliceBeginEnd is added. It seems to be testing the slicing of an array with invalid indices where the beginning index is greater than the end index.

292-301: The new test function testOutOfBoundsSlice is added. It seems to be testing the slicing of an array with indices that are out of bounds.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 85bc59d and 5651fa5.

Files selected for processing (1)

wrappers/Blobstream.sol/wrapper.go (1 hunks)

Files not summarized due to errors (1)

wrappers/Blobstream.sol/wrapper.go: Error: Message exceeds token limit

Files not reviewed due to errors (1)

wrappers/Blobstream.sol/wrapper.go (Error: diff too large)

evan-forbes

refreshing this approve

rach-id · 2023-11-09T12:49:43Z

We will go on and merge this PR as it's blocking mocha's deployment. The only concern about this implementation is documented in #259. We can open subsequent PRs if any issues arise with it.

adlerjohn · 2023-11-21T20:01:28Z

src/lib/tree/binary/BinaryMerkleTree.sol

-                return false;
+        if (numLeaves == 1) {
+            if (sideNodes.length != 0) {
+                revert("unexpected inner hashes");


Do we want to revert in this library?

I'll create an issue to return error codes instead, thanks 👍

feat: merkle tree verifier implementation to support all numbers of l…

fbb2432

…eaves

rach-id added enhancement New feature or request audit labels Nov 1, 2023

rach-id requested a review from evan-forbes November 1, 2023 17:21

rach-id self-assigned this Nov 1, 2023

rach-id requested a review from adlerjohn as a code owner November 1, 2023 17:21

rach-id added 3 commits November 1, 2023 17:24

chore: wrapper gen

de3150c

Merge branch 'master' into fix-merkle-implementatio

5c100fb

chore: wrapper gen

a413aeb

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

src/lib/tree/binary/BinaryMerkleTree.sol Outdated Show resolved Hide resolved

rach-id added 2 commits November 2, 2023 11:41

fix: bug off by one

946d5ee

Merge remote-tracking branch 'origin/fix-merkle-implementatio' into f…

01e669f

…ix-merkle-implementatio

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

fix: add _begin and _end checks

b880871

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

rach-id added 2 commits November 2, 2023 12:36

fix: add _begin and _end checks

d854b08

chore: generate wrapper

61baa70

evan-forbes reviewed Nov 2, 2023

View reviewed changes

rach-id added 3 commits November 2, 2023 18:12

chore: fix slice

fe5bf8e

Merge remote-tracking branch 'origin/fix-merkle-implementatio' into f…

bb5462e

…ix-merkle-implementatio

test: add invalid proofs tests

a8855d0

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

fix: bump number of leaves

de1f723

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

chore: wrapper gen

dc33cea

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

rach-id added 2 commits November 2, 2023 22:51

Merge branch 'master' into fix-merkle-implementatio

a8cf071

chore: regenereate wrappers

2e6acb3

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

evan-forbes previously approved these changes Nov 6, 2023

View reviewed changes

rach-id mentioned this pull request Nov 6, 2023

Setup fuzzer for binary merkle verifier #256

Open

rach-id added 2 commits November 7, 2023 10:42

test: add slice tests

f7f7d5b

Merge remote-tracking branch 'origin/fix-merkle-implementatio' into f…

85bc59d

…ix-merkle-implementatio

rach-id dismissed evan-forbes’s stale review via 85bc59d November 7, 2023 09:42

Merge branch 'master' into fix-merkle-implementatio

5215a18

# Conflicts: # wrappers/Blobstream.sol/wrapper.go

coderabbitai bot reviewed Nov 7, 2023

View reviewed changes

chore: regenerate wrapers

5651fa5

coderabbitai bot reviewed Nov 7, 2023

View reviewed changes

evan-forbes approved these changes Nov 7, 2023

View reviewed changes

rach-id mentioned this pull request Nov 9, 2023

Binary merkle proofs accepting multiple total values as correct #259

Open

rach-id merged commit 4c8099e into celestiaorg:master Nov 9, 2023
11 checks passed

adlerjohn reviewed Nov 21, 2023

View reviewed changes

rach-id mentioned this pull request Nov 22, 2023

return error codes in the binary merkle tree implementation #266

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: merkle tree verifier implementation to support all numbers of leaves #253

feat: merkle tree verifier implementation to support all numbers of leaves #253

rach-id commented Nov 1, 2023 •

edited by coderabbitai bot

Loading

rach-id commented Nov 2, 2023

coderabbitai bot commented Nov 2, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

evan-forbes left a comment

evan-forbes Nov 2, 2023

rach-id Nov 2, 2023

rach-id Nov 2, 2023

rach-id Nov 2, 2023

adlerjohn Nov 6, 2023

rach-id Nov 6, 2023

rach-id Nov 9, 2023

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

evan-forbes left a comment

rach-id commented Nov 6, 2023

coderabbitai bot left a comment

coderabbitai bot left a comment

evan-forbes left a comment

rach-id commented Nov 9, 2023

adlerjohn Nov 21, 2023

rach-id Nov 22, 2023

feat: merkle tree verifier implementation to support all numbers of leaves #253

feat: merkle tree verifier implementation to support all numbers of leaves #253

Conversation

rach-id commented Nov 1, 2023 • edited by coderabbitai bot Loading

Overview

Checklist

Summary by CodeRabbit

rach-id commented Nov 2, 2023

coderabbitai bot commented Nov 2, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

rach-id commented Nov 6, 2023

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

rach-id commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id commented Nov 1, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 2, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)