cmd/proofgen for subtree consistency proofs - #8
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree inclusion proof generation to the proofgen tool, adding the subtreeInclusionProbe struct and several helper functions to generate and write subtree inclusion test vectors (including corrupted, single-entry, static, and error cases). A review comment points out a copy-paste error in errorSubtreeInclusionProbes where all error test cases have their Desc field set to "empty root", and suggests updating them to match their actual scenarios.
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.
| tests := []struct { | ||
| filename string | ||
| probe subtreeInclusionProbe | ||
| }{ | ||
| { | ||
| filename: "everything-zero.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 0, | ||
| Start: 0, | ||
| End: 0, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "start-equals-end.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 0, | ||
| Start: 1, | ||
| End: 1, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "invalid-subtree.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 3, | ||
| Start: 3, | ||
| End: 5, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "invalid-large-subtree.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 1, | ||
| Start: 1, | ||
| End: 9223372036854775810, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "oob-left.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 0, | ||
| Start: 1, | ||
| End: 2, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "oob-right.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 3, | ||
| Start: 0, | ||
| End: 2, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "oob-right-2.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 3, | ||
| Start: 0, | ||
| End: 3, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "start-larger-than-end.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 0, | ||
| Start: 2, | ||
| End: 1, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
All of the error test cases in errorSubtreeInclusionProbes have their Desc field set to "empty root". This appears to be a copy-paste error, as each test case represents a different error condition (e.g., "everything zero", "start equals end", "out of bounds left", etc.). Updating these descriptions to match the actual test scenarios will make the generated test vectors much clearer and easier to debug.
tests := []struct {
filename string
probe subtreeInclusionProbe
}{
{
filename: "everything-zero.json",
probe: subtreeInclusionProbe{
LeafIdx: 0,
Start: 0,
End: 0,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "everything zero",
WantError: true,
},
},
{
filename: "start-equals-end.json",
probe: subtreeInclusionProbe{
LeafIdx: 0,
Start: 1,
End: 1,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "start equals end",
WantError: true,
},
},
{
filename: "invalid-subtree.json",
probe: subtreeInclusionProbe{
LeafIdx: 3,
Start: 3,
End: 5,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "invalid subtree",
WantError: true,
},
},
{
filename: "invalid-large-subtree.json",
probe: subtreeInclusionProbe{
LeafIdx: 1,
Start: 1,
End: 9223372036854775810,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "invalid large subtree",
WantError: true,
},
},
{
filename: "oob-left.json",
probe: subtreeInclusionProbe{
LeafIdx: 0,
Start: 1,
End: 2,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "out of bounds left",
WantError: true,
},
},
{
filename: "oob-right.json",
probe: subtreeInclusionProbe{
LeafIdx: 3,
Start: 0,
End: 2,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "out of bounds right",
WantError: true,
},
},
{
filename: "oob-right-2.json",
probe: subtreeInclusionProbe{
LeafIdx: 3,
Start: 0,
End: 3,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "out of bounds right 2",
WantError: true,
},
},
{
filename: "start-larger-than-end.json",
probe: subtreeInclusionProbe{
LeafIdx: 0,
Start: 2,
End: 1,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "start larger than end",
WantError: true,
},
},
}There was a problem hiding this comment.
Code Review
This pull request introduces subtree inclusion proof generation and error subtree inclusion probes for testing, adding the subtreeInclusionProbe struct and several helper functions in cmd/proofgen/main.go. The review feedback identifies two key issues: potential undefined behavior in the bitCeil function when shifting by 64 or more, and a copy-paste error in errorSubtreeInclusionProbes where multiple test cases incorrectly share the same "empty root" description.
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 bitCeil(n uint64) uint64 { | ||
| if n <= 1 { | ||
| return 1 | ||
| } | ||
| return 1 << bits.Len64(n-1) | ||
| } |
There was a problem hiding this comment.
In Go, shifting an unsigned 64-bit integer by 64 or more is undefined behavior. If n > 1<<63, then bits.Len64(n-1) will return 64, resulting in 1 << 64. To prevent undefined behavior, add a guard check to ensure n does not exceed 1<<63.
func bitCeil(n uint64) uint64 {
if n <= 1 {
return 1
}
if n > 1<<63 {
panic("bitCeil: input exceeds 1<<63")
}
return 1 << bits.Len64(n-1)
}| tests := []struct { | ||
| filename string | ||
| probe subtreeInclusionProbe | ||
| }{ | ||
| { | ||
| filename: "everything-zero.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 0, | ||
| Start: 0, | ||
| End: 0, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "start-equals-end.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 0, | ||
| Start: 1, | ||
| End: 1, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "invalid-subtree.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 3, | ||
| Start: 3, | ||
| End: 5, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "invalid-large-subtree.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 1, | ||
| Start: 1, | ||
| End: 9223372036854775810, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "oob-left.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 0, | ||
| Start: 1, | ||
| End: 2, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "oob-right.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 3, | ||
| Start: 0, | ||
| End: 2, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "oob-right-2.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 3, | ||
| Start: 0, | ||
| End: 3, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| { | ||
| filename: "start-larger-than-end.json", | ||
| probe: subtreeInclusionProbe{ | ||
| LeafIdx: 0, | ||
| Start: 2, | ||
| End: 1, | ||
| Root: sha256EmptyTreeHash, | ||
| LeafHash: leafHash, | ||
| Proof: nil, | ||
| Desc: "empty root", | ||
| WantError: true, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
There is a copy-paste error where all test cases in errorSubtreeInclusionProbes have their Desc field set to "empty root". These descriptions should be updated to accurately describe each specific test scenario (e.g., "everything zero", "start equals end", etc.) to ensure the generated test data is clear and maintainable.
tests := []struct {
filename string
probe subtreeInclusionProbe
}{
{
filename: "everything-zero.json",
probe: subtreeInclusionProbe{
LeafIdx: 0,
Start: 0,
End: 0,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "everything zero",
WantError: true,
},
},
{
filename: "start-equals-end.json",
probe: subtreeInclusionProbe{
LeafIdx: 0,
Start: 1,
End: 1,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "start equals end",
WantError: true,
},
},
{
filename: "invalid-subtree.json",
probe: subtreeInclusionProbe{
LeafIdx: 3,
Start: 3,
End: 5,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "invalid subtree",
WantError: true,
},
},
{
filename: "invalid-large-subtree.json",
probe: subtreeInclusionProbe{
LeafIdx: 1,
Start: 1,
End: 9223372036854775810,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "invalid large subtree",
WantError: true,
},
},
{
filename: "oob-left.json",
probe: subtreeInclusionProbe{
LeafIdx: 0,
Start: 1,
End: 2,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "out of bounds left",
WantError: true,
},
},
{
filename: "oob-right.json",
probe: subtreeInclusionProbe{
LeafIdx: 3,
Start: 0,
End: 2,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "out of bounds right",
WantError: true,
},
},
{
filename: "oob-right-2.json",
probe: subtreeInclusionProbe{
LeafIdx: 3,
Start: 0,
End: 3,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "out of bounds right 2",
WantError: true,
},
},
{
filename: "start-larger-than-end.json",
probe: subtreeInclusionProbe{
LeafIdx: 0,
Start: 2,
End: 1,
Root: sha256EmptyTreeHash,
LeafHash: leafHash,
Proof: nil,
Desc: "start larger than end",
WantError: true,
},
},
}2891262 to
cb579cc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree inclusion proof generation in cmd/proofgen/main.go, along with corresponding test data and error cases. The feedback recommends avoiding potential integer overflow by checking the sentinel value before adding the shift to LeafIdx. Additionally, it is advised to use named fields when initializing inclusionProbe structs to improve maintainability and prevent future breakage.
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.
| leafIdx := p.LeafIdx + shift | ||
| if p.LeafIdx == ^uint64(0) { | ||
| leafIdx = ^uint64(0) | ||
| } |
There was a problem hiding this comment.
Avoid performing an addition that overflows and wraps around when p.LeafIdx is ^uint64(0). Instead, check if p.LeafIdx is the sentinel value before performing the addition. This is cleaner and avoids relying on unsigned overflow behavior.
| leafIdx := p.LeafIdx + shift | |
| if p.LeafIdx == ^uint64(0) { | |
| leafIdx = ^uint64(0) | |
| } | |
| leafIdx := p.LeafIdx | |
| if leafIdx != ^uint64(0) { | |
| leafIdx += shift | |
| } |
| } | ||
|
|
||
| func corruptedSubtreeInclusionProbes(dir string, leafIdx, treeSize uint64, proof [][]byte, root, leafHash []byte) error { | ||
| happyPath := inclusionProbe{leafIdx, treeSize, root, leafHash, proof, "happy path", false} |
There was a problem hiding this comment.
Use named fields when initializing the inclusionProbe struct. This is a Go best practice that prevents compilation errors or silent bugs if the struct fields are reordered or new fields are added in the future.
happyPath := inclusionProbe{
LeafIdx: leafIdx,
TreeSize: treeSize,
Root: root,
LeafHash: leafHash,
Proof: proof,
Desc: "happy path",
WantError: false,
}| {emptyHash, hash, "empty root", true}, | ||
| {emptyHash, emptyHash, "empty root and leaf", true}, | ||
| } { | ||
| probe := inclusionProbe{0, 1, p.root, p.leaf, proof, p.desc, p.wantErr} |
There was a problem hiding this comment.
324958d to
f595ee4
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree inclusion proof generation and error test cases, refactoring the existing inclusion proof generation to use a writer interface for better code reuse. The review feedback suggests improving code clarity and consistency by using an explicit uint64(1) type in a bit shift operation and replacing a large magic number with a more readable expression.
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 n <= 1 { | ||
| return 1 | ||
| } | ||
| return 1 << bits.Len64(n-1) |
There was a problem hiding this comment.
| { | ||
| LeafIdx: 1, | ||
| Start: 1, | ||
| End: 9223372036854775810, |
There was a problem hiding this comment.
The decimal literal 9223372036854775810 is a magic number representing (1 << 63) + 2. Using an explicit expression like (1 << 63) + 2 or a named constant makes the code much more readable and self-documenting, especially since it relates to the 1 << 63 threshold check in isSubtreeValid.
| End: 9223372036854775810, | |
| End: (1 << 63) + 2, |
f930be2 to
bda39b0
Compare
90acf8a to
3cbfd6c
Compare
3cbfd6c to
0e03927
Compare
No description provided.