Remove superflous parameter overseer_enable_anyways and make parachain node type more explicit
#7617
Conversation
We don't need this flag, as we don't need the overseer enabled when the node isn't a collator or validator.
bkchr
added
A2-insubstantial
|
What about pov-recovery for parachain full nodes? AFAIR that was the reason for this flag. |
`IsParachainNode` is more expressive and also encapsulates the state of the parachain node being a full node. Some functionality like the overseer needs to run always when the node runs alongside a parachain node. The parachain node needs the overseer to e.g. recover PoVs. Other things like candidate validation or pvf checking are only required for when the node is running as validator.
Ahh yes, good point! Reworked the pr. |
bkchr
changed the title
overseer_enable_anywaysoverseer_enable_anyways and make parachain node type more explicit
|
@bkchr zombienet tests are failing with this error, looks like an issue in the collator image. Logs: I will take a look. Thx! |
|
I think this is because the undying collator is started with |
|
@pepoviola fixed the CI failure. It is related to how zombienet spawns the node, as @skunert explains above. I "fixed" this for now. The proper solution would be that zombienet is not using "default cli args", but that is something for another day :P |
Still slightly confused by this, |
| // Zombienet is spawning all collators currently with the same CLI, this means it | ||
| // sets `--validator` and this is wrong here. | ||
| config.role = Role::Full; |
There was a problem hiding this comment.
Should we have a followup to fix this on the zombienet side?
There was a problem hiding this comment.
Fixed here, I will create a new pr to remove this workaround.
|
bot merge |
* Companion for Polkadot#7617 paritytech/polkadot#7617 * Adapt to latest Polkadot changes. * update lockfile for {"polkadot", "substrate"} --------- Co-authored-by: parity-processbot <>
|
Thanks @bkchr / @skunert / @mrcnski, I created this followup issue to fix this in zombienet. |
…ain node type more explicit (#7617) * Remove superflous parameter `overseer_enable_anyways` We don't need this flag, as we don't need the overseer enabled when the node isn't a collator or validator. * Rename `IsCollator` to `IsParachainNode` `IsParachainNode` is more expressive and also encapsulates the state of the parachain node being a full node. Some functionality like the overseer needs to run always when the node runs alongside a parachain node. The parachain node needs the overseer to e.g. recover PoVs. Other things like candidate validation or pvf checking are only required for when the node is running as validator. * FMT * Fix CI
|
This shouldn't have been merged IMO Subsystems shouldn't be disabling themselves internally! Just don't add the subsystem if you don't need it. And we're baking in all of these half-formed assumptions about what a parachain node is or what a collator is. Or even what a full node of the relay chain is. These assumptions will slow us down later. The service instantiation is already a petrified lump. If it could speak, it'd say "kill me." There is an |
This pull request removes the superfluous parameter
overseer_enable_anywaysand renamesIsCollatortoIsParachainNode. The parameteroverseer_enable_anywayswas actually used to express that a relay chain node is running alongside a parachain full node. This lead to things like enabling of the candidate validation system running, which is only required for relay chain validators. This pull requests makes this more explicit by introducingIsParachainNode::FullNodeand ensuring that we don't enable subsystems that we don't need for parachain nodes.Cumulus companion: paritytech/cumulus#3008