Vercons easy - #12
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 verification by adding VerifySubtreeConsistency and RootFromSubtreeConsistencyProof to support arbitrary subtrees [start, end). It also refactors the existing consistency proof verification logic and removes the unused chainInnerRight function. The review feedback suggests using RootFromConsistencyProof in VerifyConsistency to maintain specific error handling and consistency, and removing a redundant isSubtreeValid check in VerifySubtreeConsistency since it is already performed inside RootFromSubtreeConsistencyProof.
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.
| // hashes. Requires 0 < size1 <= size2. | ||
| func VerifyConsistency(hasher merkle.LogHasher, size1, size2 uint64, proof [][]byte, root1, root2 []byte) error { | ||
| hash2, err := RootFromConsistencyProof(hasher, size1, size2, proof, root1) | ||
| hash2, err := RootFromSubtreeConsistencyProof(hasher, 0, size1, size2, proof, root1) |
There was a problem hiding this comment.
Calling RootFromSubtreeConsistencyProof directly here bypasses the dedicated RootFromConsistencyProof function. Using RootFromConsistencyProof instead ensures that standard consistency proof validation benefits from clearer, more specific error messages (such as distinguishing when size1 == 0 or size2 < size1) and keeps the verification path consistent.
| hash2, err := RootFromSubtreeConsistencyProof(hasher, 0, size1, size2, proof, root1) | |
| hash2, err := RootFromConsistencyProof(hasher, size1, size2, proof, root1) |
| if err := isSubtreeValid(start, end); err != nil { | ||
| return fmt.Errorf("subtree invalid: %v", err) | ||
| } | ||
| hash2, err := RootFromSubtreeConsistencyProof(hasher, start, end, size, proof, root1) |
There was a problem hiding this comment.
The call to isSubtreeValid(start, end) is redundant here because RootFromSubtreeConsistencyProof already performs this exact check and returns the identical wrapped error message. Removing this redundant check simplifies the function and avoids duplicate validation.
hash2, err := RootFromSubtreeConsistencyProof(hasher, start, end, size, proof, root1)
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency verification in proof/verify.go by adding VerifySubtreeConsistency and RootFromSubtreeConsistencyProof, along with internal helper functions like chainSubtree to compute subtree roots. It also refactors VerifyConsistency to leverage these new subtree-based implementations. The reviewer suggests simplifying RootFromSubtreeConsistencyProof by removing a redundant check for start == 0 && size == end, as this case is already handled by the underlying rootFromSubtreeConsistencyProof function.
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.
| case start == 0 && size == end: | ||
| if len(proof) > 0 { | ||
| return nil, errors.New("start=0 and end=size, but proof is not empty") | ||
| } | ||
| return root1, nil |
There was a problem hiding this comment.
This special case check for start == 0 && size == end is redundant because the underlying rootFromSubtreeConsistencyProof function already handles the end == size case (which includes start == 0) correctly. Removing this check simplifies the code without changing the behavior.
6c41795 to
f78d8cc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency verification by adding the VerifySubtreeConsistency and RootFromSubtreeConsistencyProof functions, and refactoring existing consistency verification to utilize these new subtree-based methods. The review feedback suggests two improvements: first, calling RootFromConsistencyProof instead of RootFromSubtreeConsistencyProof directly inside VerifyConsistency to preserve specific validation checks and error messages; second, removing a redundant isSubtreeValid check in VerifySubtreeConsistency since it is already performed inside RootFromSubtreeConsistencyProof.
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.
| // hashes. Requires 0 < size1 <= size2. | ||
| func VerifyConsistency(hasher merkle.LogHasher, size1, size2 uint64, proof [][]byte, root1, root2 []byte) error { | ||
| hash2, err := RootFromConsistencyProof(hasher, size1, size2, proof, root1) | ||
| hash2, err := RootFromSubtreeConsistencyProof(hasher, 0, size1, size2, proof, root1) |
There was a problem hiding this comment.
Calling RootFromSubtreeConsistencyProof directly here bypasses the specific consistency validation checks and tailored error messages implemented in RootFromConsistencyProof (such as checking if size1 == 0 or size1 == size2). It is better to call RootFromConsistencyProof to ensure consistent and descriptive error messages for consistency verification.
| hash2, err := RootFromSubtreeConsistencyProof(hasher, 0, size1, size2, proof, root1) | |
| hash2, err := RootFromConsistencyProof(hasher, size1, size2, proof, root1) |
| if err := isSubtreeValid(start, end); err != nil { | ||
| return fmt.Errorf("subtree invalid: %v", err) | ||
| } | ||
| hash2, err := RootFromSubtreeConsistencyProof(hasher, start, end, size, proof, root1) |
There was a problem hiding this comment.
The isSubtreeValid(start, end) check is redundant here because RootFromSubtreeConsistencyProof already performs this exact check at its beginning (line 152). Removing this redundant check simplifies the function and avoids duplicate execution.
hash2, err := RootFromSubtreeConsistencyProof(hasher, start, end, size, proof, root1)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency verification by adding VerifySubtreeConsistency and RootFromSubtreeConsistencyProof, and refactoring RootFromConsistencyProof to utilize the new subtree logic. Feedback on these changes highlights two potential panic risks: first, the length of root1 should be validated against hasher.Size() to prevent downstream hashing issues, and second, the length of proof must be verified before accessing proof[0] to avoid an out-of-bounds panic.
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, root1 []byte) ([]byte, error) { | ||
| // TODO: add tests with invalid subtrees. | ||
| err := isSubtreeValid(start, end) |
There was a problem hiding this comment.
It is highly recommended to validate the length of root1 against hasher.Size() to ensure correctness and prevent potential panics or unexpected behavior in downstream hashing operations.
func RootFromSubtreeConsistencyProof(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, root1 []byte) ([]byte, error) {
if got, want := len(root1), hasher.Size(); got != want {
return nil, fmt.Errorf("root1 has unexpected size %d, want %d", got, want)
}
// TODO: add tests with invalid subtrees.
err := isSubtreeValid(start, end)| seed, pStart := proof[0], 1 | ||
| // Unless the argument subtree is full, in which case that rightmost subtree | ||
| // is the argument subtree itself. Its root is not included in the proof | ||
| // since a client verifying a subtree inclusion proof is expected to already | ||
| // know what the root of that subtree is. | ||
| if (end - start) == 1<<uint(shift) { | ||
| seed, pStart = root1, 0 | ||
| } | ||
| proof = proof[start:] | ||
| // Now len(proof) == inner+border, and proof is effectively a suffix of | ||
| // inclusion proof for entry |size1-1| in a tree of size |size2|. | ||
|
|
||
| // Verify the first root, if included in the proof. | ||
| if start != 0 { | ||
| mask := (size1 - 1) >> uint(shift) // Start chaining from level |shift|. | ||
| hash1 := chainInnerRight(hasher, seed, proof[:inner], mask) | ||
| hash1 = chainBorderRight(hasher, hash1, proof[inner:]) | ||
| if err := verifyMatch(hash1, root1); err != nil { | ||
| return nil, err | ||
| } | ||
| wantProofLen := pStart + (forkLevel - shift) + bits.OnesCount64((end-1)>>forkLevel) | ||
| if got := len(proof); got != wantProofLen { | ||
| return nil, fmt.Errorf("wrong proof size %d, want %d", got, wantProofLen) | ||
| } | ||
| proof = proof[pStart:] |
There was a problem hiding this comment.
Accessing proof[0] before validating that len(proof) matches wantProofLen is risky and can lead to an out-of-bounds panic if the proof is empty. It is safer to determine pStart first, validate the proof length, and only then access proof[0] if pStart == 1.
| seed, pStart := proof[0], 1 | |
| // Unless the argument subtree is full, in which case that rightmost subtree | |
| // is the argument subtree itself. Its root is not included in the proof | |
| // since a client verifying a subtree inclusion proof is expected to already | |
| // know what the root of that subtree is. | |
| if (end - start) == 1<<uint(shift) { | |
| seed, pStart = root1, 0 | |
| } | |
| proof = proof[start:] | |
| // Now len(proof) == inner+border, and proof is effectively a suffix of | |
| // inclusion proof for entry |size1-1| in a tree of size |size2|. | |
| // Verify the first root, if included in the proof. | |
| if start != 0 { | |
| mask := (size1 - 1) >> uint(shift) // Start chaining from level |shift|. | |
| hash1 := chainInnerRight(hasher, seed, proof[:inner], mask) | |
| hash1 = chainBorderRight(hasher, hash1, proof[inner:]) | |
| if err := verifyMatch(hash1, root1); err != nil { | |
| return nil, err | |
| } | |
| wantProofLen := pStart + (forkLevel - shift) + bits.OnesCount64((end-1)>>forkLevel) | |
| if got := len(proof); got != wantProofLen { | |
| return nil, fmt.Errorf("wrong proof size %d, want %d", got, wantProofLen) | |
| } | |
| proof = proof[pStart:] | |
| var seed []byte | |
| pStart := 1 | |
| // Unless the argument subtree is full, in which case that rightmost subtree | |
| // is the argument subtree itself. Its root is not included in the proof | |
| // since a client verifying a subtree inclusion proof is expected to already | |
| // know what the root of that subtree is. | |
| if (end - start) == 1<<uint(shift) { | |
| seed, pStart = root1, 0 | |
| } | |
| wantProofLen := pStart + (forkLevel - shift) + bits.OnesCount64((end-1)>>forkLevel) | |
| if got := len(proof); got != wantProofLen { | |
| return nil, fmt.Errorf("wrong proof size %d, want %d", got, wantProofLen) | |
| } | |
| if pStart == 1 { | |
| seed = proof[0] | |
| } | |
| proof = proof[pStart:] |
1d6215b to
1d7c956
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces subtree consistency verification functionality to the proof package by adding VerifySubtreeConsistency and RootFromSubtreeConsistencyProof, along with internal helper functions like chainSubtree. It also refactors RootFromConsistencyProof to leverage these new subtree verification capabilities. The feedback suggests validating the size of the input root1 hash against the hasher's expected size to prevent potential panics or unexpected behavior.
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, root1 []byte) ([]byte, error) { | ||
| // If the right end of the subtree overlaps with the right end of the tree, |
There was a problem hiding this comment.
To ensure robustness and prevent potential panics or unexpected behavior in the underlying hasher, it is highly recommended to validate that the input root1 has the expected hash size (hasher.Size()) before proceeding with any hashing operations.
func rootFromSubtreeConsistencyProof(hasher merkle.LogHasher, start, end, size uint64, proof [][]byte, root1 []byte) ([]byte, error) {
if got, want := len(root1), hasher.Size(); got != want {
return nil, fmt.Errorf("root1 has unexpected size %d, want %d", got, want)
}
// If the right end of the subtree overlaps with the right end of the tree,
6d01611 to
12541f2
Compare
1 participant
No description provided.