fix: tx error strings should not contain multiple consecutive whitespace characters

[davidmurdoch]

519a076#diff-f5570026bc45fa69b273a873ea7ab1c4R93 caused an unintended breaking change by changing the output of the error strings.

Also, I couldn't test this because I couldn't figure out how to get things working. I also couldn't push without adding --no-verify due to the linter not working out of the box (something about ethereumjs-config-format not available).

[holgerd77]

Hi David, thanks, could you paste the output along your linting problems? We have to get this fixed since linting is an integrational part of the required CI tasks to complete. Thanks!

[davidmurdoch]

@holgerd77 The output of running npm run lint:

avid @ ~/work/ethereumjs-vm (fix/tx-error-strings)
└─ $ ▶ npm run lint

> ethereumjs-vm@4.0.0 lint /home/david/work/ethereumjs-vm
> ethereumjs-config-lint

+ exec npm run format

> ethereumjs-vm@4.0.0 format /home/david/work/ethereumjs-vm
> ethereumjs-config-format

+ exec prettier --list-different **/*.{ts,json,md}
lib/runTx.ts
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! ethereumjs-vm@4.0.0 format: `ethereumjs-config-format`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the ethereumjs-vm@4.0.0 format script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/david/.npm/_logs/2019-08-25T19_47_01_719Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! ethereumjs-vm@4.0.0 lint: `ethereumjs-config-lint`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the ethereumjs-vm@4.0.0 lint script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/david/.npm/_logs/2019-08-25T19_47_01_736Z-debug.log

[s1na]

That means there's a linting error in lib/runTx.ts (the error is confusing). You can run npm run lint:fix

[s1na]

Looks good, thanks

[davidmurdoch]

Thanks! Once this patch is released I'll be able to release a new ganache-core and ganache-cli beta with the new v4 ejsvm.

[holgerd77]

@davidmurdoch Sorry, we are not really getting the good moment for a release. We had so much active work - mainly from @s1na - that we are now pretty pretty close to be able to release the full Istanbul EIP suite 🎉 😄 - so we waited a bit more to finalize the last implementations here.

But then there is always coming something in between, now the last thing being some changes to EIP-2200, delaying our WIP implementation once again, see #590.

Do you urgently need this release? Then I will initiate an in-between v4.1 release asap. Or can you wait another let's say (hope) 5-10 days on this?

[davidmurdoch]

@holgerd77 I'd prefer an earlier release only because Ganache's large user base may help uncover other bugs introduced in the TS refactor in v4, so it may be beneficial if ganache can release a version including ethereumjs@4.0.* before Istanbul support is available.

[holgerd77]

@davidmurdoch ah, we won't do another v4.0.x release but just include the Istanbul features we have right now, we actually don't have such a fine grained release management atm and just jump up to patch, minor or major version upgrade depending on the PRs merged.

But you should be able to expose your users to the Istanbul setting or not depending on what you think is suitable?

[davidmurdoch]

@holgerd77 Ah, sorry I wasn't clear... that's not what I was suggesting.

Because 4.0.0 was a complete refactor to TypeScript there are likely still some regressions lurking in the code base. It could be beneficial for ethereumjs-vm if Ganache could upgrade now, ahead of an ethereumjs-vm release containing lots of new code changes. Upgrading now may potentially make tracking down any issues related to ethereumjs-vm (filed as Ganache's issues) a bit easier.

In the end it's probably not going to be much of a hassle to triage any new bugs in the next minor release, especially if you are saying all the new code changes will be related to Istanbul, and not changes to the core internals.

[holgerd77]

@davidmurdoch Hi David, we now have the v4.1.0 release out of door containing the demanded fix of the error message. This release also includes the feature-complete set of Istanbul EIP implementations due to the great work of Sina. 😄

Let us know if you have problems or questions regarding the integration. Would also be nice if you could give us a short ping once a Ganache or also a full Truffle release is out using this version.

Cheers
Holger

[davidmurdoch]

Hey @holgerd77 77 (/cc @s1na), I was working on the release today and ran into a performance regression in ganache's test while testing the update.

ethereumjs-vm@3.0.0: 886ms (https://travis-ci.org/trufflesuite/ganache-core/jobs/585812344#L758)

ethereumjs-vm@4.1.0: 2033ms (https://travis-ci.org/trufflesuite/ganache-core/jobs/586271031#L732)

There are many more tests with regressions, this one just happened to be triggering the test timeout, causing it to fail.

I'm attempting to track the regression(s) down now. Let me know if you are already aware or have and hints as to where the slow down could be happening.

[s1na]

Oh! I'm glad you're measuring performance and these regressions bubbled up. I can't think of any specific issue that might be causing this. I hope it comes down to a specific issue and it's not that the new architecture is overall causing the slowdown.

Please let me know how I can help with tracking down the regressions.

[holgerd77]

@davidmurdoch This seems to be something for more thorough investigation. Can you open a new issue on this - eventually copying your message above. This will otherwise likely get lost in this error string issue so contextually detached here.

[davidmurdoch]

@s1na Tracked the majority of the slowdown to this commit: 4678325

Initially I thought it was because of 4678325#diff-452d330f2397c53c4a10771e89e595b8R254-R256

But that has since been refactored. The issue here was that the vm._emit function was getting "promisified" individually for every single op code run. On Node 8 on my machine I saw a 40% performance improvement by memoizing _emit in the constructor via this._emit = promisify(vm.emit.bind(vm));.

The refactored code seems to have moved the promisification to VM itself, but it still does it for every individual emit call (https://github.com/ethereumjs/ethereumjs-vm/blob/195bba7645151f975020bebe9f8ef146cc2a1680/lib/index.ts#L193). There are other callback functions getting the promisify treatment on every call (getBlockHash, applyTransactions, applyBlock, probably more?). We should probably memoize all promisify calls where reasonable and appropriate.

Another potential performance penalty (I haven't measured myself) is due to async/await is getting transpiled into TypeScript's __awaiter which uses Promise's then internally. Promise#then() has been reported to be slower than native await (https://v8.dev/blog/fast-async and https://mathiasbynens.be/notes/async-stack-traces). async/await is technically an ES2017 feature, so you'll need to update the TS target to use it. Updating may have other side effects that cause incompatibilities in some browsers or node version, I haven't checked. Maybe you can configure your linter to dissallow most ES2017 features except async/await?

[davidmurdoch]

Opened a separate issue for this here: #598