Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

eth: fix race condition in eth/sync tests #522

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+67 −37
Open

eth: fix race condition in eth/sync tests #522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51da9a4
eth: fix race condition in tests
meowsbits Jan 30, 2023
ec625f6
eth: move chainSync.forced state type to atomic instead of bool
meowsbits Feb 7, 2023
5e018a5
eth: move var to uint32 for atomic accesses
meowsbits Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 15 additions & 7 deletions eth/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,15 @@ var (
// minArtificialFinalityPeers defines the minimum number of peers our node must be connected
// to in order to enable artificial finality features.
// A minimum number of peer connections mitigates the risk of lower-powered eclipse attacks.
minArtificialFinalityPeers = defaultMinSyncPeers
// We wrap this up in a uint32 and access it with atomic because
// the way the TestArtificialFinalityFeatureEnablingDisabling_* tests are cause
// a race condition; the tests want to assert variable operations under different conditions
// for this value, so they change it. And because they change it while they have handler instances
// running concurrently, the results are racey.
// So, tl;dr: use uint32 because atomic likes that and because the tests need it.
// Otherwise, the value should never change during the geth program, so the production race
// condition is expected to not exist (or be impracticable).
minArtificialFinalityPeers = uint32(defaultMinSyncPeers)

// artificialFinalitySafetyInterval defines the interval at which the local head is checked for staleness.
// If the head is found to be stale across this interval, artificial finality features are disabled.
Expand Down Expand Up @@ -107,7 +115,7 @@ func (h *handler) syncTransactions(p *eth.Peer) {
type chainSyncer struct {
handler *handler
force *time.Timer
forced bool // true when force timer fired
forced uint32 // true when force timer fired
warned time.Time
peerEventCh chan struct{}
doneCh chan error // non-nil when sync is running
Expand Down Expand Up @@ -166,7 +174,7 @@ func (cs *chainSyncer) loop() {
case err := <-cs.doneCh:
cs.doneCh = nil
cs.force.Reset(forceSyncCycle)
cs.forced = false
atomic.StoreUint32(&cs.forced, 0)

// If we've reached the merge transition but no beacon client is available, or
// it has not yet switched us over, keep warning the user that their infra is
Expand All @@ -176,7 +184,7 @@ func (cs *chainSyncer) loop() {
cs.warned = time.Now()
}
case <-cs.force.C:
cs.forced = true
atomic.StoreUint32(&cs.forced, 1)

case <-cs.handler.quitSync:
// Disable all insertion on the blockchain. This needs to happen before
Expand Down Expand Up @@ -210,12 +218,12 @@ func (cs *chainSyncer) nextSyncOp() *chainSyncOp {
}
// Ensure we're at minimum peer count.
minPeers := defaultMinSyncPeers
if cs.forced {
if atomic.LoadUint32(&cs.forced) == 1 {
minPeers = 1
} else if minPeers > cs.handler.maxPeers {
minPeers = cs.handler.maxPeers
}
if cs.handler.peers.len() < minArtificialFinalityPeers {
if cs.handler.peers.len() < int(atomic.LoadUint32(&minArtificialFinalityPeers)) {
if cs.handler.chain.IsArtificialFinalityEnabled() {
// If artificial finality state is forcefully set (overridden) this will just be a noop.
cs.handler.chain.EnableArtificialFinality(false, "reason", "low peers", "peers", cs.handler.peers.len())
Expand Down Expand Up @@ -245,7 +253,7 @@ func (cs *chainSyncer) nextSyncOp() *chainSyncOp {
// - In full sync mode.
if op.mode == downloader.FullSync &&
// - Have enough peers.
cs.handler.peers.len() >= minArtificialFinalityPeers &&
cs.handler.peers.len() >= int(atomic.LoadUint32(&minArtificialFinalityPeers)) &&
// - Head is not stale.
!(time.Since(time.Unix(int64(cs.handler.chain.CurrentHeader().Time), 0)) > artificialFinalitySafetyInterval) &&
// - AF is disabled (so we should reenable).
Expand Down
82 changes: 52 additions & 30 deletions eth/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 := atomic.LoadUint32(&minArtificialFinalityPeers)
defer func() {
// Clean up after, resetting global default to original value.
atomic.StoreUint32(&minArtificialFinalityPeers, oMinAFPeers)
}()
atomic.StoreUint32(&minArtificialFinalityPeers, 1)

maxBlocksCreated := 1024
genFunc := blockGenContemporaryTime(int64(maxBlocksCreated))

Expand All @@ -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 {
Expand Down Expand Up @@ -212,7 +219,8 @@ func TestArtificialFinalityFeatureEnablingDisabling(t *testing.T) {
t.Fatalf("sync failed: %v", err)
}

b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)

next := b.handler.chainSync.nextSyncOp()
if next != nil {
t.Fatal("non-nil next sync op")
Expand All @@ -225,10 +233,10 @@ func TestArtificialFinalityFeatureEnablingDisabling(t *testing.T) {
}

// Set the value back to default (more than 1).
minArtificialFinalityPeers = oMinAFPeers
atomic.StoreUint32(&minArtificialFinalityPeers, oMinAFPeers)

// Next sync op will unset AF because manager only has 1 peer.
b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)
next = b.handler.chainSync.nextSyncOp()
if next != nil {
t.Fatal("non-nil next sync op")
Expand All @@ -241,6 +249,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 := atomic.LoadUint32(&minArtificialFinalityPeers)
defer func() {
// Clean up after, resetting global default to original value.
atomic.StoreUint32(&minArtificialFinalityPeers, oMinAFPeers)
}()
atomic.StoreUint32(&minArtificialFinalityPeers, 1)

maxBlocksCreated := 1024
genFunc := blockGenContemporaryTime(int64(maxBlocksCreated))

Expand All @@ -251,13 +273,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()
Expand Down Expand Up @@ -292,12 +307,12 @@ func TestArtificialFinalityFeatureEnablingDisabling_NoDisable(t *testing.T) {
t.Fatalf("sync failed: %v", err)
}

b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)
next := b.handler.chainSync.nextSyncOp()

// Revert safety condition overrides to default values.
// Set the value back to default (more than 1).
minArtificialFinalityPeers = oMinAFPeers
atomic.StoreUint32(&minArtificialFinalityPeers, oMinAFPeers)

if next != nil {
t.Fatal("non-nil next sync op")
Expand All @@ -310,7 +325,7 @@ func TestArtificialFinalityFeatureEnablingDisabling_NoDisable(t *testing.T) {
}

// Next sync op will unset AF because manager only has 1 peer.
b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)
next = b.handler.chainSync.nextSyncOp()
if next != nil {
t.Fatal("non-nil next sync op")
Expand All @@ -328,6 +343,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 := atomic.LoadUint32(&minArtificialFinalityPeers)
defer func() {
// Clean up after, resetting global default to original value.
atomic.StoreUint32(&minArtificialFinalityPeers, oMinAFPeers)
}()
atomic.StoreUint32(&minArtificialFinalityPeers, 1)

maxBlocksCreated := 1024

// Create a full protocol manager, check that fast sync gets disabled
Expand All @@ -336,13 +365,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()
Expand Down Expand Up @@ -372,12 +394,12 @@ func TestArtificialFinalityFeatureEnablingDisabling_StaleHead(t *testing.T) {
t.Fatalf("sync failed: %v", err)
}

b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)
next := b.handler.chainSync.nextSyncOp()

// Revert safety condition overrides to default values.
// Set the value back to default (more than 1).
minArtificialFinalityPeers = oMinAFPeers
atomic.StoreUint32(&minArtificialFinalityPeers, oMinAFPeers)

if next != nil {
t.Fatal("non-nil next sync op")
Expand Down