eth: fix race condition in tests

The race was discoverable with
go test -race ./eth
or under 'slow' machine conditions, eg. GOMAXPROCS=1
and/or a 'slower' machine.

The race was around the minArtificialFinalityPeers
value, which was both
- assigned in (during) the tests, and
- checked in the nextSyncOp function

and the order of those uses was nondeterministic.

This patch resolves the issue by moving
the adhoc and temporary re-assignment of that
package-wide value to the start of the tests,
before any go routines for peer handling
or sync protocol are fired off.

Note that the real-clock timeouts in these
tests can still cause spurious test failures
under 'slow' conditions (see their 250ms sleeps).

Fixes #521

Date: 2023-01-30 13:39:38-08:00
Signed-off-by: meows <[email protected]>

eth/sync_test.go

@@ -158,7 +158,21 @@ func testSnapSyncDisabling(t *testing.T, ethVer uint, snapVer uint) {
 	}
 }
 
-func TestArtificialFinalityFeatureEnablingDisabling(t *testing.T) {
+func TestArtificialFinalityFeatureEnablingDisabling_AbleDisable(t *testing.T) {
+	// Tweak the hardcoded minArtificialFinalityPeers value
+	// before anything else (and establish its reassignment to original value).
+	// Lowering the value from the default is only done to save the boilerplate of
+	// setting up 5 peers instead of 1.
+	// Its important to do this before starting the syncer because
+	// of a potential data race with the minArtificialFinalityPeers value.
+	// This value does not (should not) change during geth runtime.
+	oMinAFPeers := minArtificialFinalityPeers
+	defer func() {
+		// Clean up after, resetting global default to original value.
+		minArtificialFinalityPeers = oMinAFPeers
+	}()
+	minArtificialFinalityPeers = 1
+
 	maxBlocksCreated := 1024
 	genFunc := blockGenContemporaryTime(int64(maxBlocksCreated))
 
@@ -172,13 +186,6 @@ func TestArtificialFinalityFeatureEnablingDisabling(t *testing.T) {
 	one := uint64(1)
 	a.chain.Config().SetECBP1100Transition(&one)
 
-	oMinAFPeers := minArtificialFinalityPeers
-	defer func() {
-		// Clean up after, resetting global default to original value.
-		minArtificialFinalityPeers = oMinAFPeers
-	}()
-	minArtificialFinalityPeers = 1
-
 	// Create a full protocol manager, check that fast sync gets disabled
 	b := newTestHandlerWithBlocksWithOpts(0, downloader.FullSync, genFunc)
 	if atomic.LoadUint32(&b.handler.snapSync) == 1 {
@@ -241,6 +248,20 @@ func TestArtificialFinalityFeatureEnablingDisabling(t *testing.T) {
 // TestArtificialFinalityFeatureEnablingDisabling_NoDisable tests that when the nodisable override
 // is in place (see NOTE1 below), AF is not disabled at the min peer floor.
 func TestArtificialFinalityFeatureEnablingDisabling_NoDisable(t *testing.T) {
+	// Tweak the hardcoded minArtificialFinalityPeers value
+	// before anything else (and establish its reassignment to original value).
+	// Lowering the value from the default is only done to save the boilerplate of
+	// setting up 5 peers instead of 1.
+	// Its important to do this before starting the syncer because
+	// of a potential data race with the minArtificialFinalityPeers value.
+	// This value does not (should not) change during geth runtime.
+	oMinAFPeers := minArtificialFinalityPeers
+	defer func() {
+		// Clean up after, resetting global default to original value.
+		minArtificialFinalityPeers = oMinAFPeers
+	}()
+	minArtificialFinalityPeers = 1
+
 	maxBlocksCreated := 1024
 	genFunc := blockGenContemporaryTime(int64(maxBlocksCreated))
 
@@ -251,13 +272,6 @@ func TestArtificialFinalityFeatureEnablingDisabling_NoDisable(t *testing.T) {
 	one := uint64(1)
 	a.chain.Config().SetECBP1100Transition(&one)
 
-	oMinAFPeers := minArtificialFinalityPeers
-	defer func() {
-		// Clean up after, resetting global default to original value.
-		minArtificialFinalityPeers = oMinAFPeers
-	}()
-	minArtificialFinalityPeers = 1
-
 	// Create a full protocol manager, check that fast sync gets disabled
 	b := newTestHandlerWithBlocksWithOpts(0, downloader.FullSync, genFunc)
 	defer b.close()
@@ -328,6 +342,20 @@ and the number of peers 'a' is connected with being only 1.)`)
 // be very old (far exceeding the auto-disable stale limit).
 // In this case, AF features should NOT be enabled.
 func TestArtificialFinalityFeatureEnablingDisabling_StaleHead(t *testing.T) {
+	// Tweak the hardcoded minArtificialFinalityPeers value
+	// before anything else (and establish its reassignment to original value).
+	// Lowering the value from the default is only done to save the boilerplate of
+	// setting up 5 peers instead of 1.
+	// Its important to do this before starting the syncer because
+	// of a potential data race with the minArtificialFinalityPeers value.
+	// This value does not (should not) change during geth runtime.
+	oMinAFPeers := minArtificialFinalityPeers
+	defer func() {
+		// Clean up after, resetting global default to original value.
+		minArtificialFinalityPeers = oMinAFPeers
+	}()
+	minArtificialFinalityPeers = 1
+
 	maxBlocksCreated := 1024
 
 	// Create a full protocol manager, check that fast sync gets disabled
@@ -336,13 +364,6 @@ func TestArtificialFinalityFeatureEnablingDisabling_StaleHead(t *testing.T) {
 	one := uint64(1)
 	a.chain.Config().SetECBP1100Transition(&one)
 
-	oMinAFPeers := minArtificialFinalityPeers
-	defer func() {
-		// Clean up after, resetting global default to original value.
-		minArtificialFinalityPeers = oMinAFPeers
-	}()
-	minArtificialFinalityPeers = 1
-
 	// Create a full protocol manager, check that fast sync gets disabled
 	b := newTestHandlerWithBlocksWithOpts(0, downloader.FullSync, nil)
 	defer b.close()