This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Split the Roles in three types #5492
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tomaka
requested review from
andresilva,
Demi-Marie,
mxinden and
tomusdrw
as code owners
cecton
reviewed
Apr 1, 2020
cecton
approved these changes
Apr 2, 2020
mxinden
reviewed
Apr 2, 2020
mxinden
reviewed
Apr 2, 2020
…o compile on my machine
andresilva
suggested changes
Apr 2, 2020
andresilva
reviewed
Apr 2, 2020
andresilva
reviewed
Apr 2, 2020
andresilva
reviewed
Apr 2, 2020
|
I think the script doesn't like the fact that it's on my fork repo. I opened #5520 instead. |
cecton
reviewed
Apr 4, 2020
| @@ -117,13 +117,7 @@ impl NetworkConfigurationParams { | |||
| config.network.non_reserved_mode = NonReservedPeerMode::Deny; | |||
| } | |||
|
|
|||
| config.network.sentry_nodes.extend(self.sentry_nodes.clone()); | |||
There was a problem hiding this comment.
@tomaka is it intended that you do not set NetworkConfiguration.sentry_nodes anymore??
There was a problem hiding this comment.
Oh I see, you removed sentry_nodes from NetworkConfiguration. But then why is it still in the cli??
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah I figured out. I think sentry_nodes should have been moved to runcmd. NetworkConfigurationParams is only supposed to populate the NetworkConfiguration. I've done it while fixing merge conflicts in #5271
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR splits the
Rolesbitfield in three:The existing
Roleshas been moved untouched into thesc_network::protocol::messagemodule, where it continues to represent the data that is transferred on the wire.sc_network::confignow contains a newRole(note the lack of 's') enum whose variants areFull,Light,SentryandAuthority. It is more robust to misuse, as one can only pass a list of sentry nodes when usingAuthority, and a valiator when usingSentry.The gossiping API now uses
ObservedRole, which is an enum that can be one ofFull,Light,OurSentry(for when you're a validator connecting to your sentry),OurGuardedValidator(for when you're a sentry connecting to your validator), orAuthority(for third-party validators).GrandPa has been adjusted to always gossip messages to nodes whose role is
OurSentryorOurGuardedValidator. This fixes some issues with validators not receiving GrandPa messages from their sentries. This change is the original reason for this PR.Additionally, the
--sentryCLI option now accepts a multiaddress as parameter, representing the address of the validator we're protecting. This is a CLI breaking change.While modifying the CLI code, I encountered some issues with how to properly build a
Role::Authoritywhen the parameter of--sentryis aString.To fix that, I changed various
Stringsin the structopt structs insc_clito instead contain aMultiaddrorMultiaddrWithPeerId(a new type introduced in this PR).Some other minor changes:
client/rpc/api/system::NodeRolenow contains aSentryvariant. In other words, querying the node role from the RPC endpoint will now return "sentry" for sentries.authority_discoverynow accepts aVec<MultiaddrWithPeerId>instead of aVec<String>in accordance with the changes in the CLI.node_rolePrometheus metric will now contain the value3for sentries, rather than4(4 meaning "authority").polkadot companion: paritytech/polkadot#960