better isSubtreeValid - #7
Conversation
|
/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 refactors subtree validation logic in the proof package, shifting validation checks into isSubtreeValid and optimizing the check using bitwise operations. It also updates the fuzz tests in the testonly package to use this validation. However, the changes introduce a compilation error because isSubtreeValid is unexported in the proof package but called as proof.IsSubtreeValid in the testonly package. The review comments correctly point out this issue and suggest exporting the function by renaming it to IsSubtreeValid and updating its references.
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 SubtreeInclusion(index, start, end uint64) (Nodes, error) { | ||
| if start >= end { | ||
| return Nodes{}, fmt.Errorf("start %d greater than or equal to end %d", start, end) | ||
| if err := isSubtreeValid(start, end); err != nil { |
There was a problem hiding this comment.
Rename isSubtreeValid to IsSubtreeValid to match the exported name, as it needs to be exported for use in the testonly package.
| if err := isSubtreeValid(start, end); err != nil { | |
| if err := IsSubtreeValid(start, end); err != nil { |
| // - all the subtree nodes | ||
| // - no extra node to the left of the subtree | ||
| // - potentially extra nodes to the right of the subtree | ||
| func isSubtreeValid(start, end uint64) error { |
There was a problem hiding this comment.
Rename isSubtreeValid to IsSubtreeValid to export it. This is required because the testonly package (which is in a different directory/package) attempts to call proof.IsSubtreeValid in testonly/tree_fuzz_test.go. Note that you will also need to update the reference in proof/verify.go (line 68), which is not currently part of this PR's diff.
| func isSubtreeValid(start, end uint64) error { | |
| func IsSubtreeValid(start, end uint64) error { |
| } | ||
| t.Logf("index=%d, start=%d, end=%d", index, start, end) | ||
| if start >= end { | ||
| if err := proof.IsSubtreeValid(start, end); err != nil { |
There was a problem hiding this comment.
This call to proof.IsSubtreeValid will fail to compile because isSubtreeValid is currently unexported in the proof package. Please export it by renaming it to IsSubtreeValid in proof/proof.go and updating its references.
There was a problem hiding this comment.
Code Review
This pull request refactors subtree validation logic in the proof package and integrates it into the fuzz tests. Specifically, it moves the start >= end check into isSubtreeValid, optimizes the power-of-two alignment check using bitwise operations, and replaces manual checks in tree_fuzz_test.go with the validation function. However, the reviewer identified a compilation error: the fuzz test attempts to call proof.IsSubtreeValid, but the function is currently unexported as isSubtreeValid in the proof package. The reviewer recommends exporting the function and updating its references to resolve this issue.
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.
| } | ||
| t.Logf("index=%d, start=%d, end=%d", index, start, end) | ||
| if start >= end { | ||
| if err := proof.IsSubtreeValid(start, end); err != nil { |
There was a problem hiding this comment.
The function proof.IsSubtreeValid is not exported in the proof package (it is defined as isSubtreeValid with a lowercase i). Since testonly is a separate package, this will cause a compilation error. To resolve this, isSubtreeValid should be exported as IsSubtreeValid in the proof package.
| // - all the subtree nodes | ||
| // - no extra node to the left of the subtree | ||
| // - potentially extra nodes to the right of the subtree | ||
| func isSubtreeValid(start, end uint64) error { |
There was a problem hiding this comment.
Rename isSubtreeValid to IsSubtreeValid to export it, allowing external packages like testonly to use it. Note that you will also need to update the reference in proof/verify.go (line 68) which is not part of this PR's diff.
| func isSubtreeValid(start, end uint64) error { | |
| func IsSubtreeValid(start, end uint64) error { |
| func SubtreeInclusion(index, start, end uint64) (Nodes, error) { | ||
| if start >= end { | ||
| return Nodes{}, fmt.Errorf("start %d greater than or equal to end %d", start, end) | ||
| if err := isSubtreeValid(start, end); err != nil { |
There was a problem hiding this comment.
Update the internal call to use the newly exported IsSubtreeValid function.
| if err := isSubtreeValid(start, end); err != nil { | |
| if err := IsSubtreeValid(start, end); err != nil { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request exports and refactors the subtree validation logic into IsSubtreeValid, optimizing the check with a bitwise operation and simplifying validation across the codebase, including tests and fuzzing. The review feedback suggests aligning the validation order in VerifySubtreeInclusion with SubtreeInclusion for consistency, removing a trailing space in an error message, and correcting a grammatical typo in the bitCeil documentation.
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.
| if index < start || index >= end { | ||
| return fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | ||
| } | ||
| if err := isSubtreeValid(start, end); err != nil { | ||
| if err := IsSubtreeValid(start, end); err != nil { | ||
| return fmt.Errorf("subtree invalid: %v", err) | ||
| } |
There was a problem hiding this comment.
To be consistent with SubtreeInclusion in proof.go, we should validate the subtree bounds using IsSubtreeValid before checking if the index is out of bounds.
Additionally, please note that the check if start >= end at lines 62-64 becomes redundant because IsSubtreeValid already performs this check, so it should be removed from the beginning of the function.
| if index < start || index >= end { | |
| return fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | |
| } | |
| if err := isSubtreeValid(start, end); err != nil { | |
| if err := IsSubtreeValid(start, end); err != nil { | |
| return fmt.Errorf("subtree invalid: %v", err) | |
| } | |
| if err := IsSubtreeValid(start, end); err != nil { | |
| return fmt.Errorf("subtree invalid: %v", err) | |
| } | |
| if index < start || index >= end { | |
| return fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end) | |
| } |
| if l > uint64(1)<<63 { | ||
| return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63. ", start, l) | ||
| } |
There was a problem hiding this comment.
There is a trailing space at the end of the error message string. It should be removed for clean formatting.
| if l > uint64(1)<<63 { | |
| return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63. ", start, l) | |
| } | |
| if l > uint64(1)<<63 { | |
| return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63.", start, l) | |
| } |
| } | ||
|
|
||
| // bitCeil returns the smallest power of 2 larger than n. | ||
| // bitCeil returns the smallest power of 2 larger or equal than n. |
There was a problem hiding this comment.
The phrase 'larger or equal than n' is grammatically incorrect. It should be 'greater than or equal to n' or 'larger than or equal to n'.
| // bitCeil returns the smallest power of 2 larger or equal than n. | |
| // bitCeil returns the smallest power of 2 greater than or equal to n. |
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors subtree validation by exporting the IsSubtreeValid function, consolidating boundary checks inside it, and optimizing the validation logic using bitwise operations. It also updates callers and tests to use this exported function. The review feedback suggests a minor improvement to conform to Go style guidelines by removing a trailing period from an 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.
|
|
||
| // special-case large subtree to avoid panic | ||
| if l > uint64(1)<<63 { | ||
| return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63", start, l) |
There was a problem hiding this comment.
According to the Go style guide, error strings should not end with punctuation or be capitalized, as they are typically wrapped or printed following other context. Consider removing the trailing period.
| return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63", start, l) | |
| return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63", start, l) |
There was a problem hiding this comment.
Code Review
This pull request refactors subtree validation by exporting isSubtreeValid to IsSubtreeValid and consolidating validation checks across SubtreeInclusion, VerifySubtreeInclusion, and fuzz tests. It also optimizes the validation logic by replacing a modulo operation with a bitwise AND check (start&(bc-1) != 0) and cleans up unused imports in the fuzz tests. There are no review comments, and the changes look solid, so I have no further 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.
a47d0fd to
d14cc61
Compare
1 participant
No description provided.