From 891b12b644feebf7fe288ae3205c92af71e0e911 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 30 Jan 2023 13:39:38 -0800 Subject: [PATCH] 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 https://github.com/etclabscore/core-geth/issues/521 Date: 2023-01-30 13:39:38-08:00 Signed-off-by: meows --- eth/sync_test.go | 65 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/eth/sync_test.go b/eth/sync_test.go index a111f65ddf..828286561c 100644 --- a/eth/sync_test.go +++ b/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()