EVM/Common: SimpleStateManager #3482

holgerd77 · 2024-07-04T12:33:52Z

~~This is a proof of concept if it is possible~~ This PR implements a SimpleStateManager with the goal to replace the default state manager implementation and depdendency from the EVM with a very simple < 5KB state manager located in Common without any further depdencies.

This would allow to ship with a significantly trimmed down base EVM, both with a substantially reduced dependency set and bundle size, going from 1.23 MB to 850 KB.

Before:

After (also note the "number of boxes": statemanager out, trie out, js-sdsl out, readable-stream out, Buffer out, lru-cache out). So if you click to enlarge, this is so much more a representation of what the EVM is about than before:

This would ease browser usage for simple EVM use cases and integrations.

Some more comments to make here, short on time right now.

I have integrated this into the general SM test suite, all checkpoint tests are passing.

For the EVM this also already gives pretty solid results, with adhoc (without any modification) only 4 / 1107 tests failing.

holgerd77 · 2024-07-05T08:04:59Z

Ok, ready! 🎉

All EVM tests passing, the one thing I needed to do for this is to also move the OriginalStorageCache short implementation to Common and integrate, since otherwise the pre-state setting use/test cases were not working.

So: I think for the EVM this is a big deal! So this lifts a weight of close to 400KB to the EVM in addition to the other reductions, and so brings this package in the realm of becoming a "normal" browser library which people just side-integrate among other dependencies. The overall code adition "on the other side" is still < 8 KB. Beyond the code reduction this frees EVM users from all browser-related hazzles coming from the statemanager side of the dependency tree (if they can live with the limitations of this state manager).

Want to nevertheless make some more cases here respectively give some additional context/explanation about design choices, so to make things more transparent and "followable".

Why Common ?

So, this is admittedly relatively low in the stack. This was mainly guided by the following design "needs":

The implementation should implement the EVMStateManager interface being located in Common (also for the same "independent from the statemanager package reason)
Being "lower" than statemanager made it possible to relatively easily and without broader moving things around re-use the tests from statemanager and run them directly

We could also think about moving this to the EVM package directly instead. This would make 2. more complicated. I also find the idea relatively charming to just have a simple state manager available "everywhere" which can be used to manage Ethereum state on a basic level for whatever use cases, so for me it feels a bit as a general Utility which fits well into the chosen setting. Comparing the total Common bundle size of 178 KB this feels worth and in-relation to me to add. I would therefore pledge for the solution.

I think this new state manager will be generally useful, also for our own use cases, mainly e.g. regarding testing, stumbled upon this thought yesterday when seeing you guys battling with the DefaultStateManager bug. So with this simpler state manager makes it really handy to dig into state changes during EVM execution, since one can plainly see the state evolving in the simple map structures. Used this yesterday during a debugging session within VSCode, and this feels really handy to have cache/trie and so on out of the mix.

Anyhow: ready for review! 🙂

Note that this PR builds upon the BLS PR (for bundle comparison reasons). I think it's better to merge BLS first since these are conceptually unrelated.

jochem-brouwer

Some comments, LGTM in general (and indeed, also super simple yet elegant! 😄 )

For the state manager tests, some now run on the DefaultStateManager and the SimpleStateManager - but only the checkpoint tests. Are there other tests which we want to incorporate here?

Also, small side note, for code coverage the tests from StateManager -> Common will not get tracked, so our common coverage will likely go down, since likely (?) SimpleStateManager is not covered by the tests.

packages/common/src/state/simple.ts

packages/statemanager/README.md

packages/statemanager/src/cache/originalStorageCache.ts

acolytec3 · 2024-07-05T10:37:11Z

packages/common/src/state/simple.ts

+    if ((await this.getAccount(address)) === undefined) {
+      await this.putAccount(address, new Account())
+    }
+    await this.modifyAccountFields(address, { codeHash: keccak256(value) })


I do not have a super strong opinion on this but it seems like we should follow the same pattern we follow through the rest of the libraries where we allow users to pass in their own crypto and not require use of this keccak256 function (like we do here) in the DefaultStateManager.

Will do/integrate. First thought in terms of "simplification", but this might not be one actually. 😆

I was going to suggest the same and then saw Andrew's comment. I agree that it would be preferable not to "hardcode" this.

acolytec3

Awesome implementation, love the simpleness of this :-). That said, I've read through the explanation of why this should be in Common and I'm not tracking. This is only used in evm and nowhere else so doesn't make sense to me include this in common. What is the driving goal for not putting this in statemanager? If it implements the EVMstateManagerInterface, I don't see any reason that tree shaking wouldn't take care of any excess bundle size concerns if a user imports it from @ethereumjs/statemanager.

