From 7944226de0a5b4919dd9388f11e8667b34593977 Mon Sep 17 00:00:00 2001
From: Martin Hutchinson <mhutchinson@gmail.com>
Date: Wed, 6 Dec 2023 17:14:54 +0000
Subject: [PATCH] Inlined the read path so that tree.StorageSettings is always
 populated on a read tree

---
 storage/mysql/admin_storage_test.go |  4 +--
 storage/mysql/sql.go                | 39 ++++++++++++++++-------------
 storage/mysql/tree_storage.go       | 32 ++++++++---------------
 3 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/storage/mysql/admin_storage_test.go b/storage/mysql/admin_storage_test.go
index 908c7cb25a..d1a7cfd81d 100644
--- a/storage/mysql/admin_storage_test.go
+++ b/storage/mysql/admin_storage_test.go
@@ -270,8 +270,8 @@ func TestAdminTX_GetTreeLegacies(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		o, err := readSettings(readTree)
-		if err != nil {
+		o := &mysqlpb.StorageOptions{}
+		if err := anypb.UnmarshalTo(readTree.StorageSettings, o, proto.UnmarshalOptions{}); err != nil {
 			t.Fatal(err)
 		}
 		if got, want := o.SubtreeRevisions, tC.wantRevisioned; got != want {
diff --git a/storage/mysql/sql.go b/storage/mysql/sql.go
index 177dd3cfa6..a48e2a554f 100644
--- a/storage/mysql/sql.go
+++ b/storage/mysql/sql.go
@@ -128,24 +128,29 @@ func readTree(r row) (*trillian.Tree, error) {
 		}
 	}
 
-	if len(publicKey) > 0 {
-		// We're going to try to interpret this as storageSettings, but it could be a public key from a
-		// really old tree.
-		buff := bytes.NewBuffer(publicKey)
-		dec := gob.NewDecoder(buff)
-		ss := &storageSettings{}
-		if err := dec.Decode(ss); err != nil {
-			// Assume this was a public key and not a storageSettings
-			tree.StorageSettings = nil
-		} else {
-			o := &mysqlpb.StorageOptions{
-				SubtreeRevisions: ss.Revisioned,
-			}
-			tree.StorageSettings, err = anypb.New(o)
-			if err != nil {
-				return nil, fmt.Errorf("failed to put StorageSettings into tree: %w", err)
-			}
+	// We're going to try to interpret PublicKey as storageSettings, but it could be a
+	// public key from a really old tree, or an empty column from a tree created in the
+	// period between Trillian key material being removed and this column being used for
+	// storing settings.
+	buff := bytes.NewBuffer(publicKey)
+	dec := gob.NewDecoder(buff)
+	ss := &storageSettings{}
+	var o *mysqlpb.StorageOptions
+	if err := dec.Decode(ss); err != nil {
+		// If there are no storageSettings then this tree was created before settings
+		// were supported, and thus we have to populate the settings with the oldest
+		// settings for features.
+		o = &mysqlpb.StorageOptions{
+			SubtreeRevisions: true,
 		}
+	} else {
+		o = &mysqlpb.StorageOptions{
+			SubtreeRevisions: ss.Revisioned,
+		}
+	}
+	tree.StorageSettings, err = anypb.New(o)
+	if err != nil {
+		return nil, fmt.Errorf("failed to put StorageSettings into tree: %w", err)
 	}
 
 	return tree, nil
diff --git a/storage/mysql/tree_storage.go b/storage/mysql/tree_storage.go
index ba0c1abf56..71edd739ff 100644
--- a/storage/mysql/tree_storage.go
+++ b/storage/mysql/tree_storage.go
@@ -159,9 +159,16 @@ func (m *mySQLTreeStorage) beginTreeTx(ctx context.Context, tree *trillian.Tree,
 		klog.Warningf("Could not start tree TX: %s", err)
 		return treeTX{}, err
 	}
-	o, err := readSettings(tree)
-	if err != nil {
-		return treeTX{}, err
+	var subtreeRevisions bool
+	if tree.StorageSettings != nil {
+		// This nil check always passes when we have read the tree from the database.
+		// This check avoids a weird test case in logtests.go that uses an uninitialized tree.
+		// TODO(mhutchinson): can the test be changed, or is this uninitialized case legit?
+		o := &mysqlpb.StorageOptions{}
+		if err := anypb.UnmarshalTo(tree.StorageSettings, o, proto.UnmarshalOptions{}); err != nil {
+			return treeTX{}, fmt.Errorf("failed to unmarshal StorageSettings: %v", err)
+		}
+		subtreeRevisions = o.SubtreeRevisions
 	}
 	return treeTX{
 		tx:            t,
@@ -171,28 +178,11 @@ func (m *mySQLTreeStorage) beginTreeTx(ctx context.Context, tree *trillian.Tree,
 		treeType:      tree.TreeType,
 		hashSizeBytes: hashSizeBytes,
 		subtreeCache:  subtreeCache,
-		subtreeRevs:   o.SubtreeRevisions,
 		writeRevision: -1,
+		subtreeRevs:   subtreeRevisions,
 	}, nil
 }
 
-// TODO(mhutchinson): inline this method.
-func readSettings(tree *trillian.Tree) (*mysqlpb.StorageOptions, error) {
-	if tree.StorageSettings == nil {
-		// If there are no StorageSettings then this tree was created before settings
-		// were supported, and thus we have to populate the settings with the oldest
-		// settings for features.
-		return &mysqlpb.StorageOptions{
-			SubtreeRevisions: true,
-		}, nil
-	}
-	o := &mysqlpb.StorageOptions{}
-	if err := anypb.UnmarshalTo(tree.StorageSettings, o, proto.UnmarshalOptions{}); err != nil {
-		return nil, fmt.Errorf("failed to unmarshal StorageSettings: %v", err)
-	}
-	return o, nil
-}
-
 type treeTX struct {
 	// mu ensures that tx can only be used for one query/exec at a time.
 	mu            *sync.Mutex