Eip1283 #17383
Eip1283 #17383
Conversation
|
This is probably outside of remit of this PR, but now that you have "originalStorage" and "dirtyStorage", you don't need "cachedStorage", which existed only to avoid repeated reads. Now you can avoid repeated reads like so: https://github.com/ethereum/go-ethereum/pull/17208/files#diff-e6890b729fad8d7cf6c1a35a31080af7R165, and "originalStorage" also allows skipping writes that do not change the stored value: https://github.com/ethereum/go-ethereum/pull/17208/files#diff-e6890b729fad8d7cf6c1a35a31080af7R193 |
|
Thanks for checking, @AlexeyAkhunov ! I had a feeling that the implementation could be done better, by making better use of the existing structures, but didn't really go in for it. If we decide to move forward with this EIP, I'll update it according to your observations. |
|
(or, actually, might be better just rewrite it based on @karalabe's PR instead) |
core/vm/gas_table.go
Outdated
| if original == (common.Hash{}) { // 2.1.1 | ||
| return 20000, nil | ||
| // The legacy gas metering only takes into consideration the current state | ||
| if !evm.chainRules.IsConstantinople { |
There was a problem hiding this comment.
I personally think it was cleaner to have two separate gas calc functions, one enabled in Constantinople and one earlier. That makes the respective methods smaller, and saves this check on every SSTORE.
There was a problem hiding this comment.
It's a single bool comparison vs. an sstore potentially loads of data from disk. Other gas calculations (e.g. gasSuicide) also use the same logic of having a single method per opcode and potentially checking whether a fork is enabled or not. It feels a bit cleaner for me to have a single method, but if you really disagree we can split it.
There was a problem hiding this comment.
Yeah, it's not a heavy toll, the main reason for me was more that it allowed splitting the methods into two separate units.
I don't feel strongly about it, you're the main maintainer :)
|
A lot of the tests are failing, since this PR messes with a lot of things that use |
|
See ethereum/tests#511 (comment) -- before all tests are regenerated, not much we can do to turn travis green ... |
Perhaps we should disable the Constantinople tests and reenable them when they're updated. I'd very much like to keep pre-Constantinople tests live in our repo. |
|
I can fix that |
This PR implements [EIP 1283(https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1283.md#specification-version-ii). 1283 is an alternative to 1087, which does not require as much post-processing.
The cost for SSTORE operations rely on current value, new value and
originalvalue, whereoriginalmeans 'what is currently in the trie' a.k.a 'the value that would remain if this current transaction was wholly reverted'.This PR uses a map in state_object to store original value whenever a slot is updated.
This PR is experimental, in that it does not contain any testcases whatsoever, the intention being more to test how complex it would be to implement rather than being a fully-fledged merge-ready PR.