Collator protocol subsystem #1659

montekki · 2020-08-30T20:34:29Z

No description provided.

coriolinus

Lots of little things; should be approvable with a few touchups.

node/network/collator-protocol/src/collator_side.rs

coriolinus · 2020-09-07T12:37:07Z

node/network/collator-protocol/src/validator_side.rs

+		// TODO: This is tricky. If our chain has moved on, we have already canceled
+		// the relevant request and removed it from the map; so and we are not expecting
+		// this reply although technically it is not a malicious behaviur.
+		modify_reputation(ctx, origin, COST_UNEXPECTED_MESSAGE).await?;


The current behavior seems plausible to me. Even so, should remove the TODO or link an issue for future discussion of what the behavior should be.

yep, mostly TODOs exist in the PR at places I have no idea what to do about, so

One possible behavior would be to keep around requests for one block and then drop them. If a request arrives from an old block we know it is expired but don't need to report as the response is genuine.

However I can see the current jobs system making that hard. @coriolinus This is the kind of stuff I was referring to when I mentioned that the jobs system is kind of fragile. We should have a notion of transitions (parent -> child) and global data in the subsystem library.

edit: oh, this isn't using the jobs framework. Then the solution I mentioned could actually be viable.

@rphmeier agree that it would be handy to have parent relation information in the active set update broadcasts, so it's possible to do things like "keep information for N blocks". However, I think that's a feature of its own which should probably be moved to its own issue so as not to delay this PR.

node/network/collator-protocol/src/validator_side.rs

node/network/collator-protocol/src/collator_side.rs

Co-authored-by: Peter Goodspeed-Niklaus <[email protected]>

node/subsystem/src/messages.rs

rphmeier · 2020-09-07T16:45:00Z

node/network/collator-protocol/src/lib.rs

+
+impl CollatorProtocolSubsystem {
+	/// Instantiate the collator protocol side of this subsystem
+	pub fn new_collator_side(id: CollatorId) -> Self {


Is there a way to avoid making this choice at startup? It might introduce a circular dependency for Cumulus

You mean CollatorId? That's necessary when collations are advertised isn't it?

I mean the choice of which side of the protocol to engage in.

rphmeier · 2020-09-07T20:43:03Z

node/network/collator-protocol/src/validator_side.rs

+	/// For each relay parent and para id we may be connected to a number
+	/// of collators each of those may have advertised a different collation.
+	/// So we group such cases here.
+	requested_collations: HashMap<(Hash, ParaId, PeerId), RequestId>,


Also worth noting that we don't need to fetch any of the collations by collators.

node/network/collator-protocol/src/validator_side.rs

rphmeier · 2020-09-07T21:18:44Z

node/network/collator-protocol/src/validator_side.rs

+				modify_reputation(ctx, peer_id.clone(), COST_REQUEST_TIMEDOUT).await?;
+
+				// the callee will check if the parent is still in view, so do no checks here.
+				request_collation(


Why does this immediately request the collation again?

If the request times out, we need to note the collator as being unreliable and reduce its priority relative to other collators. And then make another request - repeat until we get a response or the chain has moved on.

Oh, yeah, that was coming from the perspective of there being no CandidateSelection subsystem. If there is CandidateSelection, it should be CandidateSelection that decides to make another request. It doesn't really make sense to request from the collator who just timed out over and over again - the implication of the line quoted from the guide is that we should make another request of another collator.

node/network/collator-protocol/src/collator_side.rs

rphmeier

The code is really orderly.

There are a few stray TODOs and questions that need to be resolved and I don't agree with the change of RequestCollation to RequestCollations.

coriolinus

Looks good to me, pending resolution of the final TODO.

coriolinus · 2020-09-09T15:29:07Z

node/network/collator-protocol/src/validator_side.rs

+		// TODO: This is tricky. If our chain has moved on, we have already canceled
+		// the relevant request and removed it from the map; so and we are not expecting
+		// this reply although technically it is not a malicious behaviur.
+		modify_reputation(ctx, origin, COST_UNEXPECTED_MESSAGE).await?;


@rphmeier agree that it would be handy to have parent relation information in the active set update broadcasts, so it's possible to do things like "keep information for N blocks". However, I think that's a feature of its own which should probably be moved to its own issue so as not to delay this PR.

montekki · 2020-09-10T07:11:19Z

bot merge

ghost · 2020-09-10T07:11:24Z

Network error getting Process.json:

error decoding response body: unknown variant connected, expected one of added_to_project, assigned, closed, comment_deleted, converted_note_to_issue, convert_to_draft, demilestoned, head_ref_deleted, head_ref_force_pushed, head_ref_restored, labeled, locked, mentioned, marked_as_duplicate, merged, milestoned, moved_columns_in_project, ready_for_review, referenced, removed_from_project, renamed, reopened, review_dismissed, review_requested, review_request_removed, subscribed, transferred, unassigned, unlabeled, unlocked, unmarked_as_duplicate, unsubscribed, user_blocked at line 1 column 14571

montekki · 2020-09-10T13:30:32Z

bot merge

ghost · 2020-09-10T13:30:37Z

Network error getting Process.json:

error decoding response body: unknown variant connected, expected one of added_to_project, assigned, closed, comment_deleted, converted_note_to_issue, convert_to_draft, demilestoned, head_ref_deleted, head_ref_force_pushed, head_ref_restored, labeled, locked, mentioned, marked_as_duplicate, merged, milestoned, moved_columns_in_project, ready_for_review, referenced, removed_from_project, renamed, reopened, review_dismissed, review_requested, review_request_removed, subscribed, transferred, unassigned, unlabeled, unlocked, unmarked_as_duplicate, unsubscribed, user_blocked at line 1 column 14571

* 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 <[email protected]> * 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 <[email protected]>

* 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)

* stupid, but it compiles * redo * cleanup * add ValidatorDiscovery to msgs * sketch network bridge code * ConnectToAuthorities instead of validators * more stuff * cleanup * more stuff * complete ConnectToAuthoritiesState * Update node/network/bridge/src/lib.rs Co-authored-by: Peter Goodspeed-Niklaus <[email protected]> * Collator protocol subsystem (#1659) * 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 <[email protected]> * 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 <[email protected]> * handle multiple in-flight connection requests * handle cancelled requests * Update node/core/runtime-api/src/lib.rs Co-authored-by: Bernhard Schuster <[email protected]> * redo it again * more stuff * redo it again * update comments * workaround Future is not Send * fix trailing spaces * clarify comments * bridge: fix compilation in tests * update more comments * small fixes * port collator protocol to new validator discovery api * collator tests compile * collator tests pass * do not revoke a request when the stream receiver is closed * make revoking opt-in * fix is_fulfilled * handle request revokation in collator * tests * wait for validator connections asyncronously * fix compilation * relabel my todos * apply Fedor's patch * resolve reconnection TODO * resolve revoking TODO * resolve channel capacity TODO * resolve peer cloning TODO * resolve peer disconnected TODO * resolve PeerSet TODO * wip tests * more tests * resolve Arc TODO * rename pending to non_revoked * one more test * extract utility function into util crate * fix compilation in tests * Apply suggestions from code review Co-authored-by: Fedor Sakharov <[email protected]> * revert pin_project removal * fix while let loop * Revert "revert pin_project removal" This reverts commit ae7f529. * fix compilation * Update node/subsystem/src/messages.rs * docs on pub items * guide updates * remove a TODO * small guide update * fix a typo * link to the issue * validator discovery: on_request docs Co-authored-by: Peter Goodspeed-Niklaus <[email protected]> Co-authored-by: Fedor Sakharov <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]>

montekki added 3 commits August 20, 2020 18:38

WIP

a12b1ee

Merge branch 'master' into fs-collator-protocol-subsystem

b97ab57

The initial implementation of the collator side.

b6c5c2c

montekki 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 Aug 30, 2020

montekki added 5 commits August 31, 2020 17:51

Improve comments

a2255d6

Merge branch 'master' into fs-collator-protocol-subsystem

2e9d3c4

Multiple collation requests

26ff153

Add more tests and comments to validator side

00456c0

Add comments, remove dead code

d463407

montekki marked this pull request as ready for review September 3, 2020 18:42

montekki requested a review from rphmeier September 3, 2020 18:43

montekki added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 3, 2020

rphmeier requested a review from coriolinus September 4, 2020 20:47

coriolinus reviewed Sep 7, 2020

View reviewed changes

Apply suggestions from code review

eda8d7e

Co-authored-by: Peter Goodspeed-Niklaus <[email protected]>

rphmeier reviewed Sep 7, 2020

View reviewed changes

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

rphmeier reviewed Sep 7, 2020

View reviewed changes

node/network/collator-protocol/src/validator_side.rs Outdated Show resolved Hide resolved

rphmeier reviewed Sep 7, 2020

View reviewed changes

node/network/collator-protocol/src/validator_side.rs Outdated Show resolved Hide resolved

rphmeier reviewed Sep 7, 2020

View reviewed changes

node/network/collator-protocol/src/collator_side.rs Outdated Show resolved Hide resolved

rphmeier reviewed Sep 7, 2020

View reviewed changes

Fix build after suggested changes

0b6c572

coriolinus linked an issue Sep 8, 2020 that may be closed by this pull request

Implement Collator Protocol subsystem #1462

Closed

montekki added 2 commits September 8, 2020 16:20

Also connect to the next validator group

b58d785

Remove a Future impl and move TimeoutExt to util

232db34

montekki added 5 commits September 8, 2020 17:01

Minor nits

bbd1c6a

Fix build

3ebad8a

Merge branch 'master' into fs-collator-protocol-subsystem

a43e7a0

Change FetchCollations back to FetchCollation

36745f6

Merge branch 'master' into fs-collator-protocol-subsystem

bad21c9

coriolinus approved these changes Sep 9, 2020

View reviewed changes

Try this

b1c5c66

montekki mentioned this pull request Sep 10, 2020

Collator protocol follow ups. #1694

Closed

2 tasks

montekki added 2 commits September 10, 2020 09:31

Final fixes

0c48ed6

Fix build

3be73a1

montekki mentioned this pull request Sep 10, 2020

'bot merge' errors paritytech/parity-processbot#219

Closed

montekki merged commit b32dcc4 into paritytech:master Sep 10, 2020

montekki mentioned this pull request Sep 21, 2020

Collator protocol followup #1741

Merged

Collator protocol subsystem #1659

Collator protocol subsystem #1659

Uh oh!

Conversation

montekki commented Aug 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coriolinus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rphmeier left a comment

Choose a reason for hiding this comment

Uh oh!

coriolinus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

montekki commented Sep 10, 2020

Uh oh!

ghost commented Sep 10, 2020

Uh oh!

montekki commented Sep 10, 2020

Uh oh!

ghost commented Sep 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

montekki commented Aug 30, 2020 •

edited

Loading

rphmeier Sep 7, 2020 •

edited

Loading