VM, Blockchain, Client: fix storing unsettled promises (develop)

[ryanio]

This PR continues #1201 retargeted towards develop.

• VM
  • mcl is now initialized inside VM.init() and has no stored promise.
  • copy() made async to return VM.create() rather than new VM
• Blockchain
  • init is run during Blockchain.create() and has no stored promise.

Both classes have boolean _isInitialized and check internally to make sure they do not init more than once.

Sometimes in the client a Blockchain or VM is set up in a constructor. In this case, the init is added to the classes' open method.

[ryanio]

When making the blockchain or vm constructor protected, if using it in a non-async constructor (which we can later await init), I ran into this error, and not sure how to get around it, if someone wants to try and see if they can recreate or get something different:

TypeError: Cannot read properties of undefined (reading 'flags')
Occurred while linting /Users/rg/dev/ethereumjs-monorepo/packages/vm/src/index.ts:252
    at getBaseTypes (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:55660:42)
    at typeHasProtectedAccessibleBase (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:74675:29)
    at typeHasProtectedAccessibleBase (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:74704:20)
    at isConstructorAccessible (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:74723:25)
    at resolveNewExpression (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:74636:22)
    at resolveSignature (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:74964:28)
    at getResolvedSignature (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:74993:26)
    at checkCallExpression (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:75119:29)
    at checkExpressionWorker (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:77893:28)
    at checkExpression (/Users/rg/dev/ethereumjs-monorepo/node_modules/typescript/lib/typescript.js:77797:38)

index.ts:252 being:

this.blockchain = opts.blockchain ?? new Blockchain({ common: this._common })

[codecov]

Codecov Report

Merging #1811 (20134d6) into develop (0084770) will decrease coverage by 0.03%.
The diff coverage is 89.65%.

Flag	Coverage Δ
block	85.17% <ø> (ø)
blockchain	83.84% <85.71%> (-0.29%)	⬇️
client	77.21% <93.33%> (+<0.01%)	⬆️
common	94.94% <ø> (ø)
devp2p	82.96% <ø> (-0.14%)	⬇️
ethash	90.71% <ø> (ø)
trie	80.27% <ø> (ø)
tx	92.48% <ø> (ø)
util	89.45% <ø> (ø)
vm	81.96% <ø> (ø)

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

[acolytec3]

When making the blockchain or vm constructor protected, if using it in a non-async constructor (which we can later await init), I ran into this error, and not sure how to get around it, if someone wants to try and see if they can recreate or get something different:

I made the blockchain constructor protected and then changed the line you cited to:
this.blockchain = opts.blockchain ?? new (Blockchain as any)({ common: this._common })

and it seemed to work fine. Ran the VM api tests and everything passed without incident.

[holgerd77]

Hmm, I am not sure yet if I am a fan of making even the VM constructor protected and introduce an init() method there. All the examples I have every written for the VM (e.g. in the changelogs) are always using this direct instantiation.

Also from a UX perspective I have some doubts, then e.g. ts-node usage gets substantially more complicated (annoying) always having to deal with this no-top-level await stuff or otherwise do ugly any casts.

Can we let this sink in a bit more?

Maybe we leave the protected in general and just do (maybe a bit more prominent) warnings? 🤔 But also still totally on a 50-50 decision level.

[holgerd77]

(I also find our internal usage with selected any casts where it is necessary really ugly TBH)

[ryanio]

Hmm, I am not sure yet if I am a fan of making even the VM constructor protected and introduce an init() method there. All the examples I have every written for the VM (e.g. in the changelogs) are always using this direct instantiation.

We already had a vm init method as we are required to do async stuff in setup.

Also from a UX perspective I have some doubts, then e.g. ts-node usage gets substantially more complicated (annoying) always having to deal with this no-top-level await stuff or otherwise do ugly any casts.

Node allows top level await now https://v8.dev/features/top-level-await

Can we let this sink in a bit more?

Maybe we leave the protected in general and just do (maybe a bit more prominent) warnings? 🤔 But also still totally on a 50-50 decision level.

Yes the constructor is depercated so it should give substantive warnings to at least IDE users. And normal users wouldn't be able to use it without casting, so they should understand that they are doing something against convention.

(I also find our internal usage with selected any casts where it is necessary really ugly TBH)

We only use it in a couple of constructors, out of hundreds of other uses throughout the repo and tests.

[holgerd77]

Ok, going along with all.

[holgerd77]

LGTM, this look great, also thanks a lot for all these clean-ups in the PR, this always gives me re-newed confidence that we do not end up with some pile of chaotic code spaghetti at some point with all the work we are doing! 😍 🙏

So, for a last confirmation: we do not want to make Block, Tx constructors protected, since the constructors are only aliases to the fromData() static constructors, is this decided? Or is this still in the ring?`

[ryanio]

So, for a last confirmation: we do not want to make Block, Tx constructors protected, since the constructors are only aliases to the fromData() static constructors, is this decided? Or is this still in the ring?`

I think that's a separate topic, this PR was more focused on providing safe methods for async init. I think it's okay to leave those constructors public since people may use them in more direct ways, and I think that's okay with us, as long as they are strictly following the types there should be no issues (the from*Data methods do some helper conversion to the right types which is why they are more generally usable)