Skip to content
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

Do not enforce network key presence for collators #4957

Closed
wants to merge 1 commit into from

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented Jul 5, 2024

Because of

new_base.validator = self.base.validator || self.collator;
#3852 wrongly enforces that the network key is present for collators started with polkadot-parachain binary, instead of generating it on the fly.

That is not necessary and created some small friction for collators, since they have to pass --unsafe-force-node-key-generation or generate the key with the generate-node-key command.

This PR fixes that since the change was intended to apply only for relaychain authorithies.
Triggered by: https://matrix.to/#/!gTDxPnCbETlSLiqWfc:matrix.org/$Y9hLdT0z2TZCtskIcsIrD3kQm3XE5nga4b4LbGWsh-8?via=web3.foundation&via=matrix.org&via=parity.io

Because of https://github.com/paritytech/polkadot-sdk/blob/299aacb56f4f11127b194d12692b00066e91ac92/cumulus/client/cli/src/lib.rs#L318
#3852 wrongly enforces
that the network key is present for collators started with
polkadot-parachain binary, instead of generating it
on the fly.

That is not necessary and created some small friction for
collators, since they have to pass `--unsafe-force-node-key-generation`
or generate the key with the `generate-node-key` command.

This PR fixes that since the change was intended to apply
only for relaychain authorithies.

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh alexggh requested review from bkchr, dmitry-markin and lexnv July 5, 2024 15:45
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I mean it was quite clear from the beginning that this is required for every authority. I also don't really see why collators should not do the same. I mean yes, right now this is not required, but I think at some point we will also use authority discovery for parachains. Generally I see this as some kind of "hygiene". It adds some friction, but it is the same as for relay chain validator operators.

TLDR: I would keep it this way and close this pr.

@alexggh
Copy link
Contributor Author

alexggh commented Jul 8, 2024

I mean it was quite clear from the beginning that this is required for every authority. I also don't really see why collators should not do the same. I mean yes, right now this is not required, but I think at some point we will also use authority discovery for parachains. Generally I see this as some kind of "hygiene". It adds some friction, but it is the same as for relay chain validator operators.

Indeed, this is a valid point I did not know we planned to use authority-discovery for parachains as well, strictly speaking we don't need to enforce this for parachains at this moment, but if we envision that will happen in the future I agree with you that this PR does not make much sense.

TLDR: I would keep it this way and close this pr.

Given, I have just one complain datapoint for this since v1.11 and all the above I tend to agree, so closing the PR.

@alexggh alexggh closed this Jul 8, 2024
@bkchr bkchr deleted the alexaggh/fix_enforcement branch July 8, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants