Skip to content

Commit dd29481

Browse files
fgimenezprestonvanloon
fgimenez
authored and
prestonvanloon
committed
Fix beacon-chain/sync Test Race (#390)
* fix beacon-chain/sync test race * address review comments * remove unused file * gazelle * disable race for client/contracts * address review comments * fix conflict * remove duplicated deps, embed * remove unneeded file
1 parent 2060d29 commit dd29481

12 files changed

+218
-168
lines changed

‎.travis-bazelrc

+1
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,4 @@ build --verbose_failures
1212
build --sandbox_debug
1313
build --test_output=errors
1414
build --flaky_test_attempts=5
15+
build --features=race

‎beacon-chain/sync/service_test.go

+22-17
Original file line numberDiff line numberDiff line change
@@ -785,22 +785,26 @@ func TestSavingBlocksInSync(t *testing.T) {
785785
t.Fatalf("unable to get hash of crystallized state: %v", err)
786786
}
787787

788-
block := &pb.BeaconBlock{
789-
MainChainRef: []byte{1, 2, 3},
790-
ParentHash: generichash,
791-
SlotNumber: uint64(20),
792-
CrystallizedStateHash: crystallizedStateHash[:],
793-
}
788+
getBlockResponseMsg := func(slotNumber uint64) p2p.Message {
789+
block := &pb.BeaconBlock{
790+
MainChainRef: []byte{1, 2, 3},
791+
ParentHash: generichash,
792+
SlotNumber: slotNumber,
793+
CrystallizedStateHash: crystallizedStateHash[:],
794+
}
794795

795-
blockResponse := &pb.BeaconBlockResponse{
796-
Block: block,
797-
}
796+
blockResponse := &pb.BeaconBlockResponse{
797+
Block: block,
798+
}
798799

799-
msg1 := p2p.Message{
800-
Peer: p2p.Peer{},
801-
Data: blockResponse,
800+
return p2p.Message{
801+
Peer: p2p.Peer{},
802+
Data: blockResponse,
803+
}
802804
}
803805

806+
msg1 := getBlockResponseMsg(0)
807+
804808
msg2 := p2p.Message{
805809
Peer: p2p.Peer{},
806810
Data: incorrectStateResponse,
@@ -818,24 +822,25 @@ func TestSavingBlocksInSync(t *testing.T) {
818822
ss.crystallizedStateBuf <- msg2
819823

820824
if crystallizedStateHash != ss.initialCrystallizedStateHash {
821-
t.Fatalf("Crystallized state hash not updated: %x", blockResponse.Block.CrystallizedStateHash)
825+
br := msg1.Data.(*pb.BeaconBlockResponse)
826+
t.Fatalf("Crystallized state hash not updated: %x", br.Block.CrystallizedStateHash)
822827
}
823828

824-
blockResponse.Block.SlotNumber = 30
825-
msg1.Data = blockResponse
829+
msg1 = getBlockResponseMsg(30)
826830
ss.blockBuf <- msg1
827831

828832
if stateResponse.CrystallizedState.GetLastFinalizedEpoch() != ss.currentSlotNumber {
829833
t.Fatalf("slotnumber saved when it was not supposed too: %v", stateResponse.CrystallizedState.GetLastFinalizedEpoch())
830834
}
831835

832-
blockResponse.Block.SlotNumber = 100
836+
msg1 = getBlockResponseMsg(100)
833837
ss.blockBuf <- msg1
834838

835839
ss.cancel()
836840
<-exitRoutine
837841

838-
if blockResponse.Block.GetSlotNumber() != ss.currentSlotNumber {
842+
br := msg1.Data.(*pb.BeaconBlockResponse)
843+
if br.Block.GetSlotNumber() != ss.currentSlotNumber {
839844
t.Fatalf("slotnumber not updated despite receiving a valid block: %v", ss.currentSlotNumber)
840845
}
841846

‎client/contracts/BUILD.bazel

+1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ go_test(
2323
name = "go_default_test",
2424
srcs = ["sharding_manager_test.go"],
2525
embed = [":go_default_library"],
26+
race = "off",
2627
deps = [
2728
"//client/params:go_default_library",
2829
"@com_github_ethereum_go_ethereum//accounts/abi/bind:go_default_library",

‎client/syncer/BUILD.bazel

+23
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,29 @@ go_test(
4545
"@com_github_ethereum_go_ethereum//core:go_default_library",
4646
"@com_github_ethereum_go_ethereum//core/types:go_default_library",
4747
"@com_github_ethereum_go_ethereum//crypto:go_default_library",
48+
"@com_github_sirupsen_logrus//hooks/test:go_default_library",
49+
],
50+
)
51+
52+
# by default gazelle tries to add all the test files to the first
53+
# go_test; we need to exclude the tests that we want to configure
54+
# in a particular way
55+
# gazelle:exclude service_norace_test.go
56+
57+
go_test(
58+
name = "go_norace_test",
59+
srcs = ["service_norace_test.go"],
60+
embed = [":go_default_library"],
61+
race = "off", # TODO(#377): fix issues with race detection testing.
62+
deps = [
63+
"//client/mainchain:go_default_library",
64+
"//client/params:go_default_library",
65+
"//client/types:go_default_library",
66+
"//proto/sharding/p2p/v1:go_default_library",
67+
"//shared/database:go_default_library",
68+
"//shared/p2p:go_default_library",
69+
"@com_github_ethereum_go_ethereum//common:go_default_library",
70+
"@com_github_ethereum_go_ethereum//core/types:go_default_library",
4871
"@com_github_sirupsen_logrus//:go_default_library",
4972
"@com_github_sirupsen_logrus//hooks/test:go_default_library",
5073
],

‎client/syncer/service_norace_test.go

+98
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package syncer
2+
3+
import (
4+
"fmt"
5+
"io/ioutil"
6+
"math/big"
7+
"testing"
8+
9+
"github.com/ethereum/go-ethereum/common"
10+
gethTypes "github.com/ethereum/go-ethereum/core/types"
11+
"github.com/prysmaticlabs/prysm/client/mainchain"
12+
"github.com/prysmaticlabs/prysm/client/params"
13+
"github.com/prysmaticlabs/prysm/client/types"
14+
pb "github.com/prysmaticlabs/prysm/proto/sharding/p2p/v1"
15+
"github.com/prysmaticlabs/prysm/shared/database"
16+
"github.com/prysmaticlabs/prysm/shared/p2p"
17+
"github.com/sirupsen/logrus"
18+
logTest "github.com/sirupsen/logrus/hooks/test"
19+
)
20+
21+
func init() {
22+
logrus.SetLevel(logrus.DebugLevel)
23+
logrus.SetOutput(ioutil.Discard)
24+
}
25+
26+
// This test checks the proper functioning of the handleCollationBodyRequests goroutine
27+
// by listening to the responseSent channel which occurs after successful
28+
// construction and sending of a response via p2p.
29+
func TestHandleCollationBodyRequests(t *testing.T) {
30+
hook := logTest.NewGlobal()
31+
32+
config := &database.DBConfig{Name: "", DataDir: "", InMemory: true}
33+
shardChainDB, err := database.NewDB(config)
34+
if err != nil {
35+
t.Fatalf("unable to setup db: %v", err)
36+
}
37+
server, err := p2p.NewServer()
38+
if err != nil {
39+
t.Fatalf("Unable to setup p2p server: %v", err)
40+
}
41+
42+
body := []byte{1, 2, 3, 4, 5}
43+
shardID := big.NewInt(0)
44+
chunkRoot := gethTypes.DeriveSha(types.Chunks(body))
45+
period := big.NewInt(0)
46+
proposerAddress := common.BytesToAddress([]byte{})
47+
48+
header := types.NewCollationHeader(shardID, &chunkRoot, period, &proposerAddress, [32]byte{})
49+
// Stores the collation into the inmemory kv store shardChainDB.
50+
collation := types.NewCollation(header, body, nil)
51+
52+
shard := types.NewShard(shardID, shardChainDB.DB())
53+
54+
if err := shard.SaveCollation(collation); err != nil {
55+
t.Fatalf("Could not store collation in shardChainDB: %v", err)
56+
}
57+
58+
syncer, err := NewSyncer(params.DefaultConfig(), &mainchain.SMCClient{}, server, shardChainDB, 0)
59+
if err != nil {
60+
t.Fatalf("Unable to setup syncer service: %v", err)
61+
}
62+
syncer.Start()
63+
syncer.collationBodyBuf = make(chan p2p.Message)
64+
65+
doneChan := make(chan struct{})
66+
exitRoutine := make(chan bool)
67+
68+
go func() {
69+
syncer.run(doneChan)
70+
<-exitRoutine
71+
}()
72+
73+
msg := p2p.Message{
74+
Peer: p2p.Peer{},
75+
Data: &pb.CollationBodyRequest{
76+
ChunkRoot: chunkRoot.Bytes(),
77+
ShardId: shardID.Uint64(),
78+
Period: period.Uint64(),
79+
ProposerAddress: proposerAddress.Bytes(),
80+
},
81+
}
82+
syncer.collationBodyBuf <- msg
83+
doneChan <- struct{}{}
84+
exitRoutine <- true
85+
86+
logMsg := hook.Entries[1].Message
87+
want := fmt.Sprintf("Received p2p request of type: %T", &pb.CollationBodyRequest{})
88+
if logMsg != want {
89+
t.Errorf("incorrect log, expected %s, got %s", want, logMsg)
90+
}
91+
92+
logMsg = hook.Entries[4].Message
93+
want = fmt.Sprintf("Responding to p2p collation request")
94+
if logMsg != want {
95+
t.Errorf("incorrect log, expected %s, got %s", want, logMsg)
96+
}
97+
hook.Reset()
98+
}

‎client/syncer/service_test.go

-87
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,18 @@
11
package syncer
22

33
import (
4-
"fmt"
5-
"io/ioutil"
6-
"math/big"
74
"testing"
85

9-
"github.com/ethereum/go-ethereum/common"
10-
gethTypes "github.com/ethereum/go-ethereum/core/types"
116
"github.com/prysmaticlabs/prysm/client/mainchain"
127
"github.com/prysmaticlabs/prysm/client/params"
13-
"github.com/prysmaticlabs/prysm/client/types"
14-
pb "github.com/prysmaticlabs/prysm/proto/sharding/p2p/v1"
158
"github.com/prysmaticlabs/prysm/shared"
169
"github.com/prysmaticlabs/prysm/shared/database"
1710
"github.com/prysmaticlabs/prysm/shared/p2p"
18-
"github.com/sirupsen/logrus"
1911
logTest "github.com/sirupsen/logrus/hooks/test"
2012
)
2113

2214
var _ = shared.Service(&Syncer{})
2315

24-
func init() {
25-
logrus.SetLevel(logrus.DebugLevel)
26-
logrus.SetOutput(ioutil.Discard)
27-
}
28-
2916
func TestStop(t *testing.T) {
3017
hook := logTest.NewGlobal()
3118

@@ -63,77 +50,3 @@ func TestStop(t *testing.T) {
6350
}
6451
hook.Reset()
6552
}
66-
67-
// This test checks the proper functioning of the handleCollationBodyRequests goroutine
68-
// by listening to the responseSent channel which occurs after successful
69-
// construction and sending of a response via p2p.
70-
func TestHandleCollationBodyRequests(t *testing.T) {
71-
hook := logTest.NewGlobal()
72-
73-
config := &database.DBConfig{Name: "", DataDir: "", InMemory: true}
74-
shardChainDB, err := database.NewDB(config)
75-
if err != nil {
76-
t.Fatalf("unable to setup db: %v", err)
77-
}
78-
server, err := p2p.NewServer()
79-
if err != nil {
80-
t.Fatalf("Unable to setup p2p server: %v", err)
81-
}
82-
83-
body := []byte{1, 2, 3, 4, 5}
84-
shardID := big.NewInt(0)
85-
chunkRoot := gethTypes.DeriveSha(types.Chunks(body))
86-
period := big.NewInt(0)
87-
proposerAddress := common.BytesToAddress([]byte{})
88-
89-
header := types.NewCollationHeader(shardID, &chunkRoot, period, &proposerAddress, [32]byte{})
90-
// Stores the collation into the inmemory kv store shardChainDB.
91-
collation := types.NewCollation(header, body, nil)
92-
93-
shard := types.NewShard(shardID, shardChainDB.DB())
94-
95-
if err := shard.SaveCollation(collation); err != nil {
96-
t.Fatalf("Could not store collation in shardChainDB: %v", err)
97-
}
98-
99-
syncer, err := NewSyncer(params.DefaultConfig(), &mainchain.SMCClient{}, server, shardChainDB, 0)
100-
if err != nil {
101-
t.Fatalf("Unable to setup syncer service: %v", err)
102-
}
103-
syncer.Start()
104-
syncer.collationBodyBuf = make(chan p2p.Message)
105-
106-
doneChan := make(chan struct{})
107-
exitRoutine := make(chan bool)
108-
109-
go func() {
110-
syncer.run(doneChan)
111-
<-exitRoutine
112-
}()
113-
114-
msg := p2p.Message{
115-
Peer: p2p.Peer{},
116-
Data: &pb.CollationBodyRequest{
117-
ChunkRoot: chunkRoot.Bytes(),
118-
ShardId: shardID.Uint64(),
119-
Period: period.Uint64(),
120-
ProposerAddress: proposerAddress.Bytes(),
121-
},
122-
}
123-
syncer.collationBodyBuf <- msg
124-
doneChan <- struct{}{}
125-
exitRoutine <- true
126-
127-
logMsg := hook.Entries[1].Message
128-
want := fmt.Sprintf("Received p2p request of type: %T", &pb.CollationBodyRequest{})
129-
if logMsg != want {
130-
t.Errorf("incorrect log, expected %s, got %s", want, logMsg)
131-
}
132-
133-
logMsg = hook.Entries[4].Message
134-
want = fmt.Sprintf("Responding to p2p collation request")
135-
if logMsg != want {
136-
t.Errorf("incorrect log, expected %s, got %s", want, logMsg)
137-
}
138-
hook.Reset()
139-
}

‎shared/p2p/BUILD.bazel

+15-9
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ go_library(
3333
go_test(
3434
name = "go_default_test",
3535
srcs = [
36-
"discovery_test.go",
3736
"feed_example_test.go",
3837
"feed_test.go",
3938
"options_test.go",
@@ -47,22 +46,29 @@ go_test(
4746
"@com_github_ethereum_go_ethereum//event:go_default_library",
4847
"@com_github_golang_protobuf//proto:go_default_library",
4948
"@com_github_libp2p_go_floodsub//:go_default_library",
50-
"@com_github_libp2p_go_libp2p//p2p/discovery:go_default_library",
5149
"@com_github_libp2p_go_libp2p//p2p/host/basic:go_default_library",
52-
"@com_github_libp2p_go_libp2p_peer//:go_default_library",
5350
"@com_github_libp2p_go_libp2p_swarm//testing:go_default_library",
5451
"@com_github_sirupsen_logrus//:go_default_library",
55-
"@com_github_sirupsen_logrus//hooks/test:go_default_library",
5652
],
5753
)
5854

5955
# by default gazelle tries to add all the test files to the first
60-
# go_test; we want to treat feed_concurrent_test.go differently
61-
# gazelle:exclude feed_concurrent_test.go
56+
# go_test; we need to exclude the tests that we want to configure
57+
# in a particular way
58+
# gazelle:exclude discovery_norace_test.go
59+
# gazelle:exclude service_norace_test.go
6260

6361
go_test(
64-
name = "go_feed_concurrent_write_test",
65-
srcs = ["feed_concurrent_test.go"],
62+
name = "go_norace_test",
63+
srcs = [
64+
"discovery_norace_test.go",
65+
"service_norace_test.go",
66+
],
6667
embed = [":go_default_library"],
67-
race = "on",
68+
race = "off", # TODO(#377): fix issues with race detection testing.
69+
deps = [
70+
"@com_github_libp2p_go_libp2p//p2p/host/basic:go_default_library",
71+
"@com_github_libp2p_go_libp2p_swarm//testing:go_default_library",
72+
"@com_github_sirupsen_logrus//hooks/test:go_default_library",
73+
],
6874
)

0 commit comments

Comments
 (0)