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

Companion for Polkadot#7337 #2929

Merged
merged 10 commits into from
Aug 1, 2023

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Jul 24, 2023

@mrcnski mrcnski added A0-please_review Pull request needs code review. A4-companion A PR that should be considered alongside another (usually more comprehensive and detailed) PR. B1-note_worthy Changes should be noted in the release notes C5-high PR touches the given topic and has a high impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 24, 2023
Comment on lines 290 to 292
workers_path: None,
workers_names: None,
dont_use_external_workers: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we may need to rig this crate to build the workers for this to work. Can someone please explain what this crate does exactly, does it call PVF preparation/execution?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think Cumulus spawns PVF workers. There's some discussion here: paritytech/polkadot#6497

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a collator the runtime is trusted, for a validator the runtime is not.

Okay yeah, that makes perfect sense. So we don't need most of what I added here, will remove.

@mrcnski mrcnski added the T0-node This PR/Issue is related to the topic “node”. label Jul 24, 2023
@command-bot
Copy link

command-bot bot commented Jul 24, 2023

@paritytech-cicd-pr Requester could not be detected as a member of an allowed organization.

@dmitry-markin
Copy link
Contributor

dmitry-markin commented Jul 25, 2023

I'd rather somebody from the main PR reviewers with understanding of underlying changes reviewed and approved this instead of me. @koute could you have a look?

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for cf97d9f

@mrcnski
Copy link
Contributor Author

mrcnski commented Jul 31, 2023

polkadot-parachain tests are failing now with the polkadot PR merged. The polkadot-parachain node expects worker binaries even though those should be ignored for collators. So I guess this node does not run in collator mode? I think it's supposed to, can I update it or is there a reason it doesn't?

[2023-07-31 13:58:57] 2023-07-31 13:58:51 Is collating: no    

Advice would be appreciated, cumulus CI for other PRs will be broken until this is fixed.

@skunert
Copy link
Contributor

skunert commented Jul 31, 2023

https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3298541

We instantiate the polkadot node here:

let (is_collator, maybe_collator_key) = if parachain_config.role.is_authority() {
let collator_key = CollatorPair::generate().0;
(polkadot_service::IsCollator::Yes(collator_key.clone()), Some(collator_key))
} else {
(polkadot_service::IsCollator::No, None)
};
let relay_chain_full_node = polkadot_service::build_full(
config,
is_collator,
None,
// Disable BEEFY. It should not be required by the internal relay chain node.
false,
None,
telemetry_worker_handle,
true,
polkadot_service::RealOverseerGen,
None,
None,
hwbench,
)?;

It is only started as collator, if the node is run as a collator. If it is just a parachain full node, we pass polkadot_service::IsCollator::No.

@skunert
Copy link
Contributor

skunert commented Jul 31, 2023

While thinking about it, why does a polkadot full-node require the workers, even if we pass polkadot_service::IsCollator::No? We never spawn a validator here

@mrcnski
Copy link
Contributor Author

mrcnski commented Jul 31, 2023

Agreed @skunert. Perhaps we should add a separate parameter is_validator and require the workers based on that instead.

@skunert
Copy link
Contributor

skunert commented Jul 31, 2023

Agreed @skunert. Perhaps we should add a separate parameter is_validator and require the workers based on that instead.

Hmm maybe I am missing something, but that should not be necessary. Via config.role.is_authority() and the collator parameter you should be able to figure out if the node is a validator right? Similar to these checks?
https://github.com/paritytech/polkadot/blob/5303d8c2099451ad05fe7a303d8b367a0a9be0c3/node/service/src/lib.rs#L771-L772
Edit: I see that is what you did in the fix PR 👍

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit b6ff026 into master Aug 1, 2023
3 checks passed
@paritytech-processbot paritytech-processbot bot deleted the mrcnski/pvf-split-out-worker-binaries branch August 1, 2023 08:32
@mrcnski mrcnski self-assigned this Aug 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A4-companion A PR that should be considered alongside another (usually more comprehensive and detailed) PR. B1-note_worthy Changes should be noted in the release notes C5-high PR touches the given topic and has a high impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

7 participants