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 introduces subtree consistency proof generation and verification, including new test data generation in cmd/proofgen/main.go, a new verification test TestVerifySubtreeConsistencyProbes in proof/verify_test.go, and corresponding JSON test files. The review feedback highlights three key improvement opportunities: optimizing slice pre-allocation in invalidSubtreeConsistencyProof to prevent unnecessary array reallocation, utilizing the existing fileName helper in writeSubtreeConsistencyProbe to avoid shadowing and maintain consistency, and correcting a copy-paste error in the test failure 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.
| ret := make([]subtreeConsistencyProbe, len(cProbes)) | ||
| for i, p := range cProbes { | ||
| ret[i] = subtreeConsistencyProbe{ | ||
| Start: 0, | ||
| End: p.Size1, | ||
| Size: p.Size2, | ||
| Root1: p.Root1, | ||
| Root2: p.Root2, | ||
| Proof: p.Proof, | ||
| Desc: p.Desc, | ||
| WantError: p.WantError, | ||
| } | ||
| } |
There was a problem hiding this comment.
The slice ret is initialized with make([]subtreeConsistencyProbe, len(cProbes)), which pre-allocates elements with zero values. Then, the loop overwrites these elements, and a subsequent append is called to add one more element. Since the slice's length is equal to its capacity, this append triggers a reallocation of the backing array and copies all elements.
To avoid this unnecessary allocation and copy, we should pre-allocate the slice with a capacity of len(cProbes) + 1 and use append inside the loop.
| ret := make([]subtreeConsistencyProbe, len(cProbes)) | |
| for i, p := range cProbes { | |
| ret[i] = subtreeConsistencyProbe{ | |
| Start: 0, | |
| End: p.Size1, | |
| Size: p.Size2, | |
| Root1: p.Root1, | |
| Root2: p.Root2, | |
| Proof: p.Proof, | |
| Desc: p.Desc, | |
| WantError: p.WantError, | |
| } | |
| } | |
| ret := make([]subtreeConsistencyProbe, 0, len(cProbes)+1) | |
| for _, p := range cProbes { | |
| ret = append(ret, subtreeConsistencyProbe{ | |
| Start: 0, | |
| End: p.Size1, | |
| Size: p.Size2, | |
| Root1: p.Root1, | |
| Root2: p.Root2, | |
| Proof: p.Proof, | |
| Desc: p.Desc, | |
| WantError: p.WantError, | |
| }) | |
| } |
| fileName := strings.ReplaceAll(probe.Desc, " ", "-") + ".json" | ||
|
|
||
| probeJson, err := json.MarshalIndent(probe, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("marshaling probe: %s", err) | ||
| } | ||
|
|
||
| fileLocation := filepath.Join(dir, fileName) | ||
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | ||
| return fmt.Errorf("writing probe: %s: %s", fileName, err) | ||
| } |
There was a problem hiding this comment.
In writeSubtreeConsistencyProbe, the code manually constructs the filename using strings.ReplaceAll(probe.Desc, " ", "-") + ".json". This is inconsistent with other probe writers in this file (such as writeConsistencyProbe and writeInclusionProbe) which use the fileName helper function. Additionally, declaring a local variable named fileName shadows the package-level function fileName, which is a code smell.
Using the fileName helper function will ensure consistent naming conventions (e.g., removing parentheses) across all generated test files.
| fileName := strings.ReplaceAll(probe.Desc, " ", "-") + ".json" | |
| probeJson, err := json.MarshalIndent(probe, "", " ") | |
| if err != nil { | |
| return fmt.Errorf("marshaling probe: %s", err) | |
| } | |
| fileLocation := filepath.Join(dir, fileName) | |
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | |
| return fmt.Errorf("writing probe: %s: %s", fileName, err) | |
| } | |
| fn := fileName(probe.Desc) | |
| probeJson, err := json.MarshalIndent(probe, "", " ") | |
| if err != nil { | |
| return fmt.Errorf("marshaling probe: %s", err) | |
| } | |
| fileLocation := filepath.Join(dir, fn) | |
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | |
| return fmt.Errorf("writing probe: %s: %s", fn, err) | |
| } |
| } | ||
|
|
||
| if len(wrong) > 0 { | ||
| t.Errorf("errors verifying consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) |
There was a problem hiding this comment.
The error message in t.Errorf refers to "consistency probes" instead of "subtree consistency probes". This is likely a copy-paste oversight from TestVerifyConsistencyProbes and can lead to confusing test failure reports.
| t.Errorf("errors verifying consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) | |
| t.Errorf("errors verifying subtree consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency proof verification tests along with a test data generator and various test cases. The feedback highlights two main issues: a local variable shadowing a package-level helper function in the test generator, and a copy-paste error in a test failure message that incorrectly refers to 'consistency probes' instead of 'subtree consistency probes'.
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.
| fileName := strings.ReplaceAll(probe.Desc, " ", "-") + ".json" | ||
|
|
||
| probeJson, err := json.MarshalIndent(probe, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("marshaling probe: %s", err) | ||
| } | ||
|
|
||
| fileLocation := filepath.Join(dir, fileName) | ||
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | ||
| return fmt.Errorf("writing probe: %s: %s", fileName, err) | ||
| } |
There was a problem hiding this comment.
The local variable fileName shadows the package-level helper function fileName. Additionally, instead of manually replacing spaces with hyphens using strings.ReplaceAll, you should use the existing fileName helper function to ensure consistent filename formatting (e.g., removing parentheses and other special characters) across all test data generators.
| fileName := strings.ReplaceAll(probe.Desc, " ", "-") + ".json" | |
| probeJson, err := json.MarshalIndent(probe, "", " ") | |
| if err != nil { | |
| return fmt.Errorf("marshaling probe: %s", err) | |
| } | |
| fileLocation := filepath.Join(dir, fileName) | |
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | |
| return fmt.Errorf("writing probe: %s: %s", fileName, err) | |
| } | |
| fn := fileName(probe.Desc) | |
| probeJson, err := json.MarshalIndent(probe, "", " ") | |
| if err != nil { | |
| return fmt.Errorf("marshaling probe: %s", err) | |
| } | |
| fileLocation := filepath.Join(dir, fn) | |
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | |
| return fmt.Errorf("writing probe: %s: %s", fn, err) | |
| } |
| } | ||
|
|
||
| if len(wrong) > 0 { | ||
| t.Errorf("errors verifying consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) |
There was a problem hiding this comment.
There is a copy-paste error in the test failure message. It refers to 'consistency probes' instead of 'subtree consistency probes'.
| t.Errorf("errors verifying consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) | |
| t.Errorf("errors verifying subtree consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) |
verify: modify test fields
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency proof verification tests, including test data generation in cmd/proofgen/main.go, verification logic in proof/verify_test.go, and numerous JSON test data files. The review feedback suggests minor improvements: correcting a copy-paste error in a test error message, utilizing an existing fileName helper function for consistent filename generation, and pre-allocating slice capacity in invalidSubtreeConsistencyProof to optimize memory allocation.
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 len(wrong) > 0 { | ||
| t.Errorf("errors verifying consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) |
There was a problem hiding this comment.
The error message in t.Errorf refers to "consistency probes" instead of "subtree consistency probes". This is a copy-paste oversight from TestVerifyConsistencyProbes and can be misleading when debugging test failures.
| t.Errorf("errors verifying consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) | |
| t.Errorf("errors verifying subtree consistency probes: \n%d out of %d failures \nError messages: \n%s", len(wrong), len(probes), strings.Join(wrong, "\n")) |
| fileName := strings.ReplaceAll(probe.Desc, " ", "-") + ".json" | ||
|
|
||
| probeJson, err := json.MarshalIndent(probe, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("marshaling probe: %s", err) | ||
| } | ||
|
|
||
| fileLocation := filepath.Join(dir, fileName) | ||
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | ||
| return fmt.Errorf("writing probe: %s: %s", fileName, err) | ||
| } |
There was a problem hiding this comment.
The writeSubtreeConsistencyProbe function manually constructs the filename using strings.ReplaceAll(probe.Desc, " ", "-") + ".json". However, there is an existing helper function fileName(n string) defined at line 814 that handles other special characters (such as parentheses and dashes) and is used by all other probe-writing functions. Using the helper function ensures consistent filename generation across all test probes.
| fileName := strings.ReplaceAll(probe.Desc, " ", "-") + ".json" | |
| probeJson, err := json.MarshalIndent(probe, "", " ") | |
| if err != nil { | |
| return fmt.Errorf("marshaling probe: %s", err) | |
| } | |
| fileLocation := filepath.Join(dir, fileName) | |
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | |
| return fmt.Errorf("writing probe: %s: %s", fileName, err) | |
| } | |
| fn := fileName(probe.Desc) | |
| probeJson, err := json.MarshalIndent(probe, "", " ") | |
| if err != nil { | |
| return fmt.Errorf("marshaling probe: %s", err) | |
| } | |
| fileLocation := filepath.Join(dir, fn) | |
| if err := os.WriteFile(fileLocation, probeJson, 0644); err != nil { | |
| return fmt.Errorf("writing probe: %s: %s", fn, err) | |
| } |
|
|
||
| func invalidSubtreeConsistencyProof(size1, size2 uint64, root1, root2 []byte, proof [][]byte) []subtreeConsistencyProbe { | ||
| cProbes := invalidConsistencyProof(size1, size2, root1, root2, proof) | ||
| ret := make([]subtreeConsistencyProbe, len(cProbes)) |
There was a problem hiding this comment.
In invalidSubtreeConsistencyProof, ret is initialized with a length of len(cProbes). Since we append exactly one more element to ret at the end of the function, we can pre-allocate the slice with a capacity of len(cProbes) + 1 to avoid an extra slice allocation and copy when appending.
| ret := make([]subtreeConsistencyProbe, len(cProbes)) | |
| ret := make([]subtreeConsistencyProbe, len(cProbes), len(cProbes)+1) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency proof verification tests. It refactors cmd/proofgen/main.go to generate subtree consistency test data under testdata/subtreeconsistency and adds a new test suite TestVerifySubtreeConsistencyProbes in proof/verify_test.go to verify these probes using VerifySubtreeConsistency. A comprehensive set of JSON test cases covering happy paths and various failure modes has been added. 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.
1 participant
No description provided.