Conversation
* add accumulated test vectors from specs * lint + improvements
|
/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 verification functionality, including the new VerifySubtreeConsistency and RootFromSubtreeConsistencyProof functions, along with corresponding test vectors to validate subtree hashes, inclusion proofs, and consistency proofs. The review feedback suggests improving code readability by renaming confusing parameters in VerifySubtreeConsistency to better distinguish between subtree and parent roots, and using named return values in decompSubtreeProof to make its multi-value return signature self-documenting.
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 VerifySubtreeConsistency(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, root1, root2 []byte) error { | ||
| hash2, err := RootFromSubtreeConsistencyProof(hasher, start, end, size, proof, root1) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return verifyMatch(hash2, root2) |
There was a problem hiding this comment.
The parameter names root1 and root2 in VerifySubtreeConsistency can be confusing because they typically represent two roots of the same tree at different sizes (as in standard consistency proofs). Here, root1 is the root of the subtree, and root2 is the root of the parent tree. Renaming them to subRoot and parentRoot would improve readability and align with the parameter name subRoot used in RootFromSubtreeConsistencyProof.
| func VerifySubtreeConsistency(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, root1, root2 []byte) error { | |
| hash2, err := RootFromSubtreeConsistencyProof(hasher, start, end, size, proof, root1) | |
| if err != nil { | |
| return err | |
| } | |
| return verifyMatch(hash2, root2) | |
| func VerifySubtreeConsistency(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, subRoot, parentRoot []byte) error { | |
| hash2, err := RootFromSubtreeConsistencyProof(hasher, start, end, size, proof, subRoot) | |
| if err != nil { | |
| return err | |
| } | |
| return verifyMatch(hash2, parentRoot) | |
| } |
| // There's one per level and they can be inside or outside of the subtree. | ||
| // - inner+subBorder: end index of border proof hashes inside the subtree. | ||
| // (proof[inner:inner+subBorder]). These nodes are left siblings only. | ||
| func decompSubtreeProof(start, end, size uint64, border int) (int, int, int) { |
There was a problem hiding this comment.
Using named return values for decompSubtreeProof would make the function signature self-documenting and significantly improve readability, especially since it returns three int values. This also directly aligns with the excellent documentation comments above the function.
| func decompSubtreeProof(start, end, size uint64, border int) (int, int, int) { | |
| func decompSubtreeProof(start, end, size uint64, border int) (subInner, inner, borderEnd int) { |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency verification by adding the VerifySubtreeConsistency and RootFromSubtreeConsistencyProof functions, along with supporting proof decomposition helpers in proof/verify.go. It also adds comprehensive test vectors in testonly/vectors_test.go to validate subtree hashes, inclusion proofs, and consistency proofs. The review feedback recommends validating the size of subRoot in RootFromSubtreeConsistencyProof to prevent potential panics, and expanding the test suite to actively verify the generated proofs using the newly introduced verification functions.
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 RootFromSubtreeConsistencyProof(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, subRoot []byte) ([]byte, error) { | ||
| err := isSubtreeValid(start, end) | ||
| switch { | ||
| case err != nil: | ||
| return nil, fmt.Errorf("subtree invalid: %v", err) | ||
| case size < end: | ||
| return nil, fmt.Errorf("size (%d) < end (%d)", size, end) | ||
| case start == 0 && size == end: | ||
| if len(proof) > 0 { | ||
| return nil, errors.New("start=0 and end=size, but proof is not empty") | ||
| } | ||
| return subRoot, nil | ||
| case len(proof) == 0: | ||
| return nil, errors.New("empty proof") | ||
| } | ||
| return rootFromSubtreeConsistencyProof(hasher, start, end, size, proof, subRoot) | ||
| } |
There was a problem hiding this comment.
It is recommended to validate the size of subRoot against hasher.Size() before proceeding with the proof verification. This ensures that any malformed or nil subRoot is caught early, preventing potential panics or unexpected behavior in the underlying hashing functions (similar to how leafHash is validated in RootFromInclusionProof).
func RootFromSubtreeConsistencyProof(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, subRoot []byte) ([]byte, error) {
if got, want := len(subRoot), hasher.Size(); got != want {
return nil, fmt.Errorf("subRoot has unexpected size %d, want %d", got, want)
}
err := isSubtreeValid(start, end)
switch {
case err != nil:
return nil, fmt.Errorf("subtree invalid: %v", err)
case size < end:
return nil, fmt.Errorf("size (%d) < end (%d)", size, end)
case start == 0 && size == end:
if len(proof) > 0 {
return nil, errors.New("start=0 and end=size, but proof is not empty")
}
return subRoot, nil
case len(proof) == 0:
return nil, errors.New("empty proof")
}
return rootFromSubtreeConsistencyProof(hasher, start, end, size, proof, subRoot)
}| package testonly | ||
|
|
||
| import ( | ||
| "crypto/sha256" | ||
| "fmt" | ||
| "io" | ||
| "testing" | ||
| ) |
There was a problem hiding this comment.
Import the proof package to enable verification of the generated proofs in the test vectors.
| package testonly | |
| import ( | |
| "crypto/sha256" | |
| "fmt" | |
| "io" | |
| "testing" | |
| ) | |
| package testonly | |
| import ( | |
| "crypto/sha256" | |
| "fmt" | |
| "io" | |
| "testing" | |
| "github.com/transparency-dev/merkle/proof" | |
| ) |
| func TestSubtreeInclusionProofVectors(t *testing.T) { | ||
| tree := subtreeVectorTree() | ||
| h := sha256.New() | ||
| for end := uint64(1); end <= subtreeVectorMax; end++ { | ||
| for start := range end { | ||
| if err := isSubtreeValid(start, end); err != nil { | ||
| continue | ||
| } | ||
| for index := start; index < end; index++ { | ||
| proof, err := tree.SubtreeInclusionProof(index, start, end) | ||
| if err != nil { | ||
| t.Fatalf("SubtreeInclusionProof(%d, %d, %d): %v", index, start, end, err) | ||
| } | ||
| writeProofLine(t, h, fmt.Sprintf("%d [%d, %d)", index, start, end), proof) | ||
| } | ||
| } | ||
| } | ||
| const want = "ac2a8f989e44d99e399db448050ff5f19757df53cfb716aa81015d3955d8163f" | ||
| if got := fmt.Sprintf("%x", h.Sum(nil)); got != want { | ||
| t.Errorf("subtree inclusion proof vector = %s, want %s", got, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
Verify the generated subtree inclusion proofs using the new VerifySubtreeInclusion function. This ensures that the verification logic is thoroughly tested against all test vectors.
func TestSubtreeInclusionProofVectors(t *testing.T) {
tree := subtreeVectorTree()
h := sha256.New()
for end := uint64(1); end <= subtreeVectorMax; end++ {
for start := range end {
if err := isSubtreeValid(start, end); err != nil {
continue
}
for index := start; index < end; index++ {
p, err := tree.SubtreeInclusionProof(index, start, end)
if err != nil {
t.Fatalf("SubtreeInclusionProof(%d, %d, %d): %v", index, start, end, err)
}
writeProofLine(t, h, fmt.Sprintf("%d [%d, %d)", index, start, end), p)
// Verify the generated inclusion proof.
leafHash := tree.LeafHash(index)
subRoot := tree.SubtreeHashAt(start, end)
if err := proof.VerifySubtreeInclusion(tree.hasher, index, start, end, leafHash, p, subRoot); err != nil {
t.Errorf("VerifySubtreeInclusion(%d, %d, %d): %v", index, start, end, err)
}
}
}
}
const want = "ac2a8f989e44d99e399db448050ff5f19757df53cfb716aa81015d3955d8163f"
if got := fmt.Sprintf("%x", h.Sum(nil)); got != want {
t.Errorf("subtree inclusion proof vector = %s, want %s", got, want)
}
}| func TestSubtreeConsistencyProofVectors(t *testing.T) { | ||
| tree := subtreeVectorTree() | ||
| h := sha256.New() | ||
| for n := range subtreeVectorMax + 1 { | ||
| for end := uint64(1); end <= n; end++ { | ||
| for start := range end { | ||
| if err := isSubtreeValid(start, end); err != nil { | ||
| continue | ||
| } | ||
| proof, err := tree.SubtreeConsistencyProof(start, end, n) | ||
| if err != nil { | ||
| t.Fatalf("SubtreeConsistencyProof(%d, %d, %d): %v", start, end, n, err) | ||
| } | ||
| writeProofLine(t, h, fmt.Sprintf("[%d, %d) %d", start, end, n), proof) | ||
| } | ||
| } | ||
| } | ||
| const want = "c586ebbb73a5621baf2140095d87dde934e3b6503a562a1a5215b8209edd083d" | ||
| if got := fmt.Sprintf("%x", h.Sum(nil)); got != want { | ||
| t.Errorf("subtree consistency proof vector = %s, want %s", got, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
Verify the generated subtree consistency proofs using the new VerifySubtreeConsistency function. This ensures that the verification logic is thoroughly tested against all test vectors.
func TestSubtreeConsistencyProofVectors(t *testing.T) {
tree := subtreeVectorTree()
h := sha256.New()
for n := range subtreeVectorMax + 1 {
for end := uint64(1); end <= n; end++ {
for start := range end {
if err := isSubtreeValid(start, end); err != nil {
continue
}
p, err := tree.SubtreeConsistencyProof(start, end, n)
if err != nil {
t.Fatalf("SubtreeConsistencyProof(%d, %d, %d): %v", start, end, n, err)
}
writeProofLine(t, h, fmt.Sprintf("[%d, %d) %d", start, end, n), p)
// Verify the generated consistency proof.
subRoot := tree.SubtreeHashAt(start, end)
parentRoot := tree.HashAt(n)
if err := proof.VerifySubtreeConsistency(tree.hasher, start, end, n, p, subRoot, parentRoot); err != nil {
t.Errorf("VerifySubtreeConsistency(%d, %d, %d): %v", start, end, n, err)
}
}
}
}
const want = "c586ebbb73a5621baf2140095d87dde934e3b6503a562a1a5215b8209edd083d"
if got := fmt.Sprintf("%x", h.Sum(nil)); got != want {
t.Errorf("subtree consistency proof vector = %s, want %s", got, want)
}
}
70100c2 to
afb2348
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency verification functionality, including the new VerifySubtreeConsistency and RootFromSubtreeConsistencyProof functions, along with corresponding helper functions and test vectors to validate correctness. Feedback on the changes suggests adding a validation check in RootFromSubtreeConsistencyProof to ensure the length of subRoot matches hasher.Size(), which prevents potential out-of-bounds panics and aligns with other verification functions.
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 RootFromSubtreeConsistencyProof(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, subRoot []byte) ([]byte, error) { | ||
| err := isSubtreeValid(start, end) |
There was a problem hiding this comment.
For defensive programming and consistency with other verification functions (such as RootFromInclusionProof), it is highly recommended to validate the length of subRoot against hasher.Size() at the beginning of RootFromSubtreeConsistencyProof. This prevents potential out-of-bounds panics or silent verification issues if an invalid or empty slice is passed.
func RootFromSubtreeConsistencyProof(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, subRoot []byte) ([]byte, error) {
if got, want := len(subRoot), hasher.Size(); got != want {
return nil, fmt.Errorf("subRoot has unexpected size %d, want %d", got, want)
}
err := isSubtreeValid(start, end)
1 participant
No description provided.