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.

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

EVM/Monorepo: Verkle Decoupling #3462

Merged

Conversation

holgerd77
Copy link
Member

This PR works towards reducing the EVM bundle size by reducing the Verkle coupling.

The one direct casting to StatelessVerkleStateManager is removed by using the StateManager interface here by adding the Verkle methods as optional methods there. I would generally like to expand on this since it reduces the direct coupling to the SVSM class.

Additionally an interface AccessWitness is added (also to Common), which we can use instead of the direct import.

And, third measure: constants and methods not depending on Verkle cryptography have been moved to a dedicated verkle module in Util. This should make sense as well and was necessary in the EVM case, since it allowed to remove the @ethereumjs/verkle dependency completely.


I would think the above measures should "do the job". I could not yet prove it though. When running the esbuild command from #3446 (comment) build size stayed unexpectedly on the 2.6 MB level (for EVM).

Some googling brought up that esbuild just seems to not properly tree shake on unused imports from external modules, see e.g. evanw/esbuild#1794. πŸ€”

I discovered the --analyze=verbose flag along (full command: npm run build && esbuild --bundle 10EVM.mjs --outfile=out1.mjs --analyze=verbose) - generally useful - which helps to see where things are still drawn in:

   β”œ ../verkle/dist/esm/node/types.js ────────────────────────────────────────────────────── 226b ──── 0.0%
   β”‚  β”” ../verkle/dist/esm/node/index.js
   β”‚     β”” ../verkle/dist/esm/index.js
   β”‚        β”” ../statemanager/dist/esm/statelessVerkleStateManager.js
   β”‚           β”” ../statemanager/dist/esm/index.js
   β”‚              β”” dist/esm/evm.js
   β”‚                 β”” dist/esm/index.js
   β”‚                    β”” 10EVM.mjs

So this is for the @ethereumjs/verkle dependency, which should not be there anymore, and which brought me to googling for the esbuild issue.

I then started thinking about using another bundler (so: if esbuild might generally not deliver valid tree shaking results) trying rollup with the following:

npm i -g rollup
npm i -g @rollup/plugin-node-resolve
rollup --format=cjs --file=out1.mjs --plugin=node-resolve -- 10EVM.mjs

This gives a bunch of other warning and (at least) one error (where things break).

grafik

Not fully sure if it's worth to resume on that path (or try e.g. another bundler).

Maybe our tree shaking & bundling expert @roninjin10 can have a comment! πŸ™‚
rollup --format=cjs --file=out1.mjs --plugin=node-resolve -- 10EVM.mjs

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 98.82%. Comparing base (d24ca11) to head (e63ecf0).
Report is 29 commits behind head on master.

❗ Current head e63ecf0 differs from pull request most recent head 552b7d2

Please upload reports for the commit 552b7d2 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Ξ”
blockchain ?
common ?
genesis 99.98% <ΓΈ> (?)
statemanager ?
util ?
wallet 87.25% <ΓΈ> (?)

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

@@ -272,9 +271,7 @@ export class Interpreter {
const contract = this._runState.interpreter.getAddress()

if (
!(await (
this._runState.stateManager as StatelessVerkleStateManager
Copy link
Collaborator

Choose a reason for hiding this comment

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

This generally won't affect tree shaking. TypeScript types are always removed even if they are values or classes as long as they are used as a type as long as we use import type syntax

@roninjin10
Copy link
Collaborator

@holgerd77 wierd rollup didn't work but since you are already using vitest using vite is likely the easiest. I added a bundle analyzer setup you can use in this pr #3463

@holgerd77 holgerd77 force-pushed the verkle-statemanager-interface-and-evm-dependency-removal branch from 1b27cd8 to 32ab354 Compare June 18, 2024 20:27
@holgerd77 holgerd77 changed the title EVM: Replace direct Verkle / VSVSM Usages by Interfaces (Tree Shaking) EVM/Monorepo: Verkle Decoupling Jun 19, 2024
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

refactor looks superclean and modular. don't have insight into tree shaking however as of now, so for that will delegate to experts :)

@holgerd77
Copy link
Member Author

refactor looks superclean and modular. don't have insight into tree shaking however as of now, so for that will delegate to experts :)

This is still in the pre-tree-shaking era, so just removing dependencies so that they are not there. πŸ˜‚

* @return The tree key as a Uint8Array.
*/

export const getKey = (stem: Uint8Array, leaf: LeafType | Uint8Array) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note on a few of these helpers. They were initially named quite minimally getKey rather than getVerkleTreekey because the "verkle" was obvious from it being imported from the verkle package. I feel like we should likely rename these helpers more explicitly as "verkle" since they are now exported in utils (getKey could be so many things). We could perhaps do the same with VERSION_LEAF_KEY => VERKLE_VERSION_LEAF_KEY

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

Minor note on a few of these helpers. They were initially named quite minimally getKey rather than getVerkleTreekey because the "verkle" was obvious from it being imported from the verkle package. I feel like we should likely rename these helpers more explicitly as "verkle" since they are now exported in utils (getKey could be so many things). We could perhaps do the same with VERSION_LEAF_KEY => VERKLE_VERSION_LEAF_KEY

100% agree.

Copy link
Contributor

@gabrocheleau gabrocheleau left a comment

Choose a reason for hiding this comment

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

Looks great! as noted on the call, we will leave the verkle namespacing updates to a future call in order not to interfere with pending verkle work

@acolytec3 acolytec3 merged commit 1acacb8 into master Jun 19, 2024
33 of 34 checks passed
@gabrocheleau gabrocheleau deleted the verkle-statemanager-interface-and-evm-dependency-removal branch June 19, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants