Atomic vm wrapper #758

ceyonur · 2025-01-20T16:13:17Z

Why this should be merged

This adds a way to extend the inner EVM and wraps it with atomic VM. For demonstration it sets the network codec with atomic codec.

How this works

This pull request introduces significant changes to the plugin/evm package to improve the extensibility and modularity of the EVM (Ethereum Virtual Machine) implementation. The primary focus is on creating a new VM wrapper and updating initialization procedures to support network codec management. Here are the most important changes:

Unexported Shared EVM

Renamed and unexported the Shared EVM (plugin/evm/vm.go) and provided constructors.

New VM Wrapper and Initialization

plugin/evm/atomic/vm/vm.go: Introduced a new VM wrapper that implements multiple interfaces (secp256k1fx.VM, block.ChainVM, block.BuildBlockWithContextChainVM, block.StateSyncableVM). Added the WrapVM function to wrap an InnerVM and the Initialize method to set up the network codec.

Factory Updates

plugin/evm/factory.go: Modified the Factory struct to return the new VM wrapper in the New and NewPlugin methods, allowing the EVM to be used as a plugin with the appropriate network codec setup. [1] [2]

Codec Management

plugin/evm/vm.go: Removed the global networkCodec variable and added a networkCodec field to the VM struct. Implemented the SetNetworkCodec method to manage the codec. Updated various methods to use the networkCodec field instead of the global variable. [1] [2] [3] [4] [5] [6] [7] [8]

Test Updates

plugin/evm/vm_warp_test.go: Added initialization of the networkCodec variable in tests to ensure proper codec setup during test execution. [1] [2]

Main Function Update

plugin/main.go: Updated the main function to use the new Factory struct for creating the EVM plugin, ensuring the proper setup of the network codec.

How this was tested

Existing UTs should cover, but might be refactored.

Need to be documented?

No

Need to update RELEASES.md?

No

darioush · 2025-01-20T21:04:06Z

plugin/evm/vm.go

-// VM implements the snowman.ChainVM interface
-type VM struct {
+// sharedEvm implements the snowman.ChainVM interface
+type sharedEvm struct {


Could we put this back? "VM" seems sufficient and as the wrapped VM is in another package it seems that we can leave this.

I dont want to export this as without default extensions it will break. we can check if required extensions are nil in initialize to make sure it won't break if we want to. but I still slightly think we should not directly expose vm.

Smaller API / less exported is always a good thing imo, so happy to keep this unexported. Now yeah it's more not-necessarily-necessary git diffs, but fine with me.

I don't think sharedEvm is a good name here (what is it shared between? runtime? compile time?) Perhaps we can use a more descriptive name or vm?

I don't disagree that unexported is better, but renaming this seems like it should be its own PR as it is not necessary for this PR and makes this PR difficult to follow.

darioush · 2025-01-20T21:05:09Z

plugin/evm/atomic/vm/vm.go

+}
+
+// Initialize implements the snowman.ChainVM interface
+func (vm *VM) Initialize(


possibly instead of plugin/evm/atomic/vm, we can have plugin/evm/coreth which defines the VM for use in avalanchego.

Yes, I think we probably would want to have a coreth/ and subnet-evm/ packages in the merged repo. For now I did not want to break the avalanchego.

darioush · 2025-01-20T21:06:38Z

plugin/evm/block.go

 	atomicTxs []*atomic.Tx
 }

 // newBlock returns a new Block wrapping the ethBlock type and implementing the snowman.Block interface
-func (vm *VM) newBlock(ethBlock *types.Block) (*Block, error) {
+func (vm *sharedEvm) newBlock(ethBlock *types.Block) (*Block, error) {


I assume some further changes to block.go is needed to fully have the atomic vm separated from the "base" evm that we can share with subnet-evm?

plugin/evm/atomic/vm/vm.go

qdm12 · 2025-01-21T08:31:48Z

plugin/evm/admin.go

 	profiler profiler.Profiler
 }

-func NewAdminService(vm *VM, performanceDir string) *Admin {
+func NewAdminService(vm *sharedEvm, performanceDir string) *Admin {


this should be an unexported function. Otherwise the vm argument type should either be exported or be an exported interface (preferrable). But in this case, NewAdminService is only used once in the same package, I'd just unexport it.

qdm12 · 2025-01-21T08:34:56Z

plugin/evm/factory.go

 type Factory struct{}

 func (*Factory) New(logging.Logger) (interface{}, error) {
-	return &VM{}, nil
+	return atomicvm.WrapVM(NewExtensibleEVM(false)), nil


supernit: you can use constants to indicate what the bool argument is, it's nice for readability instead of having to hover over the function to check what the argument is / check the fn in code when PR reviewing on the github website

Suggested change

return atomicvm.WrapVM(NewExtensibleEVM(false)), nil

const isPlugin = false

return atomicvm.WrapVM(NewExtensibleEVM(isPlugin)), nil

Same for NewPlugin below 😉

qdm12 · 2025-01-21T08:38:15Z

plugin/evm/vm.go

+// NewDefaultEVM returns a new instance of the VM with default extensions
+// This should not be called if the VM is being extended
+func NewDefaultEVM() *sharedEvm {


Since this is only used in tests, move it to a _test.go file? You could have a helpers_test.go since it's used in various test files. Unless we want to use elsewhere 🤔
And then you can pass in a t *testing.T and use require.NoError(t, err) for error checks.

If this is meant to be used in production code, we really need to return an error I think? Even if it's mean to be in production code later, I'd rather have it in test code and then migrated to production code when it's used in production code at a later time.

I moved it to test for now, alternatively we can check these extension points in Initialize if they're nil, and if so we can provide some defaults in the vm.Initialize() directly.

plugin/evm/vm.go

qdm12 · 2025-01-21T08:45:56Z

plugin/evm/vm.go

+	return vm
+}
+
+func NewExtensibleEVM(isPlugin bool) *sharedEvm {


unexport this since it's only used in plugin/evm

If you want to export it for user by other packages, then export sharedEVM (aka re-use older VM name and reduce diffs at the same time)

plugin/evm/vm.go

qdm12 · 2025-01-21T08:51:39Z

plugin/evm/vm_warp_test.go

+var networkCodec codec.Manager
+
+func init() {
+	var err error
+	networkCodec, err = message.NewCodec(message.BlockSyncSummary{})
+	if err != nil {
+		panic(err)
+	}
+}


Instead of having this at global scope with an init block, you could have the networkCodec created each time it's needed (in 5 places below in this same file) with

networkCodec, err := message.NewCodec(message.BlockSyncSummary{}) require.NoError(t, err)

It's always better to avoid global state / init blocks if possible.

qdm12 · 2025-01-21T08:55:25Z

plugin/evm/factory.go

+// TODO: either move this from plugin or move the VM itself
 type Factory struct{}


Why do we use a factory after all actually? It looks like it's only used in plugin/main.go as

rpcchainvm.Serve(context.Background(), factory.NewPlugin())

It makes no sense to have a factory as far as I understood 🤔 ?

which could well be replaced with

factory := evm.Factory{} rpcchainvm.Serve(context.Background(), evm.NewPlugin())

Normally We implement vms.Factory from avalanchego
https://github.com/ava-labs/coreth/blob/master/plugin/evm/factory.go#L16

which is used here:
https://github.com/ava-labs/avalanchego/blob/add-l1-validators-api/node/node.go#L1240

but yea this can just be a function

Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

…tomic-vm-wrapper

darioush · 2025-01-22T01:29:41Z

plugin/evm/vm.go

-// VM implements the snowman.ChainVM interface
-type VM struct {
+// sharedEvm implements the snowman.ChainVM interface
+type sharedEvm struct {


I don't think sharedEvm is a good name here (what is it shared between? runtime? compile time?) Perhaps we can use a more descriptive name or vm?

I don't disagree that unexported is better, but renaming this seems like it should be its own PR as it is not necessary for this PR and makes this PR difficult to follow.

darioush · 2025-01-22T01:31:14Z

plugin/evm/vm.go

@@ -314,14 +308,26 @@ type VM struct {
 	rpcHandlers []interface{ Stop() }
 }

+func newExtensibleEVM(isPlugin bool) *sharedEvm {


could we have a function like newVM that takes the codec.Manager and drop the SetNetworkCodec function below?

It seems this type is not fully initialized even if it is constructed with a "new" function.

We can pass the "additional extension points" in to the "new" function (either as separate arguments or as a config struct), as all the customization should be known at construction time (i.e., is this VM coreth or subnet-evm).

ceyonur added 3 commits January 20, 2025 18:47

wrap atomic vm

ae80881

set network codec

f75f052

unexport inner evm

96eb3ed

darioush reviewed Jan 20, 2025

View reviewed changes

qdm12 reviewed Jan 21, 2025

View reviewed changes

ceyonur and others added 7 commits January 21, 2025 12:56

Update plugin/evm/atomic/vm/vm.go

84ddc34

Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

Update plugin/evm/atomic/vm/vm.go

7ba0e38

Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

Update plugin/evm/vm.go

1b5c84e

Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

Update plugin/evm/vm.go

90e248c

Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

nits

28702f3

Merge branch 'atomic-vm-wrapper' of github.com:ava-labs/coreth into a…

d3c9753

…tomic-vm-wrapper

revert extra change

75f6b0e

darioush reviewed Jan 22, 2025

View reviewed changes

add extension config

d2a2418

ceyonur closed this Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atomic vm wrapper #758

Atomic vm wrapper #758

ceyonur commented Jan 20, 2025 •

edited

Loading

darioush Jan 20, 2025

ceyonur Jan 20, 2025

qdm12 Jan 21, 2025

darioush Jan 22, 2025

darioush Jan 20, 2025

ceyonur Jan 21, 2025

darioush Jan 20, 2025

qdm12 Jan 21, 2025

qdm12 Jan 21, 2025

qdm12 Jan 21, 2025

ceyonur Jan 21, 2025

qdm12 Jan 21, 2025 •

edited

Loading

qdm12 Jan 21, 2025

qdm12 Jan 21, 2025

ceyonur Jan 21, 2025

darioush Jan 22, 2025

darioush Jan 22, 2025 •

edited

Loading

	return atomicvm.WrapVM(NewExtensibleEVM(false)), nil
	const isPlugin = false
	return atomicvm.WrapVM(NewExtensibleEVM(isPlugin)), nil

		// TODO: either move this from plugin or move the VM itself
		type Factory struct{}

Atomic vm wrapper #758

Atomic vm wrapper #758

Conversation

ceyonur commented Jan 20, 2025 • edited Loading

Why this should be merged

How this works

Unexported Shared EVM

New VM Wrapper and Initialization

Factory Updates

Codec Management

Test Updates

Main Function Update

How this was tested

Need to be documented?

Need to update RELEASES.md?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

ceyonur commented Jan 20, 2025 •

edited

Loading

qdm12 Jan 21, 2025 •

edited

Loading

darioush Jan 22, 2025 •

edited

Loading