From 732527709ce21053f36441f49ed85ee42b77dbfb Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Fri, 2 Jul 2021 17:44:59 -0400 Subject: [PATCH 01/35] add code for runtime version changed fix --- dot/core/service.go | 59 ++++++++++++++++++++++++++ dot/core/service_test.go | 7 ++- dot/rpc/modules/api.go | 1 + dot/rpc/modules/api_mocks.go | 1 + dot/rpc/subscription/listeners.go | 40 ++++++++++------- dot/rpc/subscription/listeners_test.go | 40 +++++++++++++++++ dot/rpc/subscription/websocket.go | 9 ++++ 7 files changed, 140 insertions(+), 17 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 8ee7d402f0..b9a88cf45f 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -18,8 +18,10 @@ package core import ( "bytes" "context" + "errors" "fmt" "math/big" + "math/rand" "os" "sync" @@ -69,6 +71,9 @@ type Service struct { // Keystore keys *keystore.GlobalKeystore + + runtimeChangedLock sync.RWMutex + runtimeChanged map[byte]chan<- runtime.Version } // Config holds the configuration for the core Service. @@ -151,6 +156,7 @@ func NewService(cfg *Config) (*Service, error) { codeSubstitute: cfg.CodeSubstitutes, codeSubstitutedState: cfg.CodeSubstitutedState, digestHandler: cfg.DigestHandler, + runtimeChanged: make(map[byte]chan<- runtime.Version), } return srv, nil @@ -308,6 +314,11 @@ func (s *Service) handleRuntimeChanges(newState *rtstorage.TrieState) error { return err } + ver, err := s.rt.Version() + if err == nil { + go s.notifyRuntimeUpdated(ver) + } + s.codeHash = currCodeHash err = s.codeSubstitutedState.StoreCodeSubstitutedBlockHash(common.Hash{}) @@ -463,6 +474,25 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { return nil } +func (s *Service) notifyRuntimeUpdated(version runtime.Version) { + s.runtimeChangedLock.RLock() + defer s.runtimeChangedLock.RUnlock() + + if len(s.runtimeChanged) == 0 { + return + } + + logger.Info("notifying runtime updated chans...", "chans", s.runtimeChanged) + for _, ch := range s.runtimeChanged { + go func(ch chan<- runtime.Version) { + select { + case ch <- version: + default: + } + }(ch) + } +} + // maintainTransactionPool removes any transactions that were included in the new block, revalidates the transactions in the pool, // and moves them to the queue if valid. // See https://github.com/paritytech/substrate/blob/74804b5649eccfb83c90aec87bdca58e5d5c8789/client/transaction-pool/src/lib.rs#L545 @@ -585,3 +615,32 @@ func (s *Service) GetMetadata(bhash *common.Hash) ([]byte, error) { s.rt.SetContextStorage(ts) return s.rt.Metadata() } + +func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { + s.runtimeChangedLock.RLock() + + if len(s.runtimeChanged) == 256 { + return 0, errors.New("channel limit reached") + } + + var id byte + for { + id = generateID() + if s.runtimeChanged[id] == nil { + break + } + } + + s.runtimeChangedLock.RUnlock() + + s.runtimeChangedLock.Lock() + s.runtimeChanged[id] = ch + s.runtimeChangedLock.Unlock() + return id, nil +} + +func generateID() byte { + // skipcq: GSC-G404 + id := rand.Intn(256) //nolint + return byte(id) +} \ No newline at end of file diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 51edd90a1b..569435a168 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -17,6 +17,7 @@ package core import ( + "fmt" "io/ioutil" "math/big" "os" @@ -455,6 +456,8 @@ func TestService_GetMetadata(t *testing.T) { func TestService_HandleRuntimeChanges(t *testing.T) { s := NewTestService(t, nil) codeHashBefore := s.codeHash +upChan := make(chan runtime.Version) +s.RegisterRuntimeUpdatedChannel(upChan) testRuntime, err := ioutil.ReadFile(runtime.POLKADOT_RUNTIME_FP) require.NoError(t, err) @@ -463,7 +466,9 @@ func TestService_HandleRuntimeChanges(t *testing.T) { require.NoError(t, err) ts.Set(common.CodeKey, testRuntime) - err = s.handleRuntimeChanges(ts) +go s.handleRuntimeChanges(ts) +resUp := <- upChan +fmt.Printf("ForUP %v\n", resUp) require.NoError(t, err) codeHashAfter := s.codeHash require.NotEqualf(t, codeHashBefore, codeHashAfter, "expected different code hash after runtime update") diff --git a/dot/rpc/modules/api.go b/dot/rpc/modules/api.go index 5e459b8e83..de0fbb317b 100644 --- a/dot/rpc/modules/api.go +++ b/dot/rpc/modules/api.go @@ -72,6 +72,7 @@ type CoreAPI interface { GetRuntimeVersion(bhash *common.Hash) (runtime.Version, error) HandleSubmittedExtrinsic(types.Extrinsic) error GetMetadata(bhash *common.Hash) ([]byte, error) + RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) } // RPCAPI is the interface for methods related to RPC service diff --git a/dot/rpc/modules/api_mocks.go b/dot/rpc/modules/api_mocks.go index b5cefec6c9..2eadf41e71 100644 --- a/dot/rpc/modules/api_mocks.go +++ b/dot/rpc/modules/api_mocks.go @@ -46,5 +46,6 @@ func NewMockCoreAPI() *modulesmocks.MockCoreAPI { m.On("IsBlockProducer").Return(false) m.On("HandleSubmittedExtrinsic", mock.AnythingOfType("types.Extrinsic")).Return(nil) m.On("GetMetadata", mock.AnythingOfType("*common.Hash")).Return(nil, nil) + m.On("RegisterRuntimeUpdatedChannel", mock.AnythingOfType("chan<- runtime.Version")).Return(byte(0), nil) return m } diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index a6b5d97abc..ebfdaa7cf4 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -17,6 +17,7 @@ package subscription import ( "fmt" + "github.com/ChainSafe/gossamer/lib/runtime" "reflect" "github.com/ChainSafe/gossamer/dot/rpc/modules" @@ -191,27 +192,34 @@ func (l *ExtrinsicSubmitListener) Listen() { // RuntimeVersionListener to handle listening for Runtime Version type RuntimeVersionListener struct { - wsconn *WSConn + wsconn WSConnAPI subID uint + runtimeUpdate chan runtime.Version } // Listen implementation of Listen interface to listen for runtime version changes func (l *RuntimeVersionListener) Listen() { // This sends current runtime version once when subscription is created // TODO (ed) add logic to send updates when runtime version changes - rtVersion, err := l.wsconn.CoreAPI.GetRuntimeVersion(nil) - if err != nil { - return - } - ver := modules.StateRuntimeVersionResponse{} - - ver.SpecName = string(rtVersion.SpecName()) - ver.ImplName = string(rtVersion.ImplName()) - ver.AuthoringVersion = rtVersion.AuthoringVersion() - ver.SpecVersion = rtVersion.SpecVersion() - ver.ImplVersion = rtVersion.ImplVersion() - ver.TransactionVersion = rtVersion.TransactionVersion() - ver.Apis = modules.ConvertAPIs(rtVersion.APIItems()) - - l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) + // todo (ed) figure out how to send current rt version (get ref to CoreAPI) + //rtVersion, err := l.wsconn.CoreAPI.GetRuntimeVersion(nil) + //if err != nil { + // return + //} + //ver := modules.StateRuntimeVersionResponse{} + // + //ver.SpecName = string(rtVersion.SpecName()) + //ver.ImplName = string(rtVersion.ImplName()) + //ver.AuthoringVersion = rtVersion.AuthoringVersion() + //ver.SpecVersion = rtVersion.SpecVersion() + //ver.ImplVersion = rtVersion.ImplVersion() + //ver.TransactionVersion = rtVersion.TransactionVersion() + //ver.Apis = modules.ConvertAPIs(rtVersion.APIItems()) + + //l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) + go func() { + for info := range l.runtimeUpdate { + fmt.Printf("runtime info %v\n", info) + } + }() } diff --git a/dot/rpc/subscription/listeners_test.go b/dot/rpc/subscription/listeners_test.go index 13a20914bc..4d2459b6f0 100644 --- a/dot/rpc/subscription/listeners_test.go +++ b/dot/rpc/subscription/listeners_test.go @@ -17,7 +17,11 @@ package subscription import ( + "github.com/ChainSafe/gossamer/lib/runtime" + "github.com/ChainSafe/gossamer/lib/runtime/wasmer" + "io/ioutil" "math/big" + "path/filepath" "testing" "time" @@ -159,3 +163,39 @@ func TestExtrinsicSubmitListener_Listen(t *testing.T) { expectedFinalizedRespones := newSubscriptionResponse(AuthorExtrinsicUpdates, esl.subID, resFinalised) require.Equal(t, expectedFinalizedRespones, mockConnection.lastMessage) } + +func TestRuntimeChannelListener_Listen(t *testing.T) { + notifyChan := make(chan runtime.Version) + mockConnection := &MockWSConnAPI{} + rvl := RuntimeVersionListener{ + wsconn: mockConnection, + subID: 0, + runtimeUpdate: notifyChan, + } + + instance := wasmer.NewTestInstance(t, runtime.NODE_RUNTIME) + _, err := runtime.GetRuntimeBlob(runtime.POLKADOT_RUNTIME_FP, runtime.POLKADOT_RUNTIME_URL) + require.NoError(t, err) + fp, err := filepath.Abs(runtime.POLKADOT_RUNTIME_FP) + require.NoError(t, err) + code, err := ioutil.ReadFile(fp) + require.NoError(t, err) + version, err := instance.CheckRuntimeVersion(code) + require.NoError(t, err) + + //block := runtime.Version() + //block.Header.Number = big.NewInt(1) + // + //head, err := modules.HeaderToJSON(*block.Header) + //require.NoError(t, err) + // + expectedResposnse := newSubcriptionBaseResponseJSON() + //expectedResposnse.Method = "chain_newHead" + //expectedResposnse.Params.Result = head + + go rvl.Listen() + + notifyChan <- version + time.Sleep(time.Millisecond * 10) + require.Equal(t, expectedResposnse, mockConnection.lastMessage) +} \ No newline at end of file diff --git a/dot/rpc/subscription/websocket.go b/dot/rpc/subscription/websocket.go index ab1b367449..af79a8b532 100644 --- a/dot/rpc/subscription/websocket.go +++ b/dot/rpc/subscription/websocket.go @@ -20,6 +20,7 @@ import ( "bytes" "encoding/json" "fmt" + "github.com/ChainSafe/gossamer/lib/runtime" "io/ioutil" "math/big" "net/http" @@ -356,11 +357,19 @@ func (c *WSConn) initExtrinsicWatch(reqID float64, params interface{}) (uint, er func (c *WSConn) initRuntimeVersionListener(reqID float64) (uint, error) { rvl := &RuntimeVersionListener{ wsconn: c, + runtimeUpdate: make(chan runtime.Version), } if c.CoreAPI == nil { c.safeSendError(reqID, nil, "error CoreAPI not set") return 0, fmt.Errorf("error CoreAPI not set") } + + id, err := c.CoreAPI.RegisterRuntimeUpdatedChannel(rvl.runtimeUpdate) + if err != nil { + return 0, err + } + fmt.Printf("registered update channel %v\n", id) + c.qtyListeners++ rvl.subID = c.qtyListeners c.Subscriptions[rvl.subID] = rvl From 9394d38dc4fb8425e4bacba43fce001de6cdaeb4 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 7 Jul 2021 15:07:35 -0400 Subject: [PATCH 02/35] fix return type --- dot/rpc/subscription/websocket.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/rpc/subscription/websocket.go b/dot/rpc/subscription/websocket.go index f4f1de1610..f1a0784c7a 100644 --- a/dot/rpc/subscription/websocket.go +++ b/dot/rpc/subscription/websocket.go @@ -355,7 +355,7 @@ func (c *WSConn) initRuntimeVersionListener(reqID float64, _ interface{}) (Liste id, err := c.CoreAPI.RegisterRuntimeUpdatedChannel(rvl.runtimeUpdate) if err != nil { - return 0, err + return nil, err } fmt.Printf("registered update channel %v\n", id) From cfe9117f8ada1271aec6c958b855fb4981fe38d6 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 7 Jul 2021 16:00:25 -0400 Subject: [PATCH 03/35] update mock in Makefile --- Makefile | 2 +- dot/rpc/modules/mocks/core_api.go | 119 ------------------------------ 2 files changed, 1 insertion(+), 120 deletions(-) delete mode 100644 dot/rpc/modules/mocks/core_api.go diff --git a/Makefile b/Makefile index 4a4d211730..903e964205 100644 --- a/Makefile +++ b/Makefile @@ -137,7 +137,7 @@ endif @echo "> Generating mocks at $(path)" ifeq ($(INMOCKS),1) - cd $(path); $(GOPATH)/bin/mockery --name $(interface) --inpackage --keeptree --case underscore + cd $(path); $(GOPATH)/bin/mockery --name $(interface) --structname Mock$(interface) --keeptree --case underscore else $(GOPATH)/bin/mockery --srcpkg $(path) --name $(interface) --case underscore --inpackage endif diff --git a/dot/rpc/modules/mocks/core_api.go b/dot/rpc/modules/mocks/core_api.go deleted file mode 100644 index 380de34353..0000000000 --- a/dot/rpc/modules/mocks/core_api.go +++ /dev/null @@ -1,119 +0,0 @@ -// Code generated by mockery v2.8.0. DO NOT EDIT. - -package mocks - -import ( - common "github.com/ChainSafe/gossamer/lib/common" - crypto "github.com/ChainSafe/gossamer/lib/crypto" - - mock "github.com/stretchr/testify/mock" - - runtime "github.com/ChainSafe/gossamer/lib/runtime" - - types "github.com/ChainSafe/gossamer/dot/types" -) - -// MockCoreAPI is an autogenerated mock type for the CoreAPI type -type MockCoreAPI struct { - mock.Mock -} - -// GetMetadata provides a mock function with given fields: bhash -func (_m *MockCoreAPI) GetMetadata(bhash *common.Hash) ([]byte, error) { - ret := _m.Called(bhash) - - var r0 []byte - if rf, ok := ret.Get(0).(func(*common.Hash) []byte); ok { - r0 = rf(bhash) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).([]byte) - } - } - - var r1 error - if rf, ok := ret.Get(1).(func(*common.Hash) error); ok { - r1 = rf(bhash) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// GetRuntimeVersion provides a mock function with given fields: bhash -func (_m *MockCoreAPI) GetRuntimeVersion(bhash *common.Hash) (runtime.Version, error) { - ret := _m.Called(bhash) - - var r0 runtime.Version - if rf, ok := ret.Get(0).(func(*common.Hash) runtime.Version); ok { - r0 = rf(bhash) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(runtime.Version) - } - } - - var r1 error - if rf, ok := ret.Get(1).(func(*common.Hash) error); ok { - r1 = rf(bhash) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// HandleSubmittedExtrinsic provides a mock function with given fields: _a0 -func (_m *MockCoreAPI) HandleSubmittedExtrinsic(_a0 types.Extrinsic) error { - ret := _m.Called(_a0) - - var r0 error - if rf, ok := ret.Get(0).(func(types.Extrinsic) error); ok { - r0 = rf(_a0) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// HasKey provides a mock function with given fields: pubKeyStr, keyType -func (_m *MockCoreAPI) HasKey(pubKeyStr string, keyType string) (bool, error) { - ret := _m.Called(pubKeyStr, keyType) - - var r0 bool - if rf, ok := ret.Get(0).(func(string, string) bool); ok { - r0 = rf(pubKeyStr, keyType) - } else { - r0 = ret.Get(0).(bool) - } - - var r1 error - if rf, ok := ret.Get(1).(func(string, string) error); ok { - r1 = rf(pubKeyStr, keyType) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// InsertKey provides a mock function with given fields: kp -func (_m *MockCoreAPI) InsertKey(kp crypto.Keypair) { - _m.Called(kp) -} - -// IsBlockProducer provides a mock function with given fields: -func (_m *MockCoreAPI) IsBlockProducer() bool { - ret := _m.Called() - - var r0 bool - if rf, ok := ret.Get(0).(func() bool); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(bool) - } - - return r0 -} From 37c88789c720e346bbaf66b47ef7e8f89a079d35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ecl=C3=A9sio=20J=C3=BAnior?= Date: Thu, 8 Jul 2021 15:25:11 -0400 Subject: [PATCH 04/35] fix core_api mock --- dot/rpc/modules/mocks/core_api.go | 126 ++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 dot/rpc/modules/mocks/core_api.go diff --git a/dot/rpc/modules/mocks/core_api.go b/dot/rpc/modules/mocks/core_api.go new file mode 100644 index 0000000000..013430bed9 --- /dev/null +++ b/dot/rpc/modules/mocks/core_api.go @@ -0,0 +1,126 @@ +// Code generated by mockery v2.8.0. DO NOT EDIT. + +package mocks + +import ( + common "github.com/ChainSafe/gossamer/lib/common" + crypto "github.com/ChainSafe/gossamer/lib/crypto" + + mock "github.com/stretchr/testify/mock" + + runtime "github.com/ChainSafe/gossamer/lib/runtime" + + types "github.com/ChainSafe/gossamer/dot/types" +) + +// MockCoreAPI is an autogenerated mock type for the CoreAPI type +type MockCoreAPI struct { + mock.Mock +} + +// GetMetadata provides a mock function with given fields: bhash +func (_m *MockCoreAPI) GetMetadata(bhash *common.Hash) ([]byte, error) { + ret := _m.Called(bhash) + + var r0 []byte + if rf, ok := ret.Get(0).(func(*common.Hash) []byte); ok { + r0 = rf(bhash) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]byte) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(*common.Hash) error); ok { + r1 = rf(bhash) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetRuntimeVersion provides a mock function with given fields: bhash +func (_m *MockCoreAPI) GetRuntimeVersion(bhash *common.Hash) (runtime.Version, error) { + ret := _m.Called(bhash) + + var r0 runtime.Version + if rf, ok := ret.Get(0).(func(*common.Hash) runtime.Version); ok { + r0 = rf(bhash) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(runtime.Version) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(*common.Hash) error); ok { + r1 = rf(bhash) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// HandleSubmittedExtrinsic provides a mock function with given fields: _a0 +func (_m *MockCoreAPI) HandleSubmittedExtrinsic(_a0 types.Extrinsic) error { + ret := _m.Called(_a0) + + var r0 error + if rf, ok := ret.Get(0).(func(types.Extrinsic) error); ok { + r0 = rf(_a0) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// HasKey provides a mock function with given fields: pubKeyStr, keyType +func (_m *MockCoreAPI) HasKey(pubKeyStr string, keyType string) (bool, error) { + ret := _m.Called(pubKeyStr, keyType) + + var r0 bool + if rf, ok := ret.Get(0).(func(string, string) bool); ok { + r0 = rf(pubKeyStr, keyType) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(pubKeyStr, keyType) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// InsertKey provides a mock function with given fields: kp +func (_m *MockCoreAPI) InsertKey(kp crypto.Keypair) { + _m.Called(kp) +} + +// RegisterRuntimeUpdatedChannel provides a mock function with given fields: ch +func (_m *MockCoreAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { + ret := _m.Called(ch) + + var r0 byte + if rf, ok := ret.Get(0).(func(chan<- runtime.Version) byte); ok { + r0 = rf(ch) + } else { + r0 = ret.Get(0).(byte) + } + + var r1 error + if rf, ok := ret.Get(1).(func(chan<- runtime.Version) error); ok { + r1 = rf(ch) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} From feb5f4d9bc58bc4ad5370d01aebf14a87fe45a69 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 8 Jul 2021 18:33:06 -0400 Subject: [PATCH 05/35] implement notifications for runtime updates --- dot/core/service.go | 10 +- dot/core/service_test.go | 7 +- dot/rpc/modules/api_mocks.go | 15 ++- dot/rpc/subscription/listeners.go | 54 ++++++---- dot/rpc/subscription/listeners_test.go | 45 +++++--- dot/rpc/subscription/websocket.go | 17 ++- lib/runtime/mocks/version.go | 140 +++++++++++++++++++++++++ 7 files changed, 232 insertions(+), 56 deletions(-) create mode 100644 lib/runtime/mocks/version.go diff --git a/dot/core/service.go b/dot/core/service.go index 144b0a7432..428808055c 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -72,8 +72,8 @@ type Service struct { // Keystore keys *keystore.GlobalKeystore - runtimeChangedLock sync.RWMutex - runtimeChanged map[byte]chan<- runtime.Version + runtimeChangedLock sync.RWMutex + runtimeChanged map[byte]chan<- runtime.Version } // Config holds the configuration for the core Service. @@ -156,7 +156,7 @@ func NewService(cfg *Config) (*Service, error) { codeSubstitute: cfg.CodeSubstitutes, codeSubstitutedState: cfg.CodeSubstitutedState, digestHandler: cfg.DigestHandler, - runtimeChanged: make(map[byte]chan<- runtime.Version), + runtimeChanged: make(map[byte]chan<- runtime.Version), } return srv, nil @@ -610,7 +610,7 @@ func (s *Service) GetMetadata(bhash *common.Hash) ([]byte, error) { return s.rt.Metadata() } -func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { +func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { s.runtimeChangedLock.RLock() if len(s.runtimeChanged) == 256 { @@ -637,4 +637,4 @@ func generateID() byte { // skipcq: GSC-G404 id := rand.Intn(256) //nolint return byte(id) -} \ No newline at end of file +} diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 3496b6e6cb..e775d976d8 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -17,7 +17,6 @@ package core import ( - "fmt" "io/ioutil" "math/big" "os" @@ -456,8 +455,6 @@ func TestService_GetMetadata(t *testing.T) { func TestService_HandleRuntimeChanges(t *testing.T) { s := NewTestService(t, nil) codeHashBefore := s.codeHash -upChan := make(chan runtime.Version) -s.RegisterRuntimeUpdatedChannel(upChan) testRuntime, err := ioutil.ReadFile(runtime.POLKADOT_RUNTIME_FP) require.NoError(t, err) @@ -466,9 +463,7 @@ s.RegisterRuntimeUpdatedChannel(upChan) require.NoError(t, err) ts.Set(common.CodeKey, testRuntime) -go s.handleRuntimeChanges(ts) -resUp := <- upChan -fmt.Printf("ForUP %v\n", resUp) + err = s.handleRuntimeChanges(ts) require.NoError(t, err) codeHashAfter := s.codeHash require.NotEqualf(t, codeHashBefore, codeHashAfter, "expected different code hash after runtime update") diff --git a/dot/rpc/modules/api_mocks.go b/dot/rpc/modules/api_mocks.go index 2eadf41e71..ac4692245e 100644 --- a/dot/rpc/modules/api_mocks.go +++ b/dot/rpc/modules/api_mocks.go @@ -3,6 +3,7 @@ package modules import ( modulesmocks "github.com/ChainSafe/gossamer/dot/rpc/modules/mocks" "github.com/ChainSafe/gossamer/lib/common" + runtimemocks "github.com/ChainSafe/gossamer/lib/runtime/mocks" "github.com/stretchr/testify/mock" ) @@ -42,10 +43,22 @@ func NewMockCoreAPI() *modulesmocks.MockCoreAPI { m := new(modulesmocks.MockCoreAPI) m.On("InsertKey", mock.AnythingOfType("crypto.Keypair")) m.On("HasKey", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(false, nil) - m.On("GetRuntimeVersion", mock.AnythingOfType("*common.Hash")).Return(nil, nil) + m.On("GetRuntimeVersion", mock.AnythingOfType("*common.Hash")).Return(NewMockVersion(), nil) m.On("IsBlockProducer").Return(false) m.On("HandleSubmittedExtrinsic", mock.AnythingOfType("types.Extrinsic")).Return(nil) m.On("GetMetadata", mock.AnythingOfType("*common.Hash")).Return(nil, nil) m.On("RegisterRuntimeUpdatedChannel", mock.AnythingOfType("chan<- runtime.Version")).Return(byte(0), nil) return m } + +func NewMockVersion() *runtimemocks.MockVersion { + m := new(runtimemocks.MockVersion) + m.On("SpecName").Return([]byte(`mock-spec`)) + m.On("ImplName").Return(nil) + m.On("AuthoringVersion").Return(uint32(0)) + m.On("SpecVersion").Return(uint32(0)) + m.On("ImplVersion").Return(uint32(0)) + m.On("TransactionVersion").Return(uint32(0)) + m.On("APIItems").Return(nil) + return m +} diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index 79ba5f9548..4b69860123 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -18,13 +18,13 @@ package subscription import ( "context" "fmt" - "github.com/ChainSafe/gossamer/lib/runtime" "reflect" "github.com/ChainSafe/gossamer/dot/rpc/modules" "github.com/ChainSafe/gossamer/dot/state" "github.com/ChainSafe/gossamer/dot/types" "github.com/ChainSafe/gossamer/lib/common" + "github.com/ChainSafe/gossamer/lib/runtime" ) // Listener interface for functions that define Listener related functions @@ -259,38 +259,48 @@ func (l *ExtrinsicSubmitListener) Stop() { l.cancel() } // RuntimeVersionListener to handle listening for Runtime Version type RuntimeVersionListener struct { - wsconn WSConnAPI - subID uint + wsconn WSConnAPI + subID uint runtimeUpdate chan runtime.Version + coreAPI modules.CoreAPI } // Listen implementation of Listen interface to listen for runtime version changes func (l *RuntimeVersionListener) Listen() { // This sends current runtime version once when subscription is created - // TODO (ed) add logic to send updates when runtime version changes - // todo (ed) figure out how to send current rt version (get ref to CoreAPI) - //rtVersion, err := l.wsconn.CoreAPI.GetRuntimeVersion(nil) - //if err != nil { - // return - //} - //ver := modules.StateRuntimeVersionResponse{} - // - //ver.SpecName = string(rtVersion.SpecName()) - //ver.ImplName = string(rtVersion.ImplName()) - //ver.AuthoringVersion = rtVersion.AuthoringVersion() - //ver.SpecVersion = rtVersion.SpecVersion() - //ver.ImplVersion = rtVersion.ImplVersion() - //ver.TransactionVersion = rtVersion.TransactionVersion() - //ver.Apis = modules.ConvertAPIs(rtVersion.APIItems()) - - //l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) + rtVersion, err := l.coreAPI.GetRuntimeVersion(nil) + if err != nil { + return + } + ver := modules.StateRuntimeVersionResponse{} + ver.SpecName = string(rtVersion.SpecName()) + ver.ImplName = string(rtVersion.ImplName()) + ver.AuthoringVersion = rtVersion.AuthoringVersion() + ver.SpecVersion = rtVersion.SpecVersion() + ver.ImplVersion = rtVersion.ImplVersion() + ver.TransactionVersion = rtVersion.TransactionVersion() + ver.Apis = modules.ConvertAPIs(rtVersion.APIItems()) + + go l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) + + // listen for runtime updates go func() { for info := range l.runtimeUpdate { - fmt.Printf("runtime info %v\n", info) + ver := modules.StateRuntimeVersionResponse{} + + ver.SpecName = string(info.SpecName()) + ver.ImplName = string(info.ImplName()) + ver.AuthoringVersion = info.AuthoringVersion() + ver.SpecVersion = info.SpecVersion() + ver.ImplVersion = info.ImplVersion() + ver.TransactionVersion = info.TransactionVersion() + ver.Apis = modules.ConvertAPIs(info.APIItems()) + + l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) } }() } // Stop to runtimeVersionListener not implemented yet because the listener -// does not need to be stoped +// does not need to be stopped func (l *RuntimeVersionListener) Stop() {} diff --git a/dot/rpc/subscription/listeners_test.go b/dot/rpc/subscription/listeners_test.go index 4d2459b6f0..1f6f67e9a0 100644 --- a/dot/rpc/subscription/listeners_test.go +++ b/dot/rpc/subscription/listeners_test.go @@ -17,8 +17,6 @@ package subscription import ( - "github.com/ChainSafe/gossamer/lib/runtime" - "github.com/ChainSafe/gossamer/lib/runtime/wasmer" "io/ioutil" "math/big" "path/filepath" @@ -29,6 +27,8 @@ import ( "github.com/ChainSafe/gossamer/dot/state" "github.com/ChainSafe/gossamer/dot/types" "github.com/ChainSafe/gossamer/lib/common" + "github.com/ChainSafe/gossamer/lib/runtime" + "github.com/ChainSafe/gossamer/lib/runtime/wasmer" "github.com/stretchr/testify/require" ) @@ -171,8 +171,18 @@ func TestRuntimeChannelListener_Listen(t *testing.T) { wsconn: mockConnection, subID: 0, runtimeUpdate: notifyChan, + coreAPI: modules.NewMockCoreAPI(), } + expectedInitialVersion := modules.StateRuntimeVersionResponse{ + SpecName: "mock-spec", + Apis: modules.ConvertAPIs(nil), + } + + expectedInitialResponse := newSubcriptionBaseResponseJSON() + expectedInitialResponse.Method = "state_runtimeVersion" + expectedInitialResponse.Params.Result = expectedInitialVersion + instance := wasmer.NewTestInstance(t, runtime.NODE_RUNTIME) _, err := runtime.GetRuntimeBlob(runtime.POLKADOT_RUNTIME_FP, runtime.POLKADOT_RUNTIME_URL) require.NoError(t, err) @@ -183,19 +193,28 @@ func TestRuntimeChannelListener_Listen(t *testing.T) { version, err := instance.CheckRuntimeVersion(code) require.NoError(t, err) - //block := runtime.Version() - //block.Header.Number = big.NewInt(1) - // - //head, err := modules.HeaderToJSON(*block.Header) - //require.NoError(t, err) - // - expectedResposnse := newSubcriptionBaseResponseJSON() - //expectedResposnse.Method = "chain_newHead" - //expectedResposnse.Params.Result = head + expectedUpdatedVersion := modules.StateRuntimeVersionResponse{ + SpecName: "polkadot", + ImplName: "parity-polkadot", + AuthoringVersion: 0, + SpecVersion: 25, + ImplVersion: 0, + TransactionVersion: 5, + Apis: modules.ConvertAPIs(version.APIItems()), + } + + expectedUpdateResponse := newSubcriptionBaseResponseJSON() + expectedUpdateResponse.Method = "state_runtimeVersion" + expectedUpdateResponse.Params.Result = expectedUpdatedVersion go rvl.Listen() + //check initial response + time.Sleep(time.Millisecond * 10) + require.Equal(t, expectedInitialResponse, mockConnection.lastMessage) + + // check response after update notifyChan <- version time.Sleep(time.Millisecond * 10) - require.Equal(t, expectedResposnse, mockConnection.lastMessage) -} \ No newline at end of file + require.Equal(t, expectedUpdateResponse, mockConnection.lastMessage) +} diff --git a/dot/rpc/subscription/websocket.go b/dot/rpc/subscription/websocket.go index f1a0784c7a..d5bb82b84b 100644 --- a/dot/rpc/subscription/websocket.go +++ b/dot/rpc/subscription/websocket.go @@ -21,7 +21,6 @@ import ( "encoding/json" "errors" "fmt" - "github.com/ChainSafe/gossamer/lib/runtime" "io/ioutil" "math/big" "net/http" @@ -32,6 +31,7 @@ import ( "github.com/ChainSafe/gossamer/dot/state" "github.com/ChainSafe/gossamer/dot/types" "github.com/ChainSafe/gossamer/lib/common" + "github.com/ChainSafe/gossamer/lib/runtime" log "github.com/ChainSafe/log15" "github.com/gorilla/websocket" ) @@ -343,25 +343,24 @@ func (c *WSConn) initExtrinsicWatch(reqID float64, params interface{}) (Listener } func (c *WSConn) initRuntimeVersionListener(reqID float64, _ interface{}) (Listener, error) { - rvl := &RuntimeVersionListener{ - wsconn: c, - runtimeUpdate: make(chan runtime.Version), - } - if c.CoreAPI == nil { c.safeSendError(reqID, nil, "error CoreAPI not set") return nil, fmt.Errorf("error CoreAPI not set") } - id, err := c.CoreAPI.RegisterRuntimeUpdatedChannel(rvl.runtimeUpdate) + rvl := &RuntimeVersionListener{ + wsconn: c, + runtimeUpdate: make(chan runtime.Version), + coreAPI: c.CoreAPI, + } + + _, err := c.CoreAPI.RegisterRuntimeUpdatedChannel(rvl.runtimeUpdate) if err != nil { return nil, err } - fmt.Printf("registered update channel %v\n", id) c.mu.Lock() - c.qtyListeners++ rvl.subID = c.qtyListeners c.Subscriptions[rvl.subID] = rvl diff --git a/lib/runtime/mocks/version.go b/lib/runtime/mocks/version.go new file mode 100644 index 0000000000..9d2703dedc --- /dev/null +++ b/lib/runtime/mocks/version.go @@ -0,0 +1,140 @@ +// Code generated by mockery v2.8.0. DO NOT EDIT. + +package mocks + +import ( + runtime "github.com/ChainSafe/gossamer/lib/runtime" + mock "github.com/stretchr/testify/mock" +) + +// MockVersion is an autogenerated mock type for the Version type +type MockVersion struct { + mock.Mock +} + +// APIItems provides a mock function with given fields: +func (_m *MockVersion) APIItems() []*runtime.APIItem { + ret := _m.Called() + + var r0 []*runtime.APIItem + if rf, ok := ret.Get(0).(func() []*runtime.APIItem); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*runtime.APIItem) + } + } + + return r0 +} + +// AuthoringVersion provides a mock function with given fields: +func (_m *MockVersion) AuthoringVersion() uint32 { + ret := _m.Called() + + var r0 uint32 + if rf, ok := ret.Get(0).(func() uint32); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(uint32) + } + + return r0 +} + +// Encode provides a mock function with given fields: +func (_m *MockVersion) Encode() ([]byte, error) { + ret := _m.Called() + + var r0 []byte + if rf, ok := ret.Get(0).(func() []byte); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]byte) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ImplName provides a mock function with given fields: +func (_m *MockVersion) ImplName() []byte { + ret := _m.Called() + + var r0 []byte + if rf, ok := ret.Get(0).(func() []byte); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]byte) + } + } + + return r0 +} + +// ImplVersion provides a mock function with given fields: +func (_m *MockVersion) ImplVersion() uint32 { + ret := _m.Called() + + var r0 uint32 + if rf, ok := ret.Get(0).(func() uint32); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(uint32) + } + + return r0 +} + +// SpecName provides a mock function with given fields: +func (_m *MockVersion) SpecName() []byte { + ret := _m.Called() + + var r0 []byte + if rf, ok := ret.Get(0).(func() []byte); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]byte) + } + } + + return r0 +} + +// SpecVersion provides a mock function with given fields: +func (_m *MockVersion) SpecVersion() uint32 { + ret := _m.Called() + + var r0 uint32 + if rf, ok := ret.Get(0).(func() uint32); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(uint32) + } + + return r0 +} + +// TransactionVersion provides a mock function with given fields: +func (_m *MockVersion) TransactionVersion() uint32 { + ret := _m.Called() + + var r0 uint32 + if rf, ok := ret.Get(0).(func() uint32); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(uint32) + } + + return r0 +} From a31474d861c45be31b36f9fd8dba175f905b22a3 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 8 Jul 2021 18:47:25 -0400 Subject: [PATCH 06/35] lint --- dot/core/service.go | 1 + dot/rpc/modules/api_mocks.go | 1 + 2 files changed, 2 insertions(+) diff --git a/dot/core/service.go b/dot/core/service.go index 428808055c..b8c770e1b2 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -610,6 +610,7 @@ func (s *Service) GetMetadata(bhash *common.Hash) ([]byte, error) { return s.rt.Metadata() } +// RegisterRuntimeUpdatedChannel function to register chan that is notified when runtime version changes func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { s.runtimeChangedLock.RLock() diff --git a/dot/rpc/modules/api_mocks.go b/dot/rpc/modules/api_mocks.go index ac4692245e..dbd83a6828 100644 --- a/dot/rpc/modules/api_mocks.go +++ b/dot/rpc/modules/api_mocks.go @@ -51,6 +51,7 @@ func NewMockCoreAPI() *modulesmocks.MockCoreAPI { return m } +// NewMockVersion creates and returns an runtime Version interface mock func NewMockVersion() *runtimemocks.MockVersion { m := new(runtimemocks.MockVersion) m.On("SpecName").Return([]byte(`mock-spec`)) From 8c161767f881921423371f061e2300c0bfe7461d Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 14 Jul 2021 16:08:15 -0400 Subject: [PATCH 07/35] implement unregister runtime version listener --- dot/core/service.go | 17 ++++++++++++----- dot/rpc/modules/api.go | 1 + dot/rpc/subscription/listeners.go | 9 +++++++++ dot/rpc/subscription/websocket.go | 16 +++++++++++++++- 4 files changed, 37 insertions(+), 6 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index b8c770e1b2..0bfb8a60c8 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -612,7 +612,8 @@ func (s *Service) GetMetadata(bhash *common.Hash) ([]byte, error) { // RegisterRuntimeUpdatedChannel function to register chan that is notified when runtime version changes func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { - s.runtimeChangedLock.RLock() + s.runtimeChangedLock.Lock() + defer s.runtimeChangedLock.Unlock() if len(s.runtimeChanged) == 256 { return 0, errors.New("channel limit reached") @@ -626,14 +627,20 @@ func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte } } - s.runtimeChangedLock.RUnlock() - - s.runtimeChangedLock.Lock() s.runtimeChanged[id] = ch - s.runtimeChangedLock.Unlock() return id, nil } +func (s *Service) UnregisterRuntimeUpdatedChannel (id byte) bool { + ch := s.runtimeChanged[id] + if ch != nil { + close(ch) + s.runtimeChanged[id] = nil + return true + } + return false +} + func generateID() byte { // skipcq: GSC-G404 id := rand.Intn(256) //nolint diff --git a/dot/rpc/modules/api.go b/dot/rpc/modules/api.go index 294855283d..e2123c5b90 100644 --- a/dot/rpc/modules/api.go +++ b/dot/rpc/modules/api.go @@ -75,6 +75,7 @@ type CoreAPI interface { HandleSubmittedExtrinsic(types.Extrinsic) error GetMetadata(bhash *common.Hash) ([]byte, error) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) + UnregisterRuntimeUpdatedChannel(id byte) bool } // RPCAPI is the interface for methods related to RPC service diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index 4b69860123..f10fbf8a0a 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -262,9 +262,14 @@ type RuntimeVersionListener struct { wsconn WSConnAPI subID uint runtimeUpdate chan runtime.Version + channelID byte coreAPI modules.CoreAPI } +type VersionListener interface { + GetID() byte +} + // Listen implementation of Listen interface to listen for runtime version changes func (l *RuntimeVersionListener) Listen() { // This sends current runtime version once when subscription is created @@ -304,3 +309,7 @@ func (l *RuntimeVersionListener) Listen() { // Stop to runtimeVersionListener not implemented yet because the listener // does not need to be stopped func (l *RuntimeVersionListener) Stop() {} + +func (l *RuntimeVersionListener) GetID() byte { + return l.channelID +} diff --git a/dot/rpc/subscription/websocket.go b/dot/rpc/subscription/websocket.go index ab9a813c7a..4da5001db7 100644 --- a/dot/rpc/subscription/websocket.go +++ b/dot/rpc/subscription/websocket.go @@ -357,13 +357,14 @@ func (c *WSConn) initRuntimeVersionListener(reqID float64, _ interface{}) (Liste coreAPI: c.CoreAPI, } - _, err := c.CoreAPI.RegisterRuntimeUpdatedChannel(rvl.runtimeUpdate) + chanID, err := c.CoreAPI.RegisterRuntimeUpdatedChannel(rvl.runtimeUpdate) if err != nil { return nil, err } c.mu.Lock() + rvl.channelID = chanID c.qtyListeners++ rvl.subID = c.qtyListeners c.Subscriptions[rvl.subID] = rvl @@ -375,6 +376,19 @@ func (c *WSConn) initRuntimeVersionListener(reqID float64, _ interface{}) (Liste return rvl, nil } +func (c *WSConn) unsubscribeRuntimeVersionListener(reqID float64, l Listener, _ interface{}) { + observer, ok := l.(VersionListener) + if !ok { + initRes := newBooleanResponseJSON(false, reqID) + c.safeSend(initRes) + return + } + id := observer.GetID() + + res := c.CoreAPI.UnregisterRuntimeUpdatedChannel(id) + c.safeSend(newBooleanResponseJSON(res, reqID)) +} + func (c *WSConn) safeSend(msg interface{}) { c.mu.Lock() defer c.mu.Unlock() From de0c40bada3972ed4f44a7543de9663ce96d7e6b Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 15 Jul 2021 10:30:42 -0400 Subject: [PATCH 08/35] make mocks and lint --- dot/core/service.go | 2 +- dot/rpc/modules/mocks/core_api.go | 14 ++++++++++++++ dot/rpc/subscription/listeners.go | 2 +- dot/rpc/subscription/subscription.go | 2 ++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 0bfb8a60c8..d1a2702889 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -631,7 +631,7 @@ func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte return id, nil } -func (s *Service) UnregisterRuntimeUpdatedChannel (id byte) bool { +func (s *Service) UnregisterRuntimeUpdatedChannel(id byte) bool { ch := s.runtimeChanged[id] if ch != nil { close(ch) diff --git a/dot/rpc/modules/mocks/core_api.go b/dot/rpc/modules/mocks/core_api.go index 013430bed9..188b989273 100644 --- a/dot/rpc/modules/mocks/core_api.go +++ b/dot/rpc/modules/mocks/core_api.go @@ -124,3 +124,17 @@ func (_m *MockCoreAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) return r0, r1 } + +// UnregisterRuntimeUpdatedChannel provides a mock function with given fields: id +func (_m *MockCoreAPI) UnregisterRuntimeUpdatedChannel(id byte) bool { + ret := _m.Called(id) + + var r0 bool + if rf, ok := ret.Get(0).(func(byte) bool); ok { + r0 = rf(id) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index f10fbf8a0a..782b3a5a5b 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -262,7 +262,7 @@ type RuntimeVersionListener struct { wsconn WSConnAPI subID uint runtimeUpdate chan runtime.Version - channelID byte + channelID byte coreAPI modules.CoreAPI } diff --git a/dot/rpc/subscription/subscription.go b/dot/rpc/subscription/subscription.go index 93413fec65..f174add663 100644 --- a/dot/rpc/subscription/subscription.go +++ b/dot/rpc/subscription/subscription.go @@ -45,6 +45,8 @@ func (c *WSConn) getUnsubListener(method string, params interface{}) (unsubListe switch method { case "state_unsubscribeStorage": unsub = c.unsubscribeStorageListener + case "state_unsubscribeRuntimeVersion": + unsub = c.unsubscribeRuntimeVersionListener default: return nil, nil, errCannotFindUnsubsriber } From 551234842f0b8ab1fb69cdf3ade0983949fffeb7 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 15 Jul 2021 10:42:12 -0400 Subject: [PATCH 09/35] refactor runtime subscription name, change error handeling --- dot/core/service.go | 55 +++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index d1a2702889..73db5be9dc 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -72,8 +72,8 @@ type Service struct { // Keystore keys *keystore.GlobalKeystore - runtimeChangedLock sync.RWMutex - runtimeChanged map[byte]chan<- runtime.Version + runtimeUpdateSubscriptionsLock sync.RWMutex + runtimeUpdateSubscriptions map[byte]chan<- runtime.Version } // Config holds the configuration for the core Service. @@ -146,17 +146,17 @@ func NewService(cfg *Config) (*Service, error) { cancel: cancel, rt: cfg.Runtime, codeHash: codeHash, - keys: cfg.Keystore, - blockState: cfg.BlockState, - epochState: cfg.EpochState, - storageState: cfg.StorageState, - transactionState: cfg.TransactionState, - net: cfg.Network, - blockAddCh: blockAddCh, - codeSubstitute: cfg.CodeSubstitutes, - codeSubstitutedState: cfg.CodeSubstitutedState, - digestHandler: cfg.DigestHandler, - runtimeChanged: make(map[byte]chan<- runtime.Version), + keys: cfg.Keystore, + blockState: cfg.BlockState, + epochState: cfg.EpochState, + storageState: cfg.StorageState, + transactionState: cfg.TransactionState, + net: cfg.Network, + blockAddCh: blockAddCh, + codeSubstitute: cfg.CodeSubstitutes, + codeSubstitutedState: cfg.CodeSubstitutedState, + digestHandler: cfg.DigestHandler, + runtimeUpdateSubscriptions: make(map[byte]chan<- runtime.Version), } return srv, nil @@ -315,9 +315,10 @@ func (s *Service) handleRuntimeChanges(newState *rtstorage.TrieState) error { } ver, err := s.rt.Version() - if err == nil { - go s.notifyRuntimeUpdated(ver) + if err != nil { + return err } + go s.notifyRuntimeUpdated(ver) s.codeHash = currCodeHash @@ -474,15 +475,15 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { } func (s *Service) notifyRuntimeUpdated(version runtime.Version) { - s.runtimeChangedLock.RLock() - defer s.runtimeChangedLock.RUnlock() + s.runtimeUpdateSubscriptionsLock.RLock() + defer s.runtimeUpdateSubscriptionsLock.RUnlock() - if len(s.runtimeChanged) == 0 { + if len(s.runtimeUpdateSubscriptions) == 0 { return } - logger.Info("notifying runtime updated chans...", "chans", s.runtimeChanged) - for _, ch := range s.runtimeChanged { + logger.Info("notifying runtime updated chans...", "chans", s.runtimeUpdateSubscriptions) + for _, ch := range s.runtimeUpdateSubscriptions { go func(ch chan<- runtime.Version) { select { case ch <- version: @@ -612,30 +613,30 @@ func (s *Service) GetMetadata(bhash *common.Hash) ([]byte, error) { // RegisterRuntimeUpdatedChannel function to register chan that is notified when runtime version changes func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { - s.runtimeChangedLock.Lock() - defer s.runtimeChangedLock.Unlock() + s.runtimeUpdateSubscriptionsLock.Lock() + defer s.runtimeUpdateSubscriptionsLock.Unlock() - if len(s.runtimeChanged) == 256 { + if len(s.runtimeUpdateSubscriptions) == 256 { return 0, errors.New("channel limit reached") } var id byte for { id = generateID() - if s.runtimeChanged[id] == nil { + if s.runtimeUpdateSubscriptions[id] == nil { break } } - s.runtimeChanged[id] = ch + s.runtimeUpdateSubscriptions[id] = ch return id, nil } func (s *Service) UnregisterRuntimeUpdatedChannel(id byte) bool { - ch := s.runtimeChanged[id] + ch := s.runtimeUpdateSubscriptions[id] if ch != nil { close(ch) - s.runtimeChanged[id] = nil + s.runtimeUpdateSubscriptions[id] = nil return true } return false From 09c9b2e8eae107f84feffaa64f839754ac100cc2 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 15 Jul 2021 10:46:31 -0400 Subject: [PATCH 10/35] remove unneeded select case --- dot/core/service.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 73db5be9dc..d950691503 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -142,10 +142,10 @@ func NewService(cfg *Config) (*Service, error) { ctx, cancel := context.WithCancel(context.Background()) srv := &Service{ - ctx: ctx, - cancel: cancel, - rt: cfg.Runtime, - codeHash: codeHash, + ctx: ctx, + cancel: cancel, + rt: cfg.Runtime, + codeHash: codeHash, keys: cfg.Keystore, blockState: cfg.BlockState, epochState: cfg.EpochState, @@ -485,10 +485,7 @@ func (s *Service) notifyRuntimeUpdated(version runtime.Version) { logger.Info("notifying runtime updated chans...", "chans", s.runtimeUpdateSubscriptions) for _, ch := range s.runtimeUpdateSubscriptions { go func(ch chan<- runtime.Version) { - select { - case ch <- version: - default: - } + ch <- version }(ch) } } From cf0e4773e744fc89eb3a96e0142f34243ce1d282 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 15 Jul 2021 10:55:38 -0400 Subject: [PATCH 11/35] lint, refactor GetID to GetChannelID --- dot/core/service.go | 1 + dot/rpc/subscription/listeners.go | 6 ++++-- dot/rpc/subscription/websocket.go | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index d950691503..ae223d1ed3 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -629,6 +629,7 @@ func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte return id, nil } +// UnregisterRuntimeUpdatedChannel function to unregister runtime updated channel func (s *Service) UnregisterRuntimeUpdatedChannel(id byte) bool { ch := s.runtimeUpdateSubscriptions[id] if ch != nil { diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index 782b3a5a5b..5d15f66bba 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -266,8 +266,9 @@ type RuntimeVersionListener struct { coreAPI modules.CoreAPI } +// VersionListener interface defining methods that version listener must implement type VersionListener interface { - GetID() byte + GetChannelID() byte } // Listen implementation of Listen interface to listen for runtime version changes @@ -310,6 +311,7 @@ func (l *RuntimeVersionListener) Listen() { // does not need to be stopped func (l *RuntimeVersionListener) Stop() {} -func (l *RuntimeVersionListener) GetID() byte { +// +func (l *RuntimeVersionListener) GetChannelID() byte { return l.channelID } diff --git a/dot/rpc/subscription/websocket.go b/dot/rpc/subscription/websocket.go index 4da5001db7..11ce05e3a1 100644 --- a/dot/rpc/subscription/websocket.go +++ b/dot/rpc/subscription/websocket.go @@ -383,7 +383,7 @@ func (c *WSConn) unsubscribeRuntimeVersionListener(reqID float64, l Listener, _ c.safeSend(initRes) return } - id := observer.GetID() + id := observer.GetChannelID() res := c.CoreAPI.UnregisterRuntimeUpdatedChannel(id) c.safeSend(newBooleanResponseJSON(res, reqID)) From 6c3ea54d25900866cf4753868a65c3a4a09585ae Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 15 Jul 2021 10:58:16 -0400 Subject: [PATCH 12/35] lint --- dot/rpc/subscription/listeners.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index 5d15f66bba..426ae75947 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -311,7 +311,7 @@ func (l *RuntimeVersionListener) Listen() { // does not need to be stopped func (l *RuntimeVersionListener) Stop() {} -// +// GetChannelID function that returns listener's channel ID func (l *RuntimeVersionListener) GetChannelID() byte { return l.channelID } From 8031b8218023963c1346e172562cdd988e5ddef5 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 21 Jul 2021 10:15:24 -0400 Subject: [PATCH 13/35] add sync.WaitGroup to notify Runtime Updated --- dot/core/service.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 382e26e314..46215ebd46 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -483,11 +483,15 @@ func (s *Service) notifyRuntimeUpdated(version runtime.Version) { } logger.Info("notifying runtime updated chans...", "chans", s.runtimeUpdateSubscriptions) + var wg sync.WaitGroup + wg.Add(len(s.runtimeUpdateSubscriptions)) for _, ch := range s.runtimeUpdateSubscriptions { - go func(ch chan<- runtime.Version) { + go func(ch chan<- runtime.Version, wg *sync.WaitGroup) { + defer wg.Done() ch <- version - }(ch) + }(ch, &wg) } + wg.Wait() } // maintainTransactionPool removes any transactions that were included in the new block, revalidates the transactions in the pool, From d6f6cea0d1a9ab823b0a52af399cfedf8316d147 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 21 Jul 2021 13:29:18 -0400 Subject: [PATCH 14/35] add test for Register UnRegister Runtime Update Channel --- dot/core/service_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/dot/core/service_test.go b/dot/core/service_test.go index c168d5f06d..c67793cc72 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -506,3 +506,14 @@ func TestService_HandleRuntimeChangesAfterCodeSubstitutes(t *testing.T) { require.NoError(t, err) require.NotEqualf(t, codeHashBefore, s.codeHash, "expected different code hash after runtime update") // codeHash should change after runtime change } + +func TestService_RegisterUnRegisterRuntimeUpdatedChannel(t *testing.T) { + s := NewTestService(t, nil) + ch := make(chan<- runtime.Version) + chID, err := s.RegisterRuntimeUpdatedChannel(ch) + require.NoError(t, err) + require.NotNil(t, chID) + + res := s.UnregisterRuntimeUpdatedChannel(chID) + require.True(t, res) +} From 2f6c7e2675ce04554055e253a5f6f2a84113c820 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 21 Jul 2021 13:41:48 -0400 Subject: [PATCH 15/35] add check for id --- dot/core/service.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 46215ebd46..beff3f8729 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -486,10 +486,10 @@ func (s *Service) notifyRuntimeUpdated(version runtime.Version) { var wg sync.WaitGroup wg.Add(len(s.runtimeUpdateSubscriptions)) for _, ch := range s.runtimeUpdateSubscriptions { - go func(ch chan<- runtime.Version, wg *sync.WaitGroup) { + go func(ch chan<- runtime.Version) { defer wg.Done() ch <- version - }(ch, &wg) + }(ch) } wg.Wait() } @@ -635,8 +635,8 @@ func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte // UnregisterRuntimeUpdatedChannel function to unregister runtime updated channel func (s *Service) UnregisterRuntimeUpdatedChannel(id byte) bool { - ch := s.runtimeUpdateSubscriptions[id] - if ch != nil { + ch, ok := s.runtimeUpdateSubscriptions[id] + if ok { close(ch) s.runtimeUpdateSubscriptions[id] = nil return true From 98fb9d9305bdf366aebb931c370c6da634fd9106 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 21 Jul 2021 14:10:46 -0400 Subject: [PATCH 16/35] add register panic test --- dot/core/service_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dot/core/service_test.go b/dot/core/service_test.go index c67793cc72..8b1d2c6ab1 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -17,6 +17,7 @@ package core import ( + "github.com/ChainSafe/gossamer/dot/rpc/modules" "io/ioutil" "math/big" "os" @@ -517,3 +518,15 @@ func TestService_RegisterUnRegisterRuntimeUpdatedChannel(t *testing.T) { res := s.UnregisterRuntimeUpdatedChannel(chID) require.True(t, res) } + +func TestService_RegisterUnRegisterPanic(t *testing.T) { + s := NewTestService(t, nil) + testVer := modules.NewMockVersion() + for i := 0; i < 10; i++ { + go s.notifyRuntimeUpdated(testVer) + ch := make (chan <- runtime.Version) + chID, err := s.RegisterRuntimeUpdatedChannel(ch) + require.NoError(t, err) + s.UnregisterRuntimeUpdatedChannel(chID) + } +} \ No newline at end of file From 42f436ebed94e67f4db314eda1b19d15edc9a58e Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 21 Jul 2021 15:36:45 -0400 Subject: [PATCH 17/35] add test to produce panic --- dot/core/service_test.go | 45 ++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 8b1d2c6ab1..7a2d54e75e 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -17,7 +17,6 @@ package core import ( - "github.com/ChainSafe/gossamer/dot/rpc/modules" "io/ioutil" "math/big" "os" @@ -25,6 +24,7 @@ import ( "testing" "time" + coremocks "github.com/ChainSafe/gossamer/dot/core/mocks" "github.com/ChainSafe/gossamer/dot/network" "github.com/ChainSafe/gossamer/dot/state" "github.com/ChainSafe/gossamer/dot/sync" @@ -33,13 +33,12 @@ import ( "github.com/ChainSafe/gossamer/lib/keystore" "github.com/ChainSafe/gossamer/lib/runtime" "github.com/ChainSafe/gossamer/lib/runtime/extrinsic" + runtimemocks "github.com/ChainSafe/gossamer/lib/runtime/mocks" "github.com/ChainSafe/gossamer/lib/runtime/wasmer" "github.com/ChainSafe/gossamer/lib/transaction" "github.com/ChainSafe/gossamer/lib/utils" log "github.com/ChainSafe/log15" "github.com/stretchr/testify/require" - - coremocks "github.com/ChainSafe/gossamer/dot/core/mocks" ) func addTestBlocksToState(t *testing.T, depth int, blockState BlockState) { @@ -520,13 +519,37 @@ func TestService_RegisterUnRegisterRuntimeUpdatedChannel(t *testing.T) { } func TestService_RegisterUnRegisterPanic(t *testing.T) { + t.Skip() // TODO, this test is skipped because it fails with panic, todo determine reason and fix s := NewTestService(t, nil) - testVer := modules.NewMockVersion() - for i := 0; i < 10; i++ { - go s.notifyRuntimeUpdated(testVer) - ch := make (chan <- runtime.Version) - chID, err := s.RegisterRuntimeUpdatedChannel(ch) - require.NoError(t, err) - s.UnregisterRuntimeUpdatedChannel(chID) + + go func() { + for i := 0; i < 100; i++ { + testVer := NewMockVersion(uint32(i)) + go s.notifyRuntimeUpdated(testVer) + } + }() + + for i := 0; i < 100; i++ { + go func() { + + ch := make(chan<- runtime.Version) + chID, err := s.RegisterRuntimeUpdatedChannel(ch) + require.NoError(t, err) + unReg := s.UnregisterRuntimeUpdatedChannel(chID) + require.True(t, unReg) + }() } -} \ No newline at end of file +} + +// NewMockVersion creates and returns an runtime Version interface mock +func NewMockVersion(specVer uint32) *runtimemocks.MockVersion { + m := new(runtimemocks.MockVersion) + m.On("SpecName").Return([]byte(`mock-spec`)) + m.On("ImplName").Return(nil) + m.On("AuthoringVersion").Return(uint32(0)) + m.On("SpecVersion").Return(specVer) + m.On("ImplVersion").Return(uint32(0)) + m.On("TransactionVersion").Return(uint32(0)) + m.On("APIItems").Return(nil) + return m +} From 7e7e9bda484e1a5e9076cae28bd8233a5965e1e3 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Fri, 23 Jul 2021 10:48:50 -0400 Subject: [PATCH 18/35] generate id as uuid, add locks, delete from map --- dot/core/service.go | 36 +++++++++++++++++++----------------- dot/core/service_test.go | 3 +-- go.mod | 1 + 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 38838d5fb5..b3737d148f 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -19,7 +19,6 @@ import ( "context" "errors" "math/big" - "math/rand" "os" "sync" @@ -35,6 +34,7 @@ import ( "github.com/ChainSafe/gossamer/lib/transaction" "github.com/ChainSafe/gossamer/pkg/scale" log "github.com/ChainSafe/log15" + "github.com/google/uuid" ) var ( @@ -67,7 +67,7 @@ type Service struct { keys *keystore.GlobalKeystore runtimeUpdateSubscriptionsLock sync.RWMutex - runtimeUpdateSubscriptions map[byte]chan<- runtime.Version + runtimeUpdateSubscriptions map[uint32]chan<- runtime.Version } // Config holds the configuration for the core Service. @@ -134,7 +134,7 @@ func NewService(cfg *Config) (*Service, error) { codeSubstitute: cfg.CodeSubstitutes, codeSubstitutedState: cfg.CodeSubstitutedState, digestHandler: cfg.DigestHandler, - runtimeUpdateSubscriptions: make(map[byte]chan<- runtime.Version), + runtimeUpdateSubscriptions: make(map[uint32]chan<- runtime.Version), } return srv, nil @@ -576,7 +576,7 @@ func (s *Service) GetMetadata(bhash *common.Hash) ([]byte, error) { } // RegisterRuntimeUpdatedChannel function to register chan that is notified when runtime version changes -func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { +func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) { s.runtimeUpdateSubscriptionsLock.Lock() defer s.runtimeUpdateSubscriptionsLock.Unlock() @@ -584,31 +584,33 @@ func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte return 0, errors.New("channel limit reached") } - var id byte - for { - id = generateID() - if s.runtimeUpdateSubscriptions[id] == nil { - break - } - } + id := s.generateID() s.runtimeUpdateSubscriptions[id] = ch return id, nil } // UnregisterRuntimeUpdatedChannel function to unregister runtime updated channel -func (s *Service) UnregisterRuntimeUpdatedChannel(id byte) bool { +func (s *Service) UnregisterRuntimeUpdatedChannel(id uint32) bool { + s.runtimeUpdateSubscriptionsLock.Lock() + defer s.runtimeUpdateSubscriptionsLock.Unlock() ch, ok := s.runtimeUpdateSubscriptions[id] if ok { close(ch) - s.runtimeUpdateSubscriptions[id] = nil + delete(s.runtimeUpdateSubscriptions, id) return true } return false } -func generateID() byte { - // skipcq: GSC-G404 - id := rand.Intn(256) //nolint - return byte(id) +func (s *Service) generateID() uint32 { + uuid := uuid.New() + + // todo (ed) is it still necessary to do this check since we're using a UUID which should be unique? + for { + if s.runtimeUpdateSubscriptions[uuid.ID()] == nil { + break + } + } + return uuid.ID() } diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 7af07f969d..3dc247c3e5 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -625,8 +625,7 @@ func TestService_RegisterUnRegisterRuntimeUpdatedChannel(t *testing.T) { require.True(t, res) } -func TestService_RegisterUnRegisterPanic(t *testing.T) { - t.Skip() // TODO, this test is skipped because it fails with panic, todo determine reason and fix +func TestService_RegisterUnRegisterConcurrentCalls(t *testing.T) { s := NewTestService(t, nil) go func() { diff --git a/go.mod b/go.mod index 8c3ed52d72..8af8618492 100644 --- a/go.mod +++ b/go.mod @@ -18,6 +18,7 @@ require ( github.com/go-playground/validator/v10 v10.4.1 github.com/golang/protobuf v1.4.3 github.com/google/go-cmp v0.5.6 + github.com/google/uuid v1.1.5 github.com/gorilla/mux v1.8.0 github.com/gorilla/rpc v1.2.0 github.com/gorilla/websocket v1.4.2 From ff35489bc6b4a1d36a6be9205234f16e784afa8e Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Fri, 23 Jul 2021 11:05:15 -0400 Subject: [PATCH 19/35] update runtime updated channel ids to use uint32 --- dot/rpc/modules/api.go | 4 ++-- dot/rpc/modules/api_mocks.go | 2 +- dot/rpc/modules/mocks/core_api.go | 12 ++++++------ dot/rpc/subscription/listeners.go | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/dot/rpc/modules/api.go b/dot/rpc/modules/api.go index 74fa921544..4b4e076927 100644 --- a/dot/rpc/modules/api.go +++ b/dot/rpc/modules/api.go @@ -76,8 +76,8 @@ type CoreAPI interface { GetRuntimeVersion(bhash *common.Hash) (runtime.Version, error) HandleSubmittedExtrinsic(types.Extrinsic) error GetMetadata(bhash *common.Hash) ([]byte, error) - RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) - UnregisterRuntimeUpdatedChannel(id byte) bool + RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) + UnregisterRuntimeUpdatedChannel(id uint32) bool } // RPCAPI is the interface for methods related to RPC service diff --git a/dot/rpc/modules/api_mocks.go b/dot/rpc/modules/api_mocks.go index 7362fb1c36..1526378990 100644 --- a/dot/rpc/modules/api_mocks.go +++ b/dot/rpc/modules/api_mocks.go @@ -47,7 +47,7 @@ func NewMockCoreAPI() *modulesmocks.MockCoreAPI { m.On("IsBlockProducer").Return(false) m.On("HandleSubmittedExtrinsic", mock.AnythingOfType("types.Extrinsic")).Return(nil) m.On("GetMetadata", mock.AnythingOfType("*common.Hash")).Return(nil, nil) - m.On("RegisterRuntimeUpdatedChannel", mock.AnythingOfType("chan<- runtime.Version")).Return(byte(0), nil) + m.On("RegisterRuntimeUpdatedChannel", mock.AnythingOfType("chan<- runtime.Version")).Return(uint32(0), nil) return m } diff --git a/dot/rpc/modules/mocks/core_api.go b/dot/rpc/modules/mocks/core_api.go index 188b989273..0b7c91394d 100644 --- a/dot/rpc/modules/mocks/core_api.go +++ b/dot/rpc/modules/mocks/core_api.go @@ -105,14 +105,14 @@ func (_m *MockCoreAPI) InsertKey(kp crypto.Keypair) { } // RegisterRuntimeUpdatedChannel provides a mock function with given fields: ch -func (_m *MockCoreAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (byte, error) { +func (_m *MockCoreAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) { ret := _m.Called(ch) - var r0 byte - if rf, ok := ret.Get(0).(func(chan<- runtime.Version) byte); ok { + var r0 uint32 + if rf, ok := ret.Get(0).(func(chan<- runtime.Version) uint32); ok { r0 = rf(ch) } else { - r0 = ret.Get(0).(byte) + r0 = ret.Get(0).(uint32) } var r1 error @@ -126,11 +126,11 @@ func (_m *MockCoreAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) } // UnregisterRuntimeUpdatedChannel provides a mock function with given fields: id -func (_m *MockCoreAPI) UnregisterRuntimeUpdatedChannel(id byte) bool { +func (_m *MockCoreAPI) UnregisterRuntimeUpdatedChannel(id uint32) bool { ret := _m.Called(id) var r0 bool - if rf, ok := ret.Get(0).(func(byte) bool); ok { + if rf, ok := ret.Get(0).(func(uint32) bool); ok { r0 = rf(id) } else { r0 = ret.Get(0).(bool) diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index 426ae75947..c8523ff3c9 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -262,13 +262,13 @@ type RuntimeVersionListener struct { wsconn WSConnAPI subID uint runtimeUpdate chan runtime.Version - channelID byte + channelID uint32 coreAPI modules.CoreAPI } // VersionListener interface defining methods that version listener must implement type VersionListener interface { - GetChannelID() byte + GetChannelID() uint32 } // Listen implementation of Listen interface to listen for runtime version changes @@ -312,6 +312,6 @@ func (l *RuntimeVersionListener) Listen() { func (l *RuntimeVersionListener) Stop() {} // GetChannelID function that returns listener's channel ID -func (l *RuntimeVersionListener) GetChannelID() byte { +func (l *RuntimeVersionListener) GetChannelID() uint32 { return l.channelID } From c8d8d928e830dfa2fd7647b391b0f5bc186f8931 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Tue, 27 Jul 2021 11:22:17 -0400 Subject: [PATCH 20/35] change logging to debug --- dot/core/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/core/service.go b/dot/core/service.go index 576b7d3b38..d0c8e0d583 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -422,7 +422,7 @@ func (s *Service) notifyRuntimeUpdated(version runtime.Version) { return } - logger.Info("notifying runtime updated chans...", "chans", s.runtimeUpdateSubscriptions) + logger.Debug("notifying runtime updated chans...", "chans", s.runtimeUpdateSubscriptions) var wg sync.WaitGroup wg.Add(len(s.runtimeUpdateSubscriptions)) for _, ch := range s.runtimeUpdateSubscriptions { From 4747f96b195a52f500657999d9ab031e0feea03f Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Tue, 27 Jul 2021 11:25:05 -0400 Subject: [PATCH 21/35] remove comment --- dot/core/service.go | 1 - 1 file changed, 1 deletion(-) diff --git a/dot/core/service.go b/dot/core/service.go index d0c8e0d583..4f5cbb2e9a 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -608,7 +608,6 @@ func (s *Service) UnregisterRuntimeUpdatedChannel(id uint32) bool { func (s *Service) generateID() uint32 { uuid := uuid.New() - // todo (ed) is it still necessary to do this check since we're using a UUID which should be unique? for { if s.runtimeUpdateSubscriptions[uuid.ID()] == nil { break From c0cbdc0365815e6e46f6e6150ecc2d776c7e26af Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Tue, 27 Jul 2021 13:55:29 -0400 Subject: [PATCH 22/35] move uuid into check loop --- dot/core/service.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 4f5cbb2e9a..fdeb1a90aa 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -606,12 +606,12 @@ func (s *Service) UnregisterRuntimeUpdatedChannel(id uint32) bool { } func (s *Service) generateID() uint32 { - uuid := uuid.New() - + var uid uuid.UUID for { - if s.runtimeUpdateSubscriptions[uuid.ID()] == nil { + uid = uuid.New() + if s.runtimeUpdateSubscriptions[uid.ID()] == nil { break } } - return uuid.ID() + return uid.ID() } From b70b82cb5d920da17699c5651649aef29e2f0c6d Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Tue, 27 Jul 2021 14:37:07 -0400 Subject: [PATCH 23/35] update runtime test --- dot/rpc/modules/state_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dot/rpc/modules/state_test.go b/dot/rpc/modules/state_test.go index 39fd5ede14..4a03569195 100644 --- a/dot/rpc/modules/state_test.go +++ b/dot/rpc/modules/state_test.go @@ -40,7 +40,7 @@ func TestStateModule_GetRuntimeVersion(t *testing.T) { SpecName: "node", ImplName: "substrate-node", AuthoringVersion: 10, - SpecVersion: 264, + SpecVersion: 260, ImplVersion: 0, Apis: []interface{}{ []interface{}{"0xdf6acb689907609b", uint32(3)}, @@ -54,10 +54,9 @@ func TestStateModule_GetRuntimeVersion(t *testing.T) { []interface{}{"0xbc9d89904f5b923f", uint32(1)}, []interface{}{"0x68b66ba122c93fa7", uint32(1)}, []interface{}{"0x37c8bb1350a9a2a8", uint32(1)}, - []interface{}{"0x91d5df18b0d2cf58", uint32(1)}, []interface{}{"0xab3c0572291feb8b", uint32(1)}, }, - TransactionVersion: 2, + TransactionVersion: 1, } sm, hash, _ := setupStateModule(t) From fcac2b502552711828802066f7d32cc2bc0b0b84 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Tue, 27 Jul 2021 14:43:54 -0400 Subject: [PATCH 24/35] add test for notify runtime updated --- dot/core/service_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 121b7df2f1..8af78d98f3 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -626,6 +626,12 @@ func TestService_RegisterUnRegisterRuntimeUpdatedChannel(t *testing.T) { require.True(t, res) } +func TestService_notifyRuntimeUpdated(t *testing.T) { + s := NewTestService(t, nil) + testVer := NewMockVersion(1) + s.notifyRuntimeUpdated(testVer) +} + func TestService_RegisterUnRegisterConcurrentCalls(t *testing.T) { s := NewTestService(t, nil) From a41d0afca635b2d37bdb693c68280436b0e3b531 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Tue, 27 Jul 2021 15:53:44 -0400 Subject: [PATCH 25/35] update runtime tests --- dot/core/service_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 8af78d98f3..121b7df2f1 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -626,12 +626,6 @@ func TestService_RegisterUnRegisterRuntimeUpdatedChannel(t *testing.T) { require.True(t, res) } -func TestService_notifyRuntimeUpdated(t *testing.T) { - s := NewTestService(t, nil) - testVer := NewMockVersion(1) - s.notifyRuntimeUpdated(testVer) -} - func TestService_RegisterUnRegisterConcurrentCalls(t *testing.T) { s := NewTestService(t, nil) From 568ed88c2aa695555d14008c5ad33412b3226fae Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Tue, 27 Jul 2021 17:07:05 -0400 Subject: [PATCH 26/35] move runtime notify from coreAPI to blockStateAPI --- dot/core/service.go | 107 +++++++++-------------------- dot/core/service_test.go | 47 ------------- dot/rpc/modules/api.go | 4 +- dot/rpc/modules/mocks/block_api.go | 37 ++++++++++ dot/rpc/modules/mocks/core_api.go | 35 ---------- dot/rpc/subscription/websocket.go | 4 +- dot/state/block.go | 46 +++++++------ dot/state/block_notify.go | 64 +++++++++++++++++ dot/state/block_notify_test.go | 49 ++++++++++++- 9 files changed, 210 insertions(+), 183 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index fdeb1a90aa..e82797abf8 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -17,7 +17,6 @@ package core import ( "context" - "errors" "math/big" "os" "sync" @@ -34,7 +33,6 @@ import ( "github.com/ChainSafe/gossamer/lib/transaction" "github.com/ChainSafe/gossamer/pkg/scale" log "github.com/ChainSafe/log15" - "github.com/google/uuid" ) var ( @@ -65,9 +63,6 @@ type Service struct { // Keystore keys *keystore.GlobalKeystore - - runtimeUpdateSubscriptionsLock sync.RWMutex - runtimeUpdateSubscriptions map[uint32]chan<- runtime.Version } // Config holds the configuration for the core Service. @@ -122,19 +117,18 @@ func NewService(cfg *Config) (*Service, error) { ctx, cancel := context.WithCancel(context.Background()) srv := &Service{ - ctx: ctx, - cancel: cancel, - keys: cfg.Keystore, - blockState: cfg.BlockState, - epochState: cfg.EpochState, - storageState: cfg.StorageState, - transactionState: cfg.TransactionState, - net: cfg.Network, - blockAddCh: blockAddCh, - codeSubstitute: cfg.CodeSubstitutes, - codeSubstitutedState: cfg.CodeSubstitutedState, - digestHandler: cfg.DigestHandler, - runtimeUpdateSubscriptions: make(map[uint32]chan<- runtime.Version), + ctx: ctx, + cancel: cancel, + keys: cfg.Keystore, + blockState: cfg.BlockState, + epochState: cfg.EpochState, + storageState: cfg.StorageState, + transactionState: cfg.TransactionState, + net: cfg.Network, + blockAddCh: blockAddCh, + codeSubstitute: cfg.CodeSubstitutes, + codeSubstitutedState: cfg.CodeSubstitutedState, + digestHandler: cfg.DigestHandler, } return srv, nil @@ -414,25 +408,25 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { return nil } -func (s *Service) notifyRuntimeUpdated(version runtime.Version) { - s.runtimeUpdateSubscriptionsLock.RLock() - defer s.runtimeUpdateSubscriptionsLock.RUnlock() - - if len(s.runtimeUpdateSubscriptions) == 0 { - return - } - - logger.Debug("notifying runtime updated chans...", "chans", s.runtimeUpdateSubscriptions) - var wg sync.WaitGroup - wg.Add(len(s.runtimeUpdateSubscriptions)) - for _, ch := range s.runtimeUpdateSubscriptions { - go func(ch chan<- runtime.Version) { - defer wg.Done() - ch <- version - }(ch) - } - wg.Wait() -} +//func (s *Service) notifyRuntimeUpdated(version runtime.Version) { +// s.runtimeUpdateSubscriptionsLock.RLock() +// defer s.runtimeUpdateSubscriptionsLock.RUnlock() +// +// if len(s.runtimeUpdateSubscriptions) == 0 { +// return +// } +// +// logger.Debug("notifying runtime updated chans...", "chans", s.runtimeUpdateSubscriptions) +// var wg sync.WaitGroup +// wg.Add(len(s.runtimeUpdateSubscriptions)) +// for _, ch := range s.runtimeUpdateSubscriptions { +// go func(ch chan<- runtime.Version) { +// defer wg.Done() +// ch <- version +// }(ch) +// } +// wg.Wait() +//} // maintainTransactionPool removes any transactions that were included in the new block, revalidates the transactions in the pool, // and moves them to the queue if valid. @@ -576,42 +570,3 @@ func (s *Service) GetMetadata(bhash *common.Hash) ([]byte, error) { rt.SetContextStorage(ts) return rt.Metadata() } - -// RegisterRuntimeUpdatedChannel function to register chan that is notified when runtime version changes -func (s *Service) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) { - s.runtimeUpdateSubscriptionsLock.Lock() - defer s.runtimeUpdateSubscriptionsLock.Unlock() - - if len(s.runtimeUpdateSubscriptions) == 256 { - return 0, errors.New("channel limit reached") - } - - id := s.generateID() - - s.runtimeUpdateSubscriptions[id] = ch - return id, nil -} - -// UnregisterRuntimeUpdatedChannel function to unregister runtime updated channel -func (s *Service) UnregisterRuntimeUpdatedChannel(id uint32) bool { - s.runtimeUpdateSubscriptionsLock.Lock() - defer s.runtimeUpdateSubscriptionsLock.Unlock() - ch, ok := s.runtimeUpdateSubscriptions[id] - if ok { - close(ch) - delete(s.runtimeUpdateSubscriptions, id) - return true - } - return false -} - -func (s *Service) generateID() uint32 { - var uid uuid.UUID - for { - uid = uuid.New() - if s.runtimeUpdateSubscriptions[uid.ID()] == nil { - break - } - } - return uid.ID() -} diff --git a/dot/core/service_test.go b/dot/core/service_test.go index 121b7df2f1..d417b6557f 100644 --- a/dot/core/service_test.go +++ b/dot/core/service_test.go @@ -33,7 +33,6 @@ import ( "github.com/ChainSafe/gossamer/lib/keystore" "github.com/ChainSafe/gossamer/lib/runtime" "github.com/ChainSafe/gossamer/lib/runtime/extrinsic" - runtimemocks "github.com/ChainSafe/gossamer/lib/runtime/mocks" "github.com/ChainSafe/gossamer/lib/runtime/wasmer" "github.com/ChainSafe/gossamer/lib/transaction" "github.com/ChainSafe/gossamer/lib/utils" @@ -614,49 +613,3 @@ func TestService_HandleRuntimeChangesAfterCodeSubstitutes(t *testing.T) { require.NotEqualf(t, codeHashBefore, rt.GetCodeHash(), "expected different code hash after runtime update") // codeHash should change after runtime change } - -func TestService_RegisterUnRegisterRuntimeUpdatedChannel(t *testing.T) { - s := NewTestService(t, nil) - ch := make(chan<- runtime.Version) - chID, err := s.RegisterRuntimeUpdatedChannel(ch) - require.NoError(t, err) - require.NotNil(t, chID) - - res := s.UnregisterRuntimeUpdatedChannel(chID) - require.True(t, res) -} - -func TestService_RegisterUnRegisterConcurrentCalls(t *testing.T) { - s := NewTestService(t, nil) - - go func() { - for i := 0; i < 100; i++ { - testVer := NewMockVersion(uint32(i)) - go s.notifyRuntimeUpdated(testVer) - } - }() - - for i := 0; i < 100; i++ { - go func() { - - ch := make(chan<- runtime.Version) - chID, err := s.RegisterRuntimeUpdatedChannel(ch) - require.NoError(t, err) - unReg := s.UnregisterRuntimeUpdatedChannel(chID) - require.True(t, unReg) - }() - } -} - -// NewMockVersion creates and returns an runtime Version interface mock -func NewMockVersion(specVer uint32) *runtimemocks.MockVersion { - m := new(runtimemocks.MockVersion) - m.On("SpecName").Return([]byte(`mock-spec`)) - m.On("ImplName").Return(nil) - m.On("AuthoringVersion").Return(uint32(0)) - m.On("SpecVersion").Return(specVer) - m.On("ImplVersion").Return(uint32(0)) - m.On("TransactionVersion").Return(uint32(0)) - m.On("APIItems").Return(nil) - return m -} diff --git a/dot/rpc/modules/api.go b/dot/rpc/modules/api.go index 4b4e076927..167ded1588 100644 --- a/dot/rpc/modules/api.go +++ b/dot/rpc/modules/api.go @@ -38,6 +38,8 @@ type BlockAPI interface { RegisterFinalizedChannel(ch chan<- *types.FinalisationInfo) (byte, error) UnregisterFinalisedChannel(id byte) SubChain(start, end common.Hash) ([]common.Hash, error) + RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) + UnregisterRuntimeUpdatedChannel(id uint32) bool } // NetworkAPI interface for network state methods @@ -76,8 +78,6 @@ type CoreAPI interface { GetRuntimeVersion(bhash *common.Hash) (runtime.Version, error) HandleSubmittedExtrinsic(types.Extrinsic) error GetMetadata(bhash *common.Hash) ([]byte, error) - RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) - UnregisterRuntimeUpdatedChannel(id uint32) bool } // RPCAPI is the interface for methods related to RPC service diff --git a/dot/rpc/modules/mocks/block_api.go b/dot/rpc/modules/mocks/block_api.go index 1bb565bfa0..a64e4a82bf 100644 --- a/dot/rpc/modules/mocks/block_api.go +++ b/dot/rpc/modules/mocks/block_api.go @@ -8,6 +8,8 @@ import ( common "github.com/ChainSafe/gossamer/lib/common" mock "github.com/stretchr/testify/mock" + runtime "github.com/ChainSafe/gossamer/lib/runtime" + types "github.com/ChainSafe/gossamer/dot/types" ) @@ -210,6 +212,27 @@ func (_m *MockBlockAPI) RegisterImportedChannel(ch chan<- *types.Block) (byte, e return r0, r1 } +// RegisterRuntimeUpdatedChannel provides a mock function with given fields: ch +func (_m *MockBlockAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) { + ret := _m.Called(ch) + + var r0 uint32 + if rf, ok := ret.Get(0).(func(chan<- runtime.Version) uint32); ok { + r0 = rf(ch) + } else { + r0 = ret.Get(0).(uint32) + } + + var r1 error + if rf, ok := ret.Get(1).(func(chan<- runtime.Version) error); ok { + r1 = rf(ch) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // SubChain provides a mock function with given fields: start, end func (_m *MockBlockAPI) SubChain(start common.Hash, end common.Hash) ([]common.Hash, error) { ret := _m.Called(start, end) @@ -242,3 +265,17 @@ func (_m *MockBlockAPI) UnregisterFinalisedChannel(id byte) { func (_m *MockBlockAPI) UnregisterImportedChannel(id byte) { _m.Called(id) } + +// UnregisterRuntimeUpdatedChannel provides a mock function with given fields: id +func (_m *MockBlockAPI) UnregisterRuntimeUpdatedChannel(id uint32) bool { + ret := _m.Called(id) + + var r0 bool + if rf, ok := ret.Get(0).(func(uint32) bool); ok { + r0 = rf(id) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} diff --git a/dot/rpc/modules/mocks/core_api.go b/dot/rpc/modules/mocks/core_api.go index 0b7c91394d..47c1f6c5c5 100644 --- a/dot/rpc/modules/mocks/core_api.go +++ b/dot/rpc/modules/mocks/core_api.go @@ -103,38 +103,3 @@ func (_m *MockCoreAPI) HasKey(pubKeyStr string, keyType string) (bool, error) { func (_m *MockCoreAPI) InsertKey(kp crypto.Keypair) { _m.Called(kp) } - -// RegisterRuntimeUpdatedChannel provides a mock function with given fields: ch -func (_m *MockCoreAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) { - ret := _m.Called(ch) - - var r0 uint32 - if rf, ok := ret.Get(0).(func(chan<- runtime.Version) uint32); ok { - r0 = rf(ch) - } else { - r0 = ret.Get(0).(uint32) - } - - var r1 error - if rf, ok := ret.Get(1).(func(chan<- runtime.Version) error); ok { - r1 = rf(ch) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// UnregisterRuntimeUpdatedChannel provides a mock function with given fields: id -func (_m *MockCoreAPI) UnregisterRuntimeUpdatedChannel(id uint32) bool { - ret := _m.Called(id) - - var r0 bool - if rf, ok := ret.Get(0).(func(uint32) bool); ok { - r0 = rf(id) - } else { - r0 = ret.Get(0).(bool) - } - - return r0 -} diff --git a/dot/rpc/subscription/websocket.go b/dot/rpc/subscription/websocket.go index 37212da69f..8cf2830fdd 100644 --- a/dot/rpc/subscription/websocket.go +++ b/dot/rpc/subscription/websocket.go @@ -356,7 +356,7 @@ func (c *WSConn) initRuntimeVersionListener(reqID float64, _ interface{}) (Liste coreAPI: c.CoreAPI, } - chanID, err := c.CoreAPI.RegisterRuntimeUpdatedChannel(rvl.runtimeUpdate) + chanID, err := c.BlockAPI.RegisterRuntimeUpdatedChannel(rvl.runtimeUpdate) if err != nil { return nil, err } @@ -384,7 +384,7 @@ func (c *WSConn) unsubscribeRuntimeVersionListener(reqID float64, l Listener, _ } id := observer.GetChannelID() - res := c.CoreAPI.UnregisterRuntimeUpdatedChannel(id) + res := c.BlockAPI.UnregisterRuntimeUpdatedChannel(id) c.safeSend(newBooleanResponseJSON(res, reqID)) } diff --git a/dot/state/block.go b/dot/state/block.go index 1d53e88f8a..5a011b52f3 100644 --- a/dot/state/block.go +++ b/dot/state/block.go @@ -50,12 +50,14 @@ type BlockState struct { lastFinalised common.Hash // block notifiers - imported map[byte]chan<- *types.Block - finalised map[byte]chan<- *types.FinalisationInfo - importedLock sync.RWMutex - finalisedLock sync.RWMutex - importedBytePool *common.BytePool - finalisedBytePool *common.BytePool + imported map[byte]chan<- *types.Block + finalised map[byte]chan<- *types.FinalisationInfo + importedLock sync.RWMutex + finalisedLock sync.RWMutex + importedBytePool *common.BytePool + finalisedBytePool *common.BytePool + runtimeUpdateSubscriptionsLock sync.RWMutex + runtimeUpdateSubscriptions map[uint32]chan<- runtime.Version pruneKeyCh chan *types.Header } @@ -67,13 +69,14 @@ func NewBlockState(db chaindb.Database, bt *blocktree.BlockTree) (*BlockState, e } bs := &BlockState{ - bt: bt, - dbPath: db.Path(), - baseState: NewBaseState(db), - db: chaindb.NewTable(db, blockPrefix), - imported: make(map[byte]chan<- *types.Block), - finalised: make(map[byte]chan<- *types.FinalisationInfo), - pruneKeyCh: make(chan *types.Header, pruneKeyBufferSize), + bt: bt, + dbPath: db.Path(), + baseState: NewBaseState(db), + db: chaindb.NewTable(db, blockPrefix), + imported: make(map[byte]chan<- *types.Block), + finalised: make(map[byte]chan<- *types.FinalisationInfo), + pruneKeyCh: make(chan *types.Header, pruneKeyBufferSize), + runtimeUpdateSubscriptions: make(map[uint32]chan<- runtime.Version), } genesisBlock, err := bs.GetBlockByNumber(big.NewInt(0)) @@ -97,12 +100,13 @@ func NewBlockState(db chaindb.Database, bt *blocktree.BlockTree) (*BlockState, e // NewBlockStateFromGenesis initialises a BlockState from a genesis header, saving it to the database located at basePath func NewBlockStateFromGenesis(db chaindb.Database, header *types.Header) (*BlockState, error) { bs := &BlockState{ - bt: blocktree.NewBlockTreeFromRoot(header, db), - baseState: NewBaseState(db), - db: chaindb.NewTable(db, blockPrefix), - imported: make(map[byte]chan<- *types.Block), - finalised: make(map[byte]chan<- *types.FinalisationInfo), - pruneKeyCh: make(chan *types.Header, pruneKeyBufferSize), + bt: blocktree.NewBlockTreeFromRoot(header, db), + baseState: NewBaseState(db), + db: chaindb.NewTable(db, blockPrefix), + imported: make(map[byte]chan<- *types.Block), + finalised: make(map[byte]chan<- *types.FinalisationInfo), + pruneKeyCh: make(chan *types.Header, pruneKeyBufferSize), + runtimeUpdateSubscriptions: make(map[uint32]chan<- runtime.Version), } if err := bs.setArrivalTime(header.Hash(), time.Now()); err != nil { @@ -661,8 +665,9 @@ func (bs *BlockState) HandleRuntimeChanges(newState *rtstorage.TrieState, rt run } codeSubBlockHash := bs.baseState.LoadCodeSubstitutedBlockHash() + var newVersion runtime.Version if !codeSubBlockHash.Equal(common.Hash{}) { - newVersion, err := rt.CheckRuntimeVersion(code) //nolint + newVersion, err = rt.CheckRuntimeVersion(code) if err != nil { return err } @@ -706,6 +711,7 @@ func (bs *BlockState) HandleRuntimeChanges(newState *rtstorage.TrieState, rt run return fmt.Errorf("failed to update code substituted block hash: %w", err) } + bs.notifyRuntimeUpdated(newVersion) return nil } diff --git a/dot/state/block_notify.go b/dot/state/block_notify.go index 8c8c7e7c77..fc9828d41b 100644 --- a/dot/state/block_notify.go +++ b/dot/state/block_notify.go @@ -17,8 +17,13 @@ package state import ( + "errors" + "sync" + "github.com/ChainSafe/gossamer/dot/types" "github.com/ChainSafe/gossamer/lib/common" + "github.com/ChainSafe/gossamer/lib/runtime" + "github.com/google/uuid" ) // RegisterImportedChannel registers a channel for block notification upon block import. @@ -132,3 +137,62 @@ func (bs *BlockState) notifyFinalized(hash common.Hash, round, setID uint64) { }(ch) } } + +func (bs *BlockState) notifyRuntimeUpdated(version runtime.Version) { + bs.runtimeUpdateSubscriptionsLock.RLock() + defer bs.runtimeUpdateSubscriptionsLock.RUnlock() + + if len(bs.runtimeUpdateSubscriptions) == 0 { + return + } + + logger.Debug("notifying runtime updated chans...", "chans", bs.runtimeUpdateSubscriptions) + var wg sync.WaitGroup + wg.Add(len(bs.runtimeUpdateSubscriptions)) + for _, ch := range bs.runtimeUpdateSubscriptions { + go func(ch chan<- runtime.Version) { + defer wg.Done() + ch <- version + }(ch) + } + wg.Wait() +} + +// RegisterRuntimeUpdatedChannel function to register chan that is notified when runtime version changes +func (bs *BlockState) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) { + bs.runtimeUpdateSubscriptionsLock.Lock() + defer bs.runtimeUpdateSubscriptionsLock.Unlock() + + if len(bs.runtimeUpdateSubscriptions) == 256 { + return 0, errors.New("channel limit reached") + } + + id := bs.generateID() + + bs.runtimeUpdateSubscriptions[id] = ch + return id, nil +} + +// UnregisterRuntimeUpdatedChannel function to unregister runtime updated channel +func (bs *BlockState) UnregisterRuntimeUpdatedChannel(id uint32) bool { + bs.runtimeUpdateSubscriptionsLock.Lock() + defer bs.runtimeUpdateSubscriptionsLock.Unlock() + ch, ok := bs.runtimeUpdateSubscriptions[id] + if ok { + close(ch) + delete(bs.runtimeUpdateSubscriptions, id) + return true + } + return false +} + +func (bs *BlockState) generateID() uint32 { + var uid uuid.UUID + for { + uid = uuid.New() + if bs.runtimeUpdateSubscriptions[uid.ID()] == nil { + break + } + } + return uid.ID() +} diff --git a/dot/state/block_notify_test.go b/dot/state/block_notify_test.go index 60268d1adb..6135fbaad4 100644 --- a/dot/state/block_notify_test.go +++ b/dot/state/block_notify_test.go @@ -23,7 +23,8 @@ import ( "time" "github.com/ChainSafe/gossamer/dot/types" - + "github.com/ChainSafe/gossamer/lib/runtime" + runtimemocks "github.com/ChainSafe/gossamer/lib/runtime/mocks" "github.com/stretchr/testify/require" ) @@ -153,3 +154,49 @@ func TestFinalizedChannel_Multi(t *testing.T) { bs.UnregisterFinalisedChannel(id) } } + +func TestService_RegisterUnRegisterRuntimeUpdatedChannel(t *testing.T) { + bs := newTestBlockState(t, testGenesisHeader) + ch := make(chan<- runtime.Version) + chID, err := bs.RegisterRuntimeUpdatedChannel(ch) + require.NoError(t, err) + require.NotNil(t, chID) + + res := bs.UnregisterRuntimeUpdatedChannel(chID) + require.True(t, res) +} + +func TestService_RegisterUnRegisterConcurrentCalls(t *testing.T) { + bs := newTestBlockState(t, testGenesisHeader) + + go func() { + for i := 0; i < 100; i++ { + testVer := NewMockVersion(uint32(i)) + go bs.notifyRuntimeUpdated(testVer) + } + }() + + for i := 0; i < 100; i++ { + go func() { + + ch := make(chan<- runtime.Version) + chID, err := bs.RegisterRuntimeUpdatedChannel(ch) + require.NoError(t, err) + unReg := bs.UnregisterRuntimeUpdatedChannel(chID) + require.True(t, unReg) + }() + } +} + +// NewMockVersion creates and returns an runtime Version interface mock +func NewMockVersion(specVer uint32) *runtimemocks.MockVersion { + m := new(runtimemocks.MockVersion) + m.On("SpecName").Return([]byte(`mock-spec`)) + m.On("ImplName").Return(nil) + m.On("AuthoringVersion").Return(uint32(0)) + m.On("SpecVersion").Return(specVer) + m.On("ImplVersion").Return(uint32(0)) + m.On("TransactionVersion").Return(uint32(0)) + m.On("APIItems").Return(nil) + return m +} From 4fd52d8bf69f5678d0136c32c689af985de54c23 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Tue, 27 Jul 2021 18:04:12 -0400 Subject: [PATCH 27/35] fix mocks for tests --- dot/rpc/modules/api_mocks.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dot/rpc/modules/api_mocks.go b/dot/rpc/modules/api_mocks.go index 1526378990..0515f572ae 100644 --- a/dot/rpc/modules/api_mocks.go +++ b/dot/rpc/modules/api_mocks.go @@ -35,6 +35,8 @@ func NewMockBlockAPI() *modulesmocks.MockBlockAPI { m.On("GetJustification", mock.AnythingOfType("common.Hash")).Return(make([]byte, 10), nil) m.On("HasJustification", mock.AnythingOfType("common.Hash")).Return(true, nil) m.On("SubChain", mock.AnythingOfType("common.Hash"), mock.AnythingOfType("common.Hash")).Return(make([]common.Hash, 0), nil) + m.On("RegisterRuntimeUpdatedChannel", mock.AnythingOfType("chan<- runtime.Version")).Return(uint32(0), nil) + return m } @@ -47,7 +49,6 @@ func NewMockCoreAPI() *modulesmocks.MockCoreAPI { m.On("IsBlockProducer").Return(false) m.On("HandleSubmittedExtrinsic", mock.AnythingOfType("types.Extrinsic")).Return(nil) m.On("GetMetadata", mock.AnythingOfType("*common.Hash")).Return(nil, nil) - m.On("RegisterRuntimeUpdatedChannel", mock.AnythingOfType("chan<- runtime.Version")).Return(uint32(0), nil) return m } From 5e7fba5dbb63afa431d60de6b94e002ff515c1d0 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 28 Jul 2021 16:26:26 -0400 Subject: [PATCH 28/35] refactor state_test --- dot/rpc/modules/state_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dot/rpc/modules/state_test.go b/dot/rpc/modules/state_test.go index 4a03569195..39fd5ede14 100644 --- a/dot/rpc/modules/state_test.go +++ b/dot/rpc/modules/state_test.go @@ -40,7 +40,7 @@ func TestStateModule_GetRuntimeVersion(t *testing.T) { SpecName: "node", ImplName: "substrate-node", AuthoringVersion: 10, - SpecVersion: 260, + SpecVersion: 264, ImplVersion: 0, Apis: []interface{}{ []interface{}{"0xdf6acb689907609b", uint32(3)}, @@ -54,9 +54,10 @@ func TestStateModule_GetRuntimeVersion(t *testing.T) { []interface{}{"0xbc9d89904f5b923f", uint32(1)}, []interface{}{"0x68b66ba122c93fa7", uint32(1)}, []interface{}{"0x37c8bb1350a9a2a8", uint32(1)}, + []interface{}{"0x91d5df18b0d2cf58", uint32(1)}, []interface{}{"0xab3c0572291feb8b", uint32(1)}, }, - TransactionVersion: 1, + TransactionVersion: 2, } sm, hash, _ := setupStateModule(t) From 6038b6c563c7c780f9cbf82c30eca2527963ab79 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 29 Jul 2021 10:55:03 -0400 Subject: [PATCH 29/35] add tests --- dot/rpc/modules/api_mocks_test.go | 42 +++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 dot/rpc/modules/api_mocks_test.go diff --git a/dot/rpc/modules/api_mocks_test.go b/dot/rpc/modules/api_mocks_test.go new file mode 100644 index 0000000000..0b7a77a8d8 --- /dev/null +++ b/dot/rpc/modules/api_mocks_test.go @@ -0,0 +1,42 @@ +// Copyright 2019 ChainSafe Systems (ON) Corp. +// This file is part of gossamer. +// +// The gossamer library is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// The gossamer library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with the gossamer library. If not, see . + +package modules + +import ( + "github.com/stretchr/testify/require" + "testing" +) + +func TestNewMockStorageAPI(t *testing.T) { + m := NewMockStorageAPI() + require.NotNil(t, m) +} + +func TestNewMockBlockAPI(t *testing.T) { + m := NewMockBlockAPI() + require.NotNil(t, m) +} + +func TestNewMockCoreAPI(t *testing.T) { + m := NewMockCoreAPI() + require.NotNil(t, m) +} + +func TestNewMockVersion(t *testing.T) { + m := NewMockVersion() + require.NotNil(t, m) +} \ No newline at end of file From ea37d639025074025fd969b1ee562bb0de5796a7 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 29 Jul 2021 10:55:43 -0400 Subject: [PATCH 30/35] lint --- dot/rpc/modules/api_mocks_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dot/rpc/modules/api_mocks_test.go b/dot/rpc/modules/api_mocks_test.go index 0b7a77a8d8..1cf5759c42 100644 --- a/dot/rpc/modules/api_mocks_test.go +++ b/dot/rpc/modules/api_mocks_test.go @@ -17,8 +17,9 @@ package modules import ( - "github.com/stretchr/testify/require" "testing" + + "github.com/stretchr/testify/require" ) func TestNewMockStorageAPI(t *testing.T) { @@ -39,4 +40,4 @@ func TestNewMockCoreAPI(t *testing.T) { func TestNewMockVersion(t *testing.T) { m := NewMockVersion() require.NotNil(t, m) -} \ No newline at end of file +} From 739f8aa172ba1b61d1d1e7ebfc2df31edfbf1b51 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Wed, 4 Aug 2021 17:45:50 -0400 Subject: [PATCH 31/35] remove commented code, fix var assignment --- dot/core/service.go | 20 -------------------- dot/state/block.go | 8 ++++++-- 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index e82797abf8..f4db0e7fdb 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -408,26 +408,6 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { return nil } -//func (s *Service) notifyRuntimeUpdated(version runtime.Version) { -// s.runtimeUpdateSubscriptionsLock.RLock() -// defer s.runtimeUpdateSubscriptionsLock.RUnlock() -// -// if len(s.runtimeUpdateSubscriptions) == 0 { -// return -// } -// -// logger.Debug("notifying runtime updated chans...", "chans", s.runtimeUpdateSubscriptions) -// var wg sync.WaitGroup -// wg.Add(len(s.runtimeUpdateSubscriptions)) -// for _, ch := range s.runtimeUpdateSubscriptions { -// go func(ch chan<- runtime.Version) { -// defer wg.Done() -// ch <- version -// }(ch) -// } -// wg.Wait() -//} - // maintainTransactionPool removes any transactions that were included in the new block, revalidates the transactions in the pool, // and moves them to the queue if valid. // See https://github.com/paritytech/substrate/blob/74804b5649eccfb83c90aec87bdca58e5d5c8789/client/transaction-pool/src/lib.rs#L545 diff --git a/dot/state/block.go b/dot/state/block.go index 5a011b52f3..b7f52e9fdf 100644 --- a/dot/state/block.go +++ b/dot/state/block.go @@ -665,9 +665,9 @@ func (bs *BlockState) HandleRuntimeChanges(newState *rtstorage.TrieState, rt run } codeSubBlockHash := bs.baseState.LoadCodeSubstitutedBlockHash() - var newVersion runtime.Version + if !codeSubBlockHash.Equal(common.Hash{}) { - newVersion, err = rt.CheckRuntimeVersion(code) + newVersion, err := rt.CheckRuntimeVersion(code) //nolint if err != nil { return err } @@ -711,6 +711,10 @@ func (bs *BlockState) HandleRuntimeChanges(newState *rtstorage.TrieState, rt run return fmt.Errorf("failed to update code substituted block hash: %w", err) } + newVersion, err := rt.Version() + if err != nil { + return fmt.Errorf("failed to retrieve runtime version: %w", err) + } bs.notifyRuntimeUpdated(newVersion) return nil } From cb364a9571aa119f12aa8b82a116027185a633b2 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 5 Aug 2021 12:53:29 -0400 Subject: [PATCH 32/35] fix merge conflict --- dot/rpc/subscription/listeners.go | 13 +++---------- dot/rpc/subscription/listeners_test.go | 9 ++++----- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index 8c96d92bad..c7c2ceaaac 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -284,7 +284,7 @@ func (l *ExtrinsicSubmitListener) Stop() error { // RuntimeVersionListener to handle listening for Runtime Version type RuntimeVersionListener struct { wsconn WSConnAPI - subID uint + subID uint32 runtimeUpdate chan runtime.Version channelID uint32 coreAPI modules.CoreAPI @@ -293,8 +293,6 @@ type RuntimeVersionListener struct { // VersionListener interface defining methods that version listener must implement type VersionListener interface { GetChannelID() uint32 - wsconn *WSConn - subID uint32 } // Listen implementation of Listen interface to listen for runtime version changes @@ -313,7 +311,7 @@ func (l *RuntimeVersionListener) Listen() { ver.TransactionVersion = rtVersion.TransactionVersion() ver.Apis = modules.ConvertAPIs(rtVersion.APIItems()) - go l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) + go l.wsconn.safeSend(newSubscriptionResponse(stateRuntimeVersionMethod, l.subID, ver)) // listen for runtime updates go func() { @@ -328,19 +326,14 @@ func (l *RuntimeVersionListener) Listen() { ver.TransactionVersion = info.TransactionVersion() ver.Apis = modules.ConvertAPIs(info.APIItems()) - l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) + l.wsconn.safeSend(newSubscriptionResponse(stateRuntimeVersionMethod, l.subID, ver)) } }() } -// Stop to runtimeVersionListener not implemented yet because the listener -// does not need to be stopped -func (l *RuntimeVersionListener) Stop() {} - // GetChannelID function that returns listener's channel ID func (l *RuntimeVersionListener) GetChannelID() uint32 { return l.channelID - l.wsconn.safeSend(newSubscriptionResponse(stateRuntimeVersionMethod, l.subID, ver)) } // Stop to runtimeVersionListener not implemented yet because the listener diff --git a/dot/rpc/subscription/listeners_test.go b/dot/rpc/subscription/listeners_test.go index f58490a061..78202d3799 100644 --- a/dot/rpc/subscription/listeners_test.go +++ b/dot/rpc/subscription/listeners_test.go @@ -17,15 +17,14 @@ package subscription import ( - "io/ioutil" - "math/big" - "path/filepath" "encoding/json" "fmt" + "io/ioutil" "log" "math/big" "net/http" "net/http/httptest" + "path/filepath" "strings" "testing" "time" @@ -35,12 +34,12 @@ import ( "github.com/ChainSafe/gossamer/dot/state" "github.com/ChainSafe/gossamer/dot/types" "github.com/ChainSafe/gossamer/lib/common" + "github.com/ChainSafe/gossamer/lib/grandpa" "github.com/ChainSafe/gossamer/lib/runtime" "github.com/ChainSafe/gossamer/lib/runtime/wasmer" - "github.com/ChainSafe/gossamer/lib/grandpa" "github.com/gorilla/websocket" "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/require" ) type MockWSConnAPI struct { From 84f89d3e91379a66f33f46348dfa16223cf4d561 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Thu, 5 Aug 2021 13:03:48 -0400 Subject: [PATCH 33/35] add check if channel is ok --- dot/rpc/subscription/listeners.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dot/rpc/subscription/listeners.go b/dot/rpc/subscription/listeners.go index c7c2ceaaac..3aa9287e88 100644 --- a/dot/rpc/subscription/listeners.go +++ b/dot/rpc/subscription/listeners.go @@ -315,7 +315,12 @@ func (l *RuntimeVersionListener) Listen() { // listen for runtime updates go func() { - for info := range l.runtimeUpdate { + for { + info, ok := <-l.runtimeUpdate + if !ok { + return + } + ver := modules.StateRuntimeVersionResponse{} ver.SpecName = string(info.SpecName()) From 6270edec49f4f134fa8e93fe7eb67707a8ee3e80 Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Fri, 6 Aug 2021 10:03:17 -0400 Subject: [PATCH 34/35] update unit test --- dot/rpc/modules/mocks/BlockAPI.go | 4 ++-- dot/rpc/websocket_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dot/rpc/modules/mocks/BlockAPI.go b/dot/rpc/modules/mocks/BlockAPI.go index 13348363b1..dd6e6db19b 100644 --- a/dot/rpc/modules/mocks/BlockAPI.go +++ b/dot/rpc/modules/mocks/BlockAPI.go @@ -236,7 +236,7 @@ func (_m *BlockAPI) RegisterImportedChannel(ch chan<- *types.Block) (byte, error } // RegisterRuntimeUpdatedChannel provides a mock function with given fields: ch -func (_m *MockBlockAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) { +func (_m *BlockAPI) RegisterRuntimeUpdatedChannel(ch chan<- runtime.Version) (uint32, error) { ret := _m.Called(ch) var r0 uint32 @@ -290,7 +290,7 @@ func (_m *BlockAPI) UnregisterImportedChannel(id byte) { } // UnregisterRuntimeUpdatedChannel provides a mock function with given fields: id -func (_m *MockBlockAPI) UnregisterRuntimeUpdatedChannel(id uint32) bool { +func (_m *BlockAPI) UnregisterRuntimeUpdatedChannel(id uint32) bool { ret := _m.Called(id) var r0 bool diff --git a/dot/rpc/websocket_test.go b/dot/rpc/websocket_test.go index 25cff1dc96..8520b6ad8e 100644 --- a/dot/rpc/websocket_test.go +++ b/dot/rpc/websocket_test.go @@ -43,7 +43,7 @@ var testCalls = []struct { {[]byte(`{"jsonrpc":"2.0","method":"state_subscribeStorage","params":[],"id":4}`), []byte(`{"jsonrpc":"2.0","result":2,"id":4}` + "\n")}, {[]byte(`{"jsonrpc":"2.0","method":"chain_subscribeFinalizedHeads","params":[],"id":5}`), []byte(`{"jsonrpc":"2.0","result":3,"id":5}` + "\n")}, {[]byte(`{"jsonrpc":"2.0","method":"author_submitAndWatchExtrinsic","params":["0x010203"],"id":6}`), []byte("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":null,\"message\":\"Failed to call the `TaggedTransactionQueue_validate_transaction` exported function.\"},\"id\":6}\n")}, - {[]byte(`{"jsonrpc":"2.0","method":"state_subscribeRuntimeVersion","params":[],"id":7}`), []byte("{\"jsonrpc\":\"2.0\",\"result\":5,\"id\":7}\n")}, + {[]byte(`{"jsonrpc":"2.0","method":"state_subscribeRuntimeVersion","params":[],"id":7}`), []byte("{\"jsonrpc\":\"2.0\",\"result\":6,\"id\":7}\n")}, } func TestHTTPServer_ServeHTTP(t *testing.T) { From a22fca9af1e676898eb72508df5e1f7cbdfe9e8b Mon Sep 17 00:00:00 2001 From: Edward Mack Date: Mon, 9 Aug 2021 10:29:44 -0400 Subject: [PATCH 35/35] send notification as go routine --- dot/state/block.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/state/block.go b/dot/state/block.go index 8585ee8620..26a5114d5c 100644 --- a/dot/state/block.go +++ b/dot/state/block.go @@ -714,7 +714,7 @@ func (bs *BlockState) HandleRuntimeChanges(newState *rtstorage.TrieState, rt run if err != nil { return fmt.Errorf("failed to retrieve runtime version: %w", err) } - bs.notifyRuntimeUpdated(newVersion) + go bs.notifyRuntimeUpdated(newVersion) return nil }