verifysubtreeinclusion - #3
Conversation
phbnf
commented
Jun 3, 2026
phbnf
commented
- verifysubtreeinclusion implementation
- verifysubtreeinclusion test copy + implementation
- size --> start, end in tests
- duplicate tests to -shifted.json
- shift everything by bit_ceil
- mv -shifted.json -subtree.json
- s/shifted/subtree
- add error 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 introduces a new VerifySubtreeInclusion function in proof/verify.go to verify subtree inclusion proofs, along with comprehensive test coverage and test data. Feedback on the changes highlights a misleading error message in the subtree validation check, which also poses a potential risk of panic or undefined behavior on extremely large inputs. A simplified error message was suggested to resolve this safely.
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 !isSubtreeValid(start, end) { | ||
| return fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, end-start) | ||
|
|
||
| } |
There was a problem hiding this comment.
The error message is misleading because it prints end-start as the value of bit_ceil(end - start), which is not ceiled. Additionally, directly calling bitCeil(end - start) on extremely large inputs (where end - start > 1<<63, as tested in invalid-large-subtree.json) would violate bitCeil's precondition and could cause undefined behavior or panic.
To fix this safely and cleanly, and to remove the extra blank line on line 70, we can simplify the error message to omit the ceiled value.
if !isSubtreeValid(start, end) {
return fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start)
}
There was a problem hiding this comment.
Code Review
This pull request introduces the VerifySubtreeInclusion function to verify subtree inclusion proofs, along with a comprehensive test suite and extensive JSON-based test cases. Feedback on the implementation highlights an error message formatting issue where end-start is printed instead of the actual bitCeil(end-start) value, as well as an unnecessary blank line in the validation block.
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 !isSubtreeValid(start, end) { | ||
| return fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, end-start) | ||
|
|
||
| } |
There was a problem hiding this comment.
The error message incorrectly prints end-start as the value of bit_ceil(end - start). Since bitCeil is available in the package, we should use bitCeil(end-start) to show the correct value in the error message. Additionally, there is an unnecessary blank line inside the if block.
if !isSubtreeValid(start, end) {
return fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bitCeil(end-start))
}
bca3439 to
7cd81e3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree inclusion proof verification, adding the VerifySubtreeInclusion function in proof/verify.go along with corresponding tests and a comprehensive set of JSON test cases. Feedback on the changes includes removing an unnecessary blank line in proof/verify.go, correcting a struct comment in proof/verify_test.go to refer to subtreeInclusionProbe instead of inclusionProbe, and updating a test error message to specify 'subtree inclusion probes' for better clarity.
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 !isSubtreeValid(start, end) { | ||
| return fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start) | ||
|
|
||
| } |
There was a problem hiding this comment.
There is an unnecessary blank line at the end of the if block. Running gofmt on the file will automatically remove this blank line and ensure compliance with standard Go formatting guidelines.
if !isSubtreeValid(start, end) {
return fmt.Errorf("start %d not a multiple of bit_ceil(end - start)", start)
}| WantError bool `json:"wantErr"` | ||
| } | ||
|
|
||
| // inclusionProbe is a parameter set for inclusion proof verification. |
There was a problem hiding this comment.
The comment refers to inclusionProbe, but the struct being defined is subtreeInclusionProbe. Updating the comment to match the struct name improves code readability and maintainability.
| // inclusionProbe is a parameter set for inclusion proof verification. | |
| // subtreeInclusionProbe is a parameter set for subtree inclusion proof verification. |
|
|
||
| return nil | ||
| }); err != nil { | ||
| t.Errorf("failed to read inclusion probes: %s", err) |
There was a problem hiding this comment.
The error message mentions inclusion probes, but this test is specifically for subtree inclusion probes. Updating the error message to be more specific helps with debugging test failures.
| t.Errorf("failed to read inclusion probes: %s", err) | |
| t.Errorf("failed to read subtree inclusion probes: %s", err) |
ca746d9 to
f8931b7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the VerifySubtreeInclusion function to verify subtree inclusion proofs, along with corresponding unit tests and a comprehensive set of JSON test vectors. It also refactors an error message in proof/proof.go. The review feedback recommends adding a defensive check to ensure the hasher parameter is not nil to prevent potential nil pointer dereferences.
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 VerifySubtreeInclusion(hasher merkle.LogHasher, index, start, end uint64, leafHash []byte, proof [][]byte, root []byte) error { | ||
| if start >= end { |
There was a problem hiding this comment.
The hasher parameter is not checked for nil. If a caller passes nil for hasher, the function will panic with a nil pointer dereference when hasher.Size() is called inside RootFromInclusionProof.
Consider adding a defensive check at the beginning of the function to return an error if hasher is nil.
| func VerifySubtreeInclusion(hasher merkle.LogHasher, index, start, end uint64, leafHash []byte, proof [][]byte, root []byte) error { | |
| if start >= end { | |
| func VerifySubtreeInclusion(hasher merkle.LogHasher, index, start, end uint64, leafHash []byte, proof [][]byte, root []byte) error { | |
| if hasher == nil { | |
| return errors.New("hasher cannot be nil") | |
| } | |
| if start >= end { |
f8931b7 to
e6a72d5
Compare
85206ca to
f6aeee6
Compare
1 participant
