From 29ef076f4b87e774fd1f4b17d12e376cd235f2f3 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 13 Sep 2021 11:16:39 +0300 Subject: [PATCH 1/2] network: fix race in TestTryInitStateSync Register peers properly. Fixes the following data race: ``` Read at 0x00c001184ac8 by goroutine 116: github.com/nspcc-dev/neo-go/pkg/network.(*localPeer).EnqueueHPPacket() /go/src/github.com/nspcc-dev/neo-go/pkg/network/helper_test.go:127 +0x1f2 github.com/nspcc-dev/neo-go/pkg/network.(*localPeer).EnqueuePacket() /go/src/github.com/nspcc-dev/neo-go/pkg/network/helper_test.go:114 +0xac github.com/nspcc-dev/neo-go/pkg/network.(*localPeer).EnqueueMessage() /go/src/github.com/nspcc-dev/neo-go/pkg/network/helper_test.go:111 +0xc1 github.com/nspcc-dev/neo-go/pkg/network.(*localPeer).SendPing() /go/src/github.com/nspcc-dev/neo-go/pkg/network/helper_test.go:159 +0x88 github.com/nspcc-dev/neo-go/pkg/network.(*Server).runProto() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server.go:446 +0x409 Previous write at 0x00c001184ac8 by goroutine 102: github.com/nspcc-dev/neo-go/pkg/network.newLocalPeer() /go/src/github.com/nspcc-dev/neo-go/pkg/network/helper_test.go:83 +0x476 github.com/nspcc-dev/neo-go/pkg/network.TestTryInitStateSync.func3() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:1064 +0x40f testing.tRunner() /usr/local/go/src/testing/testing.go:1123 +0x202 Goroutine 116 (running) created at: github.com/nspcc-dev/neo-go/pkg/network.(*Server).run() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server.go:358 +0x69 github.com/nspcc-dev/neo-go/pkg/network.(*Server).Start() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server.go:292 +0x488 github.com/nspcc-dev/neo-go/pkg/network.startWithChannel.func1() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:91 +0x44 Goroutine 102 (running) created at: testing.(*T).Run() /usr/local/go/src/testing/testing.go:1168 +0x5bb github.com/nspcc-dev/neo-go/pkg/network.TestTryInitStateSync() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:1056 +0xbb testing.tRunner() /usr/local/go/src/testing/testing.go:1123 +0x202 ``` --- pkg/network/server_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/network/server_test.go b/pkg/network/server_test.go index 69dc0d86b..cda5560d8 100644 --- a/pkg/network/server_test.go +++ b/pkg/network/server_test.go @@ -1059,14 +1059,15 @@ func TestTryInitStateSync(t *testing.T) { p := newLocalPeer(t, s) p.handshaked = true p.lastBlockIndex = h - s.peers[p] = true + s.register <- p } p := newLocalPeer(t, s) p.handshaked = false // one disconnected peer to check it won't be taken into attention p.lastBlockIndex = 5 - s.peers[p] = true - var expectedH uint32 = 8 // median peer + s.register <- p + require.Eventually(t, func() bool { return 7 == s.PeerCount() }, time.Second, time.Millisecond*10) + var expectedH uint32 = 8 // median peer ss := &fakechain.FakeStateSync{InitFunc: func(h uint32) error { if h != expectedH { return fmt.Errorf("invalid height: expected %d, got %d", expectedH, h) From 6357af0bb0944dcbd8eaf22beb97449a68c7e29d Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 13 Sep 2021 11:41:54 +0300 Subject: [PATCH 2/2] network: fix race in TestHandleGetMPTData Init server config before server start. Fixes the following data race: ``` WARNING: DATA RACE Write at 0x00c00032ef20 by goroutine 26: github.com/nspcc-dev/neo-go/pkg/network.TestHandleGetMPTData.func2() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:755 +0x10a testing.tRunner() /usr/local/go/src/testing/testing.go:1193 +0x202 Previous read at 0x00c00032ef20 by goroutine 24: github.com/nspcc-dev/neo-go/internal/fakechain.(*FakeChain).GetConfig() /go/src/github.com/nspcc-dev/neo-go/internal/fakechain/fakechain.go:167 +0x6f github.com/nspcc-dev/neo-go/pkg/network.(*Server).initStaleMemPools() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server.go:1433 +0x89 github.com/nspcc-dev/neo-go/pkg/network.(*Server).Start() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server.go:284 +0x288 github.com/nspcc-dev/neo-go/pkg/network.startWithChannel.func1() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:91 +0x44 Goroutine 26 (running) created at: testing.(*T).Run() /usr/local/go/src/testing/testing.go:1238 +0x5d7 github.com/nspcc-dev/neo-go/pkg/network.TestHandleGetMPTData() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:752 +0x8c testing.tRunner() /usr/local/go/src/testing/testing.go:1193 +0x202 Goroutine 24 (running) created at: github.com/nspcc-dev/neo-go/pkg/network.startWithChannel() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:90 +0x78 github.com/nspcc-dev/neo-go/pkg/network.startTestServer() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:384 +0xbd github.com/nspcc-dev/neo-go/pkg/network.TestHandleGetMPTData.func2() /go/src/github.com/nspcc-dev/neo-go/pkg/network/server_test.go:753 +0x55 testing.tRunner() /usr/local/go/src/testing/testing.go:1193 +0x202 ``` --- internal/fakechain/fakechain.go | 11 ++++++++++- pkg/network/helper_test.go | 7 ++++++- pkg/network/server_test.go | 22 +++++++++++++++------- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/internal/fakechain/fakechain.go b/internal/fakechain/fakechain.go index 426972c6b..6d67f0a6c 100644 --- a/internal/fakechain/fakechain.go +++ b/internal/fakechain/fakechain.go @@ -55,6 +55,15 @@ type FakeStateSync struct { // NewFakeChain returns new FakeChain structure. func NewFakeChain() *FakeChain { + return NewFakeChainWithCustomCfg(nil) +} + +// NewFakeChainWithCustomCfg returns new FakeChain structure with specified protocol configuration. +func NewFakeChainWithCustomCfg(protocolCfg func(c *config.ProtocolConfiguration)) *FakeChain { + cfg := config.ProtocolConfiguration{Magic: netmode.UnitTestNet, P2PNotaryRequestPayloadPoolSize: 10} + if protocolCfg != nil { + protocolCfg(&cfg) + } return &FakeChain{ Pool: mempool.New(10, 0, false), PoolTxF: func(*transaction.Transaction) error { return nil }, @@ -62,7 +71,7 @@ func NewFakeChain() *FakeChain { blocks: make(map[util.Uint256]*block.Block), hdrHashes: make(map[uint32]util.Uint256), txs: make(map[util.Uint256]*transaction.Transaction), - ProtocolConfiguration: config.ProtocolConfiguration{Magic: netmode.UnitTestNet, P2PNotaryRequestPayloadPoolSize: 10}, + ProtocolConfiguration: cfg, } } diff --git a/pkg/network/helper_test.go b/pkg/network/helper_test.go index 4a1779a78..23c399c91 100644 --- a/pkg/network/helper_test.go +++ b/pkg/network/helper_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/nspcc-dev/neo-go/internal/fakechain" + "github.com/nspcc-dev/neo-go/pkg/config" "github.com/nspcc-dev/neo-go/pkg/io" "github.com/nspcc-dev/neo-go/pkg/network/capability" "github.com/nspcc-dev/neo-go/pkg/network/payload" @@ -187,7 +188,11 @@ func (p *localPeer) CanProcessAddr() bool { } func newTestServer(t *testing.T, serverConfig ServerConfig) *Server { - s, err := newServerFromConstructors(serverConfig, fakechain.NewFakeChain(), zaptest.NewLogger(t), + return newTestServerWithCustomCfg(t, serverConfig, nil) +} + +func newTestServerWithCustomCfg(t *testing.T, serverConfig ServerConfig, protocolCfg func(*config.ProtocolConfiguration)) *Server { + s, err := newServerFromConstructors(serverConfig, fakechain.NewFakeChainWithCustomCfg(protocolCfg), zaptest.NewLogger(t), newFakeTransp, newFakeConsensus, newTestDiscovery) require.NoError(t, err) t.Cleanup(s.discovery.Close) diff --git a/pkg/network/server_test.go b/pkg/network/server_test.go index cda5560d8..e4c109c9f 100644 --- a/pkg/network/server_test.go +++ b/pkg/network/server_test.go @@ -379,8 +379,14 @@ func (s *Server) testHandleMessage(t *testing.T, p Peer, cmd CommandType, pl pay return s } -func startTestServer(t *testing.T) *Server { - s := newTestServer(t, ServerConfig{Port: 0, UserAgent: "/test/"}) +func startTestServer(t *testing.T, protocolCfg ...func(*config.ProtocolConfiguration)) *Server { + var s *Server + srvCfg := ServerConfig{Port: 0, UserAgent: "/test/"} + if protocolCfg != nil { + s = newTestServerWithCustomCfg(t, srvCfg, protocolCfg[0]) + } else { + s = newTestServer(t, srvCfg) + } ch := startWithChannel(s) t.Cleanup(func() { s.Shutdown() @@ -750,9 +756,10 @@ func TestHandleGetMPTData(t *testing.T) { }) t.Run("KeepOnlyLatestState on", func(t *testing.T) { - s := startTestServer(t) - s.chain.(*fakechain.FakeChain).P2PStateExchangeExtensions = true - s.chain.(*fakechain.FakeChain).KeepOnlyLatestState = true + s := startTestServer(t, func(c *config.ProtocolConfiguration) { + c.P2PStateExchangeExtensions = true + c.KeepOnlyLatestState = true + }) p := newLocalPeer(t, s) p.handshaked = true msg := NewMessage(CMDGetMPTData, &payload.MPTInventory{ @@ -762,8 +769,9 @@ func TestHandleGetMPTData(t *testing.T) { }) t.Run("good", func(t *testing.T) { - s := startTestServer(t) - s.chain.(*fakechain.FakeChain).P2PStateExchangeExtensions = true + s := startTestServer(t, func(c *config.ProtocolConfiguration) { + c.P2PStateExchangeExtensions = true + }) var recvResponse atomic.Bool r1 := random.Uint256() r2 := random.Uint256()