HeadFetcher data race fix (#3460)

* HeadFetcher data race fix * bazel run //:gazelle -- fix * add the db teardown to test * add TestChainService_SaveHead_DataRace test * split race and norace tests * change testset name * test CI with 'spectest' tag instead of 'raceon' * one more test CI with 'spectest' tag instead of 'raceon' * bazel run //:gazelle -- fix * set test tag to 'race_on' * not clone the state
2026-01-09 23:48:06 -05:00 · 2019-09-23 22:24:42 +03:00
parent 4432c88f73
commit b015dc793a
7 changed files with 187 additions and 12 deletions
--- a/beacon-chain/blockchain/BUILD.bazel
+++ b/beacon-chain/blockchain/BUILD.bazel
@@ -38,8 +38,16 @@ go_library(
    ],
 )

-go_test(
+test_suite(
    name = "go_default_test",
+    tests = [
+        ":go_raceoff_test",
+        ":go_raceon_test",
+    ],
+)
+
+go_test(
+    name = "go_raceoff_test",
    size = "medium",
    srcs = [
        "chain_info_test.go",
@@ -73,3 +81,38 @@ go_test(
        "@org_golang_x_net//context:go_default_library",
    ],
 )
+
+go_test(
+    name = "go_raceon_test",
+    srcs = [
+        "chain_info_norace_test.go",
+        "service_norace_test.go",
+    ],
+    embed = [":go_default_library"],
+    race = "on",
+    tags = ["race_on"],
+    deps = [
+        "//beacon-chain/cache/depositcache:go_default_library",
+        "//beacon-chain/core/blocks:go_default_library",
+        "//beacon-chain/core/helpers:go_default_library",
+        "//beacon-chain/core/state:go_default_library",
+        "//beacon-chain/db:go_default_library",
+        "//beacon-chain/db/testing:go_default_library",
+        "//beacon-chain/p2p:go_default_library",
+        "//beacon-chain/powchain:go_default_library",
+        "//proto/beacon/p2p/v1:go_default_library",
+        "//proto/eth/v1alpha1:go_default_library",
+        "//shared/bytesutil:go_default_library",
+        "//shared/event:go_default_library",
+        "//shared/params:go_default_library",
+        "//shared/testutil:go_default_library",
+        "@com_github_ethereum_go_ethereum//:go_default_library",
+        "@com_github_ethereum_go_ethereum//common:go_default_library",
+        "@com_github_ethereum_go_ethereum//core/types:go_default_library",
+        "@com_github_gogo_protobuf//proto:go_default_library",
+        "@com_github_prysmaticlabs_go_ssz//:go_default_library",
+        "@com_github_sirupsen_logrus//:go_default_library",
+        "@com_github_sirupsen_logrus//hooks/test:go_default_library",
+        "@org_golang_x_net//context:go_default_library",
+    ],
+)
--- a/beacon-chain/blockchain/chain_info.go
+++ b/beacon-chain/blockchain/chain_info.go
@@ -55,13 +55,16 @@ func (s *Service) FinalizedCheckpt() *ethpb.Checkpoint {

 // HeadSlot returns the slot of the head of the chain.
 func (s *Service) HeadSlot() uint64 {
+	s.headLock.RLock()
+	defer s.headLock.RUnlock()
+
 	return s.headSlot
 }

 // HeadRoot returns the root of the head of the chain.
 func (s *Service) HeadRoot() []byte {
-	s.canonicalRootsLock.RLock()
-	defer s.canonicalRootsLock.RUnlock()
+	s.headLock.RLock()
+	defer s.headLock.RUnlock()

 	root := s.canonicalRoots[s.headSlot]
 	if len(root) != 0 {
@@ -73,18 +76,24 @@ func (s *Service) HeadRoot() []byte {

 // HeadBlock returns the head block of the chain.
 func (s *Service) HeadBlock() *ethpb.BeaconBlock {
+	s.headLock.RLock()
+	defer s.headLock.RUnlock()
+
 	return proto.Clone(s.headBlock).(*ethpb.BeaconBlock)
 }

 // HeadState returns the head state of the chain.
 func (s *Service) HeadState() *pb.BeaconState {
+	s.headLock.RLock()
+	defer s.headLock.RUnlock()
+
 	return proto.Clone(s.headState).(*pb.BeaconState)
 }

 // CanonicalRoot returns the canonical root of a given slot.
 func (s *Service) CanonicalRoot(slot uint64) []byte {
-	s.canonicalRootsLock.RLock()
-	defer s.canonicalRootsLock.RUnlock()
+	s.headLock.RLock()
+	defer s.headLock.RUnlock()

 	return s.canonicalRoots[slot]
 }
--- a/beacon-chain/blockchain/chain_info_norace_test.go
+++ b/beacon-chain/blockchain/chain_info_norace_test.go
@@ -0,0 +1,77 @@
+package blockchain
+
+import (
+	"context"
+	"testing"
+
+	testDB "github.com/prysmaticlabs/prysm/beacon-chain/db/testing"
+	ethpb "github.com/prysmaticlabs/prysm/proto/eth/v1alpha1"
+)
+
+func TestHeadSlot_DataRace(t *testing.T) {
+	db := testDB.SetupDB(t)
+	defer testDB.TeardownDB(t, db)
+	s := &Service{
+		beaconDB:       db,
+		canonicalRoots: make(map[uint64][]byte),
+	}
+	go func() {
+		s.saveHead(
+			context.Background(),
+			&ethpb.BeaconBlock{Slot: 777},
+			[32]byte{},
+		)
+	}()
+	s.HeadSlot()
+}
+
+func TestHeadRoot_DataRace(t *testing.T) {
+	db := testDB.SetupDB(t)
+	defer testDB.TeardownDB(t, db)
+	s := &Service{
+		beaconDB:       db,
+		canonicalRoots: make(map[uint64][]byte),
+	}
+	go func() {
+		s.saveHead(
+			context.Background(),
+			&ethpb.BeaconBlock{Slot: 777},
+			[32]byte{},
+		)
+	}()
+	s.HeadRoot()
+}
+
+func TestHeadBlock_DataRace(t *testing.T) {
+	db := testDB.SetupDB(t)
+	defer testDB.TeardownDB(t, db)
+	s := &Service{
+		beaconDB:       db,
+		canonicalRoots: make(map[uint64][]byte),
+	}
+	go func() {
+		s.saveHead(
+			context.Background(),
+			&ethpb.BeaconBlock{Slot: 777},
+			[32]byte{},
+		)
+	}()
+	s.HeadBlock()
+}
+
+func TestHeadState_DataRace(t *testing.T) {
+	db := testDB.SetupDB(t)
+	defer testDB.TeardownDB(t, db)
+	s := &Service{
+		beaconDB:       db,
+		canonicalRoots: make(map[uint64][]byte),
+	}
+	go func() {
+		s.saveHead(
+			context.Background(),
+			&ethpb.BeaconBlock{Slot: 777},
+			[32]byte{},
+		)
+	}()
+	s.HeadState()
+}
--- a/beacon-chain/blockchain/receive_attestation.go
+++ b/beacon-chain/blockchain/receive_attestation.go
@@ -6,7 +6,7 @@ import (
 	"encoding/hex"

 	"github.com/pkg/errors"
-	"github.com/prysmaticlabs/go-ssz"
+	ssz "github.com/prysmaticlabs/go-ssz"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
 	ethpb "github.com/prysmaticlabs/prysm/proto/eth/v1alpha1"
 	"github.com/prysmaticlabs/prysm/shared/bytesutil"
@@ -89,6 +89,8 @@ func (s *Service) ReceiveAttestationNoPubsub(ctx context.Context, att *ethpb.Att

 	// Skip checking for competing attestation's target roots at epoch boundary.
 	if !helpers.IsEpochStart(attSlot) {
+		s.headLock.RLock()
+		defer s.headLock.RUnlock()
 		targetRoot, err := helpers.BlockRoot(s.headState, att.Data.Target.Epoch)
 		if err != nil {
 			return errors.Wrapf(err, "could not get target root for epoch %d", att.Data.Target.Epoch)
--- a/beacon-chain/blockchain/service.go
+++ b/beacon-chain/blockchain/service.go
@@ -12,7 +12,7 @@ import (
 	"time"

 	"github.com/pkg/errors"
-	"github.com/prysmaticlabs/go-ssz"
+	ssz "github.com/prysmaticlabs/go-ssz"
 	"github.com/prysmaticlabs/prysm/beacon-chain/blockchain/forkchoice"
 	"github.com/prysmaticlabs/prysm/beacon-chain/cache/depositcache"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/blocks"
@@ -61,7 +61,7 @@ type Service struct {
 	headBlock            *ethpb.BeaconBlock
 	headState            *pb.BeaconState
 	canonicalRoots       map[uint64][]byte
-	canonicalRootsLock   sync.RWMutex
+	headLock             sync.RWMutex
 }

 // Config options for the service.
@@ -206,11 +206,12 @@ func (s *Service) HeadUpdatedFeed() *event.Feed {

 // This gets called to update canonical root mapping.
 func (s *Service) saveHead(ctx context.Context, b *ethpb.BeaconBlock, r [32]byte) error {
+	s.headLock.Lock()
+	defer s.headLock.Unlock()
+
 	s.headSlot = b.Slot

-	s.canonicalRootsLock.Lock()
 	s.canonicalRoots[b.Slot] = r[:]
-	defer s.canonicalRootsLock.Unlock()

 	if err := s.beaconDB.SaveHeadBlockRoot(ctx, r); err != nil {
 		return errors.Wrap(err, "could not save head root in DB")
@@ -242,6 +243,9 @@ func (s *Service) saveGenesisValidators(ctx context.Context, state *pb.BeaconSta

 // This gets called when beacon chain is first initialized to save genesis data (state, block, and more) in db
 func (s *Service) saveGenesisData(ctx context.Context, genesisState *pb.BeaconState) error {
+	s.headLock.Lock()
+	defer s.headLock.Unlock()
+
 	stateRoot, err := ssz.HashTreeRoot(genesisState)
 	if err != nil {
 		return errors.Wrap(err, "could not tree hash genesis state")
@@ -286,6 +290,9 @@ func (s *Service) saveGenesisData(ctx context.Context, genesisState *pb.BeaconSt

 // This gets called to initialize chain info variables using the head stored in DB
 func (s *Service) initializeChainInfo(ctx context.Context) error {
+	s.headLock.Lock()
+	defer s.headLock.Unlock()
+
 	headBlock, err := s.beaconDB.HeadBlock(ctx)
 	if err != nil {
 		return errors.Wrap(err, "could not get head block in db")
--- a/beacon-chain/blockchain/service_norace_test.go
+++ b/beacon-chain/blockchain/service_norace_test.go
@@ -0,0 +1,37 @@
+package blockchain
+
+import (
+	"context"
+	"io/ioutil"
+	"testing"
+
+	testDB "github.com/prysmaticlabs/prysm/beacon-chain/db/testing"
+	ethpb "github.com/prysmaticlabs/prysm/proto/eth/v1alpha1"
+	"github.com/sirupsen/logrus"
+)
+
+func init() {
+	logrus.SetLevel(logrus.DebugLevel)
+	logrus.SetOutput(ioutil.Discard)
+}
+
+func TestChainService_SaveHead_DataRace(t *testing.T) {
+	db := testDB.SetupDB(t)
+	defer testDB.TeardownDB(t, db)
+	s := &Service{
+		beaconDB:       db,
+		canonicalRoots: make(map[uint64][]byte),
+	}
+	go func() {
+		s.saveHead(
+			context.Background(),
+			&ethpb.BeaconBlock{Slot: 777},
+			[32]byte{},
+		)
+	}()
+	s.saveHead(
+		context.Background(),
+		&ethpb.BeaconBlock{Slot: 888},
+		[32]byte{},
+	)
+}
--- a/beacon-chain/blockchain/service_test.go
+++ b/beacon-chain/blockchain/service_test.go
@@ -11,11 +11,11 @@ import (
 	"testing"
 	"time"

-	"github.com/ethereum/go-ethereum"
+	ethereum "github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	gethTypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/gogo/protobuf/proto"
-	"github.com/prysmaticlabs/go-ssz"
+	ssz "github.com/prysmaticlabs/go-ssz"
 	"github.com/prysmaticlabs/prysm/beacon-chain/cache/depositcache"
 	b "github.com/prysmaticlabs/prysm/beacon-chain/core/blocks"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"