This repository has been archived by the owner on Apr 11, 2021. It is now read-only.
Replace calls to ExecutionManager#ovmSLOAD and ExecutionManager#ovmSSTORE with native calls to SLOAD and SSTORE #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The goal here is to override the CALL opcode so we can replace calls to ovmSLOAD to native SLOAD operations. To start we've overridden the simpler MSTORE opcode and print "MSTORE Overriden" to verify the opcode has been overriden.
The goal is to override he CALL opcode and replace calls to "ovmStore" with native SSTORE calls. This first step runs a call operation and verifies that the correct arguments were passed in.
The next step is to actually hook up these opcodes to native SLOAD and SSTORE
Th remaining bytes are parameter data
..instead of the "success code"
* Use an environment variable to set EXECUTION_MANAGER_ADDRESS
masonforest
changed the title
willmeister
reviewed
Jan 24, 2020
tests/ovm_test.go
Outdated
| return output | ||
| } | ||
|
|
||
| func TestOvm(t *testing.T) { |
There was a problem hiding this comment.
FWIW, in order to set context properly, ovmSSTORE and ovmSLOAD are best tested with an involved setup where the tx does an ovmCALL to another contract C (though the OVM), and then C calls ovmSSTORE so that the storage is associated with C within ExecutionManager. this has some examples.
|
We chatted IRL: I'm going to fix the quick fixes Monday morning and then merge. After that I'm going to work on overriding "ovmCREATE" so I can mock contract creation properly and store state to the correct addresses. 🚀 |
karlfloersch
approved these changes
Jan 26, 2020
There was a problem hiding this comment.
This is a great start to building out the low level OVM implementation!!!! That said, the biggest things to note are:
- In order to bring our low-level OVM node to its full potential we will need to replace not just
ovmSLOAD/ovmSSTOREwith native calls, but we'll need to do the same to allovm*()functions in the ExecutionManager. The most critical of these is ovmCREATE, because shimming this will allow us to emit events which no longer need to be post-processed. Of course implementing all this should be done other PRs, but it's important to note that this PR does not include all features. - As @willmeister said there are certainly more tests which could go to ensuring the SSTORE // SLOAD are actually being stored and loaded from the expected locations -- making sure we're not overwriting any unintended storage.
The only thing that I might add to this PR is a test which is a bit more involved that checks to see that contracts don't overwrite each others storage as Will mentioned.
Later when we have the ovmCREATE shim we can add the tests which ensures that the contract addresses of the storage are correct.
That said, I hit approve because this looks like a solid first pass!!!! 🎉😁
* Check that `len(args)` is greater than 4
masonforest
force-pushed
the
mf-override-call
branch
from
621c607
to
a61a624
Compare
When calling the shimmed SSTORE and SLOAD we were storing to and loading from the ExecutionMananger's storage space. Instead we now call from the context of the `caller` of the contract. This gives each contract its own storage space so state isn't clobbered between contracts.
masonforest
force-pushed
the
mf-override-call
branch
from
dd5de60
to
395c483
Compare
|
I added a test to make sure OVMSSTORE doesn't overwrite values. I'm going to merge what I've got start work on the rest of the |
willmeister
pushed a commit
to ethereum-optimism/optimism
that referenced
this pull request
Feb 17, 2020
We've forked Geth to intercept calls to the ExecutionManger contract in ethereum-optimism/go-ethereum#1. Add a a test that tests these changes. The test is a copy of the existing simple storage spec with some minor changes. Namely, we don't test that storage events were emitted when ExecutionManager#ovmSSTORE is called because our forked version of Geth doesn't emit events. This also makes a couple changes to test helper file: We pass in the address of the ExecutionManager to manuallyDeployOvmContract instead of the ExecutionManager itself. This allows us to pass in an address of an ExecutionManager that's already been deployed which we need to do in our new test. Replace getTransactionReceipt with waitForTransaction. When we were using the Javascript provider transactions would be mined more-or-less immediately. This means that getTransactionReceipt could be called immediately after sendTransaction and the transaction would be ready. When we're talking to a Geth node this may not be the case. Switching to waitForTransaction allows time for the transaction to be mined.
ben-chain
added a commit
to ethereum-optimism/optimism
that referenced
this pull request
Mar 4, 2020
* fix timestamp * Changing examples package to be more generic (#64) * Changing examples package to be more generic * Updating default exec manager address * Add executable fullnode (#66) * Add executable fullnode * Add better logging with debug mode * Fix linting bug * Fixing output bytecode to not have 0x prepended (#70) * Add test for SLOAD and SSTORE shims in the govm (#39) We've forked Geth to intercept calls to the ExecutionManger contract in ethereum-optimism/go-ethereum#1. Add a a test that tests these changes. The test is a copy of the existing simple storage spec with some minor changes. Namely, we don't test that storage events were emitted when ExecutionManager#ovmSSTORE is called because our forked version of Geth doesn't emit events. This also makes a couple changes to test helper file: We pass in the address of the ExecutionManager to manuallyDeployOvmContract instead of the ExecutionManager itself. This allows us to pass in an address of an ExecutionManager that's already been deployed which we need to do in our new test. Replace getTransactionReceipt with waitForTransaction. When we were using the Javascript provider transactions would be mined more-or-less immediately. This means that getTransactionReceipt could be called immediately after sendTransaction and the transaction would be ready. When we're talking to a Geth node this may not be the case. Switching to waitForTransaction allows time for the transaction to be mined. * Truffle ERC-20 Example (#69) Working Truffle Example! Note: A small hack is required to get this to work -- documented in `packages/examples/truffle-config.js` Also adds: * Fix for nonces in ExecutionManager.sol * Gas limit fix setting it to the required 9_000_000 vs 5_000_000 in the fullnode * Util for adding padding to strings to make them of a certain length * Optional `add0x` param on `bufToHexStr(...)` * Lock around separate txs that are assumed to be run sequentially in DefaultWeb3Handler Co-authored-by: Mason Fischer <[email protected]> * Test that nonces are incremented on calls (#71) Before we were only incrementing nonces on CREATE. Wallets expect nonces to be incremented on CALLs aswell. This test tests that we are in fact inrementing nonces on CALLs aswell. Other changes: * Update the name of the executeCall test * Configuring OVM in separate truffle config as an easy example of side-by-side standard testing & ovm testing (#72) * pre rebase * move tests to fullnode pkg;2C * add txorigin test * finish dep struggles * typo Co-authored-by: Will Meister <[email protected]> Co-authored-by: Karl Floersch <[email protected]> Co-authored-by: Mason Fischer <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When executing transactions in the OVM we can’t use the SSTORE and SLOAD opcodes directly. This is because the OVM needs to keep track of state in case a transaction is contested and the state needs to be reverted. Instead we use the functions ovmSSTORE and ovmSLOAD. Use of these functions can be tracked by the execution manager and state can be reverted if need be. Executing these calls could be inefficient though. Instead we’ve forked Geth here to intercept those calls to the execution manager contract and call the native SSTORE and SLOAD opcodes instead.
Contributing Agreement