Skip to content

feat: set L2 execution price manually #557

Merged
tynes merged 9 commits intomasterfrom
feat/manual-execution-price
Apr 27, 2021
Merged

feat: set L2 execution price manually #557
tynes merged 9 commits intomasterfrom
feat/manual-execution-price

Conversation

@gakonst
Copy link
Copy Markdown
Contributor

@gakonst gakonst commented Apr 22, 2021

  • Fixes executionPrice is set via the gasPrice #536
  • Renames L1GasPrice to DataPrice
  • Adds ExecutionPrice as an extra sequencer-configured parameter, which is exposed via the private rollup APIs. These are accessible via IPC, or can be exposed via RPC behind a proxy.
  • The ExecutionPrice expresses congestion fees, and is used instead of Geth's internal SuggestPrice method, since that uses the gasPrice field from historical transactions, which we've overridden to be a meaningless constant.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 22, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: 97a1df9

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

This PR includes changesets to release 1 package
Name Type
@eth-optimism/l2geth 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

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Apr 22, 2021

After some brainstorming about setting the ExecutionPrice longer term, if it is set via IPC then the problem is that it will also need to be set via IPC for all replicas/verifiers at the same time. This is a large coordination problem that adds a lot of overhead, basically if we want to up the fee then we will need to have everybody reconfigure their nodes. Longer term the ExecutionPrice could live in the L2 state. The value could be updated behind an onlySequener modifier and the Sequencer RPC would reject transactions that do not pay a large enough fee. A problem with this approach is that verifiers that listen to L1 will have a lag in updates to the value since they will not see the updates until the transactions are batch submitted. This problem could be mitigated by only allowing fee changes every so many blocks and allowing it to only change by deterministic amounts, but that adds complexity and the problem space should be explored in greater depth. Verifiers will always be at a disadvantage compared to replicas, since replicas sync from the sequencer but there is risk involved since that data isn't technically the source of truth. A malicious sequencer may lie.

Short term we do not need this value to be pulled from the L2 state but we definitely would like it to be pulled from the L2 state before the Uniswap v3 launch. We would be able send transactions to the sequencer that updates the ExecutionPrice from a multisig to make sure the system does not get overwhelmed

@tynes tynes mentioned this pull request Apr 22, 2021
Closed
@tynes
Copy link
Copy Markdown
Contributor

tynes commented Apr 23, 2021

Having the ExecutionPrice in the state is not a short term blocker but it is a blocker for UniswapV3.
Related:

@gakonst
Copy link
Copy Markdown
Contributor Author

gakonst commented Apr 25, 2021 •
edited
Loading

@tynes In that case, it feels like we can merge this feature and roll it out ASAP, and then we can implement the smart contract-based solution in a separate PR?

tynes
tynes reviewed Apr 26, 2021
View reviewed changes
tynes
tynes requested changes Apr 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be ready to merge once the RollupOracle is safe for concurrent usage

@gakonst gakonst force-pushed the feat/manual-execution-price branch from 67ee897 to 97a1df9 Compare April 27, 2021 11:44
@gakonst
Copy link
Copy Markdown
Contributor Author

gakonst commented Apr 27, 2021

Rebased to resolve the conflict and made the GPO methods thread safe in 97a1df9

@tynes tynes merged commit 79f66e9 into master Apr 27, 2021
@tynes tynes deleted the feat/manual-execution-price branch April 27, 2021 17:19
ben-chain pushed a commit to ben-chain/optimism that referenced this pull request May 6, 2021
@gakonst @ben-chain
feat: set L2 execution price manually (ethereum-optimism#557)
fb325b6 
* refactor(l2geth): rename L1GasPrice to DataPrice

* feat(l2geth/rollup-gpo): allow execution price to be specified

* feat(l2geth/api): use the execution price instead of the historical gasprice

* feat(l2geth): allow configuring the l2 execution price

* fix(integration-tests): adjust gas costs to match 0-execution price

* fix(integration-tests): pin down l1 gas estimation costs to the expected values

* chore: add changeset

* fix(sync-service): adjust tests for the refactor

* feat(rollup-gasprice): make Rollup GPO methods thread safe
InoMurko referenced this pull request in omgnetwork/optimism May 25, 2021
@gakonst
feat: set L2 execution price manually (#557)
90a16a9 
* refactor(l2geth): rename L1GasPrice to DataPrice

* feat(l2geth/rollup-gpo): allow execution price to be specified

* feat(l2geth/api): use the execution price instead of the historical gasprice

* feat(l2geth): allow configuring the l2 execution price

* fix(integration-tests): adjust gas costs to match 0-execution price

* fix(integration-tests): pin down l1 gas estimation costs to the expected values

* chore: add changeset

* fix(sync-service): adjust tests for the refactor

* feat(rollup-gasprice): make Rollup GPO methods thread safe
theochap pushed a commit that referenced this pull request Jan 15, 2026
@RomanHodulak
feat(rpc): Add OpTransactionRequest and associate it with `Transact…
3f6f29c 
…ionRequest` of `Optimism` (#557)

## Motivation

The change #525 introduced ambiguity of
`alloy_network::TransactionBuilder` implementation for
`TransactionRequest`, being implemented twice once for `Ethereum` and
once for `Optimism`. This then requires fully qualified syntax to
differentiate.

## Solution

Bring back `OpTransactionRequest`

## PR Checklist

- [ ] Added Tests
- [ ] Added Documentation
- [ ] Breaking changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tynes tynes tynes approved these changes

@karlfloersch karlfloersch Awaiting requested review from karlfloersch

@smartcontracts smartcontracts Awaiting requested review from smartcontracts

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

executionPrice is set via the gasPrice

2 participants

@gakonst @tynes