-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve performance at ethereumjs-vm-sc #63
Comments
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, |
So essentially:
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. |
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 |
@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
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: |
Yeah, I'm happy to look into the VM optimisation, but the I'm very much in favour of bringing |
Ok great - I think it'll be ok. . . hopefully. Notes to self / steps to take:
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. |
I'll tag this vm-optimization issue 'Ready' when the new testrpc is in. |
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. |
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 |
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 |
Oh thanks @area, that's very sweet of you. |
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... |
Wow! That's still really impressive. What's different about the coverage? Also there's something really weird about the way I put 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 |
@area On a whim I tried running gnosis after making the 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 It's also possible that the post truffle exit |
From the Node docs:
It does not say the same about 'appendFile', but I think I assumed that appendFile did EDIT: In general though, on principle, I'm very dubious about just ignoring callbacks! |
Ok well then it's conclusively unsafe. However maybe we could But how can we be sure all writes have completed by the time we try to open Here's another idea . . . push all the coverage events onto an array in memory. When truffle exits in |
Closing, moot. |
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
The text was updated successfully, but these errors were encountered: