Add initial StateTransitioner & FraudProver #156

karlfloersch · 2020-06-15T02:22:22Z

Description

This PR adds a StateTransitioner & a FraudProver and a bunch of helper contracts to build up a first pass fraud proof system. It does not have good test coverage, but it does have enough that I think it mostly works & most importantly gets the point across.

TODO in later PR

We've got to update the ExecutionManager to use getOvmContractHash(..) instead of calling the codeContractHash version of the function. I didn't include this in this PR because @ben-chain is touching a bunch of EM stuff & so I don't want to make too many medium-sized changes to the EM at the same time

Metadata

Fixes

YAS-200
YAS-201
YAS-203

Contributing Agreement

I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

karlfloersch · 2020-06-15T20:54:56Z

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

+     * @param _codeContractAddress The address of the code contract.
+     * @return The bytecode at this address.
+     */
+    function getCodeContractBytecode(address _codeContractAddress) public view returns (bytes memory codeContractBytecode) {


make sure we verify the code contract bytecode even if it doesn't exist

Added another function getOvmContractBytecode(...) which will flag invalid state access

ben-chain · 2020-06-17T00:52:04Z

packages/rollup-contracts/contracts/ExecutionManager.sol

@@ -582,8 +582,6 @@ contract ExecutionManager {
            restoreContractContext(oldMsgSender, oldActiveContract);
            return false;
        }
-        // Associate the code contract with our ovm contract
-        stateManager.associateCodeContract(_newOvmContractAddress, codeContractAddress);
        bytes memory codeContractBytecode = stateManager.getCodeContractBytecode(codeContractAddress);


Note that we need to have code hash(and probably size) authenticated in the SM, so we should be pulling this directly.

ben-chain · 2020-06-17T00:53:04Z

packages/rollup-contracts/contracts/ovm/FraudVerifier.sol

+        // First, call stateManager.initNewTransactionExecution()
+        // (probably do the same with the ExecutionManager)
+        // Then actually call `executeTransaction(tx)` with the tx in question!


Is there any reason not to combine both of these? Where executeTransaction would just do the init logic.

Ah because the init logic would not be strictly necessary inside of the L2Geth

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

ben-chain · 2020-06-17T01:04:46Z

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

+        }
+    }
+
+    function ensureVerifiedContract(address _ovmContractAddress) private {


FWIW there is some nuance here where e.g. ovmEXTCODEHASH would not require the bytecode itself be verified to be a valid state access. In fact if we ship that opcode, it's probably necessary to limit fraud proof size, or a contract doing a bunch of these could be problematic.

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

ben-chain · 2020-06-17T01:11:44Z

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

+     * @param _codeContractAddress The address of the code contract.
+     * @return The bytecode at this address.
+     */
+    function getCodeContractBytecode(address _codeContractAddress) public view returns (bytes memory codeContractBytecode) {


left a comment on this above, but definitely seems like if we are smart about this, we can do everything needed passing around hashes and not the full bytecode.

ben-chain · 2020-06-17T01:13:38Z

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

+     * @return The hash of the bytecode at this address.
+     */
+    function getCodeContractHash(address _codeContractAddress) public view returns (bytes32 _codeContractHash) {
+        // TODO: Look up cached hash values eventually to avoid having to load all of this bytecode


yep, this should be authenticated against the state anyway. And ofc. also note the EXTCODEHASH opcode can be used.

ben-chain · 2020-06-17T01:14:44Z

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

+     * @param _ovmContractInitcode The bytecode of the contract to be deployed
+     * @return the codeContractAddress.
+     */
+    function deployContract(


This will be changed what with moving safety check into EM, etc. so not reviewing for now

ben-chain

Nice, looking siiiiick! Left another round of comments.

ben-chain · 2020-06-23T20:36:05Z

packages/rollup-contracts/contracts/ExecutionManager.sol

+    function setStateManager(address _stateManagerAddress) external {
+        stateManager = FullStateManager(_stateManagerAddress);
+    }


Is this function meant to be atomically called before executeTransaction? If so, might be able to move that logic there in a future PR. If not atomic, do we need to authenticate that it's coming from the fraud prover?

yeah i totally agree we might as well put it into executeTransaction(..) itself

ben-chain · 2020-06-23T20:40:21Z

packages/rollup-contracts/contracts/FullStateManager.sol

@@ -104,7 +104,7 @@ contract FullStateManager is StateManager {
     * @param _ovmContractAddress The address of the OVM contract we'd like to associate with some code.
     * @param _codeContractAddress The address of the code contract that's been deployed.
     */
-    function associateCodeContract(address _ovmContractAddress, address _codeContractAddress) external {
+    function associateCodeContract(address _ovmContractAddress, address _codeContractAddress) public {


Does this need to be authenticated? Would expect only the Fraud Verifier to be able to populate these associations.

Ah, whoops, I see that this isn't the PSM. Will just resurface the nit/bikeshed that TestStateManager and TestExecutionManager could be better names for these non-fraud-proof contracts.

ben-chain · 2020-06-23T20:42:41Z

packages/rollup-contracts/contracts/ovm/FraudVerifier.sol

+    function verifyFraud(uint _transitionIndex) public returns(bool) {
+        // TODO:
+        // Simply verify that the state transitioner has completed, and that the state root
+        // at _preStateTransitionIndex+1 is not equal to the state root which was committed for that index.
+        return true;
+    }


Note that in the current contracts I believe that the FraudVerifier is meant to call out to the state commitment chain to delete if there is fraud. It's a small change, but should update either this TODO or have one for the commitment chain.

ben-chain · 2020-06-23T20:49:13Z

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

+    * Pre-Execution *
+    ****************/
+
+    function insertVerifiedStorage(address _ovmContractAddress, bytes32 _slot, bytes32 _value) external onlyStateTransitioner {


We could potentially add a require here that the _ovmContrractAddress has been verified. I assume this would already be enforced by the state transitioner, but could be a good extra sanity check.

ben-chain · 2020-06-23T20:51:39Z

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol

+        ovmContractStorage[_ovmContractAddress][_slot] = _value;
+    }
+
+    function insertVerifiedContract(address _ovmContractAddress, address _codeContractAddress, uint _nonce) external onlyStateTransitioner {


I may have mentioned this already, but it would be nice to be able to insert the code hash and size without submitting the code itself. This would allow us to place a higher bound on the number of ovmEXTCODESIZE and ovmEXTCODEHASH. Relatively minor edge case, though; I don't think they get used very much.

ben-chain · 2020-06-23T23:49:45Z

packages/rollup-contracts/contracts/ovm/StateTransitioner.sol

+    * Transaction Execution *
+    ************************/
+    function applyTransaction() public returns(bool) {
+        // TODO:


fill or delete :)

ben-chain · 2020-06-23T23:51:08Z

packages/rollup-contracts/contracts/ovm/StateTransitioner.sol

+    ************************/
+    function applyTransaction() public returns(bool) {
+        // TODO:
+        stateManager.initNewTransactionExecution();


I think this confirms for me that we can wrap this call into executeTransaction. LMK if you think makes sense and we can write up a ticket for it.

Yeah we can deff add this to executeTransaction (of course gas cost stuff needs to be ensured to be deterministic but that should be fine)

ben-chain · 2020-06-23T23:55:12Z

packages/rollup-contracts/contracts/ovm/test-helpers/StubExecutionManager.sol

+import {PartialStateManager} from "../PartialStateManager.sol";
+
+/**
+ * @title StubExecutionManager


nit: FWIW we have a pretty frequent convention of Mock* in the rest of our codebase. Would recommend we change this and the stub safety checker to follow, but up to you.

Yeah agreed!

ben-chain · 2020-06-24T00:00:44Z

packages/rollup-contracts/contracts/ovm/test-helpers/StubExecutionManager.sol

+        // Just call the state manager to store values a couple times
+        stateManager.setStorage(0x1111111111111111111111111111111111111111, 0x1111111111111111111111111111111111111111111111111111111111111111, 0x1111111111111111111111111111111111111111111111111111111111111111);
+        stateManager.setStorage(0x2222222222222222222222222222222222222222, 0x2222222222222222222222222222222222222222222222222222222222222222, 0x2222222222222222222222222222222222222222222222222222222222222222);
+        // TODO: Make this a bit more comprehensive. Could even make it configurable?


Yeah, I can def see a world in which the callBytes is an array of raw calldata which the EM sends off unquestioningly to the SM. Would probably make unit tests very clean

ben-chain · 2020-06-24T00:01:00Z

packages/rollup-contracts/contracts/ovm/test-helpers/StubSafetyChecker.sol

+pragma solidity ^0.5.0;
+
+/**
+ * @title StubSafetyChecker


same Mock nit

- Remove extra TODO comment - Remvoe unneeded functions from the PartialStateManager

ben-chain

Approving--there are minor fixes left, but we can push those off in the interest of avoiding big merge conflicts.

* Separates storage from SCC and CTC (#151) * First pass version * More minor tweaks for tests to pass * Add authentication * Minor config updates * Fix lint error * Fix naming changes per review * Enable Deployer Whitelist (#119) * first pass, test runner updated * add ability to only validate flag, test passes * all tests passing * clean up console.logs * enforce gas refund preservation * more cleanup/import removal * whitelisted -> allowed * first pass, test runner updated * add ability to only validate flag, test passes * all tests passing * clean up console.logs * enforce gas refund preservation * more cleanup/import removal * whitelisted -> allowed * remove whitespace * Restrict StateTransitionerFactory (#140) * added msg sender check * add create test * cleanup * add param * add addressmanager.address param * CTC Chain Monotonicity Fixes (#93) * [wip] Fix block time logic * some sad path and happy tests passing * more progress * first pass sad cases tested * cleanup, adding empty tests * more reversion tests * rename shouldstartat} * add final couple tests * enable more tests * cleanup * remove .only * textual cleanup * make queue length public * improve structure, comments * update deploy config * address nits Co-authored-by: Karl Floersch <karl@karlfloersch.com> * fix declarations, lint (#152) * Adds river's new Merkle tree implementation, with some cleanup (#148) * Reverts an accidental breaking merge * Added new merkle tree impl * add comments * Final cleanups and merge Co-authored-by: Ben Jones <ben@pseudonym.party> * Fix run gas Lower Bound (#94) * added the check * add test * lower OVM TX size for Kovan * re-remove gas check * update gas vals slightly * lint * lint * Merge master into freeze integration branch (#153) * update solidity version to ^0.7.0 (#122) * update solc version to ^0.7.0 * interfaces back to solidity >0.6.0 <0.8.0 * update solc to 0.7.6 * back to 0.7.4 * upgrade to 0.7.6, fix EXTCODESIZE check * versions >0.5.0 <0.8.0 for xdomain msgers * ctc: disable appendQueueBatch (#150) * ctc: disable appendSequencerBatch * typo: fix * re-enable verifyQueueTransaction test: * add explicit test for verifying queue elements against either append Co-authored-by: Ben Jones <ben@pseudonym.party> * fix up test * remove .only Co-authored-by: Alina <alina@optimism.io> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> * add check, simple test, update deploy (#154) * go back to first name (#155) * lint * fix js number error * add error logging to help debug deploy * [code freeze] Fix deploy script (#156) * fix deploy script * add block time config * ensure value is integer * lint * remove console logs from deploy * Moves gas check to applyTransaction (#161) * move to OVM_ST, pass test * remove old test because functionality moved * linting * remove leaf hasing * use safe EXEMRG wrapper (#162) * use safeREQUIRE * add owner getter * relayer: add to config (#160) * relayer: add to config * lint: fix * Fix minor error in test config Co-authored-by: Kelvin Fichter <kelvinfichter@gmail.com> Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Alina <alina@optimism.io> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>

b5b564702 popm/wasm: add wasm-opt target to optimise wasm binary (ethereum-optimism#146) 27a5081e3 tbc: allow seeds to be overwritten (ethereum-optimism#158) 18afbc403 Fix missing panic (ethereum-optimism#156) 05cee0afb tbc: remove height as the terminal condition for indexers (ethereum-optimism#152) 4402060d6 Use hemilabs/websocket fork of nhooyr.io/websocket (ethereum-optimism#153) 3df5001c4 Add initial CODEOWNERS file (ethereum-optimism#149) a4685a57f popm/wasm: improve and tidy up Go code (ethereum-optimism#144) 41a0009c1 Add forking detection to TBC (ethereum-optimism#101) bbeed8bac ignore ulimits in tbc when on localnet (ethereum-optimism#142) 4e1914cc6 Stopgap to re-fetch blocks with no children (ethereum-optimism#131) 12eefff06 e2e: fix issues detected by staticcheck (ethereum-optimism#134) 80153372f Add more documentation for TBC and related RPC protocol (ethereum-optimism#86) 723b63704 bfg: fix issues reported by staticcheck (ethereum-optimism#132) 509fb1be6 electrumx: fix unhandled error in NewJSONRPCRequest (ethereum-optimism#133) b19af1e1f hemictl: use sort.Strings(...) instead of sort.Sort(sort.StringSlice(...)) (S1032) (ethereum-optimism#123) cf7c570f9 popmd: remove unused 'handle' func and return err in connectBFG (ethereum-optimism#124) fa2a4c4de ci: fix bug that makes version type always 'unstable' (ethereum-optimism#125) ddc39655a ci: use org DOCKERHUB_TOKEN secret (ethereum-optimism#128) 6457d16f5 bfgd: drop btc_blocks_can refresh triggers for l2_keystones/pop_basis (ethereum-optimism#120) 2b3c096bd popm: simplify receivedKeystones variable in tests (ethereum-optimism#116) 6140bbf24 tbc: move RPC tests to a separate test file (ethereum-optimism#114) c76571ff0 Add configuration option for static fees to web PoP miner (ethereum-optimism#119) 233817e65 popm: use simple conversion instead of unnecessary fmt.Sprintf (S1025) (ethereum-optimism#115) 2879f5fa7 e2e: improve name for variable with type `time.Duration` (ST1011) (ethereum-optimism#117) 4f31965e6 database,e2e: remove use of deprecated io/ioutil (SA1019) (ethereum-optimism#118) e9e090696 tbc: do not recreate outpoint for ScriptHashByOutpoint. (ethereum-optimism#109) 52eefb136 ignore l2 keystones notifications in tests (ethereum-optimism#112) eb607994c Sync Docker image environment variables with daemon configs (ethereum-optimism#111) 57281bc5d added a way to monitor and sanity-test localnet (ethereum-optimism#106) ea1a1c94c bfgd: fix loops unconditionally exited after one interation (SA4004) (ethereum-optimism#108) 29f116fb4 Add pprof http server to daemons (ethereum-optimism#105) 18a315d7e Added README for generating forks in BTC regtest for TBC fork resolution testing (ethereum-optimism#100) 55b8f52cb more robust nextPort (ethereum-optimism#103) 48f8b293f tbcapi: use reverse byte order for hashes in serialised, tidy up (ethereum-optimism#104) b7e9f5ecb restart initialblocks on-failure (ethereum-optimism#102) f9d52d423 Update required Go version to v1.22.2 (ethereum-optimism#96) f6808aa5b Fix op-proposer op-node dependency condition (fixes ethereum-optimism#98) (ethereum-optimism#99) a882529e8 Kill all pending block downloads if a peer fails (ethereum-optimism#95) eb66345e1 e2e: tidy up docker-compose file (ethereum-optimism#81) 34766f725 Track pending blocks with ttl package (ethereum-optimism#90) 6ed3eb88b Added configurable fee-per-vB to PoP Miner (ethereum-optimism#91) 509e31fbc Replace os.Kill with syscall.SIGTERM in signal.Notify calls (ethereum-optimism#87) git-subtree-dir: heminetwork git-subtree-split: b5b564702e8d3bedcdf0e0a52c22e383d7fd4dbe

karlfloersch added 4 commits June 13, 2020 16:57

WIP partial state manager

148bc00

Move associate code contract into SM deploy

0c7a2c4

Sketch out more of the partial state mgr

38f7842

Add outlines of partial state mgr & fraud verifier

f9c0736

karlfloersch added the wip label Jun 15, 2020

karlfloersch requested a review from ben-chain June 15, 2020 02:22

karlfloersch commented Jun 15, 2020

View reviewed changes

packages/rollup-contracts/contracts/ovm/PartialStateManager.sol Outdated Show resolved Hide resolved

karlfloersch commented Jun 15, 2020

View reviewed changes

ben-chain reviewed Jun 17, 2020

View reviewed changes

karlfloersch added 10 commits June 21, 2020 15:27

Add onlyFraudVerifier / EM modifiers

b172f26

Remove saftey checker from PSM & add setStateMgr to EM

ce7221b

Split up state manager into StateTransitioner

ea7f844

Make the prestate transition index more explicit

a35a34d

Begin adding pre & post tx execution

548a209

Add a basic SafeContractRegistry

40ed3a3

Remove unneeded SafeContractRegistry

94fc0c5

Add TransitionPhases & PreTransition functions

7704cf7

Add Stub EM & finish phases of Transitioner

1a62772

Lint

bbca98f

karlfloersch changed the title ~~[WIP] Add partial state manager~~ Add initial fraud proof system Jun 22, 2020

karlfloersch changed the title ~~Add initial fraud proof system~~ Add initial StateTransitioner & FraudProver Jun 22, 2020

karlfloersch removed the wip label Jun 22, 2020

Rename ensureValid.. with flagIfInvalid

9936120

ben-chain reviewed Jun 24, 2020

View reviewed changes

Move contract deployment back into the EM

73fae93

karlfloersch force-pushed the YAS-200/FraudVerifier/AddPartialStateManager branch from 3122c49 to 73fae93 Compare June 29, 2020 15:40

Small fixes

5c1e834

- Remove extra TODO comment - Remvoe unneeded functions from the PartialStateManager

ben-chain approved these changes Jul 1, 2020

View reviewed changes

karlfloersch merged commit 88f93e2 into ethereum-optimism:master Jul 1, 2020

bap2pecs pushed a commit to babylonlabs-io/optimism that referenced this pull request Jul 31, 2024

CI: Push images for covenant (ethereum-optimism#156)

fdc9cb3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial StateTransitioner & FraudProver #156

Add initial StateTransitioner & FraudProver #156

karlfloersch commented Jun 15, 2020 •

edited

Loading

karlfloersch Jun 15, 2020

karlfloersch Jun 22, 2020

ben-chain Jun 17, 2020

ben-chain Jun 17, 2020

karlfloersch Jun 17, 2020

ben-chain Jun 17, 2020

ben-chain Jun 17, 2020

ben-chain Jun 17, 2020

ben-chain Jun 17, 2020

ben-chain left a comment

ben-chain Jun 23, 2020

karlfloersch Jun 24, 2020

ben-chain Jun 23, 2020

ben-chain Jun 23, 2020

ben-chain Jun 23, 2020

ben-chain Jun 23, 2020

ben-chain Jun 23, 2020

ben-chain Jun 23, 2020

ben-chain Jun 23, 2020

karlfloersch Jun 30, 2020

ben-chain Jun 23, 2020

karlfloersch Jun 30, 2020

ben-chain Jun 24, 2020

ben-chain Jun 24, 2020

ben-chain left a comment

Add initial StateTransitioner & FraudProver #156

Add initial StateTransitioner & FraudProver #156

Conversation

karlfloersch commented Jun 15, 2020 • edited Loading

Description

TODO in later PR

Metadata

Fixes

Contributing Agreement

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-chain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-chain left a comment

Choose a reason for hiding this comment

karlfloersch commented Jun 15, 2020 •

edited

Loading