feat: add protocols to PeerInfo after muxing on dial #307
Conversation
ghost
assigned
| this.switch.emit('peer-mux-established', this.theirPeerInfo) | ||
|
|
||
| this._didUpgrade(null) | ||
| // TODO: this should really happen much earlier. However, currently |
There was a problem hiding this comment.
Can you open an issue here for this change?
|
One thing I noticed is that you didn't add any extra tests to |
dialFSM leverages ConnectionFSM. We could add tests to both, but it's a bit redundant. We're detecting the
The big thing I'd like to do before releasing this is to test it against a go-libp2p node to verify the interop is working properly, since we haven't used |
|
Any updates on getting the interop tests done? |
|
Did I understand correctly that the dialing machine will own the dial until go-libp2p behaves differently. Identify runs asynchronously after connection upgrade. Protocols relying on "new connection" events (DHT, Pubsub, etc.) try to open a stream for their proto immediately, thus kicking off Multistream 1.0 protocol negotiation on the stream. If the other party NACKS, the protocol ignores that peer and closes the stream. With this PR we avoid kicking a protocol into action at the cost of 1-RTT. I'm not sure it's worth it, since streams are cheap and even if we have upfront knowledge that the protocol is supported, MS 1.0 negotiation has to occur anyway (MS 2.0 does away with this). |
|
@Mikerah not yet, sorry about the delay, I've been working on some performance issues for the upcoming jsipfs release. My thought for interop was to link this branch in a new test at https://github.com/libp2p/interop. The new test being a simple I don't think I will be able to get to this until later tomorrow at the earliest. If you'd like to try adding that test in the interim that would be helpful. |
Yes. This probably should just happen asynchronously and potentially just get moved into js-libp2p-identify. The idea is that doing it at the beginning, once the ls issue is fixed in multistream-select, you avoid potential round trips needed for crypto / stream muxer negotiation. Right now, this probably isn't a problem for us, because that's fairly limited. js-ipfs for example is going to try to negotiation Since we're not really benefitting from the knowledge up front, we can make it asynchronous until either we move to multistream2, or we get additional crypto/muxers that warrant us having that knowledge up front.
It would be great if we had recommendations for implementations to accompany specs. With more languages implementing libp2p, using go as a reference implementation is likely going to scale poorly. It also has the risk of people replicating logic that perhaps we'd like to change/update, but haven't gotten to yet. |
|
Closing this in favor of #311, which includes an update to Identify to get the supported protocols. This is in line with how go functions. |
4 participants
This adds support on dial to run
lsfrommultistream-select. This will enable things like pubsub and the dht to check the protocols a peer supports before attempting any actions on that peer.Ideally the ls would occur much earlier in the dial, so that crypto and muxer selection could be decided based on the ls. However, due to multiformats/js-multistream-select#47, this has been moved to after the muxer upgrade. Once that issue is fixed, the ls should occur at the start of the connection to allow for more efficient protocol selection.
Resolves #304