vm, client: continuing v6 breaking changes #1815

ryanio · 2022-03-24T21:25:12Z

This PR accomplishes VM breaking change tasks listed in #1717:

Remove deprecated state (Trie) constructor option
Remove deprecated generateTxReceipt function in runBlock (preferring newer version located in runTx), remove Receipt re-exports
Remove StateManager.getStateRoot() force=true parameter (also in interface)
Removed deprecated EIP2930Receipt and EIP1559Receipt types
Do gasUsed deduplication Difference between gasUsed and execResult.gasUsed #1446 (comment)
- Moved EVMResult.gasUsed to RunTxResult where it is added to there, and execResult.gasUsed remains the same
Update BaseTxReceipt gasUsed from Buffer to BigInt (suggestion added by @ryanio)
runCode throws if gasLimit is undefined: the gasLimit parameter is mandatory
Move gasRefund to a tx-level result object instead of ExecResult todo link
- Moved one level up to EVMResult, since it does not seem appropriate (or used) in execResult

codecov · 2022-03-24T21:29:32Z

Codecov Report

Merging #1815 (702dc22) into develop (0d44c79) will decrease coverage by 1.01%.
The diff coverage is 84.28%.

❗ Current head 702dc22 differs from pull request most recent head 045cfb4. Consider uploading reports for the commit 045cfb4 to get more accurate results

Flag	Coverage Δ
block	`85.17% <ø> (?)`
blockchain	`84.13% <ø> (?)`
client	`77.20% <100.00%> (?)`
common	`94.94% <ø> (?)`
devp2p	`82.83% <ø> (ø)`
ethash	`90.71% <ø> (?)`
trie	`80.27% <ø> (?)`
tx	`92.48% <ø> (?)`
util	`89.45% <ø> (?)`
vm	`82.53% <81.96%> (?)`

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

holgerd77 · 2022-03-25T08:35:54Z

Force-pushed on this with the cherry-picked relevant commits.

holgerd77 · 2022-03-25T09:27:31Z

Not sure why CI is stuck here, otherwise I would give this a go!

ryanio · 2022-03-25T19:53:00Z

Not sure why CI is stuck here, otherwise I would give this a go!

thanks, looks good now!

holgerd77

LGTM

ryanio · 2022-03-25T20:38:52Z

ok, noticed some last few things going over the diff again, this should be ready to merge now, thanks @holgerd77 !!!

acolytec3

Looks great, and probably a long slog. I left one comment about an export that isn't directly related to the overall project of breaking changes but one we should consider.

acolytec3 · 2022-03-29T01:19:46Z

packages/client/lib/net/protocol/ethprotocol.ts

@@ -6,7 +6,8 @@ import {
  BlockBodyBuffer,
 } from '@ethereumjs/block'
 import { TransactionFactory, TypedTransaction } from '@ethereumjs/tx'
-import { bigIntToBuffer, bufferToBigInt, bufferToInt, intToBuffer, rlp } from 'ethereumjs-util'
+import { encodeReceipt } from '@ethereumjs/vm/dist/runBlock'


Is there an easy way to move encodeReceipt to a package level export since we're using this outside of the vm package and this import looks kinda hideous, maybe in a new util.ts since this looks like a pure function? This could be a follow-up as part of a more general overhaul of the vm package getting away from default exports and definitely shouldn't hold up merging this.

yeah, good eye, this isn't as scary as it looks (we access vm/dist in a few other places) but definitely a good point to ease access a bit, i think this is likely what holger was aiming with this breaking releases task:

Restructure/sort VM exports (directly expose Bloom, other?, unified type location, type/interface exposure, index.ts -> vm.ts+ index.ts only for exports

let's target as a follow-up PR, since this one is already kind of involved, we can hone in on it separately

these are our current vm/dist uses on master:

StateManager uses might be solved with the current refactor to separate it from the vm

holgerd77 · 2022-04-02T08:16:33Z

Rebased this and fixed some small conflicts along.

holgerd77 · 2022-04-02T08:22:19Z

packages/vm/src/index.ts

@@ -288,7 +283,7 @@ export default class VM extends AsyncEventEmitter {
    if (opts.stateManager) {
      this.stateManager = opts.stateManager
    } else {
-      const trie = opts.state ?? new Trie()
+      const trie = new Trie()


Side note, not directly related: I guess it would rather make sense, to make trie optional for the StateManager options and then do this trie instantiation there if not provided.

that makes sense, i should have checked, we already do that :) so i can just remove this, i will make a note for myself to do in a follow up develop PR

holgerd77

Looks great, again, thanks a lot for all the code clean-ups! 😄

* vm: v6 breaking changes * update benchmark util to use gasUsed as bigint * VM: fixed gasUsed rebase related bug Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com>

ryanio added package: vm package: client breaking target: develop labels Mar 24, 2022

ryanio mentioned this pull request Mar 24, 2022

v6 Breaking Release Planning #1717

Closed

51 tasks

holgerd77 force-pushed the develop branch from 3e981f1 to 0084770 Compare March 25, 2022 08:26

holgerd77 force-pushed the vm-v6-breaking-changes branch from 50a8612 to 4cc3a7f Compare March 25, 2022 08:35

ryanio closed this Mar 25, 2022

ryanio reopened this Mar 25, 2022

holgerd77 previously approved these changes Mar 25, 2022

View reviewed changes

ryanio dismissed holgerd77’s stale review via 916b680 March 25, 2022 20:33

acolytec3 previously approved these changes Mar 29, 2022

View reviewed changes

ryanio dismissed acolytec3’s stale review via 702dc22 March 30, 2022 20:45

ryanio requested review from holgerd77 and acolytec3 April 1, 2022 14:22

ryanio and others added 3 commits April 2, 2022 10:15

vm: v6 breaking changes

6cf40a3

update benchmark util to use gasUsed as bigint

78654c2

VM: fixed gasUsed rebase related bug

045cfb4

holgerd77 force-pushed the vm-v6-breaking-changes branch from 702dc22 to 045cfb4 Compare April 2, 2022 08:16

holgerd77 changed the title ~~vm: continuing v6 breaking changes~~ vm, client: continuing v6 breaking changes Apr 2, 2022

holgerd77 reviewed Apr 2, 2022

View reviewed changes

holgerd77 approved these changes Apr 2, 2022

View reviewed changes

holgerd77 merged commit 156b516 into develop Apr 2, 2022

holgerd77 deleted the vm-v6-breaking-changes branch April 2, 2022 08:37

ryanio mentioned this pull request Apr 5, 2022

develop: ci fix, some last backwards compatibility removals #1834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm, client: continuing v6 breaking changes #1815

vm, client: continuing v6 breaking changes #1815

ryanio commented Mar 24, 2022 •

edited

Loading

codecov bot commented Mar 24, 2022 •

edited

Loading

holgerd77 commented Mar 25, 2022

holgerd77 commented Mar 25, 2022

ryanio commented Mar 25, 2022

holgerd77 left a comment

ryanio commented Mar 25, 2022

acolytec3 left a comment

acolytec3 Mar 29, 2022

ryanio Mar 29, 2022 •

edited

Loading

ryanio Mar 29, 2022 •

edited

Loading

holgerd77 commented Apr 2, 2022

holgerd77 Apr 2, 2022

ryanio Apr 3, 2022

holgerd77 left a comment

vm, client: continuing v6 breaking changes #1815

vm, client: continuing v6 breaking changes #1815

Conversation

ryanio commented Mar 24, 2022 • edited Loading

codecov bot commented Mar 24, 2022 • edited Loading

Codecov Report

holgerd77 commented Mar 25, 2022

holgerd77 commented Mar 25, 2022

ryanio commented Mar 25, 2022

holgerd77 left a comment

Choose a reason for hiding this comment

ryanio commented Mar 25, 2022

acolytec3 left a comment

Choose a reason for hiding this comment

acolytec3 Mar 29, 2022

Choose a reason for hiding this comment

ryanio Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

ryanio Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

holgerd77 commented Apr 2, 2022

holgerd77 Apr 2, 2022

Choose a reason for hiding this comment

ryanio Apr 3, 2022

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

ryanio commented Mar 24, 2022 •

edited

Loading

codecov bot commented Mar 24, 2022 •

edited

Loading

ryanio Mar 29, 2022 •

edited

Loading

ryanio Mar 29, 2022 •

edited

Loading