feat: Matched Sequential Proof Implementation #749
Conversation
## Walkthrough
This change is a comprehensive refactor and extension of the Merkle tree implementation and its associated hashing and proof utilities. The update introduces explicit domain separation for hash functions, new utility functions for integer encoding, and a new approach to root hashing that incorporates leaf count metadata. The Merkle proof structure is redesigned, moving from indexed values and node lists to a model based on a starting index, a slice of leaves, and an array of proof elements. All related test suites are updated to reflect these structural and functional changes, with new test cases added for the new API and proof format.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------|---------------|
| `pkg/merkle/hash.go`, `pkg/merkle/hash_test.go` | Refactored hashing utilities: split and renamed hash functions for leaves, node pairs, pairless nodes, and root with metadata; added integer-to-bytes utilities; removed `HashEmptyLeaf`; updated and expanded tests to match new API using raw byte slices. |
| `pkg/merkle/proof.go`, `pkg/merkle/proof_internal_test.go`, `pkg/merkle/proof_test.go` | Redesigned `MultiProof` structure to use starting index, leaves, and proof elements; updated validation, root computation, and proof generation logic; replaced indexed-value-based tests with new structure tests; removed legacy error and duplicate index tests; added detailed sequential proof tests. |
| `pkg/merkle/tree.go`, `pkg/merkle/tree_internal_test.go`, `pkg/merkle/tree_test.go` | Changed `Node` type to byte slice alias; unified tree indexing to zero-based with root at index 0; removed leaf padding and balancing; added nil leaf validation and new error types; updated tree construction and proof generation to reflect new indexing and proof format; revised all related tests for naming, indexing, and logic consistency; added tests for nil leaves and fixed sample trees. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant MerkleTree
participant HashUtils
participant MultiProof
Client->>MerkleTree: NewMerkleTree(leaves)
MerkleTree->>HashUtils: HashLeaf(leaf) for each leaf
MerkleTree->>MerkleTree: makeTree(leafNodes)
MerkleTree->>HashUtils: HashNodePair / HashPairlessNode
MerkleTree->>HashUtils: HashRoot(leafCount, root)
MerkleTree-->>Client: MerkleTree instance
Client->>MerkleTree: GenerateMultiProofWithIndices(indices)
MerkleTree->>MerkleTree: makeProof(indices)
MerkleTree->>MultiProof: NewMerkleProof(startingIndex, leaves, proofElements)
MerkleTree-->>Client: MultiProof
Client->>MultiProof: Verify(root)
MultiProof->>MultiProof: validate()
MultiProof->>MultiProof: computeRoot()
MultiProof-->>Client: verification resultPossibly related PRs
Suggested reviewers
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
pkg/merkle/tree.go (1)
15-18: 🛠️ Refactor suggestionPackage comment is now stale – tree is zero-indexed
Earlier refactor moved the root to index 0, but the comment still says the tree is 1-indexed and the root is at index 1.
Out-of-date comments are a common source of confusion; please update to reflect the current layout:-// - The tree is 1-indexed, so root is at index 1. -// - The internal nodes are at index 1 to N-1. -// - The leaves are at index N to 2N-1. +// - The root is stored at index 0. +// - Internal nodes occupy indices 1 to N-1 (where N is the next power-of-two leaf count). +// - Leaves occupy indices N to 2N-1.
♻️ Duplicate comments (1)
pkg/merkle/tree_test.go (1)
515-520: Same parameter order issue as aboveSwap the parameters so that
7is expected andtree.LeafCount()is actual.
🧹 Nitpick comments (15)
pkg/merkle/tree_internal_test.go (1)
95-99: Rename comment – no padding is expected anymoreThe comment
// Test padding.is now misleading because the implementation no longer pads leaves. Renaming to something like// Verify Leaves() returns the original slice without modification.will keep the intent clear.pkg/merkle/proof_test.go (2)
178-184: Test description does not match data – indices are already orderedThe case is labelled “out-of-order indices” but the slice
{2, 3, 4}is strictly ordered.
Either change the label or provide an actually shuffled slice (e.g.{4, 2, 3}) to ensure the code correctly handles unsorted input.
425-426: Clarify or remove stray TODO
// TODO: Not sure what this is testingindicates uncertainty in the test purpose.
Either document the intent (what behaviour is being validated) or drop the test to keep the suite purposeful and maintainable.pkg/merkle/proof_internal_test.go (1)
245-250: Test case name does not match the condition being assertedThe case is labelled “duplicate indices” but the slice
{0, 1, 3, 2}is merely unsorted – it contains no duplicates.
This can be misleading when the test fails and when future readers look for coverage on actual duplicates.- name: "duplicate indices", + name: "unsorted indices",pkg/merkle/hash_test.go (2)
38-47: Use the correct format verb for byte-slicesEvery failing assertion will currently print
%!d([]uint8=…), because the%dverb expects an integer, not a slice.
Switch to%x(hex) or%v(default Go formatting) for clarity.- "Hash of %d = %d, expected %d", - tt.input, - result, - tt.expected, + "Hash of %x = %x, expected %x", + tt.input, + result, + tt.expected,Replicate the same change in the other helper messages (
HashLeaf,HashNodePair,HashPairlessNode,HashRoot,IntTo32Bytes,BytesToBigInt) to avoid misleading output across the file.Also applies to: 80-91, 122-133, 166-176, 208-219, 252-264, 295-307
1-8: Run gofumpt / golines to satisfy lint-go formatting checksThe CI reports
File is not properly formatted (gofumpt)andgolinesviolations for this file.
Running:gofumpt -w pkg/merkle/hash_test.go golines -w pkg/merkle/hash_test.gowill re-wrap the long composite literals and trim unnecessary whitespace, keeping the tests functionally identical while passing the style gate.
🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 5-5:
File is not properly formatted (gofumpt)pkg/merkle/tree.go (2)
294-296: 32-bit build safety – mask shift larger than word size
n |= n >> 32is harmless on 64-bit builds but is a no-op (and flagged by vet) on 32-bit architectures because the shift equals the type width.
To keep the helper strictly portable you can gate the extra shift:n |= n >> 16 +#if ^uint(0)>>32 != 0 // compile-time check for 64-bit words + n |= n >> 32 +#endif(or use a looped approach).
If 32-bit support is irrelevant for the project you can ignore this, but documenting the assumption would still be valuable.
70-85: Duplicate validation logic – consider relying onvalidateIndicesonly
GenerateMultiProofWithIndicesmanually checks bounds and ordering, then immediately callsvalidateIndices, which performs equivalent work.
Maintaining two parallel validations increases cognitive load and the risk of divergence.Either:
- Remove the manual loop here and rely solely on
validateIndices, or- Have
validateIndicesexported and call it directly to keep logic in one place.This will simplify the method and avoid double scanning of the slice.
pkg/merkle/tree_test.go (3)
86-90: **Label/parameter mismatch in un-balanced test cases **
"FiveLeaves"is given6leaves and"SevenLeaves"is given9.
Besides being confusing, this makes the assertions harder to reason about and mis-leads future maintainers. Align the names (or the numbers).
120-121: Assertion message is staleThe size is validated with
balancedLeafCount << 1, yet the failure message still says “balancedLeafCount + tc.leafCount”.
Update the string to reflect the actual expectation to avoid red-herrings when the test fails.
545-547: Ignored error fromhex.DecodeStringhides malformed test vectorsSilently discarding the decode error makes the test succeed with a
nilslice if a typo creeps in.
Fail fast instead:func getBytesFromHexString(s string) []byte { - decoded, _ := hex.DecodeString(s) - return decoded + decoded, err := hex.DecodeString(s) + if err != nil { + panic(fmt.Errorf("invalid hex test vector %q: %w", s, err)) + } + return decoded }pkg/merkle/proof.go (4)
101-105:ErrNilProofcheck misses the empty-slice caseAfter fixing the bounds check above, an empty slice will already be handled, but the loop here still allows zero-length but non-nil elements (e.g.
[]byte{}) to pass.
If those should be forbidden, add an explicit length check.
110-118: Return value comment is stale
getNextProofElementreturnsErrNoProofswhen exhausted, but the doc-comment says “safely retrieves the next proof” without mentioning the failure mode.
Keeping comments in sync avoids surprises.
177-178: Use idiomatic infinite loop formStaticcheck S1006 flags
for true {}– Go style prefersfor {}.- for true { + for {🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 177-177:
S1006: should use for {} instead of for true {} (staticcheck)
154-159:leafCountis trusted directly from the proof – consider sanity boundsA malicious proof could encode a gigantic
leafCountthat causesCalculateBalancedNodesCountto allocate or shift huge numbers.
Defensive bound-checking (e.g.if leafCount > maxAllowed) would reduce the attack surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/merkle/hash.go(3 hunks)pkg/merkle/hash_test.go(1 hunks)pkg/merkle/proof.go(4 hunks)pkg/merkle/proof_internal_test.go(5 hunks)pkg/merkle/proof_test.go(15 hunks)pkg/merkle/tree.go(10 hunks)pkg/merkle/tree_internal_test.go(3 hunks)pkg/merkle/tree_test.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
pkg/merkle/tree_internal_test.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:291-311
Timestamp: 2025-04-17T11:38:12.315Z
Learning: In the Merkle tree implementation, padded leaves (added to make leaf count a power of 2) are considered valid and should be taken into account for bounds checking. These padded leaves are initialized as empty byte arrays (`[]byte{}`) and can be accessed through indices. The validation should use `balancedLeafCount` (which includes padding) rather than just `leafCount` (actual leaves).
pkg/merkle/tree_test.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:291-311
Timestamp: 2025-04-17T11:38:12.315Z
Learning: In the Merkle tree implementation, padded leaves (added to make leaf count a power of 2) are considered valid and should be taken into account for bounds checking. These padded leaves are initialized as empty byte arrays (`[]byte{}`) and can be accessed through indices. The validation should use `balancedLeafCount` (which includes padding) rather than just `leafCount` (actual leaves).
🧬 Code Graph Analysis (4)
pkg/merkle/tree_internal_test.go (2)
pkg/merkle/tree.go (2)
Leaf(11-11)Node(9-9)pkg/merkle/hash.go (3)
HashLeaf(27-35)HashNodePair(37-46)HashPairlessNode(48-56)
pkg/merkle/proof_internal_test.go (3)
pkg/merkle/tree.go (4)
Leaf(11-11)ErrNoLeaves(25-25)ErrNilLeaf(27-27)ErrIndicesNotSorted(29-29)pkg/merkle/proof.go (4)
ProofElement(25-25)MultiProof(27-31)ErrInvalidLeafCount(17-17)ErrNilProof(18-18)pkg/merkle/hash.go (2)
IntTo32Bytes(74-82)HashLeaf(27-35)
pkg/merkle/hash_test.go (1)
pkg/merkle/hash.go (7)
Hash(21-25)HashLeaf(27-35)HashNodePair(37-46)HashPairlessNode(48-56)HashRoot(58-72)IntTo32Bytes(74-82)BytesToBigInt(84-86)
pkg/merkle/proof.go (2)
pkg/merkle/tree.go (5)
Leaf(11-11)ErrNoLeaves(25-25)ErrNilLeaf(27-27)CalculateBalancedNodesCount(271-281)ErrIndicesNotSorted(29-29)pkg/merkle/hash.go (4)
BytesToBigInt(84-86)HashRoot(58-72)HashPairlessNode(48-56)HashNodePair(37-46)
🪛 GitHub Check: Lint-Go
pkg/merkle/hash.go
[failure] 4-4:
File is not properly formatted (gofumpt)
pkg/merkle/hash_test.go
[failure] 5-5:
File is not properly formatted (gofumpt)
[failure] 18-18:
File is not properly formatted (golines)
pkg/merkle/proof.go
[failure] 177-177:
S1006: should use for {} instead of for true {} (staticcheck)
🪛 GitHub Actions: Lint
pkg/merkle/hash.go
[error] 4-4: File is not properly formatted (gofumpt)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Code Review
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Upgrade Tests
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
Implement sequential Merkle proof verification by redesigning the tree and proof structures in the
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/merkle/proof.go (1)
73-82:⚠️ Potential issueMissing validation for empty proofElements
The function attempts to access
p.proofElements[0]without first checking if the slice is non-empty, which could cause a panic.Add a check for empty proofElements before accessing the first element:
func (p *MultiProof) validate() error { if p.startingIndex < 0 { return ErrInvalidStartingIndex } + if len(p.proofElements) == 0 { + return ErrNoProofs + } leafCount := BytesToBigInt(p.proofElements[0])
🧹 Nitpick comments (2)
pkg/merkle/proof.go (2)
174-174: Use idiomatic Go for infinite loopsThe current syntax
for true {should be replaced withfor {which is the idiomatic way to write infinite loops in Go.- for true { + for {🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 174-174:
S1006: should use for {} instead of for true {} (staticcheck)
149-262: Innovative circular queue approach for proof verificationThe circular queue implementation for proof verification is memory-efficient and elegantly handles the complexities of unbalanced trees. The algorithm efficiently processes nodes level by level, determining when to use proof elements versus previously computed node hashes.
This circular queue approach is particularly well-suited for handling large Merkle trees as it:
- Minimizes memory usage by reusing queue slots
- Processes nodes in a single pass without recursion
- Correctly handles the edge cases in unbalanced trees
Consider adding more detailed comments explaining the queue indices manipulation, as this logic can be subtle and might be challenging for future maintainers.
🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 174-174:
S1006: should use for {} instead of for true {} (staticcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/merkle/proof.go(3 hunks)pkg/merkle/proof_internal_test.go(4 hunks)pkg/merkle/tree.go(11 hunks)pkg/merkle/tree_internal_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/merkle/tree_internal_test.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:291-311
Timestamp: 2025-04-17T11:38:12.315Z
Learning: In the Merkle tree implementation, padded leaves (added to make leaf count a power of 2) are considered valid and should be taken into account for bounds checking. These padded leaves are initialized as empty byte arrays (`[]byte{}`) and can be accessed through indices. The validation should use `balancedLeafCount` (which includes padding) rather than just `leafCount` (actual leaves).
🧬 Code Graph Analysis (3)
pkg/merkle/tree_internal_test.go (2)
pkg/merkle/tree.go (6)
Leaf(11-11)Node(9-9)ErrInvalidRange(30-30)ErrNoIndices(31-31)ErrIndicesOutOfBounds(32-32)ErrIndicesNotSorted(29-29)pkg/merkle/hash.go (3)
HashLeaf(27-35)HashNodePair(37-46)HashPairlessNode(48-56)
pkg/merkle/proof_internal_test.go (3)
pkg/merkle/tree.go (3)
Leaf(11-11)ErrNoLeaves(25-25)ErrNilLeaf(27-27)pkg/merkle/proof.go (4)
ProofElement(22-22)MultiProof(24-28)ErrInvalidLeafCount(15-15)ErrNilProof(16-16)pkg/merkle/hash.go (2)
IntTo32Bytes(74-82)HashLeaf(27-35)
pkg/merkle/proof.go (2)
pkg/merkle/tree.go (5)
Leaf(11-11)ErrNoLeaves(25-25)ErrIndicesOutOfBounds(32-32)ErrNilLeaf(27-27)CalculateBalancedNodesCount(273-283)pkg/merkle/hash.go (4)
BytesToBigInt(84-86)HashRoot(58-72)HashPairlessNode(48-56)HashNodePair(37-46)
🪛 GitHub Check: Lint-Go
pkg/merkle/proof.go
[failure] 174-174:
S1006: should use for {} instead of for true {} (staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
- GitHub Check: Code Review
🔇 Additional comments (25)
pkg/merkle/tree_internal_test.go (5)
10-30: Approve the test function update to match the refactored implementationThe updated test now properly checks the new
makeLeafNodesfunction that replaced the previousmakeLeaves. It's correctly testing both successful leaf node hashing and error handling for empty leaves.
32-81: Test coverage looks good for the makeTree functionThe test has been properly adapted to use the revised hashing functions (
HashNodePairandHashPairlessNode) and handles both balanced and unbalanced trees. The tests correctly check that the tree structure follows the updated zero-based indexing scheme.
96-97: Assertion matches new non-padded leaf behaviorThe updated assertion reflects the design change to not pad leaves to a power of two, which aligns with the PR objective of supporting unbalanced trees.
131-182: Well-structured test for the new makeIndices functionThis new test effectively verifies the correct generation of sequential indices and appropriate error handling for invalid inputs (negative indices, zero or negative counts).
184-235: Thorough test coverage for index validationGood test cases covering the various validation scenarios including:
- Valid indices
- Empty indices
- Out of bounds indices
- Negative indices
- Unsorted/duplicate indices
This ensures the index validation is robust, which is critical for correct proof generation.
pkg/merkle/proof_internal_test.go (6)
13-21: Properly updated test for the redesigned MultiProof structureThe test has been correctly updated to reflect the new MultiProof structure that uses
startingIndex,leaves, andproofElementsinstead of the previous indexed values approach.
27-36: Good coverage of empty leaves validationThis test ensures that the validation logic properly rejects empty leaf sets, which aligns with the PR's objective of handling leaf validation properly.
91-127: Updated test for proof element retrieval mechanismThe renamed test function appropriately tests the updated
getNextProofElementmethod, including the handling of the leaf count element and error detection when proof elements are exhausted.
140-146: Valid test for node queue building with zero-based indexingThe test has been updated to use a starting index of 0 and utilizes the new MultiProof structure with leaves, which properly tests the buildNodeQueue function with the refactored implementation.
154-157: Check queue order with correct balanced leaf count offsetThe test now correctly expects the queue indices to include the balanced leaf count offset, which matches the updated indexing scheme in the implementation.
175-177: Test updated to use sequential indicesThe test now uses sequential indices [0, 1] instead of the previous [0, 2], which is appropriate for testing the sequential proof capabilities introduced in this PR.
pkg/merkle/tree.go (8)
9-10: Simplification of Node type improves code clarityConverting the Node type from a struct to a byte slice alias simplifies the code and aligns with the PR's objective of streamlining the implementation.
25-32: Good additional error definitions for index validationThe new error variables clearly define the possible failure modes for index validation, which improves error reporting and handling throughout the codebase.
73-89: Thorough validation of indices in GenerateMultiProofWithIndicesThe implementation properly validates that indices are:
- Within bounds (not negative or beyond leaf count)
- Sorted in ascending order
- Unique (no duplicates)
This validation is essential for correct proof generation and handling.
109-153: Efficient proof element generation with clear separationThe updated proof generation algorithm clearly separates the generation of:
- The leaf count as the first proof element
- The necessary proof elements based on the tree structure
The algorithm correctly handles the logic for determining which nodes need to be included as proof elements based on whether one or both children are known.
235-247: Updated tree construction with proper root hashingThe tree construction now properly handles the root node (at index 0) by using the new
HashRootfunction that incorporates the leaf count as domain separation, which enhances security.
251-268: Good validation in makeLeafNodes functionThe function properly checks for nil leaves and ensures that each leaf is properly hashed, which improves robustness against invalid inputs.
311-323: Useful helper function for sequential index generationThe new
makeIndicesfunction offers a convenient way to generate sequential indices, with proper validation of the starting index and count.
325-342: Comprehensive index validationThis helper function properly validates indices against multiple criteria:
- Non-empty array
- Within bounds (not negative or beyond leaf count)
- Sorted in ascending order (implicitly ensuring uniqueness)
This is critical for ensuring proof correctness.
pkg/merkle/proof.go (6)
22-28: Improved MultiProof structure designThe redesigned
MultiProofstructure withstartingIndex,leaves, andproofElementsis more intuitive and better supports sequential proofs, aligning well with the PR objectives.
30-52: Clean API with getters and constructorThe new getters and constructor provide a clear API for working with the MultiProof structure, making the code more maintainable and easier to use.
144-147: Good helper function for bit manipulationThe isEven function uses efficient bit manipulation to determine if a number is even, which enhances code readability in the computation logic.
179-181: Efficient root computation with leaf count domain separationThe implementation correctly uses
HashRootto combine the leaf count with the computed sub-root, providing domain separation that enhances security.
185-205: Good handling of pairless nodes in unbalanced treesThis section correctly detects and processes pairless nodes in unbalanced trees, which is essential for the PR objective of supporting unbalanced tree structures.
214-226: Clever bit manipulation for node processing logicUsing
isEvenfor determining the right node source is an elegant approach that simplifies the complex logic of processing nodes in the proof verification.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
pkg/merkle/tree.go (2)
13-22:⚠️ Potential issueUpdate documentation to reflect zero-based indexing
The documentation comments still describe the tree as 1-indexed, but the implementation has been updated to use 0-based indexing.
// MerkleTree is a binary Merkle tree. // // tree has the collection of nodes, where: -// - The tree is 1-indexed, so root is at index 1. -// - The internal nodes are at index 1 to N-1. -// - The leaves are at index N to 2N-1. +// - The tree is 0-indexed, so root is at index 0. +// - The internal nodes are at index 1 to N-1. +// - The leaves are at index N to 2N-1.
167-167:⚠️ Potential issueUpdate documentation for Tree() method
The
Tree()method documentation still references a 1-indexed representation, but the implementation has changed to 0-indexed.-// Tree returns the 1-indexed representation of the Merkle tree. +// Tree returns the 0-indexed representation of the Merkle tree.
♻️ Duplicate comments (3)
pkg/merkle/proof_test.go (1)
329-337: Assertion assumes contiguous indices.The assertion on line 336 assumes the requested indices are contiguous and ascending. This is now part of the API contract as
GenerateMultiProofWithIndicesenforces that indices must be not only ascending but also contiguous.The code formatting on line 331 is flagged by the linter.
Please run the formatter to fix the linting issue:
- assert.Equal(t, len(tc.leaves), proof.GetLeafCount(), "Invalid leaf count") + assert.Equal(t, len(tc.leaves), proof.GetLeafCount(), "Invalid leaf count")🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 331-331:
File is not properly formatted (golines)pkg/merkle/proof.go (2)
34-36:⚠️ Potential issueAdd error checking in GetLeafCount
The function
GetLeafCount()directly accessesp.proofElements[0]without checking if the slice is empty.Based on previous review comments and the check in the
validate()method, you should add error handling here too:func (p *MultiProof) GetLeafCount() int { + if len(p.proofElements) == 0 { + return 0 // or panic("proofElements is empty") or return an error + } return Bytes32ToInt(p.proofElements[0]) }Consider returning an error as suggested in previous comments:
func (p *MultiProof) GetLeafCount() (int, error) { if len(p.proofElements) == 0 { return 0, errors.New("proofElements is empty") } return Bytes32ToInt(p.proofElements[0]), nil }
155-155:⚠️ Potential issueAdd explicit check for empty proofElements in computeRoot
Similar to the issue in
GetLeafCount(),computeRoot()directly accessesp.proofElements[0]without checking if the slice is empty.func (p *MultiProof) computeRoot() ([]byte, error) { + if len(p.proofElements) == 0 { + return nil, ErrNoProofs + } leafCount := Bytes32ToInt(p.proofElements[0])While the
validate()method already checks this, adding it here provides additional safety ifcomputeRoot()is ever called directly.
🧹 Nitpick comments (6)
pkg/merkle/proof_test.go (1)
425-425: Clarify test purpose or remove if redundant.The TODO comment indicates uncertainty about what this test is verifying. Consider either:
- Adding a clearer comment explaining the test's purpose
- Removing the test if it's redundant with other test coverage
This would improve code maintainability by ensuring all tests have clear objectives.
pkg/merkle/hash_test.go (1)
10-51: TestHash improved with direct byte slice comparisons.The test now uses clear, explicit test cases with direct byte slice comparisons rather than hex-encoded strings, improving readability and efficiency.
There is a formatting issue detected by the linter on line 19. Please run the formatter to fix this issue.
#!/bin/bash # Fix formatting issues go fmt ./pkg/merkle/hash_test.go🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 19-19:
File is not properly formatted (golines)🪛 GitHub Actions: Lint
[error] 19-19: File is not properly formatted (golines)
pkg/merkle/tree_test.go (2)
224-226: Improve code formatting for better readabilityThe static analysis tool highlights a formatting issue on line 224. While the code is functionally correct, the Go linter expects different formatting.
- assert.True(t, bytes.Equal(leafHash1, leafHash2), "Identical leaves should have identical leaf hashes") - assert.True(t, bytes.Equal(leafHash2, leafHash3), "Identical leaves should have identical leaf hashes") - assert.False(t, bytes.Equal(leafHash3, leafHash4), "Different leaves should have different leaf hashes") + assert.True( + t, + bytes.Equal(leafHash1, leafHash2), + "Identical leaves should have identical leaf hashes", + ) + assert.True( + t, + bytes.Equal(leafHash2, leafHash3), + "Identical leaves should have identical leaf hashes", + ) + assert.False( + t, + bytes.Equal(leafHash3, leafHash4), + "Different leaves should have different leaf hashes", + )🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 224-224:
File is not properly formatted (golines)
484-485: Fix typographical error in assertion messagesThe assertion messages contain "asa expected" instead of "as expected".
- "Sub Root should be asa expected", + "Sub Root should be as expected",Apply the same fix to all similar messages in the test file.
Also applies to: 491-492, 498-499, 527-528, 534-535, 541-542
pkg/merkle/proof.go (2)
178-178: Use idiomatic Go infinite loop syntaxThe Go linter flags the use of
for trueinstead of the idiomaticfor {}for infinite loops.- for true { + for {🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 178-178:
S1006: should use for {} instead of for true {} (staticcheck)
178-265: Add safeguard against infinite loopThe infinite loop in
computeRoot()relies on always hitting a return statement when the sub-root is reached. While the current logic seems correct, it's a good practice to add a safeguard to prevent infinite looping in case of unexpected conditions.Consider adding a maximum iteration count or other termination condition:
+ maxIterations := 1000 // Adjust based on expected maximum tree size + iterations := 0 for { + iterations++ + if iterations > maxIterations { + return nil, fmt.Errorf("exceeded maximum iterations, possible infinite loop") + } nodeIdx := queue[readIdx%queueLen].index // ... rest of the function🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 178-178:
S1006: should use for {} instead of for true {} (staticcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/merkle/hash.go(3 hunks)pkg/merkle/hash_test.go(1 hunks)pkg/merkle/proof.go(3 hunks)pkg/merkle/proof_internal_test.go(4 hunks)pkg/merkle/proof_test.go(15 hunks)pkg/merkle/tree.go(11 hunks)pkg/merkle/tree_internal_test.go(4 hunks)pkg/merkle/tree_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/merkle/hash.go
🧰 Additional context used
🧠 Learnings (4)
pkg/merkle/tree_internal_test.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:291-311
Timestamp: 2025-04-17T11:38:12.315Z
Learning: In the Merkle tree implementation, padded leaves (added to make leaf count a power of 2) are considered valid and should be taken into account for bounds checking. These padded leaves are initialized as empty byte arrays (`[]byte{}`) and can be accessed through indices. The validation should use `balancedLeafCount` (which includes padding) rather than just `leafCount` (actual leaves).
pkg/merkle/proof_test.go (1)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/proof_test.go:329-337
Timestamp: 2025-05-01T09:44:20.828Z
Learning: In the Merkle package, `GenerateMultiProofWithIndices` enforces that indices must be not only ascending but also contiguous, as part of the matched sequential proof implementation design.
pkg/merkle/tree.go (1)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/proof_test.go:329-337
Timestamp: 2025-05-01T09:44:20.828Z
Learning: In the Merkle package, `GenerateMultiProofWithIndices` enforces that indices must be not only ascending but also contiguous, as part of the matched sequential proof implementation design.
pkg/merkle/tree_test.go (2)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/tree_test.go:471-477
Timestamp: 2025-05-01T09:53:27.233Z
Learning: The testify assertion library's `assert.Equal()` function expects arguments in the order `Equal(t, expected, actual, ...)`. Using the correct order ensures that test failure messages properly show the difference between expected and actual values.
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:291-311
Timestamp: 2025-04-17T11:38:12.315Z
Learning: In the Merkle tree implementation, padded leaves (added to make leaf count a power of 2) are considered valid and should be taken into account for bounds checking. These padded leaves are initialized as empty byte arrays (`[]byte{}`) and can be accessed through indices. The validation should use `balancedLeafCount` (which includes padding) rather than just `leafCount` (actual leaves).
🧬 Code Graph Analysis (4)
pkg/merkle/proof_internal_test.go (3)
pkg/merkle/tree.go (4)
Leaf(11-11)ErrNoLeaves(25-25)ErrIndicesOutOfBounds(32-32)ErrNilLeaf(27-27)pkg/merkle/proof.go (6)
ProofElement(22-22)MultiProof(24-28)ErrInvalidStartingIndex(19-19)ErrNoProofs(18-18)ErrInvalidLeafCount(15-15)ErrNilProof(16-16)pkg/merkle/hash.go (2)
IntToBytes32(75-83)HashLeaf(28-36)
pkg/merkle/tree_internal_test.go (2)
pkg/merkle/tree.go (6)
Leaf(11-11)Node(9-9)ErrInvalidRange(30-30)ErrNoIndices(31-31)ErrIndicesOutOfBounds(32-32)ErrIndicesNotContinuousAndIncreasing(29-29)pkg/merkle/hash.go (3)
HashLeaf(28-36)HashNodePair(38-47)HashPairlessNode(49-57)
pkg/merkle/hash_test.go (1)
pkg/merkle/hash.go (7)
Hash(22-26)HashLeaf(28-36)HashNodePair(38-47)HashPairlessNode(49-57)HashRoot(59-73)IntToBytes32(75-83)Bytes32ToInt(85-87)
pkg/merkle/tree.go (2)
pkg/merkle/proof.go (2)
ProofElement(22-22)MultiProof(24-28)pkg/merkle/hash.go (5)
IntToBytes32(75-83)HashPairlessNode(49-57)HashNodePair(38-47)HashRoot(59-73)HashLeaf(28-36)
🪛 GitHub Check: Lint-Go
pkg/merkle/proof_test.go
[failure] 331-331:
File is not properly formatted (golines)
pkg/merkle/hash_test.go
[failure] 19-19:
File is not properly formatted (golines)
pkg/merkle/tree_test.go
[failure] 224-224:
File is not properly formatted (golines)
pkg/merkle/proof.go
[failure] 178-178:
S1006: should use for {} instead of for true {} (staticcheck)
🪛 GitHub Actions: Lint
pkg/merkle/hash_test.go
[error] 19-19: File is not properly formatted (golines)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
- GitHub Check: Code Review
🔇 Additional comments (28)
pkg/merkle/tree_internal_test.go (6)
10-30: Test successfully migrated frommakeNodestomakeLeafNodes.The test correctly validates the new leaf node hashing approach with direct byte slice comparisons, consistent with the simplified
Nodetype redesign.
58-64: Hash function changes properly implemented in tests.Updated to use the new domain-separated hash functions
HashNodePairandHashPairlessNodewith appropriate byte slice comparisons, consistent with the new hashing scheme.
75-81: Unbalanced tree test case properly updated.The test now correctly builds the expected root by explicitly using
HashPairlessNodefor singleton nodes andHashNodePairfor node pairs, properly handling the unbalanced tree structure.
97-97: Leaf padding removal verified.The assertion now correctly expects the exact number of original leaves (3) without padding, confirming the removal of leaf padding functionality as described in the PR.
131-182: Good test coverage for index generation.This new test thoroughly validates the
makeIndicesfunction with a variety of inputs including valid sequences, negative indices, zero count, and negative count. The test cases ensure the function properly generates sequential indices or returns appropriate errors.
184-235: Comprehensive validation of index constraints.The
validateIndicestests provide thorough coverage of the constraints enforced in the new sequential proof implementation, ensuring indices are present, within bounds, and in continuous ascending order.pkg/merkle/proof_internal_test.go (7)
13-21: Proof structure updates correctly implemented.The test has been properly updated to use the new MultiProof fields (
startingIndex,leaves, andproofElements), consistent with the redesigned proof verification system.
27-47: Error handling adequately updated for new proof structure.Test cases for invalid starting index and no proofs have been added, ensuring proper validation of the sequential proof structure parameters.
73-95: Nil leaf validation correctly implemented.The test now properly validates that each leaf in the proof is non-nil, replacing the previous indexed value validation.
110-121: Proper handling of single leaf trees.The test correctly verifies that a proof for a single leaf tree with just a leaf count element (no actual proof elements) is valid, which is important for edge cases.
124-160: TestGetNextProofElement thoroughly tests proof element iteration.The test has been properly renamed and updated to verify the circular queue approach for proof element retrieval, including correctly handling the leaf count element and reporting errors when elements are exhausted.
175-191: Node queue construction correctly updated.The test verifies the updated index calculation for the queue, with indices now based on the leaf count plus the position, aligned with the zero-based indexing in the tree implementation.
209-216: Index adjustment in TestComputeRoot.The test now correctly uses contiguous indices
[0, 1]instead of[0, 2], aligning with the new requirement that indices must be contiguous for the matched sequential proof implementation.pkg/merkle/proof_test.go (4)
159-163: MultiProof field assertions properly updated.Test assertions now correctly verify the new fields of the MultiProof structure (startingIndex, leafCount, leaves, proofElements), ensuring the proof is constructed correctly with the redesigned structure.
452-511: Excellent test vectors for sequential proofs in balanced trees.These detailed test assertions with fixed hex-encoded values provide a strong verification of the new sequential proof implementation for balanced trees, ensuring the implementation remains correct through future changes.
513-559: Thorough verification of unbalanced tree sequential proofs.The test demonstrates that the proof system correctly handles unbalanced trees with sequential leaves, a key objective of this PR. The explicit test vectors ensure correctness and stability.
561-604: Comprehensive end-to-end verification test.This test provides a complete verification of the proof system with a larger set of leaves and explicit proof elements, ensuring the entire verification pipeline works correctly for the new proof format.
pkg/merkle/hash_test.go (6)
1-8: Package organization improved.Moving from package
merkletomerkle_testand explicitly importing themerklepackage follows Go best practices for testing and improves separation of test code from implementation.
53-94: TestHashLeaf provides thorough coverage of leaf hashing.This new test verifies the domain-separated leaf hashing function with multiple test cases, ensuring consistent, correct leaf node hashing behavior.
96-137: TestHashNodePair properly tests pair hashing.The test provides comprehensive validation of the node pair hashing function with different input combinations, ensuring correct hash construction for internal nodes.
139-180: TestHashPairlessNode verifies single-node handling.This test ensures correct behavior when hashing individual nodes without pairs, an important case for unbalanced trees where some nodes don't have siblings.
182-223: TestHashRoot validates the new root hashing approach.This test verifies the new domain separation for root hashes that incorporates leaf count metadata, a critical feature of the redesigned Merkle tree implementation.
225-309: Integer encoding/decoding properly tested.The tests for
IntToBytes32andBytes32ToIntensure that integers are correctly encoded to fixed-size 32-byte arrays and decoded back, which is essential for the leaf count encoding in proof elements.pkg/merkle/tree_test.go (2)
137-137: TODO item remains in test functionThe TODO comment in
TestUnbalancedTreesindicates that a part of the test logic is not implemented yet: checking that internal nodes up to the root are not nil for unbalanced trees.This might impact test coverage for unbalanced trees. Consider implementing the missing verification or removing the TODO if it's no longer needed.
471-477: Correct assertion argument orderBased on the learning from a previous comment, the arguments in
assert.Equalshould be in the orderEqual(t, expected, actual, ...).I'm happy to see the correct order is used here with the expected value (8) preceding the actual value (
tree.LeafCount()). This ensures clear test failure messages.pkg/merkle/tree.go (2)
73-89: Robust input validation in GenerateMultiProofWithIndicesThe function now enforces that indices must be continuous and increasing, validating this requirement before proceeding with proof generation.
This validation ensures that the indices represent a contiguous sequence, which matches the learning from previous PRs that
GenerateMultiProofWithIndicesenforces indices to be not only ascending but contiguous.
296-296:Details
❌ Incorrect review comment
Potential redundant bit shift operation
Adding a shift by 32 bits will have no effect on 32-bit integers, as they'll be 0 after shifting right by 32 positions.
🏁 Script executed:
#!/bin/bash # Check if this operation is needed by searching for large integer test cases rg "roundUpToPowerOf2.*64" --type goLength of output: 38
🏁 Script executed:
#!/bin/bash # Display the signature of roundUpToPowerOf2 and context around the 32-bit shift rg -n "func roundUpToPowerOf2" -n pkg/merkle/tree.go rg -n "n |= n >> 32" -A3 -B3 pkg/merkle/tree.goLength of output: 7297
Retain 32-bit shift for cross-platform support
The
roundUpToPowerOf2function uses Go’sinttype, which is 64-bit on most platforms. Then |= n >> 32step is required to propagate high bits on 64-bit builds; on 32-bit builds the shift yields zero and is harmless. No changes needed here.Likely an incorrect or invalid review comment.
pkg/merkle/proof.go (1)
78-83: Properly handle empty proofElementsThe check for empty proofElements before accessing index 0 is a good addition that addresses previous review comments.
This check prevents potential panics when validating proofs with no elements. Good defensive programming!
| func getBytesFromHexString(s string) []byte { | ||
| decoded, _ := hex.DecodeString(s) | ||
| return decoded | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling in helper function
The getBytesFromHexString function discards the error returned by hex.DecodeString().
func getBytesFromHexString(s string) []byte {
- decoded, _ := hex.DecodeString(s)
+ decoded, err := hex.DecodeString(s)
+ if err != nil {
+ panic(fmt.Sprintf("Invalid hex string: %s, error: %v", s, err))
+ }
return decoded
}Since this is only used in tests, a panic is appropriate for invalid input. Alternatively, you could modify test functions to handle the error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getBytesFromHexString(s string) []byte { | |
| decoded, _ := hex.DecodeString(s) | |
| return decoded | |
| } | |
| func getBytesFromHexString(s string) []byte { | |
| decoded, err := hex.DecodeString(s) | |
| if err != nil { | |
| panic(fmt.Sprintf("Invalid hex string: %s, error: %v", s, err)) | |
| } | |
| return decoded | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pkg/merkle/proof_test.go (2)
329-342: Verify indices assumption aligns with design intent.The assertion on line 341 assumes contiguous indices, which matches the behavior clarified in the retrieved learning that
GenerateMultiProofWithIndicesenforces contiguous indices.
570-573: 🛠️ Refactor suggestionMissing error handling in hex decoder.
The
getBytesFromHexStringfunction discards the error returned byhex.DecodeString().func getBytesFromHexString(s string) []byte { - decoded, _ := hex.DecodeString(s) + decoded, err := hex.DecodeString(s) + if err != nil { + panic(fmt.Sprintf("Invalid hex string: %s, error: %v", s, err)) + } return decoded }pkg/merkle/tree_test.go (1)
570-573: 🛠️ Refactor suggestionMissing error handling in hex decoder.
The
getBytesFromHexStringfunction discards the error returned byhex.DecodeString().func getBytesFromHexString(s string) []byte { - decoded, _ := hex.DecodeString(s) + decoded, err := hex.DecodeString(s) + if err != nil { + panic(fmt.Sprintf("Invalid hex string: %s, error: %v", s, err)) + } return decoded }
🧹 Nitpick comments (1)
pkg/merkle/proof_test.go (1)
430-430: Unclear test purpose needs documentation.The TODO comment indicates uncertainty about what this test is checking. Consider adding documentation clarifying the purpose of this test or removing it if redundant.
-// TODO: Not sure what this is testing +// Tests that the MultiProof structure correctly preserves the leaves from the original tree func TestMultiProofManipulation(t *testing.T) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/merkle/hash_test.go(1 hunks)pkg/merkle/proof_test.go(15 hunks)pkg/merkle/tree_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/merkle/hash_test.go
🧰 Additional context used
🧠 Learnings (2)
pkg/merkle/proof_test.go (1)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/proof_test.go:329-337
Timestamp: 2025-05-01T09:44:20.855Z
Learning: In the Merkle package, `GenerateMultiProofWithIndices` enforces that indices must be not only ascending but also contiguous, as part of the matched sequential proof implementation design.
pkg/merkle/tree_test.go (2)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/tree_test.go:471-477
Timestamp: 2025-05-01T09:53:27.260Z
Learning: The testify assertion library's `assert.Equal()` function expects arguments in the order `Equal(t, expected, actual, ...)`. Using the correct order ensures that test failure messages properly show the difference between expected and actual values.
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:291-311
Timestamp: 2025-04-17T11:38:12.315Z
Learning: In the Merkle tree implementation, padded leaves (added to make leaf count a power of 2) are considered valid and should be taken into account for bounds checking. These padded leaves are initialized as empty byte arrays (`[]byte{}`) and can be accessed through indices. The validation should use `balancedLeafCount` (which includes padding) rather than just `leafCount` (actual leaves).
🧬 Code Graph Analysis (1)
pkg/merkle/tree_test.go (2)
pkg/merkle/tree.go (6)
ErrTreeEmpty(26-26)Leaf(11-11)NewMerkleTree(36-52)Node(9-9)CalculateBalancedNodesCount(273-283)ErrNilLeaf(27-27)pkg/merkle/hash.go (4)
HashLeaf(28-36)HashNodePair(38-47)HashPairlessNode(49-57)HashRoot(59-73)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Code Review
🔇 Additional comments (10)
pkg/merkle/proof_test.go (4)
31-31: Changes towantProofCountalign with new MultiProof structure.The proof count expectations have been updated to reflect the redesigned proof structure that now uses sequential leaves with starting indices.
Also applies to: 41-41, 51-51, 61-61, 71-71, 101-101
159-162: Good addition of comprehensive proof verification.These assertions properly verify all aspects of the new MultiProof structure including starting index, leaf count, and proof elements.
457-536: Well-designed test vectors for sequential proof verification.The comprehensive test cases with explicit hex values provide excellent verification of the new proof structure for both balanced and unbalanced trees.
598-667: Thorough testing of proof verification with fixed test vectors.
TestVerifySample1validates the proof verification logic with carefully constructed test vectors that cover multiple edge cases.pkg/merkle/tree_test.go (6)
58-68: Properly updated leaf node hashing check.The test correctly uses the new
HashLeaffunction to verify leaf hashes, aligning with the implementation change that introduced domain separation for leaf hashing.
113-134: Well-structured verification for unbalanced trees.The test properly calculates balanced leaf count and uses the correct indexing to verify leaves in unbalanced trees, showing good understanding of the new tree structure.
206-241: Good test coverage for duplicate leaves.This test properly verifies that identical leaves produce identical leaf hashes while different leaves produce different hashes, validating the leaf hashing function.
287-297: Proper nil leaf validation test.This test correctly verifies that the
ErrNilLeaferror is returned when nil leaves are provided, ensuring the implementation properly validates leaves.
358-393: Comprehensive hash verification.These detailed hash verifications for all nodes ensure the correctness of the new hashing functions including the domain separation for different node types.
475-568: Well-structured sample tests with explicit hash verification.The sample tests with explicit hex-encoded values provide excellent validation of the tree construction and hashing for both balanced and unbalanced trees.
|
I'll try to list the changes here in two categories:
Required for compatibility:
Not required for compatibility:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
pkg/merkle/proof_test.go (1)
340-343: Assertion assumes contiguous indicesThe loop
for i := 0; i < len(leaves); i++with the assertionassert.Equal(t, tc.leaves[tc.indices[0]+i], leaves[i])assumes the requested indices are contiguous, which is now enforced by theGenerateMultiProofWithIndicesfunction.This is appropriate given the learning from earlier comments that the Merkle package now enforces indices to be not only ascending but also contiguous as part of the matched sequential proof implementation design.
pkg/merkle/tree.go (1)
179-179:⚠️ Potential issueAdd a null check to prevent potential panic
The
Root()method directly accessesm.tree[0]without checking if the tree slice is empty, which could lead to an index out-of-range panic.func (m *MerkleTree) Root() []byte { + if len(m.tree) == 0 { + return nil + } return m.tree[0] }pkg/merkle/tree_test.go (2)
629-632: Add error handling in helper functionThe
getBytesFromHexStringfunction discards the error returned byhex.DecodeString().func getBytesFromHexString(s string) []byte { - decoded, _ := hex.DecodeString(s) + decoded, err := hex.DecodeString(s) + if err != nil { + panic(fmt.Sprintf("Invalid hex string: %s, error: %v", s, err)) + } return decoded }Since this is only used in tests, a panic is appropriate for invalid input.
136-136: 🛠️ Refactor suggestionIncomplete test implementation.
The TODO comment indicates that the test for checking internal nodes in unbalanced trees is incomplete.
// TODO: Check that all internal nodes, let of the upperBound, up to the root are not nil. +for i := 1; i < balancedLeafCount; i++ { + nodeIndex := i + // Skip checking nodes that would be beyond the actual tree size + if nodeIndex < balancedLeafCount && nodeIndex < len(tree.Tree()) { + assert.NotNil( + t, + tree.Tree()[nodeIndex], + "Internal node should not be nil if within tree bounds", + ) + } +}
🧹 Nitpick comments (1)
pkg/merkle/proof_test.go (1)
629-632: Add error handling in helper functionThe
getBytesFromHexStringfunction discards the error returned byhex.DecodeString().func getBytesFromHexString(s string) []byte { - decoded, _ := hex.DecodeString(s) + decoded, err := hex.DecodeString(s) + if err != nil { + panic(fmt.Sprintf("Invalid hex string: %s, error: %v", s, err)) + } return decoded }Since this is only used in tests, a panic is appropriate for invalid input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/merkle/proof_test.go(16 hunks)pkg/merkle/tree.go(12 hunks)pkg/merkle/tree_internal_test.go(4 hunks)pkg/merkle/tree_test.go(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
pkg/merkle/tree_internal_test.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:291-311
Timestamp: 2025-04-17T11:38:12.315Z
Learning: In the Merkle tree implementation, padded leaves (added to make leaf count a power of 2) are considered valid and should be taken into account for bounds checking. These padded leaves are initialized as empty byte arrays (`[]byte{}`) and can be accessed through indices. The validation should use `balancedLeafCount` (which includes padding) rather than just `leafCount` (actual leaves).
pkg/merkle/proof_test.go (1)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/proof_test.go:329-337
Timestamp: 2025-05-01T09:44:20.855Z
Learning: In the Merkle package, `GenerateMultiProofWithIndices` enforces that indices must be not only ascending but also contiguous, as part of the matched sequential proof implementation design.
pkg/merkle/tree.go (1)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/proof_test.go:329-337
Timestamp: 2025-05-01T09:44:20.855Z
Learning: In the Merkle package, `GenerateMultiProofWithIndices` enforces that indices must be not only ascending but also contiguous, as part of the matched sequential proof implementation design.
pkg/merkle/tree_test.go (2)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/tree_test.go:471-477
Timestamp: 2025-05-01T09:53:27.260Z
Learning: The testify assertion library's `assert.Equal()` function expects arguments in the order `Equal(t, expected, actual, ...)`. Using the correct order ensures that test failure messages properly show the difference between expected and actual values.
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:291-311
Timestamp: 2025-04-17T11:38:12.315Z
Learning: In the Merkle tree implementation, padded leaves (added to make leaf count a power of 2) are considered valid and should be taken into account for bounds checking. These padded leaves are initialized as empty byte arrays (`[]byte{}`) and can be accessed through indices. The validation should use `balancedLeafCount` (which includes padding) rather than just `leafCount` (actual leaves).
🧬 Code Graph Analysis (3)
pkg/merkle/proof_test.go (2)
pkg/merkle/tree.go (2)
NewMerkleTree(36-52)Leaf(11-11)pkg/merkle/proof.go (3)
Verify(55-70)ProofElement(22-22)NewMerkleProof(46-52)
pkg/merkle/tree.go (2)
pkg/merkle/proof.go (2)
ProofElement(22-22)MultiProof(24-28)pkg/merkle/hash.go (5)
IntToBytes32(75-83)HashPairlessNode(49-57)HashNodePair(38-47)HashRoot(59-73)HashLeaf(28-36)
pkg/merkle/tree_test.go (2)
pkg/merkle/tree.go (6)
ErrNoLeaves(25-25)Leaf(11-11)NewMerkleTree(36-52)Node(9-9)CalculateBalancedNodesCount(273-290)ErrNilLeaf(27-27)pkg/merkle/hash.go (4)
HashLeaf(28-36)HashNodePair(38-47)HashPairlessNode(49-57)HashRoot(59-73)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Code Review
🔇 Additional comments (13)
pkg/merkle/tree_internal_test.go (3)
97-97: Leaf padding is now removed in agreement with PR objectivesThis change confirms that the tree implementation now keeps the original leaf count without padding to a power of 2, which aligns with the PR objective to maintain the original length of the leaves array for compatibility with reference implementations.
131-182: Good test coverage for the newmakeIndicesfunctionThe new test thoroughly verifies edge cases including negative starting indices, zero count, and negative count scenarios. This ensures the helper function behaves correctly and fails appropriately for invalid inputs.
184-235: Comprehensive validation tests for sequential indicesGood coverage of various validation scenarios including:
- Valid sequential indices
- Empty indices
- Out-of-bounds indices
- Negative indices
- Non-continuous indices
This aligns with the PR objectives that require indices to be continuous and sequential for the matched sequential proof implementation.
pkg/merkle/proof_test.go (2)
458-590: Effective test vectors with fixed hex samplesThe new test cases with fixed hex-encoded leaves and expected outputs are excellent for verifying compatibility with reference implementations. These static test vectors provide a solid reference point for current and future implementations.
715-793: Comprehensive root verification for fixed test vectorsThe proof verification tests with fixed hex-encoded inputs are valuable for ensuring the verification logic works correctly. The test vectors match the ones used in generation tests, forming a complete test suite.
pkg/merkle/tree.go (4)
73-89: Enforcing indices to be continuous and increasingThe updated implementation of
GenerateMultiProofWithIndicesnow explicitly enforces that indices must be not only in ascending order but also continuous. This aligns with the PR objectives for the matched sequential proof implementation design.
120-122: Leaf count included as first proof elementAdding the leaf count as the first proof element enables trivial leaf count proofs and makes the verification algorithm simpler. This matches the PR objectives for compatibility with reference JavaScript and Solidity implementations.
283-288: Special handling for the single-leaf caseThe updated
CalculateBalancedNodesCountnow properly handles the single-leaf case by returning 2. This special case handling ensures that the algorithm works correctly for all input sizes, aligning with the PR objectives.
332-349: Good index validationThe new
validateIndiceshelper function provides robust validation of indices, ensuring they are:
- Not empty
- Within bounds (0 to leafCount-1)
- Continuous and increasing
This enhances the API contract clarity and aligns with the PR objectives for tightening proof validation.
pkg/merkle/tree_test.go (4)
286-296: Good test for nil leaf validationThis new test properly verifies that the implementation rejects nil leaves, which is a key change in the refactored code. This aligns with the PR objectives of disallowing nil leaves while still permitting empty leaves.
358-379: Detailed hash verification for tree nodesThe test now explicitly verifies the correct hashing of internal nodes using the new domain-separated hashing functions:
HashNodePairandHashPairlessNode. This ensures the internal tree structure is correctly built.
381-392: Root hash verification with metadataThe test now properly verifies that the root node (at index 0) includes the leaf count metadata using the
HashRootfunction. This ensures the new domain separation method for root hashes incorporating leaf count is working correctly.
440-535: Excellent balanced tree test vectorsThe new balanced sample tests with fixed hex-encoded inputs and expected outputs provide a strong guarantee of compatibility with reference implementations. The explicit verification of both the sub-root and final root ensures the entire hashing chain is correct.
| func TestRoundUpToPowerOf2(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input int | ||
| expected int | ||
| }{ | ||
| {"one", 1, 1}, | ||
| {"already power of 2", 4, 4}, | ||
| {"already power of 2 (large)", 16384, 16384}, | ||
| {"regular case", 5, 8}, | ||
| {"regular case (large)", 5000, 8192}, | ||
| {"large number", 1<<30 - 1, 1 << 30}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := roundUpToPowerOf2(tt.input) | ||
| if result != tt.expected { | ||
| t.Errorf("roundUpToPowerOf2(%d) = %d, expected %d", tt.input, result, tt.expected) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider architecture compatibility in roundUpToPowerOf2 tests
The test cases validate the function for various inputs, including edge cases. However, the test for "large number" uses 1<<30 - 1, which assumes a system that can handle 64-bit shifts (as implemented in the updated code with n |= n >> 32).
🏁 Script executed:
#!/bin/bash
# Check what architecture the system is using to ensure compatibility
echo "Architecture: $(uname -m)"
# Check if the code is cross-platform compatible by examining the C compiler's pre-defined macros
echo "Int size: $(gcc -E -dM - </dev/null | grep INT_MAX)"Length of output: 219
🏁 Script executed:
#!/bin/bash
set -e
# Search for the implementation of roundUpToPowerOf2
rg --context 3 "func roundUpToPowerOf2" -n .Length of output: 382
🏁 Script executed:
#!/bin/bash
set -e
# Show any Go build constraints in pkg/merkle
grep -R "//go:build" -n pkg/merkle || true
grep -R "+build" -n pkg/merkle || true
# Show the full implementation of roundUpToPowerOf2 with context
rg -C 15 "func roundUpToPowerOf2" -n pkg/merkle/tree.goLength of output: 1069
Ensure cross-platform support in roundUpToPowerOf2 implementation
The real issue isn’t in the tests (the “large number” case using 1<<30-1 is safe on 32-bit), but in the implementation’s unconditional n |= n >> 32, which won’t compile when int/uint is only 32 bits wide. To fix:
• Replace the hard-coded 32-bit shift with a dynamic loop based on bits.UintSize, for example:
// roundUpToPowerOf2 rounds up n to the next power of 2.
func roundUpToPowerOf2(n int) int {
if bits.OnesCount(uint(n)) == 1 {
return n
}
// propagate highest bit to all lower bits
for shift := 1; shift < bits.UintSize; shift <<= 1 {
n |= n >> shift
}
return n + 1
}• Alternatively, guard the 32-bit shift behind a build tag or if bits.UintSize == 64 check.
This ensures roundUpToPowerOf2 compiles and behaves correctly on both 32- and 64-bit platforms.
pkg/merkle/tree.go
Outdated
| } | ||
|
|
||
| // The first proof element is always the leaf count. | ||
| proofElements = append(proofElements, IntToBytes32(m.LeafCount())) |
There was a problem hiding this comment.
I thought we didn't need compact proofs. What's the gas saving of including the leafCount as part of the proof vs. passing it as an parameter?
There was a problem hiding this comment.
I have tried for days, and there is simply not enough stack space in Solidity (without going to assembly or using compiler flags that make traces and some gas/slither/coverage analysis impossible) to have the leafCount be its own variable. Also, semantically, it is ok to say "here are the leaves I am proving exist in a sequence, starting from this index, and here is all the proof data you need, which is opaque to me, as a prover".
There was a problem hiding this comment.
Makes sense to adapt the golang code in that case.
pkg/merkle/tree.go
Outdated
| tree[i] = HashNodePair(tree[leftChildIndex], tree[rightChildIndex]) | ||
| } | ||
|
|
||
| tree[0] = HashRoot(leafCount, tree[1]) |
There was a problem hiding this comment.
Two roots are computed in the tree; tree[1] = Hash(left, right), and tree[0] = (rootPrefix, leafCount, tree[1]), tree[1] is never used except for computing tree[0].
I think leafCount is not necessary to be included there, as the tree structure is fixed and any attempt to include new leaves would require recomputing the tree. The root prefix is not needed either as collision resistance is already guaranteed with leaf and node prefixes.
My suggestion here; modify the code so the merkle tree is 0-indexed, and when building a tree the last steps computes tree[0] as wanted. That way we avoid having two roots.
There was a problem hiding this comment.
The root is a the hash of the leaf count and the leaf root because:
- its a further commitment that secures the algorithm, much like the node prefix (which does not need to exist, since there is already a leaf prefix)
- it allows for proofs of just the tree's size
Also, the algorithm is already completed on 3 separate coded bases, and, in my opinion, this 1 extra hash isn't worth going through all those codebases to change them, and then fix all the test fixtures.
| leaves := make([]Leaf, len(iv)) | ||
| for i, v := range iv { | ||
| leaves[i] = Leaf(v.value) | ||
| func NewMerkleProof(startingIndex int, leaves []Leaf, proofElements []ProofElement) *MultiProof { |
There was a problem hiding this comment.
This func is never used except in tests, meaning that the test could use the respective GenerateMultiProof function and this removed.
There was a problem hiding this comment.
I agree, however:
- as I have never really written large scale Go, I did not know it was ok to include the tests in the same package (since then the tests get complied for prod), so the only way to allow tests in a
merkle_testpackage to build aMultiProofwas via thisNewMerkleProoffunction. I have since learned it is acceptable to have tests in themerklepackage. - I thought other nodes (or even via an API), would want to take a
startingIndex,leaves, andproofElementsto build aMultiProofand check it, so this method allows that to be possible in a node without needing all the leaves to build a tree first
There was a problem hiding this comment.
I thought other nodes (or even via an API), would want to take a startingIndex, leaves, and proofElements to build a MultiProof and check it, so this method allows that to be possible in a node without needing all the leaves to build a tree first
Up to you! Realistically this not going to happen, similarly to what was discussed around generating proofs with indices.
- `HashRoot` returns hash and error - `Bytes32ToInt` returns hash and error - `Bytes32ToInt` requires a 32 byte buffer with 24 leading zeros - empty trees supported for trees and proofs - proofs of no leafs (only size) supported - `GenerateMultiProofWithIndices` and `validateIndices` removed - PR suggestions included - updated and corrected tests
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
pkg/merkle/tree.go (1)
270-283: 🛠️ Refactor suggestionFix compatibility issue with 32-bit architectures.
The current implementation of
roundUpToPowerOf2doesn't include the shift by 32 bits, which avoids the potential undefined behavior on 32-bit systems that was mentioned in previous comments. However, this means the function won't properly handle very large inputs on 64-bit systems.Consider using a more architecture-aware approach:
func roundUpToPowerOf2(n int) int { if bits.OnesCount(uint(n)) == 1 { return n } n |= n >> 1 n |= n >> 2 n |= n >> 4 n |= n >> 8 n |= n >> 16 + // Only apply 32-bit shift on 64-bit architectures + if bits.UintSize == 64 { + n |= n >> 32 + } return n + 1 }This ensures the function works correctly on both 32-bit and 64-bit architectures.
♻️ Duplicate comments (2)
pkg/merkle/tree_test.go (2)
660-663: 🛠️ Refactor suggestionAdd error handling to prevent silent failures.
The
getBytesFromHexStringfunction ignores errors fromhex.DecodeString(), which could lead to unexpected behavior if invalid hex strings are provided.func getBytesFromHexString(s string) []byte { - decoded, _ := hex.DecodeString(s) + decoded, err := hex.DecodeString(s) + if err != nil { + panic(fmt.Sprintf("Invalid hex string: %s, error: %v", s, err)) + } return decoded }
131-131: 🛠️ Refactor suggestionImplement the TODO for checking internal nodes.
This TODO comment indicates incomplete test coverage for internal nodes in unbalanced trees. Not implementing this check could miss issues with tree construction.
Complete the implementation to verify internal nodes in unbalanced trees:
- // TODO: Check that all internal nodes, let of the upperBound, up to the root are not nil. + // Check that all internal nodes up to the root are not nil + upperBound := balancedLeafCount + tc.leafCount - 1 + for i := 1; i < balancedLeafCount; i++ { + // Check nodes that would be within the tree bounds + if i <= upperBound >> 1 { + assert.NotNil( + t, + tree.Tree()[i], + "Internal node should not be nil if within tree bounds", + ) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/merkle/hash.go(3 hunks)pkg/merkle/hash_test.go(1 hunks)pkg/merkle/proof.go(3 hunks)pkg/merkle/proof_internal_test.go(4 hunks)pkg/merkle/proof_test.go(10 hunks)pkg/merkle/tree.go(8 hunks)pkg/merkle/tree_internal_test.go(1 hunks)pkg/merkle/tree_test.go(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/merkle/tree_test.go (1)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/tree_test.go:471-477
Timestamp: 2025-05-01T09:53:27.260Z
Learning: The testify assertion library's `assert.Equal()` function expects arguments in the order `Equal(t, expected, actual, ...)`. Using the correct order ensures that test failure messages properly show the difference between expected and actual values.
🧬 Code Graph Analysis (4)
pkg/merkle/proof_internal_test.go (3)
pkg/merkle/tree.go (3)
Leaf(11-11)ErrIndicesOutOfBounds(28-28)ErrNilLeaf(26-26)pkg/merkle/proof.go (6)
ProofElement(22-22)MultiProof(24-28)ErrInvalidStartingIndex(18-18)ErrNoProofs(17-17)ErrInsufficientProofs(19-19)ErrNilProof(15-15)pkg/merkle/hash.go (2)
IntToBytes32(82-90)HashLeaf(31-39)
pkg/merkle/hash_test.go (1)
pkg/merkle/hash.go (7)
Hash(25-29)HashLeaf(31-39)HashNodePair(41-50)HashPairlessNode(52-60)HashRoot(62-80)IntToBytes32(82-90)Bytes32ToInt(92-111)
pkg/merkle/proof_test.go (2)
pkg/merkle/tree.go (2)
Leaf(11-11)NewMerkleTree(32-44)pkg/merkle/proof.go (3)
ProofElement(22-22)NewMerkleProof(50-56)Verify(59-74)
pkg/merkle/tree_test.go (2)
pkg/merkle/tree.go (6)
Leaf(11-11)NewMerkleTree(32-44)Node(9-9)CalculateBalancedNodesCount(247-268)ErrNilLeaf(26-26)EmptyTreeRoot(25-25)pkg/merkle/hash.go (4)
HashLeaf(31-39)HashNodePair(41-50)HashPairlessNode(52-60)HashRoot(62-80)
🪛 GitHub Check: Lint-Go
pkg/merkle/proof.go
[failure] 287-287:
unreachable: unreachable code (govet)
🪛 GitHub Actions: Lint
pkg/merkle/proof.go
[error] 287-287: golangci-lint (govet): unreachable code detected
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
- GitHub Check: Code Review
🔇 Additional comments (18)
pkg/merkle/proof_internal_test.go (4)
124-125: Renamed function correctly reflects implementation detailGood update to rename
TestGetNextProoftoTestGetNextProofElementto accurately reflect that it's testing the extraction of individual proof elements rather than complete proofs.
174-191: Updated queue indices reflect zero-based indexingThe test correctly uses new zero-based indexing with balanced leaf count offsets. Specifically:
- index 5 = 4+1 (balanced leaf count + index)
- index 4 = 4+0 (balanced leaf count + index)
This matches the design described in the PR objectives for using zero-based indexing in the tree array.
209-210: Sequential proof generation matches requirementsGood adaptation of the test to use
GenerateMultiProofSequentialwhich enforces sequential indices, starting from 0 with a count of 2. This aligns with the PR objective to require sequential indices for proof generation.
13-15:Details
✅ Verification successful
Confirm this is the expected leaf encoding in your test
This test is using a single test leaf with a simple byte value and expecting that to be valid with two proof elements. Make sure this aligns with your implementation, as
IntToBytes32(2)suggests your proof expects exactly 2 leaves, but you're only providing one.
🏁 Script executed:
#!/bin/bash # Check how many leaves are needed in the implementation rg -B 5 -A 5 "validate" pkg/merkle/proof.go | grep -A 10 "func.*validate" || echo "Could not find validate function"Length of output: 249
🏁 Script executed:
#!/bin/bash # Locate the test file and show its contents around the lines in question file_path=$(rg -l "proof_internal_test.go") echo "File: $file_path" sed -n '1,200p' "$file_path"Length of output: 130
🏁 Script executed:
#!/bin/bash # Locate the test file's exact path find . -type f | grep 'proof_internal_test.go' || echo "No test file found"Length of output: 82
🏁 Script executed:
#!/bin/bash # Show the contents of the test file around the snippet sed -n '1,200p' pkg/merkle/proof_internal_test.goLength of output: 4938
🏁 Script executed:
#!/bin/bash # Extract the full implementation of MultiProof.validate start=$(grep -nE "func \(p \*MultiProof\) validate" -n pkg/merkle/proof.go | cut -d: -f1) end=$((start + 200)) sed -n "${start},${end}p" pkg/merkle/proof.goLength of output: 5993
No changes needed—test correctly encodes a two-leaf proof for a single leaf
The
validatemethod treats the first element ofproofElementsasleafCount. In a two-leaf tree where you’re proving one leaf (startingIndex=0,len(leaves)=1), you must supply:
- proofElements[0] =
IntToBytes32(2)(the total leaf count)- proofElements[1] = sibling decommitment
Your test’s use of
[]ProofElement{IntToBytes32(2), []byte("proof")}aligns perfectly with the implementation.pkg/merkle/hash.go (2)
62-80: Improved root hashing with leaf count validationGreat implementation of the
HashRootfunction that incorporates the leaf count into the root hash with proper domain separation. The validation of leaf count range is particularly important.However, there's a potential issue when converting from
inttouint64that was noted in previous reviews.Given the previous discussion about maintaining API consistency, this is a reasonable approach for now. As noted in the learning, this could be addressed in a future update:
The team plans to address this in a future update to maintain API consistency.
92-111: Thorough validation in Bytes32ToIntExcellent implementation of
Bytes32ToIntwith comprehensive validation:
- Checking the buffer length is exactly 32 bytes
- Verifying the first 28 bytes are all zeros (ensuring it fits in a 32-bit int)
- Validating the value doesn't exceed the maximum 31-bit signed integer
This prevents potential panics and ensures the integer representation is valid.
pkg/merkle/tree_internal_test.go (4)
10-31: Updated leaf node handling matches implementationThe test has been appropriately renamed from
TestMakeNodestoTestMakeLeafNodesto match the function being tested. The test cases correctly verify that leaf nodes are created from leaves using the newHashLeaffunction.
51-53: Fixed tree size for single leaf treeThe test correctly expects a tree size of 4 for a single leaf, which matches the PR objectives stating:
Trees with a single leaf are treated as unbalanced, requiring a tree array size of 4 (2 * 2) to ensure consistent hashing with node prefixes.
This ensures compatibility with the JavaScript and Solidity reference implementations.
80-87: Updated node hashing for unbalanced treesThe test correctly uses the new
HashPairlessNodefunction for the rightmost leaf in an unbalanced tree, which aligns with the PR objectives:Pairless nodes (left siblings without right siblings) are hashed alone with a node prefix instead of hashing against a nil sibling.
This change improves compatibility with reference implementations.
184-206:Details
❌ Incorrect review comment
Potential cross-platform compatibility issue in roundUpToPowerOf2 test
The test for large numbers uses
1<<30 - 1, which assumes the implementation can handle 64-bit shifts. This was previously flagged in a review comment.Verify that the implementation of
roundUpToPowerOf2handles both 32-bit and 64-bit platforms correctly, especially when implementing the bit-shifting operation.
🏁 Script executed:
#!/bin/bash # Find the implementation of roundUpToPowerOf2 rg -C 10 "func roundUpToPowerOf2" pkg/merkle/Length of output: 918
🏁 Script executed:
#!/bin/bash # Display full implementation of roundUpToPowerOf2 rg -A20 "func roundUpToPowerOf2" pkg/merkle/tree.goLength of output: 495
No cross-platform issue with roundUpToPowerOf2 test
The literal1<<30(1,073,741,824) fits within a 32-bitinton both 32- and 64-bit Go targets, and the bit-twiddling hack (shifts by 1, 2, 4, 8, 16) correctly rounds up any value ≤ 2³¹ − 1. There’s no overflow or missing shift for the range you’re testing—feel free to ignore the previous compatibility concern.Likely an incorrect or invalid review comment.
pkg/merkle/hash_test.go (4)
1-9: Package separation improves test isolationGood change to move the test package to
merkle_testand explicitly import themerklepackage. This ensures proper separation between implementation and tests, and verifies the public API works as expected.
186-359: Comprehensive tests for HashLeaf functionThe new
TestHashLeaffunction provides thorough testing of the leaf hashing functionality with multiple test cases. Using raw byte slices for expected values makes the tests more precise than hex-encoded strings, though it does make the test file significantly longer.
1008-1402: Complete test coverage for HashRootThe
TestHashRootfunction thoroughly tests the root hashing with various leaf counts, including error cases:
- Valid leaf counts: 1, 78, 46984
- Invalid leaf counts: negative numbers and values exceeding 31-bit signed integers
This comprehensive testing ensures the leaf count validation in
HashRootworks correctly.
1579-1919: Thorough validation testing in Bytes32ToIntThe
TestBytes32ToIntfunction includes robust error case testing:
- Buffers with incorrect length (less than or more than 32 bytes)
- Non-zero values in the first 28 bytes
- Values exceeding the maximum signed 32-bit integer
These tests ensure the validation logic in
Bytes32ToIntcorrectly handles all edge cases.pkg/merkle/proof_test.go (1)
293-429: Well-structured test cases with fixed test vectors!The use of fixed, hex-encoded test vectors for leaves and expected proof elements provides excellent verification of the sequential proof implementation. This approach makes it easier to validate against reference implementations and ensures consistent behavior across platforms.
pkg/merkle/tree.go (1)
173-227: Excellent rewrite of the tree construction algorithm.The rewritten
makeTreefunction correctly implements the 0-indexed tree representation and properly handles unbalanced trees with pairless nodes. The separation of leaf node hashing inmakeLeafNodesand the use of domain-specific hashing functions improve the clarity and correctness of the code.pkg/merkle/proof.go (1)
201-286: Efficient circular queue implementation for proof verification.The redesigned proof verification algorithm using a fixed-size circular queue is an elegant solution that mirrors the reference JavaScript and Solidity implementations. The approach efficiently processes nodes level by level, handling both balanced and unbalanced trees correctly.
pkg/merkle/tree_test.go (1)
459-658: Excellent coverage with concrete test vectors.The sample-based tests with fixed hex-encoded inputs and expected outputs provide strong verification of the Merkle tree implementation. These tests ensure consistency with reference implementations and make it easier to validate correctness across different platforms.
| leaves: []string{ | ||
| "4a864e860c0d0247c6aa5ebcb2bc3f15fc4ddf86213258f4bf0b72e51c9d9c69", | ||
| "51b7ae2bab96bd3fbb3b26e1efefb0b9b6a60054ed7ffcfa700374d58f315a31", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to prevent silent failures.
The getBytesFromHexString function ignores errors from hex.DecodeString(), which could lead to unexpected behavior if invalid hex strings are provided.
func getBytesFromHexString(s string) []byte {
- decoded, _ := hex.DecodeString(s)
+ decoded, err := hex.DecodeString(s)
+ if err != nil {
+ panic(fmt.Sprintf("Invalid hex string: %s, error: %v", s, err))
+ }
return decoded
}Since this is a test helper, panicking on invalid input is appropriate. Alternatively, you could modify the function to return the error and handle it in each test.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/merkle/proof.go (1)
285-287: Remove unreachable code.Line 287 contains no code and is unreachable. This was flagged in a previous static analysis.
-} - +}
🧹 Nitpick comments (6)
pkg/merkle/proof.go (6)
161-164: Consider adding empty proofElements check for defensive programming.While
validate()should catch empty proof elements before this function is called, adding an explicit check here as well would make this function more robust when used independently.- leafCount, err := Bytes32ToInt(p.proofElements[0]) + if len(p.proofElements) == 0 { + return nil, ErrNoProofs + } + leafCount, err := Bytes32ToInt(p.proofElements[0])
201-201: Consider using a more explicit loop condition.While using
for {}with early returns is valid, using a more explicit condition would make the code's intent clearer.- for { + for readIdx < queueLen || writeIdx < 2*queueLen {
192-198: Consider moving variable declarations closer to their usage.The
rightandleftvariables are reassigned in each iteration of the loop. Defining them inside the relevant code blocks could improve readability and make the code easier to follow.var ( - readIdx, writeIdx, proofIdx = 0, 0, 1 - upperBound = balancedLeafCount + leafCount - 1 - lowestTreeIndex = balancedLeafCount + p.startingIndex - right []byte - left []byte + readIdx, writeIdx, proofIdx = 0, 0, 1 + upperBound = balancedLeafCount + leafCount - 1 + lowestTreeIndex = balancedLeafCount + p.startingIndex )Then declare
rightandleftat the beginning of their respective blocks:// Inside the if-else blocks where right is assigned var right []byte // Inside the if-else blocks where left is assigned var left []byte
201-285: Consider breaking down the complex computeRoot loop into smaller functions.The main loop in
computeRootis quite complex with multiple nested conditions and different behaviors based on node indices. Breaking it down into smaller, well-named helper functions would improve readability and maintainability.For example:
- A function to handle the sub-root case
- A function to handle pairless nodes
- A function to determine the right hash
- A function to determine the left hash
This would make the algorithm's logic more apparent and easier to review.
271-278: The check and comment logic could be clarified.The comment contains a complex explanation that could be simplified or complemented with a clearer conditional statement.
- // If we are not at the lowest tree index (i.e. there are nodes to the left that we have yet to process at this - // level), then continue. - // NOTE: Technically, if only `nextNodeIndex_ == lowestTreeIndex_`, and we did not use the hash at that - // `nextNodeIndex_` as part of this step's hashing, then it was a node not yet handled, but it will be handled - // in the next iteration, so the process will continue normally even if we prematurely "leveled up". - if nodeIdx != lowestTreeIndex && nextNodeIdx != lowestTreeIndex { + // Continue processing at the current level if there are more nodes to the left + // (neither the current nor the next node is at the lowest index for this level) + if nodeIdx != lowestTreeIndex && nextNodeIdx != lowestTreeIndex {
87-90: Use GetLeafCount to avoid code duplication.Consider using the already defined
GetLeafCountmethod here to avoid code duplication and improve maintainability.- leafCount, err := Bytes32ToInt(p.proofElements[0]) - if err != nil { - return fmt.Errorf(ErrVerifyProof, err) - } + leafCount, err := p.GetLeafCount() + if err != nil { + return fmt.Errorf(ErrVerifyProof, err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/merkle/proof.go(3 hunks)
🔇 Additional comments (4)
pkg/merkle/proof.go (4)
34-40: Good implementation of error checking for GetLeafCount.The additional check for empty proof elements addresses prior feedback and prevents potential panics when accessing
p.proofElements[0].
50-56: Constructor function for enhanced usability is a good addition.This new function aligns well with your goal of simplifying API usage by allowing proof verification without constructing the entire tree. As you mentioned in your response to the previous review, this supports both testing scenarios and API use cases where one might need to build proofs directly.
83-85: Good addition of empty proof elements check.This validation addresses the previously identified issue where accessing
p.proofElements[0]without checking if the slice is non-empty could result in a runtime panic.
166-176: Good handling of edge cases.The special handling for empty trees and cases where no leaves need to be proven improves the robustness of the implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
pkg/merkle/tree.go (1)
9-22:⚠️ Potential issueInconsistent documentation with implementation
The comment for the
MerkleTreestruct describes a 1-indexed tree, but the implementation has been changed to be 0-indexed based on the PR objectives and themakeTreefunction comments.// MerkleTree is a binary Merkle tree. // // tree has the collection of nodes, where: - // - The tree is 1-indexed, so root is at index 1. - // - The internal nodes are at index 1 to N-1. - // - The leaves are at index N to 2N-1. + // - The tree is 0-indexed, so root is at index 0. + // - The internal nodes are at index 1 to N-1. + // - The leaves are at index N to 2N-1. type MerkleTree struct { tree []Node leaves []Leaf }
🧹 Nitpick comments (4)
pkg/merkle/proof_internal_test.go (2)
203-219: Check queue order and values based on the new zero-indexed structureThe test correctly validates the queue order, but the index values should be updated to match the new zero-indexed tree structure mentioned in the PR objectives and implemented in
tree.go.
237-238: Consider explicit testing of the sequential nature of proofsThe test uses
GenerateMultiProofSequentialbut doesn't explicitly verify that the proof is sequential. Consider adding assertions to confirm that the generated proof has contiguous leaves starting from index 0.// Test with valid proof. multiProof, err := tree.GenerateMultiProofSequential(0, 2) require.NoError(t, err) +// Verify that the proof contains 2 sequential leaves starting from index 0 +assert.Equal(t, 0, multiProof.startingIndex) +assert.Equal(t, 2, len(multiProof.leaves)) +assert.Equal(t, leaves[0], multiProof.leaves[0]) +assert.Equal(t, leaves[1], multiProof.leaves[1])pkg/merkle/hash.go (1)
101-120: Good validation in Bytes32ToInt, but consider using binary.BigEndian.Uint32The function properly validates the buffer length and ensures the first 28 bytes are 0, which is a significant improvement for error handling. However, manual extraction of the uint32 value could be simplified by using binary.BigEndian.Uint32.
func Bytes32ToInt(buffer []byte) (int, error) { if len(buffer) != 32 { return 0, ErrInvalidBufferLength } // Check that all of the first 28 bytes are 0 for i := 0; i < 28; i++ { if buffer[i] != 0 { return 0, ErrInvalidBytes32ToIntInput } } - uint32Value := binary.BigEndian.Uint32(buffer[28:]) + uint32Value := binary.BigEndian.Uint32(buffer[28:32]) if uint32Value > 1<<31-1 { return 0, ErrInvalidBytes32ToIntInput } return int(uint32Value), nil }pkg/merkle/tree.go (1)
226-231: Error handling for HashRootThe code now properly handles errors from
HashRoot, but consider adding a more descriptive error message that includes the original error.tree[0], err = HashRoot(leafCount, tree[1]) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to hash root: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/merkle/hash.go(3 hunks)pkg/merkle/hash_test.go(1 hunks)pkg/merkle/proof_internal_test.go(4 hunks)pkg/merkle/tree.go(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
pkg/merkle/hash.go (2)
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/hash.go:58-72
Timestamp: 2025-05-01T09:37:04.365Z
Learning: The `HashRoot(leafCount int, root []byte)` function in `pkg/merkle/hash.go` accepts a potentially negative `int` value for `leafCount` and converts it to `uint64` without validation, which could lead to silent overflow or unexpected behavior. The team plans to address this in a future update to maintain API consistency.
Learnt from: deluca-mike
PR: xmtp/xmtpd#749
File: pkg/merkle/hash.go:82-90
Timestamp: 2025-05-08T17:09:55.763Z
Learning: The `IntToBytes32(value int)` function in `pkg/merkle/hash.go` will be updated to return an error when input values are out of bounds (negative or exceeding the max 31-bit integer range), rather than silently converting them to uint64.
pkg/merkle/tree.go (1)
Learnt from: mkysel
PR: xmtp/xmtpd#712
File: pkg/merkle/hash.go:7-9
Timestamp: 2025-04-11T20:14:15.793Z
Learning: In Merkle tree implementations, domain separation is important for security. Use either explicit prefixes for both leaf and internal nodes (e.g., "leaf|" and "node|") or double hashing to prevent second preimage attacks. Simply prefixing leaf nodes without prefixing internal nodes doesn't provide true domain separation and could be vulnerable to attacks.
🧬 Code Graph Analysis (1)
pkg/merkle/hash_test.go (1)
pkg/merkle/hash.go (7)
Hash(27-31)HashLeaf(33-41)HashNodePair(43-52)HashPairlessNode(54-62)HashRoot(64-85)IntToBytes32(87-99)Bytes32ToInt(101-120)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
- GitHub Check: Code Review
🔇 Additional comments (17)
pkg/merkle/proof_internal_test.go (2)
136-151: LGTM: Good edge case coverage for single leaf treesThis test case correctly verifies that a single leaf tree only needs one proof element (the leaf count), which is an important edge case for Merkle trees.
155-182: LGTM: Clear test case forgetNextProofElementThe renamed test function and updated implementation properly test the functionality of retrieving proof elements sequentially from the proof elements array.
pkg/merkle/hash.go (3)
87-99: Improved validation in IntToBytes32The function now properly validates input values and returns errors for out-of-bounds inputs, addressing the learning from previous reviews. This is a significant improvement for error handling.
I see you've implemented the error handling we discussed in previous reviews. This is exactly what was needed to prevent silent overflow or unexpected behavior.
16-24: Well-structured error variables and domain separationThe domain separation prefixes and corresponding error variables are well-defined. Using specific error variables for each validation failure makes error handling clearer for callers.
The implementation follows best practices for domain separation in cryptographic hashing, addressing the learned security practices from previous reviews.
64-85:Details
❌ Incorrect review comment
Add bounds checking to ensure root isn't nil
The
HashRootfunction properly validates the leaf count but doesn't check if therootparameter is nil. Since this function is the final step in building a Merkle tree, adding nil validation would prevent potential nil pointer dereferences when calculating the length of the root.func HashRoot(leafCount int, root []byte) ([]byte, error) { if leafCount < 0 || leafCount > (1<<31)-1 { return nil, ErrInvalidLeafCount } + + if root == nil { + return nil, errors.New("root cannot be nil") + } leafCountBytes, err := IntToBytes32(leafCount) if err != nil { return nil, err } rootPrefixLen := len(rootPrefixBytes) leafCountLen := len(leafCountBytes) // Length of the byte representation rootLen := len(root) buffer := make([]byte, rootPrefixLen+leafCountLen+rootLen) copy(buffer[:rootPrefixLen], rootPrefixBytes) copy(buffer[rootPrefixLen:rootPrefixLen+leafCountLen], leafCountBytes) // Copy the bytes copy(buffer[rootPrefixLen+leafCountLen:], root) // Copy the root return Hash(buffer), nil }Let's verify if this is necessary:
🏁 Script executed:
#!/bin/bash # Check if HashRoot is ever called with a nil root parameter grep -rn "HashRoot" --include="*.go" . | grep -v "func HashRoot" | grep -v "test"Length of output: 327
🏁 Script executed:
#!/bin/bash # Display contents of pkg/merkle/hash.go for review sed -n '1,200p' pkg/merkle/hash.goLength of output: 2978
Unnecessary nil check for slices
In Go, calling
lenorcopyon a nil slice is safe (it treats a nil slice as length 0), soHashRootwon’t panic ifrootis nil. The proposedif root == nilguard is therefore unnecessary. You can safely ignore this change.Likely an incorrect or invalid review comment.
pkg/merkle/hash_test.go (6)
1-9: LGTM: Package separation for testsUsing
package merkle_testinstead ofpackage merkleis a good practice that enforces proper API design by ensuring tests only access public APIs.
186-359: Good test coverage for HashLeaf functionThe test cases for
HashLeafprovide good coverage with various input sizes. Using byte arrays directly instead of hex strings makes the tests more efficient.
701-1006: LGTM: Comprehensive tests for HashPairlessNodeThe test cases for
HashPairlessNodethoroughly verify domain separation and correct hashing with various input types, including all zeros and all ones.
1008-1402: Good error case coverage for HashRootThe test cases for
HashRootinclude both valid inputs and various error cases, validating that proper error handling is implemented.
1404-1665: LGTM: Thorough validation testing for IntToBytes32The test cases cover the complete range of valid and invalid inputs, ensuring the function correctly handles all edge cases.
1667-2007: Comprehensive input validation tests for Bytes32ToIntThe test cases for
Bytes32ToIntthoroughly verify error handling for invalid buffer length, non-zero bytes in the first 28 positions, and values exceeding the max signed 32-bit value.pkg/merkle/tree.go (6)
153-160: Fixed potential nil pointer dereferenceThe updated
Rootmethod now correctly checks if the tree array is empty before attempting to access its first element. This addresses the concern raised in a previous review comment and prevents a potential index out-of-bounds panic.
83-90: LGTM: Leaf count is now properly structured as proof elementThe implementation now correctly includes the leaf count as the first proof element, which aligns with the PR objectives and allows for trivial leaf count proofs.
209-214: LGTM: Properly handling pairless nodesThe implementation now correctly handles pairless nodes by hashing them with a node prefix instead of pairing them with nil siblings, which aligns with the PR objectives.
234-247: LGTM: Proper validation against nil leavesThe
makeLeafNodesfunction now explicitly checks for nil leaves and returns an appropriate error, which aligns with the PR objectives of disallowing nil leaves.
265-270: Good handling of special case for trees with a single leafThe implementation correctly identifies that a tree with a single leaf requires special handling, as it needs both a leaf hash and a node hash. This aligns with the PR objectives.
300-312: LGTM: Simple and effective indices generationThe
makeIndicesfunction provides a clean implementation for generating sequential indices, with proper validation for negative inputs.
| leaves := []Leaf{[]byte("test")} | ||
|
|
||
| proofs := []Node{ | ||
| {hash: []byte("proof")}, | ||
| } | ||
| leafCountBytes, err := IntToBytes32(2) | ||
| require.NoError(t, err) | ||
| proofElements := []ProofElement{ProofElement(leafCountBytes), []byte("proof")} | ||
|
|
||
| proof := MultiProof{ | ||
| values: values, | ||
| proofs: proofs, | ||
| leafCount: 2, | ||
| startingIndex: 0, | ||
| leaves: leaves, | ||
| proofElements: proofElements, | ||
| } | ||
|
|
||
| err := proof.validate() | ||
| err = proof.validate() | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify proper error handling in case of failure
The test creates a valid proof with a leafCount of 2 and a starting index of 0, but it has only one leaf in the array. This could cause issues when verifying proofs for larger trees.
Adding a test case where the number of leaves matches the leaf count would provide better coverage.
func TestValidate(t *testing.T) {
t.Run("valid proof", func(t *testing.T) {
leaves := []Leaf{[]byte("test")}
leafCountBytes, err := IntToBytes32(2)
require.NoError(t, err)
proofElements := []ProofElement{ProofElement(leafCountBytes), []byte("proof")}
proof := MultiProof{
startingIndex: 0,
leaves: leaves,
proofElements: proofElements,
}
err = proof.validate()
assert.NoError(t, err)
})
+ t.Run("valid proof with matching leaf count", func(t *testing.T) {
+ leaves := []Leaf{[]byte("test1"), []byte("test2")}
+
+ leafCountBytes, err := IntToBytes32(2)
+ require.NoError(t, err)
+ proofElements := []ProofElement{ProofElement(leafCountBytes), []byte("proof")}
+
+ proof := MultiProof{
+ startingIndex: 0,
+ leaves: leaves,
+ proofElements: proofElements,
+ }
+
+ err = proof.validate()
+ assert.NoError(t, err)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| leaves := []Leaf{[]byte("test")} | |
| proofs := []Node{ | |
| {hash: []byte("proof")}, | |
| } | |
| leafCountBytes, err := IntToBytes32(2) | |
| require.NoError(t, err) | |
| proofElements := []ProofElement{ProofElement(leafCountBytes), []byte("proof")} | |
| proof := MultiProof{ | |
| values: values, | |
| proofs: proofs, | |
| leafCount: 2, | |
| startingIndex: 0, | |
| leaves: leaves, | |
| proofElements: proofElements, | |
| } | |
| err := proof.validate() | |
| err = proof.validate() | |
| assert.NoError(t, err) | |
| func TestValidate(t *testing.T) { | |
| t.Run("valid proof", func(t *testing.T) { | |
| leaves := []Leaf{[]byte("test")} | |
| leafCountBytes, err := IntToBytes32(2) | |
| require.NoError(t, err) | |
| proofElements := []ProofElement{ProofElement(leafCountBytes), []byte("proof")} | |
| proof := MultiProof{ | |
| startingIndex: 0, | |
| leaves: leaves, | |
| proofElements: proofElements, | |
| } | |
| err = proof.validate() | |
| assert.NoError(t, err) | |
| }) | |
| t.Run("valid proof with matching leaf count", func(t *testing.T) { | |
| leaves := []Leaf{[]byte("test1"), []byte("test2")} | |
| leafCountBytes, err := IntToBytes32(2) | |
| require.NoError(t, err) | |
| proofElements := []ProofElement{ProofElement(leafCountBytes), []byte("proof")} | |
| proof := MultiProof{ | |
| startingIndex: 0, | |
| leaves: leaves, | |
| proofElements: proofElements, | |
| } | |
| err = proof.validate() | |
| assert.NoError(t, err) | |
| }) | |
| } |
| // | ||
| // The tree is 1-indexed, so root is at index 1. | ||
| // The internal nodes are at index 2 to N. | ||
| // The leaves are at index N+1 to 2N-1. | ||
| // For a tree that, when balanced, would have N leaves: | ||
| // The root is at index 0. | ||
| // The internal nodes are at index 1 to N-1. | ||
| // The leaves are at index N to 2N-1. | ||
| // | ||
| // For any node at index i: | ||
| // - left child is at index 2*i | ||
| // - right child is at index 2*i+1 | ||
| // - parent is at floor(i/2) | ||
| func makeTree(nodes []Node) ([]Node, error) { | ||
| if len(nodes) == 0 { | ||
| return nil, ErrTreeEmpty | ||
| } | ||
| func makeTree(leafNodes []Node) ([]Node, error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Updated tree indexing documentation
The documentation for makeTree correctly describes the new 0-indexed tree structure, but it conflicts with the MerkleTree struct documentation. Make sure all documentation is consistent.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := roundUpToPowerOf2(tt.input) | ||
| if result != tt.expected { |
There was a problem hiding this comment.
require.Equal(t, result, tt.expected) is a nice shorthand for this
Implement matched sequential proof verification in the merkle package to support unbalanced trees
The merkle package has been refactored with a new proof verification system that supports unbalanced trees. Key changes include:
MultiProofstruct in proof.go to use sequential leaves with starting indicesNodetype to a byte slice and updated tree indexing in tree.go📍Where to Start
Start with the
NewMerkleTreefunction in tree.go which implements the core tree construction logic, followed by the proof verification implementation in proof.go.Macroscope summarized d8dd4d9.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests