Skip to content
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

feat(dot/parachain): create worker pool for PVF host #4101

Merged
merged 30 commits into from
Sep 19, 2024

Conversation

edwardmack
Copy link
Member

@edwardmack edwardmack commented Jul 23, 2024

Changes

This creates a PVF host which will be called for candidate validation. It enables the use of a worker pool that will contain workers that will preform the candidate validation task.

  • The candidate validation subsystem creates and starts an instance of the PVF Host.
  • The PVF host contains a pool of candidate validation workers (one for each of the validation code instances)
  • The candidate validation subsystem calls the Validate method of the PVF host to validate candidates as needed.
  • The PVF host initializes validation workers when needed

Tests

go test github.com/ChainSafe/gossamer/dot/parachain/candidate-validation

Issues

closes #3934

@edwardmack edwardmack force-pushed the ed/feat/candidate_validation_pvf_host_base branch from 8ead58f to 7e314f3 Compare August 6, 2024 21:25
@edwardmack edwardmack changed the title Draft: create worker pool skeleton for PVF host feat(dot/parachain): create worker pool for PVF host Aug 12, 2024
@edwardmack edwardmack marked this pull request as ready for review August 12, 2024 14:26
@EclesioMeloJunior
Copy link
Member

Hey @edwardmack could you explain how the candidate validation workers are instantiated? what is the logic to start a new worker?

@edwardmack
Copy link
Member Author

Hey @edwardmack could you explain how the candidate validation workers are instantiated? what is the logic to start a new worker?

Hi @EclesioMeloJunior when a call to host.Validate is made the validate function calls poolContainsWorker to determine if there is a worker with the given code hash, if not, then one is created by calling workerPool.newValidationWorker.

dot/parachain/pvf/host.go Outdated Show resolved Hide resolved
dot/parachain/pvf/host.go Outdated Show resolved Hide resolved
dot/parachain/pvf/host.go Outdated Show resolved Hide resolved
dot/parachain/pvf/host.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can put more comments explaining code, that would help us understand the code and design better while reviewing.

dot/parachain/pvf/host.go Outdated Show resolved Hide resolved
dot/parachain/pvf/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/pvf/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/pvf/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/pvf/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/pvf/worker.go Outdated Show resolved Hide resolved
dot/parachain/pvf/host.go Outdated Show resolved Hide resolved
dot/parachain/candidate-validation/candidate_validation.go Outdated Show resolved Hide resolved
dot/parachain/pvf/worker.go Outdated Show resolved Hide resolved
dot/parachain/pvf/worker.go Outdated Show resolved Hide resolved
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work Ed 🚀

dot/parachain/candidate-validation/worker.go Outdated Show resolved Hide resolved
Copy link
Contributor

@axaysagathiya axaysagathiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed a few files and made some comments. I will look into other files later.

dot/parachain/candidate-validation/worker.go Outdated Show resolved Hide resolved
dot/parachain/candidate-validation/worker.go Outdated Show resolved Hide resolved
dot/parachain/candidate-validation/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/candidate-validation/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/candidate-validation/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/candidate-validation/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/candidate-validation/worker_pool.go Outdated Show resolved Hide resolved
dot/parachain/candidate-validation/worker_pool.go Outdated Show resolved Hide resolved
@edwardmack
Copy link
Member Author

@kishansagathiya and @axaysagathiya, I think I've addressed all comments, so they can be resolved, and this is ready for a re-review. If you still find changes that need to be done before approval please let me know.

Copy link
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉
I have some small comments. Approving to avoid one more iteration. So, you can resolve those yourself and merge this PR.

dot/parachain/runtime/instance.go Show resolved Hide resolved
dot/parachain/candidate-validation/worker.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the concurrency related code has been removed from this PR. So, this is basically same as previously in terms of behaviour but the code has been moved around.

If we were to merge this we need to have a separate issue to actually put concurrency in this an make pvf host functional.

I want to get a nod from @timwu20 and @EclesioMeloJunior on whether you are okay to merge this in. I am asking because I am bit confused on this.

@edwardmack
Copy link
Member Author

If we were to merge this we need to have a separate issue to actually put concurrency in this an make pvf host functional.

Issue #4185 has been created to track this issue.

@edwardmack edwardmack merged commit 768b6d5 into feat/parachain Sep 19, 2024
22 checks passed
@edwardmack edwardmack deleted the ed/feat/candidate_validation_pvf_host_base branch September 19, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants