CalculateBalanceNodesCount to handle int #733
Conversation
WalkthroughThe changes update the internal handling of node count parameters in the Merkle tree implementation from unsigned 32-bit integers ( Changes
Assessment against linked issues
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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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 (
|
Update
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/merkle/tree.go (1)
288-289: Consider revising error message grammar.The error message uses awkward phrasing with "less or equal than". While functionally correct, it would be more natural to say "less than or equal to" which is standard English.
- return 0, fmt.Errorf("count must be less or equal than max int32 (%d)", 1<<31-1) + return 0, fmt.Errorf("count must be less than or equal to max int32 (%d)", 1<<31-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)
⏰ 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: Code Review
- GitHub Check: Upgrade Tests
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
🔇 Additional comments (5)
pkg/merkle/tree.go (3)
282-283: Function documentation correctly updated.The documentation now properly reflects that the function returns an error if the count is too large for an int32 representation, aligning with the change from uint32 to int.
292-292: Function call correctly updated to match new signature.The call to
roundUpToPowerOf2properly passes the int parameter, which aligns with the updated function signature.
296-297: Appropriate type conversion for bit counting.The function type has been changed from uint32 to int, and the bit counting logic has been properly updated to use
bits.OnesCountwith an explicit cast touint. This maintains the functionality while supporting the type change.pkg/merkle/tree_test.go (2)
440-440: Test assertion correctly updated to match new error message.The assertion has been updated to check for the new error message used in the implementation, maintaining test coverage for the maximum size validation.
437-439:Details
✅ Verification successful
Verify handling of large values.
The test properly verifies the function rejects values larger than int32 maximum. This is critical for the PR's goal of ensuring compatibility with 32-bit systems.
🏁 Script executed:
#!/bin/bash # Check if there are other test cases that might need updating due to the uint32 to int type change echo "Checking for potentially affected tests:" rg --no-heading "uint32" pkg/merkle/Length of output: 273
All large-value test cases are correctly handled
- Only two references to
uint32appear inpkg/merkle/tree_test.go(the comment and themassiveInputdeclaration on lines 437–439).- No other tests use
uint32, so no further updates are required.- This test ensures
CalculateBalancedNodesCountreturns an error for values exceeding 32‑bit limits.
fbac
force-pushed
the
04-22-merkle_fixes
branch
from
c237d3a to
1569bf7
Compare
fbac
force-pushed
the
04-22-merkle_fixes
branch
from
1569bf7 to
d88b88d
Compare
| // roundUpToPowerOf2 rounds up a number to the next power of 2. | ||
| func roundUpToPowerOf2(n uint32) uint32 { | ||
| if bits.OnesCount32(n) == 1 { | ||
| func roundUpToPowerOf2(n int) int { |
There was a problem hiding this comment.
@coderabbitai is this function correct for 64 bit numbers?
There was a problem hiding this comment.
No description provided.
There was a problem hiding this comment.
@mkysel Looking at the updated CalculateBalancedNodesCount function more carefully:
The function is correctly implemented but with a deliberate 32-bit integer limit, even on 64-bit systems. Here's why:
- On a 64-bit system, Go's
inttype can theoretically hold values up to 2^63-1 - However, the function explicitly checks
if count > 1<<31-1(max int32 value) - This intentionally limits input to 31-bit positive integers regardless of platform
To fully utilize 64-bit integers on 64-bit systems, you would need to:
- Replace the check with
if count > 1<<63-1(for max int64) - The updated
roundUpToPowerOf2function would already handle 64-bit numbers correctly with itsbits.OnesCount(uint(n))call
According to the PR description, this limitation is by design since "the code does not expect to handle very large trees."
🧠 Learnings used
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).
Closes #726
I'd assume our code will always run in 64 bit hardware, but it's non cost to also make this package compatible with 32 bits, as we don't expect such huge trees anyway.
Summary by CodeRabbit