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

fix: reconnect should trigger topology on connect if protocol stored #54

Conversation

vasco-santos
Copy link
Member

Considering a scenario where Peer A and B start, connect to each other using a multicodec topology. Then, Peer B restarts, it is still not connected to A. It will run the update peers: https://github.com/libp2p/js-libp2p-interfaces/blob/master/src/topology/multicodec-topology.js#L68 but it does not have a connection to A yet, so it will not call onConnect. After this, identify will run, but the protocols will not change and the protocol change handler will not be called.

Fixes libp2p/js-libp2p#658

@vasco-santos vasco-santos force-pushed the fix/reconnect-should-trigger-topology-on-connect-if-protocol-stored branch from f2ab2cd to bcc380e Compare July 3, 2020 12:57
@vasco-santos vasco-santos marked this pull request as ready for review July 3, 2020 13:08
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

LGTM just a minor nit

return
}

if (this.multicodecs.filter(multicodec => protocols.includes(multicodec)).length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd change this to a find, we don't need to go through all the multicodecs if we find one.

@jacobheun jacobheun merged commit e10a154 into master Jul 3, 2020
@jacobheun jacobheun deleted the fix/reconnect-should-trigger-topology-on-connect-if-protocol-stored branch July 3, 2020 13:48
@jacobheun
Copy link
Contributor

We may want to backport this to 0.2.x for those who still need to upgrade from the peerinfo changes.

@vasco-santos
Copy link
Member Author

On it

@vasco-santos
Copy link
Member Author

It is not as easy as this one. Registrar does not have access to the connectionManager events, as in [email protected] we have the connect events in libp2p: https://github.com/libp2p/js-libp2p/blob/0.27.x/src/registrar.js

I need to add libp2p to registrar in 0.27.x branch and use it here in 0.2.x. Is something we should do?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection drop causes gossipsub to stop working
2 participants