cmd, les, eth, eth/gasprice: using new gas price oracle #13853
Conversation
|
@zsfelfoldi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fjl, @karalabe and @obscuren to be potential reviewers. |
eth/gasprice/gasprice.go
Outdated
| GpobaseCorrectionFactor int | ||
| GpoBlocks int | ||
| GpoPercentile int | ||
| GpoDefault *big.Int |
There was a problem hiding this comment.
There's no need for these fields to have the Gpo prefix. Every field is a configuration parameter of the gas price oracle. No need to remind everywhere.
eth/gasprice/gasprice.go
Outdated
| type blockPriceInfo struct { | ||
| baseGasPrice *big.Int | ||
| } | ||
|
|
||
| type GpoParams struct { |
There was a problem hiding this comment.
Just call this struct Config. It's obvious from its type (gasprice.Config) what it does.
cmd/utils/flags.go
Outdated
| Usage: "Suggested gas price base correction factor (%)", | ||
| Value: 110, | ||
| GpoPercentileFlag = cli.IntFlag{ | ||
| Name: "gpopercent", |
There was a problem hiding this comment.
Please call this gpopercentile. Percent is a different concept.
cmd/utils/flags.go
Outdated
| Value: 110, | ||
| GpoPercentileFlag = cli.IntFlag{ | ||
| Name: "gpopercent", | ||
| Usage: "Percentile of recent transaction gas prices", |
There was a problem hiding this comment.
This doesn't really explain what it does. Please formulate it a bit more meaningfully :P
eth/gasprice/gasprice.go
Outdated
| } | ||
|
|
||
| // GasPriceOracle recommends gas prices based on the content of recent | ||
| // blocks. | ||
| // blocks. Suitable for both light and full clients. | ||
| type GasPriceOracle struct { |
There was a problem hiding this comment.
The full type of this (i.e. what you would see whenever referring to it) is gasprice.GasPriceOracle. There's no need imho for this stuttering. Please only use Oracle for the type name.
eth/gasprice/gasprice.go
Outdated
| chn := make(chan lpResult, self.checkBlocks) | ||
| sent := 0 | ||
| exp := 0 | ||
| var lps bigIntArray |
There was a problem hiding this comment.
Please name this something meaningful, not lps. Also don't use bigIntArray as the type. If you need to sort big ints, it's enough to cast it to the sorting interface exactly at the point where you call a Sort on it. There's no need to litter the sorting interface all over the code as it makes everything harder to understand.
eth/gasprice/gasprice.go
Outdated
| exp-- | ||
| if len(res.prices) > 0 { | ||
| lps = append(lps, res.prices...) | ||
| } else { |
There was a problem hiding this comment.
Instead of the
if len(res.prices) > 0 {
lps = append(lps, res.prices...)
} else {
construct, please use
if len(res.prices) > 0 {
lps = append(lps, res.prices...)
continue
}
It avoids these deep nestings that you just made here.
eth/gasprice/gasprice.go
Outdated
| } else { | ||
| if maxEmpty > 0 { | ||
| maxEmpty-- | ||
| } else { |
There was a problem hiding this comment.
Same here, please use continue instead of nesting yet another layer.
eth/gasprice/gasprice.go
Outdated
| if block != nil { | ||
| self.processBlock(block) | ||
| // SuggestPrice returns the recommended gas price. | ||
| func (self *GasPriceOracle) SuggestPrice(ctx context.Context) (*big.Int, error) { |
There was a problem hiding this comment.
Please replace all instances of self with gpo for example. The keyword self is a very bad pattern in Go code and is advised against.
eth/gasprice/gasprice.go
Outdated
| } | ||
|
|
||
| self.fetchLock.Lock() | ||
| defer self.fetchLock.Unlock() |
There was a problem hiding this comment.
I'm a bit unsure about this pattern here. Fetching the last N blocks may run for a considerable amount of time. The current approach seems to flat out block on it. That's ok for a full client since fetching the blocks should be fast. But how much will a light client block? Perhaps a saner thing would be to try and update the gas price in the background, but return the previous price to the user if X seconds pass?
There was a problem hiding this comment.
Blocks are fetched in parallel, it usually only takes a fraction of a second in light client mode. Also, an old suggestion is not necessarily a good suggestion so I'd rather keep it like this until it proves to be a problem.
eth/gasprice/gasprice.go
Outdated
| lastPrice: params.Default, | ||
| checkBlocks: blocks, | ||
| minBlocks: (blocks 1) / 2, | ||
| maxBlocks: blocks * 5, |
There was a problem hiding this comment.
What's the rational behind these values?
minBlocks: (blocks 1) / 2,
maxBlocks: blocks * 5,
And a bit below why is maxEmpty := gpo.checkBlocks - gpo.minBlocks this particular value?
For example if I tell the oracle to use 10 blocks, why is minBlocks == 5, maxEmpty == 5 and maxBlocks = 50?
Do we use minBlocks for anything apart from calcualting maxEmpty? If not, I'd suggest adding maxEmpty to the struct instead and not storing minBlocks at all.
| GpoPercentileFlag = cli.IntFlag{ | ||
| Name: "gpopercentile", | ||
| Usage: "Suggested gas price is the given percentile of a set of recent transaction gas prices", | ||
| Value: 50, |
There was a problem hiding this comment.
This 50th percentile seems a bit arbitrary to me. Perhaps it would be nice to "scan" the mainnet transactions and see what a sane vaue would be for the default parameters.
This PR improves the newer version of the Gas Price Oracle (formerly used by light client mode only) and removes the old version, now using the same algorithm for both client modes.