Merge #34651 #34659

34651: server: rework TestClusterVersionBootstrapStrict r=andreimatei a=andreimatei

This test... I'm not entirely sure what it was supposed to test to be
honest, but it seemed to be more complicated than it needed to be. It
forced and emphasized MinSupportedVersion being equal to
BinaryServerVersion (which is generally not a thing). I've simplified
it, making it not muck with the versions, while keep (I think) the
things it was testing (to the extent that it was testing anything).

This test was also in my way because it created servers that pretended
to be versions that are not technically supported by the binary, and
this kind of funkiness is making my life hard as I'm trying to rework
the way in which versions are propagated and what knobs servers have,
etc.

Release note: None

34659: storage: don't leak committed protos to pushers on reproposal r=bdarnell,andreimatei a=tbg

TODO: test

----

A recent commit (master only) reintroduced a bug that we ironically had
spent a lot of time on [before]. In summary, it would allow the result
of an EndTransaction which would in itself *not* apply to leak and would
result in intents being committed even though their transaction
ultimately would not:

#34025 (comment)

We've diagnosed this pretty quickly the second time around, but clearly
we didn't do a good job at preventing the regression. I can see how this
would happen as the method this code is in is notoriously difficult to
test for it interfaces so much with everything else that it's difficult
to unit test it; one needs to jump through lots of hoops to target it,
and so we do it less than we ought to.

I believe this wasn't released in any alpha (nor backported anywhere),
so no release note is necessary.

Fixes #34025.

[before]: #30792 (comment)

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>

pkg/server/version_cluster_test.go

@@ -17,6 +17,7 @@ package server_test
 import (
 	"context"
 	gosql "database/sql"
+	"fmt"
 	"path/filepath"
 	"strconv"
 	"sync/atomic"
@@ -312,44 +313,38 @@ func TestClusterVersionUpgrade(t *testing.T) {
 	}
 }
 
-func TestClusterVersionBootstrapStrict(t *testing.T) {
+// Test that, after cluster bootstrap, the different ways of getting the cluster
+// version all agree.
+func TestAllVersionsAgree(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	ctx := context.Background()
 
-	// Four nodes that are all strictly version X without accepting anything else.
-	for _, versions := range [][][2]string{
-		{{"1.1", "1.1"}, {"1.1", "1.1"}, {"1.1", "1.1"}, {"1.1", "1.1"}},
-		{{"4.7", "4.7"}, {"4.7", "4.7"}, {"4.7", "4.7"}, {"4.7", "4.7"}},
-	} {
-		func() {
-			bootstrapVersion := cluster.ClusterVersion{
-				Version: roachpb.MustParseVersion(versions[0][0]),
-			}
-
-			knobs := base.TestingKnobs{
-				Store: &storage.StoreTestingKnobs{
-					BootstrapVersion: &bootstrapVersion,
-				},
-			}
-			tc := setupMixedCluster(t, knobs, versions, "")
-			defer tc.Stopper().Stop(ctx)
+	tcRaw := testcluster.StartTestCluster(t, 3, base.TestClusterArgs{})
+	defer tcRaw.Stopper().Stop(ctx)
+	tc := testClusterWithHelpers{
+		T:           t,
+		TestCluster: tcRaw,
+	}
 
-			exp := versions[0][0]
+	exp := cluster.BinaryServerVersion.String()
 
-			for i := 0; i < tc.NumServers(); i++ {
-				if version := tc.getVersionFromSetting(i).Version().Version.String(); version != exp {
-					t.Fatalf("%d: incorrect version %s (wanted %s)", i, version, exp)
-				}
-				if version := tc.getVersionFromShow(i); version != exp {
-					t.Fatalf("%d: incorrect version %s (wanted %s)", i, version, exp)
-				}
-
-				if version := tc.getVersionFromSelect(i); version != exp {
-					t.Fatalf("%d: incorrect version %q (wanted %s)", i, version, exp)
-				}
+	// The node bootstrapping the cluster starts at BinaryServerVersion, the
+	// others start at MinimumSupportedVersion and it takes them a gossip update
+	// to get to BinaryServerVersion. Hence, we loop until that gossip comes.
+	testutils.SucceedsSoon(tc, func() error {
+		for i := 0; i < tc.NumServers(); i++ {
+			if version := tc.getVersionFromSetting(i).Version().Version.String(); version != exp {
+				return fmt.Errorf("%d: incorrect version %s (wanted %s)", i, version, exp)
 			}
-		}()
-	}
+			if version := tc.getVersionFromShow(i); version != exp {
+				return fmt.Errorf("%d: incorrect version %s (wanted %s)", i, version, exp)
+			}
+			if version := tc.getVersionFromSelect(i); version != exp {
+				return fmt.Errorf("%d: incorrect version %q (wanted %s)", i, version, exp)
+			}
+		}
+		return nil
+	})
 }
 
 func TestClusterVersionMixedVersionTooOld(t *testing.T) {

pkg/storage/replica_raft.go

@@ -1927,7 +1927,7 @@ func (r *Replica) processRaftCommand(
 				log.Fatalf(ctx, "proposal must return either a reply or an error: %+v", proposal)
 			}
 			response.Intents = proposal.Local.DetachIntents()
-			response.EndTxns = proposal.Local.DetachEndTxns(response.Err != nil)
+			response.EndTxns = proposal.Local.DetachEndTxns(pErr != nil)
 			if pErr == nil {
 				lResult = proposal.Local
 			}