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

VM/SM: Bundle Optimizations (Default wo Caches+EVMMockBlockchain, SM Code put() Fix, VerkleSM out, runTx()+Code Opts) #3601

Merged
merged 25 commits into from
Aug 19, 2024

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Aug 17, 2024

Closes #3575, #3574, #3573, partly addresses #3576

Ok, so this issue takes a wholistic view on the VM optimizations, going off from the examples/runTx.ts example (respectively: taking this as a reference point, (most) optimizations will be effective library-wide).

Bundle size of the VM (respecively: the example) is getting down from 1.35 MB to 999.95 KB when all changes applied! 🎉

Going through the changes here one-by-one (again, sorry for having one PR for this, but would not have time to split up before holidays, also, changeset for single changes is not so big):

1. Default VM without Caches

Commit range: 57d382f..57cad15
Bundle Changes: SM (within VM) 156.5 KB -> 77.7 KB, VM 1.35 MB -> 1.25 MB

This change sets the VM to work without SM caches by default. This was a bit tricky (Gabriel also tested, but had a lot of test failures), since it turned out that our Code SM-part was just not "survivable" without the caches initialized since the no-Caches using put() method was just not doing anything at all! 😂 This is an important bug fix also done along this PR range. Then it was relatively easily possible to update!

@gabrocheleau So this is the change I mentioned in #3604 as a replacement. Sorry for being a bit harsh and directly closing your SimpleStateManager PR, but I also played around with the other version a bit and gave this some additional thought and deeper look and I think this evolved approach is just clearly the better solution, since it also has a substantial tree shaking effect while - if we would replace with simple SM - we would just taking a better-on-the-one-side-worse-on-the-other solution. So we would only waste time with an additional debate here!

2. Use EVMMockBlockchain(Interface) as default for the VM

Commit range: b903ad4..1692654
Bundle changes: Blockchain 116 KB -> 0 KB, VM 1.25 MB -> 1.11 MB

This is the most profound change with the strongest effect on bundle. So this removes the default blockchain in the VM and replaces with a (a bit) extended dummy one from the EVM. The mock blockchain there is a bit more expressively named to avoid confusion.

This has some effect on typing for the cases where the blockchain is drawn/picked out of the VM and then being done something with the blockchain, this can be seen in some test adjustments which became necessary in the commit range for these changes. My argument here is that these use cases are generally rare (only a tiny fraction of tests affected) and that this is generally not a good coding pattern which is even good to discourage a bit. So if one wants to work with a blockchain, do genesis generation, whatever, one should just not pick the black blox blockchain from VM but initialize ones own (which can very simply be done).

Otherwise this change has no effects on the functionality of the VM package (which is great! 🤩).

3. StatelessVerkleStateManager out of VM

Commit range: 83db446..78939cb
Bundle changes: SM 70 KB -> 39 KB, VM 1.11 MB -> 1.07 MB

This change removes all the casting to StatelessVerkleStateManager in VM (and client) and instead makes the uses Verkle API methods explicit by adding them as optional methods to the SM interface. This is generally a good thing since goes back to a unified SM interface (or is at least an in between step, but now we just have all methods used "at hand" and have the full overview over the interface).

Regarding tree shaking this has the effect that in the VM the StatelessVerkleStateManager can now be tree-shaked out! 😄

4. runTx(): Default Block Usage / "Less Block" in General

Commit range: 6a803a1..599bcca
Bundle changes: VM 1.07 MB -> 1.03 MB

Drawing in all the tx type code in VM.runTx() (so: to have the 1559 code (and all other types) in if you want to run a legacy tx) actually came from the default block initialization with createBlock(), who would have thought? 🤯 (and not the typing for the TypedTransaction type, cc @ScottyPoi ). createBlock() does need the ability to create txs of all types (if there are any in), and hence all code needs to be there.

I first-round solved this by adding a new simple createEmptyBlock() constructor which does the job but I have replaced with a more elegant (or at least: even more effective) in round 2. I think we can keep the simple createEmptyBlock() constructor nevertheless, it has very few code lines, has a test and might be useful for other use cases (so this is generally superior to createBlock() with no data passed and does the same creation).

Second round solution is to fully replace the default block there with a default header, since this is already enough and then additionally saves all the block code (so: for the Block class) for tree shaking! 🙂

5. Various Code Optimizations

Commit range: 8aa1e27..b1369d0
Bundle changes: VM 1.03 MB -> 999.95 KB

