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

[Constantinople] EIP 1283 SSTORE net metering #354

Closed
holgerd77 opened this issue Sep 24, 2018 · 6 comments · Fixed by #367
Closed

[Constantinople] EIP 1283 SSTORE net metering #354

holgerd77 opened this issue Sep 24, 2018 · 6 comments · Fixed by #367

Comments

@holgerd77
Copy link
Member

holgerd77 commented Sep 24, 2018

EIP 1283 SSTORE net metering without dirty maps (replaces EIP-1087)

Test Situation:
Open PR, affecting lots of tests
ethereum/tests#511
ethereum/tests#483

Implementation in the VM:
Mostly/all in the SSTORE opcode code.

@yann300
Copy link
Contributor

yann300 commented Sep 24, 2018

I just looked at this one, what would be the best way to get an initial storage value, seems not really obvious. Is there some refactoring already happening for that?

@holgerd77
Copy link
Member Author

No idea yet, we'll have to work this out together.

@holgerd77
Copy link
Member Author

@yann300 Update: haven't looked deeper, but is stateManager.getContractStorage() already used in the opcode not already doing this?

@yann300
Copy link
Contributor

yann300 commented Oct 1, 2018

it is but does it get the initial storage or the current storage?

@vpulim vpulim mentioned this issue Oct 8, 2018
@vpulim
Copy link
Contributor

vpulim commented Oct 10, 2018

as @yann300 mentioned, stateManager.getContractStorage() only gets the current storage. I submitted a PR (#367) that sets runState.originalState to a copy of the stateManager trie so that it can later be used in the SSTORE opcode to read the original storage values. There may be a more efficient way to do this using the stateManager checkpoints but it will likely require non-trivial changes to stateManager

@holgerd77
Copy link
Member Author

@vpulim For now this should work, better to have these updates ready in time for the dev tools to be used that start with complex StateManager refactorings.

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

Successfully merging a pull request may close this issue.

4 participants