Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Network Bridge Refactoring #1535

Merged
12 commits merged into from
Aug 5, 2020
Merged

Network Bridge Refactoring #1535

12 commits merged into from
Aug 5, 2020

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Aug 4, 2020

This PR makes a few major changes to the structure of our networking subsystems and the network bridge subsystem that interacts with the lower-level substrate-network crate.

The largest change is reducing the amount of abstraction over message types. The picture of how to do protocol upgrades via the network bridge was quite fuzzy before because each subsystem was effectively its own subprotocol that accepted bytes and sent bytes out.

My proposal is that we don't actually need abstraction over the message types of the network at this level, and that the only purpose the network bridge should serve is abstracting over how exactly those messages are sent and received.

So this PR explicitly defines the message types sent and received by the network in v1 and forwards them to subsystems as concrete types. This also reduces the amount of places where we'd have to decode bytes from the network and allows our network subsystems to deal in concrete types.

The other major change this PR makes is the elaboration on multiple peer-sets in the network bridge. This change was first introduced in #1452 and I've expounded on it further here.

@rphmeier rphmeier 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 4, 2020
@rphmeier rphmeier marked this pull request as ready for review August 4, 2020 14:26
@github-actions github-actions bot 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 Aug 4, 2020
Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 It makes more sense to me to put everything related to networking (including the messages themselves) in a single place.

roadmap/implementers-guide/src/SUMMARY.md Outdated Show resolved Hide resolved
rphmeier and others added 3 commits August 4, 2020 21:44
roadmap/implementers-guide/src/types/network.md Outdated Show resolved Hide resolved

```rust
type RequestId = u64;
type ProtocolVersion = u32;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is not used anywhere currently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, because we don't have more than one protocol version yet.

Co-authored-by: Sergei Shulepov <sergei@parity.io>
@rphmeier
Copy link
Contributor Author

rphmeier commented Aug 5, 2020

bot merge

@ghost
Copy link

ghost commented Aug 5, 2020

Waiting for commit status.

```rust
enum AvailabilityDistributionV1Message {
/// An erasure chunk for a given candidate hash.
Chunk(Hash, ErasureChunk),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ghost ghost merged commit 125efa9 into master Aug 5, 2020
@ghost ghost deleted the rh-network-bridge-refactoring branch August 5, 2020 09:04
@paritytech paritytech deleted a comment Aug 5, 2020
ordian added a commit that referenced this pull request Aug 5, 2020
* master:
  Network Bridge Refactoring (#1535)
  Use DNS hostnames for westend bootnodes (#1528)
  Companion PR: add weightinfo for democracy (#1522)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants