Atomic vm refactor #768

ceyonur · 2025-01-28T16:06:59Z

Why this should be merged

This PR moves atomic logic to atomic pkg and adds atomic VM that wraps the evm/vm.

AvalancheGo PR: ava-labs/avalanchego#3702

How this works

Defines InnerVM/ExtensibleVM interfaces to extend and wrap plugin/vm with atomic logic and capabilities. Implement InnerVM/ExtensibleVM in evm/VM.

How this was tested

Extended unit tests
moved existing test to atomic pkg
e2e tests should cover
will bootstrap fuji/mainnet

Need to be documented?

No

Need to update RELEASES.md?

updated

Co-authored-by: Quentin McGaw <quentin.mcgaw@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

…tomic-vm-wrapper

ceyonur · 2025-02-07T14:29:33Z

plugin/evm/atomic/state/atomic_state.go

-	for chainID, requests := range requests {
-		mergeAtomicOpsToMap(a.atomicOps, chainID, requests)
-	}


this is a no-op since requests was nil here: https://github.com/ava-labs/coreth/blob/seperate-atomic-pkg-base/plugin/evm/block.go#L186

ceyonur · 2025-02-07T14:53:23Z

plugin/evm/atomic/sync/atomic_summary_provider.go

-	blk := a.chain.GetBlockByNumber(height)
-	if blk == nil {
-		return nil, fmt.Errorf("block not found for height (%d)", height)
-	}
-
-	if !a.chain.HasState(blk.Root()) {
-		return nil, fmt.Errorf("block root does not exist for height (%d), root (%s)", height, blk.Root())
-	}
-


moved to syncervm_server.go as this is a common check with state trie.

ceyonur · 2025-02-07T15:27:05Z

plugin/evm/atomic/sync/atomic_sync_extender.go

-		atomicTrie:           atomicTrie,
-		stateSyncRequestSize: stateSyncRequestSize,
-	}
+func (a *AtomicSyncExtender) Initialize(backend AtomicBackend, atomicTrie AtomicTrie, stateSyncRequestSize uint16) {


This needs to be called after innervm initialize (so atomicbackend can be available)

ceyonur · 2025-02-07T15:27:21Z

plugin/evm/atomic/tx.go

+type Visitor interface {
+	ImportTx(*UnsignedImportTx) error
+	ExportTx(*UnsignedExportTx) error
+}


This worked like a charm

ceyonur · 2025-02-07T15:31:51Z

plugin/evm/atomic/state/atomic_backend.go

-	atomicTrieDBPrefix           = []byte("atomicTrieDB")
-	atomicTrieMetaDBPrefix       = []byte("atomicTrieMetaDB")
-	appliedSharedMemoryCursorKey = []byte("atomicTrieLastAppliedToSharedMemory")


moved all these to repo as it seemed to me that it should be the one handling database operations.

ceyonur · 2025-02-07T16:39:49Z

plugin/evm/sync/syncervm_client.go

@@ -165,13 +171,15 @@ func (client *stateSyncerClient) stateSync(ctx context.Context) error {
 		return err
 	}

-	// Sync the EVM trie and then the atomic trie. These steps could be done
-	// in parallel or in the opposite order. Keeping them serial for simplicity for now.


hmmmm looks like it's actually simpler so we can run them in parallel now.

definetly not in the current scope though

ceyonur · 2025-02-07T16:41:32Z

plugin/evm/testutils/genesis.go

+		b, _ = cb58.Decode(key)
+		pk, _ := secp256k1.ToPrivateKey(b)
+		balance := new(big.Int).Mul(big.NewInt(params.Ether), big.NewInt(10))
+		g.Alloc[pk.EthAddress()] = types.GenesisAccount{Balance: balance}


test private keys are not init here yet. (we call this in a var block above)

ceyonur · 2025-02-07T16:44:25Z

plugin/evm/atomic/vm/vm_test.go

+	*avalancheatomic.Memory,
+	*enginetest.Sender,
+) {
+	vm := WrapVM(&evm.VM{})


we import evm.VM here (only here)

ceyonur · 2025-02-07T16:46:23Z

plugin/evm/vm.go

 	}

 	// if the chainCtx.NetworkUpgrades is not empty, set the chain config
 	// normally it should not be empty, but some tests may not set it
 	if chainCtx.NetworkUpgrades != (upgrade.Config{}) {
-		g.Config = params.GetChainConfig(chainCtx.NetworkUpgrades, new(big.Int).Set(chainID))
+		g.Config.NetworkUpgrades = params.GetNetworkUpgrades(chainCtx.NetworkUpgrades)


We should probably stop defaulting any part of genesis/chainConfig. This still modifies it but only the NetworkUpgrades.

we had few ugly cases in subnet-evm because we filled genesis/chainConfig and turned out that it was even more confusing to maintain.

ceyonur · 2025-02-07T16:48:10Z

plugin/evm/vm.go

-	}
-
-	// initialize peer network
-	if vm.p2pSender == nil {


seems this is unnecessary since we can directly pass appSender here.

this was used in some testing, perhaps check with @joshua-kim

It's not read anywhere in the repo, from what I can tell?

even if this is being used anywhere, I think it was wrong.

ceyonur · 2025-02-07T20:25:02Z

plugin/evm/atomic/import_tx.go

@@ -185,93 +182,6 @@ func (utx *UnsignedImportTx) Burned(assetID ids.ID) (uint64, error) {
 	return math.Sub(input, spent)
 }

-// SemanticVerify this transaction is valid.
-func (utx *UnsignedImportTx) SemanticVerify(


moved to tx_semantic_verifier

ceyonur · 2025-02-07T20:25:17Z

plugin/evm/atomic/export_tx.go

@@ -178,75 +177,7 @@ func (utx *UnsignedExportTx) Burned(assetID ids.ID) (uint64, error) {
 	return math.Sub(input, spent)
 }

-// SemanticVerify this transaction is valid.
-func (utx *UnsignedExportTx) SemanticVerify(


moved to tx_semantic_verifier

darioush · 2025-02-07T14:21:43Z

vmerrs/vmerrs.go

@@ -47,4 +47,7 @@ var (
 	ErrInvalidCode              = errors.New("invalid code: must not begin with 0xef")
 	ErrNonceUintOverflow        = errors.New("nonce uint64 overflow")
 	ErrAddrProhibited           = errors.New("prohibited address cannot be sender or created contract address")
+	ErrGenerateBlockFailed      = errors.New("failed to generate block")


I'm trying to get rid of this package. Could we move this to a new package under plugin/evm so we don't mix ETH errors with avalanche vm errors?

renamed this file to coreerrs (which IMO makes more sense) and moved this to plugin/evm/vmerrors

I agree - very clear

utils/handler.go

sync/handlers/stats/stats.go

plugin/factory/factory.go

plugin/evm/atomic/atomictest/tx.go

plugin/evm/atomic/sync/atomic_summary_provider.go

plugin/evm/atomic/tx.go

plugin/evm/vm.go

plugin/evm/extension/config.go

…mmit

coreerrors/errors.go

plugin/evm/vmerrors/errors.go

core/vm/evm.go

plugin/evm/atomic/state/atomic_backend.go

plugin/evm/vm_extensible.go

plugin/evm/vm_warp_test.go

plugin/evm/vm.go

plugin/evm/atomic/state/atomic_tx_repository.go

darioush

This change seems correct.
I think there is a lot of complexity in the initialize flow, which is a result of how we structured dependencies.

However, I think the tech debt of having two repositories is much greater than some added complexity to the VM initialization flow.

I will continue to look for some improvements but I think we should prioritize the repository merging then revisit cleaning up Initialize and improving the innerVM and wrapperVM abstractions.

In any case, in its current state, this would need a README.md that explains what's going on and which structs need to go through initialization prior to use and in which order.

darioush · 2025-02-12T13:59:08Z

plugin/evm/vm_extensible.go

+)
+
+/*
+ // All these methods assumes that VM is already initialized


nit: I think we should move SetExtensionConfig above this comment.
Also not sure if you prefer the block comment /**/ here, seems we can just use //

darioush · 2025-02-12T14:01:22Z

plugin/evm/vm.go

-	}
-
-	// initialize peer network
-	if vm.p2pSender == nil {


this was used in some testing, perhaps check with @joshua-kim

plugin/evm/vm.go

darioush · 2025-02-12T14:07:38Z

plugin/evm/block.go

-			_ = atomicState.Reject() // ignore this error so we can return the original error instead.
-		}
+	if b.extension != nil && (err != nil || !writes) {
+		b.extension.OnError(b)


I know I originally requested to call this OnError, but it's also called on !writes,
so perhaps a name like UnpinVerified() or something else is better.

I think it might make sense to move the call to SemanticVerify on extensions to this fn too (so all the extension calls are in a single place).

I think unpinning is specific to atomic implementation. This should probably be called CleanupVerified (since we actually cleanup everytime if !writes regarding of error)

darioush · 2025-02-12T14:10:57Z

plugin/evm/atomic/vm/atomic_leaf_handler.go

+type uninitializedHandler struct{}
+
+func (h *uninitializedHandler) OnLeafsRequest(ctx context.Context, nodeID ids.NodeID, requestID uint32, leafsRequest message.LeafsRequest) ([]byte, error) {
+	return nil, errUninitialized


Would it be possible for this code path to be called? if so we shouldn't return an error since it would be fatal

Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

…atomic-vm-refactor

ceyonur and others added 12 commits January 20, 2025 18:47

wrap atomic vm

ae80881

set network codec

f75f052

unexport inner evm

96eb3ed

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

84ddc34

Co-authored-by: Quentin McGaw <quentin.mcgaw@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

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

7ba0e38

Co-authored-by: Quentin McGaw <quentin.mcgaw@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

Update plugin/evm/vm.go

1b5c84e

Co-authored-by: Quentin McGaw <quentin.mcgaw@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

Update plugin/evm/vm.go

90e248c

Co-authored-by: Quentin McGaw <quentin.mcgaw@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

nits

28702f3

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

d3c9753

…tomic-vm-wrapper

revert extra change

75f6b0e

add extension config

d2a2418

move with interfaces

1dcfa22

ceyonur changed the base branch from master to atomic-vm-wrapper January 28, 2025 16:07

ceyonur added 6 commits February 4, 2025 16:06

use visitors

52d78cd

fix tests

c61e190

fix linter

6776c19

move to factory pkg

f9f54ef

change main function

b754ffa

bump avago

58a2bb8

ceyonur changed the base branch from atomic-vm-wrapper to seperate-atomic-pkg-base February 6, 2025 00:06

ceyonur and others added 3 commits February 6, 2025 03:33

Merge branch 'seperate-atomic-pkg-base' into atomic-vm-refactor

92f1abe

refactor: unexport commit from atomic trie

f16b85b

few nits

54e8801

ceyonur commented Feb 7, 2025

View reviewed changes

ceyonur added 2 commits February 7, 2025 20:32

fix ext config validate

ce08bac

update releases.md

264ddbb

ceyonur commented Feb 7, 2025

View reviewed changes

ceyonur marked this pull request as ready for review February 7, 2025 20:28

ceyonur requested a review from darioush as a code owner February 7, 2025 20:28

ceyonur requested a review from a team as a code owner February 7, 2025 20:28

darioush reviewed Feb 8, 2025

View reviewed changes

darioush and others added 5 commits February 7, 2025 18:43

Merge branch 'atomic-vm-refactor' into atomic-vm-refactor-unexport-co…

626b319

…mmit

refactor: unexport commit from atomic trie (#782)

4f69bfc

rename vmerrs to coreerors/move vmerrors to plugin

c388601

revert format changes

7f9b894

reviews

d54a539

darioush reviewed Feb 11, 2025

View reviewed changes

ceyonur and others added 10 commits February 11, 2025 17:02

nits and reviews

da82817

fix unsaved changes

90893c2

set block eth upgrades

f9f4824

enable dao in test chain configs

6145546

simplify: remove blockManager type (#786)

c372ec8

remove block manager and move to block

f4b2a10

bump avalanchego dep

9d7bfc7

bump e2e version

3eb68c6

Merge branch 'atomic-vm-refactor' into remove-block-manager

9a5e7d3

remove block manager and move to block (#787)

7dcfc6a

darioush reviewed Feb 12, 2025

View reviewed changes

ceyonur and others added 6 commits February 12, 2025 23:13

Update plugin/evm/vm.go

a0767d3

Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

Update plugin/evm/vm.go

4634917

Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

rename onerror

a30fae0

move set extension fn above

7a79b9e

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

d6f4ca1

…atomic-vm-refactor

reorder/fix semantic verify

d83860b

darioush mentioned this pull request Feb 13, 2025

cleanup: chain config changes for similarty with subnet-evm #798

Merged

ceyonur added 4 commits February 14, 2025 23:09

nits

b6bc1bb

remove unused err

314f447

Merge branch 'seperate-atomic-pkg-base' into atomic-vm-refactor

e82419f

reintroduce ap3 check in tip

950e83a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atomic vm refactor #768

Atomic vm refactor #768

ceyonur commented Jan 28, 2025 •

edited

Loading

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025 •

edited

Loading

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025 •

edited

Loading

darioush Feb 12, 2025

alarso16 Feb 13, 2025

ceyonur Feb 14, 2025

ceyonur Feb 7, 2025

ceyonur Feb 7, 2025

darioush Feb 7, 2025

ceyonur Feb 10, 2025

alarso16 Feb 13, 2025

darioush left a comment

darioush Feb 12, 2025

darioush Feb 12, 2025

darioush Feb 12, 2025

ceyonur Feb 12, 2025

darioush Feb 12, 2025

Atomic vm refactor #768

Are you sure you want to change the base?

Atomic vm refactor #768

Conversation

ceyonur commented Jan 28, 2025 • edited Loading

Why this should be merged

How this works

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

ceyonur Feb 7, 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

ceyonur Feb 7, 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

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 left a 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

ceyonur commented Jan 28, 2025 •

edited

Loading

ceyonur Feb 7, 2025 •

edited

Loading

ceyonur Feb 7, 2025 •

edited

Loading