Skip to content
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

Finalize StateManager API #503

Closed
holgerd77 opened this issue Apr 30, 2019 · 9 comments
Closed

Finalize StateManager API #503

holgerd77 opened this issue Apr 30, 2019 · 9 comments

Comments

@holgerd77
Copy link
Member

Follow-up of #268.

To not get overloaded by the vast discussion from the issue above I will start a fresh issue here.

StateManager is now running unmodified for some time. We should do a last round to see if the API is fitting/not too complex/has other caveats and can be declared stable in its current or slightly modified form.

Also cc-ing @mattdean-digicatapult for this: maybe you are interested to follow or give final comments. 😄

@kumavis
Copy link
Member

kumavis commented May 13, 2019

The only thing I want from the StateManager is to be able to use a different backend for reads. For example I found it very useful to be able to run a tx and have that run lazily do state lookups via the json rpc. There was the HookedVm before that achieved it by overwriting some private api. https://github.com/ethereumjs/ethereumjs-vm/blob/994556ac998183cbf31dcbfd97d5f533f0690ee6/lib/hooked.js

@holgerd77
Copy link
Member Author

@kumavis hmm, there might be others who have different needs? Can you achieve your use case by customizing/subclassing the new StateManager class?

@kumavis
Copy link
Member

kumavis commented May 15, 2019

possibly! i havent followed ethereumjs-vm development for a long time, but was sad to see HookedVm get removed. I'll need this functionality again for Metamask's mustekala light client, currently we're running a very outdated version of the vm that still has HookedVm.

@kumavis
Copy link
Member

kumavis commented May 15, 2019

previously we only needed to hook a couple places

vm.stateManager._lookupStorageTrie = createAccountStorageTrie
vm.stateManager._cache._lookupAccount = loadAccount
vm.stateManager.getContractCode = codeStore.get.bind(codeStore)
vm.stateManager.setContractCode = codeStore.set.bind(codeStore)

@holgerd77
Copy link
Member Author

@kumavis hmm, I think you can just subclass the StateManager and replace the respective functionality with your own methods and then pass it to the VM. Shouldn't be too much of an effort? Let me know if there are unexpected hurdles.

@kumavis
Copy link
Member

kumavis commented May 18, 2019

yeah I'll take a swing at it when I get a chance.

Just want to highlight that I think ethereumjs-vm will be significantly used in scenarios where the state data is not locally available (eg light client), and so api for hooking those state lookups shouldnt be an after thought

@s1na
Copy link
Contributor

s1na commented May 20, 2019

One reason for dropping HookedVM was we weren't sure about the use-case, and it was difficult to ensure no unexpected behaviour happens in the VM when the stateManager is hooked with custom logic, specially when it's tightly coupled with Trie as its data source.

Just want to highlight that I think ethereumjs-vm will be significantly used in scenarios where the state data is not locally available (eg light client)

I think that makes sense. We can try to make this easier. I'll think about this more, and would also appreciate feedback, but something I can think of now is to de-couple Trie and Cache somewhat more from stateManager, e.g. by introducing a StateDB interface, which would be what stateManager uses to fetch/update data. Then we could have multiple implementations of StateDB, the default one being one that uses Trie, but maybe another one that uses a json-rpc backend.

Some difficulties I see with this is that when running a tx, the VM not only reads data, but modifies data and has checkpoint/revert functionality e.g. for when a revert happens. We'll also have to think whether caching should be done by the StateDB or StateManager.

@holgerd77
Copy link
Member Author

Not sure, just a suspicion, but what I think for quite some time is that it might give us a lot of confusion in distributing responsibilities of components is that we refer to this outer thing with runTx, runBlock,... functionality as "VM", which technically is just this inner part now being in the evm folder.

Would it make sense to significantly downgrade this outer part, have these as utility functions for the inner VM and also make this stateless (so: not class-based) but functional with just a well defined input and output?

Not sure if this fits to the tasks processed, just feels more correct to me on first thought, might not be practical or fitting though.

@holgerd77
Copy link
Member Author

Closed by #763

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants