refactor: do not use identify protocol, and actually use the nodes from the config for peer exchange #5212
Conversation
Jenkins BuildsClick to see older builds (20)
|
richard-ramos
force-pushed
the
identify-peer-exchange
branch
from
1870f00
to
7da01c4
Compare
wakuv2/waku.go
Outdated
|
|
||
| // Attempt to connect to the peers. | ||
| // Peers will be added to the libp2p peer store thanks to identify | ||
| go w.connect(w.ctx, discoveredNode.PeerInfo) |
There was a problem hiding this comment.
Nice, This looks like the changes i was doing while integrating new Filter-mgmt into status-go as test keeps failing.
Thanks for this.
|
|
||
| // Attempt to connect to the peers. | ||
| // Peers will be added to the libp2p peer store thanks to identify | ||
| go w.connect(discoveredNode.PeerInfo, wps.DNSDiscovery) |
There was a problem hiding this comment.
It should be added to peer store already with discv5 right? wondering why make such connection in peer exchange loop.
There was a problem hiding this comment.
The reason why I'm trying to connect again is because DNS Discovery might have failed for one of the enrtrees, so what this code does is attempt to connect to all nodes, but assuming the nodes are already connected, it should be fine, since no new connection will be made.
But I will improve this conde by checking if the peer is already connected to skip this step.
There was a problem hiding this comment.
It shouldn't matter, because invoking connect on already connected peer doesn't do anything.
It only does a dial if there is no connection to that peer. Reference libp2p code.
https://pkg.go.dev/github.com/libp2p/go-libp2p@v0.32.2/core/host#Host.Connect
There was a problem hiding this comment.
It should be added to peer store already with discv5 right? wondering why make such connection in peer exchange loop.
In case of lightClient discv5 won't be there, so these peers that are discovered using dnsDiscovery should be added to and connected explicitly.
wakuv2/waku.go
Outdated
| // The peer connector in go-waku uses Connect, so it will execute identify as part of its | ||
| connectedness := w.node.Host().Network().Connectedness(peerInfo.ID) | ||
| if connectedness == network.NotConnected || connectedness == network.CanConnect { | ||
| w.node.AddDiscoveredPeer(peerInfo.ID, peerInfo.Addrs, origin, nil, true) |
There was a problem hiding this comment.
Wondering if pubsubTopics list should not be empty rather we should use defaultShardedPubsubTopic here.
Because if the node is lightClient, then pubsubTopic list won't be populated for the peers in peerstore and will lead to an issue in peer selection.
WDYT @richard-ramos
There was a problem hiding this comment.
Makes sense. I'll introduce that change. Nodes for the network should still support the default shard pubsub topic so it makes sense to use it here
richard-ramos
force-pushed
the
identify-peer-exchange
branch
from
7cea1b6
to
0328d23
Compare
| } | ||
| } | ||
| func (w *Waku) connect(peerInfo peer.AddrInfo, origin wps.Origin) { | ||
| // Connection will be prunned eventually by the connection manager if needed |
There was a problem hiding this comment.
One doubt i have is, we have keepAlive check going for all peers. If so, would there by any pruning happening by connection manager?
And peers that get connected here, would never be disconnected right.
There was a problem hiding this comment.
Hm. I do not know tbh. I'll look into what criteria is used by the connection manager to prune connections. I do remember reading that once it reaches the high watermark it will prune nodes until it reaches the low watermark. I wonder if doing a ping affects the criteria somehow?
|
@richard-ramos looks like conventional commit check is failing which is causing tests to not get executed. Looks like |
uff, i always forget this. Sadly, in status-go, there's nothing conventional about the format used for conventional commits ;) |
…rom the config for peer exchange
richard-ramos
force-pushed
the
identify-peer-exchange
branch
from
0328d23
to
31104b1
Compare
…rom the config for peer exchange (#5212)
Connectmethod from go-libp2p triggers the identify protocol anyway