eth/gasprice: improve stability of estimated price #22722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GregTheGreek
requested review from
holiman,
karalabe and
rjl493456442
as code owners
|
Commit e617d50 fixes a failing test due to the new calculations but I'm concerned that it doesn't make sense for it to equal 1 GWEI based on the deterministic gas values values. I'll look into it further tomorrow. |
eth/gasprice/gasprice_test.go
Outdated
|
|
||
| res := removeOutliers(cpy) | ||
| // It should remove most of the higher values | ||
| if len(cpy) < len(res) { |
There was a problem hiding this comment.
Isn't len(cpy) < len(res) always false?
eth/gasprice/gasprice_test.go
Outdated
| cpy = append(cpy, big.NewInt(2)) | ||
| res := removeOutliers(cpy) | ||
| // It should remove most of the higher values | ||
| if len(cpy) < len(res) { |
There was a problem hiding this comment.
Isn't len(cpy) < len(res) always false?
There was a problem hiding this comment.
answering for both comments.
In theory yes, assuming the function operates correctly, I could switch to deterministic values instead ?
There was a problem hiding this comment.
I mean even the outliers are not filtered out, the len(res) should be 0. And len(cpy) < len(res) can't be true in any case.
There was a problem hiding this comment.
All of these tests should use len(gasPrices) != len(res) for the error check. Just like TestRemoveLowOutliers
|
I am wondering should we firstly filter out all 0 gasprice transactions? In theory because of the MEV a bunch of zero-price transactions can be included. Why not just filter out them first? |
I agree. If a block comes in which contains only MEV transactions + the miners own gastoken-minting + the miners own pool payouts, it can be basically all |
fjl
changed the title
|
I've filtered out the 0 gas transactions, still trying to think of a better way test case for simple unit test |
|
Not sure how to handle the AppVeyor failure, seems like some file system permission issues? |
junderw
reviewed
Apr 26, 2021
eth/gasprice/gasprice.go
Outdated
| sum.Add(sum, price) | ||
| } | ||
| mean = big.NewInt(0).Div(sum, length) | ||
| // Calculate variance (sum(x - mean)^2 )/ length |
There was a problem hiding this comment.
// Calculate variance (sum((x - mean)^2)/ length) ?
There was a problem hiding this comment.
Woops this is old.
Could someone try running this on a large chunk of previous blocks and plotting it out against the current (pre-PR) oracle? I'd be interested to see if this actually decreases the occurrence of sudden dips and rises. |
|
I did a little experiment. I exported the real blocks from the issue linked above, using this js: function exportPrices(start, end){
var result = {}
for (var num = start; num <= end; num++){
var block = eth.getBlock(num)
var blockprices = []
block.transactions.forEach(function(hash){
var tx = eth.getTransaction(hash)
if (tx.from != block.miner){
blockprices.push(tx.gasPrice)
}
})
if (blockprices.length > 0) { result[num] = blockprices }
}
console.log(JSON.stringify(result))
}
exportPrices(12290300, 12290900) // mainnetThen I printed out the prices, sorting them and showing the smallest three (because Doing so, two things become obvioius: it's the |
holiman
reviewed
Apr 26, 2021
|
If anyone wants to play with the dataset: https://gist.github.com/holiman/ffa1c7ba3ade8e73ffea5e832ae9cd72 |
|
I think my preferred fix would simply be diff --git a/eth/gasprice/gasprice.go b/eth/gasprice/gasprice.go
index 5d8be08e0b..560722bec0 100644
--- a/eth/gasprice/gasprice.go
+++ b/eth/gasprice/gasprice.go
@@ -199,6 +199,9 @@ func (gpo *Oracle) getBlockPrices(ctx context.Context, signer types.Signer, bloc
var prices []*big.Int
for _, tx := range txs {
+ if tx.GasPriceIntCmp(common.Big1) <= 0 {
+ continue
+ }
sender, err := types.Sender(signer, tx)
if err == nil && sender != block.Coinbase() {
prices = append(prices, tx.GasPrice()) |
|
I'm pulling some of my own data right now and will get back to you @holiman I agree though, if 0,1 are the only things to filter out then I'm fine simplifying this |
19 tasks
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses #22715
Yesterday some of our software started estimating really low gasPrices which was presumably associated to miners accepting tx's with a lower gasPrice (mev?).
This PR introduces a few changes to the gas price oracle (gpo) package.
Note:
I chose the 3rd deviation rather arbitrarly, with the goal to capture almost everything except the really far out ones. I chose a higher block sample which mimics what OpenEthereum does (I know), because our software that was using OE successfully estimated an accurate gasPrice.