-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Block gas limit calibration #1273
Conversation
7117efc
to
a11a780
Compare
@@ -596,7 +596,108 @@ fn slope(x_y: Vec<(u64, u64)>) -> f64 { | |||
.map(|(x, y)| (*x as f64 - avg_x) * (*y as f64 - avg_y)) | |||
.sum(); | |||
let sq_x: f64 = x_y.iter().map(|(x, _)| (*x as f64 - avg_x).powi(2)).sum(); | |||
sum_x_y / sq_x | |||
sq_x / sum_x_y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct for the least squares method of linear regression, it should be sum_x_y / sq_x
:
https://en.wikipedia.org/wiki/Simple_linear_regression#Fitting_the_regression_line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but we need value for y
, not x
. And it is the same behavior as before(after slope
we divided 1/slope(x_y)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment=)
// y = B * x | ||
// | | ||
// \|/ | ||
// 1 / B = x / y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] could add a transformation to both sides of the equation like (rhs)^-1 = (lhs)^-1
let last = x_y.last().unwrap(); | ||
|
||
let near_linear = linear_regression * NEAR_LINEAR; | ||
let expected_type = if (linear_regression - first.amount()).abs() < near_linear |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more sound approach for testing the goodness-of-fit using linear regression would be the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use it here, but we don't have cases of linear behavior, so the super-complex formula is overkill. Because each opcode includes the base execution time, it looks like a logarithmic almost in all cases.
Plus, we still need to play with the threshold for R
. The code for the logarithm behavior should cover almost all cases, and the min
strategy means we can only overcharge user. If later, with benchmarking target block time, we will see that some opcodes are really overcharged, then we can try to improve the algorithm. But right now, it looks like our benchmarks produce undercharge costs.
} | ||
}) | ||
.map(|p| p.amount()) | ||
.min_by(|a, b| a.partial_cmp(b).unwrap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this potentially leave a range of inputs somewhere between the base and the min(p.amount()) where we undercharge for execution time? would it be safer to just take the slope off the base, even if it overcharges?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also nit - isn't this a more complicated expression of simply using .min()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Floats don't support min
and you need to use partial_cmp
=(
Remember, the lower value of amount
means we charge the user more because of args / dep_per_unit
. So we will only overcharge user, not undercharge.
base = *p; | ||
true | ||
} else { | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return some kind of error if we never find a base using the slope comparison? Otherwise it will just default to the first item which could be a very small base fee and be very inadequate for charging users.
For example if no base is found and the first point is x=10, and then we use the lowest amount from the high end of the points like x = 1,000,000 this could severely undercharge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be possible. Our function is linear if the initial value is around the predicted value. So the initial price is already fair enough in this case.
block_target_gas
, that runs different simple scripts with opcode and measures the block execution time. It allows us to track how accurate is our gas cost. If each benchmark takes ~ the same amount of time, then our gas cost is accurate. The current costs is not accurate because thenoop
takes ~1s for 100M gas, whilemeq
- ~6s,logd
- ~23s... Later we can have more opcodes tested and the end goal is to optimize opcodes/calibrate benchmarks to have the same execution time.base
cost, we can make the cost per element much cheaper. So, the initial benchmarks with10
,1000
, and10000
elements can be included in thebase
cost to decrease the cost for each new element. The predicted value from linear regression is used to find this point.