-
Notifications
You must be signed in to change notification settings - Fork 1.6k
gossip: do not try to connect if we are not validators #2786
Changes from all commits
60d7578
2c21070
c139fbe
73534e4
f78992a
0b5025b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,11 @@ authors = ["Parity Technologies <[email protected]>"] | |
edition = "2018" | ||
|
||
[dependencies] | ||
sp-api = { git = "https://github.com/paritytech/substrate", branch = "master" } | ||
sp-application-crypto = { git = "https://github.com/paritytech/substrate", branch = "master" } | ||
sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" } | ||
sp-authority-discovery = { git = "https://github.com/paritytech/substrate", branch = "master" } | ||
|
||
polkadot-node-network-protocol = { path = "../protocol" } | ||
polkadot-node-subsystem = { path = "../../subsystem" } | ||
polkadot-node-subsystem-util = { path = "../../subsystem-util" } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,9 +57,12 @@ impl PeerSet { | |
notifications_protocol: protocol, | ||
max_notification_size, | ||
set_config: sc_network::config::SetConfig { | ||
// we want our gossip subset to always include reserved peers | ||
in_peers: super::MIN_GOSSIP_PEERS as u32 / 2, | ||
out_peers: 0, | ||
// we allow full nodes to connect to validators for gossip | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be worthwhile to extend this comment to also note that block authors will also not be considered parachain validators in the case that the maximum number of validators participating in parachain validation is limited. Although...due to https://github.com/paritytech/polkadot/blob/master/runtime/parachains/src/runtime_api_impl/v1.rs#L243 we currently include all of the next authority IDs (even beyond There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's exactly the motivation for using authorities api instead of session_info There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind adding some of that info to the comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, do we actually want to add all validators to the validation reserved peerset or just the ones participating in the parachain validation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ordian We had this discussion earlier today, and I believe the outcome was:
Technically we don't need to include block authors as they should be reachable by We don't want to set it to reserved-only as full nodes should still have access to the information gossiped There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can not rely on that, peer sets can easily get filled either maliciously or by accident. We also don't punish inactive peers, so that's a really cheap attack. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copying my DM from Element: here's my understanding of what we want
We could make e.g. statement distribution and disputes use a separate peerset each and set reserved validation to only parachain validators. By using AuthorityDiscoveryApi::authorities we will set reserved peerset to a superset of that, but as you mentioned that's mostly two validator groups, also this will ensure we're potentially connected to validators in the next session in advance, which is nice |
||
// to ensure any `MIN_GOSSIP_PEERS` always include reserved peers | ||
// we limit the amount of non-reserved slots to be less | ||
// than `MIN_GOSSIP_PEERS` in total | ||
in_peers: super::MIN_GOSSIP_PEERS as u32 / 2 - 1, | ||
out_peers: super::MIN_GOSSIP_PEERS as u32 / 2 - 1, | ||
reserved_nodes: Vec::new(), | ||
non_reserved_mode: sc_network::config::NonReservedPeerMode::Accept, | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# Gossip Support | ||
|
||
The Gossip Support Subsystem is responsible for keeping track of session changes | ||
and issuing a connection request to all validators in the next, current and a few past sessions | ||
if we are a validator in these sessions. | ||
The request will add all validators to a reserved PeerSet, meaning we will not reject a connection request | ||
from any validator in that set. | ||
|
||
Gossiping subsystems will be notified when a new peer connects or disconnects by network bridge. | ||
It is their responsibility to limit the amount of outgoing gossip messages. | ||
At the moment we enforce a cap of `max(sqrt(peers.len()), 25)` message recipients at a time in each gossiping subsystem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, so block authors pass this check too. Seems fine to me