Add pkg merkle #712
Add pkg merkle #712
Conversation
WalkthroughThis pull request introduces several new files in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MT as MerkleTree
participant ML as makeNodes
participant MTREE as makeTree
U->>MT: NewMerkleTree(leaves)
MT->>ML: makeNodes(leaves)
ML-->>MT: nodes (hashed leaves)
MT->>MTREE: makeTree(nodes)
MTREE-->>MT: tree structure & root
MT-->>U: MerkleTree instance
sequenceDiagram
participant U as User
participant MT as MerkleTree
participant GP as GenerateMultiProof
participant VP as Verify
U->>MT: GenerateMultiProofSequential(startIndex, count)
MT->>GP: makeProof(tree, root, indices, leafCount)
GP-->>MT: MultiProof instance
MT-->>U: MultiProof instance
U->>VP: Verify(root, MultiProof)
VP->>VP: validate inputs and proof
VP->>VP: computeRoot from elements and proofs
VP-->>U: Verification result (true/false, error)
sequenceDiagram
participant U as User
participant MT as MerkleTree
participant GP as GenerateMultiProofWithIndices
participant VP as Verify
U->>MT: GenerateMultiProofWithIndices(indices)
MT->>GP: makeProof(tree, root, indices, leafCount)
GP-->>MT: MultiProof instance
MT-->>U: MultiProof instance
U->>VP: Verify(root, MultiProof)
VP->>VP: validate inputs and proof
VP->>VP: computeRoot from elements and proofs
VP-->>U: Verification result (true/false, error)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
pkg/merkle/hash.go (1)
11-16: Consider using the direct Summation-based Keccak256 helper for clarity.
Usingsha3.NewLegacyKeccak256()andhash.Write(buffer)is totally valid here. However, if performance or simplicity is desired when hashing single buffers, you might consider using a direct summation approach such assha3.SumLegacyKeccak256(buffer)to avoid manual hasher creation and writing.pkg/merkle/proof_indices.go (3)
43-43: Consolidation TODO.
The plan to abstract out shared logic from both sequential and index-based verification is laudable. This can reduce code duplication in future.
44-81: Verification quietly returns (false, nil) on invalid proof.
WhengetRootIndicesreturnsnil, the code returns(false, nil)rather than an error. Consider returning(false, error)to provide clearer diagnostic feedback to callers.
83-212: Complex but logical root computation.
While thorough and correct, this routine is quite large. You could consider modularizing it into smaller functions to simplify readability and maintenance.pkg/merkle/proof.go (2)
28-36: Consider validating out-of-range or duplicate indices ingenerateProof.Currently,
generateProofdoes not explicitly check whether indices exceed the tree’s range or if they contain duplicates. Although these checks may be performed in the calling functions, you might strengthen error safety by ensuring that indices are valid here as well, preventing potential out-of-bounds access on line 60 (leafCount + idx).
155-183: Potential performance bottleneck incombineLeaves.For very large sets of leaves, repeatedly creating a new level by hashing adjacent pairs could be optimized via parallel processing or by minimizing intermediate slice allocations. If performance at scale becomes a concern, consider a more efficient approach or concurrency in safe contexts.
pkg/merkle/tree.go (1)
19-26: Validate non-nil elements on tree creation.In
NewMerkleTree, there is no check for nil elements in the input slice. If an element is nil,HashLeaf(nil)might behave unexpectedly or degrade security. Consider validating them here to ensure consistent leaf data.pkg/merkle/proof_sequential.go (1)
89-200: Simplify or further documentgetRootSequentially.The circular queue approach is correct but quite complex, potentially impacting maintainability and readability. Adding more explanatory comments or breaking out sub-steps into helper functions may help future contributors understand and maintain this logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/merkle/hash.go(1 hunks)pkg/merkle/proof.go(1 hunks)pkg/merkle/proof_indices.go(1 hunks)pkg/merkle/proof_sequential.go(1 hunks)pkg/merkle/tree.go(1 hunks)pkg/merkle/tree_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/merkle/proof_indices.go (3)
pkg/merkle/tree.go (1)
MerkleTree(10-16)pkg/merkle/proof.go (1)
MultiProof(9-16)pkg/merkle/hash.go (2)
HashLeaf(24-26)HashNode(19-21)
pkg/merkle/proof_sequential.go (3)
pkg/merkle/tree.go (1)
MerkleTree(10-16)pkg/merkle/proof.go (1)
MultiProof(9-16)pkg/merkle/hash.go (2)
HashLeaf(24-26)HashNode(19-21)
pkg/merkle/proof.go (1)
pkg/merkle/hash.go (1)
HashNode(19-21)
⏰ 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: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
🔇 Additional comments (15)
pkg/merkle/hash.go (3)
7-9: Good use of a dedicated leaf prefix.
This clear separation of leaf nodes helps avoid potential collision or ambiguity in hashing different node types.
18-21: Straightforward node-hashing approach.
No functional concerns; this is a neat and consistent way to combine child nodes.
23-26: Good leaf-hashing pattern with the prefix.
This ensures that leaf nodes cannot collide with internal node concatenations, which is critical for Merkle tree security.pkg/merkle/tree_test.go (9)
10-18: Meaningful empty-tree test case.
Properly ensures that constructing an empty Merkle tree returns an error as expected.
20-81: Comprehensive balanced-tree scenarios.
These tests methodically verify that the tree’s depth, leaf count, and array size are computed accurately for powers-of-two element counts.
83-114: Robust unbalanced-tree testing.
Validates correctness when the element count isn’t a power of two. This coverage prevents overlooking off-by-one or indexing errors.
116-172: Large-tree stress tests.
These scenarios provide excellent coverage for performance and correctness under higher loads. Sampling leaves is a practical approach to keep tests efficient.
174-196: Duplicate elements test.
Well-handled test verifying that identical leaves yield identical hashes, while unique elements differ. Great for ensuring collision correctness.
198-216: Validation of large byte-slice elements.
Ensures the process scales to significant input sizes and the final root remains correct.
218-239: Empty-element handling.
Shows that empty leaves still result in a valid hash, which is critical for flexible usage scenarios.
241-285: Manual verification of internal nodes.
Covers indexing logic thoroughly in a small, unbalanced tree. Excellent approach for verifying complex tree structures.
286-332: Helper function for unbalanced structure integrity.
This function effectively checks parent-child relationships, ensuring no corner cases are missed.pkg/merkle/proof_indices.go (2)
9-41: Efficient multi-proof generation with early validations.
Duplicates and out-of-range checks guard correctness by ensuring that proof generation starts only with valid indices.
214-247: Robust proof indices validation.
Ensures the indices match the elements and fall within valid bounds, detecting duplicates early. This is crucial for the multi-proof’s reliability.pkg/merkle/proof.go (1)
98-132: Good coverage invalidateProofBase.This function thoroughly checks for nil roots, missing elements, invalid element counts, and partial proof decommitments. It effectively prevents many common proof pitfalls.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
pkg/merkle/tree.go
Outdated
|
|
||
| // roundUpToPowerOf2 rounds up a number to the next power of 2. | ||
| // Rounding up to the next power of 2 is necessary to ensure that the tree is balanced. | ||
| func roundUpToPowerOf2(number uint32) uint32 { |
There was a problem hiding this comment.
fancy bit smearing!
I like this one more than the btcd float conversion
There was a problem hiding this comment.
This might be even faster:
func roundUpToPowerOf2(n uint32) uint32 {
n--
n |= n >> 1
n |= n >> 2
n |= n >> 4
n |= n >> 8
n |= n >> 16
n++
return n
}
pkg/merkle/hash.go
Outdated
| // LEAF_PREFIX is the prefix for leaf nodes. | ||
| // It is used to distinguish leaf nodes from other types of nodes. | ||
| const LEAF_PREFIX = "leaf|" |
There was a problem hiding this comment.
you can't really retrieve the fact that some cryptographic hash has a LEAF| prefix by looking at the hash. So you can write it, but you can't ever read it out again.
What purpose does it serve?
There was a problem hiding this comment.
we can get into the weeds of cryptography here, but a standalone prefix can not solve domain separation.
If we assume that the purpose of this prefix is to prevent a malicious leaf input from colliding with an internal node operation, it does not quite work that way.
leaf| aka 0x4C4541467C is just a prefix. If you can find an internal node that starts with exactly these bytes you can craft a payload that is "(nothing instead of leaf|) || leaf1-suffix || leaf2) and you will get a collision.
I think that you need to prefix both the leaf and the internal node to achieve true domain separation.
There was a problem hiding this comment.
or just double hash :D https://github.com/OpenZeppelin/merkle-tree/blob/master/src/hashes.ts
There was a problem hiding this comment.
Hey @macroscopeapp. Hey @coderabbitai. Compare the correctness, structural integrity and safety of Hash(LEAF|payload) == Hash(leftHash || rightHash) vs Hash(LEAF|payload) == Hash(NODE|leftHash || rightHash) vs Hash(payload) == Hash(leftHash || rightHash)
There was a problem hiding this comment.
@mkysel Let me compare the three hashing approaches:
-
Hash(LEAF|payload) == Hash(leftHash || rightHash) (current implementation):
- Security: Vulnerable to second preimage attacks. An attacker could potentially craft a payload that when hashed without the LEAF prefix produces a hash that collides with an internal node hash.
- Integrity: Only partially protects the tree structure by distinguishing leaves from internal nodes in one direction.
- Safety: Insufficient domain separation as internal nodes aren't prefixed.
-
Hash(LEAF|payload) == Hash(NODE|leftHash || rightHash):
- Security: Much stronger. Creates true domain separation by ensuring leaf and internal node hashes come from distinct domains.
- Integrity: Fully protects the tree structure as neither node type can be substituted for the other.
- Safety: Prevents an attacker from constructing a leaf whose hash could be mistaken for a node hash and vice versa.
-
Hash(payload) == Hash(leftHash || rightHash):
- Security: Most vulnerable to second preimage attacks.
- Integrity: No structural protection; leaf nodes could be confused with internal nodes.
- Safety: Allows for potential substitution attacks.
The second approach with both prefixes is the most secure option as it implements true domain separation. Alternatively, the double hashing approach used by OpenZeppelin (that you referenced) also provides strong security by hashing twice rather than using prefixes.
For cryptographic security in Merkle trees, I would recommend either using both prefixes or implementing the double hashing approach.
✏️ Learnings added
Learnt from: mkysel
PR: xmtp/xmtpd#712
File: pkg/merkle/hash.go:7-9
Timestamp: 2025-04-11T20:14:15.760Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
This is really interesting. :)
I wrote a new HashNode which uses a prefix for nodes, and also uses a buffer for easy optimizations:
func HashNode(left, right []byte) []byte {
nodePrefixLen := len(nodePrefixBytes)
buffer := make([]byte, nodePrefixLen+len(left)+len(right))
copy(buffer[:nodePrefixLen], nodePrefixBytes)
copy(buffer[nodePrefixLen:], left)
copy(buffer[nodePrefixLen+len(left):], right)
return Hash(buffer)
}
It settles around ~278.0 ns/op with improved security, while the old HashNode marks 280 ns/op.
|
@coderabbitai can you generate a random stress test for the merkle tree implementation? Keep generating trees, adding nodes and asking for proofs for a configurable duration. The purpose of the stress test is to look for edge conditions and out-of-bounds errors. If you can, sanity check as much as possible. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@mkysel I'll create a comprehensive stress test for the Merkle tree implementation. First, let me check the code to understand the API properly. 🏁 Script executed: Length of output: 3933 🏁 Script executed: Length of output: 17615 🏁 Script executed: Length of output: 9737 & Validation"] --> @mkysel I'll create a comprehensive stress test for the Merkle tree implementation. After examining the code, I've designed a stress test that will thoroughly exercise the implementation to find any potential edge cases or out-of-bounds errors. The stress test will:
Another agent will come along to implement this plan. The stress test will include comprehensive reporting to help identify any potential issues. ⏳ Preparing changes... |
|
A summary of the edits CodeRabbit can apply:
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets): 📄 pkg/merkle/stress_test.goPlace the package declaration and necessary imports at the top of the file:
Define the main stress test function TestMerkleTreeStress. Within this function, configure parameters such as:
Implement helper functions:
definitions. Add two testing helper functions: At strategic points (for example, every 100 iterations) log the progress (iteration count, tree count, proof count, verifications, and failures) using t.Logf. After the loop, log the total elapsed time and the final statistics. If any verification failures were encountered, call t.Errorf with an appropriate message. Combine all the code segments, ensuring that all the helper functions are included in this new file. The final file should be pasted in its entirety under pkg/merkle/stress_test.go. A summary of the context that CodeRabbit is considering across the codebase:
|
|
@coderabbitai here is a set of rules https://github.com/xmtp/xmtpd/blob/main/.cursor/rules/100-go-mistakes.mdc, you can also find them here https://100go.co/. Can you check this PR for any violations and if you find any, make relevant comments inline? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3534 🏁 Script executed: Length of output: 729 🏁 Script executed: Length of output: 4780 🏁 Script executed: Length of output: 6512 🏁 Script executed: Length of output: 6193 🏁 Script executed: Length of output: 3875 🏁 Script executed: Length of output: 8931 🏁 Script executed: Length of output: 1982 @mkysel I've reviewed the PR against the 100 Go Mistakes rules and found a few issues that could be addressed:
Would you like more detailed comments on any of these particular issues? |
Changes in dcc177c
Macroscope summarized dcc177c. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
pkg/merkle/proof_indices.go (2)
9-16: Check negative indices in a dedicated error branch
WhilehasOutOfBoundslikely covers negative indices by extension, consider adding an explicit negative-index check to provide clearer error messages for invalid inputs.
57-185: Consider refactoringgetRootIndices
This function is lengthy, with multiple index variables. Extracting chunks of logic (e.g., sibling checks, proof consumption) into helper functions or adding in-code comments can improve long-term maintenance and reduce confusion.pkg/merkle/proof_sequential.go (2)
7-18: Check for negative or zerocount
Lines 11-12 only validate thatstartingIndex+count <= leafCount. If a caller mistakenly passes a negative or zero value forcount, the error might be cryptic. Consider adding an explicit check forcount <= 0to fail fast.
57-168: Large function complexity
The nested index manipulations can be tricky to follow. Splitting out repeated logic (e.g., sibling detection, proof consumption) might help reduce complexity for future maintainers.pkg/merkle/tree.go (2)
9-16: Consider a custom node type
Storing raw[][]byteis flexible yet can hinder readability. A small wrapper type—liketype Node struct { Hash []byte }—can make it clearer where each element is used as a node.
120-155: Edge case for a single leaf
getDepth(1)returns 0, which is valid unless your application expects a baseline depth of 1. You might clarify in docs or error-check if single-element trees pose any special constraints.pkg/merkle/proof.go (5)
19-29: Consider removing or consolidating unused error constants.Some of these declared constants (e.g.,
ErrProofInvalidElementCount,ErrProofInvalidRange, etc.) do not appear to be used, while other errors are declared inside the functions usingfmt.Errorf(...). For clarity, either remove the unused constants or standardize their usage throughout the file.
118-135: Rename "leafs" to "leaves" for clarity.For readability and consistency, consider renaming the
leafsvariable toleaves, which is more standard English usage.- root := HashLeaf(proof.Elements[0]) ... - leafs := make([][]byte, len(proof.Elements)) - for i, element := range proof.Elements { - leafs[i] = HashLeaf(element) - } + leaves := make([][]byte, len(proof.Elements)) + for i, element := range proof.Elements { + leaves[i] = HashLeaf(element) + } ... - root = combineLeaves(leafs) + root = combineLeaves(leaves)
150-184: Standardize error handling for proof validation.This block uses
fmt.Errorf(...)to create new errors, whereas multiple proof-related error constants are declared globally. For consistency, either replace these inline errors with the existing constants (e.g.,ErrProofNoElements, etc.) or remove the unused constants.
206-235: Consider parallelizing leaf combination for large inputs.In
combineLeaves, the algorithm sequentially calculates parent hashes. If you anticipate very large input sets and need higher performance, parallelizing this level-building process (e.g., using goroutines) may significantly reduce computation time. This is optional and depends on performance requirements.
237-244: Evaluate whether buffer cloning is necessary.
cloneBufferduplicates each byte slice. If you can guarantee no modifications to the original slice after this function call, skipping the clone operation would improve performance and reduce allocations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/merkle/proof.go(1 hunks)pkg/merkle/proof_indices.go(1 hunks)pkg/merkle/proof_sequential.go(1 hunks)pkg/merkle/tree.go(1 hunks)pkg/merkle/tree_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/merkle/tree_test.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/merkle/proof_indices.go (3)
pkg/merkle/tree.go (1)
MerkleTree(10-16)pkg/merkle/proof.go (1)
MultiProof(10-17)pkg/merkle/hash.go (1)
HashNode(19-21)
pkg/merkle/proof.go (1)
pkg/merkle/hash.go (2)
HashLeaf(24-26)HashNode(19-21)
pkg/merkle/proof_sequential.go (3)
pkg/merkle/tree.go (1)
MerkleTree(10-16)pkg/merkle/proof.go (1)
MultiProof(10-17)pkg/merkle/hash.go (1)
HashNode(19-21)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Code Review
🔇 Additional comments (7)
pkg/merkle/proof_indices.go (2)
30-38: Sorting modifies the proof indices
Because the proof is returned with sorted indices, the user’s original index ordering will differ. If the intended behavior is to preserve the exact ordering, you might keep a separate unsorted copy or document that the proof uses sorted indices.
189-220: Thorough validation
The index validation logic (checking for out-of-range, duplicates, etc.) is solid and ensures data integrity for multi-proof verification.pkg/merkle/proof_sequential.go (2)
30-35: Verifying array boundaries
This loop retrievesm.elements[startingIndex + i]. Although line 11 covers the range, it’s prudent in critical systems to re-validate in the loop or add an explanatory comment ensuring thatiwon’t exceed bounds.
170-208: Neat sequential validation
The checks for sequential coherence (StartingIndex + i) are logical, and optional index validation is well thought out.pkg/merkle/tree.go (2)
18-38: Tree initialization is straightforward
Instantiating leaves, building the tree, and storing the root is logically consistent and meets common Merkle tree expectations.
65-118: Confirm partial coverage handling
The loop skips building certain parent nodes whenleftChildIndex > upperBound. This is often correct for Merkle trees with unpopulated leaves, but please confirm it aligns with your usage requirements (e.g., no unexpected nil parents).pkg/merkle/proof.go (1)
65-72: Ensure out-of-bounds protection for 'leafCount + idx'.When marking known leaves, the code does not guard against cases in which
leafCount + idxmight exceed the array bounds oftree. Relying on the caller to validate the indices can be risky because a single invalid index can panic the entire function. Consider a protective check or an explicit error if the sum exceeds the slice length.
Changes in a252245
Macroscope summarized a252245. |
Changes in a4b966f
Macroscope summarized a4b966f. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/merkle/proof.go (2)
336-339: Comment contradicts implementation
isLeftChildreturnstruefor even indices (left child), yet the comment claims the opposite. Please update the comment (or the code) for clarity.
77-85:⚠️ Potential issueAdd a nil–check for
proofto prevent panics
Verifydereferencesproofimmediately (proof.validate()), so a caller that accidentally passesnilwill trigger a runtime panic. A tiny guard avoids the crash and surfaces a clear error:func Verify(root []byte, proof *MultiProof) (bool, error) { - if len(root) == 0 { + if proof == nil { + return false, fmt.Errorf(ErrVerifyProof, ErrNilProof) + } + if len(root) == 0 { return false, fmt.Errorf(ErrVerifyProof, ErrNilRoot) } if err := proof.validate(); err != nil { return false, fmt.Errorf(ErrVerifyProof, err) }pkg/merkle/tree.go (1)
294-306: 🛠️ Refactor suggestion
roundUpToPowerOf2still overflows formath.MaxUint32
n + 1wraps to0for0xFFFFFFFF, which later converts to a negativeinton 32‑bit builds.
Either cap the input earlier or guard here:- return n + 1 + if n == ^uint32(0) { + return n // already max; cannot round further + } + return n + 1
🧹 Nitpick comments (4)
pkg/merkle/tree.go (3)
24-27: Fix off‑by‑one / typo in public documentationThe comment says
- The hashed are at index N+1 to 2N-1.but leaves actually occupy indices
N…2N‑1(seelowerBound := balancedLeafCount). Mis‑documenting the layout makes the public API harder to reason about—please adjust.
10-16: Expose a copy to keep node hashes immutable
Hash()returns the backing slice, so callers can mutate it and silently corrupt the tree/proofs:func (n *Node) Hash() []byte { - return n.hash + dup := make([]byte, len(n.hash)) + copy(dup, n.hash) + return dup }Alternatively, document that callers must treat the slice as read‑only.
46-54: Deep‑copy individual leaves to avoid shared mutability
copy(leavesDeepCopy, leaves)duplicates the slice header only; each[]byteelement still aliases the caller’s buffer.
Either:
- Clarify in docs that the caller must not mutate after construction, or
- Perform a per‑element copy:
leavesDeepCopy := make([]Leaf, len(leaves)) for i, l := range leaves { cpy := make([]byte, len(l)) copy(cpy, l) leavesDeepCopy[i] = cpy }pkg/merkle/proof_test.go (1)
441-445: Optional: assert immutability rather than equality onlyThe test correctly checks value equality, but not that the returned slices are copies (immutable).
If you adopt the earlierHash()/ leaf‑copy changes, consider extending the test:orig := values[0].GetValue() orig[0] ^= 0xFF require.False(t, bytes.Equal(values[0].GetValue(), orig), "proof exposes mutable slice")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/merkle/hash.go(1 hunks)pkg/merkle/proof.go(1 hunks)pkg/merkle/proof_test.go(1 hunks)pkg/merkle/tree.go(1 hunks)pkg/merkle/tree_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/merkle/hash.go
- pkg/merkle/tree_test.go
🧰 Additional context used
🧠 Learnings (1)
pkg/merkle/proof.go (2)
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:0-0
Timestamp: 2025-04-14T12:55:15.436Z
Learning: In Merkle tree implementations, when indices are validated using a function like `validateIndices(indices, elementCount)` which checks that indices are less than the element count, additional bounds checking for accessing tree nodes at `leafCount+idx` is redundant because the element count corresponds to the number of leaf nodes.
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:71-72
Timestamp: 2025-04-15T11:29:03.575Z
Learning: In Go, when a byte slice (`[]byte`) is nil, `len(slice)` returns 0. Therefore, checking `if len(slice) == 0` will catch both nil slices and empty slices (initialized but with no elements).
🧬 Code Graph Analysis (1)
pkg/merkle/tree.go (2)
pkg/merkle/hash.go (4)
Hash(18-22)HashEmptyLeaf(45-47)HashNode(24-33)HashLeaf(35-43)pkg/merkle/proof.go (2)
MultiProof(28-32)ErrGenerateProof(11-11)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
pkg/merkle/tree.go (1)
294-306: Potential overflow inroundUpToPowerOf2remains unaddressedPrior reviews noted that
n + 1can wrap to 0 whenn == ^uint32(0).
AlthoughCalculateBalancedNodesCountnow caps inputs, the helper is still callable elsewhere and is safer with an explicit guard.(See earlier comment; not repeated here.)
pkg/merkle/proof.go (3)
77-85: Nil‑proof dereference can panic
Verifydereferencesproofwithout checking fornil; passing anilpointer will panic.func Verify(root []byte, proof *MultiProof) (bool, error) { + if proof == nil { + return false, fmt.Errorf(ErrVerifyProof, ErrNilProof) + } if len(root) == 0 { return false, fmt.Errorf(ErrVerifyProof, ErrNilRoot) }(This issue was flagged previously and is still outstanding.)
291-311: Indices are validated against padded leaf count, allowing empty‑leaf proofs
validateIndicesbounds‑checks againstbalancedLeafCount, which includes the synthetic padding leaves.
Callers can therefore request proofs for indices that have no real data (e.g. index 3 in a 3‑leaf tree padded to 4).Re–use the actual
leafCountinstead:- balancedLeafCount, err := CalculateBalancedNodesCount(leafCount) - if err != nil { - return err - } ... - if hasOutOfBounds(sortedIndices, balancedLeafCount) { + if hasOutOfBounds(sortedIndices, leafCount) { return ErrIndicesOutOfBounds }Prevents silent acceptance of invalid proofs.
336-339: Comment contradicts implementation
isLeftChildreturnstruefor even indices, but the comment claims the opposite. Correcting the comment avoids mental traps.-// isLeftChild returns true if the given index is odd (right child). +// isLeftChild returns true if the given index is even (left child).
🧹 Nitpick comments (4)
pkg/merkle/tree.go (3)
20-26: Fix inaccurate documentation of slice layoutThe comments describing the 1‑indexed layout are off by one:
- Internal nodes occupy indices 2 … N‑1 (not 2 … N).
- Leaves sit at indices N … 2N‑1 (not N+1 … 2N‑1).
- “…hashed” → “…leaves”.
Keeping these doc‑strings correct prevents future off‑by‑one bugs for anyone maintaining the code.
-// - The internal nodes are at index 2 to N. -// - The hashed are at index N+1 to 2N-1. +// - Internal nodes are at indices 2 to N‑1. +// - Leaves are at indices N to 2N‑1.
14-16: Return a defensive copy to prevent external mutation
Hash()exposes the internal byte‐slice directly. A caller can mutate the slice in‑place and corrupt the tree.-func (n *Node) Hash() []byte { - return n.hash +func (n *Node) Hash() []byte { + // Return a copy so callers cannot mutate internal state. + if n.hash == nil { + return nil + } + cp := make([]byte, len(n.hash)) + copy(cp, n.hash) + return cp }
46-57: Double copying of leaves is unnecessary
NewMerkleTreedeep‑copiesleavesintoleavesDeepCopy, thenmakeLeavescopies again.
Unless the original slice must remain untouched, a single copy insidemakeLeavessuffices and avoids an extra allocation.Refactor idea:
- Drop
leavesDeepCopyaltogether.- Let
makeLeaveshandle the copy/padding.This reduces allocations for large trees.
pkg/merkle/proof.go (1)
203-206: Remove unused variable to silencego vet
upperBoundis declared but never referenced after this point.- upperBound = blc + p.leafCount - 1 + // upperBound was part of an earlier algorithm; remove to avoid dead code.An unused local will trigger
go vetwarnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/merkle/hash.go(1 hunks)pkg/merkle/proof.go(1 hunks)pkg/merkle/proof_test.go(1 hunks)pkg/merkle/tree.go(1 hunks)pkg/merkle/tree_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/merkle/hash.go
- pkg/merkle/tree_test.go
- pkg/merkle/proof_test.go
🧰 Additional context used
🧠 Learnings (1)
pkg/merkle/proof.go (2)
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:0-0
Timestamp: 2025-04-14T12:55:15.436Z
Learning: In Merkle tree implementations, when indices are validated using a function like `validateIndices(indices, elementCount)` which checks that indices are less than the element count, additional bounds checking for accessing tree nodes at `leafCount+idx` is redundant because the element count corresponds to the number of leaf nodes.
Learnt from: fbac
PR: xmtp/xmtpd#712
File: pkg/merkle/proof.go:71-72
Timestamp: 2025-04-15T11:29:03.575Z
Learning: In Go, when a byte slice (`[]byte`) is nil, `len(slice)` returns 0. Therefore, checking `if len(slice) == 0` will catch both nil slices and empty slices (initialized but with no elements).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/merkle/tree.go (1)
297-309: Potential overflow inroundUpToPowerOf2remains unhandledThe function still does
return n + 1after the bit‑smearing step.
For inputs such as0xFFFF_FFFFthis wraps to0, breaking subsequent size calculations. Previous review threads already highlighted this; please address it or document why the current behaviour is acceptable.- return n + 1 + if n == ^uint32(0) { + // Overflow would wrap to zero – reject or cap explicitly. + return n // or: panic("overflow in roundUpToPowerOf2") + } + return n + 1
🧹 Nitpick comments (1)
pkg/merkle/tree.go (1)
14-23: Avoid repeated allocations inNode.Hash()
Node.Hash()defensively copies the underlying slice on every call.
While this guarantees immutability, it also allocatesO(n)memory for every hash access, which becomes expensive when walking large trees or generating proofs.If the tree is treated as immutable after construction (which appears to be the case), you can safely return the slice itself and document it as read‑only. A middle ground is to expose both a zero‑allocation accessor (
HashNoCopy) and keep the safe variant for external callers.-func (n *Node) Hash() []byte { - if n.hash == nil { - return nil - } - - cp := make([]byte, len(n.hash)) - copy(cp, n.hash) - return cp -} +// Hash returns the node hash. The slice **must not** be mutated. +func (n *Node) Hash() []byte { + return n.hash +}If total immutability is required, consider caching the copy once and re‑using it instead of re‑allocating each time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/merkle/hash.go(1 hunks)pkg/merkle/proof.go(1 hunks)pkg/merkle/proof_test.go(1 hunks)pkg/merkle/tree.go(1 hunks)pkg/merkle/tree_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/merkle/proof_test.go
- pkg/merkle/hash.go
- pkg/merkle/tree_test.go
- pkg/merkle/proof.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/merkle/tree.go (2)
pkg/merkle/hash.go (4)
Hash(18-22)HashEmptyLeaf(45-47)HashNode(24-33)HashLeaf(35-43)pkg/merkle/proof.go (2)
MultiProof(28-32)ErrGenerateProof(11-11)
⏰ 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 (1)
pkg/merkle/tree.go (1)
129-160: Proof sibling ordering might break verification – please double‑check
makeProofappends a sibling as soon as exactly one child is known.
The resulting order ofproofsdepends on the traversal sequence (idescending) rather than on the position of the sibling in the tree. Many multi‑proof verification algorithms expect a deterministic left‑to‑right (or bottom‑up) ordering of proof hashes; a mismatch will yield false‑negative verification.Please verify that
MultiProof.validate()(and the public verifier) consume theproofsslice according to this exact production order. If not, you may need to:
- Collect the sibling hashes first, then reverse or sort them to the expected order, or
- Switch to a breadth‑first / left‑to‑right traversal when building
proofs.Failing to align the orders will surface only under certain index combinations, so an explicit test is advisable.
|
|
||
| // CalculateBalancedNodesCount returns the number of nodes in a balanced tree. | ||
| // To calculate the number of nodes in a tree, we need to round up to the next power of 2. | ||
| // Returns an error if the element count is too large to be represented in a uint32. |
There was a problem hiding this comment.
these type casts here... It's such a shame that Go len() is int instead of uint. I understand the design decision, but it makes some things a bit more annoying to cast.
I would assume that all of our code will be running on 64-bit architecture and so int will be int64. roundUpToPowerOf2 can be easily adapted to accept and return uint64. Just add a few more shifts :) Or make it accept int and handle both architectures.
That might remove the need for the overflow check and a bunch of these casts.
There was a problem hiding this comment.
I would assume that all of our code will be running on 64-bit architecture and so int will be int64. roundUpToPowerOf2 can be easily adapted to accept and return uint64. Just add a few more shifts :) Or make it accept int and handle both architectures.
My assumption too, extracting this to an issue to work on it.
| return 0, nil | ||
| } | ||
|
|
||
| if count > int(1<<31) { |
There was a problem hiding this comment.
this does not work on 32bit architecture. It will overflow and hence go negative.
|
I've been trying to figure out where a malicious actor can inject payloads. It's hard to reason about because so many functions are exposed (public). I assume you have done that because I suggested to have the tests live in their own package and test the externally exposed functionality. Can you explain what the entry points are? The two MultiProof functions? We could always formalize that by using an |
Changes in d34a3eb
Macroscope summarized d34a3eb. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/merkle/tree.go (2)
177-179: 🛠️ Refactor suggestionAdd bounds check for Root() method.
The
Root()method directly accessesm.tree[1]without checking if the tree slice has sufficient length. This could lead to a panic if the tree is empty or malformed.func (m *MerkleTree) Root() []byte { + if len(m.tree) <= 1 { + return nil + } return m.tree[1].Hash() }
295-308:⚠️ Potential issueHandle potential overflow in roundUpToPowerOf2.
When
nis close to the maximum uint32 value (0xFFFFFFFF), adding 1 could cause an overflow, resulting in a value of 0. This would cause unexpected behavior when calculating the balanced node count.func roundUpToPowerOf2(n uint32) uint32 { if bits.OnesCount32(n) == 1 { return n } n |= n >> 1 n |= n >> 2 n |= n >> 4 n |= n >> 8 n |= n >> 16 + // Check for potential overflow + if n == math.MaxUint32 { + return n + } return n + 1 }Don't forget to add the import:
import "math"
🧹 Nitpick comments (4)
pkg/merkle/tree.go (4)
106-164: Simplify nested conditional logic in makeProof.The nested if statements in the makeProof function could be simplified for better readability while preserving the same logic.
// Only one of children is known, so we need the sibling as a proof. if left != right { - if right { - proofs = append(proofs, m.tree[leftChildIdx]) - } else { - proofs = append(proofs, m.tree[rightChildIdx]) - } + siblingIdx := rightChildIdx + if right { + siblingIdx = leftChildIdx + } + proofs = append(proofs, m.tree[siblingIdx]) }
280-293: Error message improvement in CalculateBalancedNodesCount.The error messages could be more informative to help diagnose issues. Consider adding details about the actual input value that caused the error.
func CalculateBalancedNodesCount(count int) (int, error) { if count <= 0 { - return 0, fmt.Errorf("count must be greater than 0") + return 0, fmt.Errorf("count must be greater than 0, got %d", count) } if count > int(1<<31) { - return 0, fmt.Errorf("count must be less than or equal to 2^31") + return 0, fmt.Errorf("count must be less than or equal to 2^31, got %d", count) } return int(roundUpToPowerOf2(uint32(count))), nil }
69-104: Consider adding validation for negative indices in GenerateMultiProofWithIndices.While the
makeProoffunction validates indices, adding an early check for negative values in the public API methods would provide clearer error messages.func (m *MerkleTree) GenerateMultiProofWithIndices(indices []int) (*MultiProof, error) { + // Early validation for negative indices + for _, idx := range indices { + if idx < 0 { + return nil, fmt.Errorf("negative index %d is invalid", idx) + } + } + sortedIndices := make([]int, len(indices)) copy(sortedIndices, indices) sort.Ints(sortedIndices)
209-219: Consider using constants for clarity in makeTree.The tree building logic relies on understanding the 1-indexed representation. Using named constants could make the code more readable.
+ // Constants for clarity + const ROOT_INDEX = 1 lowerBound := balancedLeafCount upperBound := balancedLeafCount + len(nodes) - 1 // Copy leaves into the tree, starting at index N. for i := 0; i < len(nodes); i++ { tree[lowerBound+i] = nodes[i] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/merkle/tree.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/merkle/tree.go (2)
pkg/merkle/hash.go (4)
Hash(18-22)HashEmptyLeaf(45-47)HashNode(24-33)HashLeaf(35-43)pkg/merkle/proof.go (2)
MultiProof(28-32)ErrGenerateProof(11-11)
⏰ 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: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Upgrade Tests
- GitHub Check: Code Review
🔇 Additional comments (4)
pkg/merkle/tree.go (4)
14-23: Defensive copy in Hash() method is a good practice.Creating a defensive copy of the hash ensures that external code cannot modify the internal state of the tree nodes. This immutability helps prevent subtle bugs that could arise from accidental modifications.
27-38: Well-documented data structure design.The documentation clearly explains the 1-indexed array structure of the Merkle tree, which makes the code easier to understand. This implementation choice follows a common approach for binary trees where parent-child relationships can be calculated using simple arithmetic.
45-67: Proper error handling in NewMerkleTree.The constructor properly propagates errors from helper functions, ensuring that any issues during tree construction are reported accurately to the caller.
220-240:Details
❌ Incorrect review comment
Check for nil hash before dereferencing in makeTree.
When building the tree, the code doesn't check if
Hash()returns nil before using it inHashNode(). This could lead to a panic if one of the child nodes has a nil hash.tree[i] = Node{ - hash: HashNode(tree[leftChildIndex].Hash(), tree[rightChildIndex].Hash()), + hash: HashNode( + getNonNilHash(tree[leftChildIndex].Hash()), + getNonNilHash(tree[rightChildIndex].Hash()), + ), } // Add this helper function: // func getNonNilHash(hash []byte) []byte { // if hash == nil { // return []byte{} // } // return hash // }Let's verify if a nil hash can actually occur in practice:
🏁 Script executed:
#!/bin/bash # Search for any places where a Node is created with a nil hash rg -A 2 -B 2 "Node\s*{(\s*hash:\s*nil|\s*}\s*)" pkg/merkle/Length of output: 184
Let’s inspect the
Nodetype, itsHash()method, theHashNodefunction, and the surroundingmakeTreelogic to confirm whether a nil‐slice can flow through:
🏁 Script executed:
#!/bin/bash echo "=== Node struct definition ===" rg -n 'type Node' -n pkg/merkle echo echo "=== HashNode function ===" rg -n 'func HashNode' -n pkg/merkle echo echo "=== Hash() method ===" rg -n 'func .*Hash()' -n pkg/merkle echo echo "=== makeTree snippet (lines 200–260) ===" sed -n '200,260p' pkg/merkle/tree.goLength of output: 2157
Let’s confirm the implementations of
HashNode,HashLeaf, andHashEmptyLeafto see if they ever returnnil:
🏁 Script executed:
#!/bin/bash echo "=== pkg/merkle/hash.go ===" sed -n '1,200p' pkg/merkle/hash.goLength of output: 1051
Drop the nil‐check guard: hashes are always non‐nil
AllNode.hashfields are seeded viaHashLeaf,HashEmptyLeaf, orHashNode—and each of those callsHash(buffer), which always returns a non‑nil[]byte. Additionally,func (n *Node) Hash()is called on a value receiver (nevernil). The proposedgetNonNilHashhelper isn’t needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/merkle/tree.go (2)
295-308: Consider adding a const for the maximum accepted value.While the overflow check in CalculateBalancedNodesCount prevents integer overflow by limiting the count to 2^31, consider adding a named constant for this maximum value to make the code more readable and maintainable.
+ const MaxNodeCount = 1 << 31 // Maximum node count to prevent overflow func CalculateBalancedNodesCount(count int) (int, error) { if count <= 0 { return 0, fmt.Errorf("count must be greater than 0") } - if count > int(1<<31) { + if count > MaxNodeCount { return 0, fmt.Errorf("count must be less than or equal to 2^31") } return int(roundUpToPowerOf2(uint32(count))), nil }
310-318: Consider using more idiomatic Go function names.The functions
GetLeftChildandGetRightChildfollow traditional OOP naming conventions. In Go, the "Get" prefix is typically omitted. Consider renaming toLeftChildandRightChildto follow idiomatic Go naming conventions.- func GetLeftChild(index int) int { + func LeftChild(index int) int { return index << 1 // index * 2 } - func GetRightChild(index int) int { + func RightChild(index int) int { return (index << 1) + 1 // index * 2 + 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/merkle/tree.go(1 hunks)pkg/merkle/tree_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/merkle/tree_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/merkle/tree.go (2)
pkg/merkle/hash.go (4)
Hash(18-22)HashEmptyLeaf(45-47)HashNode(24-33)HashLeaf(35-43)pkg/merkle/proof.go (2)
MultiProof(28-32)ErrGenerateProof(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (8)
pkg/merkle/tree.go (8)
10-23: LGTM: Node struct with defensive Hash method.The Node struct definition is clean and the Hash() method correctly handles nil values and defensively returns a copy of the hash value to prevent accidental modifications of the internal state. This is a good practice for immutability.
27-38: LGTM: Well-documented MerkleTree struct with clear indexing explanation.The struct definition and comments clearly explain the 1-indexed nature of the tree array representation. This explicit documentation of the indexing scheme is essential for understanding the implementation.
45-67: LGTM: Clean constructor with proper error handling.The NewMerkleTree function follows a clear step-by-step construction process with appropriate error handling at each stage. The delegation to helper functions keeps the constructor clean and focused.
69-86: LGTM: Sequential multi-proof generation with proper validation.The GenerateMultiProofSequential method properly creates and validates indices before generating the proof. The final validation step ensures the generated proof is correct.
88-104: LGTM: Multi-proof generation for arbitrary indices with sorting.The GenerateMultiProofWithIndices method correctly sorts the indices to ensure consistent proof generation. This is important for the internal algorithm and allows for verification using the canonical ordering.
106-164: LGTM: Well-implemented proof generation algorithm.The makeProof method correctly validates inputs, handles the single-element tree edge case, and implements the proof generation algorithm. The approach of marking nodes as known and walking up the tree is efficient and matches the standard Merkle proof generation approach.
186-244: LGTM: Thorough tree construction implementation.The makeTree function correctly implements the bottom-up tree construction algorithm with proper error handling and empty tree checks. The algorithm efficiently builds the tree by calculating hashes from the leaf level up.
176-179:Details
❌ Incorrect review comment
Ensure Root() works with empty trees.
The Root() method directly accesses index 1 without checking if the tree array has at least 2 elements. While the constructor ensures non-empty trees, defensive programming would suggest adding a bounds check.
🏁 Script executed:
#!/bin/bash # Check if there are any functions that call Root() that might not verify the tree is non-empty first rg -A 5 "Root\(\)" pkg/merkleLength of output: 4860
No change needed:
Root()is safe for empty treesThe MerkleTree constructor uses
CalculateBalancedNodesCount, which guarantees a minimum leaf count of 1 (som.treeis always at least length 2). Existing tests—especiallyTestTreeWithEmptyElements—already exerciseRoot()on an empty tree and assert it’s non‑nil. A bounds check isn’t required here.Likely an incorrect or invalid review comment.
This package aims to be a generic Merkle Tree implementation, in our case it means some limits should be enforced in the xmtpd main logic consuming this pkg. The entrypoints are:
Takes an arbitrary amount of leaves, the package only forces a positive amount of them. Ideally the consumer also forces a maximum amount of leaves, depending on the use case. It would make sense for our payer service constructing the tree to have a max size. A
A
MerkleTree methods:
This are only used to generate a Utility functions - I don't see any injection possibilities here:
Interesting action items:
@mkysel @neekolas any other suggestion here to harden the package? For the sake of legibility I'll work on those in a separate PR. :) |
Implement Merkle tree data structure with proof generation and verification in
pkg/merkleMerkleTreestruct and tree construction logicMultiProofstruct📍Where to Start
Start with the
NewMerkleTreefunction in tree.go which implements the core tree construction logic and serves as the entry point for the package.Macroscope summarized 5eb4a35.
Summary by CodeRabbit