-
Notifications
You must be signed in to change notification settings - Fork 140
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
Fix gas accounting for seal proof verification #1852
Comments
Couple of things to note:
|
There are two things in the current price, these is empirical discount from parallelism, on the other hand there were some improvements to proof verification performance since then. There is also the debate whether the parallelism should be discounted because unoccupied threads are useful for processing alternative tipsets. |
I've benchmarked proof verification calls using code in #1882. I propose doing away with any parallelism discounts, and simply charging for the cost of verifying the proofs. I propose we continue to verify them in parallel (assuming multiple proofs), but charging just based on the number of proofs. Here are the results in nanoseconds:
These are obtained after stripping out all failures (bad proofs), and stripping out the top and bottom 2% of results. Neither of those operations changed the result significantly. I propose using the 60th percentile value of the 32 GiB sector for all sector sizes, which would yield a cost of 41.78M gas per proof verification. This is only slightly more than the current cost of 34.72M gas per proof. |
This seems like a reasonable approach. If it becomes an issue, we can likely change it later. The core issue here is that requiring additional cores has an opportunity cost. |
Yup. I'm also generally thinking that it'll be easier to lower the cost than increase it moving forward. |
I can't verify the numbers, however, I agree with the choices towards no-parallization assumption approach. |
Were there any high-side outliers in case of bad proofs?
I agree with the approach as there is cost to parallelism that is hard to estimate and capture. |
No, there were only three bad proofs, and they were all 43M gas. |
Final proposal: 42M gas for both 32 GiB and 64 GiB sectors. |
Upcoming changes to built-in actors to implement direct data onboarding will invoke the
batch_verify_seals
syscall directly from the miner actor. In the past, this syscall has been invoked only from the power actor's cron handler. It does not charge the correct amount of gas, though there's no apparent reason why it can't. This syscall should instead charge the correct amount of gas. Doing so won't have any effect on the existing use since the cron originator has unlimited gas budget.Fixing this is necessary for the deployment of direct data onboarding. I don't think it needs a FIP since the end result is just that the syscall works as expected, with no change to existing on-chain observable behaviour. Any necessary things can be specified in the direct onboarding FIP.
verify_seal_base
from 2000 to a real number for per-proof verification.Changing the
verify_seal_base
price to34721049
the amount hardcoded in the power actor, would then price this the same as the current cost of seal proof validation. However this price might (?) include an empirical discount that assumes some unknown number of additional proofs are submitted in the same epoch which can be verified in parallel. But a fixed price per item also obviously doesn't include a discount for the known number of proofs that actually are submitted when this is called directly from the miner.FYI @ZenGround0 @Kubuxu
The text was updated successfully, but these errors were encountered: