subtreeconsistency - #2
Conversation
phbnf
commented
May 29, 2026
phbnf
commented
- SubtreeInclusion
- SubtreeInclusionProof Tests
- SubtreeConsistency
- spcial case when a subtree len is a pow of 2
- fold use SubtreeConsistency for Consistency
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces SubtreeInclusion and SubtreeConsistency functions to proof/proof.go to support fetching and constructing inclusion and consistency proofs for subtrees of a log Merkle tree, along with comprehensive unit tests in proof/proof_test.go. The reviewer identified a critical issue in both new functions where a large difference between end and start (greater than 1 << 63) can cause a shift of 64 or more, leading to undefined behavior and a potential division-by-zero panic during the modulo operation. Code suggestions were provided to safely handle this edge case.
| if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | ||
| } |
There was a problem hiding this comment.
If 'end - start' is large enough (specifically, 'end - start > 1 << 63'), then 'end - start - 1' will have its 64th bit set, meaning 'bits.Len64(end - start - 1)' returns '64'. Shifting a 64-bit integer by 64 ('uint64(1) << 64') is undefined behavior in Go and can result in '0' (or other platform-dependent values), which then causes a division-by-zero panic during the modulo operation 'start % bc'.
To prevent this, we should check if the shift is 64 or greater. If it is, the only valid 'start' value that is a multiple of '1 << 64' is '0'.
| if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } | |
| if shift := bits.Len64(end - start - 1); shift >= 64 { | |
| if start != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start)", start) | |
| } | |
| } else if bc := uint64(1) << shift; start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } |
There was a problem hiding this comment.
But both end and starts are uint64, and end > start. So it's not possible.
| if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | ||
| } |
There was a problem hiding this comment.
Similar to 'SubtreeInclusion', if 'end - start > 1 << 63', 'bits.Len64(end - start - 1)' returns '64'. Shifting a 64-bit integer by 64 is undefined behavior in Go and can result in '0', leading to a division-by-zero panic during 'start % bc'.
We should safely handle the case where the shift is 64 or greater.
| if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } | |
| if shift := bits.Len64(end - start - 1); shift >= 64 { | |
| if start != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start)", start) | |
| } | |
| } else if bc := uint64(1) << shift; start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } |
There was a problem hiding this comment.
Code Review
This pull request introduces 'SubtreeInclusion' and 'SubtreeConsistency' functions to the 'proof' package, allowing inclusion and consistency proofs to be generated for subtrees. It also refactors the existing 'Consistency' function to leverage 'SubtreeConsistency' and adds comprehensive unit tests covering these new functions and their edge cases. The review feedback highlights a potential integer division by zero panic in both 'SubtreeInclusion' and 'SubtreeConsistency' when 'end - start' is greater than '1 << 63' (causing a shift overflow to 0), and provides code suggestions to safely handle this case.
| if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | ||
| } |
There was a problem hiding this comment.
If end - start is greater than 1 << 63, then end - start - 1 is at least 1 << 63, which means bits.Len64(end - start - 1) will return 64. Shifting uint64(1) << 64 results in 0 in Go, which then causes a runtime panic (integer divide by zero) when evaluating start % bc.
To prevent this panic, we should explicitly handle the case where shift == 64. Since start is a uint64, the only multiple of 1 << 64 is 0. Thus, if shift == 64, start must be 0.
| if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } | |
| shift := bits.Len64(end - start - 1) | |
| if shift == 64 { | |
| if start != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start)", start) | |
| } | |
| } else if bc := uint64(1) << shift; start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } |
| if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | ||
| } |
There was a problem hiding this comment.
If end - start is greater than 1 << 63, then end - start - 1 is at least 1 << 63, which means bits.Len64(end - start - 1) will return 64. Shifting uint64(1) << 64 results in 0 in Go, which then causes a runtime panic (integer divide by zero) when evaluating start % bc.
To prevent this panic, we should explicitly handle the case where shift == 64. Since start is a uint64, the only multiple of 1 << 64 is 0. Thus, if shift == 64, start must be 0.
| if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } | |
| shift := bits.Len64(end - start - 1) | |
| if shift == 64 { | |
| if start != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start)", start) | |
| } | |
| } else if bc := uint64(1) << shift; start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d is not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } |
473dce7 to
f602ae6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces SubtreeInclusion and SubtreeConsistency functions to support inclusion and consistency proofs for log Merkle subtrees, along with extensive unit tests. The review feedback suggests refactoring duplicated subtree validation logic into a shared helper function, fixing a minor typo in a comment, and expanding test coverage in TestSubtreeConsistencySucceedsUpToTreeSize by adjusting a loop boundary to include the end == size case.
| if start >= end { | ||
| return Nodes{}, fmt.Errorf("start %d larger or equal than end %d", start, end) | ||
| } | ||
| if index < start || index >= end { | ||
| return Nodes{}, fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | ||
| } | ||
| if shift := bits.Len64(end - start - 1); shift >= 64 { | ||
| if start != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start) | ||
| } | ||
| } else if bc := uint64(1) << shift; start%bc != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc) | ||
| } |
There was a problem hiding this comment.
The subtree validation logic is duplicated between SubtreeInclusion and SubtreeConsistency. Extracting this validation into a helper function improves maintainability and readability.
| if start >= end { | |
| return Nodes{}, fmt.Errorf("start %d larger or equal than end %d", start, end) | |
| } | |
| if index < start || index >= end { | |
| return Nodes{}, fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | |
| } | |
| if shift := bits.Len64(end - start - 1); shift >= 64 { | |
| if start != 0 { | |
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start) | |
| } | |
| } else if bc := uint64(1) << shift; start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } | |
| if err := validateSubtreeRange(start, end); err != nil { | |
| return Nodes{}, err | |
| } | |
| if index < start || index >= end { | |
| return Nodes{}, fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | |
| } |
| return p.skipFirst(), nil | ||
| } |
There was a problem hiding this comment.
Define the validateSubtreeRange helper function here to centralize the subtree validation logic.
return p.skipFirst(), nil
}
func validateSubtreeRange(start, end uint64) error {
if start >= end {
return fmt.Errorf("start %d larger or equal than end %d", start, end)
}
if shift := bits.Len64(end - start - 1); shift >= 64 {
if start != 0 {
return fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start)
}
} else if bc := uint64(1) << shift; start%bc != 0 {
return fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc)
}
return nil
}| if start >= end { | ||
| return Nodes{}, fmt.Errorf("start %d larger or equal than end %d", start, end) | ||
| } | ||
| if end > size { | ||
| return Nodes{}, fmt.Errorf("subtree end %d strictly larger than tree size %d", end, size) | ||
| } | ||
| if shift := bits.Len64(end - start - 1); shift >= 64 { | ||
| if start != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start) | ||
| } | ||
| } else if bc := uint64(1) << shift; start%bc != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc) | ||
| } |
There was a problem hiding this comment.
Use the validateSubtreeRange helper function here to avoid code duplication.
| if start >= end { | |
| return Nodes{}, fmt.Errorf("start %d larger or equal than end %d", start, end) | |
| } | |
| if end > size { | |
| return Nodes{}, fmt.Errorf("subtree end %d strictly larger than tree size %d", end, size) | |
| } | |
| if shift := bits.Len64(end - start - 1); shift >= 64 { | |
| if start != 0 { | |
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start) | |
| } | |
| } else if bc := uint64(1) << shift; start%bc != 0 { | |
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc) | |
| } | |
| if err := validateSubtreeRange(start, end); err != nil { | |
| return Nodes{}, err | |
| } | |
| if end > size { | |
| return Nodes{}, fmt.Errorf("subtree end %d strictly larger than tree size %d", end, size) | |
| } |
|
|
||
| // Handle the case when size1 is a power of 2. | ||
| if index == 0 { | ||
| // Handle the case when the subtree size if a power of 2. |
There was a problem hiding this comment.
Fix typo in comment: 'if' -> 'is'.
| // Handle the case when the subtree size if a power of 2. | |
| // Handle the case when the subtree size is a power of 2. |
| func TestSubtreeConsistencySucceedsUpToTreeSize(t *testing.T) { | ||
| const maxSize = uint64(100) | ||
| for sbe := uint64(1); sbe < maxSize; sbe++ { | ||
| for s := sbe + 1; s <= maxSize; s++ { |
There was a problem hiding this comment.
By starting s at sbe instead of sbe + 1, we can also test the case where end == size (i.e., sbe == s) for subtrees with start > 0. This significantly improves test coverage for subtree consistency proofs.
| for s := sbe + 1; s <= maxSize; s++ { | |
| for s := sbe; s <= maxSize; s++ { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces SubtreeInclusion and SubtreeConsistency functions to support generating inclusion and consistency proofs for log Merkle subtrees, along with comprehensive unit tests. Two issues were identified in the review: first, an incorrect loop condition in TestSubtreeConsistencySucceedsUpToTreeSize causes the test cases to be silently skipped; second, SubtreeConsistency does not correctly handle empty subtrees when start > 0 and start == end due to an incorrect boundary check.
| for sbe := uint64(1); sbe < maxSize; sbe++ { | ||
| for s := sbe + 1; s <= maxSize; s++ { | ||
| for sbs := uint64(0); sbs < sbe; sbs++ { | ||
| if bc := uint64(1) << bits.Len64(sbe-sbs-1); bc != 0 { |
There was a problem hiding this comment.
The loop condition bc != 0 is always true because bc is a power of two (and cannot overflow to 0 given the maxSize of 100). This causes the loop to always continue and skip all test cases, meaning SubtreeConsistency is never actually tested. It should be sbs%bc != 0 instead, matching the logic in TestInclusionSubtreeSucceedsUpToTreeSize.
| if bc := uint64(1) << bits.Len64(sbe-sbs-1); bc != 0 { | |
| if bc := uint64(1) << bits.Len64(sbe-sbs-1); sbs%bc != 0 { |
| return Nodes{}, fmt.Errorf("subtree end %d strictly greater than tree size %d", end, size) | ||
| } | ||
| if size1 == size2 || size1 == 0 { | ||
| if (end == 0) || (start == 0 && end == size) { |
There was a problem hiding this comment.
When start > 0 and start == end, the subtree is empty, so the consistency proof should be empty. However, because end == 0 is checked instead of start == end, it proceeds to the alignment check and incorrectly returns an error. Changing the check to start == end correctly handles empty subtrees for any starting index.
| if (end == 0) || (start == 0 && end == size) { | |
| if (start == end) || (start == 0 && end == size) { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree-based inclusion and consistency proof generation by adding the SubtreeInclusion and SubtreeConsistency functions to the proof package, along with comprehensive unit tests. The feedback suggests extracting the duplicated subtree alignment validation logic shared between these two new functions into a single helper function to improve maintainability.
| if shift := bits.Len64(end - start - 1); shift >= 64 { | ||
| if start != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start) | ||
| } | ||
| } else if bc := uint64(1) << shift; start%bc != 0 { | ||
| return Nodes{}, fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc) | ||
| } |
There was a problem hiding this comment.
The subtree alignment validation logic is duplicated between SubtreeInclusion (lines 68-74) and SubtreeConsistency (lines 111-117).
To improve maintainability and adhere to DRY (Don't Repeat Yourself) principles, consider extracting this logic into a private helper function, such as:
func checkSubtreeAlignment(start, end uint64) error {
shift := bits.Len64(end - start - 1)
if shift >= 64 {
if start != 0 {
return fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start)
}
} else if bc := uint64(1) << shift; start%bc != 0 {
return fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc)
}
return nil
}Then you can simply call it in both functions:
if err := checkSubtreeAlignment(start, end); err != nil {
return Nodes{}, err
} if err := checkSubtreeAlignment(start, end); err != nil {
return Nodes{}, err
}
7a4802a to
89a59d6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeConsistency function to calculate consistency proofs for Merkle subtrees covering a range [start, end) within a larger tree, and refactors the existing Consistency function to delegate to it. It also adds comprehensive unit tests for this new functionality. The reviewer identified a regression in the refactored Consistency function, where calling it with size1 == 0 will now incorrectly return an error instead of an empty proof, and provided a code suggestion to handle this case and restore the original error reporting.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func Consistency(size1, size2 uint64) (Nodes, error) { | ||
| if size1 > size2 { | ||
| return Nodes{}, fmt.Errorf("tree size %d > %d", size1, size2) | ||
| return SubtreeConsistency(0, size1, size2) | ||
| } |
There was a problem hiding this comment.
Delegating directly to SubtreeConsistency(0, size1, size2) introduces a regression when size1 == 0. The Consistency function is documented to require 0 <= size1 <= size2, meaning size1 can be 0. However, SubtreeConsistency requires start < end (validated via isSubtreeValid), which fails when size1 == 0 (since end == 0). This causes Consistency(0, size2) to return an error instead of an empty proof.
Additionally, the original error message for size1 > size2 is lost. We should explicitly handle size1 > size2 and size1 == 0 in Consistency before delegating.
| func Consistency(size1, size2 uint64) (Nodes, error) { | |
| if size1 > size2 { | |
| return Nodes{}, fmt.Errorf("tree size %d > %d", size1, size2) | |
| return SubtreeConsistency(0, size1, size2) | |
| } | |
| func Consistency(size1, size2 uint64) (Nodes, error) { | |
| if size1 > size2 { | |
| return Nodes{}, fmt.Errorf("tree size %d > %d", size1, size2) | |
| } | |
| if size1 == 0 { | |
| return Nodes{IDs: []compact.NodeID{}}, nil | |
| } | |
| return SubtreeConsistency(0, size1, size2) | |
| } |
89a59d6 to
536e59c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces SubtreeConsistency, a new function to calculate consistency proofs for Merkle subtrees, and refactors the existing Consistency function to utilize it. It also includes comprehensive unit tests for the new functionality and updates verification logic and test cases. The review feedback highlights a regression where calling Consistency with a size1 of 0 now returns a subtree validation error instead of an empty proof, and suggests explicitly handling this case to provide a clearer error message.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func Consistency(size1, size2 uint64) (Nodes, error) { | ||
| if size1 > size2 { | ||
| return Nodes{}, fmt.Errorf("tree size %d > %d", size1, size2) | ||
| return SubtreeConsistency(0, size1, size2) | ||
| } |
There was a problem hiding this comment.
Calling Consistency(0, size2) now returns an error because SubtreeConsistency(0, 0, size2) fails the isSubtreeValid(0, 0) check (returning start 0 must be strictly less than end 0). In the previous implementation, Consistency(0, size2) returned an empty proof without an error.
If returning an error for size1 == 0 is the intended behavior (since consistency proofs from an empty tree are indeed meaningless), we should handle this case explicitly in Consistency to return a clear, user-friendly error message rather than exposing internal subtree validation details like start 0 must be strictly less than end 0.
| func Consistency(size1, size2 uint64) (Nodes, error) { | |
| if size1 > size2 { | |
| return Nodes{}, fmt.Errorf("tree size %d > %d", size1, size2) | |
| return SubtreeConsistency(0, size1, size2) | |
| } | |
| func Consistency(size1, size2 uint64) (Nodes, error) { | |
| if size1 == 0 { | |
| return Nodes{}, fmt.Errorf("size1 must be greater than 0") | |
| } | |
| return SubtreeConsistency(0, size1, size2) | |
| } |
536e59c to
df8e3fe
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new SubtreeConsistency function in the proof package to compute consistency proofs for arbitrary Merkle subtrees. It also refactors the existing Consistency function to require a non-zero starting size (size1 > 0). Corresponding unit tests, fuzz tests, and verification logic have been updated to handle and validate these changes, specifically ensuring that consistency proofs starting from size 0 are treated as invalid. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
df8e3fe to
5f4c675
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeConsistency function to calculate consistency proofs for Merkle subtrees covering [start, end). The existing Consistency function is refactored to utilize this new helper and now requires size1 > 0. Related tests, fuzzers, and verification logic in verify.go have been updated to accommodate these changes and correctly handle or reject zero-sized inputs. As there are no review comments provided, we have no feedback to offer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
46776ea to
51ac723
Compare
66b4046 to
6a38871
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeConsistency function in proof/proof.go to compute consistency proofs between a Merkle subtree and a larger Merkle tree, refactoring the existing Consistency function to use this new helper. It also adds comprehensive unit tests in proof/proof_test.go, including TestSubtreeConsistency and TestSubtreeConsistencySucceedsUpToTreeSize, to verify the implementation. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeConsistency function and its helper subtreeConsistency in proof/proof.go to calculate consistency proofs for arbitrary Merkle subtrees. The existing Consistency function is refactored to delegate to this new implementation. Additionally, comprehensive unit tests, including TestSubtreeConsistency and TestSubtreeConsistencySucceedsUpToTreeSize, are added in proof/proof_test.go to validate the correctness of the subtree consistency logic. I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
6a38871 to
c0d56c7
Compare
1 participant
