Skip to content

l2geth: sequencer fee buffer#1239

Merged
tynes merged 1 commit intodevelopfrom
fix/fee-buffer
Jul 9, 2021
Merged

l2geth: sequencer fee buffer#1239
tynes merged 1 commit intodevelopfrom
fix/fee-buffer

Conversation

@tynes
Copy link
Copy Markdown
Contributor

@tynes tynes commented Jul 8, 2021

Description

The fees are currently calculated as a sum of the L1 fee and the L2 fee
where the L1 fee is the approximate cost of the batch submission of the
transaction (L1 gas price * L1 gas used) and the L2 fee is the
approximate cost of the execution on L2 taking into account congestion
(L2 gas price * L2 gas limit).

When either the L1 or L2 gas price is volatile, it can result in the
quote that the user receives from eth_estimateGas to be rejected
as the fee that the Sequencer is expecting has gone up.

This PR adds logic to set a buffer in either direction of the current
price that will allow the sequencer to still accept transactions within.

Two new config options are added:

  • --rollup.feethresholddown or ROLLUP_FEE_THRESHOLD_DOWN
  • --rollup.feethresholdup or ROLLUP_FEE_THRESHOLD_UP

Note that these config options are only useful for when running
in Sequencer mode, they are not useful for replicas/verifiers.
This is because the Sequencer is the only write node in the network.

These config options are interpreted as floating point numbers and are
multiplied against the current fee that the sequencer is expecting.
To allow for a downward buffer of 10% and an upward buffer of 100%,
use the options:

  • ROLLUP_FEE_THRESHOLD_DOWN=0.9
  • ROLLUP_FEE_THRESHOLD_UP=2

This will allow for slight fee volatility downwards and prevent users
from excessively overpaying on fees accidentally.

This is a UX and profit tradeoff for the sequencer and can be exploited
by bots. If bots are consistently taking advantage of this, the max
threshold down will have to be calibrated to what the normal fee is
today.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jul 8, 2021

🦋 Changeset detected

Latest commit: c612a90

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


// PaysEnough returns an error if the fee is not large enough
// `GasPrice` and `Fee` are required arguments.
func PaysEnough(opts *PaysEnoughOpts) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be good to add more input validation -- eg. opts.ThresholdUp >= 1 & opts.ThresholdDown <= 1 && >= 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This validation happens in the constructor of the sync service

Copy link
Copy Markdown
Contributor

@karlfloersch karlfloersch Jul 8, 2021

Choose a reason for hiding this comment

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

Hmm I guess I slightly prefer co-locating the input validation with the struct declaration as much as possible, but not super strongly opinionated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem is a NewPaysEnoughOpts function would revert to having the problem that I was trying to avoid which was having a bunch of ordered arguments, the point of PaysEnoughOpts was that it would make it very explicit what the args were. We could add the checking of the threshold up and down in this function

@tynes tynes force-pushed the fix/fee-buffer branch 3 times, most recently from 15b32c4 to 34383a1 Compare July 8, 2021 20:49
The fees are currently calculated as a sum of the L1 fee and the L2 fee
where the L1 fee is the approximate cost of the batch submission of the
transaction (L1 gas price * L1 gas used) and the L2 fee is the
approximate cost of the execution on L2 taking into account congestion
(L2 gas price * L2 gas limit).

When either the L1 or L2 gas price is volatile, it can result in the
quote that the user receives from `eth_estimateGas` to be rejected
as the fee that the Sequencer is expecting has gone up.

This PR adds logic to set a buffer in either direction of the current
price that will allow the sequencer to still accept transactions within.

Two new config options are added:

- `--rollup.feethresholddown` or `ROLLUP_FEE_THRESHOLD_DOWN`
- `--rollup.feethresholdup` or `ROLLUP_FEE_THRESHOLD_UP`

Note that these config options are only useful for when running
in Sequencer mode, they are not useful for replicas/verifiers.
This is because the Sequencer is the only write node in the network.

These config options are interpreted as floating point numbers and are
multiplied against the current fee that the sequencer is expecting.
To allow for a downward buffer of 10% and an upward buffer of 100%,
use the options:

- `ROLLUP_FEE_THRESHOLD_DOWN=0.9`
- `ROLLUP_FEE_THRESHOLD_UP=2`

This will allow for slight fee volatility downwards and prevent users
from excessively overpaying on fees accidentally.

This is a UX and profit tradeoff for the sequencer and can be exploited
by bots. If bots are consistently taking advantage of this, the max
threshold down will have to be calibrated to what the normal fee is
today.

Both config options are sanity checked in the `SyncService` constructor
and will result in errors if they are bad. The threshold down must
be less than 1 and the threshold up must be greater than 1.
@tynes tynes force-pushed the fix/fee-buffer branch from 34383a1 to c612a90 Compare July 9, 2021 02:22
@tynes tynes merged commit 78bd450 into develop Jul 9, 2021
@tynes tynes deleted the fix/fee-buffer branch July 9, 2021 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants