Sequencer Fee Pricing Part 2: Electric Boogaloo: Override EstimateGas to take data price into account #273

gakonst · 2021-03-10T13:41:31Z

Description

Fixes https://github.com/ethereum-optimism/roadmap/issues/715:

implements gas estimation as follows: rollupTxSize * dataPrice + executionPrice * gasUsed, where executionPrice is fetched via the standard eth_gasPrice rules that geth uses based on the current l2 congestion, and dataPrice is a value set by the sequencer based on the current l1 congestion. gasUsed is the standard result of eth_estimateGas for a transaction, and rollupTxSize is the size of the serialized rollup tx if it were published on L1
1. eth_gasPrice is set to always return 1 to clients
2. eth_estimateGas returns the overridden value from step 1, this means that transactions with GasLimit > block.GasLimit are now valid, requiring that we disable a check in the mempool. It's not clear if this approach is DoS-safe.
the L1 gas price is automatically fetched from the Data Transport Layer at the beginning of the sequencing loop
allows the sequencer to configure the L1 Gas Price over a rollup_personal namespace via a setL1GasPrice method. This approach is nice because the gasPrice is stored in memory, configurable via CLI/RPC and requires nothing else.

internal/ethapi/api.go

eth/backend.go

tynes

This approach generally looks good so far

internal/ethapi/api.go

karlfloersch

Awesome! This looks great to me so far! Structure seems pretty perfect. I'm curious what is going wrong with the unit tests though hmm

internal/ethapi/api.go

tynes

Generally looks good to me, left a few comments

internal/ethapi/api.go

rollup/sync_service.go

internal/ethapi/api.go

tynes · 2021-03-18T16:02:16Z

core/rollup_fee.go

+/// ROLLUP_BASE_TX_SIZE is the encoded rollup transaction's compressed size excluding
+/// the variable length data.
+/// Ref: https://github.com/ethereum-optimism/contracts/blob/409f190518b90301db20d0d4f53760021bc203a8/contracts/optimistic-ethereum/OVM/precompiles/OVM_SequencerEntrypoint.sol#L47
+var ROLLUP_BASE_TX_SIZE int = 96


This will likely need to change due to ethereum-optimism/contracts#300
Also what do you think about making this a const?

👍 if that PR gets merged first, I'll adjust it. This means I'll have to serialize the transaction each time, instead of assuming that the data is constant size right?

Yup, will need to rlp.EncodeToBytes and use the length of that -

go-ethereum/rlp/encode.go

Line 70 in 2e46827

func EncodeToBytes(val interface{}) ([]byte, error) {

core/rollup_fee.go

rollup/sync_service.go

… creation

* experimenting with gas fudge factor * fix formatting * try using 1m gas constant * Add log statement for the gas estimate * Add more detail to gas estimate log * one more log edit * Try new formula * Bump base gas * Even more base gas * even more * Just 1m? * one more time * Final cleanup * Minor fix * Update internal/ethapi/api.go * don't use cap-1 * Make sure data is not nil Co-authored-by: Karl Floersch <[email protected]>

…C namespace

…vice (#277) * feat: pull L1GasPrice from the data transport service * refactor: split out single-iteration of sequencer loop * test(sync-service): ensure L1GasPrice is set correctly * chore: gofmt * fix: parse json response as string and test

* do not cast gasUsed * adjust comment on GasPrice method * add nil check for args.Data * log gas price changes in the sync service

Security Question: does this introduce a DoS vector?

tynes · 2021-04-09T03:35:39Z

core/tx_pool.go

 // whitelisted, preventing any associated transaction from being dropped out of the pool
 // due to pricing constraints.
 func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err error) {
+	log.Debug("received tx", "gas", tx.Gas(), "gasprice", tx.GasPrice().Uint64())


Transactions are not added to the TxPool so this should never log

karlfloersch

LGTM

gakonst force-pushed the feat/fee-pricing branch from e859f35 to 6c7971a Compare March 10, 2021 13:41

gakonst marked this pull request as draft March 10, 2021 13:43

gakonst force-pushed the feat/fee-pricing branch from 6c7971a to 8089d62 Compare March 10, 2021 13:45

gakonst requested a review from karlfloersch March 10, 2021 13:46

snario reviewed Mar 10, 2021

View reviewed changes

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

tynes reviewed Mar 11, 2021

View reviewed changes

eth/backend.go Outdated Show resolved Hide resolved

tynes reviewed Mar 11, 2021

View reviewed changes

eth/backend.go Outdated Show resolved Hide resolved

tynes reviewed Mar 11, 2021

View reviewed changes

internal/ethapi/api.go Show resolved Hide resolved

internal/ethapi/api.go Show resolved Hide resolved

karlfloersch reviewed Mar 11, 2021

View reviewed changes

internal/ethapi/api.go Show resolved Hide resolved

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

internal/ethapi/api.go Show resolved Hide resolved

gakonst changed the title ~~feat: Sequencer Fee Pricing~~ Sequencer Fee Pricing Part 2: Override EstimateGas to take data price into account Mar 11, 2021

gakonst force-pushed the feat/fee-pricing branch from 8089d62 to 2ebd400 Compare March 11, 2021 14:01

gakonst changed the base branch from master to fix/gas-estimation March 11, 2021 14:02

gakonst marked this pull request as ready for review March 11, 2021 14:07

smartcontracts changed the title ~~Sequencer Fee Pricing Part 2: Override EstimateGas to take data price into account~~ Sequencer Fee Pricing Part 2: Electric Boogaloo: Override EstimateGas to take data price into account Mar 11, 2021

gakonst force-pushed the fix/gas-estimation branch from da52ce3 to 5081b63 Compare March 17, 2021 18:37

gakonst force-pushed the feat/fee-pricing branch from 716f75f to a5c0c86 Compare March 18, 2021 10:40

gakonst changed the base branch from fix/gas-estimation to master March 18, 2021 10:43

gakonst changed the base branch from master to fix/gas-estimation March 18, 2021 10:43

gakonst requested a review from tynes March 18, 2021 10:48

tynes approved these changes Mar 18, 2021

View reviewed changes

gakonst force-pushed the fix/gas-estimation branch from 5081b63 to 117494d Compare March 23, 2021 10:41

gakonst force-pushed the feat/fee-pricing branch 2 times, most recently from b236ab7 to f6bf300 Compare March 23, 2021 10:52

gakonst changed the base branch from fix/gas-estimation to master March 23, 2021 11:22

gakonst force-pushed the feat/fee-pricing branch from f6bf300 to 479d23c Compare March 23, 2021 11:45

ben-chain and others added 2 commits March 27, 2021 18:51

fix: add nil check when estimating gas to avoid panic during contract…

4290b08

… creation

fix: revert DoEstimateGas changes introduced as a temp fix in #22

ad78282

gakonst force-pushed the feat/fee-pricing branch from ba71677 to aaa0a34 Compare March 27, 2021 16:55

smartcontracts and others added 2 commits March 29, 2021 16:50

feat(api): make eth_gasPrice always return 1 gwei

486d555

gakonst added 12 commits March 30, 2021 09:01

feat: overload estimateGas to return the data+execution fee

e99c376

feat: implement static L1 Gas Price oracle

0abd730

feat: allow configuring the L1GasPrice via CLI at node startup

eb3e487

feat: allow configuring the L1GasPrice remotely over a new private RP…

40fe63a

…C namespace

chore: expose L1Gpo in the sync service

a7eead8

refactor: create helper function for calculating the rollup fee

0603746

docs: add doc for rollup tx size constant

b7a2c6c

chore(console_test): add rollup_personal to test

147b81c

chore: remove empty line

68ba95a

chore: adjust review comments

ea47616

* do not cast gasUsed * adjust comment on GasPrice method * add nil check for args.Data * log gas price changes in the sync service

chore: re-order imports

32a4e5d

gakonst force-pushed the feat/fee-pricing branch from e99fba1 to 32a4e5d Compare March 30, 2021 06:01

gakonst and others added 4 commits March 30, 2021 09:26

fix: skip txpool max gas check for rollup txs

9113ae9

Security Question: does this introduce a DoS vector?

chore: debug log

42c8e30

fix(rollup_fee): normalize charged gas by the charged gas price

bcbbfb4

Merge branch 'master' into feat/fee-pricing

e02931d

smartcontracts self-requested a review as a code owner April 5, 2021 20:48

smartcontracts added 4 commits April 5, 2021 17:35

remove intrinsic gas checks

45b3516

move intrinsic gas check behind non-ovm codepath

4b78e4e

remove fudging code

f5eb534

fix wrong gas limit

65f2d58

tynes mentioned this pull request Apr 8, 2021

fix: Have OVM provide all available gas to txs #302

Closed

1 task

tynes reviewed Apr 9, 2021

View reviewed changes

karlfloersch approved these changes Apr 9, 2021

View reviewed changes

tynes merged commit 30b541a into master Apr 9, 2021

tynes deleted the feat/fee-pricing branch April 9, 2021 04:01

Sequencer Fee Pricing Part 2: Electric Boogaloo: Override EstimateGas to take data price into account #273

Sequencer Fee Pricing Part 2: Electric Boogaloo: Override EstimateGas to take data price into account #273

Uh oh!

Conversation

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

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tynes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

karlfloersch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tynes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tynes Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

gakonst Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

tynes Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tynes Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

karlfloersch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gakonst commented Mar 10, 2021 •

edited

Loading