VM, Blockchain: fix storing unsettled promises #1201

ryanio · 2021-04-12T23:44:54Z

This PR resolves storing unsettled promises in VM, Blockchain by preferring to use VM.create and Blockchain.create instead of new VM and new Blockchain as this lets us safely run async init code.

VM: mcl is now initialized inside VM.init() and has no stored promise.
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.

codecov · 2021-04-12T23:46:36Z

Codecov Report

Merging #1201 (3473e72) into master (e2a0a4d) will decrease coverage by 0.06%.
The diff coverage is 88.46%.

❗ Current head 3473e72 differs from pull request most recent head 2b91fe5. Consider uploading reports for the commit 2b91fe5 to get more accurate results

Flag	Coverage Δ
block	`86.73% <ø> (+0.13%)`	⬆️
blockchain	`82.90% <85.71%> (-0.53%)`	⬇️
client	`83.79% <100.00%> (-0.26%)`	⬇️
common	`94.39% <ø> (ø)`
devp2p	`83.64% <ø> (+0.31%)`	⬆️
ethash	`82.83% <ø> (ø)`
tx	`88.36% <ø> (+0.11%)`	⬆️
vm	`79.19% <75.00%> (-0.15%)`	⬇️

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

holgerd77 · 2021-04-20T21:30:38Z

This PR has somewhat conflicting readyness settings, what's the status?

ryanio · 2021-04-26T20:30:19Z

I don't think I can make any of the changes I would have liked in this PR without any breaking changes, since moving from stored to unstored promises is a breaking change itself (for example if anyone was using or relying on them).

I think we can put this PR on the back burner until we are ready to merge breaking changes for v6 where we can make some broader changes and refactor the async init code.

holgerd77 · 2021-04-27T07:36:35Z

@ryanio ok, have added to the v6 planning notes and added a "blocked" label here.

* make constructor private (prefer use of async `Blockchain.create`) * update usage, tests * move types to dedicated file

* make constructor private (prefer use of async `VM.create`) * update usage, tests

ryanio · 2021-08-13T23:24:49Z

Phew, rebased this, that was not trivial :)

Holger I know you decided against this approach since private constructors block inheritance, so I am going to think more about it, maybe we just make them deprecated and note they are not private for the reason of inheritance but their use is strongly discouraged over the async methods. (edit: ok I undid the private constructor and made sure both had deprecation messages)

holgerd77 · 2021-08-16T09:46:31Z

Holger I know you decided against this approach since private constructors block inheritance, so I am going to think more about it, maybe we just make them deprecated and note they are not private for the reason of inheritance but their use is strongly discouraged over the async methods. (edit: ok I undid the private constructor and made sure both had deprecation messages)

That's what I also suggested yeah, but I have to admit that I am also still unsure. On the other hand it would be a huge win to have this somewhat ensured that devs are not unsafely calling into this. But I do am somewhat anxious on this that we might have not considered all use cases here and might put some unpractical restrictions on users by doing so.

Some additional thoughts I had here:

We can actually also use protected instead of private. This might generally be the better fit, not sure why I didn't think about this yet. Just tested and works as expected:

This change is actually a type "we-can-try-and-see-if-we-get-through-with-it" change. 😜 So on a breaking release it is actually both safe to introduce as well as easy to later remove. So if we are at least somewhat convinced that this is a good thing and assume that the positive outcome overweight the negatives we can actually try (then eventually version 1. with protected) and - while not pretty - we can still revert in some worst case scenario of a substantial amount of substantial negative community feedback.

holgerd77 · 2021-08-16T09:52:04Z

packages/blockchain/src/types.ts

+   * provided from the `common` will be used.
+   */
+  genesisBlock?: Block
+}


Where is this huge type file coming from? Is this a forgotten artifact from the interface PR which also happened around that time? 🤔

ah no I was just moving them from index.ts because it was getting so long. I will undo this though because I see the benefit of keeping it along with the source code now.

Ah, I guess on Blockchain we likely also want to restructure the exports and use the index.ts file just for exporting things and move code and types to dedicated files, a bit as we are planning for the VM (see #1024 VM TODOs).

If you agree respectively have no reservations you might directly want to do in this PR since this is breaking already?

ryanio · 2021-08-16T19:27:38Z

oh nice, protected seems great. I will add that.

alcuadrado · 2021-08-16T22:12:32Z

IMO protected constructors with async create can be great, as long as all the operation of the class are async or it's expected to be used in an async context. It can lead to much cleaner code, while it may lead to the consumer having to do the lazy init() thing, but I guess is not probable and ok in this context.

ryanio · 2021-08-16T23:14:59Z

@alcuadrado great, thanks!

that's weird, typescript is erroring out (on local and ci) when I added protected in 2b91fe5, I tried to google around and couldn't find an answer to this, I tried upgrading typescript to latest to see if there were any fixes as well and still same error:

> @ethereumjs/vm@5.5.2 build
> ../../config/cli/ts-build.sh

[Node build] Using tsconfig.prod.json
> tsc --build ./tsconfig.prod.json
[Node build] Working...
/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:35107
                else if (type.symbol.flags & (32 | 64)) {
                                     ^
TypeError: Cannot read property 'flags' of undefined
    at getBaseTypes (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:35107:38)
    at typeHasProtectedAccessibleBase (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:49112:29)
    at typeHasProtectedAccessibleBase (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:49140:20)
    at isConstructorAccessible (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:49157:25)
    at resolveNewExpression (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:49085:22)
    at resolveSignature (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:49357:28)
    at getResolvedSignature (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:49375:26)
    at checkCallExpression (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:49441:29)
    at checkExpressionWorker (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:51593:28)
    at checkExpression (/Users/ryanghods/dev/ethereumjs-monorepo/packages/vm/node_modules/typescript/lib/tsc.js:51510:38)

holgerd77 · 2022-01-11T11:49:15Z

Am I correct that we would now redo (respectively: re-point) this towards develop and - also - that this actually hasn't been reopened (and eventually already merged) towards develop yet?

ryanio · 2022-01-11T19:30:58Z

this hasn't been updated or merged into anything yet, a good candidate i can pick up when we are ready to bring it in :)

ryanio · 2022-03-11T18:14:13Z

I will pick this up next week and start fresh with a new PR targeting develop.

ryanio added PR state: needs review package: blockchain package: vm labels Apr 12, 2021

ryanio force-pushed the fix-unsettled-promises branch from a5eb812 to 902add9 Compare April 12, 2021 23:53

ryanio added PR state: WIP and removed PR state: needs review labels Apr 26, 2021

holgerd77 mentioned this pull request Apr 27, 2021

Breaking Releases - Meta Issue (VM v6, others) (old) #1024

Closed

50 tasks

holgerd77 added the PR/Issue state: blocked label Apr 27, 2021

holgerd77 added the breaking label Jun 29, 2021

ryanio linked an issue Aug 13, 2021 that may be closed by this pull request

Unsettled promises shouldn't be stored. #1181

Closed

ryanio added 3 commits August 13, 2021 15:14

blockchain:

7383c77

* make constructor private (prefer use of async `Blockchain.create`) * update usage, tests * move types to dedicated file

vm:

a7fedd8

* make constructor private (prefer use of async `VM.create`) * update usage, tests

fixes from rebase, lint

a0e000c

ryanio force-pushed the fix-unsettled-promises branch from 902add9 to a0e000c Compare August 13, 2021 23:22

ryanio added 2 commits August 13, 2021 16:41

undo private constructors, update docs

60f0086

update new VM use to create

3abb33b

ryanio force-pushed the fix-unsettled-promises branch from 3b211e1 to 0651191 Compare August 14, 2021 00:15

make vm.copy() async, other misc. fixes

3473e72

ryanio force-pushed the fix-unsettled-promises branch from 0651191 to 3473e72 Compare August 14, 2021 00:30

holgerd77 reviewed Aug 16, 2021

View reviewed changes

move interface back to index.ts

61e06cd

add protected to vm and blockchain constructors

2b91fe5

holgerd77 mentioned this pull request Feb 16, 2022

v6 Breaking Release Planning #1717

Closed

51 tasks

ryanio mentioned this pull request Mar 24, 2022

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

Merged

ryanio closed this Mar 24, 2022

holgerd77 deleted the fix-unsettled-promises branch March 24, 2022 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VM, Blockchain: fix storing unsettled promises #1201

VM, Blockchain: fix storing unsettled promises #1201

ryanio commented Apr 12, 2021

codecov bot commented Apr 12, 2021 •

edited

Loading

holgerd77 commented Apr 20, 2021

ryanio commented Apr 26, 2021

holgerd77 commented Apr 27, 2021

ryanio commented Aug 13, 2021 •

edited

Loading

holgerd77 commented Aug 16, 2021

holgerd77 Aug 16, 2021 •

edited

Loading

ryanio Aug 16, 2021

holgerd77 Aug 17, 2021

ryanio commented Aug 16, 2021

alcuadrado commented Aug 16, 2021

ryanio commented Aug 16, 2021

holgerd77 commented Jan 11, 2022

ryanio commented Jan 11, 2022

ryanio commented Mar 11, 2022

VM, Blockchain: fix storing unsettled promises #1201

VM, Blockchain: fix storing unsettled promises #1201

Conversation

ryanio commented Apr 12, 2021

codecov bot commented Apr 12, 2021 • edited Loading

Codecov Report

holgerd77 commented Apr 20, 2021

ryanio commented Apr 26, 2021

holgerd77 commented Apr 27, 2021

ryanio commented Aug 13, 2021 • edited Loading

holgerd77 commented Aug 16, 2021

holgerd77 Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

ryanio Aug 16, 2021

Choose a reason for hiding this comment

holgerd77 Aug 17, 2021

Choose a reason for hiding this comment

ryanio commented Aug 16, 2021

alcuadrado commented Aug 16, 2021

ryanio commented Aug 16, 2021

holgerd77 commented Jan 11, 2022

ryanio commented Jan 11, 2022

ryanio commented Mar 11, 2022

codecov bot commented Apr 12, 2021 •

edited

Loading

ryanio commented Aug 13, 2021 •

edited

Loading

holgerd77 Aug 16, 2021 •

edited

Loading