packages/common/src/state/originalStorageCache.ts

packages/common/src/state/simple.ts

gabrocheleau · 2024-07-05T13:55:45Z

packages/common/src/state/simple.ts

+    for (const [address, account] of newTopA) {
+      const accountCopy =
+        account !== undefined
+          ? Object.assign(Object.create(Object.getPrototypeOf(account)), account)


I believe that this would be more efficient:

Object.create(Object.getPrototypeOf(account), Object.getOwnPropertyDescriptors(account));

Since it avoids the intermediate step of creating an empty object and then copying properties over to it, and it ensures all properties, including non-enumerable ones, are copied.

gabrocheleau · 2024-07-05T14:00:57Z

packages/common/src/state/simple.ts

+  }
+
+  async getContractStorage(address: Address, key: Uint8Array): Promise<Uint8Array> {
+    return this.topS().get(`${address.toString()}_${bytesToHex(key)}`) ?? new Uint8Array(0)


Perhaps we could create a helper for storageKeyFormatting that parsed an address + key into the expected string? This could be helpful notably if we need to switch around the format (pre and post verkle comes to mind), then we don't have to modify it in multiple places.

Will keep for now, we can still easily do later on

packages/statemanager/README.md

gabrocheleau

Wow, that's some great work! Really like how this removes the statemanager package dependency from the evm. Left some minor comments.

…ependency (already in through Util dependency)

jochem-brouwer · 2024-07-08T11:23:10Z

The git diff is looking messed up 🤔 It tries to re-import the changes to BLS?

holgerd77 · 2024-07-08T11:45:26Z

The git diff is looking messed up 🤔 It tries to re-import the changes to BLS?

Yeah, I've cherry picked the commits out locally, will force-push

Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>

…nager

holgerd77 · 2024-07-12T18:00:50Z

packages/evm/src/types.ts

+   *
+   * The `@ethereumjs/statemanager` package provides a variety of state manager
+   * implementations for different needs (tree backed, RPC, experimental verkle)
+   * which can be used by this option as a replacement.


Can you please update the docs to now reflect the new SSM location?

holgerd77

Looks otherwise good, feel free to merge (also ok to spare out this one doc change from my side, just saw it's really this one very mention, can also update later, not sure if worth to trigger whole CI)

…nager

jochem-brouwer

3 small-ish comments

packages/statemanager/README.md

jochem-brouwer · 2024-07-12T19:03:18Z

packages/statemanager/src/simpleStateManager.ts

+    account.nonce = accountFields.nonce ?? account.nonce
+    account.balance = accountFields.balance ?? account.balance
+    account.storageRoot = accountFields.storageRoot ?? account.storageRoot
+    account.codeHash = accountFields.codeHash ?? account.codeHash


I just checked, even if account is returned undefined, and we provide the accountFields which is Partial<Account> this still works fine (we default the account to default values)

Is there a question here? I'm not sure what you're getting at?

packages/statemanager/src/stateManager.ts

jochem-brouwer

LGTM

holgerd77 added PR state: WIP type: refactor package: common package: statemanager package: evm labels Jul 4, 2024

holgerd77 changed the title ~~EVM: SimpleStateManager PoC~~ EVM/Common: SimpleStateManager Jul 5, 2024

holgerd77 added PR state: needs review and removed PR state: WIP labels Jul 5, 2024

jochem-brouwer reviewed Jul 5, 2024

View reviewed changes

acolytec3 reviewed Jul 5, 2024

View reviewed changes

gabrocheleau reviewed Jul 5, 2024

View reviewed changes

packages/common/src/state/originalStorageCache.ts Outdated Show resolved Hide resolved

gabrocheleau reviewed Jul 5, 2024

View reviewed changes

packages/common/src/state/simple.ts Outdated Show resolved Hide resolved

Base automatically changed from evm-js-default-for-bls-precompiles to master July 5, 2024 13:46

gabrocheleau reviewed Jul 5, 2024

View reviewed changes

packages/statemanager/README.md Outdated Show resolved Hide resolved

gabrocheleau reviewed Jul 5, 2024

View reviewed changes

holgerd77 added PR state: do-not-merge PR/Issue state: blocked labels Jul 8, 2024

holgerd77 added 9 commits July 8, 2024 12:57

Add SimpleStateManager implementation

8b3aa4c

Common SimpleStateManager fixes, add explicit ethereum-cryptography d…

1337780

…ependency (already in through Util dependency)

Rebuild package-lock.json

e952385

Integrate SimpleStateManager into existing checkpointing tests

7cb8886

Integrate into EVM, remove stateManager dependency

8ec735e

Rebuild packack-lock.json

a317fdb

Move OriginalStorageCache class to Common, deprecate old location

3d2fc30

Use full OriginalStorageCache implementation in SimpleStateManager

0bc4896

Add some docs

a2305a7

holgerd77 added 3 commits July 8, 2024 13:05

Rename topA, topC, topS to something more expressive, make protected

8237bf9

Rename add() -> checkpointSync(), make protected

6741648

More elegant commit() implementation

d7fdeb9

holgerd77 added 2 commits July 8, 2024 13:41

Add flexible keccak256

04e76db

Minor

2252759

holgerd77 force-pushed the add-simple-state-manager branch from d701dfa to 2252759 Compare July 8, 2024 11:46

scorbajio and others added 6 commits July 8, 2024 18:08

Update packages/common/src/state/simple.ts

45992c9

Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>

Merge remote-tracking branch 'origin/master' into add-simple-state-ma…

a9fe1c5

…nager

move simple to stateManager

c2d2909

add simple example

8c07625

lint

64609d8

Revert bundler config changes

688e64c

holgerd77 commented Jul 12, 2024

View reviewed changes

acolytec3 added 2 commits July 12, 2024 14:22

Merge remote-tracking branch 'origin/master' into add-simple-state-ma…

5fc9ed1

…nager

update docs

84e176f

jochem-brouwer reviewed Jul 12, 2024

View reviewed changes

acolytec3 added 2 commits July 12, 2024 15:11

address feedback

51c316f

address feedback

f5366b2

jochem-brouwer approved these changes Jul 12, 2024

View reviewed changes

jochem-brouwer merged commit ef20930 into master Jul 12, 2024
34 checks passed

jochem-brouwer deleted the add-simple-state-manager branch July 12, 2024 19:30

holgerd77 removed PR state: do-not-merge PR/Issue state: blocked labels Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM/Common: SimpleStateManager #3482

EVM/Common: SimpleStateManager #3482

holgerd77 commented Jul 4, 2024 •

edited

Loading

holgerd77 commented Jul 5, 2024 •

edited

Loading

jochem-brouwer left a comment

acolytec3 Jul 5, 2024

holgerd77 Jul 5, 2024

gabrocheleau Jul 5, 2024

holgerd77 Jul 8, 2024

acolytec3 left a comment

gabrocheleau Jul 5, 2024

gabrocheleau Jul 5, 2024

holgerd77 Jul 8, 2024

gabrocheleau left a comment

jochem-brouwer commented Jul 8, 2024

holgerd77 commented Jul 8, 2024

holgerd77 Jul 12, 2024

holgerd77 left a comment

jochem-brouwer left a comment

jochem-brouwer Jul 12, 2024

acolytec3 Jul 12, 2024

jochem-brouwer left a comment

EVM/Common: SimpleStateManager #3482

EVM/Common: SimpleStateManager #3482

Conversation

holgerd77 commented Jul 4, 2024 • edited Loading

holgerd77 commented Jul 5, 2024 • edited Loading

Why Common ?

jochem-brouwer 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

acolytec3 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

gabrocheleau left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Jul 8, 2024

holgerd77 commented Jul 8, 2024

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

holgerd77 commented Jul 4, 2024 •

edited

Loading

holgerd77 commented Jul 5, 2024 •

edited

Loading