Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Candidate Validation Subsystem #1432

Merged
31 commits merged into from
Jul 30, 2020
Merged

Candidate Validation Subsystem #1432

31 commits merged into from
Jul 30, 2020

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Jul 17, 2020 •
edited
Loading

Closes #1241

This is a very basic implementation; it doesn't do jobs or results-caching or anything like that. It simply wraps a pool of processes used for candidate validation (a utility we had already implemented for v0) and forwards all validation requests to it.

Still TODO:

  • Fetching necessary data from chain state
  • Fixing fallout in CandidateBacking
  • Tests

@rphmeier rphmeier added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 17, 2020
@rphmeier
Copy link
Contributor Author

rphmeier commented Jul 17, 2020 •
edited
Loading

Also, I think we might need something like tokio::blocking. These candidate validation tasks are waiting on a response or time-out from a subprocess for a long time, but I'm not sure how we can do that without introducing a hard dependency on tokio.

Alternatively, there's no explicit reason to not just wrap a rayon or CpuPool threadpool or similar directly in this subsystem. That might be a better fit than using the SubsystemContext::spawn method for backgrounding validation work.

@rphmeier rphmeier mentioned this pull request Jul 21, 2020
Merged
@rphmeier rphmeier marked this pull request as ready for review July 27, 2020 20:17
@rphmeier rphmeier added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 27, 2020
@bkchr
Copy link
Member

bkchr commented Jul 27, 2020

The SpawnNamed trait provides you with a function for spawning blocking tasks. (I did not looked at the pr on how you solved this now)

@rphmeier
Copy link
Contributor Author

rphmeier commented Jul 28, 2020 •
edited
Loading

@bkchr OK, we need to add that to the SubsystemContext, which we use to send tasks to the overseer to be spawned. I will file an issue for both.

@rphmeier rphmeier requested a review from drahnr July 30, 2020 01:21
@rphmeier
Copy link
Contributor Author

Final sign-off?

drahnr
drahnr approved these changes Jul 30, 2020
View reviewed changes
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A few smaller nitpicks, but looks very good overall

@rphmeier
Copy link
Contributor Author

Thanks for the review @drahnr

@rphmeier
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Jul 30, 2020

Waiting for commit status.

@ghost
Copy link

ghost commented Jul 30, 2020

Checks failed; merge aborted.

@rphmeier
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Jul 30, 2020

Trying merge.

@ghost ghost merged commit cdb5c40 into master Jul 30, 2020
@ghost ghost deleted the rh-candidate-validation branch July 30, 2020 21:50
ordian added a commit that referenced this pull request Jul 31, 2020
@ordian
Merge branch 'master' into ao-chain-api-subsystem
a5b0913 
* master:
  Candidate Validation Subsystem (#1432)
  reduce slash defer durations (#1504)
  Implement the Runtime API subsystem (#1494)
  Companion for #6610 (Balances Weight Trait) (#1425)
  [CI] Publish draft release redux (#1493)
  Update README docs related to local build (#1479)
  Add a default trie-memory-tracker feature to the cli (#1502)
  Companion PR for `Add a `DefaultQueue` type alias to remove the need to use `sp_api::TransactionFor`` (#1499)
  Fix bitfield signing (#1466)
  Update Substrate, bump versions, clean up sort (#1496)
  Bump Substrate
@rphmeier rphmeier mentioned this pull request Sep 26, 2022
Merged
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@drahnr drahnr drahnr approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement: Candidate Validation
3 participants
@rphmeier @bkchr @drahnr