[WIP] feat: Have predeploys use OVM solc only #475

smartcontracts · 2021-04-15T22:42:54Z

>>> Not ready for review <<<

Description
Working on this in a draft PR so I can have CI run.

Fixes #495

changeset-bot · 2021-04-15T22:42:57Z

⚠️ No Changeset found

Latest commit: 7881bc6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

smartcontracts · 2021-04-16T21:55:22Z

Integration tests are just hanging and I have no clue how to debug that... @gakonst or @tynes?

tynes · 2021-04-17T00:15:02Z

Integration tests are just hanging and I have no clue how to debug that... @gakonst or @tynes?

I think the key will be to run the tests locally since docker-compose up is ran in the background

tynes · 2021-04-17T00:20:46Z

When building locally:

$ /optimism/node_modules/.bin/ts-node bin/take-dump.ts
(node:28) UnhandledPromiseRejectionWarning: Error: Unable to find artifact for contract: OVM_L2CrossDomainMessenger
    at Object.getContractDefinition (/optimism/packages/contracts/src/contract-defs.ts:33:15)
    at Object.getContractFactory (/optimism/packages/contracts/src/contract-defs.ts:43:32)
    at Object.makeContractDeployConfig (/optimism/packages/contracts/src/contract-deployment/config.ts:15:38)
    at Object.deploy (/optimism/packages/contracts/src/contract-deployment/deploy.ts:17:49)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at Object.makeStateDump (/optimism/packages/contracts/src/contract-dumps.ts:135:30)
    at /optimism/packages/contracts/bin/take-dump.ts:37:18
(Use `node --trace-warnings ...` to show where the warning was created)
(node:28) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 5)
(node:28) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

tynes · 2021-04-17T00:28:21Z

Running it locally found:

deployer_1         | 192.168.192.8 - - [17/Apr/2021 00:26:02] "GET /state-dump.latest.json HTTP/1.1" 404 -
l2geth_1           | panic: Cannot fetch state dump: State dump not found
l2geth_1           |
l2geth_1           | goroutine 1 [running]:
l2geth_1           | github.com/ethereum/go-ethereum/core.DeveloperGenesisBlock(0x0, 0xcdbd24fc22d45828, 0x6a322d4ce8dc026c, 0xe970b65919984a67, 0x20af5177420a9dfa, 0x7b8570a91967671d, 0x260974e5b3be8a99, 0x924707f651b7e52a, 0x3e61331b, 0x0, ...)
l2geth_1           |    /go-ethereum/core/genesis.go:486 +0x1166
l2geth_1           | github.com/ethereum/go-ethereum/cmd/utils.SetEthConfig(0xc00029a6e0, 0xc0002b0280, 0xc00016f200)
l2geth_1           |    /go-ethereum/cmd/utils/flags.go:1731 +0xf19
l2geth_1           | main.makeConfigNode(0xc00029a6e0, 0xc00016ec00, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
l2geth_1           |    /go-ethereum/cmd/geth/config.go:129 +0x3cb
l2geth_1           | main.makeFullNode(0xc00029a6e0, 0x127167d)
l2geth_1           |    /go-ethereum/cmd/geth/config.go:149 +0x72
l2geth_1           | main.geth(0xc00029a6e0, 0x0, 0x0)
l2geth_1           |    /go-ethereum/cmd/geth/main.go:324 +0xf7
l2geth_1           | gopkg.in/urfave/cli%2ev1.HandleAction(0x10bf240, 0x134da38, 0xc00029a6e0, 0xc000281ce0, 0x0)
l2geth_1           |    /go/pkg/mod/gopkg.in/urfave/[email protected]/app.go:490 +0xc8
l2geth_1           | gopkg.in/urfave/cli%2ev1.(*App).Run(0xc0002b6000, 0xc000126020, 0x2, 0x2, 0x0, 0x0)
l2geth_1           |    /go/pkg/mod/gopkg.in/urfave/[email protected]/app.go:264 +0x5dd
l2geth_1           | main.main()
l2geth_1           |    /go-ethereum/cmd/geth/main.go:266 +0x55

Nit: the filename should be state-dump-latest.json instead of state-dump.latest.json so that it doesn't confuse things that expect a single dot to mark the file extension

tynes · 2021-04-17T00:28:59Z

@smartcontracts The state dump file fails to build so it is not created and served to l2geth on startup

…eat/predeploys-use-ovm-solc

snario · 2021-04-19T19:29:12Z

This is the monorepo port of ethereum-optimism/contracts#300

karlfloersch · 2021-04-20T05:34:38Z

After doing more debugging it seems integration tests are failing due to a bug in the compiler, probably related to kall. You can reproduce a relatively simple case by running this integration test:

optimism/integration-tests/test/rpc.spec.ts

Lines 84 to 100 in d76f0ce

    
           it('should return the block and all included transactions', async () => { 
        
             // Send a transaction and wait for it to be mined. 
        
             const tx = DEFAULT_TRANSACTION 
        
             const result = await wallet.sendTransaction(tx) 
        
             const receipt = await result.wait() 
        
             const block = (await provider.getBlockWithTransactions( 
        
               receipt.blockHash 
        
             )) as any 
        
             expect(block.number).to.not.equal(0) 
        
             expect(typeof block.stateRoot).to.equal('string') 
        
             expect(block.transactions.length).to.equal(1) 
        
             expect(block.transactions[0].txType).to.equal('EIP155') 
        
             expect(block.transactions[0].queueOrigin).to.equal('sequencer') 
        
             expect(block.transactions[0].l1TxOrigin).to.equal(null) 
        
           })

And then observing the L2Geth logs. It should fail at const result = await wallet.sendTransaction(tx)

@ben-chain is going to help look into the compiler issue

ben-chain

Left a good number of comments; this is looking great! Need to figure out how to get the two deleted tests back, though -- not obvious to me what the dependency problem is there, but I'm assuming that's what it is.

ben-chain · 2021-04-20T23:24:51Z

l2geth/core/vm/evm.go

 	OriginalTargetReached bool
 	OvmExecutionManager   dump.OvmDumpAccount
 	OvmStateManager       dump.OvmDumpAccount
-	OvmMockAccount        dump.OvmDumpAccount


nit: this file diff seems unrelated and probably should have been a separate PR

ben-chain · 2021-04-20T23:26:24Z

packages/contracts/contracts/optimistic-ethereum/OVM/accounts/OVM_ECDSAContractAccount.sol

 pragma experimental ABIEncoderV2;

 /* Interface Imports */
+import { iOVM_ERC20 } from "../../iOVM/predeploys/iOVM_ERC20.sol";


Could this be OVM_ETH as we do have a strict assumption that it will be that specific ERC20?

ben-chain · 2021-04-20T23:27:41Z

packages/contracts/contracts/optimistic-ethereum/OVM/accounts/OVM_ECDSAContractAccount.sol

-        Lib_SafeExecutionManagerWrapper.safeREQUIRE(
-            decodedTx.chainId == Lib_SafeExecutionManagerWrapper.safeCHAINID(),
+        require(
+            decodedTx.chainId == Lib_ExecutionManagerWrapper.ovmCHAINID(),


could alternatively use Yul here now; so real preference from me though.

ben-chain · 2021-04-20T23:33:33Z

packages/contracts/contracts/optimistic-ethereum/OVM/accounts/OVM_ECDSAContractAccount.sol

        // // Need to make sure that the gas is sufficient to execute the transaction.
-        // Lib_SafeExecutionManagerWrapper.safeREQUIRE(
-        //    gasleft() >= Lib_SafeMathWrapper.add(decodedTx.gasLimit, EXECUTION_VALIDATION_GAS_OVERHEAD),
+        // Lib_ExecutionManagerWrapper.safeREQUIRE(


nit: this can be replaced with an explicit require, just like the nonce check above

ben-chain · 2021-04-20T23:34:25Z

packages/contracts/contracts/optimistic-ethereum/OVM/accounts/OVM_ECDSAContractAccount.sol

+                if iszero(created) {
+                    let size := returndatasize()
+                    revertdata := mload(0x40)
+                    mstore(0x40, add(revertdata, and(add(add(size, 0x20), 0x1f), not(0x1f))))
+                    mstore(revertdata, size)
+                    returndatacopy(add(revertdata, 0x20), 0x0, size)
+                }


This block is a little brittle, at minimum I think adding comments is required here. This could have been left as-is though, right? I have mixed feelings--on one hand, this block of assembly would make no sense in the EVM context, so it is a little confusing to read, and the functionality feels very "wrapper-ey" to me. OTOH, I get that it does add overhead to a reviewer's life.

ben-chain · 2021-04-21T00:07:30Z

packages/contracts/src/contract-dumps.ts

      predeploys[name] ||
      `0xdeaddeaddeaddeaddeaddeaddeaddeaddead${i.toString(16).padStart(4, '0')}`

+    let def: any


One problem this just raised for me is that because of unsupported: evm we will now be in the situation that contracts which use kall cannot be referenced by EVM contracts. Solvable with interfaces but we should think about if there's something more clever to be done here.

ben-chain · 2021-04-21T00:34:16Z

packages/contracts/test/contracts/OVM/accounts/OVM_ECDSAContractAccount.spec.ts

  before(async () => {
-    Factory__OVM_ECDSAContractAccount = await ethers.getContractFactory(
-      'OVM_ECDSAContractAccount'
+    Factory__OVM_ECDSAContractAccount = getContractFactory(


LOL very glad that this works in either runtime

ben-chain · 2021-04-21T00:36:49Z

packages/contracts/test/contracts/OVM/accounts/OVM_ECDSAContractAccount.spec.ts

-    Mock__OVM_ExecutionManager.smocked.ovmCALL.will.return.with([true, '0x'])
+    Mock__OVM_ExecutionManager.smocked.ovmCALL.will.return.with(
+      (gasLimit, target, data) => {
+        if (target === '0x4200000000000000000000000000000000000006') {


nit: getting this from the predeploys export would improve readability on this test

ben-chain · 2021-04-21T00:38:08Z

packages/contracts/test/contracts/OVM/accounts/OVM_ECDSAContractAccount.spec.ts

+    )
+    Mock__OVM_ExecutionManager.smocked.ovmSTATICCALL.will.return.with(
+      (gasLimit, target, data) => {
+        if (target === '0x0000000000000000000000000000000000000001') {


Suggested change

if (target === '0x0000000000000000000000000000000000000001') {

// smock ecrecover precompile

if (target === '0x0000000000000000000000000000000000000001') {

ben-chain · 2021-04-21T00:38:38Z

packages/contracts/test/contracts/OVM/accounts/OVM_ECDSAContractAccount.spec.ts

-      Mock__OVM_ExecutionManager.smocked.ovmCALL.will.return.with([false, '0x'])
+      Mock__OVM_ExecutionManager.smocked.ovmCALL.will.return.with(
+        (gasLimit, target, data) => {
+          if (target === '0x4200000000000000000000000000000000000006') {


ditto on lookup from predeploys

smartcontracts · 2021-04-21T18:40:41Z

Going to be dropping this PR in favor of several smaller PRs. Leaving this up as reference for myself.

gakonst · 2021-04-21T20:02:31Z

@smartcontracts closing - can still view this as a closed PR

feat: Have predeploys use OVM solc only

7387bff

tweaks to get tests running

e8baf2e

smartcontracts changed the title ~~feat: Have predeploys use OVM solc only~~ [WIP] feat: Have predeploys use OVM solc only Apr 15, 2021

smartcontracts added 2 commits April 15, 2021 18:04

got tests to work

a283439

fix nuisance gas check

36869a5

smartcontracts added 2 commits April 16, 2021 18:01

fix dump script

d76f0ce

Merge branch 'master' of github.com:ethereum-optimism/optimism into f…

3c9350c

…eat/predeploys-use-ovm-solc

karlfloersch mentioned this pull request Apr 19, 2021

Upgrade the Solidity compiler to simplify pre-deploy contract code #495

Closed

2 tasks

smartcontracts added 3 commits April 20, 2021 19:11

added back in some unit tests + use new compiler

a4f042a

fix nuisance gas test return value

e77b1cd

Fix incorrect naming in predeploys

7881bc6

ben-chain requested changes Apr 21, 2021

View reviewed changes

smartcontracts mentioned this pull request Apr 21, 2021

Port SequencerEntrypoint to use ovm solc #535

Merged

gakonst closed this Apr 21, 2021

gakonst deleted the feat/predeploys-use-ovm-solc branch April 21, 2021 20:02

This was referenced Apr 21, 2021

refactor[contracts]: Port OVM_ProxyEOA to use ovm-solc #545

Merged

refactor[contracts]: Port OVM_ECDSAContractAccount to use ovm-solc #546

Merged

refactor[contracts]: Port OVM_DeployerWhitelist to use ovm-solc #548

Merged

	if (target === '0x0000000000000000000000000000000000000001') {
	// smock ecrecover precompile
	if (target === '0x0000000000000000000000000000000000000001') {

[WIP] feat: Have predeploys use OVM solc only #475

[WIP] feat: Have predeploys use OVM solc only #475

Uh oh!

Conversation

smartcontracts commented Apr 15, 2021 • edited by karlfloersch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

smartcontracts commented Apr 16, 2021

Uh oh!

tynes commented Apr 17, 2021

Uh oh!

tynes commented Apr 17, 2021

Uh oh!

tynes commented Apr 17, 2021

Uh oh!

tynes commented Apr 17, 2021

Uh oh!

snario commented Apr 19, 2021

Uh oh!

karlfloersch commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ben-chain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smartcontracts commented Apr 21, 2021

Uh oh!

gakonst commented Apr 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

smartcontracts commented Apr 15, 2021 •

edited by karlfloersch

Loading

changeset-bot bot commented Apr 15, 2021 •

edited

Loading

karlfloersch commented Apr 20, 2021 •

edited

Loading