-
Notifications
You must be signed in to change notification settings - Fork 112
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
feat(dot/parachain): create worker pool for PVF host #4101
Conversation
8ead58f
to
7e314f3
Compare
Hey @edwardmack could you explain how the |
Hi @EclesioMeloJunior when a call to host.Validate is made the validate function calls |
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.
You can put more comments explaining code, that would help us understand the code and design better while reviewing.
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.
LGTM! Great work Ed 🚀
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 have reviewed a few files and made some comments. I will look into other files later.
@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. |
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 have some small comments. Approving to avoid one more iteration. So, you can resolve those yourself and merge this PR.
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.
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.
Issue #4185 has been created to track this issue. |
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.
Tests
go test github.com/ChainSafe/gossamer/dot/parachain/candidate-validation
Issues
closes #3934