Ok, I was now so close to the 1 MB mark that I absolutely wanted to get under 😂 so I did a round of additional micro-code optimizations. I nevertheless tried to do structurally useful things which generally improve the code. If you are totally unhappy with some subset of changes let me know, we can definitely also delete 1-2 "too much" commits if needed! (the EOF stuff got a little less readable, I would still think it's still ok but we can also take out. And with the EOF comments I got fully desperate 😂 (it was already pretty late), but I also do think here that the comments were a bit too verbose anyhow and a bit more compact version is a bit better to read. Both not changes I would loose a fight over though. 😂 ).

Ok. So: open for review! 🙂

Copy link

codecov bot commented Aug 17, 2024

Codecov Report

Attention: Patch coverage is 68.29268% with 13 lines in your changes missing coverage. Please review.

Project coverage is 66.87%. Comparing base (4470cc3) to head (6a803a1).
Report is 13 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 66.90% <100.00%> (-6.56%) ⬇️
blockchain 84.92% <ø> (?)
client ?
common 89.68% <ø> (?)
evm 64.13% <100.00%> (?)
statemanager 66.17% <100.00%> (?)
trie 52.05% <ø> (?)
tx 76.90% <ø> (-0.88%) ⬇️
util 70.20% <ø> (?)
vm 58.73% <50.00%> (?)
wallet ?

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

@holgerd77 holgerd77 changed the title VM: Bundle Optimizations VM/SM: Bundle Optimizations (Default without Caches + EVMMockBlockchain, SM Code put() Fix, Verkle SM out, runTx() Opts, Code Optimizations) Aug 19, 2024
@holgerd77 holgerd77 changed the title VM/SM: Bundle Optimizations (Default without Caches + EVMMockBlockchain, SM Code put() Fix, Verkle SM out, runTx() Opts, Code Optimizations) VM/SM: Default without Caches + EVMMockBlockchain, SM Code put() Fix, Verkle SM out, runTx() + Code Opts Aug 19, 2024
@holgerd77 holgerd77 changed the title VM/SM: Default wo Caches+EVMMockBlockchain, SM Code put() Fix, VerkleSM out, runTx()+Code Opts VM/SM: Bundle Optimizations (Default wo Caches+EVMMockBlockchain, SM Code put() Fix, VerkleSM out, runTx()+Code Opts) Aug 19, 2024
@holgerd77
Copy link
Member Author

Ah, and here for completeness also the final bundle image. So the interesting thing is actually what is missing. So: full blockchain is missing, (more or less) all caches code from StateManager, the stateless Verkle SM, the tx types (actually, most of: there are some still in (4844), but this has other (code) reasons, the block block.js code: 🙂

grafik

acolytec3
acolytec3 previously approved these changes Aug 19, 2024
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.

I'm going to cautiously approve this one, though a little ambivalent that we are getting close to the point where we're taking things too far (see my comment on readability vs compactness).

holgerd77 and others added 5 commits August 19, 2024 13:17
* monorepo: add cspell, add ALL unknown words to valid words

* cspell: split unknown words in ts/md

* filter out wrong words in cspell-ts.json

* cspell ignore hex values

* fix typos in all packages

* cspell: use cache

* cspell: update commands

* cspell: update md/ts words

* Typo fixes for README/CHANGELOG files

* cspell: ensure all relevant monorepo md files are checked

* ci: add cspell job

* cspell: update command

* temp add bogus to markdown

* remove bogus spell

* update ci name

* fix remaining typos + add words to cspell dict

* Update packages/client/CHANGELOG.md

* Update packages/util/CHANGELOG.md

* address review

* Remove almost all `cspell:ignore` (#3599)

* remove almost all cspell:ignore

* more spell changes

* cspell: fix problems

* evm: fix quadCoefficient

* cspell: fixes

* remove disable line

---------

Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>
acolytec3
acolytec3 previously approved these changes Aug 19, 2024

const verkleCrypto = await loadVerkleCrypto()
const sm = new StatelessVerkleStateManager({ verkleCrypto })
console.log(sm)
Copy link
Member

Choose a reason for hiding this comment

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

This file should likely be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove, accidentally committed

@@ -52,6 +51,8 @@ import type {
const debug = debugDefault('vm:tx')
const debugGas = debugDefault('vm:tx:gas')

const DEFAULT_HEADER = createBlockHeader()
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this import the block header in runTx? Why do we need this? Could we not instantiate this with the appropriate fields here, so we don't import the whole BlockHeader class? The header needs: coinbase, baseFeePerGas, gasLimit, getBlobGasPrice(), gasUsed (can mock this one in another PR if wanted)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is still drawing in the header (before block + all txs).

I experimented with fully removing in between, this turned out not to be feasible. Let's please not mock too much, this otherwise goes at the cost of robustness of the implementations. E.g. for getBlobGasPrice(), this is depending on various Common parameters (which might change along HFs). If we mock things like that (also: simpler header defaults), we introduce redundancy and unnecessary error opportunities.

This is going into the area where it is not worth it anymore.

const vmCopy = await vm.shallowCopy()
assert.isUndefined(
await vmCopy.stateManager.getAccount(address),
'non-committed checkpoints will not be copied',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure why this is deleted, am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

The behavior just changed here, non-committed checkpoints are now copied in the default case of non caches used (which is ok, the checkpointing is in this case fully done by the trie checkpointing mechanism).

Since this check is not testing a required property but just a it-was-like-this-in-this-case property which now changed I removed the test (the alternative would have been to setup a full with-caches-without-caches setup respetively introduce some differentiation here, which I totally found overblown - again, since this is just not testing something which needs to have the tested property.

Hope this is clear. 🙂

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

A few comments, but looks very good 👍 😄

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

LGTM (assuming CI passes)

@holgerd77 holgerd77 merged commit 9856f66 into master Aug 19, 2024
35 checks passed
@holgerd77 holgerd77 deleted the vm-statemanager-bundle-optimizations branch August 19, 2024 13:36
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.

VM: Replace Class-Referencing Tx Typing
3 participants