Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore(lib/runtime): CheckRuntimeVersion independent from existing runtime instance #2687

Merged
merged 6 commits into from
Aug 15, 2022
Merged

chore(lib/runtime): CheckRuntimeVersion independent from existing runtime instance #2687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6cac143
`CheckRuntimeVersion` without function receiver
qdm12 Jul 21, 2022
0e089f0
`ext_misc_runtime_version_version_1` uses `CheckRuntimeVersion`
qdm12 Jul 21, 2022
8190fd8
Add `Benchmark_CheckRuntimeVersion`
qdm12 Aug 3, 2022
754be03
`CheckRuntimeVersion` uses `NewInstance`
qdm12 Aug 5, 2022
d7e0ad1
`CheckRuntimeVersion` -> `GetRuntimeVersion`
qdm12 Aug 15, 2022
6bce0f6
Update mocks
qdm12 Aug 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions dot/core/mocks_runtime_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 1 addition & 2 deletions dot/rpc/subscription/listeners_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,12 +351,11 @@ func TestRuntimeChannelListener_Listen(t *testing.T) {
expectedInitialResponse.Method = "state_runtimeVersion"
expectedInitialResponse.Params.Result = expectedInitialVersion

instance := wasmer.NewTestInstance(t, runtime.NODE_RUNTIME)
polkadotRuntimeFilepath, err := runtime.GetRuntime(context.Background(), runtime.POLKADOT_RUNTIME)
require.NoError(t, err)
code, err := os.ReadFile(polkadotRuntimeFilepath)
require.NoError(t, err)
version, err := instance.CheckRuntimeVersion(code)
version, err := wasmer.GetRuntimeVersion(code)
require.NoError(t, err)

expectedUpdatedVersion := modules.StateRuntimeVersionResponse{
Expand Down
2 changes: 1 addition & 1 deletion dot/state/block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ func (bs *BlockState) HandleRuntimeChanges(newState *rtstorage.TrieState,
codeSubBlockHash := bs.baseState.LoadCodeSubstitutedBlockHash()

if !codeSubBlockHash.Equal(common.Hash{}) {
newVersion, err := rt.CheckRuntimeVersion(code)
newVersion, err := wasmer.GetRuntimeVersion(code)
if err != nil {
return err
}
Expand Down
15 changes: 0 additions & 15 deletions dot/sync/mock_instance_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 0 additions & 15 deletions lib/blocktree/mock_instance_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion lib/runtime/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
// Instance is the interface a v0.8 runtime instance must implement
type Instance interface {
UpdateRuntimeCode([]byte) error
CheckRuntimeVersion([]byte) (Version, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you removed this function from the interface since it creates a new instance to do the version check, but how will we mock this out now? We'd have to change the function signatures of all callers of this function to include a func([]byte) (Version, error)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say it's better to not mock it, since:

  • it's fast
  • it's all CPU/in-memory and no IO
  • it's predictable
  • it adds testing depth which we should crave for when those 3 above are true

If you still want to mock it somewhere though, then you can set it as a functional field on the receiving struct like

type Service struct {
  checkRuntimeVersion(code []byte) (version Version, err error)
}

func New() *Service {
  return &Service{
    checkRuntimeVersion: wasmer.CheckRuntimeVersion,
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's fast at all. It's instantiating a wasmer instance, loading the code in, binding the imports, etc. Now all of the unit tests are runtime specific if we remove it from the Instance interface.

Why aren't we just implementing a RuntimeVersion() (Version, error) (no blob) on Instance. Then anyone who needs to version check needs to instantiate an instance via NewInstance.

Copy link
Contributor Author

@qdm12 qdm12 Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It takes 500ms to load the version, so not that fast indeed.
But if you have a look, there was no configuration of the CheckRuntimeVersion mock method expectation, so it won't change a thing with our code as it is.
And as I mentioned above, if in the future we want to mock the runtime check (for test coverage or testing speed), we can just set it as a field on the receiving struct (like we would with timeNow func() time.Time to mock time).

Also (in my opinionated opinion), having a function as a method just for the sake of it being part of an interface for testing is a bit weird, especially if it doesn't depend on the struct/implementation at all (wasn't the case before though, but it is now).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the interface again. The Instance interface includes a Version() (Version, error) method. I think we should remove CheckRuntimeVersion from the runtime package and make this a private method in dot/state and a helper function for the tests. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, although that would require to export a bunch of things such as setupVM, the wasmer Instance struct fields vm and ctx, and I think ultimately we should keep this in the wasmer package since it's quite tightly coupled with wasmer thingies? I would rather export CheckRuntimeVersion than these other function/fields, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need to export those private functions? To check a version of a given wasm blob, you instantiate an instance by calling NewInstance(code []byte, cfg runtime.InstanceConfig) and call Version() on that instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, sorry I misunderstood.
I changed CheckRuntimeVersion to use NewInstance, simplifying it a bit / reducing code duplication (although it sets up extra stuff it doesn't need, but that's a dep injection problem for another day).

On the other hand, ext_misc_runtime_version_version_1 in wasmer/imports.go uses CheckRuntimeVersion so I think we should still keep it in the wasmer package? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have dissonance with having a function on the public package API that achieves the same functionality that can be achieved by instantiating an instance and calling a receiver method on that instance. But that would lead to code duplication cause we use it in various places. I gave it a shot in this commit, and I'm not really happy with it. So I'm ok with leaving with leaving it in the wasmer package.

Stop()
NodeStorage() NodeStorage
NetworkService() BasicNetwork
Expand Down
23 changes: 0 additions & 23 deletions lib/runtime/mocks/instance.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 2 additions & 15 deletions lib/runtime/wasmer/imports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,13 @@ import (
"time"
"unsafe"

"github.com/ChainSafe/gossamer/internal/log"
"github.com/ChainSafe/gossamer/lib/common"
rtype "github.com/ChainSafe/gossamer/lib/common/types"
"github.com/ChainSafe/gossamer/lib/crypto"
"github.com/ChainSafe/gossamer/lib/crypto/ed25519"
"github.com/ChainSafe/gossamer/lib/crypto/secp256k1"
"github.com/ChainSafe/gossamer/lib/crypto/sr25519"
"github.com/ChainSafe/gossamer/lib/runtime"
rtstorage "github.com/ChainSafe/gossamer/lib/runtime/storage"
"github.com/ChainSafe/gossamer/lib/transaction"
"github.com/ChainSafe/gossamer/lib/trie"
"github.com/ChainSafe/gossamer/lib/trie/proof"
Expand Down Expand Up @@ -944,20 +942,9 @@ func ext_misc_runtime_version_version_1(context unsafe.Pointer, dataSpan C.int64
logger.Trace("executing...")

instanceContext := wasm.IntoInstanceContext(context)
data := asMemorySlice(instanceContext, dataSpan)

cfg := runtime.InstanceConfig{
LogLvl: log.DoNotChange,
Storage: rtstorage.NewTrieState(nil),
}

instance, err := NewInstance(data, cfg)
if err != nil {
logger.Errorf("failed to create instance: %s", err)
return 0
}
code := asMemorySlice(instanceContext, dataSpan)

version, err := instance.Version()
version, err := GetRuntimeVersion(code)
if err != nil {
logger.Errorf("failed to get runtime version: %s", err)
out, _ := toWasmMemoryOptional(instanceContext, nil)
Expand Down
27 changes: 13 additions & 14 deletions lib/runtime/wasmer/instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,25 +156,24 @@ func (in *Instance) UpdateRuntimeCode(code []byte) (err error) {
return nil
}

// CheckRuntimeVersion calculates runtime Version for runtime blob passed in
func (in *Instance) CheckRuntimeVersion(code []byte) (runtime.Version, error) {
in.mutex.Lock()
defer in.mutex.Unlock()

wasmInstance, allocator, err := setupVM(code)
// GetRuntimeVersion finds the runtime version by initiating a temporary
// runtime instance using the WASM code provided, and querying it.
func GetRuntimeVersion(code []byte) (version runtime.Version, err error) {
config := runtime.InstanceConfig{
LogLvl: log.DoNotChange,
}
instance, err := NewInstance(code, config)
if err != nil {
return nil, fmt.Errorf("setting up VM: %w", err)
return version, fmt.Errorf("creating runtime instance: %w", err)
}
defer instance.Stop()

in.ctx.Allocator = allocator // TODO we should no change the allocator of the parent instance
wasmInstance.SetContextData(in.ctx)

instance := Instance{
vm: wasmInstance,
ctx: in.ctx,
version, err = instance.Version()
if err != nil {
return nil, fmt.Errorf("running runtime: %w", err)
}

return instance.Version()
return version, nil
}

var (
Expand Down
17 changes: 14 additions & 3 deletions lib/runtime/wasmer/instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,13 @@ func TestPointerSize(t *testing.T) {
require.Equal(t, in, res)
}

func TestInstance_CheckRuntimeVersion(t *testing.T) {
instance := NewTestInstance(t, runtime.NODE_RUNTIME)
func Test_GetRuntimeVersion(t *testing.T) {
polkadotRuntimeFilepath, err := runtime.GetRuntime(
context.Background(), runtime.POLKADOT_RUNTIME)
require.NoError(t, err)
code, err := os.ReadFile(polkadotRuntimeFilepath)
require.NoError(t, err)
version, err := instance.CheckRuntimeVersion(code)
version, err := GetRuntimeVersion(code)
require.NoError(t, err)

expected := runtime.NewVersionData(
Expand All @@ -65,6 +64,18 @@ func TestInstance_CheckRuntimeVersion(t *testing.T) {
require.Equal(t, expected.TransactionVersion(), version.TransactionVersion())
}

func Benchmark_GetRuntimeVersion(b *testing.B) {
polkadotRuntimeFilepath, err := runtime.GetRuntime(
context.Background(), runtime.POLKADOT_RUNTIME)
require.NoError(b, err)

b.ResetTimer()
for i := 0; i < b.N; i++ {
code, _ := os.ReadFile(polkadotRuntimeFilepath)
_, _ = GetRuntimeVersion(code)
}
}

func TestDecompressWasm(t *testing.T) {
encoder, err := zstd.NewWriter(nil)
require.NoError(t, err)
Expand Down