eth/gasprice: set default percentile to 60%, count blocks instead of transactions #15828
Conversation
|
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Arachnid
changed the title
|
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Arachnid
changed the title
eth/gasprice/gasprice.go
Outdated
| continue | ||
| } | ||
| if minPrice == nil || tx.GasPrice().Cmp(minPrice) < 0 { | ||
| minPrice = tx.GasPrice() |
There was a problem hiding this comment.
tx.GasPrice creates a copy internally. Calling it twice here will alloc twice. It would be cleaner to do:
if curPrice := tx.GasPrice(); minPrice == nil || curPrice.Cmp(minPrice) < 0 {
minPrice = curPrice
}
eth/gasprice/gasprice.go
Outdated
| prices[i] = tx.GasPrice() | ||
| var minPrice *big.Int | ||
| for _, tx := range txs { | ||
| sender, err := types.Sender(signer, tx) |
There was a problem hiding this comment.
I'm a bit reluctant about this change here as is. This essentially does an ecrecover. So for a single gas estimation, on mainnet we would iterate over 20 blocks x 200-300 tx, that's 4-6K ecrecovers. That would imho be a very expensive operation to do, borderline unusable on a mobile phone (maybe fully).
There was a problem hiding this comment.
Is there a way we could cache the senders of transactions so we don't need to ecrecover all the time?
There was a problem hiding this comment.
In here, or in general? We could modify this to not reprocess existing blocks, if they've been seen already by a previous call to the oracle.
There was a problem hiding this comment.
Unless I'm very mistaken, tx sender caching helps here. The GPO loads recent blocks, so they'll be in the BlockChain LRU. The txs in those blocks have cached sender information.
fjl
changed the title
eth/gasprice/gasprice.go
Outdated
| txs := block.Transactions() | ||
| var minPrice *big.Int | ||
| sort.Sort(transactionsByGasPrice(txs)) |
There was a problem hiding this comment.
block.Transactions() returns the internal slice (https://github.com/ethereum/go-ethereum/blob/master/core/types/block.go#L287), which means sorting it will have nasty side effects.
…transactions (ethereum#15828) The first should address a long term issue where we recommend a gas price that is greater than that required for 50% of transactions in recent blocks, which can lead to gas price inflation as people take this figure and add a margin to it, resulting in a positive feedback loop.
The first should address a long term issue where we recommend a gas price that is greater than that required for 50% of transactions in recent blocks, which can lead to gas price inflation as people take this figure and add a margin to it, resulting in a positive feedback loop.
The latter is a more experimental change following ethgasstation's suggestion: https://twitter.com/ETHGasStation/status/949264877788585984
The insight being that each miner can set their own inclusion policy; rather than looking at a percentile of recent transactions, we should instead look at a percentile of recent blocks; a gas price that was sufficient to be included in n% of recent blocks should be sufficient to get our TX included quickly.
This will need some experimentation and trialling on nodes to see how well it fares before public release.
Still todo: