Percentile function to handle out of range percentiles #983
Conversation
dylanjw
changed the title
dylanjw
force-pushed
the
percentile_low_data_points
branch
from
379eb7a to
605d136
Compare
dylanjw
force-pushed
the
percentile_low_data_points
branch
from
605d136 to
ae718ba
Compare
web3/gas_strategies/time_based.py
Outdated
| try: | ||
| price_percentile = percentile(gas_prices, percentile=15) | ||
| except InsufficientData: | ||
| price_percentile = min(gas_prices) |
There was a problem hiding this comment.
My instinct would be max() here: usually would prefer overpaying than getting the transaction stuck. Any reason to prefer min()?
Also, is it definitely impossible for gas_prices to be empty?
There was a problem hiding this comment.
Just because min() is what was being calculated before the percentile function. In a private network with just a few transactions per block, wouldn't using the last block max(prices) potentially send the price on a continual upward trend?
There was a problem hiding this comment.
I suppose, but it could also trend down with min. Also maybe it doesn't matter on a private network? I don't feel strongly about it.
There was a problem hiding this comment.
Couldn't you also include transactions from the last few blocks when there is not enough data available?
There was a problem hiding this comment.
It's already looking across 120 blocks by default, but this is grouped by miner. So a few scenarios could cause a small data set:
- we are near the beginning of the chain, with only a few blocks to sample
- even with many blocks and transactions, maybe this particular miner has only mined a few blocks/transactions
dylanjw
changed the title
dylanjw
force-pushed
the
percentile_low_data_points
branch
from
a2dac87 to
cab6a5b
Compare
|
This is ready for a review. I found and resolved an error when a percentile falls outside the range of numbers. For example, the 15th percentile would break for <7 data points because 1/6 ~ 17%. Now when the percentile choice falls below the lowest value, the lowest value is returned. With this change the percentile function will work with a single data point. |
web3/utils/math.py
Outdated
| if values in [None, tuple(), []] or len(values) < 3: | ||
| raise ValueError("Expected a sequence of at least 3 integers, got {0}".format(values)) | ||
| if values in [None, tuple(), []] or len(values) < 1: | ||
| raise InsufficientData("Expected a sequence of at least 1 integers, got {0}".format(values)) |
There was a problem hiding this comment.
Maybe got {0!r} would help show exactly what weird value was sent.
| (dict(max_wait_seconds=20, sample_size=5, probability=50), 50), | ||
| (dict(max_wait_seconds=20, sample_size=5, probability=98), 100), | ||
| (dict(max_wait_seconds=20, sample_size=5, probability=90), 100), | ||
| (dict(max_wait_seconds=20, sample_size=5, probability=50), 34), |
There was a problem hiding this comment.
I'm not reviewing these numbers, just trusting that you double-checked them. :)
There was a problem hiding this comment.
It might be a good idea to at least once check how accurate the target times are when compared to actual times on ropsten or even mainnet.
There was a problem hiding this comment.
👍
Also, as a quicker test, we could compare against https://ethgasstation.info/.
There was a problem hiding this comment.
Ooh yeah. Then I could compare to main net without spending any gas.
There was a problem hiding this comment.
The estimates are comparable to ethgasstation.info, but I noticed that with longer acceptable wait times, our estimates are a bit lower. I ran some tests by sending a transaction back and forth on on mainnet using our estimate. I tried raising the percentile. Given the results, I am going to bump up the percentile to 20%, since it yielded the best results.
15% percentile
| Expected | Actual |
|---|---|
| <2m | 2m |
| <5m | 10m, 4m |
| <30m | 13m |
20% percentile
| Expected | Actual |
|---|---|
| <2m | 1.5m |
| <5m | 2m, 4m, 3m |
| <30m | 15m |
25%
| Expected | Actual |
|---|---|
| <2m | 2m |
| <5m | 2m |
| <30m | 3m |
40%
| Expected | Actual |
|---|---|
| <30m | 7m |
dylanjw
force-pushed
the
percentile_low_data_points
branch
from
fb9cddd to
9ccc9d1
Compare
dylanjw
force-pushed
the
percentile_low_data_points
branch
from
9ccc9d1 to
ce5f8c8
Compare
What was wrong?
Fixes #979
How was it fixed?
The percentile function requires at least 3 data points to make a calculation. The percentile now throws a custom exception that is caught and
min()is used as an alternative in these cases.Cute Animal Picture