feat: Use standard RLP transactions by smartcontracts · Pull Request #300 · ethereum-optimism/contracts

smartcontracts · 2021-03-08T02:50:22Z

Description

Updates our sequencer entrypoint and ECDSA contracts to use the standard RLP encoding that we're using in geth now.

Metadata

Fixes

Fixes roadmap#754

Contributing Agreement

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

smartcontracts · 2021-03-08T02:50:45Z

contracts/optimistic-ethereum/OVM/accounts/OVM_ProxyEOA.sol


 /* Library Imports */
 import { Lib_Bytes32Utils } from "../../libraries/utils/Lib_Bytes32Utils.sol";
-import { Lib_OVMCodec } from "../../libraries/codec/Lib_OVMCodec.sol";


Files were unused, I removed them.

smartcontracts · 2021-03-08T02:50:58Z

contracts/optimistic-ethereum/OVM/precompiles/OVM_SequencerEntrypoint.sol

-import { Lib_BytesUtils } from "../../libraries/utils/Lib_BytesUtils.sol";
-import { Lib_OVMCodec } from "../../libraries/codec/Lib_OVMCodec.sol";
-import { Lib_ECDSAUtils } from "../../libraries/utils/Lib_ECDSAUtils.sol";
+import { Lib_EIP155Tx } from "../../libraries/codec/Lib_EIP155Tx.sol";


This file gets way simpler.

smartcontracts · 2021-03-08T02:51:30Z

contracts/optimistic-ethereum/libraries/codec/Lib_EIP155Tx.sol

@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >0.5.0 <0.9.0;


Pulled decoding logic into this file to make review easier. It's mostly the same as what was originally in OVM codec. We can confirm this with some careful review.

smartcontracts · 2021-03-08T02:52:01Z

contracts/optimistic-ethereum/libraries/utils/Lib_ECDSAUtils.sol

@@ -1,98 +0,0 @@
-// SPDX-License-Identifier: MIT


File is no longer needed.

smartcontracts · 2021-03-08T02:52:23Z

contracts/optimistic-ethereum/mockOVM/accounts/mockOVM_ECDSAContractAccount.sol

@@ -1,95 +0,0 @@
-// SPDX-License-Identifier: MIT


We don't use this in geth and we shouldn't use this in ethereumjs-vm.

smartcontracts · 2021-03-08T02:52:36Z

test/data/json/libraries/codec/Lib_OVMCodec.test.json

@@ -1,22 +1,4 @@
 {
    "tests": {
-        "decompressEIP155Transaction": {


Tests move to new eip155tx library.

contracts/optimistic-ethereum/libraries/codec/Lib_EIP155Tx.sol

tynes · 2021-03-08T04:28:15Z

contracts/optimistic-ethereum/libraries/codec/Lib_EIP155Tx.sol

+        address to;
+        uint256 value;
+        bytes data;
+        uint8 recoveryParam;


This is basically just caching the recovery param so that it is only computed once instead of needing to compute it when calling sender which is only called once and is always called. Might save gas to use it depending on how well packed this struct can be made

Right. Given a chain ID, the recovery param will never change. So it seems a bit easier to compute this once (we do actually end up computing this in two different places otherwise).

tynes · 2021-03-08T04:33:45Z

Overall good changes. The abi is changing, we need to make sure to not break everything when we publish these changes. Also we dont have hardhat deploy so there are no deployment artifacts in this repo so we need to be mindful about this change.

tynes · 2021-03-08T04:47:35Z

contracts/optimistic-ethereum/OVM/precompiles/OVM_SequencerEntrypoint.sol

-        TransactionType transactionType = _getTransactionType(Lib_BytesUtils.toUint8(msg.data, 0));
-
-        bytes32 r = Lib_BytesUtils.toBytes32(Lib_BytesUtils.slice(msg.data, 1, 32));
-        bytes32 s = Lib_BytesUtils.toBytes32(Lib_BytesUtils.slice(msg.data, 33, 32));


Removes the calls to slice which is good

tynes · 2021-03-08T04:49:17Z

The calldata size of using a RLP encoded tx is about ~250 gas more than using the custom serialization. Trading a bit of cost for simplicity, but its possible that the gas usage was decreased elsewhere in these changes. We really need automated gas metering in CI

tynes · 2021-03-08T04:49:52Z

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

 import { Lib_SafeExecutionManagerWrapper } from "../../libraries/wrappers/Lib_SafeExecutionManagerWrapper.sol";
 import { Lib_SafeMathWrapper } from "../../libraries/wrappers/Lib_SafeMathWrapper.sol";

+import { console } from "hardhat/console.sol";


I know this is WIP, but just noting to remove this

smartcontracts · 2021-03-08T07:58:15Z

contracts/optimistic-ethereum/libraries/rlp/Lib_RLPWriter.sol

+            bytes memory _out
+        )
+    {
+        return writeBytes(abi.encodePacked(_in));


bytes32 isn't treated any differently from bytes when it comes to RLP.

smartcontracts · 2021-03-08T07:58:40Z

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

-      const message = serializeNativeTransaction(DEFAULT_EIP155_TX)
-      const sig = await signNativeTransaction(wallet, DEFAULT_EIP155_TX)
+      const transaction = DEFAULT_EIP155_TX
+      const encodedTransaction = await wallet.signTransaction(transaction)


These tests get way simpler.

smartcontracts · 2021-03-08T08:34:51Z

test/data/json/libraries/codec/Lib_EIP155Tx.test.json

@@ -0,0 +1,248 @@
+{


Using this test file as the source of these tests: https://github.com/ethereumjs/ethereumjs-tx/blob/master/test/ttTransactionTestEip155VitaliksTests.json

I didn't test hash here, but sender implicitly tests this under the hood so it should be fine.

Covers tests for decode, encode, and sender.

smartcontracts · 2021-03-08T08:45:12Z

test/data/json/libraries/codec/Lib_EIP155Tx.test.json

+                        "0x044852b2a670ade5407e78fb2863c51de9fcb96542a07186fe3aeda6bb8a116d",
+                        "0x01",
+                        "0x00",
+                        false


Format is:

[ "0x00", // nonce "0x04a817c800", // gasPrice "0x5208", // gasLimit "0x3535353535353535353535353535353535353535", // to "0x00", // value "0x", // data "0x25", // v "0x044852b2a670ade5407e78fb2863c51de9fcb96542a07186fe3aeda6bb8a116d", // r "0x044852b2a670ade5407e78fb2863c51de9fcb96542a07186fe3aeda6bb8a116d", // s "0x01", // chainId "0x00", // recoveryParam false // isCreate ]

smartcontracts · 2021-03-08T08:46:17Z

test/helpers/test-runner/json-test-runner.ts

 import { Contract, BigNumber } from 'ethers'

-const bigNumberify = (arr) => {
+const bnRegex = /^\d+n$/gm


Made this generally more reliable. Treat more things as big numbers so that equality checking is easier.

tynes · 2021-03-08T16:41:57Z

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

    })

    // TEMPORARY: Skip gas checks for minnet.
    it.skip(`should revert on insufficient gas`, async () => {


Skipped test has been modified. Should double check it still passes

tynes · 2021-03-13T21:15:07Z

Note that this change must be done in a way that is backwards compatible with the current ctc elements. From the point of view of geth, the old contract must be used when syncing the initial set of transactions. This makes me lean towards not changing the contract and adding a new one that is OVM_SequencerEntrypointv2. This way geth can use the old sequencer entrypoint to sync and switch out at a certain point.

This means that geth needs the old serialization logic still and we need a way to selectively set rawTransaction in the data transport layer based on the how the transaction is serialized

gakonst · 2021-03-19T12:21:26Z

CI Fails at:

h_l2_1               | DEBUG[03-16|01:02:16.924] Calling Known Contract                   ID=85f757a4 Name=OVM_ExecutionManager Method=run
geth_l2_1               | DEBUG[03-16|01:02:16.924] Calling Known Contract                   ID=85f757a4 Name=OVM_StateManager     Method=isAuthenticated
geth_l2_1               | DEBUG[03-16|01:02:16.924] Calling Known Contract                   ID=85f757a4 Name=OVM_StateManager     Method=hasAccount
geth_l2_1               | DEBUG[03-16|01:02:16.924] Calling Known Contract                   ID=85f757a4 Name=OVM_StateManager     Method=testAndSetAccountLoaded
geth_l2_1               | DEBUG[03-16|01:02:16.924] Calling Known Contract                   ID=85f757a4 Name=OVM_StateManager     Method=getAccountEthAddress
geth_l2_1               | DEBUG[03-16|01:02:16.924] Calling Known Contract                   ID=85f757a4 Name=OVM_SequencerEntrypoint Message="no method with id: 0x00dede53"
geth_l2_1               | DEBUG[03-16|01:02:16.924] Calling Known Contract                   ID=85f757a4 Name=OVM_ExecutionManager    Method=ovmCHAINID
geth_l2_1               | DEBUG[03-16|01:02:16.925] Reached the end of an OVM execution      ID=85f757a4 Return Data=0x Error="ovm execution failed"
geth_l2_1               | INFO [03-16|01:02:16.925] New block                                index=5     l1-timestamp=1615856583 l1-blocknumber=64 tx-hash=0x40483b4c114a3330ce0afb10500ad4ca304c1a327c5fcab06f0b9b19353074de queue-orign=0 type=0 gas=294814 fees=0 elapsed=4.505ms

Depends on ethereum-optimism/contracts#300

maurelian · 2021-03-23T13:09:06Z

Converting to draft until checks pass.

maurelian · 2021-04-09T15:24:28Z

Is this dead?

feat: Use standard RLP transactions

35f48aa

smartcontracts requested review from ben-chain and maurelian as code owners March 8, 2021 02:50

smartcontracts marked this pull request as draft March 8, 2021 02:50

smartcontracts commented Mar 8, 2021

View reviewed changes

Better support for chain IDs

0cdf826

tynes reviewed Mar 8, 2021

View reviewed changes

contracts/optimistic-ethereum/libraries/codec/Lib_EIP155Tx.sol Show resolved Hide resolved

tynes reviewed Mar 8, 2021

View reviewed changes

contracts/optimistic-ethereum/libraries/codec/Lib_EIP155Tx.sol Show resolved Hide resolved

tynes reviewed Mar 8, 2021

View reviewed changes

smartcontracts added 2 commits March 7, 2021 23:47

Cleanup files and add comments

d722c02

Expand on why uint8 is ok

4d8d722

smartcontracts commented Mar 8, 2021

View reviewed changes

Added tests for decoding

b57a94b

smartcontracts commented Mar 8, 2021

View reviewed changes

Added encoding and sender tests

0b76bd9

smartcontracts commented Mar 8, 2021

View reviewed changes

Updated config so dump won't throw

e6ecb6e

tynes reviewed Mar 8, 2021

View reviewed changes

smartcontracts marked this pull request as ready for review March 8, 2021 23:06

Merge branch 'master' into feat/standard-rlp-transaction

9d87dd3

smartcontracts changed the title ~~[WIP] feat: Use standard RLP transactions~~ feat: Use standard RLP transactions Mar 8, 2021

This was referenced Mar 9, 2021

feat: raw tx ethereum-optimism/go-ethereum#255

Merged

tests: fix the enums to match what is batch submitted #303

Closed

types: fix enums to match batch submitter ethereum-optimism/go-ethereum#145

Closed

smartcontracts mentioned this pull request Mar 12, 2021

Improve clarity of chainID recovery logic #243

Open

merge master

b3f694f

platocrat mentioned this pull request Mar 16, 2021

[WIP] Clarifying comments for chainID recovery logic #272

Closed

1 task

tynes mentioned this pull request Mar 18, 2021

Sequencer Fee Pricing Part 2: Electric Boogaloo: Override EstimateGas to take data price into account ethereum-optimism/go-ethereum#273

Merged

gakonst added a commit to ethereum-optimism/go-ethereum that referenced this pull request Mar 23, 2021

feat: calculate fees using RLP encoded transactions

dda664a

Depends on ethereum-optimism/contracts#300

maurelian marked this pull request as draft March 23, 2021 13:08

resolve merge conflicts

957de33

smartcontracts added Breaks: Batch Submitter Breaks: Geth L2 Status: WIP labels Mar 23, 2021

maurelian changed the base branch from master to OZ April 9, 2021 00:27

maurelian mentioned this pull request Apr 9, 2021

Oz conflicts resolved #375

Open

maurelian changed the base branch from OZ to master April 11, 2021 22:32

tynes mentioned this pull request Apr 13, 2021

geth: only accept txs with a 0 value ethereum-optimism/optimism#435

Merged

maurelian mentioned this pull request Apr 16, 2021

Import changes from deprecated contracts repo ethereum-optimism/optimism#476

Closed

snario mentioned this pull request Apr 19, 2021

[WIP] feat: Have predeploys use OVM solc only ethereum-optimism/optimism#475

Closed

		@@ -0,0 +1,111 @@
		// SPDX-License-Identifier: MIT
		pragma solidity >0.5.0 <0.9.0;

Conversation

smartcontracts commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Metadata

Fixes

Contributing Agreement

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented Mar 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented Mar 8, 2021

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!

tynes commented Mar 13, 2021

Uh oh!

gakonst commented Mar 19, 2021

Uh oh!

maurelian commented Mar 23, 2021

Uh oh!

maurelian commented Apr 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smartcontracts commented Mar 8, 2021 •

edited

Loading