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) | |
| } |
phbnf
force-pushed
the
subtreeconsistency
branch
2 times, most recently
from
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
}
1 participant
