Skip to content

implement more accurate & predicable priority fee suggestion algorithm for chains like Optimism#77

Merged
ajsutton merged 2 commits intoethereum-optimism:optimismfrom
roberto-bayardo:simplify-gas-estimate
Jun 5, 2023
Merged

implement more accurate & predicable priority fee suggestion algorithm for chains like Optimism#77
ajsutton merged 2 commits intoethereum-optimism:optimismfrom
roberto-bayardo:simplify-gas-estimate

Conversation

@roberto-bayardo
Copy link
Copy Markdown
Contributor

@roberto-bayardo roberto-bayardo commented Mar 30, 2023

Description

Provides a new max priority fee (aka "tip cap") suggestion algorithm appropriate for chains like Optimism with a single known block builder. In the typical case, which results whenever the last block had room for more transactions, the algorithm returns a minimum allowed priority fee. Otherwise it returns 10% over the median effective priority fee from the last block.

Tests

Includes unit tests covering at & under capacity cases.

Additional context

This fixes an issue we are encountering on Base goerli testnet, and I imagine affects other chains where blocks are not (yet) near full. Once a node sees a few transactions with a given priority fee, it effectively locks in that first-seen priority fee as its tip estimate even when there is plenty of blockspace. Without a bot or some other activity forcing txs with lower-than-estimated gas tip, it never drops.

@ajsutton
Copy link
Copy Markdown
Contributor

I think that makes a fair bit of sense to me. It's very predictable and fits with what needs to be done to get a tx on chain in reality.

I'd say it's worth adding some unit tests for the optimism gas price functionality to make sure we don't get regressions in the future. For a lot of stuff we write more end to end tests in the optimism monorepo but I suspect unit tests will be easier to write here and they should be separate enough that future upstream changes won't cause conflicts so maintenance should be low.

@roberto-bayardo
Copy link
Copy Markdown
Contributor Author

roberto-bayardo commented Apr 20, 2023

thanks for taking a look Adrian, yes I have some unit tests just haven't pushed them yet. I was waiting until we put this change through a load test before I am comfortable taking it out of draft mode.

@roberto-bayardo roberto-bayardo force-pushed the simplify-gas-estimate branch 5 times, most recently from a86455c to 81e8b6b Compare May 1, 2023 22:19
@roberto-bayardo roberto-bayardo force-pushed the simplify-gas-estimate branch from 81e8b6b to 99ffcfa Compare May 23, 2023 14:57
@roberto-bayardo
Copy link
Copy Markdown
Contributor Author

roberto-bayardo commented May 25, 2023

Taking this out of draft mode. Unit tests are now included, and we have done a preliminary load test that filled up blocks on a devchain and showed the priority fee suggestion increasing 10%. We still want to do a more thorough load test that uses the suggested fee and confirm the priority fee suggestion continues to rise in this scenario as blocks remain full.

(It's also been running on Base-goerli for > 1 month)

@roberto-bayardo roberto-bayardo marked this pull request as ready for review May 25, 2023 20:18
Copy link
Copy Markdown
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Sorry for the really long delay on this one - it looks good. Just worth making the min suggested fee customisable since there may be cases where the miner.gasprice is adjusted on the sequencer and ideally this estimated price should be that same value so its the minimum required to be included.

Comment thread eth/gasprice/optimism-gasprice.go Outdated
Comment on lines +16 to +17
// TODO: What's the recommended way to make this a configurable parameter?
MinSuggestedOptimismPriorityFee = big.NewInt(1e8 * params.Wei) // 0.1 gwei
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.

This probably should be the same value as used for the --miner.gasprice flag since that's the minimum price required for a transaction to be included at all. Alternatively it could be a new CLI flag that adds its value into gasprice.Config which gets passed through to NewOracle.

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.

As this is a fee "suggestion" I'd rather not override miner.gasprice, which is a hard minimum under which the tx would just be discarded. Let me look into gasprice.Config.

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.

Why would you want to always pay more than the minimum that would be accepted though? Making it separate probably makes sense so there's more control but I think I'd be setting it to be the same as the minimum miner price to minimise fees paid.

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.

@ajsutton the value is often described as a "tip", and the way I see it is a tip shouldn't be mandatory, but it's a reasonable question to ask as it's certainly the perogative of the block builder to reject a tx based on arbitrary logic. Until there is more than one builder though, maybe it's best to be permissive?

P.S.: it is in fact true that one can already specify a priority fee much lower than any existing suggestions (be they in wallets or as implemented in Ethereum) and the tx will still go through, since geth has such a low IgnorePrice by default. I do this all the time!

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.

Yeah my thinking is that for L2s where there's a big focus on keeping fees low (and the baseFee isn't burnt) then you'd want the estimation to always give the minimum tip that can be paid to be included which with a centralised sequencer would be the same as its miner.gasprice.

But it does make sense to keep them as separate config items because of the extra flexibility it gives. You can always set both options to the same value if you want to recommend paying the minimum tip that you'd accept.

@roberto-bayardo roberto-bayardo force-pushed the simplify-gas-estimate branch from 4ace92e to 2f3d09a Compare June 4, 2023 23:41
@roberto-bayardo
Copy link
Copy Markdown
Contributor Author

Pushed an additional commit that makes the value configurable via gasprice.Config.

Copy link
Copy Markdown
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

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