EVM, remove EEI: cleanup statemanager #2702

jochem-brouwer · 2023-05-15T15:26:23Z

Follow-up of #2649

This PR intends to remove all EVM-related logic from state manager, in particular:

Touched accounts
Warm addresses
Warm slots

This PR will also:

Optimize getOriginalContractStorage
Cleanup the EVMStateManagerInterface in common
Ensure EthersStateManager is compatible with EVMStateManagerInterface and is thus able to also run VM (@holgerd77 could you check this?)
- (fixes the TODO tests in the ethers state manager tests)

Super WIP

codecov · 2023-05-15T15:34:24Z

Codecov Report

Merging #2702 (25105e9) into master (238b1eb) will increase coverage by 0.35%.
The diff coverage is 88.73%.

Additional details and impacted files

Flag	Coverage Δ
block	`90.82% <ø> (ø)`
blockchain	`90.72% <ø> (ø)`
client	`86.82% <ø> (ø)`
common	`97.00% <100.00%> (-0.06%)`	⬇️
devp2p	`89.40% <ø> (+0.12%)`	⬆️
ethash	`∅ <ø> (∅)`
rlp	`?`
statemanager	`86.28% <100.00%> (+5.74%)`	⬆️
trie	`89.59% <ø> (-0.34%)`	⬇️
tx	`95.76% <ø> (ø)`
util	`81.13% <ø> (ø)`
vm	`81.07% <77.77%> (-0.30%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

jochem-brouwer · 2023-05-16T10:31:24Z

Will use this comment as a scratchpad otherwise I will lose my line of thinking, so ignore this comment.

For _accessedStorageReverted we only use this in generate access list, this should not be a Map, it should just be a Set and each time a frame is reverted, put all items on the _accessedStorageReverted. (Also most likely add a flag to disable this behavior since we only need it to generate the access list).

Maybe for the access list just change the behavior: keep track of a set, each time a new slot is touched (whether it is reverted or not) just put it on the access list, this simplifies it a lot.

gabrocheleau · 2023-05-30T15:56:57Z

packages/statemanager/test/vmState.spec.ts

    st.deepEqual(origRes, value)

    st.end()
  })

  t.test("getOriginalContractStorage should validate the key's length", async (st) => {
    try {
-      await (<any>stateManager).getOriginalContractStorage(address, new Uint8Array(12))
+      await (<any>stateManager).originalStorageCache.getOriginalContractStorage(


gabrocheleau · 2023-05-30T16:02:24Z

packages/vm/src/runTx.ts

-        state.addWarmedStorage(address, toBytes(storageKey))
+        this.evm.evmJournal.addPreWarmedSlot(accessListItem.address, storageKey, true)


I don't fully grasp the accessListItem logic of this code section, but is it intentional that we're refactoring addWarmedStorage to addPreWarmedSlot (since we have a addWarmedStorage method on the evmJournal)?

EDIT: nevermind, seems fine based on other comments.

I will explain at the call tomorrow, but the pre-warmed addresses and "normal" warmed addresses behave differently, in the sense that pre-warmed addresses can never be marked cold again, but "normal" warmed addresses can (if a call frame reverts).

gabrocheleau · 2023-05-30T16:13:33Z

packages/vm/test/api/runTx.spec.ts

@@ -474,7 +474,7 @@ tape('runTx() -> runtime behavior', async (t) => {
      await vm.runTx({ tx }) // this tx will fail, but we have to ensure that the cache is cleared

      t.equal(
-        (<any>vm.stateManager)._originalStorageCache.size,
+        (<any>vm.stateManager).originalStorageCache.map.size,


Is the ` necessary?

gabrocheleau · 2023-05-30T16:14:35Z

packages/vm/test/util.ts

-  ;(<any>state).touchedJournal.clear()
+  //;(<any>state).touchedJournal.clear()


Intentional?

Ah yes, in this method the state is setup via StateManager, not EVM (so there are no touched accounts). Therefore this cleanup action is not necessary. Should remove anyways.

holgerd77 · 2023-05-31T09:26:58Z

packages/statemanager/src/cache/originalStorageCache.ts

+  clearOriginalStorageCache(): void {
+    this.map = new Map()
+  }
+}


I would have a tendency to drastically shorten the names here, so just use get, put and clear. 😋

Think that makes the API more elegant and readable and what this does can be taken very much from the context, so e.g. this._stateManager.originalStorageCache.get(...) (still long enough 🙂).

holgerd77 · 2023-05-31T10:18:33Z

packages/evm/src/evmJournal.ts

