better isSubtreeValid (transparency-dev#233)

Author: phbnf

proof/proof.go

@@ -59,15 +59,12 @@ func Inclusion(index, size uint64) (Nodes, error) {
 //   - start to be a multiple of the smallest power of two greater than or equal to
 //     (end - start)
 func SubtreeInclusion(index, start, end uint64) (Nodes, error) {
-	if start >= end {
-		return Nodes{}, fmt.Errorf("start %d greater than or equal to end %d", start, end)
+	if err := isSubtreeValid(start, end); err != nil {
+		return Nodes{}, fmt.Errorf("subtree invalid: %v", err)
 	}
 	if index < start || index >= end {
 		return Nodes{}, fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end)
 	}
-	if err := isSubtreeValid(start, end); err != nil {
-		return Nodes{}, fmt.Errorf("subtree invalid: %v", err)
-	}
 
 	// Shift the subtree to the left, such that it starts at 0.
 	p := nodes(index-start, 0, end-start).skipFirst()
@@ -229,20 +226,27 @@ func reverse(ids []compact.NodeID) {
 // - no extra node to the left of the subtree
 // - potentially extra nodes to the right of the subtree
 func isSubtreeValid(start, end uint64) error {
-	l := end - start
+	if start >= end {
+		return fmt.Errorf("start %d must be strictly less than end %d", start, end)
+	}
 	if start == 0 {
 		return nil
-	} else if l > uint64(1)<<63 {
-		// special-case large subtree to avoid panic
-		return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63. ", start, l)
 	}
-	if bc := bitCeil(l); start%bc != 0 {
+
+	l := end - start
+
+	// special-case large subtree to avoid panic
+	if l > uint64(1)<<63 {
+		return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63", start, l)
+	}
+	if bc := bitCeil(l); start&(bc-1) != 0 {
 		return fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc)
 	}
+
 	return nil
 }
 
-// bitCeil returns the smallest power of 2 larger than n.
+// bitCeil returns the smallest power of 2 larger than or equal to n.
 // MUST NOT be used with n larger than uint64(1)<<63.
 func bitCeil(n uint64) uint64 {
 	if n <= 1 {

proof/verify.go

@@ -59,15 +59,12 @@ func VerifyInclusion(hasher merkle.LogHasher, index, size uint64, leafHash []byt
 //   - start to be a multiple of the smallest power of two greater than or equal to
 //     (end - start)
 func VerifySubtreeInclusion(hasher merkle.LogHasher, index, start, end uint64, leafHash []byte, proof [][]byte, root []byte) error {
-	if start >= end {
-		return fmt.Errorf("start %d greater than or equal to end %d", start, end)
+	if err := isSubtreeValid(start, end); err != nil {
+		return fmt.Errorf("subtree invalid: %v", err)
 	}
 	if index < start || index >= end {
 		return fmt.Errorf("index %d out of bounds for subtree [%d, %d)", index, start, end)
 	}
-	if err := isSubtreeValid(start, end); err != nil {
-		return fmt.Errorf("subtree invalid: %v", err)
-	}
 	calcRoot, err := RootFromInclusionProof(hasher, index-start, end-start, leafHash, proof)
 	if err != nil {
 		return err

testonly/tree.go

@@ -15,6 +15,9 @@
 package testonly
 
 import (
+	"fmt"
+	"math/bits"
+
 	"github.com/transparency-dev/merkle"
 	"github.com/transparency-dev/merkle/compact"
 	"github.com/transparency-dev/merkle/proof"
@@ -143,3 +146,38 @@ func (t *Tree) getNodes(ids []compact.NodeID) [][]byte {
 	}
 	return hashes
 }
+
+// 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
+// - potentially extra nodes to the right of the subtree
+func isSubtreeValid(start, end uint64) error {
+	if start >= end {
+		return fmt.Errorf("start %d must be strictly less than end %d", start, end)
+	}
+	if start == 0 {
+		return nil
+	}
+
+	l := end - start
+
+	// special-case large subtree to avoid panic
+	if l > uint64(1)<<63 {
+		return fmt.Errorf("start %d must be 0 when subtree length %d > 1<<63", start, l)
+	}
+	if bc := bitCeil(l); start&(bc-1) != 0 {
+		return fmt.Errorf("start %d not a multiple of bit_ceil(end - start) = %d", start, bc)
+	}
+
+	return nil
+}
+
+// bitCeil returns the smallest power of 2 larger than or equal to n.
+// MUST NOT be used with n larger than uint64(1)<<63.
+func bitCeil(n uint64) uint64 {
+	if n <= 1 {
+		return 1
+	}
+	return uint64(1) << bits.Len64(n-1)
+}

testonly/tree_fuzz_test.go

@@ -3,7 +3,6 @@ package testonly
 import (
 	"bytes"
 	"math"
-	"math/bits"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -86,8 +85,7 @@ func FuzzSubtreeInclusionProofAndVerify(f *testing.F) {
 		if end >= math.MaxUint16 {
 			return
 		}
-		t.Logf("index=%d, start=%d, end=%d", index, start, end)
-		if start >= end {
+		if err := isSubtreeValid(start, end); err != nil {
 			return
 		}
 		if index < start {
@@ -96,9 +94,7 @@ func FuzzSubtreeInclusionProofAndVerify(f *testing.F) {
 		if index >= end {
 			return
 		}
-		if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 {
-			return
-		}
+		t.Logf("index=%d, start=%d, end=%d", index, start, end)
 		tree := newTree(genEntries(end))
 		p, err := tree.SubtreeInclusionProof(index, start, end)
 		t.Logf("proof=%v", p)