Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update mempool 1559 params #7170

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Update mempool 1559 params #7170

merged 2 commits into from
Dec 21, 2023

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Dec 21, 2023

This PR updates mempool 1559 params to update a little slower. Its reasoned about, due to low gas usage txs on mainnet being observed to be around 30M gas, so were not lowering fast enough, and were spiking too fast.

As a comparable ETH block time (15s) is 2.5x slower than us, and they update at a maximum +- of 12.5%. Under spam we are currently updating at (1 + (50/70)/10)**2.5 = 18.8% upwards. We currently update at 0% usage for 15 seconds at -23%. However we do not see 0% usage, we instead see usage around 20-40M, so lets say 30M. That puts us at (1 + (-40/70)/10)**2.5 = -13.7% .

So we see this asymmetrical upwards increase in our real settings. Lets assume that 30M is userflow/relayers, who are getting inputted Keplr's much higher gas numbers.

I suggest changing this target block gas param to 75M, which would then change

  • max update per block to: 6%
  • max update over 15 seconds to: 15.6%
  • max decrease per block (unchanged)
  • expected decrease per block (30M): -14.4%

However, the spam / old txs are at a lower fee. They only gets evicted once base fee exceeds recheck_factor * spam_fee_rate. Currently this is set at 4 for high conservatism, especially around clients who haven't patched. Almost all clients in the ecosystem have patched, so we can reduce this. Also clients have standardized around using a fee much higher than base fee.

This PR changes it 3. 3.3 gives the same guarantee to clients under max load with the above changes. (Follow the log line in the doc).

The change to 3 will make max gas usage take 19 blocks to evict instead of 21. I believe we can get away with far lower to be honest, due to wallets over-estimating fee right now.

The effects of this are:

  • I believe the recheck factor is what controls the height of the peaks. This reduction from 4->3, should cause a 25% reduction I hope, and probably better.
  • Lowering the max rate of change will smoothen out the curves, and lower "edge effects" causing higher tops than we need.
  • Raising target gas will help improve recovery time once we've past the over-filling point.
  • We may see periods of high block gas due to spam take a few more seconds to clear out

@ValarDragon ValarDragon added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v21.x backport patches to v21.x branch labels Dec 21, 2023
@ValarDragon
Copy link
Member Author

ValarDragon commented Dec 21, 2023

Osmosis mainnet basefee for the last 3 hours (ignore the blue line)

You see the high spikyness, and the downside being slower to come down than upside.
image

*/

var (
DefaultBaseFee = sdk.MustNewDecFromStr("0.01")
MinBaseFee = sdk.MustNewDecFromStr("0.0025")
MaxBaseFee = sdk.MustNewDecFromStr("10")
MaxBaseFee = sdk.MustNewDecFromStr("5")
Copy link
Member Author

Choose a reason for hiding this comment

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

This was just overdue. Have gotten multiple points of feedback from integrators asking for this. It doesn't really matter either way. Highest we've seen is .07.

@nicolaslara
Copy link
Contributor

This generally makes sense to me, but I think it's one of those things where we need to test it and see how it behaves under real load

Copy link
Contributor

@nicolaslara nicolaslara left a comment

Choose a reason for hiding this comment

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

code looks good

@ValarDragon ValarDragon merged commit 764886e into main Dec 21, 2023
1 check passed
@ValarDragon ValarDragon deleted the dev/update_1559_params branch December 21, 2023 15:23
mergify bot pushed a commit that referenced this pull request Dec 21, 2023
* Update mempool 1559 params

* recheck constant 3.3 -> 3

(cherry picked from commit 764886e)
ValarDragon added a commit that referenced this pull request Dec 21, 2023
* Update mempool 1559 params

* recheck constant 3.3 -> 3

(cherry picked from commit 764886e)

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
ValarDragon added a commit that referenced this pull request Dec 21, 2023
* Update mempool 1559 params

* recheck constant 3.3 -> 3

(cherry picked from commit 764886e)

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v21.x backport patches to v21.x branch A:no-changelog C:x/txfees V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants