Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
propose: js active conn mgr #81
propose: js active conn mgr #81
Changes from 1 commit
68577df
fa120b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd distinguish between connection management as in "managing/closing existing connections" and "peer/service discovery". This proposal currently discusses both but they're pretty separate issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the misunderstanding with the proposal comes from this. You are probably right that the connection manager should be essentially responsible for "managing/closing existing connections". However, the role of
peer/service discovery
in my mind is as simple as discovering a given peer (with its announced multiaddrs) and store them in the PeerStore. This also relates on how the peer discovery interface is defined (I think this also is the case in go?).After adding to the PeerStore, another libp2p "component" should act to figure out if we should try to connect to this peer or not, taking into account the topologies needs as well as the general needs of libp2p, such as relays...
I think this is the gap here. I am seeing the connection manager to be responsible for two main things:
Proactively establishing connections is divided into two other things:
So, we essentially need to have this entity who is responsible to act as an orchestrator by leveraging Peer Discovery + Peer Store to fulfil the needs of topologies + libp2p. I see some overlap in this entity with the connection manager, such as decisions on wether we should close a connection in favour of establishing a connection with a peer that has more value for the node's needs, as well as to free connections that are not needed in the long run (like bootstrap nodes).
Libp2p topology is probably the service we are talking here. I agree that it is a simpler option to just let services be responsible for forming the connections. However, this also has some implications that should be considered if we want to create a libp2p spec out of this. On top of my mind, we will not take efficient decisions and try to leverage connections that offer us more (decide between a peer running pubsub or a peer running pubsub + part of the dapp context), a peer part of more than one topology will be counted several times and make better decisions when trimming connections.
There is the Connection Overhaul Issue libp2p/js-libp2p#744 with a lot of information on how this would work, as well as an initial draft on how each component would interact. It is a pretty extensive issue, but perhaps it is worth reading.
Let me know what you think, if we can get to a better place to have this logic than the connection manager it might be good to separate the logic between proactive connections vs trimming. But, I overall think there will be overlap on the decision making logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closest in terms of geography, ping latency or Kademlia XOR distance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kademlia distance so that we increase the probability that peers searching for our PeerId are able to find us. In Go this is done as part of the DHT refresh interval, but for JS, since we have multiple discovery mechanisms (delegate routers) this happens up a layer. It's worth noting that this behavior does now exist in JS, but reliability is questionable.
The browser doesn't benefit from this, because it won't be able to connect to those peers in most cases. This is where having an active relay (dial the peer for me), or a rendezvous style service would be beneficial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does gossipsub have peer exchange yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has, but disabled by default. Anyway, the point here is mostly regarding the initial bootstrap of the pubsub overlay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-libp2p now has a list of known-good "relays". We've found that random relays on the network just don't behave well enough to be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not binding to relays automatically yet, just have things in place if people want to enable it. However, I agree that this should probably be provided (but perhaps in the bootstrap list?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in
js-libp2p
, we have introduced the concept of Topologies. It has some thoughts from libp2p/notes#13In summary what happens is:
If a dapp creates a protocol (let's consider Slate example again), they will likely want to have a Slate topology where once a new peer in slate is running they can be notified and establish an "application overlay". Another thing to consider here would be the concept of
MetadataTopology
together withMulticodecTopology
as we should be able to create topologies per metadata as well to enable some scenarios.Note that the topology receives a
minPeer
,maxPeer
but we are not using them yet. This is related to the goal of this proposal. The idea here is to inform libp2p of the requirements of the topology in terms of numbers of needed peers to operate reliably. With this, libp2p can act to fulfil the needs of each requirements and properly manage the connections that are needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vasco-santos @aschmahmann @jacobheun can you help flesh this one out a little more (here in comments is probably fine)?
What can we assert today about the demand for solid pubsub in the browser? Do we have good use-cases that we know users are attempting to rely on today? And what signals (maybe in the form of possible use-cases they are/have been attempting to use) should we be looking for when talking to users to back up this assumption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flow here is that when I subscribe to a topic, libp2p should automatically find peers for me. In Go, this involves advertising and querying the DHT for topics I am subscribed too. In order for JS to effectively discover those peers, regardless of their implementation language, we need to advertise and discover topics on the same service (the public DHT). So the dependency here is being able to effectively query the DHT to discover and advertise our topics.
Re: the browser, pubsub is one of the more effective forms of communicating with peers browser to browser, as I can leverage indirect links to communicate, which is ideal due to the connection limits of browsers. Usage here is mostly anecdotal at this point, there was a lot of interest here during HackFS last year that Vasco and I spent quite a bit of time supporting people on. Matrix would be a strong potential use case here, and would be great to interview for gaps in the stack.
I don't think this matters for the connection manager though. The key thing here is to be able to tag/weight connections to protect valuable ones, and ensure we're trimming stale/less useful connections:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacobheun I think you are focused on the flows after we have the connections. The first step should be to have an active connection manager to replace our current
autoDial
approach.The connection manager should actively:
After the connection manager is able to actively establish important connection rather than let's connect every peer discovered unless we already have too many connections, the second part would be the trimming scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to connection gating? This sounds like it's trying to account for discovery mechanisms which would be out of scope for this. Being able to restrict who I am connecting to/is connecting to me would be in this scope though. Although it could be done as a separate piece of work. Priority here being:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related to: #81 (comment)
There was a problem hiding this comment.
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 fits here, how does this apply to connection management?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what I was trying to get from this, so I will remove it for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatives is probably not the best place for this, but as prior art/previous discussions here are some previous notes on a topology/mesh approach to connection management libp2p/notes#13. Tagging is likely simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more a question of solution design right? The issue where we have been discussing the general solution for ConnectionManager already includes reference for it: libp2p/js-libp2p#744