-
Notifications
You must be signed in to change notification settings - Fork 767
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
Move PVF code and PoV decompression to PVF host workers #5142
Conversation
@sandreim Before this one gets even fatter, I have to ask. I can make it simpler by moving some metrics (namely, PVF size and compressed/uncompressed PoV size) from candidate validation subsys to the PVF host. This will most probably break some Grafana dashboards, and I'm not sure what else. Is it worth the code simplification? |
Yes, moving the code dealing with the blobs to pvf host simplifies things in candidate validation. We could keep the old metric names so nothing is broken. |
The CI pipeline was cancelled due to failure one of the required jobs. |
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 feel that there is an issue with passing the same data through so many functions. However, this concern is not related to the current PR, as it simply follows the chosen approach.
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.
Nice work @s0me0ne-unkn0wn !
- name: polkadot-overseer | ||
bump: patch | ||
- name: polkadot-node-core-pvf | ||
bump: major |
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.
Yeah, we could have done better to just require a minor bump if we added a new API that takes in the uncompressed blob along with some types for compressed and uncompressed blobs. But since these crates are just used internally by the polkadot node I think it doesn't worth investing in.
) Closes paritytech#5071 This PR aims to * Move all the blocking decompression from the candidate validation subsystem to the PVF host workers; * Run the candidate validation subsystem on the non-blocking pool again. Upsides: no blocking operations in the subsystem's main loop. PVF throughput is not limited by the ability of the subsystem to decompress a lot of stuff. Correctness and homogeneity improve, as the artifact used to be identified by the hash of decompressed code, and now they are identified by the hash of compressed code, which coincides with the on-chain `ValidationCodeHash`. Downsides: the PVF code decompression is now accounted for in the PVF preparation timeout (be it pre-checking or actual preparation). Taking into account that the decompression duration is on the order of milliseconds, and the preparation timeout is on the order of seconds, I believe it is negligible.
Closes #5071
This PR aims to
Upsides: no blocking operations in the subsystem's main loop. PVF throughput is not limited by the ability of the subsystem to decompress a lot of stuff. Correctness and homogeneity improve, as the artifact used to be identified by the hash of decompressed code, and now they are identified by the hash of compressed code, which coincides with the on-chain
ValidationCodeHash
.Downsides: the PVF code decompression is now accounted for in the PVF preparation timeout (be it pre-checking or actual preparation). Taking into account that the decompression duration is on the order of milliseconds, and the preparation timeout is on the order of seconds, I believe it is negligible.