NetworkBridge: validator (authorities) discovery api #1699

ordian · 2020-09-10T14:15:14Z

Part of #1461 (only current session for now).

My suggestion is to limit NetworkBridge to AuthorityIds only and to not track the ValidatorId -> AuthorityId mapping (already implemented as runtime API call validator_discovery), and ask a caller to do that.
This logic could be extracted somewhere to be reused by different subsystems. This separates concerns better IMHO especially when dealing with multiple sessions.

Unresolved:

Would it be better to return a Stream instead of a Vec so that we can wait for the first connected validator?
Probably fine as is: NetworkBridge: validator (authorities) discovery api #1699 (comment)
Alternatively, pass a timeout and return whatever we've got not to wait for all validators to be connected?
NetworkBridge: validator (authorities) discovery api #1699 (comment)
What to do if a peer we are waiting for was connected but then disconnected. Should we add it back to pending list or just return a list w/o this peer? Currently the latter is implemented.

Todo:

Update collation protocol to use ConnectToAuthorities
Handle multiple in-flight requests
API for revoking discovery requests
Tests

node/network/bridge/src/lib.rs

node/subsystem/src/messages.rs

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

rphmeier · 2020-09-10T14:42:19Z

runtime/parachains/src/runtime_api_impl/v1.rs

+where
+	T: initializer::Trait + pallet_authority_discovery::Trait,
+{
+	// FIXME: the mapping might be invalid if a session change happens in between the calls


I'm adding a SessionInfo module that will alleviate this in #1691

* WIP * The initial implementation of the collator side. * Improve comments * Multiple collation requests * Add more tests and comments to validator side * Add comments, remove dead code * Apply suggestions from code review Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com> * Fix build after suggested changes * Also connect to the next validator group * Remove a Future impl and move TimeoutExt to util * Minor nits * Fix build * Change FetchCollations back to FetchCollation * Try this * Final fixes * Fix build Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

* master: Allow the watermark to always land on the relay parent (#1689) Limit the maximum size of a downward message (#1690) Add deb and RPM repository config and documentation (#1676) Collator protocol subsystem (#1659)

node/core/runtime-api/src/lib.rs

node/network/bridge/src/lib.rs

ordian · 2020-09-11T08:56:12Z

I implemented multiple in-flight requests in b80e050, but handling leakage is tricky. If we have another message for revoking, since validator sets of different requests may overlap (or is it not true?), what happens is one subsystem revokes an id another is waiting on?

Another unresolved question is what to do if a peer we are waiting for was connected but then disconnected. Should we add it back to pending list or just return a list w/o this peer? Currently the latter is implemented.

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

node/network/bridge/src/lib.rs

node/network/bridge/src/validator_discovery.rs

rphmeier · 2020-09-28T15:08:08Z

node/network/bridge/src/validator_discovery.rs

+use polkadot_node_network_protocol::PeerId;
+use polkadot_primitives::v1::{AuthorityDiscoveryId, Block, Hash};
+
+const PRIORITY_GROUP: &'static str = "parachain_validators";


Should each protocol (collation, validation) have its own priority group?

that's a good question, I'm not sure about this.
From the NetworkBridge perspective there is no difference. Or rather, what would be the benefit of separating the two?

node/subsystem-util/src/validator_discovery.rs

rphmeier · 2020-09-28T15:23:46Z

node/subsystem/src/messages.rs

@@ -389,6 +403,11 @@ pub enum RuntimeApiRequest {
 	/// Get all events concerning candidates (backing, inclusion, time-out) in the parent of
 	/// the block in whose state this request is executed.
 	CandidateEvents(RuntimeApiSender<Vec<CandidateEvent>>),
+	/// Get the `AuthorityDiscoveryId`s corresponding to the given `ValidatorId`s.
+	/// Currently this request is limited to validators in the current session. 


I see, then let's ignore the session index elsewhere for now as is currently done.

Yeah, what I'm worried about though is breaking changes. It's not clear how to make the API future-proof though.

rphmeier

General approach looks good. Are there associated guide changes?

primitives/src/v1.rs

* master: Add the XCM primitives crate. (#1760) Register listeners in statement distribution (#1759)

node/network/bridge/src/validator_discovery.rs

ordian · 2020-09-29T13:31:16Z

Not sure if we want to merge it now or wait till 0.8.25 branches out.
Also does it need burnin?

rphmeier · 2020-09-29T13:33:55Z

I don't think this needs any burn-in. I'm not too worried about breaking changes before any point where we deploy this on Kusama or Polkadot.

ordian · 2020-09-30T09:24:03Z

To clarify my worry:
if we merge the PR now, it will get deployed on Kusama and Polkadot in 0.8.25 (if I understand correctly)
this PR introduces a runtime API call validator_discovery that doesn't take SessionIndex into account, so it will be changed sometime in the future anyway
if we don't merge it now, there is a slight chance that we will fix the API before the next release.

rphmeier · 2020-10-01T23:05:08Z

if we merge the PR now, it will get deployed on Kusama and Polkadot in 0.8.25 (if I understand correctly)
this PR introduces a runtime API call validator_discovery that doesn't take SessionIndex into account, so it will be changed sometime in the future anyway

There is a dummy implementation of the v1 runtime APIs including this new set of calls on Polkadot and Kusama. I figure that's OK as long as the node-side is resilient to runtime API errors, so it can author blocks even if a runtime API call fails. Runtime API call failure is really the only type of failure we have to worry about, as long as the dummy API is OK for authoring under the overseer. It would happen because we'd update the runtime API definition to be different than what the runtime actually exposes.

bkchr · 2020-10-06T07:39:29Z

node/network/bridge/src/validator_discovery.rs

+
+	// this method takes `network_service` and `authority_discovery_service` by value
+	// and returns them as a workaround for the Future: Send requirement imposed by async fn impl.
+	pub async fn on_request(


Some docs what this is doing?

* master: Make collation an optional return (#1787) XCM: Land xcm-handler and xcm-executor (#1771) v0.8.25 (#1785) add two node local net script (#1781) Adjust max nominators down to 128 (from 256) (#1782) Companion for substrate/pull/7215 (#1768) Remove Stale Upgrades (#1780) Update Polkadot Weights for Substrate 2.0 (#1761) Parachains v1 registrar module. (#1559) Derive `From` for `AllMessages` and simplify `send_msg` (#1774) implement remaining subsystem metrics (#1770) Companion for paritytech/substrate#7236 (#1773) WIP: remove deprecated only/except clauses, build is now manual on PRs (#1769) Increase Westend `spec_version` (#1766) move Metrics to utils (#1765)

* master: NetworkBridge: validator (authorities) discovery api (#1699) Registrar v1 follow-ups (#1786) Make collation an optional return (#1787) XCM: Land xcm-handler and xcm-executor (#1771) v0.8.25 (#1785) add two node local net script (#1781) Adjust max nominators down to 128 (from 256) (#1782) Companion for substrate/pull/7215 (#1768) Remove Stale Upgrades (#1780) Update Polkadot Weights for Substrate 2.0 (#1761)

ordian added 10 commits September 10, 2020 14:03

stupid, but it compiles

3e9abcf

redo

e1fe858

cleanup

d09622b

add ValidatorDiscovery to msgs

9e8cfe2

sketch network bridge code

0bae9e4

ConnectToAuthorities instead of validators

4fa0236

more stuff

9f81f90

cleanup

d8d1302

more stuff

6c18572

complete ConnectToAuthoritiesState

251427d

ordian added A3-in_progress Pull request is in progress. No review needed at this stage. 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 Sep 10, 2020

rphmeier reviewed Sep 10, 2020

View reviewed changes

node/network/bridge/src/lib.rs Outdated Show resolved Hide resolved

rphmeier reviewed Sep 10, 2020

View reviewed changes

node/network/bridge/src/lib.rs Outdated Show resolved Hide resolved

rphmeier reviewed Sep 10, 2020

View reviewed changes

node/network/bridge/src/lib.rs Outdated Show resolved Hide resolved

rphmeier reviewed Sep 10, 2020

View reviewed changes

node/network/bridge/src/lib.rs Outdated Show resolved Hide resolved

coriolinus reviewed Sep 10, 2020

View reviewed changes

node/network/bridge/src/lib.rs Outdated Show resolved Hide resolved

node/subsystem/src/messages.rs Outdated Show resolved Hide resolved

Update node/network/bridge/src/lib.rs

31d7329

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

rphmeier reviewed Sep 10, 2020

View reviewed changes

montekki and others added 4 commits September 10, 2020 16:42

handle multiple in-flight connection requests

b80e050

handle cancelled requests

0e0525d

Merge branch 'master' into ao-validator-discovery-api

64dcdb1

* master: Allow the watermark to always land on the relay parent (#1689) Limit the maximum size of a downward message (#1690) Add deb and RPM repository config and documentation (#1676) Collator protocol subsystem (#1659)

drahnr reviewed Sep 11, 2020

View reviewed changes

node/core/runtime-api/src/lib.rs Outdated Show resolved Hide resolved

drahnr reviewed Sep 11, 2020

View reviewed changes

node/network/bridge/src/lib.rs Outdated Show resolved Hide resolved

drahnr reviewed Sep 11, 2020

View reviewed changes

node/network/bridge/src/lib.rs Outdated Show resolved Hide resolved

drahnr reviewed Sep 11, 2020

View reviewed changes

node/network/bridge/src/lib.rs Outdated Show resolved Hide resolved

Update node/core/runtime-api/src/lib.rs

2cf1610

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

rphmeier reviewed Sep 28, 2020

View reviewed changes

node/network/bridge/src/lib.rs Show resolved Hide resolved

rphmeier reviewed Sep 28, 2020

View reviewed changes

node/network/bridge/src/validator_discovery.rs Show resolved Hide resolved

rphmeier reviewed Sep 28, 2020

View reviewed changes

node/subsystem-util/src/validator_discovery.rs Show resolved Hide resolved

rphmeier reviewed Sep 28, 2020

View reviewed changes

node/subsystem-util/src/validator_discovery.rs Show resolved Hide resolved

rphmeier reviewed Sep 28, 2020

View reviewed changes

node/subsystem-util/src/validator_discovery.rs Show resolved Hide resolved

rphmeier reviewed Sep 28, 2020

View reviewed changes

rphmeier approved these changes Sep 28, 2020

View reviewed changes

ordian commented Sep 28, 2020

View reviewed changes

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

ordian added 6 commits September 28, 2020 18:21

docs on pub items

7e67489

guide updates

30da479

Merge branch 'master' into ao-validator-discovery-api

9c5b654

* master: Add the XCM primitives crate. (#1760) Register listeners in statement distribution (#1759)

remove a TODO

0e9771c

small guide update

4cc5f3e

fix a typo

23bde8d

ordian mentioned this pull request Sep 28, 2020

NetworkBridge: use incremental priority_group updates #1763

Closed

link to the issue

88e83ed

montekki approved these changes Sep 29, 2020

View reviewed changes

node/network/bridge/src/validator_discovery.rs Show resolved Hide resolved

bkchr reviewed Oct 6, 2020

View reviewed changes

ordian added 2 commits October 6, 2020 12:41

validator discovery: on_request docs

a7b5ddc

ordian merged commit b84f3c0 into master Oct 6, 2020

ordian deleted the ao-validator-discovery-api branch October 6, 2020 11:34

rphmeier mentioned this pull request Oct 26, 2020

Implement past-session validator discovery APIs for Network Bridge #1461

Closed

ordian mentioned this pull request Sep 27, 2022

Member Request polkadot-fellows/seeding#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetworkBridge: validator (authorities) discovery api #1699

NetworkBridge: validator (authorities) discovery api #1699

ordian commented Sep 10, 2020 •

edited

Loading

rphmeier Sep 10, 2020

ordian commented Sep 11, 2020 •

edited

Loading

rphmeier Sep 28, 2020

ordian Sep 28, 2020

rphmeier Sep 28, 2020

ordian Sep 28, 2020

rphmeier left a comment

ordian commented Sep 29, 2020

rphmeier commented Sep 29, 2020

ordian commented Sep 30, 2020

rphmeier commented Oct 1, 2020

bkchr Oct 6, 2020

NetworkBridge: validator (authorities) discovery api #1699

NetworkBridge: validator (authorities) discovery api #1699

Conversation

ordian commented Sep 10, 2020 • edited Loading

rphmeier Sep 10, 2020

Choose a reason for hiding this comment

ordian commented Sep 11, 2020 • edited Loading

rphmeier Sep 28, 2020

Choose a reason for hiding this comment

ordian Sep 28, 2020

Choose a reason for hiding this comment

rphmeier Sep 28, 2020

Choose a reason for hiding this comment

ordian Sep 28, 2020

Choose a reason for hiding this comment

rphmeier left a comment

Choose a reason for hiding this comment

ordian commented Sep 29, 2020

rphmeier commented Sep 29, 2020

ordian commented Sep 30, 2020

rphmeier commented Oct 1, 2020

bkchr Oct 6, 2020

Choose a reason for hiding this comment

ordian commented Sep 10, 2020 •

edited

Loading

ordian commented Sep 11, 2020 •

edited

Loading