l2geth: updated calculate rollup fee by tynes · Pull Request #906 · ethereum-optimism/optimism

tynes · 2021-05-18T21:18:50Z

Description
PR that implements the transaction fee scheme describe in: #823 (comment)

This scheme encodes the L2 Gas Price inside of the L1 Gas Limit which also encodes the full fee because the gas price is set to 1 wei. The L2 gas price is encoded in the lower order bits of the tx.gasLimit using a scheme by which allows the L2 gas limit to be human readable as well as easily recoverable. The gas limits must follow the form:

The L1 gas price must satisfy the equation `x * (10**8)`
The L2 gas price must satisfy the equation `x * (10**8) + 1`

It also adds new functionality to the L2 GasPrice Oracle where it polls the L2 state for a particular storage slot for the current L2 gasprice. It is held in memory and is used to reject transactions coming in via RPC that have too low of a gas price. It is also used in the calculation of the fee for eth_estimateGas. (The fee is returned here because eth_gasPrice returns 1). This gasprice can be updated via a transaction sent to the OVM_GasPriceOracle contract, see #936

It adds new config options:

"ROLLUP_GAS_PRICE_ORACLE_ADDRESS" - address of the OVM_GasPriceOracle
"ROLLUP_ENABLE_L2_GAS_POLLING" - allows for backwards compatibility by turning off the gas price polling
"ROLLUP_ENFORCE_FEES" - turns off accepting 0 gasprice transactions

A new RPC endpoint is added called eth_estimateExecutionGas that mimics the L1 eth_estimateGas. This will allow users to know how much gas a transaction will use without needing to decode the value from the L2 eth_estimateGas

Some of the files got changed due to a yarn lint

Metadata

changeset-bot · 2021-05-18T21:18:52Z

🦋 Changeset detected

Latest commit: 81e32b1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@eth-optimism/integration-tests	Patch
@eth-optimism/l2geth	Patch
@eth-optimism/core-utils	Patch

Not sure what this means? Click here to learn what changesets are.

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

codecov-commenter · 2021-05-18T21:30:00Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@e3b138b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop     #906   +/-   ##
==========================================
  Coverage           ?   82.21%           
==========================================
  Files              ?       48           
  Lines              ?     1895           
  Branches           ?      303           
==========================================
  Hits               ?     1558           
  Misses             ?      337           
  Partials           ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3b138b...81e32b1. Read the comment docs.

tynes · 2021-05-19T05:12:05Z

I'd like to improve the testing of this, it will build on #912

l2geth/core/rollup_fee.go

tynes · 2021-05-20T05:25:32Z

Integration tests are failing with invalid L2 gas price: rollup fee: invalid gas price - I wonder if the gasPrice is being hardcoded as 0, it could be hitting the bug in my implementation of the algorithm where 0 and 1 don't correctly work. See the todo in the tests

This has been fixed and is no longer an issue. Passing in the L2 gasprice of 0 is a special case that is handled

l2geth/eth/gasprice/rollup_gasprice.go

* l2geth: rollup gas price oracle state polling * l2geth: read l2 gasprice from the tip * l2geth: add config options for L2 gas price * l2geth: comment to remove code in future

* core-utils: add fees package * integration-tests: refactor for new fees * l2geth: end to end fee spec * l2geth: use new env var * deps: regenerate * lint: fix * l2geth: fee verification * tests: update gas prices * tests: update gas prices * tests: cleanup * l2geth: small cleanup * l2geth: fix max * feat: fix fee calculations with bigints * tests: fix * tests: lint * core-utils: rename fees to L2GasLimit * l2geth: fix comment * l2geth: fix name of env var * l2geth: delete extra print statement * l2geth: fix logline * tests: fix typo * l2geth: improve readability

tynes · 2021-05-25T01:41:35Z

integration-tests/test/native-eth.spec.ts

      const addr = '0x' + '1234'.repeat(10)
      const gas = await env.ovmEth.estimateGas.transfer(addr, amount)
-      expect(gas).to.be.deep.eq(BigNumber.from(21000))
+      expect(gas).to.be.deep.eq(BigNumber.from(0x35008e17cb6d))


Decode and assert against the gas limit to decouple actual value being hardcoded

I'm going to leave this out of scope for this PR and we should address the fact that the integration tests are coupled such that they all cannot run against kovan/goerli in an additional PR

integration-tests/test/rpc.spec.ts

l2geth/cmd/geth/main.go

l2geth/cmd/utils/flags.go

l2geth/core/rollup_fee.go

l2geth/internal/ethapi/api.go

tynes · 2021-05-25T23:39:52Z

Attempting to game the system by handcrafting an RLP transaction. I created a transaction and then updated the gasLimit (see 0xff.....) by adding an additional byte. This require me to increment the first byte (0xe5 -> 0xe6) and the prefix (0x88 -> 0x89)

Trying to deserialize this resulted in an error

Simple Tx
0xe5808088ffffffffffffffff9400000000000000000000000000000000000000008000808080
Evil Tx
0xe6808089ffffffffffffffffff9400000000000000000000000000000000000000008000808080
rlp: input string too long for uint64, decoding into (types.Transaction)(types.txdata).GasLimit

Users that send transactions with too large of a fee will fail to have their transaction accepted by the sequencer. Currently, the eth_estimateGas result will overflow and result in a small fee that will end up being rejected. I think the best way would be to return an error message. This should never happen unless the fee is larger than 18.44... ETH

$ python
Python 3.8.4 (default, Jul 15 2020, 10:38:22)
[GCC 10.1.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> max_u64 = 0xFFFFFFFFFFFFFFFF
>>> max_u64 * 10**-18
18.446744073709553

l2geth/core/rollup_fee.go

l2geth/rollup/sync_service.go

l2geth/core/rollup_fee.go

l2geth/rollup/sync_service.go

integration-tests/test/rpc.spec.ts

l2geth/rollup/sync_service.go

tynes · 2021-05-26T00:29:47Z

l2geth/rollup/sync_service.go

+		return nil
+	}
+	// Make sure that the fee is paid
+	if tx.Gas() < fee.Uint64() {


Analyze volatile fees on L1 causing this to fail

are these like reminders you set to yourself?

packages/core-utils/src/fees.ts

smartcontracts · 2021-05-26T00:54:46Z

I went through this with @tynes via phone call. We left various comments using @tynes's account. I'm going to work on a small update to the documentation that explains this new fee mechanism and what users should be on the lookout for.

Signed-off-by: piguagua <piguagua@aliyun.com>

github-actions bot added the A-geth label May 18, 2021

tynes force-pushed the feat/gaslimit-encoding branch 2 times, most recently from 8e1cf35 to 6636805 Compare May 19, 2021 05:11

l2geth: updated calculate rollup fee

6636805

tynes added 3 commits May 19, 2021 19:28

l2geth: implement the gasprice serialization

739ef48

lint: fix

2ec5aba

rollup-test: fix

d09e81c

tynes commented May 20, 2021

View reviewed changes

l2geth/core/rollup_fee.go Outdated Show resolved Hide resolved

tynes commented May 20, 2021

View reviewed changes

l2geth/core/rollup_fee.go Outdated Show resolved Hide resolved

tynes mentioned this pull request May 20, 2021

l2geth: gaslimit encoding state polling #927

Merged

tynes commented May 20, 2021

View reviewed changes

l2geth/eth/gasprice/rollup_gasprice.go Show resolved Hide resolved

Merge branch 'develop' into feat/gaslimit-encoding

7765be0

github-actions bot added the 2-reviewers label May 20, 2021

tynes added 3 commits May 20, 2021 13:21

l2geth: gaslimit encoding state polling (#927)

7b1801e

* l2geth: rollup gas price oracle state polling * l2geth: read l2 gasprice from the tip * l2geth: add config options for L2 gas price * l2geth: comment to remove code in future

l2geth: handle 0 and 1 fees

89bd118

l2geth: enable 0 and 1 fee tests

4c4df82

tynes mentioned this pull request May 24, 2021

fix(l2geth): do not throw an error when estimating gas for txs with nil data #940

Merged

github-actions bot added A-integration-tests A-pkg-core-utils Area: packages/core-utils A-ops Area: ops labels May 24, 2021

tynes added 3 commits May 24, 2021 16:03

Merge branch 'develop' into feat/gaslimit-encoding

8541b7c

chore: add changeset

3a0278f

l2geth: fix compiler error

619795d

tynes commented May 25, 2021

View reviewed changes

integration-tests/test/rpc.spec.ts Show resolved Hide resolved

tynes commented May 25, 2021

View reviewed changes

l2geth/cmd/geth/main.go Show resolved Hide resolved

tynes commented May 25, 2021

View reviewed changes

l2geth/cmd/utils/flags.go Show resolved Hide resolved

tynes commented May 25, 2021

View reviewed changes

l2geth/core/rollup_fee.go Outdated Show resolved Hide resolved

tynes commented May 25, 2021

View reviewed changes

l2geth/internal/ethapi/api.go Outdated Show resolved Hide resolved

tynes force-pushed the feat/gaslimit-encoding branch from 27800da to a76fd99 Compare May 25, 2021 21:09

tynes added 4 commits May 25, 2021 14:09

feat: clean up and fix fees

a76fd99

lint: fix

588cbf7

core-utils: refactor api to be more friendly

7e01267

lint: fix

e9de730

tynes commented May 25, 2021

View reviewed changes

l2geth/core/rollup_fee.go Outdated Show resolved Hide resolved

tynes commented May 25, 2021

View reviewed changes

l2geth/core/rollup_fee.go Show resolved Hide resolved