{"action":"created","issue":{"url":"https://api.github.com/repos/ethereumjs/ethereumjs-vm/issues/376","repository_url":"https://api.github.com/repos/ethereumjs/ethereumjs-vm","labels_url":"https://api.github.com/repos/ethereumjs/ethereumjs-vm/issues/376/labels{/name}","comments_url":"https://api.github.com/repos/ethereumjs/ethereumjs-vm/issues/376/comments","events_url":"https://api.github.com/repos/ethereumjs/ethereumjs-vm/issues/376/events","html_url":"https://github.com/ethereumjs/ethereumjs-vm/pull/376","id":375954292,"node_id":"MDExOlB1bGxSZXF1ZXN0MjI3Mjc5MjI0","number":376,"title":"Remove vm accesses to stateManager trie and cache","user":{"login":"mattdean-digicatapult","id":29942957,"node_id":"MDQ6VXNlcjI5OTQyOTU3","avatar_url":"https://avatars1.githubusercontent.com/u/29942957?v=4","gravatar_id":"","url":"https://api.github.com/users/mattdean-digicatapult","html_url":"https://github.com/mattdean-digicatapult","followers_url":"https://api.github.com/users/mattdean-digicatapult/followers","following_url":"https://api.github.com/users/mattdean-digicatapult/following{/other_user}","gists_url":"https://api.github.com/users/mattdean-digicatapult/gists{/gist_id}","starred_url":"https://api.github.com/users/mattdean-digicatapult/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mattdean-digicatapult/subscriptions","organizations_url":"https://api.github.com/users/mattdean-digicatapult/orgs","repos_url":"https://api.github.com/users/mattdean-digicatapult/repos","events_url":"https://api.github.com/users/mattdean-digicatapult/events{/privacy}","received_events_url":"https://api.github.com/users/mattdean-digicatapult/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":8,"created_at":"2018-10-31T13:09:43Z","updated_at":"2018-11-05T14:20:10Z","closed_at":null,"author_association":"COLLABORATOR","pull_request":{"url":"https://api.github.com/repos/ethereumjs/ethereumjs-vm/pulls/376","html_url":"https://github.com/ethereumjs/ethereumjs-vm/pull/376","diff_url":"https://github.com/ethereumjs/ethereumjs-vm/pull/376.diff","patch_url":"https://github.com/ethereumjs/ethereumjs-vm/pull/376.patch"},"body":"This is part of the broader discussion in #268 and #309 so readers should probably start there.\r\n\r\nThis PR removes all direct accesses, from the VM functions, to the `cache` and `trie`  properties of `stateManager`. This is essentially done by replacing calls to these properties to the equivalent   `stateManager` functions.\r\n\r\nThere are potentially two more controversial change here. The first is the removal of the  `loadCompiled` method on the VM. This method is unused internally and undocumented. It is problematic to keep this function as it interferes with checkpoint/commit structure used in `stateManager`. My assumption is that if someone desires this functionality they would interact with the `stateManager` directly prior to instantiating the vm. The second is the change of the `step` event which now exposes the `stateManager` instance rather than the `cache`. This is essentially a breaking change as the event object structure is documented."},"comment":{"url":"https://api.github.com/repos/ethereumjs/ethereumjs-vm/issues/comments/435891723","html_url":"https://github.com/ethereumjs/ethereumjs-vm/pull/376#issuecomment-435891723","issue_url":"https://api.github.com/repos/ethereumjs/ethereumjs-vm/issues/376","id":435891723,"node_id":"MDEyOklzc3VlQ29tbWVudDQzNTg5MTcyMw==","user":{"login":"mattdean-digicatapult","id":29942957,"node_id":"MDQ6VXNlcjI5OTQyOTU3","avatar_url":"https://avatars1.githubusercontent.com/u/29942957?v=4","gravatar_id":"","url":"https://api.github.com/users/mattdean-digicatapult","html_url":"https://github.com/mattdean-digicatapult","followers_url":"https://api.github.com/users/mattdean-digicatapult/followers","following_url":"https://api.github.com/users/mattdean-digicatapult/following{/other_user}","gists_url":"https://api.github.com/users/mattdean-digicatapult/gists{/gist_id}","starred_url":"https://api.github.com/users/mattdean-digicatapult/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mattdean-digicatapult/subscriptions","organizations_url":"https://api.github.com/users/mattdean-digicatapult/orgs","repos_url":"https://api.github.com/users/mattdean-digicatapult/repos","events_url":"https://api.github.com/users/mattdean-digicatapult/events{/privacy}","received_events_url":"https://api.github.com/users/mattdean-digicatapult/received_events","type":"User","site_admin":false},"created_at":"2018-11-05T14:20:10Z","updated_at":"2018-11-05T14:20:10Z","author_association":"COLLABORATOR","body":"So after a bit of investigating, I found that the changes introduced in #375 were not particularly compatible with the work I've been doing on #309. Essentially the issue is that the `cache` doesn't handle checkpoints, in my mind, correctly when it comes to deletes. My expectation is that the `cache` should, in the case of a checkpoint revert, restore the state of deletes at the time of the corresponding checkpoint. This was not happening, but by assuming that deletes only occur at particular times things worked out. What broke is that the `cache` needs to flush at the end of the transaction in order to clear touched accounts correctly. These changes prevented that. \r\n\r\nMy additional changes make it so that the `cache` will, on a revert, restore the original delete set. This is done in much the same way that the modified accounts set is maintained, i.e. on the cache value. This removes the `_getAccountPure` method on `stateManager` as it no longer matters if the empty account is added to the `cache` during the `accountIsEmpty` call. I've also made clearer the logic in `stateManager` to flush the cache when the checkpoint stack reaches zero. If anyone doesn't like this logic my alternative would be to add a `flush` method to `stateManager` but I don't feel this is necessary as we only need to flush when leaving a user initiated `vm` method.\r\n\r\nThis has made this PR, once again, larger than I would like. The alternative would be to split this up to first change the `cache` behaviour and then complete this PR with it's original intention. "}}
{"actor":{"display_login":"mattdean-digicatapult"}}