Remove the on-by-default ReferenceTransactions feature flagavar/remove-reference-transaction-feature-flag

The ReferenceTransactions flag has been on by default since
2074eaf50 (Revert "featureflag: Remove reference transaction feature
flag", 2020-12-07), and the non-transaction code for the Ruby codepath
got removed before that in d3ddbdbad (hooks: Remove the Ruby
reference-transaction hook feature flag, 2020-12-04).

It looks like we can remove it now, and since most of the if/else
around keeping this flag was in tests I've worked on actively I was
interested in removing it to speed them up, and make further changes
on them easier.

9 files changed, 41 insertions, 94 deletions

diff --git a/changelogs/unreleased/avar-remove-reference-transaction-feature-flag.yml b/changelogs/unreleased/avar-remove-reference-transaction-feature-flag.yml
new file mode 100644
index 000000000..06e5496eb
--- /dev/null
+++ b/changelogs/unreleased/avar-remove-reference-transaction-feature-flag.yml
@@ -0,0 +1,5 @@
+---
+title: Remove the on-by-default ReferenceTransactions feature flag
+merge_request: 3205
+author:
+type: changed
diff --git a/doc/design_ha.md b/doc/design_ha.md
index ff95c5484..be3d9b653 100644
--- a/doc/design_ha.md
+++ b/doc/design_ha.md
@@ -511,12 +511,8 @@ changes.
 
 ## Using Strong Consistency
 
-The current implementation of strong consistency via reference-transaction hook
-is enabled by default. You can use the following feature flags to change its
-behavior:
-
-- `gitaly_reference_transactions`: This feature flag is enabled by default. If
-  disabled, reference transactions will not be used.
+The current implementation of strong consistency is implemented via a
+`reference-transaction` hook.
 
 In order to observe reference transactions, the following metrics can be used:
 
@@ -531,9 +527,6 @@ In order to observe reference transactions, the following metrics can be used:
 - `gitaly_praefect_voters_per_transaction_total`: Number of nodes which have
   cast a vote in a given transaction.
 
-**Note:** Required work is only present in Gitaly starting with release
-v13.1.0-rc3.
-
 ## Notes
 * Existing discussions
 	* Requirements: https://gitlab.com/gitlab-org/gitaly/issues/1332
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index db379bb70..8d3e2de87 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -15,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -217,12 +216,9 @@ func TestUserCreateBranchWithTransaction(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulGitHooksForUserCreateBranchRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -436,13 +432,10 @@ func TestFailedUserCreateBranchRequest(t *testing.T) {
 	}
 }
 
-func TestSuccessfulUserDeleteBranchRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserDeleteBranchRequest)
-}
+func testSuccessfulUserDeleteBranchRequest(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserDeleteBranchRequest(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index 0bf67abb3..e1c297ebb 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"context"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -19,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -28,12 +26,9 @@ import (
 )
 
 func TestSuccessfulUserDeleteTagRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserDeleteTagRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -61,12 +56,9 @@ func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) {
 }
 
 func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulGitHooksForUserDeleteTagRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -170,13 +162,10 @@ end`, config.Config.Git.BinPath)
 	return hookPath, cleanup
 }
 
-func TestSuccessfulUserCreateTagRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserCreateTagRequest)
-}
+func testSuccessfulUserCreateTagRequest(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserCreateTagRequest(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -427,13 +416,10 @@ func TestUserCreateTagWithTransaction(t *testing.T) {
 	}
 }
 
-func TestSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation)
-}
+func testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -529,13 +515,10 @@ func testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *tes
 	}
 }
 
-func TestSuccessfulUserCreateTagRequestWithParsedTargetRevision(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserCreateTagRequestWithParsedTargetRevision)
-}
+func testSuccessfulUserCreateTagRequestWithParsedTargetRevision(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserCreateTagRequestWithParsedTargetRevision(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -905,16 +888,10 @@ func TestUserCreateTagStableTagIDs(t *testing.T) {
 	}, response.Tag)
 }
 
-// TODO: Rename to TestUserDeleteTag_successfulDeletionOfPrefixedTag,
-// see
-// https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2839#note_458751929
 func TestUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testUserDeleteTagsuccessfulDeletionOfPrefixedTag)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -1146,12 +1123,9 @@ func TestFailedUserDeleteTagRequestDueToValidation(t *testing.T) {
 }
 
 func TestFailedUserDeleteTagDueToHooks(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testFailedUserDeleteTagDueToHooks)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -1404,12 +1378,9 @@ func TestFailedUserCreateTagRequestDueToValidation(t *testing.T) {
 }
 
 func TestTagHookOutput(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testTagHookOutput)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testTagHookOutput(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go
index bcde99d2e..9068dab60 100644
--- a/internal/gitaly/service/operations/update_branches_test.go
+++ b/internal/gitaly/service/operations/update_branches_test.go
@@ -25,7 +25,6 @@ var (
 
 func TestSuccessfulUserUpdateBranchRequest(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
 		featureflag.GoUserUpdateBranch,
 	}).Run(t, testSuccessfulUserUpdateBranchRequest)
 }
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index d5dc4ca61..6eb3f054c 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -474,12 +474,9 @@ func runSmartHTTPHookServiceServer(t *testing.T, cfg config.Cfg) (*grpc.Server,
 }
 
 func TestPostReceiveWithTransactionsViaPraefect(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testPostReceiveWithTransactionsViaPraefect)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testPostReceiveWithTransactionsViaPraefect(t *testing.T, ctx context.Context) {
 	defer func(cfg config.Cfg) {
 		config.Config = cfg
 	}(config.Config)
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 71fcf6581..10edf4c3c 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -11,8 +11,6 @@ type FeatureFlag struct {
 var (
 	// DistributedReads allows praefect to redirect accessor operations to up-to-date secondaries
 	DistributedReads = FeatureFlag{Name: "distributed_reads", OnByDefault: true}
-	// ReferenceTransactions will handle Git reference updates via the transaction service for strong consistency
-	ReferenceTransactions = FeatureFlag{Name: "reference_transactions", OnByDefault: true}
 	// LogCommandStats will log additional rusage stats for commands
 	LogCommandStats = FeatureFlag{Name: "log_command_stats", OnByDefault: false}
 	// GoUserCherryPick enables the Go implementation of UserCherryPick
@@ -105,7 +103,6 @@ var (
 var All = []FeatureFlag{
 	DistributedReads,
 	LogCommandStats,
-	ReferenceTransactions,
 	GoUserCherryPick,
 	GoUserUpdateBranch,
 	GoUserCommitFiles,
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 206c040e8..8a6c353b8 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -144,10 +144,6 @@ func init() {
 }
 
 func shouldUseTransaction(ctx context.Context, method string) bool {
-	if !featureflag.IsEnabled(ctx, featureflag.ReferenceTransactions) {
-		return false
-	}
-
 	condition, ok := transactionRPCs[method]
 	if !ok {
 		return false
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index 45f173999..7b9d8b5b5 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -1269,19 +1269,15 @@ func TestStreamDirectorStorageScopeError(t *testing.T) {
 }
 
 func TestDisabledTransactionsWithFeatureFlag(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		for rpc, enabledFn := range transactionRPCs {
-			if enabledFn(ctx) {
-				require.Equal(t,
-					featureflag.IsEnabled(ctx, featureflag.ReferenceTransactions),
-					shouldUseTransaction(ctx, rpc),
-				)
-				break
-			}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	for rpc, enabledFn := range transactionRPCs {
+		if enabledFn(ctx) {
+			require.True(t, shouldUseTransaction(ctx, rpc))
+			break
 		}
-	})
+	}
 }
 
 func requireScopeOperation(t *testing.T, registry *protoregistry.Registry, fullMethod string, scope protoregistry.Scope, op protoregistry.OpType) {