Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion proof/proof.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func reverse(ids []compact.NodeID) {
}
}

// isSubTreeValid returns whether a subtree covers a valid range.
// isSubtreeValid returns whether a subtree covers a valid range.
// A subtree is valid if there exist a parent tree node to:
// - all the subtree nodes
// - no extra node to the left of the subtree
Expand Down
25 changes: 23 additions & 2 deletions testonly/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,19 @@ func (t *Tree) Hash() []byte {
// HashAt returns the root hash at the given size.
// Requires 0 <= size <= Size(), otherwise panics.
func (t *Tree) HashAt(size uint64) []byte {
if size == 0 {
return t.SubtreeHashAt(0, size)
}

// 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")
}
Comment on lines +91 to +93

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")
	}

if start == end {
return t.hasher.EmptyRoot()
}
hashes := t.getNodes(compact.RangeNodes(0, size, nil))
hashes := t.getNodes(compact.RangeNodes(start, end, nil))

hash := hashes[len(hashes)-1]
for i := len(hashes) - 2; i >= 0; i-- {
Expand All @@ -105,6 +114,18 @@ func (t *Tree) InclusionProof(index, size uint64) ([][]byte, error) {
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(), and may panic otherwise.
// May return and error if the subtree boundaries or the index are not valid.
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)
}
Comment on lines +117 to +127

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)
}


// ConsistencyProof returns the consistency proof between the two given tree
// sizes. Requires 0 <= size1 <= size2 <= Size(), otherwise may panic.
func (t *Tree) ConsistencyProof(size1, size2 uint64) ([][]byte, error) {
Expand Down
40 changes: 40 additions & 0 deletions testonly/tree_fuzz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package testonly
import (
"bytes"
"math"
"math/bits"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -74,6 +75,45 @@ func FuzzInclusionProofAndVerify(f *testing.F) {
})
}

// Compute and verify inclusion proofs
func FuzzSubtreeInclusionProofAndVerify(f *testing.F) {
for end := 0; end <= 8; end++ {
for start := 0; start <= end; start++ {
for index := start; index <= end; index++ {
f.Add(uint64(index), uint64(start), uint64(end))
}
}
}
f.Fuzz(func(t *testing.T, index, start, end uint64) {
if end >= math.MaxUint16 {
return
}
t.Logf("index=%d, start=%d, end=%d", index, start, end)
if start >= end {
return
}
if index < start {
return
}
if index >= end {
return
}
if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 {
return
}
tree := newTree(genEntries(end))
p, err := tree.SubtreeInclusionProof(index, start, end)
t.Logf("proof=%v", p)
if err != nil {
t.Error(err)
}
Comment on lines +107 to +109

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
}

err = proof.VerifySubtreeInclusion(tree.hasher, index, start, end, tree.LeafHash(index), p, tree.SubtreeHashAt(start, end))
if err != nil {
t.Error(err)
}
})
}

func FuzzHashAtAgainstReferenceImplementation(f *testing.F) {
for size := 0; size <= 8; size++ {
for index := 0; index <= size; index++ {
Expand Down
Loading