PVF: Fix external workers being required for non-collator full nodes #7566
Conversation
mrcnski
added
A0-please_review
| #[allow(missing_docs)] | ||
| #[derive(Debug, Parser)] | ||
| pub struct ValidationWorkerCommand { | ||
| /// The path to the validation host's socket. | ||
| #[arg(long)] | ||
| pub socket_path: String, | ||
| /// Calling node implementation version | ||
| #[arg(long)] | ||
| pub node_impl_version: String, | ||
| } |
There was a problem hiding this comment.
Sorry for unrelated change, this was dead code
| @@ -909,7 +909,7 @@ pub fn new_full<OverseerGenerator: OverseerGen>( | |||
| slot_duration_millis: slot_duration.as_millis() as u64, | |||
| }; | |||
|
|
|||
| let candidate_validation_config = if is_collator.is_collator() { | |||
| let candidate_validation_config = if role.is_authority() && !is_collator.is_collator() { | |||
There was a problem hiding this comment.
doesn't this instantiate a dummy subsystem for candidate validation for every node that's an authority and is not a collator?
There was a problem hiding this comment.
Sorry, did that too fast!
Wanted to keep the conditional as role.is_authority() && !is_collator.is_collator() instead of negating it, since it's the same conditional that's used for pre_checker_enabled above.
There was a problem hiding this comment.
This might as well be if role.is_authority() as there aren't really any nodes that are both authorities and collators. But if there were a node that is an authority and a collator, it'd probably want to validate candidates (in its role as an authority).
There was a problem hiding this comment.
The check above is probably wrong too, based on first principles.
There was a problem hiding this comment.
This doesn't capture the possibility of nodes not being authorities and also checking parablocks, but we can cross that bridge when we get to it. Currently dispute infrastructure doesn't allow it, so kind of a moot point. I do wish these interfaces were a bit more forward thinking...
There was a problem hiding this comment.
The check above is probably wrong too, based on first principles.
I'm kind of scared to touch it here. Changed the current check though.
There was a problem hiding this comment.
@rphmeier The zombienet tests were failing without the !is_collator check so I guess it is needed. (local cumulus tests had worked without it though.) Perhaps this is something I can revisit in the refactor, for now I will merge this in to fix CI.
> This might as well be if role.is_authority() as there aren't really any nodes > that are both authorities and collators. But if there were a node that is an > authority and a collator, it'd probably want to validate candidates (in its > role as an authority).
|
bot merge |
|
Error: Statuses failed for 36264cb |
|
bot merge |
See title and this thread.
Cumulus companion: paritytech/cumulus#2929