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

add: fundamentals doc on connections #202

Closed
wants to merge 5 commits into from
Closed

Conversation

salmad3
Copy link
Member

@salmad3 salmad3 commented Oct 3, 2022

Context

Latest Preview

Please view the latest Fleek preview here.

@salmad3 salmad3 added the ready for review PR is ready for review label Oct 3, 2022
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Couple of comments. Thanks for looking into this.

As a general theme, I think this is too detailed and thus inconsistent with the many implementations out there. What do others think?

content/concepts/connections.md Outdated Show resolved Hide resolved
content/concepts/connections.md Outdated Show resolved Hide resolved
content/concepts/connections.md Outdated Show resolved Hide resolved
content/concepts/connections.md Outdated Show resolved Hide resolved
@salmad3 salmad3 requested a review from mxinden October 6, 2022 09:01
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the follow-ups.

I think there is a lot of confusion here. Instead of continuing on the document, I think it would make more sense to help you understand these concepts in depth first.

I don't have the capacity to do this in the next week.

In case you would like to make progress on this in the meantime, maybe someone else from the team can help out.

In case you will be at LabWeek, we can sit together and talk through these.

Comment on lines +20 to +21
- **Peer-to-Peer (P2P)**: a distributed network in which workloads are
shared between *Peers*.
Copy link
Member

Choose a reason for hiding this comment

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

Does this belong in the connections documentation or in a general glossary?

Also referencing https://docs.ipfs.io/concepts/glossary/ just in case you haven't seen it yet.

Comment on lines +49 to +52
An interface called a [switch](/concepts/stream-multiplexing/#switch/swarm)
(and sometimes swarm) implements a basic dial queue that manages an application's
dialing and listening state. This prevents multiple,
simultaneous dial requests to the same peer.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry in case I sparked confusion here before. As a data point, rust-libp2p manages a dial queue in its ConnectionPool and not in its Swarm.

I would drop these implementation specific terms here entirely.

Just my opinion. Maybe others feel strongly about it.

Comment on lines +54 to +59
### How do peers know what transports other peers support?

Each libp2p node has information about the transports it supports.
Peers can obtain this information by dialing a peer and listen
for updates on the network. More information about peers is available
on the [peers guide](/concepts/peers).
Copy link
Member

Choose a reason for hiding this comment

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

I am guessing you are referring to the identify protocol?

I would suggest dropping this to avoid any confusion.

Comment on lines +65 to +66
to the listening peer, known as the *Responder* of the connection, via the Initiator's
multiaddr over the transport. To accept connections, a libp2p application registers
Copy link
Member

Choose a reason for hiding this comment

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

I am confused. Why is the initiator's multiaddr relevant here?

Comment on lines +73 to +77
### How does the switch decide on which transport protocol to use?

The switch conducts a protocol negotiation process between two peers through an
interface known as [`multistream-select`](https://github.com/multiformats/multistream-select).
In particular, the multicodec is negotiated between two peers.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is accurate.

A node learns about a peers multiaddresses. Based on these multiaddresses the local node dials one or more addresses, i.e. decides on one or more transport protocols.


### What happens when two peers do not support the same transport?

If a Responder doesn't support a particular protocol, they may respond with "na"
Copy link
Member

Choose a reason for hiding this comment

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

I think you are mixing a lot of concepts here.

With Transport are you referring to TCP and QUIC? If so, none of these are negotiated. The local node learns whether a remote peer supports one of these transports through the remote peer's multiaddresses.

Comment on lines +99 to +102
If a peer relies on a relay node to listen to a dial request, circuit relaying
creates a peer-to-peer circuit by adding the connection path to the connection
multiaddr. Thus, a listening peer can use the same path to dial back to a peer
that dialed to it.
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing to me. A listening peer would need to know the local peer's multiaddr to dial back.

content/concepts/connections.md Outdated Show resolved Hide resolved
Comment on lines +136 to +137
Several security protocols are supported in libp2p for encryption, the two primary
ones being Noise and TLS 1.3. See the secure channel guide for more information on
Copy link
Member

Choose a reason for hiding this comment

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

Actually, these are the only ones.

Co-authored-by: Max Inden <mail@max-inden.de>
@salmad3 salmad3 mentioned this pull request Oct 19, 2022
2 tasks
@salmad3 salmad3 added change request PR requires changes. and removed ready for review PR is ready for review labels Oct 22, 2022
@p-shahi p-shahi linked an issue Oct 25, 2022 that may be closed by this pull request
@salmad3 salmad3 changed the title Add Connections concept doc add/fundamentals-doc-on-connections Nov 11, 2022
@salmad3 salmad3 marked this pull request as draft November 17, 2022 16:37
@salmad3 salmad3 changed the title add/fundamentals-doc-on-connections add: fundamentals doc on connections Nov 22, 2022
@salmad3 salmad3 mentioned this pull request Jan 3, 2023
@salmad3 salmad3 added wontfix This will not be worked on and removed change request PR requires changes. labels Jan 3, 2023
@salmad3
Copy link
Member Author

salmad3 commented Jan 3, 2023

replaced by most recent linked PR.

@salmad3 salmad3 closed this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

Update switch stream muxer doc Add doc on connections
2 participants