-
Notifications
You must be signed in to change notification settings - Fork 772
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
Remove explicit direct cache interactions #366
Conversation
Hi @mattdean-digicatapult, super-great, thanks for this new PR! I am a bit scratching my head how we get this coordinated on merging and rebasing together with the other also broadly scoped Constantinople PRs, especially the broad changes in the If you have ideas on that let me know. //cc @vpulim |
Hi @holgerd77, I'm not too worried about either #367 or #329 which I'm thinking would be the two PRs of immediate concern. They both access If those changes are near completion I'm happy to rebase on top of them once they are merged; I get that the Constantinople changes are probably of immediate priority. Alternatively if they are still a way off, and these changes are acceptable, I'm happy to help with the merge of this stuff into those branches to keep things moving. |
Hi @mattdean-digicatapult, sounds great, thanks! Yes, you are right, Constantinople stuff has very much high priority atm so we'll likely then first do the Constantinople merges, but would like to come to your work shortly after since this is also highly anticipated. |
3f1e0ad
to
79198f0
Compare
Since Constantinople is delayed anyhow I think we can in parallel going more actively forward with the |
(rebasing would be especially helpful since now blockchain tests are running by default - see #341 - and we'll get a lot better test coverage (in the sense: seeing what could be wrong) with this now.) |
The method `warmCache` on `stateManager` and the `populateCache` option directly expose the existence of the state cache. This makes removing all references to the cache impossible.
79198f0
to
f9df024
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just doing the merge together with @mattdean-digicatapult at Devcon having some extra explanations, great work, thanks!
This is part of the broader discussion in #268 and #309 so readers should probably start there.
One of the goals of defining and simplifying the
stateManager
interface is to allow others to implement their own underlying storage infrastructure which may or may not include caching. The current implementation exposes the cache both through direct accesses by the vm and through thewarmCache
method onstateManager
. The existence of the cache is also exposed by thepopulateCache
option on a couple of the vm methods. This PR acts to remove these in order to allow us to in future hide the existence of the cache from the vm entirely.The change essentially removes the options, and modifies synchronous cache accesses, which assume the account has been preloaded, to instead operate against
stateManager
asynchronously. The diff here is a little larger than I would like but a lot of this is down to making accesses asynchronous.