ChainSafe / js-libp2p-gossipsub

TypeScript implementation of Gossipsub
Apache License 2.0
145 stars 43 forks source link

v03.5.0: Too many topic peers #313

Closed twoeths closed 2 years ago

twoeths commented 2 years ago

Description There are only 21 peers

Screen Shot 2022-08-04 at 12 25 13

But it showed a lot of topic peers

Screen Shot 2022-08-04 at 12 26 08

Quick debug

Screen Shot 2022-08-04 at 12 28 28
twoeths commented 2 years ago

there are too many connected peers too

Screen Shot 2022-08-04 at 16 58 17

looks like there are some peers disconnected without calling GossipSub.removePeer() properly

Expected:

Screen Shot 2022-08-04 at 17 00 32
wemeetagain commented 2 years ago

It seems that this library relies on the registrar properly registering a peer to this protocol in order for both the onPeerDisconnected and onPeerConnected routines to be triggered. So if we get an inbound-negotiated stream with a peer and libp2p doesn't successfully get a response from the identify protocol, then the registrar won't call onPeerDisconnected for that peer. Likewise, if we connect to a peer, but don't get a response from the identify protocol, then we won't initiate a gossipsub stream with that peer (unless that peer opens a gossipsub stream).

I think for the purposes of networks that do not necessarily support the identify protocol (eg: ethereum network) we need the following:

twoeths commented 2 years ago

@wemeetagain I don't see this issue happen in the current version of lodestar, do you know how does this work earlier (pre v3.x)?

wemeetagain commented 2 years ago

old version of libp2p would trigger onDisconnect to all peers

https://github.com/libp2p/js-libp2p/blob/v0.36.2/src/registrar.js#L54 https://github.com/libp2p/js-libp2p/blob/v0.36.2/src/registrar.js#L122

wemeetagain commented 2 years ago

Current version checks the protoBook, only calls onDisconnect for protocols that are registered to the peer https://github.com/libp2p/js-libp2p/blob/master/src/registrar.ts#L152

twoeths commented 2 years ago

this issue could be the reason why memory keeps growing since the pushable inside OutboundStream stores a lot of internal buffers inside, see https://github.com/ChainSafe/js-libp2p-gossipsub/pull/314

twoeths commented 2 years ago

resolved in latest libp2p + gossipsub