Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update networking section #333

Merged
merged 65 commits into from
Feb 4, 2021
Merged

Update networking section #333

merged 65 commits into from
Feb 4, 2021

Conversation

lamafab
Copy link
Contributor

@lamafab lamafab commented Dec 15, 2020

Fixes #234.

Things to consider:

  • The Kademlia/discovery process is quite thin. Based on a conversation with @infinity0, we should just refer to the libp2p website and spec the details of any questions the other implementers come up with.
  • Currently, we just refer to external specs regarding libp2p, noise, mplex/substreams. Unless we try to reimplement this from scratch, we probably need to rely on the feedback of implementers for what must be expanded.

Copy link
Collaborator

@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.

Grandpa and authority-discovery are missing, but I guess it's intentional for now

host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
Copy link
Contributor

@drskalman drskalman left a comment

Choose a reason for hiding this comment

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

Reviewed Appendix E. Comments are in a0c8160

host-spec/c06-consensus.tm Outdated Show resolved Hide resolved
@drskalman
Copy link
Contributor

Appendix e reviewed by @drskalman. After @lamafab goes over the changes, @FlorianFranzen will review the result again.

@FlorianFranzen FlorianFranzen marked this pull request as ready for review February 2, 2021 12:57
Copy link
Contributor

@FlorianFranzen FlorianFranzen left a comment

Choose a reason for hiding this comment

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

Very nice work on consolidating all these different sources and getting rid of all these outdated chapters and appendices.

I would just like to ask you to make it a bit more clear how the protocol identifiers in the sections "Connection establishment" and "Protocols and Substreams" are related, including defining what "After the protocol is negotiated" means and which substreams uses which type of substream protocols.

It might also be useful to mention that protobuffer encodes each fields as a key value pair using the provided id, etc, but it is hardly a must. People can always read the official documentation.

host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
@lamafab
Copy link
Contributor Author

lamafab commented Feb 3, 2021

@FlorianFranzen The "protocol identifiers" section was removed from the spec, since it's not directly relevant. This spec is exclusively about Polkadot and those identifiers are essentially just prefixes on substreams. I did add a note in the "Protocols and Substreams" section about it, however.

Each substream now mentions the substream type (Request-Response or Notification substream).

image

Copy link
Contributor

@FlorianFranzen FlorianFranzen left a comment

Choose a reason for hiding this comment

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

Great. Thank you for getting these last things fixed. LGTM!

Copy link
Contributor

@FlorianFranzen FlorianFranzen left a comment

Choose a reason for hiding this comment

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

Very nice. I am sure that was not fun to rebase. Just a few smaller things left to fix.

host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/c04-networking.tm Outdated Show resolved Hide resolved
host-spec/ae-hostapi.tm Outdated Show resolved Hide resolved
host-spec/ae-hostapi.tm Show resolved Hide resolved
host-spec/ae-hostapi.tm Outdated Show resolved Hide resolved
@lamafab lamafab merged commit 40f2117 into master Feb 4, 2021
@lamafab lamafab deleted the update-networking branch February 4, 2021 17:32
@FlorianFranzen FlorianFranzen changed the title [Early Review] Update networking section Update networking section Feb 5, 2021
FlorianFranzen pushed a commit that referenced this pull request Apr 12, 2021
[Early Review] Update networking section
FlorianFranzen pushed a commit that referenced this pull request Apr 12, 2021
[Early Review] Update networking section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Networking Specification
4 participants