Improve performance at ethereumjs-vm-sc

[cgewecke]

Gnosis has a test that takes like 10 minutes to cover and the file i/o at the LOG opcode could be part of that. It would be nice to find where the vm exits so we could write the logs in bulk.

And this, while visually balanced, is also insane.

Speeding things up is critical if the coverage tool is going to get plugged into a fuzzer

[area]

I think we should be able to get rid of the contract-specific events entirely, now that solidity allows events with the same name again, which would solve point 2 (though I don't think I properly understand the exact issue you're alluding to is).

Those individual tests that Gnosis have taking multiple minutes is pretty incredible! You're right, appendFileSync is a good candidate for improving that time. I think the issue is going to be bubbling up the events without altering things too much. I wonder if we can piggyback on an existing object that's passed around (runState, maybe? I'm very unfamiliar with the ethereumjs-vm code!).

[cgewecke]

So essentially:

• undo the adriamb inheritance fix and do that coverage topic filter write a single time for an entire project. Instead of four times (now actually only once ) per file.
• Agreed - runState is a good candidate for hitching a ride on. That and find all the unique exits in the vm so we can write successfully.

The Gnosis test takes more than a minute without coverage iterating maybe [edit] 400 times. So, it's an atypical test but I'd like to try to write a coverage based contract fuzzer based on this that hits the code with hundreds of thousands of sequences of contract API calls to help identify vulnerabilities & bugs. Not sure this is viable for several reasons but one is the time cost.

[area]

Running a fuzzer against a contract would be really interesting, that's a very respectable goal!

I'm not sure how to practically make these changes without potentially breaking the NPM package, given the ethereum-testrpc-sc and ethereumjs-vm-sc dependencies within depend on the master branches. Should we pin those to specific commits on master, at least?

[cgewecke]

@area I'm glad you raised this because it relates to issue #64 which I'm hoping to do this week and wanted your advice about. At the moment testrpc-sc is equivalent to testrpc@3.0.3 and we're a full version behind the upstream.

In turn, testrpc-4.x.x is now actually ganache-core and all of its dependencies bundled into a webpack for fast-download and insulation from npm installation issues. It seems like the following is necessary:

• create ganache-core-sc
• turn testrpc-sc into a webpack bundler (like its parent)
• publish testrpc-sc on npm and pin it in given versions of solidity-coverage.

Doing that will mean that you can modify the behavior of ethereumjs-vm-sc safely. We can always re-bundle, re-publish and pin things. It will also make debug_traceTransaction available for development.

Questions:
What are your thoughts on the above?
What testrpc version are you using over there?
Should I interpret your note as you self-assigning this vm optimization issue?

[area]

Yeah, I'm happy to look into the VM optimisation, but the npm stuff has me spooked, as I've never had anything published on NPM before, let alone anything that was actually being used by other projects.

I'm very much in favour of bringing testrpc-sc up to date against upstream.

[cgewecke]

Ok great - I think it'll be ok. . . hopefully.

Notes to self / steps to take:

• publish an npm version of solidity-coverage with all the testrpc-sc deps pinned to commits instead of master so there's a safe state.
• work out webpack / ganache-core in a dev branch
• validate against a few projects
• make testrpc 4.x new master
• write some dev instructions for us so we know how to update the webpack build
• npm publish testrpc-sc --> myrmi / cgewecke
• publish an npm version of soldity-coverage pinned to specific testrpc-sc version on npm

That should get us back on the right track. I think only two projects have coverage in CI - Zeppelin's using yarn up there so . . . might be ok? And doing the above could actually fix a problem Gnosis is having in CI.

The ship's still close to shore here, just need to get this done quickly.

[cgewecke]

I'll tag this vm-optimization issue 'Ready' when the new testrpc is in.

[area]

Great, I'll keep an eye open. I also don't think that it makes sense to move this back to the joinColony repository, given the number of forks of other projects required; much more sensible to keep them all together under an organisation. I'll update the joinColony repository to indicate as much.

[cgewecke]

Ok! Well . . . at the risk of belabouring a point - as far as I'm concerned you have the run of the place. Please do as you see fit - push to master and publish as you wish - and let me know if there's anything you think we should do to coordinate. We're behind npm in this repo at least so it's not the end of the world if master isn't perfect every day.

As far as validation tests for major changes, I'm just cloning zeppelin-solidity, modifying the package to pull down the solidity-coverage branch in question and running npm run coverage. If that passes locally and our unit tests are clear, everything's probably fine.

[area]

Don't worry, I don't perceive you as coming in and taking over! I've been really appreciative of the work you've been doing to keep this project improving over the last few months while things have been hectic at Colony. In particular, the work you've done with projects in the Ethereum space to work through issues they've had trying to use solidity-coverage to encourage adoption has been brilliant to see from the sidelines.

[cgewecke]

Oh thanks @area, that's very sweet of you.

[area]

Okay, so, I've taken a crack at lifting the writing of the 'allFiredEvents' file to the end of every transaction, rather than every LOG event in the VM. Running coverage against the Gnosis repository, the time taken to run the tests with coverage goes from 22:13 to 14:20. Uninstrumented, the time taken is just over two minutes, so we still have some way to go!

Unfortunately, it looks like the coverage is different - I haven't got to the bottom of that yet...

[cgewecke]

Wow! That's still really impressive. What's different about the coverage?

Also there's something really weird about the way I putprovider-engine-sc in back in April. We use branch 8.1.19 and it looks like the npm package misnames the ethereumjs-vm dependency? I don't understand why that's even working.

Don't know if this message will go through - GitHub seems to be having an outage.

[EDIT] Actually it doesn't matter that that's wrong - the requires just find ethereumjs-vm somewhere in the node_modules and it all gets bundled in ok without side-effects.

[cgewecke]

@area On a whim I tried running gnosis after making the allFiredEvents file write async:

fs.appendFile('./allFiredEvents', JSON.stringify(toWrite) + '\n', () => {});

On my box it came in at just over 10 minutes with a summary report that's identical to what they are currently showing on Travis / master.

Do you have any thoughts about the safety of this approach?

Rooted around a little and all I came up with was this article from years ago which claims that writeSync isn't 'really' synchronous and only blocks the event loop.

It's also possible that the post truffle exit readSync in app.js will actually wait for allFiredEvents to become available if it's still being written to by a separate process and async write is always ok. But I'm not sure about that at all - it seems like a low level OS resource policy thing.

[area]

From the Node docs:

Note that it is unsafe to use fs.writeFile multiple times on the same file without waiting for the callback. For this scenario, fs.createWriteStream is strongly recommended.

It does not say the same about 'appendFile', but I think I assumed that appendFile did writeFile( , {flag: 'a'} under the hood. If that's not the case, it's possible that it's safe....

EDIT: In general though, on principle, I'm very dubious about just ignoring callbacks!

[cgewecke]

Ok well then it's conclusively unsafe. However maybe we could createWriteStream when we load the filter topics.

But how can we be sure all writes have completed by the time we try to open allFiredEvents?

Here's another idea . . . push all the coverage events onto an array in memory. When truffle exits in app.js, send testrpc-sc a 'special transaction' that we can somehow detect in op.fns. Write once. Do you think that might be possible?

[cgewecke]

Closing, moot.