-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(rpc/subscription): subscribe runtime version notify when version changes #1686
Changes from 12 commits
7325277
2d324de
9394d38
cfe9117
37c8878
fb75b48
feb5f4d
a31474d
3017ed3
fd83528
432d604
0dcc08f
d7c5d59
8c16176
de0c40b
5512348
09c9b2e
cf0e477
6c3ea54
ade2a82
c0eb904
8031b82
d6f6cea
2f6c7e2
98fb9d9
42f436e
125a5d1
0e2fcd7
7e7e9bd
66bd9e2
ff35489
70d6c27
cecc811
c8d8d92
4747f96
c0cbdc0
b70b82c
fcac2b5
a41d0af
568ed88
4fd52d8
5e7fba5
6038b6c
ea37d63
dc78e71
0de3c60
739f8aa
e3e5cef
39f4a88
cb364a9
84f89d3
1a877ac
6270ede
9be6179
a22fca9
154061f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this errors that is bad, so should handle that error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated to return err here. |
||
|
||
s.codeHash = currCodeHash | ||
|
||
err = s.codeSubstitutedState.StoreCodeSubstitutedBlockHash(common.Hash{}) | ||
|
@@ -462,6 +473,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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is the channel being written to as a select case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, no reason, so I've removed the select case. |
||
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 | ||
|
@@ -579,3 +609,33 @@ func (s *Service) GetMetadata(bhash *common.Hash) ([]byte, error) { | |
s.rt.SetContextStorage(ts) | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice if we have an Unregister method that closes the channel and remove it from the map There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added Unregister method. |
||
s.runtimeChangedLock.RLock() | ||
|
||
if len(s.runtimeChanged) == 256 { | ||
return 0, errors.New("channel limit reached") | ||
noot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
var id byte | ||
for { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that seems better. |
||
id = generateID() | ||
if s.runtimeChanged[id] == nil { | ||
break | ||
} | ||
} | ||
|
||
s.runtimeChangedLock.RUnlock() | ||
|
||
s.runtimeChangedLock.Lock() | ||
s.runtimeChanged[id] = ch | ||
s.runtimeChangedLock.Unlock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are needed these two locks? Just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I've updated to use just one lock (and unlock with defer). |
||
return id, nil | ||
} | ||
|
||
func generateID() byte { | ||
// skipcq: GSC-G404 | ||
id := rand.Intn(256) //nolint | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe use a uuid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to use uuid, do I still need to do the loop check since this is a uuid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea you should still check even though it's extremely unlikely. |
||
return byte(id) | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -24,6 +24,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" | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
// Listener interface for functions that define Listener related functions | ||||||||||||||||
|
@@ -258,20 +259,20 @@ func (l *ExtrinsicSubmitListener) Stop() { l.cancel() } | |||||||||||||||
|
||||||||||||||||
// RuntimeVersionListener to handle listening for Runtime Version | ||||||||||||||||
type RuntimeVersionListener struct { | ||||||||||||||||
wsconn *WSConn | ||||||||||||||||
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 | ||||||||||||||||
rtVersion, err := l.wsconn.CoreAPI.GetRuntimeVersion(nil) | ||||||||||||||||
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() | ||||||||||||||||
|
@@ -280,9 +281,26 @@ func (l *RuntimeVersionListener) Listen() { | |||||||||||||||
ver.TransactionVersion = rtVersion.TransactionVersion() | ||||||||||||||||
ver.Apis = modules.ConvertAPIs(rtVersion.APIItems()) | ||||||||||||||||
|
||||||||||||||||
l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) | ||||||||||||||||
go l.wsconn.safeSend(newSubscriptionResponse("state_runtimeVersion", l.subID, ver)) | ||||||||||||||||
|
||||||||||||||||
// listen for runtime updates | ||||||||||||||||
go func() { | ||||||||||||||||
for info := range l.runtimeUpdate { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
add closed channel check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I try to add ok assignment as above, I get compile error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh nvm I missed that this is a range, you'd have to change it to read from the channel in a for loop
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just pinging on this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated. |
||||||||||||||||
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() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename
runtimeUpdateSubscriptions
or some thing like that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to
runtimeUpdateSubscriptions
andruntimeUpdateSubscriptionsLock