Support both revisioned and unrevisioned subtrees

The same schema is used for both revisioned and unrevisioned subtrees. The difference is that we always write a revision of 0 in the unrevisioned case, which still means that there will only be a single entry per subtree.

The old PublicKey column has been used to store a settings object for newly created trees.
If this settings object is found in this column, then it will be parsed and checked for a property indicating that this tree is revisionless.
If the property is successfully confirmed to be revisionless then all writes to the subtree table will have a revision of 0, and all reads will skip the nested inner query that was causing slow queries.
If the property cannot be confirmed to be revisionless (no settings persisted, or settings persisted but explictly say to use revisioned), then the functionality will continue in the old way.
This preserves backwards compatibility, but makes it so that new trees will gain these features.

For users with legacy trees that wish to take advantage of the smaller storage costs and faster queries of the new revisionless storage, the proposed migration mechanism is to use migrillian to clone the old tree to a new tree. If anyone is interested in doing this then I recommend speaking to us on Slack (https://join.slack.com/t/transparency-dev/shared_invite/zt-27pkqo21d-okUFhur7YZ0rFoJVIOPznQ).

integration/admin/admin_integration_test.go

@@ -220,8 +220,9 @@ func TestAdminServer_UpdateTree(t *testing.T) {
 		want.TreeId = tree.TreeId
 		want.CreateTime = tree.CreateTime
 		want.UpdateTime = tree.UpdateTime
+		want.StorageSettings = tree.StorageSettings
 		if !proto.Equal(tree, want) {
-			diff := cmp.Diff(tree, want)
+			diff := cmp.Diff(tree, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-UpdateTree diff:\n%v", test.desc, diff)
 		}
 	}
@@ -357,7 +358,7 @@ func TestAdminServer_DeleteTree(t *testing.T) {
 		want.Deleted = true
 		want.DeleteTime = deletedTree.DeleteTime
 		if got := deletedTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-DeleteTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 
@@ -366,7 +367,7 @@ func TestAdminServer_DeleteTree(t *testing.T) {
 			t.Fatalf("%v: GetTree() returned err = %v", test.desc, err)
 		}
 		if got, want := storedTree, deletedTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-GetTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 	}
@@ -447,7 +448,7 @@ func TestAdminServer_UndeleteTree(t *testing.T) {
 			continue
 		}
 		if got, want := undeletedTree, createdTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-UndeleteTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 
@@ -456,7 +457,7 @@ func TestAdminServer_UndeleteTree(t *testing.T) {
 			t.Fatalf("%v: GetTree() returned err = %v", test.desc, err)
 		}
 		if got, want := storedTree, createdTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-GetTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 	}

storage/mysql/admin_storage.go

@@ -15,17 +15,21 @@
 package mysql
 
 import (
+	"bytes"
 	"context"
 	"database/sql"
+	"encoding/gob"
 	"fmt"
 	"sync"
 	"time"
 
 	"github.com/google/trillian"
 	"github.com/google/trillian/storage"
+	"github.com/google/trillian/storage/mysql/mysqlpb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/anypb"
 	"google.golang.org/protobuf/types/known/timestamppb"
 	"k8s.io/klog/v2"
 )
@@ -47,8 +51,8 @@ const (
 			Description,
 			CreateTimeMillis,
 			UpdateTimeMillis,
-			PrivateKey,
-			PublicKey,
+			PrivateKey, -- Unused
+			PublicKey, -- Used to store StorageSettings
 			MaxRootDurationMillis,
 			Deleted,
 			DeleteTimeMillis
@@ -62,7 +66,7 @@ const (
 )
 
 // NewAdminStorage returns a MySQL storage.AdminStorage implementation backed by DB.
-func NewAdminStorage(db *sql.DB) storage.AdminStorage {
+func NewAdminStorage(db *sql.DB) *mysqlAdminStorage {
 	return &mysqlAdminStorage{db}
 }
 
@@ -223,6 +227,39 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 	}
 	rootDuration := newTree.MaxRootDuration.AsDuration()
 
+	// When creating a new tree we automatically add StorageSettings to allow us to
+	// determine that this tree can support newer storage features. When reading
+	// trees that do not have this StorageSettings populated, it must be assumed that
+	// the tree was created with the oldest settings.
+	// The gist of this code is super simple: create a new StorageSettings with the most
+	// modern defaults if the created tree does not have one, and then create a struct that
+	// represents this to store in the DB. Unfortunately because this involves anypb, struct
+	// copies, marshalling, and proper error handling this turns into a scary amount of code.
+	if tree.StorageSettings != nil {
+		newTree.StorageSettings = proto.Clone(tree.StorageSettings).(*anypb.Any)
+	} else {
+		o := &mysqlpb.StorageOptions{
+			SubtreeRevisions: false, // Default behaviour for new trees is to skip writing subtree revisions.
+		}
+		a, err := anypb.New(o)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create new StorageOptions: %v", err)
+		}
+		newTree.StorageSettings = a
+	}
+	o := &mysqlpb.StorageOptions{}
+	if err := anypb.UnmarshalTo(newTree.StorageSettings, o, proto.UnmarshalOptions{}); err != nil {
+		return nil, fmt.Errorf("failed to unmarshal StorageOptions: %v", err)
+	}
+	ss := storageSettings{
+		Revisioned: o.SubtreeRevisions,
+	}
+	buff := &bytes.Buffer{}
+	enc := gob.NewEncoder(buff)
+	if err := enc.Encode(ss); err != nil {
+		return nil, fmt.Errorf("failed to encode storageSettings: %v", err)
+	}
+
 	insertTreeStmt, err := t.tx.PrepareContext(
 		ctx,
 		`INSERT INTO Trees(
@@ -236,8 +273,8 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 			Description,
 			CreateTimeMillis,
 			UpdateTimeMillis,
-			PrivateKey,
-			PublicKey,
+			PrivateKey, -- Unused
+			PublicKey, -- Used to store StorageSettings
 			MaxRootDurationMillis)
 		VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`)
 	if err != nil {
@@ -261,8 +298,8 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 		newTree.Description,
 		nowMillis,
 		nowMillis,
-		[]byte{}, // Unused, filling in for backward compatibility.
-		[]byte{}, // Unused, filling in for backward compatibility.
+		[]byte{},     // PrivateKey: Unused, filling in for backward compatibility.
+		buff.Bytes(), // Using the otherwise unused PublicKey for storing StorageSettings.
 		rootDuration/time.Millisecond,
 	)
 	if err != nil {
@@ -419,8 +456,21 @@ func validateDeleted(ctx context.Context, tx *sql.Tx, treeID int64, wantDeleted
 }
 
 func validateStorageSettings(tree *trillian.Tree) error {
-	if tree.StorageSettings != nil {
-		return fmt.Errorf("storage_settings not supported, but got %v", tree.StorageSettings)
+	if tree.StorageSettings.MessageIs(&mysqlpb.StorageOptions{}) {
+		return nil
 	}
-	return nil
+	if tree.StorageSettings == nil {
+		// No storage settings is OK, we'll just use the defaults for new trees
+		return nil
+	}
+	return fmt.Errorf("storage_settings must be nil or mysqlpb.StorageOptions, but got %v", tree.StorageSettings)
+}
+
+// storageSettings allows us to persist storage settings to the DB.
+// It is a tempting trap to use protos for this, but the way they encode
+// makes it impossible to tell the difference between no value ever written
+// and a value that was written with the default values for each field.
+// Using an explicit struct and gob encoding allows us to tell the difference.
+type storageSettings struct {
+	Revisioned bool
 }

storage/mysql/admin_storage_test.go

@@ -15,13 +15,16 @@
 package mysql
 
 import (
+	"bytes"
 	"context"
 	"database/sql"
+	"encoding/gob"
 	"fmt"
 	"testing"
 
 	"github.com/google/trillian"
 	"github.com/google/trillian/storage"
+	"github.com/google/trillian/storage/mysql/mysqlpb"
 	"github.com/google/trillian/storage/testonly"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/anypb"
@@ -128,12 +131,16 @@ func TestAdminTX_TreeWithNulls(t *testing.T) {
 	}
 }
 
-func TestAdminTX_StorageSettingsNotSupported(t *testing.T) {
+func TestAdminTX_StorageSettings(t *testing.T) {
 	cleanTestDB(DB)
 	s := NewAdminStorage(DB)
 	ctx := context.Background()
 
-	settings, err := anypb.New(&trillian.Tree{})
+	badSettings, err := anypb.New(&trillian.Tree{})
+	if err != nil {
+		t.Fatalf("Error marshaling proto: %v", err)
+	}
+	goodSettings, err := anypb.New(&mysqlpb.StorageOptions{})
 	if err != nil {
 		t.Fatalf("Error marshaling proto: %v", err)
 	}
@@ -142,16 +149,38 @@ func TestAdminTX_StorageSettingsNotSupported(t *testing.T) {
 		desc string
 		// fn attempts to either create or update a tree with a non-nil, valid Any proto
 		// on Tree.StorageSettings. It's expected to return an error.
-		fn func(storage.AdminStorage) error
+		fn      func(storage.AdminStorage) error
+		wantErr bool
 	}{
 		{
-			desc: "CreateTree",
+			desc: "CreateTree Bad Settings",
+			fn: func(s storage.AdminStorage) error {
+				tree := proto.Clone(testonly.LogTree).(*trillian.Tree)
+				tree.StorageSettings = badSettings
+				_, err := storage.CreateTree(ctx, s, tree)
+				return err
+			},
+			wantErr: true,
+		},
+		{
+			desc: "CreateTree nil Settings",
 			fn: func(s storage.AdminStorage) error {
 				tree := proto.Clone(testonly.LogTree).(*trillian.Tree)
-				tree.StorageSettings = settings
+				tree.StorageSettings = nil
 				_, err := storage.CreateTree(ctx, s, tree)
 				return err
 			},
+			wantErr: false,
+		},
+		{
+			desc: "CreateTree StorageOptions Settings",
+			fn: func(s storage.AdminStorage) error {
+				tree := proto.Clone(testonly.LogTree).(*trillian.Tree)
+				tree.StorageSettings = goodSettings
+				_, err := storage.CreateTree(ctx, s, tree)
+				return err
+			},
+			wantErr: false,
 		},
 		{
 			desc: "UpdateTree",
@@ -160,14 +189,93 @@ func TestAdminTX_StorageSettingsNotSupported(t *testing.T) {
 				if err != nil {
 					t.Fatalf("CreateTree() failed with err = %v", err)
 				}
-				_, err = storage.UpdateTree(ctx, s, tree.TreeId, func(tree *trillian.Tree) { tree.StorageSettings = settings })
+				_, err = storage.UpdateTree(ctx, s, tree.TreeId, func(tree *trillian.Tree) { tree.StorageSettings = badSettings })
 				return err
 			},
+			wantErr: true,
 		},
 	}
 	for _, test := range tests {
-		if err := test.fn(s); err == nil {
-			t.Errorf("%v: err = nil, want non-nil", test.desc)
+		if err := test.fn(s); (err != nil) != test.wantErr {
+			t.Errorf("err: %v, wantErr = %v", err, test.wantErr)
+		}
+	}
+}
+
+// Test reading variants of trees that could have been created by old versions
+// of Trillian to check we infer the correct storage options.
+func TestAdminTX_GetTreeLegacies(t *testing.T) {
+	cleanTestDB(DB)
+	s := NewAdminStorage(DB)
+	ctx := context.Background()
+
+	serializedStorageSettings := func(revisioned bool) []byte {
+		ss := storageSettings{
+			Revisioned: revisioned,
+		}
+		buff := &bytes.Buffer{}
+		enc := gob.NewEncoder(buff)
+		if err := enc.Encode(ss); err != nil {
+			t.Fatalf("failed to encode storageSettings: %v", err)
+		}
+		return buff.Bytes()
+	}
+	tests := []struct {
+		desc           string
+		key            []byte
+		wantRevisioned bool
+	}{
+		{
+			desc:           "No data",
+			key:            []byte{},
+			wantRevisioned: true,
+		},
+		{
+			desc:           "Public key",
+			key:            []byte("trustmethatthisisapublickey"),
+			wantRevisioned: true,
+		},
+		{
+			desc:           "StorageOptions revisioned",
+			key:            serializedStorageSettings(true),
+			wantRevisioned: true,
+		},
+		{
+			desc:           "StorageOptions revisionless",
+			key:            serializedStorageSettings(false),
+			wantRevisioned: false,
+		},
+	}
+	for _, tC := range tests {
+		// Create a tree with default settings, and then reach into the DB to override
+		// whatever was written into the persisted settings to align with the test case.
+		tree, err := storage.CreateTree(ctx, s, testonly.LogTree)
+		if err != nil {
+			t.Fatal(err)
+		}
+		// We are reaching really into the internals here, but it's the only way to set up
+		// archival state. Going through the Create/Update methods will change the storage
+		// options.
+		tx, err := s.db.BeginTx(ctx, nil /* opts */)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err := tx.Exec("UPDATE Trees SET PublicKey = ? WHERE TreeId = ?", tC.key, tree.TreeId); err != nil {
+			t.Fatal(err)
+		}
+		if err := tx.Commit(); err != nil {
+			t.Fatal(err)
+		}
+		readTree, err := storage.GetTree(ctx, s, tree.TreeId)
+		if err != nil {
+			t.Fatal(err)
+		}
+		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 {
+			t.Errorf("%s SubtreeRevisions: got %t, wanted %t", tC.desc, got, want)
 		}
 	}
 }

storage/mysql/log_storage.go

@@ -324,7 +324,7 @@ type logTreeTX struct {
 func (t *logTreeTX) GetMerkleNodes(ctx context.Context, ids []compact.NodeID) ([]tree.Node, error) {
 	t.treeTX.mu.Lock()
 	defer t.treeTX.mu.Unlock()
-	return t.subtreeCache.GetNodes(ids, t.getSubtreesFunc(ctx))
+	return t.subtreeCache.GetNodes(ids, t.getSubtreesAtRev(ctx, t.readRev))
 }
 
 func (t *logTreeTX) DequeueLeaves(ctx context.Context, limit int, cutoffTime time.Time) ([]*trillian.LogLeaf, error) {

storage/mysql/mysqlpb/gen.go

@@ -0,0 +1,18 @@
+// Copyright 2023 Google LLC. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package mysqlpb contains protobuf definitions used by the mysql implementation.
+package mysqlpb
+
+//go:generate protoc -I=. --go_out=paths=source_relative:. options.proto