Gas estimation: Overestimate based on (actor, method) tuples #8620
Conversation
Signed-off-by: Jakub Sztandera <[email protected]>
Kubuxu
force-pushed
the
feat/upgrade-gas-est
branch
from
2f65389
to
5fb6928
Compare
|
|
||
| if builtin.IsStorageMinerActor(act.Code) { | ||
| switch msgIn.Method { | ||
| case 5: |
There was a problem hiding this comment.
case miner.Methods.SubmitWindowedPoSt? (and so on for others)
There was a problem hiding this comment.
This corresponds to what is reported in collected statistics so it is easier to verify if it stays as numbers, also as this is very temporary code there is little to no harm to having raw numbers.
If you feel strongly about it I can add comments with method names.
There was a problem hiding this comment.
I don't feel strongly about it, no.
if you want me to double check the values against the collected statistics, i can, but i trust you
| if ts.Height() <= build.UpgradeFVM1Height && (build.UpgradeFVM1Height-ts.Height() <= 20) { | ||
| transitionalMulti = 2.0 | ||
|
|
||
| func() { |
There was a problem hiding this comment.
what do we gain by using an anonymous function here?
There was a problem hiding this comment.
It cleans up the error handling.
| @@ -297,6 +297,47 @@ func gasEstimateGasLimit( | |||
|
|
|||
| ret := res.MsgRct.GasUsed | |||
|
|
|||
| transitionalMulti := 1.0 | |||
| // Overestimate gas around the upgrade | |||
| if ts.Height() <= build.UpgradeFVM1Height && (build.UpgradeFVM1Height-ts.Height() <= 20) { | |||
There was a problem hiding this comment.
This gives us 10 minutes of overestimation, there are few reasons I went with 20 epochs.
I would be comfortable with 5 minutes, if we didn’t have tipset system, possible off-by-ones somewhere so on, and I feel like increasing it to 10min has a low cost.
There was a problem hiding this comment.
Seems reasonable -- i don't think there's a "correct" answer here.
Is this PR still going for option 2 like the one before? |
|
It is option 2 with an additional tweak for a more precise estimation based on the 80th percentile of some messages. |
Related Issues
Resolves #8417
Proposed Changes
Have few unique multipleirs apart from the base multipleir