Skip to content

fuzz tests - #4

Open
phbnf wants to merge 2 commits into
mainfrom
fuzztests
Open

fuzz tests#4
phbnf wants to merge 2 commits into
mainfrom
fuzztests

Conversation

@phbnf

@phbnf phbnf commented Jun 8, 2026

Copy link
Copy Markdown
Owner

No description provided.

@phbnf

phbnf commented Jun 8, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request exports the IsSubtreeValid function and introduces helper methods (SubtreeHashAt and SubtreeInclusionProof) to the test-only Tree implementation, along with a new fuzz test to verify subtree inclusion proofs. The review feedback highlights a potential Out of Memory (OOM) crash in the fuzz test due to an unrestricted end parameter, a logic error in the fuzz test's seed corpus generation that limits the index range, and a minor Go style convention violation in the documentation comment of IsSubtreeValid.

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.

Comment thread testonly/tree_fuzz_test.go Outdated
Comment on lines +87 to +89
if (end - start) >= math.MaxUint16 {
return
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The check (end - start) >= math.MaxUint16 only limits the size of the subtree, but does not limit the absolute value of end. Since newTree(genEntries(end)) is called later, a very large end value (e.g., end = 1000000000 with start = 1000000000) will pass this check and attempt to allocate a massive slice in genEntries(end), leading to an Out Of Memory (OOM) crash of the fuzzer.

You should restrict end instead of end - start, similar to how size is restricted in other fuzz tests in this file.

Suggested change
if (end - start) >= math.MaxUint16 {
return
}
if end >= math.MaxUint16 {
return
}

Comment thread proof/proof.go Outdated
}

// isSubTreeValid returns whether a subtree covers a valid range.
// IsSubTreeValid returns whether a subtree covers a valid range.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

According to Go style conventions, the comment for an exported function should start with the function's name. The function is named IsSubtreeValid (with a lowercase 't'), but the comment starts with IsSubTreeValid (with an uppercase 'T').

Suggested change
// IsSubTreeValid returns whether a subtree covers a valid range.
// IsSubtreeValid returns whether a subtree covers a valid range.

Comment thread testonly/tree_fuzz_test.go Outdated
func FuzzSubtreeInclusionProofAndVerify(f *testing.F) {
for end := 0; end <= 8; end++ {
for start := 0; start <= end; start++ {
for index := start; index <= start; index++ {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The loop condition index <= start limits the seed corpus to only include index == start. This means other valid indices within the subtree (i.e., start < index < end) are not added to the seed corpus. Changing this to index <= end will correctly add all valid indices and the boundary out-of-bounds index to the seed corpus, matching the pattern used in FuzzInclusionProofAndVerify.

Suggested change
for index := start; index <= start; index++ {
for index := start; index <= end; index++ {

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request exports the IsSubtreeValid function and introduces helper methods SubtreeHashAt and SubtreeInclusionProof to the Tree struct, along with a new fuzz test FuzzSubtreeInclusionProofAndVerify. Feedback highlights the need to bound the end parameter in the fuzz test to prevent out-of-memory errors, fix a logic error in the fuzz seed corpus loop, and add an explicit bounds check to the newly exported IsSubtreeValid function to prevent integer underflow.

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.

Comment thread testonly/tree_fuzz_test.go Outdated
Comment on lines +87 to +89
if (end - start) >= math.MaxUint16 {
return
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In FuzzSubtreeInclusionProofAndVerify, the fuzzer only checks if the subtree size (end - start) is less than math.MaxUint16. However, end itself is not bounded and can be up to math.MaxUint64. Since newTree(genEntries(end)) is called, a very large end value will cause genEntries to attempt to allocate an extremely large slice, leading to resource exhaustion (OOM) and fuzzer crashes. Bounding end directly resolves this issue.

Suggested change
if (end - start) >= math.MaxUint16 {
return
}
if end >= math.MaxUint16 {
return
}

Comment thread testonly/tree_fuzz_test.go Outdated
Comment on lines +81 to +83
for index := start; index <= start; index++ {
f.Add(uint64(index), uint64(start), uint64(end))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The loop for generating the seed corpus has a typo/logic error: for index := start; index <= start; index++. This limits the seed corpus to only test index == start. To properly seed the fuzzer with a variety of index values (including valid ones and boundary cases), the loop should go up to end.

Suggested change
for index := start; index <= start; index++ {
f.Add(uint64(index), uint64(start), uint64(end))
}
for index := start; index <= end; index++ {
f.Add(uint64(index), uint64(start), uint64(end))
}

Comment thread proof/proof.go Outdated
Comment on lines 231 to 234
func IsSubtreeValid(start, end uint64) error {
l := end - start
if start == 0 {
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since IsSubtreeValid is now an exported function, it can be called with arbitrary inputs. If start > end, the subtraction end - start underflows, resulting in a very large value for l. This triggers the l > uint64(1)<<63 check, which returns a confusing error message about start needing to be 0. Adding an explicit check for start > end at the beginning of the function improves robustness and error clarity.

Suggested change
func IsSubtreeValid(start, end uint64) error {
l := end - start
if start == 0 {
return nil
func IsSubtreeValid(start, end uint64) error {
if start > end {
return fmt.Errorf("start %d greater than end %d", start, end)
}
l := end - start
if start == 0 {
return nil

@phbnf

phbnf commented Jun 8, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request exports the IsSubtreeValid function from the proof package and introduces new helper methods, SubtreeHashAt and SubtreeInclusionProof, to the Tree struct in testonly/tree.go. It also adds a new fuzz test to verify subtree inclusion proofs. The review feedback suggests correcting the documentation comments for both new methods to accurately reflect their behavior and adding explicit validation checks to prevent cryptic panics when preconditions are violated.

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.

Comment thread testonly/tree.go Outdated
Comment on lines +97 to +110
// HashAt returns the root hash of the [start, end) subtree.
// Requires 0 <= start <= end <= Size() otherwise panics.
func (t *Tree) SubtreeHashAt(start, end uint64) []byte {
if start == end {
return t.hasher.EmptyRoot()
}
hashes := t.getNodes(compact.RangeNodes(start, end, nil))

hash := hashes[len(hashes)-1]
for i := len(hashes) - 2; i >= 0; i-- {
hash = t.hasher.HashChildren(hashes[i], hash)
}
return hash
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The comment for SubtreeHashAt incorrectly refers to the function as HashAt. Additionally, for defensive programming, we should explicitly validate the preconditions start <= end and end <= t.size to avoid cryptic out-of-bounds panics deep inside getNodes or incorrect results due to integer overflow.

// SubtreeHashAt returns the root hash of the [start, end) subtree.
// Requires 0 <= start <= end <= Size() otherwise panics.
func (t *Tree) SubtreeHashAt(start, end uint64) []byte {
	if start > end {
		panic(fmt.Sprintf("start %d > end %d", start, end))
	}
	if end > t.size {
		panic(fmt.Sprintf("end %d > size %d", end, t.size))
	}
	if start == end {
		return t.hasher.EmptyRoot()
	}
	hashes := t.getNodes(compact.RangeNodes(start, end, nil))

	hash := hashes[len(hashes)-1]
	for i := len(hashes) - 2; i >= 0; i-- {
		hash = t.hasher.HashChildren(hashes[i], hash)
	}
	return hash
}

Comment thread testonly/tree.go
Comment on lines +123 to +135
// SubtreeInclusionProof returns the inclusion proof for the given leaf index in the
// [start, end) subtree.
// It requires (panics otherwise):
// - 0 <= start <= index < end
// - start to be a multiple of the smallest power of two greater than or equal to
// (end - start)
func (t *Tree) SubtreeInclusionProof(index, start, end uint64) ([][]byte, error) {
nodes, err := proof.SubtreeInclusion(index, start, end)
if err != nil {
return nil, err
}
return nodes.Rehash(t.getNodes(nodes.IDs), t.hasher.HashChildren)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The documentation comment states that the function panics if 0 <= start <= index < end is violated, but it actually returns an error instead of panicking. Conversely, it does panic if end > Size(), which is not documented. Let's update the comment to accurately reflect the error and panic conditions, and add an explicit check for end > t.size to panic with a clear message.

Suggested change
// SubtreeInclusionProof returns the inclusion proof for the given leaf index in the
// [start, end) subtree.
// It requires (panics otherwise):
// - 0 <= start <= index < end
// - start to be a multiple of the smallest power of two greater than or equal to
// (end - start)
func (t *Tree) SubtreeInclusionProof(index, start, end uint64) ([][]byte, error) {
nodes, err := proof.SubtreeInclusion(index, start, end)
if err != nil {
return nil, err
}
return nodes.Rehash(t.getNodes(nodes.IDs), t.hasher.HashChildren)
}
// SubtreeInclusionProof returns the inclusion proof for the given leaf index in the
// [start, end) subtree.
// It requires end <= Size() (panics otherwise).
// It returns an error if:
// - index is out of bounds [start, end)
// - the subtree [start, end) is invalid
func (t *Tree) SubtreeInclusionProof(index, start, end uint64) ([][]byte, error) {
if end > t.size {
panic(fmt.Sprintf("end %d > size %d", end, t.size))
}
nodes, err := proof.SubtreeInclusion(index, start, end)
if err != nil {
return nil, err
}
return nodes.Rehash(t.getNodes(nodes.IDs), t.hasher.HashChildren)
}

@phbnf

phbnf commented Jun 8, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces subtree-related helper methods to the test-only tree implementation, specifically SubtreeHashAt and SubtreeInclusionProof, along with a corresponding fuzz test FuzzSubtreeInclusionProofAndVerify to validate subtree inclusion proofs. It also fixes a minor typo in a comment within proof/proof.go. Feedback was provided to correct a mismatch in the documentation comment for SubtreeHashAt and to add explicit range validation to ensure the function panics as documented when preconditions are violated.

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.

Comment thread testonly/tree.go Outdated
Comment on lines +97 to +103
// HashSubtreeAt returns the root hash of the [start, end) subtree.
// Requires 0 <= start <= end <= Size() otherwise panics.
func (t *Tree) SubtreeHashAt(start, end uint64) []byte {
if start == end {
return t.hasher.EmptyRoot()
}
hashes := t.getNodes(compact.RangeNodes(start, end, nil))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The comment on line 97 refers to HashSubtreeAt, but the function is named SubtreeHashAt. Additionally, the function documentation states that it requires start <= end <= Size(), otherwise it panics. However, if start > end, the function does not explicitly validate this and may not panic, potentially returning an incorrect hash or panicking with an index out of bounds inside getNodes depending on the tree size.

We should add an explicit check to enforce the precondition and fix the comment name.

// SubtreeHashAt returns the root hash of the [start, end) subtree.
// Requires 0 <= start <= end <= Size() otherwise panics.
func (t *Tree) SubtreeHashAt(start, end uint64) []byte {
	if start > end || end > t.size {
		panic("invalid subtree range")
	}
	if start == end {
		return t.hasher.EmptyRoot()
	}
	hashes := t.getNodes(compact.RangeNodes(start, end, nil))

@phbnf

phbnf commented Jun 8, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for computing subtree root hashes and subtree inclusion proofs in the testonly Tree helper, accompanied by a new fuzz test to verify their correctness. Feedback was provided on the SubtreeHashAt method to enforce and validate that the subtree range is MTC-compliant, preventing potential silent correctness issues during verification.

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.

Comment thread testonly/tree.go
Comment on lines +97 to +113
// SubtreeHashAt returns the root hash of the [start, end) subtree.
// Requires 0 <= start <= end <= Size() otherwise panics.
func (t *Tree) SubtreeHashAt(start, end uint64) []byte {
if start > end || end > t.size {
panic("invalid subtree range")
}
if start == end {
return t.hasher.EmptyRoot()
}
hashes := t.getNodes(compact.RangeNodes(start, end, nil))

hash := hashes[len(hashes)-1]
for i := len(hashes) - 2; i >= 0; i-- {
hash = t.hasher.HashChildren(hashes[i], hash)
}
return hash
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The SubtreeHashAt method computes the root hash of the [start, end) subtree by hashing the compact range nodes. However, if the subtree range is not MTC-compliant (i.e., start is not a multiple of bitCeil(end - start)), the resulting hash will not match the root of a standalone Merkle tree of size end - start (which is what VerifySubtreeInclusion expects).

To prevent silent correctness issues and ensure consistency with SubtreeInclusionProof and VerifySubtreeInclusion, we should enforce that the subtree range is valid/MTC-compliant, or at least document this requirement and panic if it is violated.

// SubtreeHashAt returns the root hash of the [start, end) subtree.
// Requires 0 <= start <= end <= Size(), and the subtree range to be MTC compliant, otherwise panics.
func (t *Tree) SubtreeHashAt(start, end uint64) []byte {
	if start > end || end > t.size {
		panic("invalid subtree range")
	}
	if start == end {
		return t.hasher.EmptyRoot()
	}
	if start != 0 {
		l := end - start
		bc := uint64(1)
		for bc < l {
			bc <<= 1
		}
		if start%bc != 0 {
			panic("subtree range is not MTC compliant")
		}
	}
	hashes := t.getNodes(compact.RangeNodes(start, end, nil))

	hash := hashes[len(hashes)-1]
	for i := len(hashes) - 2; i >= 0; i-- {
		hash = t.hasher.HashChildren(hashes[i], hash)
	}
	return hash
}

@phbnf

phbnf commented Jun 9, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces subtree-related operations to the test tree, specifically adding SubtreeHashAt and SubtreeInclusionProof methods, refactoring HashAt to use the new subtree hashing, and adding a fuzz test to verify subtree inclusion proofs. The review feedback suggests adding validation in SubtreeHashAt to ensure the provided range represents a valid subtree, and returning early in the fuzz test if generating the inclusion proof fails to prevent secondary panics.

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.

Comment thread testonly/tree.go
Comment on lines +91 to +93
if start > end || end > t.size {
panic("invalid subtree range")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The SubtreeHashAt method does not validate whether the [start, end) range represents a valid subtree (i.e., whether start is aligned to a multiple of the smallest power of two greater than or equal to the subtree length). If an invalid range is provided, the method will silently compute and return an incorrect/garbage hash by combining the compact range roots, which can lead to hard-to-debug test failures. Adding a validation check to panic on invalid subtree ranges ensures correctness.

	if start > end || end > t.size {
		panic("invalid subtree range")
	}
	l := end - start
	var bc uint64 = 1
	for bc < l && bc != 0 {
		bc <<= 1
	}
	if bc == 0 || start%bc != 0 {
		panic("invalid subtree range")
	}

Comment on lines +107 to +109
if err != nil {
t.Error(err)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If SubtreeInclusionProof returns an error, continuing to call VerifySubtreeInclusion with an invalid or nil proof p will likely cause a secondary failure or panic, which can obscure the original error. Returning early when err != nil ensures that the test fails cleanly with the actual error.

Suggested change
if err != nil {
t.Error(err)
}
if err != nil {
t.Error(err)
return
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant