subtreeinclusion - #1
Conversation
phbnf
commented
May 29, 2026
phbnf
commented
- SubtreeInclusion
- SubtreeInclusionProof Tests
|
/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 adds the SubtreeInclusion function to compute inclusion proofs for a leaf index in a log Merkle subtree, along with comprehensive unit tests. The review identified a critical bug in the bounds check of SubtreeInclusion that allows index == end, potentially causing integer overflow. Additionally, a bug was found in TestInclusionSubtreeSucceedsUpToTreeSize where the innermost loop never executes, and suggestions were provided to fix the loop logic and skip invalid test parameters.
| if index < start || index > end { | ||
| return Nodes{}, fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | ||
| } |
There was a problem hiding this comment.
The bounds check allows index == end because index > end is used instead of index >= end. If index == end, it will bypass this check and call nodes(end-start, 0, end-start), which leads to index == size in nodes. This causes inner to be computed as -1, leading to integer overflow/underflow when converted to uint and causing undefined behavior or panics.
| if index < start || index > end { | |
| return Nodes{}, fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | |
| } | |
| if index < start || index >= end { | |
| return Nodes{}, fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | |
| } |
| for sbe := uint64(1); sbe <= maxSize; sbe++ { | ||
| for sbs := uint64(1); sbs < sbe; sbs++ { | ||
| for i := sbe; i < sbe; i++ { | ||
| if _, err := SubtreeInclusion(i, sbs, sbe); err != nil { | ||
| t.Errorf("SubtreeInclusion(ts:%d, i:%d) = %v", sbe, i, err) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two major issues with this test:
- The innermost loop
for i := sbe; i < sbe; i++never executes becauseiis initialized tosbeand the condition isi < sbe. - If the loop is fixed to start at
sbs, the test will fail for many combinations ofsbsandsbebecauseSubtreeInclusionrequiressbsto be a multiple ofbit_ceil(sbe - sbs). We should skip invalid combinations and also allowsbsto start at0to cover more cases.
| for sbe := uint64(1); sbe <= maxSize; sbe++ { | |
| for sbs := uint64(1); sbs < sbe; sbs++ { | |
| for i := sbe; i < sbe; i++ { | |
| if _, err := SubtreeInclusion(i, sbs, sbe); err != nil { | |
| t.Errorf("SubtreeInclusion(ts:%d, i:%d) = %v", sbe, i, err) | |
| } | |
| } | |
| } | |
| } | |
| for sbe := uint64(1); sbe <= maxSize; sbe++ { | |
| for sbs := uint64(0); sbs < sbe; sbs++ { | |
| bc := uint64(1) << bits.Len64(sbe-sbs-1) | |
| if sbs%bc != 0 { | |
| continue | |
| } | |
| for i := sbs; i < sbe; i++ { | |
| if _, err := SubtreeInclusion(i, sbs, sbe); err != nil { | |
| t.Errorf("SubtreeInclusion(i:%d, sbs:%d, sbe:%d) = %v", i, sbs, sbe, err) | |
| } | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeInclusion function to calculate inclusion proofs for a leaf index within a Merkle subtree, along with corresponding unit tests. The review identified two critical issues: an incorrect boundary check in SubtreeInclusion that allows index == end and can lead to undefined behavior or panics, and a loop initialization bug in TestInclusionSubtreeSucceedsUpToTreeSize that prevents the test from executing.
| if start >= end { | ||
| return Nodes{}, fmt.Errorf("start %d larger or equal than end %d", start, end) | ||
| } | ||
| if index < start || index > end { |
There was a problem hiding this comment.
The boundary check for index is incorrect. The comment states that 0 <= start <= index < end, meaning index must be strictly less than end. However, the check index > end allows index == end to pass.
If index == end, index - start equals end - start (the subtree size). Passing an index equal to the size to nodes() results in inner being -1, leading to undefined behavior, extremely large levels (uint(-1)), and an infinite loop or panic in nodes().
Change the condition to index >= end.
| if index < start || index > end { | |
| if index < start || index >= end { |
| const maxSize = uint64(555) | ||
| for sbe := uint64(1); sbe <= maxSize; sbe++ { | ||
| for sbs := uint64(1); sbs < sbe; sbs++ { | ||
| for i := sbe; i < sbe; i++ { |
There was a problem hiding this comment.
The loop variable i is initialized to sbe and the loop condition is i < sbe. This means the loop body never executes, and no subtree inclusion proofs are actually tested in this function.
It should be initialized to sbs to test all valid leaf indices within the subtree.
| for i := sbe; i < sbe; i++ { | |
| for i := sbs; i < sbe; i++ { |
54dfa13 to
baaac88
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeInclusion function in proof/proof.go to construct inclusion proofs for a leaf index within a log Merkle subtree, along with comprehensive unit tests in proof/proof_test.go. The reviewer provided valuable feedback on the tests, recommending a reduction of maxSize in TestInclusionSubtreeSucceedsUpToTreeSize from 555 to 128 to prevent slow test execution due to cubic complexity, and suggesting that the error message parameters be reordered to match the signature of SubtreeInclusion for clearer debugging.
| } | ||
|
|
||
| func TestInclusionSubtreeSucceedsUpToTreeSize(t *testing.T) { | ||
| const maxSize = uint64(555) |
There was a problem hiding this comment.
The loop in TestInclusionSubtreeSucceedsUpToTreeSize has cubic complexity maxSize. With maxSize = 555, this results in over 28 million iterations, which can significantly slow down the unit test execution. Reducing maxSize to 128 is sufficient to cover all tree shapes and alignments up to level 7, while reducing the iteration count to around 350,000 (an ~80x speedup).
| const maxSize = uint64(555) | |
| const maxSize = uint64(128) |
| for i := sbs; i < sbe; i++ { | ||
| _, err := SubtreeInclusion(i, sbs, sbe) | ||
| if !wantErr && err != nil { | ||
| t.Errorf("SubtreeInclusion(sbs:%d, sbe: %d, i:%d) = %v", sbs, sbe, i, err) | ||
| } | ||
| if wantErr && err == nil { | ||
| t.Errorf("SubtreeInclusion(sbs:%d, sbe: %d, i:%d) = %v", sbs, sbe, i, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The error messages in t.Errorf print the parameters in the order (sbs, sbe, i), but the actual signature of SubtreeInclusion is SubtreeInclusion(index, start, end). This mismatch can be highly confusing when debugging test failures. Updating the format string to match the parameter names and order of the function signature improves clarity.
| for i := sbs; i < sbe; i++ { | |
| _, err := SubtreeInclusion(i, sbs, sbe) | |
| if !wantErr && err != nil { | |
| t.Errorf("SubtreeInclusion(sbs:%d, sbe: %d, i:%d) = %v", sbs, sbe, i, err) | |
| } | |
| if wantErr && err == nil { | |
| t.Errorf("SubtreeInclusion(sbs:%d, sbe: %d, i:%d) = %v", sbs, sbe, i, err) | |
| } | |
| } | |
| for i := sbs; i < sbe; i++ { | |
| _, err := SubtreeInclusion(i, sbs, sbe) | |
| if !wantErr && err != nil { | |
| t.Errorf("SubtreeInclusion(index:%d, start:%d, end:%d) = %v", i, sbs, sbe, err) | |
| } | |
| if wantErr && err == nil { | |
| t.Errorf("SubtreeInclusion(index:%d, start:%d, end:%d) = %v", i, sbs, sbe, err) | |
| } | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeInclusion function to fetch and construct inclusion proofs for log Merkle subtrees, along with comprehensive unit tests covering various subtree configurations and edge cases. The feedback suggests improving the documentation of SubtreeInclusion to clarify the alignment requirements for the start parameter, correcting a grammatical error in an error message, and optimizing the function by calling skipFirst() earlier to avoid unnecessary index shifting on the discarded first node.
| // SubtreeInclusion returns the information on how to fetch and construct an inclusion | ||
| // proof for the given leaf index in a log Merkle subtree covering [start, end). | ||
| // It requires 0 <= start <= index < end. |
There was a problem hiding this comment.
The documentation for SubtreeInclusion does not mention the alignment requirement for start (i.e., that start must be a multiple of the smallest power of two greater than or equal to end - start). Since violating this requirement results in an error, it should be explicitly documented to help users of the API.
// SubtreeInclusion returns the information on how to fetch and construct an inclusion
// proof for the given leaf index in a log Merkle subtree covering [start, end).
// It requires 0 <= start <= index < end, and start must be a multiple of the smallest
// power of two greater than or equal to (end - start).| if start >= end { | ||
| return Nodes{}, fmt.Errorf("start %d larger or equal than end %d", start, end) | ||
| } |
There was a problem hiding this comment.
The error message uses the grammatically incorrect phrase 'larger or equal than'. It should be 'greater than or equal to' or 'larger than or equal to' for clarity and standard terminology.
| if start >= end { | |
| return Nodes{}, fmt.Errorf("start %d larger or equal than end %d", start, end) | |
| } | |
| if start >= end { | |
| return Nodes{}, fmt.Errorf("start %d greater than or equal to end %d", start, end) | |
| } |
| // Shift the subtree to the left, such that it starts at 0. | ||
| p := nodes(index-start, 0, end-start) | ||
|
|
||
| // Shift nodes back to the right, in line with the original subtree position. | ||
| for n := range p.IDs { | ||
| p.IDs[n].Index += start >> p.IDs[n].Level | ||
| } | ||
| p.ephem.Index += start >> p.ephem.Level | ||
|
|
||
| return p.skipFirst(), nil |
There was a problem hiding this comment.
We can call skipFirst() immediately after retrieving the nodes from nodes(...). This avoids performing the index shifting operation on the first node, which is immediately discarded by skipFirst() anyway.
| // Shift the subtree to the left, such that it starts at 0. | |
| p := nodes(index-start, 0, end-start) | |
| // Shift nodes back to the right, in line with the original subtree position. | |
| for n := range p.IDs { | |
| p.IDs[n].Index += start >> p.IDs[n].Level | |
| } | |
| p.ephem.Index += start >> p.ephem.Level | |
| return p.skipFirst(), nil | |
| // Shift the subtree to the left, such that it starts at 0. | |
| p := nodes(index-start, 0, end-start).skipFirst() | |
| // Shift nodes back to the right, in line with the original subtree position. | |
| for n := range p.IDs { | |
| p.IDs[n].Index += start >> p.IDs[n].Level | |
| } | |
| p.ephem.Index += start >> p.ephem.Level | |
| return p, nil |
6e018c6 to
143671d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeInclusion function to calculate inclusion proofs for a leaf index within a log Merkle subtree, along with comprehensive unit tests. Feedback recommends reducing the maxSize constant in TestInclusionSubtreeSucceedsUpToTreeSize from 555 to 128 to prevent excessive memory allocation and slow test execution caused by the cubic complexity of the nested loops.
| } | ||
|
|
||
| func TestInclusionSubtreeSucceedsUpToTreeSize(t *testing.T) { | ||
| const maxSize = uint64(555) |
There was a problem hiding this comment.
The nested loops in TestInclusionSubtreeSucceedsUpToTreeSize run in maxSize = 555, this results in exactly 28,645,600 iterations. Since each iteration performs slice allocations in SubtreeInclusion and nodes, this test will allocate gigabytes of memory and take several seconds to complete, significantly slowing down the test suite.\n\nReducing maxSize to 128 (which is a power of two and sufficient to cover all boundary conditions) reduces the total iterations to 182,304, making the test run in milliseconds while maintaining excellent coverage.
| const maxSize = uint64(555) | |
| const maxSize = uint64(128) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeInclusion function to calculate inclusion proofs for a leaf index within a log Merkle subtree, along with comprehensive unit tests. A critical issue was identified in the TestInclusionSubtreeSucceedsUpToTreeSize test, where an incorrect condition (bc != 0) causes the test loop to be silently skipped. Correcting this to sbs%bc != 0 is necessary to ensure the tests actually run.
| if bc := uint64(1) << bits.Len64(sbe-sbs-1); bc != 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
In TestInclusionSubtreeSucceedsUpToTreeSize, the condition bc != 0 is always true because bc is a power of two and sbe - sbs <= 555 (so the shift is at most 10). As a result, the continue statement is always executed, and the entire test loop is silently skipped without running any assertions.\n\nTo fix this and ensure the test actually runs, the condition should check whether sbs (the subtree start index) is a multiple of bc (the bit ceiling of the subtree length), i.e., sbs % bc != 0.
| if bc := uint64(1) << bits.Len64(sbe-sbs-1); bc != 0 { | |
| continue | |
| } | |
| if bc := uint64(1) << bits.Len64(sbe-sbs-1); sbs%bc != 0 { | |
| continue | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SubtreeInclusion function in proof/proof.go to fetch and construct inclusion proofs for a leaf index within a specific log Merkle subtree [start, end). It also adds corresponding unit tests in proof/proof_test.go to verify subtree inclusion, correctness across various tree sizes, and ephemeral node calculations. There are no review comments, and I have no feedback to provide.
99fdb32 to
7b03e29
Compare
1 participant
