Skip to content

fix: don't limit peer exchange to dns-discovered peers #6941

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 22, 2025
Merged

fix: don't limit peer exchange to dns-discovered peers #6941

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 9 additions & 26 deletions messaging/waku/gowaku.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,32 +626,15 @@ func (w *Waku) runPeerExchangeLoop() {
w.logger.Debug("Peer exchange loop stopped")
return
case <-ticker.C:
w.logger.Info("Running peer exchange loop")

// We select only the nodes discovered via DNS Discovery that support peer exchange
// We assume that those peers are running peer exchange according to infra config,
// If not, the peer selection process in go-waku will filter them out anyway
w.dnsAddressCacheLock.RLock()
var peers peer.IDSlice
for _, record := range w.dnsAddressCache {
Copy link
Contributor

@chaitanyaprem chaitanyaprem Sep 21, 2025

Choose a reason for hiding this comment

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

wondering if we can keep this logic of adding peers discovered via DNS to peerStore and connect to them.
This would provide better/more peers to choose from.

Copy link
Collaborator Author

@igor-sirotin igor-sirotin Sep 21, 2025

Choose a reason for hiding this comment

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

@chaitanyaprem, wouldn't we have DNS-discovered peers already in the peerStore? Or we're not always connecting to them, only use for discovery?

So you want like no matter if we are connected to them or not, we should keep the peer exchange running with them?

Copy link
Contributor

@chaitanyaprem chaitanyaprem Sep 21, 2025

Choose a reason for hiding this comment

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

@chaitanyaprem, wouldn't we have DNS-discovered peers already in the peerStore? Or we're not always connecting to them, only use for discovery?

Since they were being connected to here, was not sure if they are being added to peerStore somewhere else. If they are being added to peerStore then this can be removed.

So you want like no matter if we are connected to them or not, we should keep the peer exchange running with them?

This would be better imo as they would be bootnodes the chance of getting high quality peers is high.

Copy link
Collaborator Author

@igor-sirotin igor-sirotin Sep 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ah, indeed. I was sure they we were connecting to them elsewhere, but couldn't find it now.
Thank you, @chaitanyaprem, I will make sure we still connect to them 👍

Copy link
Collaborator Author

@igor-sirotin igor-sirotin Sep 22, 2025

Choose a reason for hiding this comment

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

Apparently we do connect to the DNS-discovered nodes, here:

status-go/messaging/waku/gowaku.go

Lines 536 to 543 in cbbbec4

func (w *Waku) discoverAndConnectPeers() {
for _, addrString := range w.cfg.WakuNodes {
err := handlePeerAddress(addrString, w)
if err != nil {
w.logger.Warn("failed to handle peer address", zap.String("addr", addrString), zap.Error(err))
}
}
}

status-go/messaging/waku/gowaku.go

Lines 563 to 567 in cbbbec4

func handlePeerAddress(addr string, handler peerAddressHandler) error {
if strings.HasPrefix(addr, "enrtree://") {
handler.discoverAndConnect(addr)
return nil
}

And we duplicate the enrtree:// in the fleet configuration, for both static connection and Discv5 connection:

status-go/params/cluster.go

Lines 79 to 85 in f176cba

FleetStatusProd: {
ClusterID: 16,
WakuNodes: []string{
"enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.prod.status.nodes.status.im",
},
DiscV5BootstrapNodes: []string{
"enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.prod.status.nodes.status.im",

So. All should be good. We have the fleet enrtree:// and always connect to all retrieved nodes. So they will not only appear in dnsAddressCache, but also in the peer cache itself. So we will always run peer exchange with them.

for _, discoveredNode := range record {
if len(discoveredNode.PeerInfo.Addrs) == 0 {
continue
}
// Attempt to connect to the peers.
// Peers will be added to the libp2p peer store thanks to identify
go w.connect(discoveredNode.PeerInfo, discoveredNode.ENR, wps.DNSDiscovery)
peers = append(peers, discoveredNode.PeerID)
}
}
w.dnsAddressCacheLock.RUnlock()

if len(peers) != 0 {
err := w.node.PeerExchange().Request(w.ctx, w.cfg.DiscoveryLimit, peer_exchange.WithAutomaticPeerSelection(peers...),
peer_exchange.FilterByShard(int(w.defaultShardInfo.ClusterID), int(w.defaultShardInfo.ShardIDs[0])))
if err != nil {
w.logger.Error("couldnt request peers via peer exchange", zap.Error(err))
}
w.logger.Debug("Running peer exchange loop")
err := w.node.PeerExchange().Request(
w.ctx,
w.cfg.DiscoveryLimit,
peer_exchange.WithAutomaticPeerSelection(),
peer_exchange.FilterByShard(int(w.defaultShardInfo.ClusterID), int(w.defaultShardInfo.ShardIDs[0])),
)
if err != nil {
w.logger.Error("could not request peers via peer exchange", zap.Error(err))
}
}
}
Expand Down
Loading