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

Add a --gpo.ignoreprice option to ignore MEV-priced txs #22752

Merged
merged 2 commits into from
May 11, 2021

Conversation

ryanschneider
Copy link
Contributor

We had a user report that periodically eth_gasPrice would return insanely low values, either 6gwei or 10gwei when moments before it was reporting seemingly correct values well over 100gwei.

The first step to track down the issue was to reproduce it, which was hard since eth_gasPrice only operates on the current block. We had some metrics on gas prices, but were only recording at ~15m granularity, so usually weren't catching these dips.

So, to debug the issue, I created this commit (ryanschneider@6acdbb4) that added a eth_historicalGasPrice RPC for testing. However, to keep the current PR focused, I did NOT include this RPC in this PR; if we feel it has value we can discuss adding it separately.

With this change deployed on a client, I was able to find historical blocks where the GPO dipped down drastically, for example around block 12275922 where the GPO predicted a 6gwei gas price. I'm not that familiar with the gas price oracle, but it appeared that the issue was caused by MEV txs with 0 or very low gas prices were skewing the results.

Then, I scanned all the recent blocks starting at 12200000 and created this CSV (https://gist.githubusercontent.com/ryanschneider/001d50cbd73e3b93d70c8a54c4aeb35b/raw/25923cfd54dc95b6cca02530b5ce144dc2b4a44a/mev.csv) that captured every block where eth_gasPrice would've returned a different value if txs below 10gwei were discarded. As you can see, this gives very different results at some points:

12275920,301000001459,313000000000
12275921,301000001459,313000000000
12275922,6000000000,304000001459
12275923,6000000000,304000001459
12275924,6000000000,304000001459
12275925,6000000000,304000001459
12275926,6000000000,303000000000
12275927,6000000000,301000001460
12275928,6000000000,300000001347
12275929,9000000000,300000000000
12275930,6000000000,300000000000
12275931,9000000000,300000000000
12275932,262000000000,300000000000
12275933,10000000000,300000000000
12275934,10000000000,294000000000

At block 12275922, the standard eth_gasPrice would return 6gwei, but would return 304gwei if the txs below 10gwei were ignored.

However, since networks other than mainnet might have totally different gas pricing rules, I think it makes sense to make this cut-off value user configurable, so that's what this PR does, it adds a --gpo.ignoreprice option that defaults to 0, any txs processed by the GPO with a gas price strictly below the configured value is ignored.

It's possible there's something more subtle going on here that adding this exclusion is glossing over, so if someone familiar with the GPO can help provide context I'm happy to dig in deeper to see why a relatively small number of MEV txs are able to skew the results so much.

@fjl
Copy link
Contributor

fjl commented Apr 27, 2021

This is also being worked on in #22722

@ryanschneider
Copy link
Contributor Author

Oops, also pointed to wrong historical gas price RPC commit, this one here supports ignoring txs under a passed-in price, and is the one I used to generate the CSV: ryanschneider@8c6531e

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Generally lgtm, some nitpicks

eth/gasprice/gasprice.go Outdated Show resolved Hide resolved
@@ -32,12 +32,14 @@ import (
const sampleNumber = 3 // Number of transactions sampled in a block

var DefaultMaxPrice = big.NewInt(500 * params.GWei)
var DefaultIgnorePrice = big.NewInt(0 * params.GWei)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you set the default to 2 * params.Wei, then we'd keep the current (just merged) behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I rebased off master and applied this change as well as your other suggestions as part of the updated 4096b9aba790b850ad495e560bb56557aaeae31f commit.

eth/gasprice/gasprice.go Outdated Show resolved Hide resolved
eth/gasprice/gasprice.go Outdated Show resolved Hide resolved
@@ -32,12 +32,14 @@ import (
const sampleNumber = 3 // Number of transactions sampled in a block

var DefaultMaxPrice = big.NewInt(500 * params.GWei)
var DefaultIgnorePrice = big.NewInt(2 * params.GWei)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would bump the current default (2) with nine orders of magnitude. I think it's unintentional?

For sure, 2 might be too low, but let's change these things slowly. I think getting rid of 0 and 1 already will make a big difference. Let's gather some data before bumping it another time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, will fix :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and rebased in 25bd9cd

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ryanschneider
Copy link
Contributor Author

@holiman does this need another ✅ to get merged?

@holiman
Copy link
Contributor

holiman commented May 5, 2021

@holiman does this need another white_check_mark to get merged?

Technically, no. However, since it adds yet another option to the UX, I would like one more ✔️ (or discuss it in a triage) before it gets merged. We don't want to add things only to have to redefine/deprecate them later because we come up with a better way to handle it, or find that we should've named it differently or something.

@holiman holiman added this to the 1.10.4 milestone May 11, 2021
@holiman holiman merged commit ca98080 into ethereum:master May 11, 2021
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
ethereum#22752)

This adds a cmd line parameter `--gpo.ignoreprice`, to make the gas price oracle ignore transactions below the given threshold.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants