gossip: do not try to connect if we are not validators #2786

ordian · 2021-03-31T14:42:21Z

rphmeier · 2021-03-31T15:36:56Z

To confirm - regular nodes not in the validator set will still connect to other nodes via discovery (in_peers)? Otherwise, we should add a few out_peers to ensure that the gossip graph reaches all full nodes interested in the validation protocol.

ordian · 2021-03-31T15:53:25Z

To confirm - regular nodes not in the validator set will still connect to other nodes via discovery (in_peers)? Otherwise, we should add a few out_peers to ensure that the gossip graph reaches all full nodes interested in the validation protocol.

We have only two places of issuing connection requests for the validation peerset, it's not happening via discovery:

gossip support -- to all validators
availability distribution -- to our group

Full nodes won't issue connection requests via availability distribution, since we try to find our validator index first. And after this PR is merge they won't issue it via gossip support either.

Not sure whether we need to bump out_peers though.

ordian · 2021-03-31T16:02:24Z

To confirm - regular nodes not in the validator set will still connect to other nodes via discovery (in_peers)? Otherwise, we should add a few out_peers to ensure that the gossip graph reaches all full nodes interested in the validation protocol.

We have only two places of issuing connection requests for the validation peerset, it's not happening via discovery:
* gossip support -- to all validators

* availability distribution -- to our group
Full nodes won't issue connection requests via availability distribution, since we try to find our validator index first. And after this PR is merge they won't issue it via gossip support either.

Not sure whether we need to bump out_peers though.

Actually I realized we don't need to issue requests in availability distribution anymore.

primitives/src/v1.rs

rphmeier · 2021-03-31T21:13:01Z

node/network/protocol/src/peer_set.rs

-					// 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


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 max_validators) which are most likely the same as current-session.

that's exactly the motivation for using authorities api instead of session_info

Do you mind adding some of that info to the comment?

Hmm, do we actually want to add all validators to the validation reserved peerset or just the ones participating in the parachain validation? ~~If we add all, then we can set it to reserved only, no?~~ @eskimor

@ordian We had this discussion earlier today, and I believe the outcome was:

Connect to parachain validators of the last N sessions

Connect to all validators of the current session so block authors are included too.

Technically we don't need to include block authors as they should be reachable by in_peers and out_peers, but this ensures they're only one hop away.

We don't want to set it to reserved-only as full nodes should still have access to the information gossiped

Technically we don't need to include block authors as they should be reachable by in_peers and out_peers, but this ensures they're only one hop away.

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.

Copying my DM from Element:

here's my understanding of what we want

a non-degenerate graph of all validators with a clique of parachain validators, although the latter part is optional

for disputes we want parachain validators of some previous sessions

We could make e.g. statement distribution and disputes use a separate peerset each and set reserved validation to only parachain validators.
But for now, using authorities is fine I think.

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

…ators * master: statement-distribution: do not use OurViewChange (#2790) Better timeout values now that we are going to be connected to all nodes. (#2778) proper executor/block type for benchmarks and try-runtime (#2771) Fix future-polling loop in availability and add a better early-exit (#2779)

rphmeier · 2021-04-01T16:09:03Z

node/network/gossip-support/src/lib.rs

-				let validators = determine_relevant_validators(ctx, relay_parent, new_session).await?;
-				tracing::debug!(target: LOG_TARGET, num = ?validators.len(), "Issuing a connection request");
+				let authorities = determine_relevant_authorities(client.clone(), relay_parent).await?;
+				ensure_i_am_an_authority(keystore, &authorities).await?;


cool, so block authors pass this check too. Seems fine to me

* gossip: do not issue a connection request if we are not a validator * guide updates * use all relevant authorities when issuing a request * use AuthorityDiscoveryApi instead * update comments to the status quo

ordian added 2 commits March 31, 2021 16:11

gossip: do not issue a connection request if we are not a validator

60d7578

guide updates

2c21070

ordian added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 31, 2021

ordian requested a review from eskimor March 31, 2021 14:42

eskimor approved these changes Mar 31, 2021

View reviewed changes

eskimor mentioned this pull request Mar 31, 2021

Validators should connect to non validator nodes on the validation peer set #2787

Open

use all relevant authorities when issuing a request

c139fbe

ordian requested a review from rphmeier March 31, 2021 20:03

ordian added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. label Mar 31, 2021

rphmeier changed the title ~~gossip: do no try to connect if we are no validators~~ gossip: do not try to connect if we are no validators Mar 31, 2021

rphmeier reviewed Mar 31, 2021

View reviewed changes

primitives/src/v1.rs Outdated Show resolved Hide resolved

rphmeier reviewed Mar 31, 2021

View reviewed changes

ordian added 3 commits April 1, 2021 00:11

use AuthorityDiscoveryApi instead

73534e4

update comments to the status quo

0b5025b

rphmeier changed the title ~~gossip: do not try to connect if we are no validators~~ gossip: do not try to connect if we are not validators Apr 1, 2021

rphmeier reviewed Apr 1, 2021

View reviewed changes

rphmeier merged commit 05cd0d2 into master Apr 1, 2021

rphmeier deleted the ao-do-no-try-to-connect-if-we-are-no-validators branch April 1, 2021 16:11

ordian mentioned this pull request Apr 1, 2021

gossip: move authorities request to runtime api subsystem #2798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gossip: do not try to connect if we are not validators #2786

gossip: do not try to connect if we are not validators #2786

ordian commented Mar 31, 2021

rphmeier commented Mar 31, 2021

ordian commented Mar 31, 2021

ordian commented Mar 31, 2021 •

edited

Loading

rphmeier Mar 31, 2021

ordian Mar 31, 2021

rphmeier Mar 31, 2021

ordian Mar 31, 2021 •

edited

Loading

rphmeier Mar 31, 2021 •

edited

Loading

eskimor Apr 1, 2021

ordian Apr 1, 2021 •

edited

Loading

rphmeier Apr 1, 2021 •

edited

Loading

gossip: do not try to connect if we are not validators #2786

gossip: do not try to connect if we are not validators #2786

Conversation

ordian commented Mar 31, 2021

rphmeier commented Mar 31, 2021

ordian commented Mar 31, 2021

ordian commented Mar 31, 2021 • edited Loading

rphmeier Mar 31, 2021

Choose a reason for hiding this comment

ordian Mar 31, 2021

Choose a reason for hiding this comment

rphmeier Mar 31, 2021

Choose a reason for hiding this comment

ordian Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

rphmeier Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

eskimor Apr 1, 2021

Choose a reason for hiding this comment

ordian Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

rphmeier Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

ordian commented Mar 31, 2021 •

edited

Loading

ordian Mar 31, 2021 •

edited

Loading

rphmeier Mar 31, 2021 •

edited

Loading

ordian Apr 1, 2021 •

edited

Loading

rphmeier Apr 1, 2021 •

edited

Loading