+//AddressString | Map<AddressString, SlotString>
+type JournalHeight = number
+
+export class EvmJournal {


Here as well, I would suggest to just name Journal and also simplify the file name evmJournal.ts -> journal.ts.

Should I rename evm.evmJournal to evm.journal as well?

I would vote yes! 🙂

…eumjs-monorepo into cleanup-statemanager-v2

jochem-brouwer · 2023-05-31T20:14:38Z

Ok, I should have resolved most review items except if I should rename evm.evmJournal to evm.journal (I think I have the tendency to do so).

Furthermore, I have removed accountExists and accountIsEmptyOrNonExistent from the interface. However, accountExists is still in EthersStateManager, since this is rather educational: it shows how to know, from eth_getProof if an account exists in the trie or not. You cannot use eth_getAccount for this, since it will always return an empty account if the account does not exist in the trie, or it exists in the trie but is empty.

I have also cleaned up the method names of originalStorageCache.

jochem-brouwer · 2023-05-31T20:15:34Z

And possibly preWarmed -> alwaysWarm?

holgerd77 · 2023-05-31T20:21:07Z

And possibly preWarmed -> alwaysWarm?

Was very much a quick-shot 😋, I think I still find it substantially easier to grasp though, also on second thought (preWarmed can lead thoughts somewhat in a wrong direction).

jochem-brouwer · 2023-05-31T20:26:26Z

Yup, have addressed!

holgerd77 · 2023-06-01T07:12:15Z

Short in-between note: I now synced with this branch > block 1Mio with the client, no anomalies to detect. 🙂

holgerd77 · 2023-06-01T08:44:34Z

packages/evm/src/interpreter.ts

@@ -10,6 +10,7 @@ import { trap } from './opcodes'
 import { Stack } from './stack'

 import type { EVM, EVMResult } from './evm'
+import type { Journal } from './journal'
 import type { AsyncOpHandler, OpHandler, Opcode } from './opcodes'
 import type { Block, Blockchain, Log } from './types'
 import type { Common, EVMStateManagerInterface } from '@ethereumjs/common'


Debug logger from below, currently evm:eei:gas (so annying this untouched-lines-comment-deactivation), should be renamed.

Simply evm:gas?

Yes, evm:gas is perfect. I will do a quick check to see if there are any other eei leftovers.

jochem-brouwer · 2023-06-01T08:47:18Z

Short in-between note: I now synced with this branch > block 1Mio with the client, no anomalies to detect. 🙂

Cool! I also wonder slightly about performance, did you notice anything?

We could also do a quick test with disk reads before/after this PR. It should decrease (and hopefully a lot! :) )

holgerd77 · 2023-06-01T09:10:27Z

Short in-between note: I now synced with this branch > block 1Mio with the client, no anomalies to detect. 🙂

Cool! I also wonder slightly about performance, did you notice anything?

We could also do a quick test with disk reads before/after this PR. It should decrease (and hopefully a lot! :) )

Performance is always hard to grasp, so I am getting more and more cautious to do any early statements here.

Doing catch-up-execution right now (by deactivating block sync), at ~1.2 Mio (Homestead) atm, everything still running smoothly atm, though block execution has gotten a bit slower, but that's also always dependent of the respective usage of the respective block parts.

I wanted to sync into the "problematic ranges" again, that start in the ~ 1.6 Mio or so block numbers. These are always easier to compare, since base execution numbers for a single block are already pretty high.

Also not totally meaningful then (since single blocks/txs are often targeting very specfic EVM functionality). So at the end best is always to do 2-4 different ranges.

gabrocheleau · 2023-06-01T10:56:57Z

Ok, I should have resolved most review items except if I should rename evm.evmJournal to evm.journal (I think I have the tendency to do so).

Furthermore, I have removed accountExists and accountIsEmptyOrNonExistent from the interface. However, accountExists is still in EthersStateManager, since this is rather educational: it shows how to know, from eth_getProof if an account exists in the trie or not. You cannot use eth_getAccount for this, since it will always return an empty account if the account does not exist in the trie, or it exists in the trie but is empty.

I have also cleaned up the method names of originalStorageCache.

Nice, I really like the API simplifications!

acolytec3

Looks great with the additional edits!

jochem-brouwer added 5 commits May 15, 2023 17:03

evm: move touched accounts into evm from stateManager

d915d07

evm: use evmJournal commit/revert

6bd0c27

add deleteAccount

558cc6b

vm: fix build

73858dc

vm: fix test runner

95cd78a

jochem-brouwer added PR state: WIP package: vm type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. package: statemanager package: evm labels May 15, 2023

evmJournal: prepare for using maps to start using warm addresses/slots

600bee9

jochem-brouwer added 15 commits May 16, 2023 12:55

evmJournal: eip2929 preparations

2ba50a7

vm: fix build and logic

d8de4a7

stateManager: mark methods as not implemented

ee6dfba

stash work

fc7f6b8

evm: update journal logic [no ci]

003b8b7

evm: update journal

4f7e27d

vm/evm/common: update journaler

915fa48

evm: journal fixes

e846c93

evm/vm: journaling fixes regarding checkpoitn logic [no ci]

c9621c3

test setup remove me

97b65c9

Merge branch 'master' into cleanup-statemanager-v2

ce97405

evm: journal fix

a399392

evm: journal cleanup and fixes

9c81cd6

vm: fix state tests

2445acd

vm: add TODO

bbc888a

This was referenced May 23, 2023

v7 Breaking Release Planning #2561

Closed

Improved Optimism Bedrock Compatibility #2713

Open

gabrocheleau reviewed May 30, 2023

View reviewed changes

holgerd77 reviewed May 31, 2023

View reviewed changes

jochem-brouwer added 4 commits May 31, 2023 22:06

monorepo: address feedback, remove two methods from statemanager

34c62ce

evm: rename evmjournal to journal

040ac47

statemanager/evm/vm: rename originalStorageCache methods

ae93f52

Merge branch 'cleanup-statemanager-v2' of github.com:ethereumjs/ether…

7c4722b

…eumjs-monorepo into cleanup-statemanager-v2

Merge branch 'master' into cleanup-statemanager-v2

d66a318

jochem-brouwer requested review from holgerd77, acolytec3 and gabrocheleau May 31, 2023 20:15

evm/vm: rename evmJournal -> journal

abcf830

evm/vm: move prewarmed -> alwayswarm

5678c55

stateManager: fix tests

cd4a2c9

holgerd77 reviewed Jun 1, 2023

View reviewed changes

monorepo: remove eei leftovers

25105e9

acolytec3 approved these changes Jun 1, 2023

View reviewed changes

jochem-brouwer merged commit 02df7c8 into master Jun 1, 2023

holgerd77 deleted the cleanup-statemanager-v2 branch June 1, 2023 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM, remove EEI: cleanup statemanager #2702

EVM, remove EEI: cleanup statemanager #2702

jochem-brouwer commented May 15, 2023 •

edited

Loading

codecov bot commented May 15, 2023 •

edited

Loading

jochem-brouwer commented May 16, 2023

gabrocheleau May 30, 2023

gabrocheleau May 30, 2023 •

edited

Loading

jochem-brouwer May 30, 2023

gabrocheleau May 30, 2023

gabrocheleau May 30, 2023

jochem-brouwer May 30, 2023

holgerd77 May 31, 2023

holgerd77 May 31, 2023

jochem-brouwer May 31, 2023

holgerd77 May 31, 2023

jochem-brouwer commented May 31, 2023

jochem-brouwer commented May 31, 2023

holgerd77 commented May 31, 2023

jochem-brouwer commented May 31, 2023

holgerd77 commented Jun 1, 2023

holgerd77 Jun 1, 2023

jochem-brouwer Jun 1, 2023

jochem-brouwer commented Jun 1, 2023

holgerd77 commented Jun 1, 2023

gabrocheleau commented Jun 1, 2023

acolytec3 left a comment

		state.addWarmedStorage(address, toBytes(storageKey))
		this.evm.evmJournal.addPreWarmedSlot(accessListItem.address, storageKey, true)

		;(<any>state).touchedJournal.clear()
		//;(<any>state).touchedJournal.clear()

EVM, remove EEI: cleanup statemanager #2702

EVM, remove EEI: cleanup statemanager #2702

Conversation

jochem-brouwer commented May 15, 2023 • edited Loading

codecov bot commented May 15, 2023 • edited Loading

Codecov Report

jochem-brouwer commented May 16, 2023

Choose a reason for hiding this comment

gabrocheleau May 30, 2023 • 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

jochem-brouwer commented May 31, 2023

jochem-brouwer commented May 31, 2023

holgerd77 commented May 31, 2023

jochem-brouwer commented May 31, 2023

holgerd77 commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer commented Jun 1, 2023

holgerd77 commented Jun 1, 2023

gabrocheleau commented Jun 1, 2023

acolytec3 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented May 15, 2023 •

edited

Loading

codecov bot commented May 15, 2023 •

edited

Loading

gabrocheleau May 30, 2023 •

edited

Loading