VM, Common: Implement EIP 1153, Transient storage opcodes #1768
Conversation
|
pretty cool, thanks for taking the initiative! looks like a pretty clean implementation so far on first look :) |
jochem-brouwer
changed the title
packages/vm/src/evm/opcodes/codes.ts
Outdated
| { | ||
| // These numbers differ than what is currently implemented | ||
| // in geth due to other eips being included here that are | ||
| // not in geth |
There was a problem hiding this comment.
Right, the EIP also does not yet state whatever opcodes numbers we should use.
There was a problem hiding this comment.
We now have some numbers that we can use: ethereum/EIPs@b6b3857
Thank you for the quick review! I'll address the comments when I have some time over the next couple of days |
|
This pr also needs to take into account the debug logging that shows what values are being stored/read still |
|
Some context: Tweet from Tim Beiko on ACD from January 2022 |
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
looks like coverage is down a lot, let's make sure we have all new lines covered with this PR as to not affect overall VM code coverage. probably just need a few more test cases to test the uncovered lines. |
Working on adding more coverage still, will squash my commits once I get some more coverage |
tynes
force-pushed
the
eip1153
branch
2 times, most recently
from
cfeb752
to
e66da59
Compare
Based on https://app.codecov.io/gh/ethereumjs/ethereumjs-monorepo/compare/1768/overview it looks like codecov is still working off of older code. Is there any way to refresh codecov? I've just been running the coverage locally to be able to add more coverage |
|
There are a couple branches inside the transient storage class that aren't covered, they are when optional chaining is used to make typescript remove the "object may be undefined" error in cases where it is known that the object will not be undefined. 100% of lines are covered |
|
We pushed a new approach and added some additional test coverage, would love to hear what you think of the new approach. Now there are no changes to the state manager and the changes are encapsulated in the eei |
| if (!changeset) { | ||
| throw new Error('cannot revert without a changeset') | ||
| } | ||
| const reversed = changeset.slice().reverse() |
There was a problem hiding this comment.
The slice copies the array, right? And the reversal seems like it can be optimized, for each key we would only need the first entry in the changeset? As that is the original value which we should revert to?
There was a problem hiding this comment.
According to the docs, slice creates a shallow copy
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice
I can refactor this to use a simple for loop that decrements
You need to iterate through the changeset and apply each diff. Perhaps some better naming could help here.
As things are added to transient storage, a TransientStorageModification is pushed to the latest changeset array. On a commit, the latest changeset is discarded and on a revert the latest changeset is used to set each value that was changed to its previous value and then discarded
export type Changeset = TransientStorageModification[]You only need to touch the latest changeset and all items in that changeset.
There was a problem hiding this comment.
the array of modifications could be optimized to be a map, since we only care about the first previous value
There was a problem hiding this comment.
i addressed this in aa79dcd
There was a problem hiding this comment.
Oh I see what you mean, thanks for the commit @moodysalem
- add a test about copying - add a test about reverting - add a test about stringifying map keys
holgerd77
changed the title
This is a draft PR that implements EIP 1153. I'm opening it to get feedback on its structure, to make sure its going in the correct direction and to see if its something that is desired to be upstreamed.
Its only slightly tested and there is a problem where the opcodes for simple subroutines clash with the opcodes used in our forks of geth and solc. This must be fixed before merging is considered
Ideally if this was merged and [1] is implemented in hardhat, then users will be able to very easily develop EIP 1153 based smart contracts in hardhat
[1] - NomicFoundation/hardhat#2463
If it is something desired, I will clean up the commits and add more tests